)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"9c58bf482c750c4cd761e424898467b02883d81e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"1f1f4e74_6c6d888e","updated":"2021-10-26 20:22:14.000000000","message":"The change itself makes sense, and it simple enough. The -1 is because I think we need to use this opportunity to add some functional tests around rollback and port binding. We already have the LiveMigrationNeutronInteractionsTest class in nova/tests/functional/compute/test_live_migration.py, I think adding a couple of test methods there can work. Also, having functional tests to show that this new way of doing things continues to work as expected would remove that last niggle I have about this.","commit_id":"7dea22d60df3f271d4d36b48c3fa04c30240452a"}],"nova/compute/manager.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"1f6cb6736cf26be792c96e077785a051cd990b8b","unresolved":true,"context_lines":[{"line_number":9137,"context_line":"                bdms\u003dbdms)"},{"line_number":9138,"context_line":""},{"line_number":9139,"context_line":"        # The port binding profiles need to be cleaned up."},{"line_number":9140,"context_line":"        with errors_out_migration_ctxt(migration):"},{"line_number":9141,"context_line":"            try:"},{"line_number":9142,"context_line":"                # This call will delete any inactive destination host"},{"line_number":9143,"context_line":"                # port bindings."}],"source_content_type":"text/x-python","patch_set":3,"id":"8220fed1_b166b0bc","line":9140,"updated":"2021-10-26 20:26:15.000000000","message":"So for the tests, we can add 3 test methods.\n\n1. The happy path, where we mock something (TBD) to explode and trigger the rollback, but then everything goes as planned and we rollback successfully.","commit_id":"7dea22d60df3f271d4d36b48c3fa04c30240452a"},{"author":{"_account_id":19234,"name":"Alexey Stupnikov","email":"aleksey.stupnikov@gmail.com","username":"astupnikov"},"change_message_id":"31fd06983d077453b111cf74292270589627550d","unresolved":true,"context_lines":[{"line_number":9137,"context_line":"                bdms\u003dbdms)"},{"line_number":9138,"context_line":""},{"line_number":9139,"context_line":"        # The port binding profiles need to be cleaned up."},{"line_number":9140,"context_line":"        with errors_out_migration_ctxt(migration):"},{"line_number":9141,"context_line":"            try:"},{"line_number":9142,"context_line":"                # This call will delete any inactive destination host"},{"line_number":9143,"context_line":"                # port bindings."}],"source_content_type":"text/x-python","patch_set":3,"id":"74f76710_ae90b3f9","line":9140,"in_reply_to":"8220fed1_b166b0bc","updated":"2022-02-23 16:39:52.000000000","message":"Ack, will take a second look at the options available and would implement something.","commit_id":"7dea22d60df3f271d4d36b48c3fa04c30240452a"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"1f6cb6736cf26be792c96e077785a051cd990b8b","unresolved":true,"context_lines":[{"line_number":9143,"context_line":"                # port bindings."},{"line_number":9144,"context_line":"                self.network_api.setup_networks_on_host("},{"line_number":9145,"context_line":"                    context, instance, host\u003ddest, teardown\u003dTrue)"},{"line_number":9146,"context_line":"            except exception.PortBindingDeletionFailed as e:"},{"line_number":9147,"context_line":"                # Removing the inactive port bindings from the destination"},{"line_number":9148,"context_line":"                # host is not critical so just log an error but don\u0027t fail."},{"line_number":9149,"context_line":"                LOG.error("}],"source_content_type":"text/x-python","patch_set":3,"id":"00b33a52_daa3de9e","line":9146,"updated":"2021-10-26 20:26:15.000000000","message":"2. A test that mocks setup_networks_on_host() to raise PortBindingDeletionFailed, and we asser that the bindings remain on the dest, but everything else is rolled back correctly.","commit_id":"7dea22d60df3f271d4d36b48c3fa04c30240452a"},{"author":{"_account_id":19234,"name":"Alexey Stupnikov","email":"aleksey.stupnikov@gmail.com","username":"astupnikov"},"change_message_id":"31fd06983d077453b111cf74292270589627550d","unresolved":true,"context_lines":[{"line_number":9143,"context_line":"                # port bindings."},{"line_number":9144,"context_line":"                self.network_api.setup_networks_on_host("},{"line_number":9145,"context_line":"                    context, instance, host\u003ddest, teardown\u003dTrue)"},{"line_number":9146,"context_line":"            except exception.PortBindingDeletionFailed as e:"},{"line_number":9147,"context_line":"                # Removing the inactive port bindings from the destination"},{"line_number":9148,"context_line":"                # host is not critical so just log an error but don\u0027t fail."},{"line_number":9149,"context_line":"                LOG.error("}],"source_content_type":"text/x-python","patch_set":3,"id":"7cb0cbb0_e0415cfe","line":9146,"in_reply_to":"00b33a52_daa3de9e","updated":"2022-02-23 16:39:52.000000000","message":"Ack, would do.","commit_id":"7dea22d60df3f271d4d36b48c3fa04c30240452a"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"1f6cb6736cf26be792c96e077785a051cd990b8b","unresolved":true,"context_lines":[{"line_number":9151,"context_line":"                    \u0027during live migration rollback. You may need to \u0027"},{"line_number":9152,"context_line":"                    \u0027manually clean up resources in the network service. \u0027"},{"line_number":9153,"context_line":"                    \u0027Error: %s\u0027, dest, str(e))"},{"line_number":9154,"context_line":"            except Exception:"},{"line_number":9155,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":9156,"context_line":"                    LOG.exception("},{"line_number":9157,"context_line":"                        \u0027An error occurred while cleaning up networking \u0027"}],"source_content_type":"text/x-python","patch_set":3,"id":"75bec6cf_98820c2e","line":9154,"updated":"2021-10-26 20:26:15.000000000","message":"3. Something to hit this part of the code, where we don\u0027t actually complete the rollback.\n\n... in fact, is this even valid? We might want to rethink this bit of exception handling, as we should probably still rollback everything else, and leave the port bindings as they are?","commit_id":"7dea22d60df3f271d4d36b48c3fa04c30240452a"},{"author":{"_account_id":19234,"name":"Alexey Stupnikov","email":"aleksey.stupnikov@gmail.com","username":"astupnikov"},"change_message_id":"31fd06983d077453b111cf74292270589627550d","unresolved":true,"context_lines":[{"line_number":9151,"context_line":"                    \u0027during live migration rollback. You may need to \u0027"},{"line_number":9152,"context_line":"                    \u0027manually clean up resources in the network service. \u0027"},{"line_number":9153,"context_line":"                    \u0027Error: %s\u0027, dest, str(e))"},{"line_number":9154,"context_line":"            except Exception:"},{"line_number":9155,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":9156,"context_line":"                    LOG.exception("},{"line_number":9157,"context_line":"                        \u0027An error occurred while cleaning up networking \u0027"}],"source_content_type":"text/x-python","patch_set":3,"id":"276c1627_f9086f3d","line":9154,"in_reply_to":"75bec6cf_98820c2e","updated":"2022-02-23 16:39:52.000000000","message":"This makes sense if we want to be slightly more aggressive when doing cleanups: unknown failure to remove port binding is not necessary a blocker for disk cleanups on destination host. Although I am not sure if we need to revert instance\u0027s state.\n\nexcept statement itself looks relevant to me because PortBindingDeletionFailed doesn\u0027t seem to be the only possible exception to be raised. So I can move this cleanup statement after self.compute_rpcapi.rollback_live_migration_at_destination call. What do you think?","commit_id":"7dea22d60df3f271d4d36b48c3fa04c30240452a"}]}