)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"aec84b6c03723992fc0df513ef896321aa9b9584","unresolved":true,"context_lines":[{"line_number":11,"context_line":"CPU-bound (single core) instead of network-bound, slowing it down."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Operators may prefer to disable compression for raw images."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: I37c587575c32f23636b3f7e61ae9b18230b0244c"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"a65de2f3_43b8bdbb","line":14,"updated":"2021-11-15 18:45:03.000000000","message":"im conflictied if this should be tracked as a bug or a specless blueprint.\n\nwe do need to file one or the the other to track this however before we move forward with this.","commit_id":"82c2f30864457c23b90d1e51f9bc397e5904a63c"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"aec84b6c03723992fc0df513ef896321aa9b9584","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"921fc7b7_a27bcafa","updated":"2021-11-15 18:45:03.000000000","message":"this is a soft -1 i don\u0027t  think there is any harm in makeing this configuratble however we will need a specless blueprint to track this change. you already have a release note that is good and have provided documentation that is part of the config option. you might want to consider extending the install guide or other admin docs on cold migration to make note of this but i think what you have in the current versions is sufficent.\n\nthere is no upgrade impact since the defualt list is the same.\nwhile it might produce asymetic behaivor if you dont set this the same on all host i dont think it\nshould break anything so i dont see any upgrade concerns form that aspect either so i dont belive this needs a spec.","commit_id":"82c2f30864457c23b90d1e51f9bc397e5904a63c"},{"author":{"_account_id":10338,"name":"Daniel Speichert","email":"daniel@speichert.pl","username":"daniel"},"change_message_id":"311a7aae2e10a525eceb7be1107e7574204fcfc1","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c66cf71a_f04145af","in_reply_to":"921fc7b7_a27bcafa","updated":"2021-11-15 19:15:01.000000000","message":"Thanks Sean for the quick and comprehensive review.\n\nIdeally, I think it makes sense for most environments to also change the default value (or disable compression completely?). Based on my tests, in networks 10G, compression is slowing things down. Do you think it would stall the process (or require additional specs) to do so?\nI\u0027d rather have that configurable without changing the default than to get the whole thing get stuck somewhere in the pipeline if it needs further debate.","commit_id":"82c2f30864457c23b90d1e51f9bc397e5904a63c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"cebddef81aa60b4716967175ac20b47bbecf828c","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"5a51d748_f08318f0","in_reply_to":"c66cf71a_f04145af","updated":"2021-11-16 01:29:30.000000000","message":"how big where the root disk you were testing with and how compressible where they?\nis there an infleciton point in your data eg. \u003c20G no copresssion is better \u003e500G compression wins?\n\nthere are two remote file system dirver by the way\n\nthe ssh driver just adds the -C option to enable stream compression at the ssh level when establishing the connection to the remote server.\n\nhttps://github.com/openstack/nova/blob/50fdbc752a9ca9c31488140ef2997ed59d861a41/nova/virt/libvirt/volume/remotefs.py#L192-L193\n\nfor ssh it may in fact slow things down on a high speed link but the rsync driver may scale better so if we were to change the default the new default would have to work for both drivers.\n\ni actully dont think it makes sense to disbale compression by default at least not on  \u003c10G networks. 25G+ probably would be better off with compression disabled in some caes  but if you want to change the behavior to disbale compression by default then we need to discuss the upgrade impacts exctra which may require a spec.\n\nwe would certenly at least want to know how you tested, the type of data in the disk (empty, text, imcompresable video, db) and are there any hardware factors that might come into play.\n\na 10G network is a realtivily slow network link in most datacenters today, i would expect copying a 100G raw image with compression if its relitivly spares/empty to be faster with compression then without but perhaps not.\n\nfor openstack deployment that still use 1G network (they do still exist) i woudl expect compression to help but if you can provide data for use to review we can certenly discuss a default change. i would suggest makeing an default change a follow up patch that we can review an accept/reject seperatly.\n\nyour current proposal make this configurable and you can tweak this in your deployment with your installer or manually so i think you already have a good compromise of being able to expand this for your usecaes without affecting existing usage.\n\ni would keep this patch simple and potentially back portable and defer the rest to a second patch. not that i expect this to be backproted upstream nessisarly but without changing any beahivor by default that is at least an option.","commit_id":"82c2f30864457c23b90d1e51f9bc397e5904a63c"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"0d68bded9e10a836e51d944d862649aec5c562f6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"69f40a66_1117ee0b","updated":"2022-02-03 15:44:57.000000000","message":"Looks clear and simple. Please add a unit test to change. (similarly to the existing test nova.tests.unit.virt.libvirt.test_driver.LibvirtDriverTestCase.test_migrate_disk_and_power_off_swap) Once that test is done I will +2 this.","commit_id":"3660d3d0652f31196b69d66e1e7253b53ae9b0ca"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"aec84b6c03723992fc0df513ef896321aa9b9584","unresolved":true,"context_lines":[{"line_number":10831,"context_line":"                if fname \u003d\u003d \u0027disk.swap\u0027:"},{"line_number":10832,"context_line":"                    continue"},{"line_number":10833,"context_line":""},{"line_number":10834,"context_line":"                compression \u003d info[\u0027type\u0027] not in \\"},{"line_number":10835,"context_line":"                              CONF.libvirt.no_compression_image_types"},{"line_number":10836,"context_line":"                libvirt_utils.copy_image(from_path, img_path, host\u003ddest,"},{"line_number":10837,"context_line":"                                         on_execute\u003don_execute,"}],"source_content_type":"text/x-python","patch_set":1,"id":"b9cec4c4_831e45c5","line":10834,"range":{"start_line":10834,"start_character":50,"end_line":10834,"end_character":51},"updated":"2021-11-15 18:45:03.000000000","message":"nit i generally prefer if you use () instead of line continuation \"\\\" in this case but that just a minor style point.","commit_id":"82c2f30864457c23b90d1e51f9bc397e5904a63c"}],"releasenotes/notes/libvirt-add-no_compression_image_types-5f04b87dc9eb67e0.yaml":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"aec84b6c03723992fc0df513ef896321aa9b9584","unresolved":true,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    The libvirt driver now allows excluding more image types from compression"},{"line_number":5,"context_line":"    upon migrating over network using ``[libvirt]/remote_filesystem_transport``"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"51974c49_0886039f","line":2,"range":{"start_line":2,"start_character":0,"end_line":2,"end_character":8},"updated":"2021-11-15 18:45:03.000000000","message":"since you have declared this as a feature can you file a blueprint to track this and we can reivew it for approval in the next nova team meeting tomorrow.\n\ni think this likely can be approved as a specless blueprint.","commit_id":"82c2f30864457c23b90d1e51f9bc397e5904a63c"}]}