)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"26e3288eafd245bf55382f848560148afa67e23a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"92ed2be5_e507b22a","updated":"2023-06-13 01:11:05.000000000","message":"recheck","commit_id":"f0dd02099cdf182c2142f4a8bd9e3d471b098bf7"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"6fe79db39a28f7dafbed15b5230bb2b522419c98","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3428fd1a_1b3dc74e","updated":"2023-06-05 18:14:38.000000000","message":"recheck detach volume/api timeout","commit_id":"f0dd02099cdf182c2142f4a8bd9e3d471b098bf7"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"6844c4014f7da0e56617033e9e53c6301ea52f82","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"08885019_cea826c2","updated":"2023-05-26 17:50:33.000000000","message":"recheck timeouts","commit_id":"f0dd02099cdf182c2142f4a8bd9e3d471b098bf7"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"fc6b561931c5593d91153ab03ba47254056bd5ce","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"8ff28152_88099a5a","updated":"2023-06-09 14:00:13.000000000","message":"recheck volume SSH timeout","commit_id":"f0dd02099cdf182c2142f4a8bd9e3d471b098bf7"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"675e17ef547738952900a14dfc6432c3f1e55dd9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"2824e992_7f28a7b5","updated":"2023-07-13 07:36:16.000000000","message":"recheck","commit_id":"445c1114e44c0833c6d070ae6dd1a8803e1f2741"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e9e9b74d475f12c9e498a61ee424edd56782a688","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"b9395540_29f97c2e","updated":"2023-06-15 11:19:23.000000000","message":"you could pull the notification hardening out into a second commit before the parallelizing work but i think this ok to do it in this one.","commit_id":"445c1114e44c0833c6d070ae6dd1a8803e1f2741"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"ce36a0adb21f5d6e83e91ca2b7efa7f525cc2453","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"bc0f8295_34d979b6","updated":"2023-11-01 02:36:19.000000000","message":"I\u0027m not crazy about the scope creep, but OK.","commit_id":"78f55b78880d1db07b3d5465e3ae6b044edbb88f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"63c4c517fb6a16d5d9feae130456be35f178c879","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":5,"id":"25468b6a_e04ec928","updated":"2023-09-04 12:18:51.000000000","message":"lgtm, \ncan we also have some negative tests for notifications and networks migrate_instance_start.\n\nthere are some numa live migration and regression test but they are specific to thier bug report.","commit_id":"78f55b78880d1db07b3d5465e3ae6b044edbb88f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"85ed5bf2da9a4ebe6690505fdda8c00cfffc1166","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"1bb0f0ec_6471c3c8","updated":"2023-08-28 15:10:01.000000000","message":"recheck","commit_id":"78f55b78880d1db07b3d5465e3ae6b044edbb88f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"5892e10f5bae2307fa44155fd5b76732fb216f6f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"9e65b41e_3620efe6","updated":"2023-08-09 18:59:08.000000000","message":"recheck","commit_id":"78f55b78880d1db07b3d5465e3ae6b044edbb88f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"510c167035bf6d406d517a91f94421544c5f3b76","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"746f7fd4_136c6220","updated":"2023-08-28 18:26:13.000000000","message":"recheck tests successful, but timeout","commit_id":"78f55b78880d1db07b3d5465e3ae6b044edbb88f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"c9515bc392aa8d6624331cf69894fef1282aa1b5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"515e1a36_fa330f4d","updated":"2023-12-16 17:54:37.000000000","message":"I\u0027ve restored PS5. All it does is change the ordering, and put the source volume cleanup in a finally block. That\u0027s my understanding of what was agreed, though I did add an expanded comment explaining the new ordering and the try/finally. The notification best effort stuff is now in the patch above this one.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"e11c6c842b75530eadcfa7e17dea82c7176fbe80","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"ee1f6e9a_a010f684","updated":"2023-11-23 02:05:19.000000000","message":"The overall change here looks reasonable to me but I hesitate on a couple of points:\n\n1) No test coverage or code comments about the ordering. If we think it\u0027s important to change the ordering, IMHO it would be good to add something to help preserve the ordering going forward.\n\n2) I agree the notifications handling is muddling up the clarity in the method. I don\u0027t really _want_ it to be part of this patch but currently driver.post_live_migration() is called first and without any calls that can fail before it. I\u0027m a bit wary of potential scenarios if an MQ issue could result in a messier mess for the operator to clean up when a live migration fails than it is today. TBC, I could be convinced that an MQ fail before driver.post_live_migration would not cause a messier mess -- I just wanted to at least discuss it first.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"eee4dd529ffe237ad78dc0f7a2aa3559cc730000","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"616cd5b5_d4a384f3","updated":"2023-11-01 12:07:06.000000000","message":"as a minimal backportable change im generally ok with this","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"af2e43c3ed735a34f6830e1b200bb9272bae8101","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"049e9612_76175537","updated":"2023-12-05 09:03:08.000000000","message":"lets see if we can add the test coverage and code comment melaine asked for.\ni like the idea of pulling the notfication error handeling into a central place.\nim not sure if that should be in this patch or not.\n\nin genral i dont think the notificaiton code should ever raise as its always best effort so i would generally lean to defautling to best_effort\u003dtrue or just making the try excpet logic uncondtional and always log instead.\n\nthats perhaps out of scope of this however but im ok with including it provide its not too invasive. i fear however it would require a lot of code changes in the tests so a seperate patch for that is prorpbly cleaner since we want to backport this.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"9a45ce3da788beb08475f5fdc69eefd3a05eabe9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"6f91d379_f837d70f","updated":"2023-11-01 12:34:23.000000000","message":"very nice! iirc i\u0027ve actually seen this issue happen in production","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3d64ba45ec41865a75ad65d862c165e1e4ef8566","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"a79ee8ad_5c0a5ef1","in_reply_to":"049e9612_76175537","updated":"2023-12-05 09:27:00.000000000","message":"To be clear, I was saying test coverage OR code comment. Asserting the ordering in a test might be overkill ... I just wanted there to be an artifact of some kind that demonstrates that this order is important. To help ensure people consider that before changing it again in the future.\n\nAnd yeah.. maybe it would be best to do the notification thing as a patch on top of this one and backport both? It would make each change a lot cleaner. I just agree the notifications stuff in this patch hurts the clarity and simplicity quite a bit.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"720b4e2807930b997a0c799d21615a2a697b02dd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"30c31f76_52fe3c53","in_reply_to":"a79ee8ad_5c0a5ef1","updated":"2023-12-05 11:05:45.000000000","message":"i would prefer to backport both.\nto me the fact that notficiton failure coudl cause the handeler to fail is a latent bug of equal severity to the behavior we are tryign to mitgate with the long runing volume operations so i think its imporant to either fix both in the same patch or backport both patches.\n\ni dont really have a stong opion on doign it as 1 or 2 patches however.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"bfcf6fa170469a832ab3d7c7662ec18718bc871e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"5f426ac1_a6512a80","updated":"2023-12-18 17:37:06.000000000","message":"(adding a post approval comment) The split into two patches looks really good, thanks for your patience","commit_id":"26fbc9e8e7d353e66739f910865d0b6498811bb0"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"eb3af9385bf1b168ba20c4c1724ed2fb1671c5e6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"bd103c05_ca994f81","updated":"2023-12-18 14:59:21.000000000","message":"looks good to me","commit_id":"26fbc9e8e7d353e66739f910865d0b6498811bb0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"833408864dd59e855c639c865e0f2873fd54ee41","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"6abf2c1f_17bbabbd","updated":"2023-12-17 13:37:39.000000000","message":"recheck","commit_id":"26fbc9e8e7d353e66739f910865d0b6498811bb0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"3f11a31c99ca559a4701819981f2cd6cabf8c599","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"a4db61af_aeac94bb","updated":"2023-12-19 20:14:13.000000000","message":"recheck glance weirdness","commit_id":"26fbc9e8e7d353e66739f910865d0b6498811bb0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a098e61db44b2856c8e3c018392e40fc8c2a6102","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"ae689102_ced6cb4b","updated":"2023-12-20 04:51:06.000000000","message":"recheck grenade SSH fail","commit_id":"26fbc9e8e7d353e66739f910865d0b6498811bb0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"043fed9ce65598a10ab5c6ff0fc375667a98fa86","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"155e9c41_e4360604","updated":"2023-12-18 19:24:11.000000000","message":"recheck guest kernel panic","commit_id":"26fbc9e8e7d353e66739f910865d0b6498811bb0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"7a7914ac13c81133402f51a736c80c7c9cb4f5e9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"ba4d3b79_2e82999f","updated":"2023-12-18 14:38:02.000000000","message":"recheck keystone fail","commit_id":"26fbc9e8e7d353e66739f910865d0b6498811bb0"}],"nova/compute/manager.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ceaec0cd1be2f8eb5100c068d8d6c2a7a1fcfdd4","unresolved":true,"context_lines":[{"line_number":9250,"context_line":"        compute_utils.notify_about_instance_action("},{"line_number":9251,"context_line":"            ctxt, instance, self.host,"},{"line_number":9252,"context_line":"            action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,"},{"line_number":9253,"context_line":"            phase\u003dfields.NotificationPhase.START)"},{"line_number":9254,"context_line":""},{"line_number":9255,"context_line":"        migration \u003d objects.Migration("},{"line_number":9256,"context_line":"            source_compute\u003dself.host, dest_compute\u003ddest,"}],"source_content_type":"text/x-python","patch_set":3,"id":"057cd389_ae5b55f7","line":9253,"updated":"2023-06-14 21:52:24.000000000","message":"I think my only potential concern with this change would be these notification calls not being wrapped in the try block. I have (unfortunately) seen customer issues where a disruption in the notification message queue or rabbit in general caused a failure or timeout, causing things after the notification(s) not to run.\n\nMaybe that is OK in this case but just saying it\u0027s something to think about. Is there any harm if driver.post_live_migration and _post_live_migration_remove_source_vol_connections are not reached if _notify_about_instance_usage or notify_about_instance_action raise.","commit_id":"8eb5e47ed800fda841faa07c948ebd399d078743"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"559e667473569efe18d41932f1caa25683029a41","unresolved":true,"context_lines":[{"line_number":9250,"context_line":"        compute_utils.notify_about_instance_action("},{"line_number":9251,"context_line":"            ctxt, instance, self.host,"},{"line_number":9252,"context_line":"            action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,"},{"line_number":9253,"context_line":"            phase\u003dfields.NotificationPhase.START)"},{"line_number":9254,"context_line":""},{"line_number":9255,"context_line":"        migration \u003d objects.Migration("},{"line_number":9256,"context_line":"            source_compute\u003dself.host, dest_compute\u003ddest,"}],"source_content_type":"text/x-python","patch_set":3,"id":"b414b8d3_c227fe95","line":9253,"in_reply_to":"057cd389_ae5b55f7","updated":"2023-06-15 01:13:42.000000000","message":"No, this is a valid point. Before this patch, literally nothing happened before the source volume cleanup, so there was no chance of the cleanup not running because of an uncaught exception. Since we\u0027re moving the cleanup to further down in the code flow, anything before it that can raise needs to be inside a try/finally.\n\nPS:\n\u003e I have (unfortunately) seen customer issues where a disruption in the notification message queue or rabbit in general caused a failure or timeout, causing things after the notification(s) not to run.\n\nThis is such a sad lol :(","commit_id":"8eb5e47ed800fda841faa07c948ebd399d078743"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"af2e43c3ed735a34f6830e1b200bb9272bae8101","unresolved":false,"context_lines":[{"line_number":9250,"context_line":"        compute_utils.notify_about_instance_action("},{"line_number":9251,"context_line":"            ctxt, instance, self.host,"},{"line_number":9252,"context_line":"            action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,"},{"line_number":9253,"context_line":"            phase\u003dfields.NotificationPhase.START)"},{"line_number":9254,"context_line":""},{"line_number":9255,"context_line":"        migration \u003d objects.Migration("},{"line_number":9256,"context_line":"            source_compute\u003dself.host, dest_compute\u003ddest,"}],"source_content_type":"text/x-python","patch_set":3,"id":"351f97d8_b75644a3","line":9253,"in_reply_to":"b414b8d3_c227fe95","updated":"2023-12-05 09:03:08.000000000","message":"Acknowledged","commit_id":"8eb5e47ed800fda841faa07c948ebd399d078743"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e9e9b74d475f12c9e498a61ee424edd56782a688","unresolved":true,"context_lines":[{"line_number":9241,"context_line":"        # avoid any potential data leaks, so we put the cleanup code in a"},{"line_number":9242,"context_line":"        # finally block afterwards."},{"line_number":9243,"context_line":"        try:"},{"line_number":9244,"context_line":"            # NOTE(artom) At this point in time we have not bound the ports to"},{"line_number":9245,"context_line":"            # the destination host yet (this happens in"},{"line_number":9246,"context_line":"            # migrate_instance_start() below). Therefore, the \"old\" source"},{"line_number":9247,"context_line":"            # network info that\u0027s still in the instance info cache is safe to"},{"line_number":9248,"context_line":"            # use here, since it\u0027ll be used below during"},{"line_number":9249,"context_line":"            # driver.post_live_migration_at_source() to unplug the VIFs on the"},{"line_number":9250,"context_line":"            # source."},{"line_number":9251,"context_line":"            network_info \u003d instance.get_network_info()"},{"line_number":9252,"context_line":""},{"line_number":9253,"context_line":"            self._notify_about_instance_usage(ctxt, instance,"},{"line_number":9254,"context_line":"                                              \"live_migration._post.start\","},{"line_number":9255,"context_line":"                                              network_info\u003dnetwork_info)"},{"line_number":9256,"context_line":"            compute_utils.notify_about_instance_action("},{"line_number":9257,"context_line":"                ctxt, instance, self.host,"},{"line_number":9258,"context_line":"                action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,"},{"line_number":9259,"context_line":"                phase\u003dfields.NotificationPhase.START)"},{"line_number":9260,"context_line":""},{"line_number":9261,"context_line":"            migration \u003d objects.Migration("},{"line_number":9262,"context_line":"                source_compute\u003dself.host, dest_compute\u003ddest,"}],"source_content_type":"text/x-python","patch_set":4,"id":"afd51355_d497b7fc","line":9259,"range":{"start_line":9244,"start_character":5,"end_line":9259,"end_character":53},"updated":"2023-06-15 11:19:23.000000000","message":"to mel\u0027s question, i would be ok with moving these lines\nto there own try/excpet that ignored any error in sending the notification.\n\nnotifcations are best effert so there failure shoudl not prevent us proceeding with the rest of the of post live migrate\n\n\n\nso this would end up being\n\n       try:\n            # NOTE(artom) At this point in time we have not bound the ports to\n            # the destination host yet (this happens in\n            # migrate_instance_start() below). Therefore, the \"old\" source\n            # network info that\u0027s still in the instance info cache is safe to\n            # use here, since it\u0027ll be used below during\n            # driver.post_live_migration_at_source() to unplug the VIFs on the\n            # source.\n            network_info \u003d instance.get_network_info()\n            self._notify_about_instance_usage(ctxt, instance,\n                                              \"live_migration._post.start\",\n                                              network_info\u003dnetwork_info)\n            compute_utils.notify_about_instance_action(\n                ctxt, instance, self.host,\n                action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,\n                phase\u003dfields.NotificationPhase.START)\n        except:\n            LOG.warning(\"unable to send post_live_mirgation_start notification\", instance\u003dinstance)\n        # NOTE(artom) In case anything in the try block raises an uncaught\n        # exception, we still want to cleanup volumes on the source (us) to\n        # avoid any potential data leaks, so we put the cleanup code in a\n        # finally block afterwards.\n        try:\n            migration \u003d objects.Migration(\n                source_compute\u003dself.host, dest_compute\u003ddest,\n            )\n            # For neutron, migrate_instance_start will activate the destination\n            # host port bindings, if there are any created by conductor before live\n            # migration started.\n            self.network_api.migrate_instance_start(ctxt, instance, migration)\n        finally:\n            # Cleanup source host post live-migration\n            block_device_info \u003d self._get_instance_block_device_info(\n                                ctxt, instance, bdms\u003dsource_bdms)\n            self.driver.post_live_migration(ctxt, instance, block_device_info,\n                                            migrate_data)\n            # Disconnect volumes from this (the source) host.\n            self._post_live_migration_remove_source_vol_connections(\n                ctxt, instance, source_bdms)\n\n\n\nthis way we correctly send the notification at the start of post_live_migrate\nits failure cant impact the rest of the function\nand we dont actuly change the timing form what you were already proposing.","commit_id":"445c1114e44c0833c6d070ae6dd1a8803e1f2741"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"b2d84ddb011be89709c710d274edeaa11d218ab0","unresolved":true,"context_lines":[{"line_number":9241,"context_line":"        # avoid any potential data leaks, so we put the cleanup code in a"},{"line_number":9242,"context_line":"        # finally block afterwards."},{"line_number":9243,"context_line":"        try:"},{"line_number":9244,"context_line":"            # NOTE(artom) At this point in time we have not bound the ports to"},{"line_number":9245,"context_line":"            # the destination host yet (this happens in"},{"line_number":9246,"context_line":"            # migrate_instance_start() below). Therefore, the \"old\" source"},{"line_number":9247,"context_line":"            # network info that\u0027s still in the instance info cache is safe to"},{"line_number":9248,"context_line":"            # use here, since it\u0027ll be used below during"},{"line_number":9249,"context_line":"            # driver.post_live_migration_at_source() to unplug the VIFs on the"},{"line_number":9250,"context_line":"            # source."},{"line_number":9251,"context_line":"            network_info \u003d instance.get_network_info()"},{"line_number":9252,"context_line":""},{"line_number":9253,"context_line":"            self._notify_about_instance_usage(ctxt, instance,"},{"line_number":9254,"context_line":"                                              \"live_migration._post.start\","},{"line_number":9255,"context_line":"                                              network_info\u003dnetwork_info)"},{"line_number":9256,"context_line":"            compute_utils.notify_about_instance_action("},{"line_number":9257,"context_line":"                ctxt, instance, self.host,"},{"line_number":9258,"context_line":"                action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,"},{"line_number":9259,"context_line":"                phase\u003dfields.NotificationPhase.START)"},{"line_number":9260,"context_line":""},{"line_number":9261,"context_line":"            migration \u003d objects.Migration("},{"line_number":9262,"context_line":"                source_compute\u003dself.host, dest_compute\u003ddest,"}],"source_content_type":"text/x-python","patch_set":4,"id":"e4b0ca67_573922c1","line":9259,"range":{"start_line":9244,"start_character":5,"end_line":9259,"end_character":53},"in_reply_to":"afd51355_d497b7fc","updated":"2023-09-04 12:38:52.000000000","message":"i woudl still prefer if you split out the notifications like this.","commit_id":"445c1114e44c0833c6d070ae6dd1a8803e1f2741"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"ce36a0adb21f5d6e83e91ca2b7efa7f525cc2453","unresolved":false,"context_lines":[{"line_number":9241,"context_line":"        # avoid any potential data leaks, so we put the cleanup code in a"},{"line_number":9242,"context_line":"        # finally block afterwards."},{"line_number":9243,"context_line":"        try:"},{"line_number":9244,"context_line":"            # NOTE(artom) At this point in time we have not bound the ports to"},{"line_number":9245,"context_line":"            # the destination host yet (this happens in"},{"line_number":9246,"context_line":"            # migrate_instance_start() below). Therefore, the \"old\" source"},{"line_number":9247,"context_line":"            # network info that\u0027s still in the instance info cache is safe to"},{"line_number":9248,"context_line":"            # use here, since it\u0027ll be used below during"},{"line_number":9249,"context_line":"            # driver.post_live_migration_at_source() to unplug the VIFs on the"},{"line_number":9250,"context_line":"            # source."},{"line_number":9251,"context_line":"            network_info \u003d instance.get_network_info()"},{"line_number":9252,"context_line":""},{"line_number":9253,"context_line":"            self._notify_about_instance_usage(ctxt, instance,"},{"line_number":9254,"context_line":"                                              \"live_migration._post.start\","},{"line_number":9255,"context_line":"                                              network_info\u003dnetwork_info)"},{"line_number":9256,"context_line":"            compute_utils.notify_about_instance_action("},{"line_number":9257,"context_line":"                ctxt, instance, self.host,"},{"line_number":9258,"context_line":"                action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,"},{"line_number":9259,"context_line":"                phase\u003dfields.NotificationPhase.START)"},{"line_number":9260,"context_line":""},{"line_number":9261,"context_line":"            migration \u003d objects.Migration("},{"line_number":9262,"context_line":"                source_compute\u003dself.host, dest_compute\u003ddest,"}],"source_content_type":"text/x-python","patch_set":4,"id":"266dcbef_b277a30b","line":9259,"range":{"start_line":9244,"start_character":5,"end_line":9259,"end_character":53},"in_reply_to":"e4b0ca67_573922c1","updated":"2023-11-01 02:36:19.000000000","message":"Done","commit_id":"445c1114e44c0833c6d070ae6dd1a8803e1f2741"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e9e9b74d475f12c9e498a61ee424edd56782a688","unresolved":true,"context_lines":[{"line_number":9339,"context_line":"        self.update_available_resource(ctxt)"},{"line_number":9340,"context_line":""},{"line_number":9341,"context_line":"        self._update_scheduler_instance_info(ctxt, instance)"},{"line_number":9342,"context_line":"        self._notify_about_instance_usage(ctxt, instance,"},{"line_number":9343,"context_line":"                                          \"live_migration._post.end\","},{"line_number":9344,"context_line":"                                          network_info\u003dnetwork_info)"},{"line_number":9345,"context_line":"        compute_utils.notify_about_instance_action("},{"line_number":9346,"context_line":"            ctxt, instance, self.host,"},{"line_number":9347,"context_line":"            action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,"},{"line_number":9348,"context_line":"            phase\u003dfields.NotificationPhase.END)"},{"line_number":9349,"context_line":"        if post_at_dest_success:"},{"line_number":9350,"context_line":"            LOG.info(\u0027Migrating instance to %s finished successfully.\u0027,"},{"line_number":9351,"context_line":"                     dest, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":4,"id":"f8e47965_0f919fc7","line":9348,"range":{"start_line":9342,"start_character":7,"end_line":9348,"end_character":47},"updated":"2023-06-15 11:19:23.000000000","message":"you could also apply the same logic here and wrap this in a try excpet","commit_id":"445c1114e44c0833c6d070ae6dd1a8803e1f2741"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"ce36a0adb21f5d6e83e91ca2b7efa7f525cc2453","unresolved":false,"context_lines":[{"line_number":9339,"context_line":"        self.update_available_resource(ctxt)"},{"line_number":9340,"context_line":""},{"line_number":9341,"context_line":"        self._update_scheduler_instance_info(ctxt, instance)"},{"line_number":9342,"context_line":"        self._notify_about_instance_usage(ctxt, instance,"},{"line_number":9343,"context_line":"                                          \"live_migration._post.end\","},{"line_number":9344,"context_line":"                                          network_info\u003dnetwork_info)"},{"line_number":9345,"context_line":"        compute_utils.notify_about_instance_action("},{"line_number":9346,"context_line":"            ctxt, instance, self.host,"},{"line_number":9347,"context_line":"            action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,"},{"line_number":9348,"context_line":"            phase\u003dfields.NotificationPhase.END)"},{"line_number":9349,"context_line":"        if post_at_dest_success:"},{"line_number":9350,"context_line":"            LOG.info(\u0027Migrating instance to %s finished successfully.\u0027,"},{"line_number":9351,"context_line":"                     dest, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":4,"id":"11ddb687_f5e77e52","line":9348,"range":{"start_line":9342,"start_character":7,"end_line":9348,"end_character":47},"in_reply_to":"ef2d5e00_74e79414","updated":"2023-11-01 02:36:19.000000000","message":"Done","commit_id":"445c1114e44c0833c6d070ae6dd1a8803e1f2741"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"b2d84ddb011be89709c710d274edeaa11d218ab0","unresolved":true,"context_lines":[{"line_number":9339,"context_line":"        self.update_available_resource(ctxt)"},{"line_number":9340,"context_line":""},{"line_number":9341,"context_line":"        self._update_scheduler_instance_info(ctxt, instance)"},{"line_number":9342,"context_line":"        self._notify_about_instance_usage(ctxt, instance,"},{"line_number":9343,"context_line":"                                          \"live_migration._post.end\","},{"line_number":9344,"context_line":"                                          network_info\u003dnetwork_info)"},{"line_number":9345,"context_line":"        compute_utils.notify_about_instance_action("},{"line_number":9346,"context_line":"            ctxt, instance, self.host,"},{"line_number":9347,"context_line":"            action\u003dfields.NotificationAction.LIVE_MIGRATION_POST,"},{"line_number":9348,"context_line":"            phase\u003dfields.NotificationPhase.END)"},{"line_number":9349,"context_line":"        if post_at_dest_success:"},{"line_number":9350,"context_line":"            LOG.info(\u0027Migrating instance to %s finished successfully.\u0027,"},{"line_number":9351,"context_line":"                     dest, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":4,"id":"ef2d5e00_74e79414","line":9348,"range":{"start_line":9342,"start_character":7,"end_line":9348,"end_character":47},"in_reply_to":"f8e47965_0f919fc7","updated":"2023-09-04 12:38:52.000000000","message":"this is still relevent","commit_id":"445c1114e44c0833c6d070ae6dd1a8803e1f2741"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"63c4c517fb6a16d5d9feae130456be35f178c879","unresolved":true,"context_lines":[{"line_number":9218,"context_line":"                             source_bdms\u003dNone):"},{"line_number":9219,"context_line":"        \"\"\"Post operations for live migration."},{"line_number":9220,"context_line":""},{"line_number":9221,"context_line":"        This method is called from live_migration"},{"line_number":9222,"context_line":"        and mainly updating database record."},{"line_number":9223,"context_line":""},{"line_number":9224,"context_line":"        :param ctxt: security context"},{"line_number":9225,"context_line":"        :param instance: instance object"}],"source_content_type":"text/x-python","patch_set":5,"id":"c358fe60_cbdaaa51","line":9222,"range":{"start_line":9221,"start_character":8,"end_line":9222,"end_character":44},"updated":"2023-09-04 12:18:51.000000000","message":"here doc string says this method mainly update DB, which sounds only Nova DB.\n\nbut its  asking Cinder (to delete connection at their end) and Neutron  (to activate port binding) to preform actions at their end.\n\nshall we update doc string ?","commit_id":"78f55b78880d1db07b3d5465e3ae6b044edbb88f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"eee4dd529ffe237ad78dc0f7a2aa3559cc730000","unresolved":true,"context_lines":[{"line_number":9338,"context_line":""},{"line_number":9339,"context_line":"        # NOTE(artom) Best effort notification sending - the rest of"},{"line_number":9340,"context_line":"        # post_live_migration should still be attempted even if sending the"},{"line_number":9341,"context_line":"        # notifications fails."},{"line_number":9342,"context_line":"        try:"},{"line_number":9343,"context_line":"            # NOTE(artom) At this point in time we have not bound the ports to"},{"line_number":9344,"context_line":"            # the destination host yet (this happens in"}],"source_content_type":"text/x-python","patch_set":6,"id":"03d7bf07_bdf1ddbc","line":9341,"updated":"2023-11-01 12:07:06.000000000","message":"so it would be nice to add testing to assert that this works.\n\ni.e. just mock out _notify_about_instance_usage or notify_about_instance_action\nto raiase something.\n\nbut other hten that we are just reordeing calls and i think we have enouch existng coverage of this so this is more an nit then a hard requirement\n\nif you respin please add a test for this.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"e11c6c842b75530eadcfa7e17dea82c7176fbe80","unresolved":true,"context_lines":[{"line_number":9338,"context_line":""},{"line_number":9339,"context_line":"        # NOTE(artom) Best effort notification sending - the rest of"},{"line_number":9340,"context_line":"        # post_live_migration should still be attempted even if sending the"},{"line_number":9341,"context_line":"        # notifications fails."},{"line_number":9342,"context_line":"        try:"},{"line_number":9343,"context_line":"            # NOTE(artom) At this point in time we have not bound the ports to"},{"line_number":9344,"context_line":"            # the destination host yet (this happens in"}],"source_content_type":"text/x-python","patch_set":6,"id":"58526757_f856d4eb","line":9341,"in_reply_to":"03d7bf07_bdf1ddbc","updated":"2023-11-23 02:05:19.000000000","message":"It does feel weird to not have a test in this patch but I get the thinking that existing tests are \"probably\" covering the main points unrelated to the ordering.\n\nWe may want to consider adding a test that asserts the new ordering here ... from the perspective of, is it clear that this ordering has a specific purpose if/when there\u0027s a patch that proposes changing the ordering again in the future?\n\nAlternatively maybe just a code comment in here that explains why we activate the port bindings first.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"af2e43c3ed735a34f6830e1b200bb9272bae8101","unresolved":true,"context_lines":[{"line_number":9338,"context_line":""},{"line_number":9339,"context_line":"        # NOTE(artom) Best effort notification sending - the rest of"},{"line_number":9340,"context_line":"        # post_live_migration should still be attempted even if sending the"},{"line_number":9341,"context_line":"        # notifications fails."},{"line_number":9342,"context_line":"        try:"},{"line_number":9343,"context_line":"            # NOTE(artom) At this point in time we have not bound the ports to"},{"line_number":9344,"context_line":"            # the destination host yet (this happens in"}],"source_content_type":"text/x-python","patch_set":6,"id":"ae31e167_2797597e","line":9341,"in_reply_to":"58526757_f856d4eb","updated":"2023-12-05 09:03:08.000000000","message":"so the new behavior that is untest is if the notificaiton sending fails we should still proceed with the rest of the method.\n\nso i would prefer if this patch had a test that made _notify_about_instance_usage or notify_about_instance_action fail. \n\nwe could also indriectly test the ordering by making the _get_instance_block_device_info raise and assert the dest port bindings have been activated.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"e11c6c842b75530eadcfa7e17dea82c7176fbe80","unresolved":true,"context_lines":[{"line_number":9460,"context_line":"                phase\u003dfields.NotificationPhase.END)"},{"line_number":9461,"context_line":"        except Exception as e:"},{"line_number":9462,"context_line":"            LOG.warning(\u0027Unable to send live_migration._post.end \u0027"},{"line_number":9463,"context_line":"                        \u0027proceeding anyways. Error: %s\u0027, e, instance\u003dinstance)"},{"line_number":9464,"context_line":""},{"line_number":9465,"context_line":"        if post_at_dest_success:"},{"line_number":9466,"context_line":"            LOG.info(\u0027Migrating instance to %s finished successfully.\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"56b92073_aa451d75","line":9463,"updated":"2023-11-23 02:05:19.000000000","message":"Note (if we choose to keep the notification handling): the repetition with the notifications makes me think maybe it might be worth adding a \"best_effort\" kwarg or similar that defaults to False to these couple of notification sending functions and contain the try-excepts in there. If best_effort\u003dFalse, it reraises the exception else it warns and passes.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"720b4e2807930b997a0c799d21615a2a697b02dd","unresolved":true,"context_lines":[{"line_number":9460,"context_line":"                phase\u003dfields.NotificationPhase.END)"},{"line_number":9461,"context_line":"        except Exception as e:"},{"line_number":9462,"context_line":"            LOG.warning(\u0027Unable to send live_migration._post.end \u0027"},{"line_number":9463,"context_line":"                        \u0027proceeding anyways. Error: %s\u0027, e, instance\u003dinstance)"},{"line_number":9464,"context_line":""},{"line_number":9465,"context_line":"        if post_at_dest_success:"},{"line_number":9466,"context_line":"            LOG.info(\u0027Migrating instance to %s finished successfully.\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"36c9e270_d5345523","line":9463,"in_reply_to":"00bdd065_ba09c468","updated":"2023-12-05 11:05:45.000000000","message":"thats a good question im not sure.\nwhat i was thinking of when consdierign this is do we want to implented defered notifctions.\ni.e. if a notifcation fails to send add it to a quene in memory and have that queue either sent when the next notification is sent or on a periodic.\n\n\nby using a queue we can maintain order with littl extra work if we want to make it more robost my concen with that is what happens if the queue grows large and rabiit comes back. that could result in a lot of load when its starting up again so without a ttl or a usign a expirign queue wehre only the last n notifcation where we purge the older oens i would be hesitent to make that change.\n\nif we want to do the refactor with best_effort\u003dfalse as melaine suggest in this patch and then change the default in a followup that works for me too.\n\ni dont think we should ever try to claim that notification are \"reliable\" but im not agaisnt some kind of limted async retry that wont block the current operation.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3d64ba45ec41865a75ad65d862c165e1e4ef8566","unresolved":true,"context_lines":[{"line_number":9460,"context_line":"                phase\u003dfields.NotificationPhase.END)"},{"line_number":9461,"context_line":"        except Exception as e:"},{"line_number":9462,"context_line":"            LOG.warning(\u0027Unable to send live_migration._post.end \u0027"},{"line_number":9463,"context_line":"                        \u0027proceeding anyways. Error: %s\u0027, e, instance\u003dinstance)"},{"line_number":9464,"context_line":""},{"line_number":9465,"context_line":"        if post_at_dest_success:"},{"line_number":9466,"context_line":"            LOG.info(\u0027Migrating instance to %s finished successfully.\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"36e0a684_ca41a34b","line":9463,"in_reply_to":"069ac687_c5515c25","updated":"2023-12-05 09:27:00.000000000","message":"False would preserve the current default behavior. I agree that True is the ideal default behavior but I guess I\u0027m not sure what all might fail in our existing tests if we were to default best_effort\u003dTrue. Maybe nothing. If unrelated test changes are needed to fix them to work with \"best_effort\" notifications in this file, then the patch gets even larger ☹️","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"55b08c1beb558a7bb8c3f721222c7083d522ad8f","unresolved":true,"context_lines":[{"line_number":9460,"context_line":"                phase\u003dfields.NotificationPhase.END)"},{"line_number":9461,"context_line":"        except Exception as e:"},{"line_number":9462,"context_line":"            LOG.warning(\u0027Unable to send live_migration._post.end \u0027"},{"line_number":9463,"context_line":"                        \u0027proceeding anyways. Error: %s\u0027, e, instance\u003dinstance)"},{"line_number":9464,"context_line":""},{"line_number":9465,"context_line":"        if post_at_dest_success:"},{"line_number":9466,"context_line":"            LOG.info(\u0027Migrating instance to %s finished successfully.\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"00bdd065_ba09c468","line":9463,"in_reply_to":"36e0a684_ca41a34b","updated":"2023-12-05 10:09:42.000000000","message":"I\u0027m OK to make the notification sending best effort and moving the try-catch to the helpers to avoid repetition. Changing of the default behavior through nova should be in a different commit to limit the side of this commit.\n\nIf we are afraid of the notification sending errors then we might also want to check what kind of reconnect loop oslo messaging applies on notification sending. Would such retry loop, if exists, hold up the nova operation for a long time?","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"af2e43c3ed735a34f6830e1b200bb9272bae8101","unresolved":true,"context_lines":[{"line_number":9460,"context_line":"                phase\u003dfields.NotificationPhase.END)"},{"line_number":9461,"context_line":"        except Exception as e:"},{"line_number":9462,"context_line":"            LOG.warning(\u0027Unable to send live_migration._post.end \u0027"},{"line_number":9463,"context_line":"                        \u0027proceeding anyways. Error: %s\u0027, e, instance\u003dinstance)"},{"line_number":9464,"context_line":""},{"line_number":9465,"context_line":"        if post_at_dest_success:"},{"line_number":9466,"context_line":"            LOG.info(\u0027Migrating instance to %s finished successfully.\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"069ac687_c5515c25","line":9463,"in_reply_to":"56b92073_aa451d75","updated":"2023-12-05 09:03:08.000000000","message":"why false? all our notificaitons are best effort so if we were to put this into the sendign functions which i dont have an issue with then we likely should make this the default behavior.","commit_id":"bf4fd3a1909d7a20370dfa266faa439323406621"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"042a79dc4bae38a559a51cf95e2b8b4672e5db74","unresolved":true,"context_lines":[{"line_number":9371,"context_line":"            # host port bindings, if there are any created by conductor before"},{"line_number":9372,"context_line":"            # live migration started."},{"line_number":9373,"context_line":"            self.network_api.migrate_instance_start(ctxt, instance, migration)"},{"line_number":9374,"context_line":"        finally:"},{"line_number":9375,"context_line":"            # Cleanup source host post live-migration"},{"line_number":9376,"context_line":"            block_device_info \u003d self._get_instance_block_device_info("},{"line_number":9377,"context_line":"                                ctxt, instance, bdms\u003dsource_bdms)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5aa2b558_e283c32b","line":9374,"updated":"2023-12-18 15:14:59.000000000","message":"just one thing to note.\n\nthe way a finally block works is it is run regardless of if the try block raises or not\n\nmenaing we will still try to invoke this if the portbindign failed to activate.\n\nthat is simpler to fix then dangeling volumes and its not actully a chagne in behvior form before i.e. if the voluems succeded before and hte binding activation did not then you would have been in the same state of storage clenaed up but networkign not acitvated.\n\nthis can be adressed using the neuton api to activate the port bindigns again and then hard reboot the instance. manual cleanup of the souce node would be required as it is today so there is no regression here.","commit_id":"26fbc9e8e7d353e66739f910865d0b6498811bb0"}]}