)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":33910,"name":"Tyler Stachecki","display_name":"Tyler Stachecki","email":"tstachecki@bloomberg.net","username":"tjstachecki"},"change_message_id":"ab8418864ef6f0daffcbf0af2a8bc6371feb04eb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"a4182a2c_b372a550","updated":"2022-08-08 23:22:07.000000000","message":"\u003e Patch Set 1:\n\u003e \n\u003e If you\u0027re happy improving the error message (and maybe we can figure out if we can error out earlier in the process), I\u0027m more than happy conceding that work and helping you out with reviews. I don\u0027t get much \"head down\" time lately, so jumping in and out of a review feels like a more attainable goal for me :)\n\nCertainly - I\u0027ll still w/d this PS but, I\u0027ll put it on my list of things to do and tag you on a subsequent PS!","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"866f9ec451562ebe37ad17df02f553f7649f0d13","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"66a2839b_b7346ea0","updated":"2022-08-08 21:51:28.000000000","message":"Hey, awesome that you\u0027re doing this! I have a couple of very old WIP (work-in-progress) that I was reminded of when I saw this patch come up. There\u0027s a regression test (feel absolutely free to adapt/reuse it!), and a \"fix\" patch on top.\n\nAs you can see in the review comments on https://review.opendev.org/c/openstack/nova/+/791553/1 and in our (Red Hat) internal bug report for this (https://bugzilla.redhat.com/show_bug.cgi?id\u003d1943613), the current consensus is to implement supporting changing the MTU as a new feature, and in the meantime to just continue erroring out the live migration if there\u0027s been a change in MTU, but to give a better error message to the user.\n\nWould this approach (at least the second half of that, I\u0027m not going to ask you to implement the new feature) work for you?","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":33910,"name":"Tyler Stachecki","display_name":"Tyler Stachecki","email":"tstachecki@bloomberg.net","username":"tjstachecki"},"change_message_id":"64e82755b741c671de63f9dc8dd005df45395e8d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"9a493f2a_36f9a5a6","updated":"2022-08-08 21:40:53.000000000","message":"I will add a unit test soon. Just wanted to get this on the board and run through CI quickly now.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"0e67db119a2400e70696d1b3c3ccbf2d80afb443","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"31f84159_7ccfdb59","updated":"2022-08-09 00:30:08.000000000","message":"So I was thinking some more about this, and the existing error message is actually mostly OK, what sucks is that it goes all the way to actually trying the live migration. If we were to try and catch a changed MTU earlier, I *think* the best place would be where I left my inline comments.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":33910,"name":"Tyler Stachecki","display_name":"Tyler Stachecki","email":"tstachecki@bloomberg.net","username":"tjstachecki"},"change_message_id":"c8a17ee4fe1ae6835fd23860c7053c6f4bc61d6b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"ea7c4544_dc50f2d0","updated":"2022-08-09 03:12:38.000000000","message":"Threw something up at: https://review.opendev.org/c/openstack/nova/+/852367/1/nova/virt/libvirt/vif.py\n\nThis is closed and should probably remain that way to make it clear that this is not the right approach?","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":33910,"name":"Tyler Stachecki","display_name":"Tyler Stachecki","email":"tstachecki@bloomberg.net","username":"tjstachecki"},"change_message_id":"ca3caa5c5fc80aed81a9be2c359a0d9e9b42b6b5","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"50e7d04c_553625a8","in_reply_to":"2acec5d3_388b2451","updated":"2022-08-08 23:10:07.000000000","message":"err, I mean withdraw the patch? If you\u0027re already improving the error message that\u0027s good for me.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":33910,"name":"Tyler Stachecki","display_name":"Tyler Stachecki","email":"tstachecki@bloomberg.net","username":"tjstachecki"},"change_message_id":"c514704a8b607d006c7e44b32d893d21b5d8339d","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"ffca4f9c_8531fa82","in_reply_to":"31f84159_7ccfdb59","updated":"2022-08-09 00:58:05.000000000","message":"oh, perfect! I was thinking along these same lines as well, but having the spot pinpointed makes this change easier.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"52026238f4fa6b22af629e976fa94ed5c9d3096b","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"ce19d059_ed6acf7e","in_reply_to":"531d4523_2cb496b9","updated":"2022-08-08 22:33:13.000000000","message":"The problem is that the MTU on Neutron ports (which is what Nova uses as its source of truth) should not be mutable until Nova implements proper support for changing it on the fly. If we take the approach of keeping the original MTU during a live migration, we end up with a split brain situation with Neutron saying one thing, and the libvirt XML saying another. That\u0027s not great. Since Neutron can\u0027t (or won\u0027t? I\u0027m not sure of the politics here) make the MTU immutable, our next best option is to prevent operator \"error\" and refuse to live migrate with a new MTU.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":33910,"name":"Tyler Stachecki","display_name":"Tyler Stachecki","email":"tstachecki@bloomberg.net","username":"tjstachecki"},"change_message_id":"620060ef9b047242cbe181c5f98257a33a134917","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"531d4523_2cb496b9","in_reply_to":"66a2839b_b7346ea0","updated":"2022-08-08 22:16:30.000000000","message":"hmm... I\u0027m surprised that patch was rejected. We have a legitimate use case here in that we are trying to, as you say, prevent things from exploding when the underlying network MTU changes. Retaining it is perfectly fine for now, and that\u0027s great that there\u0027s progress on allowing it to change -- we just want the live-migration to not fail in such cases in the meantime...\n\nI\u0027ve tested your patch works just as well for me. The existing comment led me to believe that if the MTU is not specified, we should not add an element to the tree.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":33910,"name":"Tyler Stachecki","display_name":"Tyler Stachecki","email":"tstachecki@bloomberg.net","username":"tjstachecki"},"change_message_id":"8c3567fcf9ba2cec56bbc58b4d0bb9a70411acae","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"2acec5d3_388b2451","in_reply_to":"ce19d059_ed6acf7e","updated":"2022-08-08 22:49:50.000000000","message":"ahh, interesting, thanks for taking the time to explain! As a Calico user, I am seeing our taps plugged at the \"correct\" MTU after live-migration... but I can see this as being an enormous problem for the rest of the community.\n\nBummer, but I\u0027ll improve the error message here, instead, then.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"}],"nova/compute/manager.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"0e67db119a2400e70696d1b3c3ccbf2d80afb443","unresolved":true,"context_lines":[{"line_number":8111,"context_line":"            self._get_compute_info(ctxt, instance.host))"},{"line_number":8112,"context_line":"        dst_compute_info \u003d obj_base.obj_to_primitive("},{"line_number":8113,"context_line":"            self._get_compute_info(ctxt, self.host))"},{"line_number":8114,"context_line":"        dest_check_data \u003d self.driver.check_can_live_migrate_destination(ctxt,"},{"line_number":8115,"context_line":"            instance, src_compute_info, dst_compute_info,"},{"line_number":8116,"context_line":"            block_migration, disk_over_commit)"},{"line_number":8117,"context_line":"        dest_check_data \u003d self._dest_can_numa_live_migrate(dest_check_data,"}],"source_content_type":"text/x-python","patch_set":1,"id":"a59d6b7f_23d1f8f6","line":8114,"updated":"2022-08-09 00:30:08.000000000","message":"When the compute manager gets the RPC call, here is where it calls the virt driver.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"}],"nova/conductor/tasks/live_migrate.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"0e67db119a2400e70696d1b3c3ccbf2d80afb443","unresolved":true,"context_lines":[{"line_number":356,"context_line":"        self._check_can_migrate_pci(self.source, destination)"},{"line_number":357,"context_line":"        try:"},{"line_number":358,"context_line":"            self.migrate_data \u003d self.compute_rpcapi.\\"},{"line_number":359,"context_line":"                check_can_live_migrate_destination(self.context, self.instance,"},{"line_number":360,"context_line":"                    destination, self.block_migration, self.disk_over_commit,"},{"line_number":361,"context_line":"                    self.migration, self.limits)"},{"line_number":362,"context_line":"        except messaging.MessagingTimeout:"}],"source_content_type":"text/x-python","patch_set":1,"id":"d489ac8d_ceb884f2","side":"PARENT","line":359,"updated":"2022-08-09 00:30:08.000000000","message":"So I think this is the first time where we call down to the destination compute, which can then query its virt driver.","commit_id":"c53ec4e48884235566962bc934cbf292ad5b67b8"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"0e67db119a2400e70696d1b3c3ccbf2d80afb443","unresolved":true,"context_lines":[{"line_number":9333,"context_line":"    def check_instance_shared_storage_cleanup(self, context, data):"},{"line_number":9334,"context_line":"        fileutils.delete_if_exists(data[\"filename\"])"},{"line_number":9335,"context_line":""},{"line_number":9336,"context_line":"    def check_can_live_migrate_destination(self, context, instance,"},{"line_number":9337,"context_line":"                                           src_compute_info, dst_compute_info,"},{"line_number":9338,"context_line":"                                           block_migration\u003dFalse,"},{"line_number":9339,"context_line":"                                           disk_over_commit\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":1,"id":"b83df1ac_1396dbf2","line":9336,"updated":"2022-08-09 00:30:08.000000000","message":"This is the first time we talk to the virt driver on the destination host, so if we were to try and catch a changed MTU, it would be in here somewhere. We can\u0027t do it in the compute manager because it\u0027s libvirt-specific.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"},{"author":{"_account_id":33910,"name":"Tyler Stachecki","display_name":"Tyler Stachecki","email":"tstachecki@bloomberg.net","username":"tjstachecki"},"change_message_id":"c514704a8b607d006c7e44b32d893d21b5d8339d","unresolved":true,"context_lines":[{"line_number":9333,"context_line":"    def check_instance_shared_storage_cleanup(self, context, data):"},{"line_number":9334,"context_line":"        fileutils.delete_if_exists(data[\"filename\"])"},{"line_number":9335,"context_line":""},{"line_number":9336,"context_line":"    def check_can_live_migrate_destination(self, context, instance,"},{"line_number":9337,"context_line":"                                           src_compute_info, dst_compute_info,"},{"line_number":9338,"context_line":"                                           block_migration\u003dFalse,"},{"line_number":9339,"context_line":"                                           disk_over_commit\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":1,"id":"467a0475_ed1677e0","line":9336,"in_reply_to":"b83df1ac_1396dbf2","updated":"2022-08-09 00:58:05.000000000","message":"Just to be clear -- we could do it no earlier than where this is called from (ref: your comment for tasks/live_migration.py)? This function effectively looks like an abstract function that is implemented by each virt driver.","commit_id":"da1f3d0bd34c9110e6fda38dfa38edb786ee1f6b"}]}