)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"df56c7602e8ee0029492c39592d3310324b03522","unresolved":false,"context_lines":[{"line_number":18,"context_line":"so we have to do it ourselves on rollback."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Change-Id: I7e17f6bb5ee553896f6021a8c5f3fe378792ee5b"},{"line_number":21,"context_line":"Closes-Bug: #1788014"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_6c5414cb","line":21,"range":{"start_line":21,"start_character":0,"end_line":21,"end_character":6},"updated":"2018-08-21 22:21:16.000000000","message":"Likely should be Related-Bug. The root cause of Sean\u0027s bug is different, but if we start post-copy and activate the dest host port bindings, and then post-copy fails, I\u0027m assuming the live migration overall will fail and then we should rollback and activate the source host port bindings.","commit_id":"6bd9643b4283676834eb43ccc76393a76f843685"}],"nova/compute/manager.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"9a26ecab7997e0c8b928544f9318cd65e9cd0f5d","unresolved":false,"context_lines":[{"line_number":6896,"context_line":"                migrate_data)"},{"line_number":6897,"context_line":""},{"line_number":6898,"context_line":"        if do_cleanup:"},{"line_number":6899,"context_line":"            self.compute_rpcapi.rollback_live_migration_at_destination("},{"line_number":6900,"context_line":"                    context, instance, dest, destroy_disks\u003ddestroy_disks,"},{"line_number":6901,"context_line":"                    migrate_data\u003dmigrate_data)"},{"line_number":6902,"context_line":"        elif utils.is_neutron():"}],"source_content_type":"text/x-python","patch_set":1,"id":"5faad753_5911b703","line":6899,"updated":"2019-09-13 15:00:53.000000000","message":"If do_cleanup is True, we call this, which hits L6968, which activates the old bindings twice. Is that supposed to happen? Seems like needless work...","commit_id":"6bd9643b4283676834eb43ccc76393a76f843685"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"55bea4f90473880dfdcf0a4a3a0aa1b9eb2ca74e","unresolved":false,"context_lines":[{"line_number":6896,"context_line":"                migrate_data)"},{"line_number":6897,"context_line":""},{"line_number":6898,"context_line":"        if do_cleanup:"},{"line_number":6899,"context_line":"            self.compute_rpcapi.rollback_live_migration_at_destination("},{"line_number":6900,"context_line":"                    context, instance, dest, destroy_disks\u003ddestroy_disks,"},{"line_number":6901,"context_line":"                    migrate_data\u003dmigrate_data)"},{"line_number":6902,"context_line":"        elif utils.is_neutron():"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_4c6c41cf","line":6899,"in_reply_to":"5faad753_5911b703","updated":"2019-11-04 20:07:48.000000000","message":"If do_cleanup is True, we won\u0027t get to L6908 which is why it\u0027s handled in both places. Or am I misunderstanding you?","commit_id":"6bd9643b4283676834eb43ccc76393a76f843685"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"e7ba6de417a1fe7035a61ec338786d7d331265bf","unresolved":false,"context_lines":[{"line_number":6899,"context_line":"            self.compute_rpcapi.rollback_live_migration_at_destination("},{"line_number":6900,"context_line":"                    context, instance, dest, destroy_disks\u003ddestroy_disks,"},{"line_number":6901,"context_line":"                    migrate_data\u003dmigrate_data)"},{"line_number":6902,"context_line":"        elif utils.is_neutron():"},{"line_number":6903,"context_line":"            # The port binding profiles need to be cleaned up."},{"line_number":6904,"context_line":"            with errors_out_migration_ctxt(migration):"},{"line_number":6905,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_ccb4b138","line":6902,"updated":"2019-11-04 20:12:33.000000000","message":"Surprise, there\u0027s an \u0027else\u0027 here. Surprise! The \u0027if\u0027 block is hit if do_cleanup is True, and we activate the old bindings in rollback_live_migration_at_destination(). If do_cleanup is false, we activate them in this \u0027else\u0027 block.","commit_id":"6bd9643b4283676834eb43ccc76393a76f843685"}],"nova/network/neutronv2/api.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8cf7b68a92434fea2c8c40ed2856bfd5d9902768","unresolved":false,"context_lines":[{"line_number":427,"context_line":"                            # this should be done automatically in neutron"},{"line_number":428,"context_line":"                            # when the port bindings are deleted, just like the"},{"line_number":429,"context_line":"                            # atomic update on port binding activation."},{"line_number":430,"context_line":"                            self.activate_port_binding("},{"line_number":431,"context_line":"                                context, port[\u0027id\u0027], instance.host)"},{"line_number":432,"context_line":"                        # This call is safe in that 404s for non-existing"},{"line_number":433,"context_line":"                        # bindings are ignored."}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_cc5fd10c","line":430,"updated":"2019-11-04 20:09:50.000000000","message":"This could raise PortBindingActivationFailed, should we handle that like PortBindingDeletionFailed below and re-raise? Either way the docstring should be updated to mention this method can raise PortBindingActivationFailed now.","commit_id":"6bd9643b4283676834eb43ccc76393a76f843685"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"aefe4941f76690087cfa19ce7d42ab86642f81a7","unresolved":false,"context_lines":[{"line_number":428,"context_line":"                            # when the port bindings are deleted, just like the"},{"line_number":429,"context_line":"                            # atomic update on port binding activation."},{"line_number":430,"context_line":"                            self.activate_port_binding("},{"line_number":431,"context_line":"                                context, port[\u0027id\u0027], instance.host)"},{"line_number":432,"context_line":"                        # This call is safe in that 404s for non-existing"},{"line_number":433,"context_line":"                        # bindings are ignored."},{"line_number":434,"context_line":"                        try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_0cd169fb","line":431,"range":{"start_line":431,"start_character":53,"end_line":431,"end_character":66},"updated":"2019-11-05 15:06:06.000000000","message":"Eesh, we\u0027re relying on our caller here to make sure that instance.host is the correct thing. I guess you\u0027re trying to minimize collateral damage for backportability, but what about changing this to something like teardown_on \u003d host, activate_on \u003d host? I realize we\u0027d have to change all the callers but it seems like it would make the code a bit clearer...","commit_id":"6bd9643b4283676834eb43ccc76393a76f843685"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f8a11255e925a04fed0c8f9707e971713c1a6dd9","unresolved":false,"context_lines":[{"line_number":428,"context_line":"                            # when the port bindings are deleted, just like the"},{"line_number":429,"context_line":"                            # atomic update on port binding activation."},{"line_number":430,"context_line":"                            self.activate_port_binding("},{"line_number":431,"context_line":"                                context, port[\u0027id\u0027], instance.host)"},{"line_number":432,"context_line":"                        # This call is safe in that 404s for non-existing"},{"line_number":433,"context_line":"                        # bindings are ignored."},{"line_number":434,"context_line":"                        try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_272314aa","line":431,"range":{"start_line":431,"start_character":53,"end_line":431,"end_character":66},"in_reply_to":"3fa7e38b_0cd169fb","updated":"2019-11-05 15:28:13.000000000","message":"\u003e Eesh, we\u0027re relying on our caller here to make sure that\n \u003e instance.host is the correct thing.\n\nYup, which can get a bit gross e.g.:\n\nhttps://review.opendev.org/#/c/637630/48/nova/compute/manager.py@4560\n\n \u003e I guess you\u0027re trying to\n \u003e minimize collateral damage for backportability,\n\nI guess, but this has been sitting so long I\u0027m not sure I\u0027d backport it anyway. I think it was more about not having to make major changes to the interface.\n\n \u003e but what about\n \u003e changing this to something like teardown_on \u003d host, activate_on \u003d\n \u003e host? I realize we\u0027d have to change all the callers but it seems\n \u003e like it would make the code a bit clearer...\n\nThat might work. I\u0027m not sure it would be all the callers. If teardown_on defaults to None then it\u0027s still Falsey and anything not tearing down doesn\u0027t change, only cases where callers are passing teardown\u003dTrue.\n\nAnd activate_on is only 2 call sites right now for rolling back a live migration. The other call site would be in my cross-cell resize series here:\n\nhttps://review.opendev.org/#/c/637630/48/nova/compute/manager.py@4551\n\nOverall this method has turned into a monster because it does the port migration profile stuff which for neutron only matters if DVR is involved, which is probably not the majority of deployments these days. But we\u0027re also using this method for other things, and we also have migrate_instance_start/finish, and setup_instance_network_on_host which is also confusing since sounds a lot like this method (setup_networks_on_host). Hell for that matter we also have cleanup_instance_network_on_host which for neutron is a no-op today.\n\nSo other options that don\u0027t involve messing with this method at all would be:\n\n1. Doing like https://review.opendev.org/#/c/637630/48/nova/compute/manager.py@4547 by mutating the migration set dest_compute\u003dsource_compute and call migrate_instance_start. But that mutation sucks since it bleeds / couples the internals of how that method works.\n\n2. Write a new method that\u0027s not so fucking dumb and just call it active_port_bindings_on_host(instance, host) and let the caller determine what to pass in without the migration object. That\u0027s much more straight forward. migrate_instance_start could also be changed to use that.\n\nThe downside to either of the above options is the caller still has to make two calls, one to activate on the source and one to teardown on the dest (the call to this method setup_networks_on_host). Maybe that\u0027s fine, I mean that\u0027s what this change is already doing, but it\u0027s doing it in one place and arguably making this already complicated method do even more complicated stuff.\n\nMaybe a way to solve that complication is to stack a change on top which is a convenience method for this 2-call operation where teardown_host\u003d and activate_host\u003d kwargs determine what to do and then that method calls active_port_bindings_on_host and setup_networks_on_host under the covers.","commit_id":"6bd9643b4283676834eb43ccc76393a76f843685"}]}