)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"98250fbc_773f93a7","updated":"2025-11-20 10:26:16.000000000","message":"Thanks for the review, I hope that I fixed all the comments in last PS","commit_id":"74ae9424f30bde0571a3f88b96277c0137d8eaad"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"46bfe5d635cdec98c00dc77c23eecc3a923ca561","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"214c2320_00671e39","updated":"2025-11-21 17:11:03.000000000","message":"There are 2 different ways of implementing this I think. One is to add this config to the Migrate action, and provide these values to the Helper class.\nIn this change you implemented a configuration for the Helper class, which doesn\u0027t allow actions to customize these parameters in future. If you want to implement a timeout per audit in the future, this will also be a problem. Wdyt?\n-1: to discuss more about the proposal that we want to follow.\nI remember that we discussed in irc about a possible default global timeout for actions, but still, would be inherited on each action, not in the helper.","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"ab7875eabbdb40eae28f225448f96248e1117f29","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"76fee3be_d662a9d2","updated":"2025-11-21 13:06:32.000000000","message":"check-rdo","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"74b378e70b346125e7d269916c531475f5474e28","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"c0cb0b9c_aec73474","updated":"2025-11-21 07:53:11.000000000","message":"check-rdo","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"5543c8323617bde688de4b080cea5a2bd51a0251","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"b9f46cc2_98a312df","in_reply_to":"214c2320_00671e39","updated":"2025-11-24 09:49:40.000000000","message":"In PS5, I kept the option in the method to override de global config\n\nhttps://review.opendev.org/c/openstack/watcher/+/967693/5/watcher/common/nova_helper.py\n\nYou can see the discussion there. Given that we are not setting per-audit or strategy timeout, it was requested to remove the option as it\u0027s kind of dead code. We can add it when we introduce the feature.","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"change_message_id":"0b30d68fcb520c55a8557bec927aeea0d7bea4c6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"6b032158_2c80a25e","updated":"2025-11-27 14:14:10.000000000","message":"teim-ci: manual","commit_id":"4d0d81ee5abe29e857d1a338a0d6d7c6b7297244"},{"author":{"_account_id":28006,"name":"teim-ci","display_name":"teim-ci","email":"ci@seanmooney.info","username":"ci-sean-mooney","status":"this is a third-party ci account run by sean-k-mooney on irc\nhosted at zuul.teim.app"},"change_message_id":"2878a9ea3ec344e3d37845f2be6a3cc08a25c4c8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"8cf09b88_6efb2aac","updated":"2025-11-27 13:34:19.000000000","message":"teim-ci: manual","commit_id":"4d0d81ee5abe29e857d1a338a0d6d7c6b7297244"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"453b58ad280ccb5cbea6e244c74a6aa4aad236bb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"7ce99072_dcd4898d","updated":"2025-11-28 18:25:38.000000000","message":"I agree with current approach, if you are going to sent a new PS for unit tests, I can review that again.","commit_id":"b1d2ac84bb54fae642921ec2b62dd9bd8f4239b1"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"b07c64a54db7e4e8b4e9f87ba849bf67994b0e31","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"cf4bad82_ffd5a956","updated":"2025-11-28 10:30:15.000000000","message":"thanks for the changes Alfredo, I think this is good to go","commit_id":"b1d2ac84bb54fae642921ec2b62dd9bd8f4239b1"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"cd0b860c09f46f818f9260f20b741da0aa75c94e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"9ec8f495_80ae08e5","updated":"2025-12-02 09:54:49.000000000","message":"readding vote after rebase","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"78a11c0d4da196d111ec65b0f4b4d45bfc109c24","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"4f0a23c3_dc7ab834","updated":"2025-12-03 07:21:24.000000000","message":"recheck","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"820f4c5380e1c0466f276e737666cc6cc3b56f13","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"9f3c350c_d0fc02e0","updated":"2025-12-02 16:31:31.000000000","message":"recheck","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"f05159be571de0f27c125f73828e2568ce80058d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"d6e54bf7_7500ea64","updated":"2025-12-03 10:45:14.000000000","message":"recheck","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"380a04f41c4869e054dc470e2fe3d435437762ed","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"defc5389_c7d01ce8","updated":"2025-12-02 13:03:54.000000000","message":"recheck","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"3223d8fe4995f12c9a091e39819a682b0fd6c35d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"6897d7e5_0496a5ca","updated":"2025-12-02 10:52:46.000000000","message":"thanks","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"}],"watcher/common/nova_helper.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"9b8208ab08225a0ae356f5806bf8388e749eafc3","unresolved":true,"context_lines":[{"line_number":224,"context_line":"                   [\"VERIFY_RESIZE\", \"ERROR\"] and retry \u003e 0):"},{"line_number":225,"context_line":"                instance \u003d self.nova.servers.get(instance.id)"},{"line_number":226,"context_line":"                time.sleep(sleep_interval)"},{"line_number":227,"context_line":"                retry -\u003d sleep_interval"},{"line_number":228,"context_line":"            new_hostname \u003d getattr(instance, \u0027OS-EXT-SRV-ATTR:host\u0027)"},{"line_number":229,"context_line":""},{"line_number":230,"context_line":"            if (host_name !\u003d new_hostname and"}],"source_content_type":"text/x-python","patch_set":3,"id":"8b09a19c_9e38f0a2","line":227,"in_reply_to":"229a54c3_49033b9b","updated":"2025-11-19 17:49:37.000000000","message":"let rename this to timeout in the function parmaters too","commit_id":"399e851ecc5dc8ce92fbf624d8225a3f78b5d4eb"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"00c2a34edf6e1df6b4f5860087f6b1f3a1ad17f0","unresolved":false,"context_lines":[{"line_number":224,"context_line":"                   [\"VERIFY_RESIZE\", \"ERROR\"] and retry \u003e 0):"},{"line_number":225,"context_line":"                instance \u003d self.nova.servers.get(instance.id)"},{"line_number":226,"context_line":"                time.sleep(sleep_interval)"},{"line_number":227,"context_line":"                retry -\u003d sleep_interval"},{"line_number":228,"context_line":"            new_hostname \u003d getattr(instance, \u0027OS-EXT-SRV-ATTR:host\u0027)"},{"line_number":229,"context_line":""},{"line_number":230,"context_line":"            if (host_name !\u003d new_hostname and"}],"source_content_type":"text/x-python","patch_set":3,"id":"bb05623d_a5cabc1f","line":227,"in_reply_to":"8b09a19c_9e38f0a2","updated":"2025-12-02 10:44:10.000000000","message":"Acknowledged","commit_id":"399e851ecc5dc8ce92fbf624d8225a3f78b5d4eb"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"23d3bc57a956342dac45c64ce93c4f74a862a8a1","unresolved":true,"context_lines":[{"line_number":224,"context_line":"                   [\"VERIFY_RESIZE\", \"ERROR\"] and retry \u003e 0):"},{"line_number":225,"context_line":"                instance \u003d self.nova.servers.get(instance.id)"},{"line_number":226,"context_line":"                time.sleep(sleep_interval)"},{"line_number":227,"context_line":"                retry -\u003d sleep_interval"},{"line_number":228,"context_line":"            new_hostname \u003d getattr(instance, \u0027OS-EXT-SRV-ATTR:host\u0027)"},{"line_number":229,"context_line":""},{"line_number":230,"context_line":"            if (host_name !\u003d new_hostname and"}],"source_content_type":"text/x-python","patch_set":3,"id":"229a54c3_49033b9b","line":227,"in_reply_to":"97bd82b6_8139d751","updated":"2025-11-19 16:04:49.000000000","message":"I\u0027m checking retry \u003e 0, Is there any problem of retry -\u003d sleep_interval being negative? BTW, i added a test to cover that case:\n\nCONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)\nCONF.set_override(\u0027migrate_polling\u0027, 3, group\u003d\u0027watcher_applier\u0027)","commit_id":"399e851ecc5dc8ce92fbf624d8225a3f78b5d4eb"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a6b90fb26245cf43baf0bed1c67ad4b19a8edee2","unresolved":true,"context_lines":[{"line_number":224,"context_line":"                   [\"VERIFY_RESIZE\", \"ERROR\"] and retry \u003e 0):"},{"line_number":225,"context_line":"                instance \u003d self.nova.servers.get(instance.id)"},{"line_number":226,"context_line":"                time.sleep(sleep_interval)"},{"line_number":227,"context_line":"                retry -\u003d sleep_interval"},{"line_number":228,"context_line":"            new_hostname \u003d getattr(instance, \u0027OS-EXT-SRV-ATTR:host\u0027)"},{"line_number":229,"context_line":""},{"line_number":230,"context_line":"            if (host_name !\u003d new_hostname and"}],"source_content_type":"text/x-python","patch_set":3,"id":"97bd82b6_8139d751","line":227,"in_reply_to":"b82a145e_9bd7118b","updated":"2025-11-19 15:17:04.000000000","message":"\u003e Potential issue with timeout calculation logic - using retry -\u003d sleep_interval may lead to negative values\n\u003e \n\u003e **Severity**: WARNING | **Confidence**: 0.7\n\u003e \n\u003e **Impact**: May cause unexpected behavior when timeout calculations result in negative retry values\n\u003e \n\u003e **Suggestion**:\n\u003e Consider using absolute time comparison instead of countdown arithmetic to ensure accurate timeout handling\n\nthis seams liek a bug and it shoudl just be 1","commit_id":"399e851ecc5dc8ce92fbf624d8225a3f78b5d4eb"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":181,"context_line":"        return volume.status \u003d\u003d status"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"    def watcher_non_live_migrate_instance(self, instance_id, dest_hostname,"},{"line_number":184,"context_line":"                                          retry\u003dNone):"},{"line_number":185,"context_line":"        \"\"\"This method migrates a given instance"},{"line_number":186,"context_line":""},{"line_number":187,"context_line":"        This method uses the Nova built-in migrate()"}],"source_content_type":"text/x-python","patch_set":5,"id":"a5e1d1c7_4bb6c662","line":184,"range":{"start_line":184,"start_character":42,"end_line":184,"end_character":51},"updated":"2025-11-19 18:43:06.000000000","message":"thinking about this more, we dont need to supprot this at all today because we have no way to pass it.\n\ni am not agaisnt suproting that in the future but we normlaly try not ot have dead code that is not called so it woudl be better to remvoe this for now.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"5543c8323617bde688de4b080cea5a2bd51a0251","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        return volume.status \u003d\u003d status"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"    def watcher_non_live_migrate_instance(self, instance_id, dest_hostname,"},{"line_number":184,"context_line":"                                          retry\u003dNone):"},{"line_number":185,"context_line":"        \"\"\"This method migrates a given instance"},{"line_number":186,"context_line":""},{"line_number":187,"context_line":"        This method uses the Nova built-in migrate()"}],"source_content_type":"text/x-python","patch_set":5,"id":"3946cd9e_1f7f5fd3","line":184,"range":{"start_line":184,"start_character":42,"end_line":184,"end_character":51},"in_reply_to":"5e3f4f8e_fcc25154","updated":"2025-11-24 09:49:40.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":true,"context_lines":[{"line_number":181,"context_line":"        return volume.status \u003d\u003d status"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"    def watcher_non_live_migrate_instance(self, instance_id, dest_hostname,"},{"line_number":184,"context_line":"                                          retry\u003dNone):"},{"line_number":185,"context_line":"        \"\"\"This method migrates a given instance"},{"line_number":186,"context_line":""},{"line_number":187,"context_line":"        This method uses the Nova built-in migrate()"}],"source_content_type":"text/x-python","patch_set":5,"id":"5e3f4f8e_fcc25154","line":184,"range":{"start_line":184,"start_character":42,"end_line":184,"end_character":51},"in_reply_to":"a5e1d1c7_4bb6c662","updated":"2025-11-20 10:26:16.000000000","message":"I didn\u0027t want to change the existing parameters of the method in a non-backwards compatible way but tbh it\u0027s not used anywhere so I guess we can freely remove it.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":307,"context_line":""},{"line_number":308,"context_line":"        return True"},{"line_number":309,"context_line":""},{"line_number":310,"context_line":"    def live_migrate_instance(self, instance_id, dest_hostname, retry\u003dNone):"},{"line_number":311,"context_line":"        \"\"\"This method does a live migration of a given instance"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"        This method uses the Nova built-in live_migrate()"}],"source_content_type":"text/x-python","patch_set":5,"id":"10d04016_8986f39e","line":310,"updated":"2025-11-19 18:43:06.000000000","message":"same here we should remove the dead code path related to passign retry explcity\n\nif we ever supprot a non config based way to do this we can add a timeout parmater then.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":true,"context_lines":[{"line_number":307,"context_line":""},{"line_number":308,"context_line":"        return True"},{"line_number":309,"context_line":""},{"line_number":310,"context_line":"    def live_migrate_instance(self, instance_id, dest_hostname, retry\u003dNone):"},{"line_number":311,"context_line":"        \"\"\"This method does a live migration of a given instance"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"        This method uses the Nova built-in live_migrate()"}],"source_content_type":"text/x-python","patch_set":5,"id":"be32edbe_85d7bf37","line":310,"in_reply_to":"10d04016_8986f39e","updated":"2025-11-20 10:26:16.000000000","message":"ditto","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"5543c8323617bde688de4b080cea5a2bd51a0251","unresolved":false,"context_lines":[{"line_number":307,"context_line":""},{"line_number":308,"context_line":"        return True"},{"line_number":309,"context_line":""},{"line_number":310,"context_line":"    def live_migrate_instance(self, instance_id, dest_hostname, retry\u003dNone):"},{"line_number":311,"context_line":"        \"\"\"This method does a live migration of a given instance"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"        This method uses the Nova built-in live_migrate()"}],"source_content_type":"text/x-python","patch_set":5,"id":"d1a7fbf2_1a49bc95","line":310,"in_reply_to":"be32edbe_85d7bf37","updated":"2025-11-24 09:49:40.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"568f000c47e62b5288a06df9f58a2ab42dc8fb45","unresolved":true,"context_lines":[{"line_number":320,"context_line":"        :param dest_hostname: the name of the destination compute node, if"},{"line_number":321,"context_line":"                              destination_node is None, nova scheduler choose"},{"line_number":322,"context_line":"                              the destination host"},{"line_number":323,"context_line":"        :param retry: timeout in seconds for the migration. If None, uses the"},{"line_number":324,"context_line":"                      value from CONF.watcher_applier.migrate_timeout"},{"line_number":325,"context_line":"        \"\"\""},{"line_number":326,"context_line":"        if retry:"}],"source_content_type":"text/x-python","patch_set":5,"id":"04a96034_f115ddb4","line":323,"updated":"2025-11-19 18:09:14.000000000","message":"also timeout here","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"00c2a34edf6e1df6b4f5860087f6b1f3a1ad17f0","unresolved":false,"context_lines":[{"line_number":320,"context_line":"        :param dest_hostname: the name of the destination compute node, if"},{"line_number":321,"context_line":"                              destination_node is None, nova scheduler choose"},{"line_number":322,"context_line":"                              the destination host"},{"line_number":323,"context_line":"        :param retry: timeout in seconds for the migration. If None, uses the"},{"line_number":324,"context_line":"                      value from CONF.watcher_applier.migrate_timeout"},{"line_number":325,"context_line":"        \"\"\""},{"line_number":326,"context_line":"        if retry:"}],"source_content_type":"text/x-python","patch_set":5,"id":"528a0f3a_cec1066e","line":323,"in_reply_to":"04a96034_f115ddb4","updated":"2025-12-02 10:44:10.000000000","message":"Acknowledged","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"af897b4e7158d9c173be2b8b6f8dfb7f1040fac6","unresolved":true,"context_lines":[{"line_number":401,"context_line":"            try:"},{"line_number":402,"context_line":"                self.nova.server_migrations.live_migration_abort("},{"line_number":403,"context_line":"                    server\u003dinstance_id, migration\u003dmigration_id)"},{"line_number":404,"context_line":"            except exception as e:"},{"line_number":405,"context_line":"                # Note: Does not return from here, as abort request can\u0027t be"},{"line_number":406,"context_line":"                # accepted but migration still going on."},{"line_number":407,"context_line":"                LOG.exception(e)"}],"source_content_type":"text/x-python","patch_set":5,"id":"966e8095_0e6b30d6","line":404,"in_reply_to":"39820266_1eca1887","updated":"2025-11-19 16:40:54.000000000","message":"\u003e Bare except clause in abort_live_migrate method\n\u003e \n\u003e **Severity**: HIGH | **Confidence**: 0.9\n\u003e \n\u003e **Risk**: Catches all exceptions including system exceptions like KeyboardInterrupt and SystemExit, making debugging difficult\n\u003e \n\u003e **Priority**: Before merge\n\u003e **Why This Matters**: Bare except clauses violate OpenStack hacking rules (H201) and can mask serious errors, making debugging very difficult in production\n\u003e \n\u003e **Recommendation**:\n\u003e Replace \u0027except exception:\u0027 with specific exception types like \u0027except (nvexceptions.ClientException, Exception) as e:\u0027 to catch only expected exceptions while still allowing proper error handling.\n\nout of the scope for this patch.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"af897b4e7158d9c173be2b8b6f8dfb7f1040fac6","unresolved":true,"context_lines":[{"line_number":427,"context_line":"            return False"},{"line_number":428,"context_line":""},{"line_number":429,"context_line":"        else:"},{"line_number":430,"context_line":"            raise Exception(\"Live migration execution and abort both failed \""},{"line_number":431,"context_line":"                            f\"for the instance {instance_id}\")"},{"line_number":432,"context_line":""},{"line_number":433,"context_line":"    def enable_service_nova_compute(self, hostname):"}],"source_content_type":"text/x-python","patch_set":5,"id":"41244689_5eaf308f","line":430,"in_reply_to":"15a1284f_6a9472c2","updated":"2025-11-19 16:40:54.000000000","message":"\u003e f-string formatting in exception message instead of delayed interpolation\n\u003e \n\u003e **Severity**: WARNING | **Confidence**: 0.8\n\u003e \n\u003e **Impact**: Violates OpenStack logging guidelines (H702) that recommend delayed interpolation for performance and consistency\n\u003e \n\u003e **Suggestion**:\n\u003e Replace f-string formatting with old-style formatting: \u0027Live migration execution and abort both failed for instance %s\u0027 % instance_id\n\nout of the scope for this patch.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"46bfe5d635cdec98c00dc77c23eecc3a923ca561","unresolved":true,"context_lines":[{"line_number":196,"context_line":"                              destination_node is None, nova scheduler choose"},{"line_number":197,"context_line":"                              the destination host"},{"line_number":198,"context_line":"        \"\"\""},{"line_number":199,"context_line":"        timeout \u003d CONF.watcher_applier.migrate_timeout"},{"line_number":200,"context_line":"        LOG.debug("},{"line_number":201,"context_line":"            \"Trying a cold migrate of instance \u0027%s\u0027 \", instance_id)"},{"line_number":202,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"8a67f666_ba2c355e","line":199,"range":{"start_line":199,"start_character":0,"end_line":199,"end_character":54},"updated":"2025-11-21 17:11:03.000000000","message":"Not sure if was just me but I was expecting these timeouts to be in the Action, and not to the helper function.","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"00c2a34edf6e1df6b4f5860087f6b1f3a1ad17f0","unresolved":false,"context_lines":[{"line_number":196,"context_line":"                              destination_node is None, nova scheduler choose"},{"line_number":197,"context_line":"                              the destination host"},{"line_number":198,"context_line":"        \"\"\""},{"line_number":199,"context_line":"        timeout \u003d CONF.watcher_applier.migrate_timeout"},{"line_number":200,"context_line":"        LOG.debug("},{"line_number":201,"context_line":"            \"Trying a cold migrate of instance \u0027%s\u0027 \", instance_id)"},{"line_number":202,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"259b59c6_ff566af3","line":199,"range":{"start_line":199,"start_character":0,"end_line":199,"end_character":54},"in_reply_to":"15e78688_f4715e6f","updated":"2025-12-02 10:44:10.000000000","message":"Done","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"4581d4c1e267a97876d56fafea6d7e2fd89058f0","unresolved":true,"context_lines":[{"line_number":196,"context_line":"                              destination_node is None, nova scheduler choose"},{"line_number":197,"context_line":"                              the destination host"},{"line_number":198,"context_line":"        \"\"\""},{"line_number":199,"context_line":"        timeout \u003d CONF.watcher_applier.migrate_timeout"},{"line_number":200,"context_line":"        LOG.debug("},{"line_number":201,"context_line":"            \"Trying a cold migrate of instance \u0027%s\u0027 \", instance_id)"},{"line_number":202,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"15e78688_f4715e6f","line":199,"range":{"start_line":199,"start_character":0,"end_line":199,"end_character":54},"in_reply_to":"5376ba6b_3203f609","updated":"2025-11-24 19:44:28.000000000","message":"yeah, I was thinking similar to Joan here. The helper could accept both parameters, and by adding a new one (interval), you wouldn\u0027t be breaking the interface. I think that removing the `timeout` is a blocker for backporting this patch.\nI would like to have a configuration per action instead(in this case, we should use the `get_config_opts`). Setting this configuration directly in the helper, you may block custom action plugins to also configure their own timeouts. What do you guys think?","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"5543c8323617bde688de4b080cea5a2bd51a0251","unresolved":true,"context_lines":[{"line_number":196,"context_line":"                              destination_node is None, nova scheduler choose"},{"line_number":197,"context_line":"                              the destination host"},{"line_number":198,"context_line":"        \"\"\""},{"line_number":199,"context_line":"        timeout \u003d CONF.watcher_applier.migrate_timeout"},{"line_number":200,"context_line":"        LOG.debug("},{"line_number":201,"context_line":"            \"Trying a cold migrate of instance \u0027%s\u0027 \", instance_id)"},{"line_number":202,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"d7a1c4dc_ff176e01","line":199,"range":{"start_line":199,"start_character":0,"end_line":199,"end_character":54},"in_reply_to":"8a67f666_ba2c355e","updated":"2025-11-24 09:49:40.000000000","message":"IIUC, the timeout itself needs to be applied in the helper function, as it\u0027s the only place where we poll and can break the loop. We can do it differently, add the config parameter to the action migrate and pass it to the helper function. IMO, it\u0027d be pretty similar from a functional point of view, but I can change it if it\u0027s better. What I want to avoid in this patch is to modify the action parameters. But, other than that, I think both options are doable.","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"7c7f8077e02558eff8b51071e21d98fe1864995e","unresolved":true,"context_lines":[{"line_number":196,"context_line":"                              destination_node is None, nova scheduler choose"},{"line_number":197,"context_line":"                              the destination host"},{"line_number":198,"context_line":"        \"\"\""},{"line_number":199,"context_line":"        timeout \u003d CONF.watcher_applier.migrate_timeout"},{"line_number":200,"context_line":"        LOG.debug("},{"line_number":201,"context_line":"            \"Trying a cold migrate of instance \u0027%s\u0027 \", instance_id)"},{"line_number":202,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5376ba6b_3203f609","line":199,"range":{"start_line":199,"start_character":0,"end_line":199,"end_character":54},"in_reply_to":"d7a1c4dc_ff176e01","updated":"2025-11-24 15:45:19.000000000","message":"I think we should add both the `timeout` and `sleep_interval` as parameters to the method, since in a future patch we want to support configuring those values as audit parameters, and keep the conf values as default values for the parameters to avoid breaking changes. In the action code I would for now read the values from the configuration and once we support setting in the audit parameters we can add them to the action input parameters","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"}],"watcher/conf/applier.py":[{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"19ab2d902f22d654b3177ca37a7852cf119f520a","unresolved":true,"context_lines":[{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"},{"line_number":53,"context_line":"                    \u0027Default value is 900 seconds (15 minutes).\u0027),"},{"line_number":54,"context_line":"]"}],"source_content_type":"text/x-python","patch_set":3,"id":"37f1794c_f69caf0f","line":51,"range":{"start_line":51,"start_character":23,"end_line":51,"end_character":26},"updated":"2025-11-19 14:22:59.000000000","message":"Rethinking about the default value, according to:\n\nhttps://docs.openstack.org/nova/latest//configuration/config.html\n\nThe default in nova is not static 800s, but live_migration_completion_timeout*(ramGB), so for a 8GB vm it\u0027d be 6400s, so 900 seconds it\u0027s likely too small.\n\nWhat do you think that could be a good default value?","commit_id":"399e851ecc5dc8ce92fbf624d8225a3f78b5d4eb"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a6b90fb26245cf43baf0bed1c67ad4b19a8edee2","unresolved":true,"context_lines":[{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"},{"line_number":53,"context_line":"                    \u0027Default value is 900 seconds (15 minutes).\u0027),"},{"line_number":54,"context_line":"]"}],"source_content_type":"text/x-python","patch_set":3,"id":"4af7ac1d_30a4d501","line":51,"range":{"start_line":51,"start_character":23,"end_line":51,"end_character":26},"in_reply_to":"37f1794c_f69caf0f","updated":"2025-11-19 15:17:04.000000000","message":"900s i htink is a good baseline\n\nits 15m and while we may expect some live migration to take longer we woudl expect a large percentatge, \u003e80% would be my guess, to complete in this limit\n\nthe defautl in nova were established when 10g networkign was considerd high speed networkign wich it very much is not today.\n\nnova also finally gained the ablity to do multi thread live migraiton which should decresase live migraiton time even more.\n\nwe can make this large later if we see form testing that its too low as a default.","commit_id":"399e851ecc5dc8ce92fbf624d8225a3f78b5d4eb"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"9b8208ab08225a0ae356f5806bf8388e749eafc3","unresolved":true,"context_lines":[{"line_number":47,"context_line":"                default\u003dFalse,"},{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"},{"line_number":53,"context_line":"                    \u0027Default value is 900 seconds (15 minutes).\u0027),"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf50dd69_bedd439f","line":50,"in_reply_to":"1615e7d4_b516b3cd","updated":"2025-11-19 17:49:37.000000000","message":"i woudl add min\u003d1  unless you want to add min\u003d0 and have 0 be no time out which is also valid. honestly there is no other reasonabel lowwer bound but htis should not be negitive.\n\n\ni agree we do not need a max","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"9b8208ab08225a0ae356f5806bf8388e749eafc3","unresolved":true,"context_lines":[{"line_number":47,"context_line":"                default\u003dFalse,"},{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"},{"line_number":53,"context_line":"                    \u0027Default value is 900 seconds (15 minutes).\u0027),"}],"source_content_type":"text/x-python","patch_set":5,"id":"52ba9a12_99bcda23","line":50,"in_reply_to":"2948fa28_5c4a9a9c","updated":"2025-11-19 17:49:37.000000000","message":"\u003e Add docstring examples for the new configuration options\n\u003e \n\u003e **Severity**: SUGGESTION | **Confidence**: 0.6\n\u003e \n\u003e **Benefit**: Would improve user understanding of how to use these migration timeout options effectively\n\u003e \n\u003e **Recommendation**:\n\u003e Enhance help text with usage examples showing how to configure timeouts for different migration scenarios\n\ni guess we coudl but honeslty this depens on your network bandwith and other factors so i would leave it as is.\n\nwe do something add tuning examples liek this however.\n\nat most i woudl recommend against reducign it but also say that extreamly lsarge value like 1 hour is not adviced.\n\nlive migration should complete in a timely maner so we calarify the workding\n```\n help\u003d\u0027Timeout in seconds for VM migration actions. \u0027\n                    \u0027Default value is 900 seconds (15 minutes).\u0027),\n```\n\nto \n```\n help\u003d\"\"\"\n Timeout in seconds for VM migration actions.\n Default value is 900 seconds (15 minutes).\n \n This is an upper bound on the maximum expected time for a migration to complete\n and generally should not be decrease or increase over 3600s (one hour).\n lower values may cause action to be marked as failed if vms are under load.\n larger value might mask infrastructure issues or indicate that a workload\n is not suitable for live migrations.\n \n In most case migration are expected to complete in seconds or single\n digit minutes. This option allow you accommodate low networks bandwidth\n or hevially loaded vms. it is stongly recommend that you consider cold\n migation over setting this to a large value.\n      ),\n```\n\nsomethign like that.\n\nbasiclly if you need to set this to somethign other then the default it shoudl be informed by data and testing in your real envionment and you woudl be better off investiating upgrading your network infra or tuneing nova for better live migration performace.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":true,"context_lines":[{"line_number":47,"context_line":"                default\u003dFalse,"},{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"},{"line_number":53,"context_line":"                    \u0027Default value is 900 seconds (15 minutes).\u0027),"}],"source_content_type":"text/x-python","patch_set":5,"id":"69f6a02c_90d2183f","line":50,"in_reply_to":"52ba9a12_99bcda23","updated":"2025-11-20 10:26:16.000000000","message":"I\u0027ll extend the help and include the recommendation of not decreasing it or doing it higher that one hour.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a3d0b31ee197be418487b1a9125c81c1e91895e6","unresolved":false,"context_lines":[{"line_number":47,"context_line":"                default\u003dFalse,"},{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"},{"line_number":53,"context_line":"                    \u0027Default value is 900 seconds (15 minutes).\u0027),"}],"source_content_type":"text/x-python","patch_set":5,"id":"48596956_e070eef6","line":50,"in_reply_to":"69f6a02c_90d2183f","updated":"2025-12-02 07:45:13.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"af897b4e7158d9c173be2b8b6f8dfb7f1040fac6","unresolved":true,"context_lines":[{"line_number":47,"context_line":"                default\u003dFalse,"},{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"},{"line_number":53,"context_line":"                    \u0027Default value is 900 seconds (15 minutes).\u0027),"}],"source_content_type":"text/x-python","patch_set":5,"id":"1615e7d4_b516b3cd","line":50,"in_reply_to":"84cb6583_5823af06","updated":"2025-11-19 16:40:54.000000000","message":"\u003e Consider adding validation for migrate_timeout and migrate_polling configuration values\n\u003e \n\u003e **Severity**: SUGGESTION | **Confidence**: 0.7\n\u003e \n\u003e **Benefit**: Would prevent configuration errors and provide better user feedback for invalid values\n\u003e \n\u003e **Recommendation**:\n\u003e Add min/max validation for migrate_timeout (e.g., min\u003d30, max\u003d3600) and migrate_polling (e.g., min\u003d0.1, max\u003d60) to ensure reasonable values\n\nWhile we could set max values for these, I think keeping this unlimited is good enough, otherwise i can use some values to check something like 0 \u003c migrate_polling \u003c 120 and 0 \u003c migrate_timeout \u003c 36000 (10 hours) to make sure nobody uses crazy values","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"9b8208ab08225a0ae356f5806bf8388e749eafc3","unresolved":true,"context_lines":[{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"},{"line_number":53,"context_line":"                    \u0027Default value is 900 seconds (15 minutes).\u0027),"},{"line_number":54,"context_line":"    cfg.FloatOpt(\u0027migrate_polling\u0027,"},{"line_number":55,"context_line":"                 default\u003d5.0,"},{"line_number":56,"context_line":"                 help\u003d\u0027Polling time in seconds to check the status in \u0027"},{"line_number":57,"context_line":"                      \u0027VM migration actions (value is float). Default value \u0027"}],"source_content_type":"text/x-python","patch_set":5,"id":"345ca096_424e4287","line":54,"in_reply_to":"255035e6_cbd4474f","updated":"2025-11-19 17:49:37.000000000","message":"\u003e Missing type annotation for migrate_polling configuration option\n\u003e \n\u003e **Severity**: WARNING | **Confidence**: 0.7\n\u003e \n\u003e **Impact**: Configuration option documentation lacks type clarity for users\n\u003e \n\u003e **Suggestion**:\n\u003e Add type annotation to the help text or use cfg.FloatOpt with type parameter to make it clear this expects a float value\n\nthis is a false positive.\nit does not under snad how typing is done with oslo config currently.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"00c2a34edf6e1df6b4f5860087f6b1f3a1ad17f0","unresolved":false,"context_lines":[{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"},{"line_number":53,"context_line":"                    \u0027Default value is 900 seconds (15 minutes).\u0027),"},{"line_number":54,"context_line":"    cfg.FloatOpt(\u0027migrate_polling\u0027,"},{"line_number":55,"context_line":"                 default\u003d5.0,"},{"line_number":56,"context_line":"                 help\u003d\u0027Polling time in seconds to check the status in \u0027"},{"line_number":57,"context_line":"                      \u0027VM migration actions (value is float). Default value \u0027"}],"source_content_type":"text/x-python","patch_set":5,"id":"553fd388_5d0f9a77","line":54,"in_reply_to":"345ca096_424e4287","updated":"2025-12-02 10:44:10.000000000","message":"Acknowledged","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"46bfe5d635cdec98c00dc77c23eecc3a923ca561","unresolved":true,"context_lines":[{"line_number":47,"context_line":"                default\u003dFalse,"},{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               min\u003d1,"},{"line_number":53,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"06f5cd49_6c070502","line":50,"range":{"start_line":50,"start_character":16,"end_line":50,"end_character":31},"updated":"2025-11-21 17:11:03.000000000","message":"why setting as global applier timeout and not as Action migrate config opts?","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"5543c8323617bde688de4b080cea5a2bd51a0251","unresolved":true,"context_lines":[{"line_number":47,"context_line":"                default\u003dFalse,"},{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               min\u003d1,"},{"line_number":53,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"c8796cba_89b47ab7","line":50,"range":{"start_line":50,"start_character":16,"end_line":50,"end_character":31},"in_reply_to":"06f5cd49_6c070502","updated":"2025-11-24 09:49:40.000000000","message":"I simply wanted to reuse an existing config section that makes sense for me, as it\u0027s only used in the applier and applies to any migrate action. I don\u0027t mind moving it to other section or creating a new one.","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"7c7f8077e02558eff8b51071e21d98fe1864995e","unresolved":true,"context_lines":[{"line_number":47,"context_line":"                default\u003dFalse,"},{"line_number":48,"context_line":"                help\u003d\u0027If set True, the failed actionplan will rollback \u0027"},{"line_number":49,"context_line":"                     \u0027when executing. Default value is False.\u0027),"},{"line_number":50,"context_line":"    cfg.IntOpt(\u0027migrate_timeout\u0027,"},{"line_number":51,"context_line":"               default\u003d900,"},{"line_number":52,"context_line":"               min\u003d1,"},{"line_number":53,"context_line":"               help\u003d\u0027Timeout in seconds for VM migration actions. \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"d46049fc_eaaff6ed","line":50,"range":{"start_line":50,"start_character":16,"end_line":50,"end_character":31},"in_reply_to":"c8796cba_89b47ab7","updated":"2025-11-24 15:45:19.000000000","message":"I feel like it would be a bit clearer to have them in a separate section per action","commit_id":"223047e67b6ec362fd84134bf87c52451276dde4"}],"watcher/conf/nova.py":[{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"453b58ad280ccb5cbea6e244c74a6aa4aad236bb","unresolved":true,"context_lines":[{"line_number":24,"context_line":"               default\u003d180,"},{"line_number":25,"context_line":"               min\u003d1,"},{"line_number":26,"context_line":"               help\u003d\u0027Maximum number of retries for Nova migrations \u0027"},{"line_number":27,"context_line":"                    \u0027before giving up and considering the action failed. \u0027"},{"line_number":28,"context_line":"                    \u0027Default value is 180, which for the default \u0027"},{"line_number":29,"context_line":"                    \u0027migration_interval (5 seconds), makes a default \u0027"},{"line_number":30,"context_line":"                    \u0027migration timeout of 900 seconds (15 minutes).\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"bd95bb98_6c803bb4","line":27,"range":{"start_line":27,"start_character":58,"end_line":27,"end_character":64},"updated":"2025-11-28 18:25:38.000000000","message":"nit: operation","commit_id":"b1d2ac84bb54fae642921ec2b62dd9bd8f4239b1"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a3d0b31ee197be418487b1a9125c81c1e91895e6","unresolved":false,"context_lines":[{"line_number":24,"context_line":"               default\u003d180,"},{"line_number":25,"context_line":"               min\u003d1,"},{"line_number":26,"context_line":"               help\u003d\u0027Maximum number of retries for Nova migrations \u0027"},{"line_number":27,"context_line":"                    \u0027before giving up and considering the action failed. \u0027"},{"line_number":28,"context_line":"                    \u0027Default value is 180, which for the default \u0027"},{"line_number":29,"context_line":"                    \u0027migration_interval (5 seconds), makes a default \u0027"},{"line_number":30,"context_line":"                    \u0027migration timeout of 900 seconds (15 minutes).\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"9d7b52eb_41cf9cbd","line":27,"range":{"start_line":27,"start_character":58,"end_line":27,"end_character":64},"in_reply_to":"bd95bb98_6c803bb4","updated":"2025-12-02 07:45:13.000000000","message":"Done","commit_id":"b1d2ac84bb54fae642921ec2b62dd9bd8f4239b1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"00c2a34edf6e1df6b4f5860087f6b1f3a1ad17f0","unresolved":false,"context_lines":[{"line_number":20,"context_line":"                    \u0027configuration\u0027)"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"NOVA_OPTS \u003d ["},{"line_number":23,"context_line":"    cfg.IntOpt(\u0027migration_max_retries\u0027,"},{"line_number":24,"context_line":"               default\u003d180,"},{"line_number":25,"context_line":"               min\u003d1,"},{"line_number":26,"context_line":"               help\u003d\u0027Maximum number of retries for Nova migrations \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"566fdc98_370391e4","line":23,"in_reply_to":"51385bae_feffa058","updated":"2025-12-02 10:44:10.000000000","message":"there is no obvious upper bound value.\nwe coudl set max to 1 hour assuming the default value of the interval.\nhowever that is going to fial on teh nova side in most cases based on its timeout and our 15 minute deault is already on the high side so resolving this","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"d7b3409cee0e6cbc8d43796ca26a30db60d65ef4","unresolved":false,"context_lines":[{"line_number":23,"context_line":"    cfg.IntOpt(\u0027migration_max_retries\u0027,"},{"line_number":24,"context_line":"               default\u003d180,"},{"line_number":25,"context_line":"               min\u003d1,"},{"line_number":26,"context_line":"               help\u003d\u0027Maximum number of retries for Nova migrations \u0027"},{"line_number":27,"context_line":"                    \u0027before giving up and considering the operation failed. \u0027"},{"line_number":28,"context_line":"                    \u0027Default value is 180, which for the default \u0027"},{"line_number":29,"context_line":"                    \u0027migration_interval (5 seconds), makes a default \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"caecc6e9_af262989","line":26,"in_reply_to":"16242f66_169eb5a7","updated":"2026-02-09 17:44:09.000000000","message":"Done","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"00c2a34edf6e1df6b4f5860087f6b1f3a1ad17f0","unresolved":true,"context_lines":[{"line_number":23,"context_line":"    cfg.IntOpt(\u0027migration_max_retries\u0027,"},{"line_number":24,"context_line":"               default\u003d180,"},{"line_number":25,"context_line":"               min\u003d1,"},{"line_number":26,"context_line":"               help\u003d\u0027Maximum number of retries for Nova migrations \u0027"},{"line_number":27,"context_line":"                    \u0027before giving up and considering the operation failed. \u0027"},{"line_number":28,"context_line":"                    \u0027Default value is 180, which for the default \u0027"},{"line_number":29,"context_line":"                    \u0027migration_interval (5 seconds), makes a default \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"16242f66_169eb5a7","line":26,"in_reply_to":"fdd3dd86_83003d56","updated":"2025-12-02 10:44:10.000000000","message":"\u003e Configuration option help text formatting inconsistency\n\u003e \n\u003e **Severity**: WARNING | **Confidence**: 0.8\n\u003e \n\u003e **Impact**: Documentation quality and user experience\n\u003e \n\u003e **Suggestion**:\n\u003e Improve help text formatting to be more consistent and readable, consider breaking the long help string into multiple lines with proper indentation\n\nwe do not need to fix this but for future refernece we tend\nto swap to multi line string if it required more then 1 or 2 string concatonations.\n```\nhelp\u003d\u0027Maximum number of retries for Nova migrations \u0027\n                    \u0027before giving up and considering the operation failed. \u0027\n                    \u0027Default value is 180, which for the default \u0027\n                    \u0027migration_interval (5 seconds), makes a default \u0027\n                    \u0027migration timeout of 900 seconds (15 minutes).\u0027\n                    \u0027This is an upper bound on the maximum expected. This \u0027\n                    \u0027should not be decreased or increased over 3600s \u0027\n                    \u0027(one hour). Shorter values may cause the actions to \u0027\n                    \u0027fail and higher values may hide infrastructure issues.\u0027),\n```\nto\n```\nhelp\u003d\"\"\"\nMaximum number of retries for Nova migrations\nbefore giving up and considering the operation failed.\nDefault value is 180, which for the default\nmigration_interval (5 seconds), makes a default\nmigration timeout of 900 seconds (15 minutes).\nThis is an upper bound on the maximum expected.\nThis should not be decreased or increased over 3600s\n(one hour). Shorter values may cause the actions to\nfail and higher values may hide infrastructure issues.\"\"\"),\n```                    \nhttps://github.com/openstack/nova/blob/master/nova/conf/base.py#L30-L40\nhttps://github.com/openstack/watcher/blob/master/watcher/conf/collector.py#L26-L37\n\nwhen you are writign longer config help text in the future please prefer that pattern\n\nno need to respin for htis.","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"}],"watcher/tests/common/test_nova_helper.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"568f000c47e62b5288a06df9f58a2ab42dc8fb45","unresolved":false,"context_lines":[{"line_number":33,"context_line":"CONF \u003d conf.CONF"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"@mock.patch.object(clients.OpenStackClients, \u0027nova\u0027)"},{"line_number":37,"context_line":"@mock.patch.object(clients.OpenStackClients, \u0027cinder\u0027)"},{"line_number":38,"context_line":"class TestNovaHelper(base.TestCase):"},{"line_number":39,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"d1e87d06_2d22b949","line":36,"in_reply_to":"47f0f7ad_aa83ae20","updated":"2025-11-19 18:09:14.000000000","message":"ya i have it setup to review the entire commit so it wil review all the modifed fiels not just the modifed hunks\n\nit shoudl focus on your change btu ti will find things like this.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"af897b4e7158d9c173be2b8b6f8dfb7f1040fac6","unresolved":true,"context_lines":[{"line_number":33,"context_line":"CONF \u003d conf.CONF"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"@mock.patch.object(clients.OpenStackClients, \u0027nova\u0027)"},{"line_number":37,"context_line":"@mock.patch.object(clients.OpenStackClients, \u0027cinder\u0027)"},{"line_number":38,"context_line":"class TestNovaHelper(base.TestCase):"},{"line_number":39,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"47f0f7ad_aa83ae20","line":36,"in_reply_to":"90fd45b4_bdc22676","updated":"2025-11-19 16:40:54.000000000","message":"\u003e Test method uses generic exception without autospec in some mock patches\n\u003e \n\u003e **Severity**: WARNING | **Confidence**: 0.6\n\u003e \n\u003e **Impact**: Missing autospec\u003dTrue in mock decorators is not following OpenStack testing best practices (H210)\n\u003e \n\u003e **Suggestion**:\n\u003e Add autospec\u003dTrue to all @mock.patch decorators for better mock behavior and compliance with OpenStack testing standards\n\nout of the scope for this patch.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"568f000c47e62b5288a06df9f58a2ab42dc8fb45","unresolved":true,"context_lines":[{"line_number":443,"context_line":"        )"},{"line_number":444,"context_line":"        self.assertTrue(is_success)"},{"line_number":445,"context_line":""},{"line_number":446,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":447,"context_line":"    def test_watcher_non_live_migrate_instance_retry_default("},{"line_number":448,"context_line":"            self, mock_cinder, mock_nova):"},{"line_number":449,"context_line":"        \"\"\"Test that watcher_non_live_migrate uses config timeout by default\"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"219c7358_4e941f0f","line":446,"range":{"start_line":446,"start_character":3,"end_line":446,"end_character":50},"updated":"2025-11-19 18:09:14.000000000","message":"this work but either do \n\n```suggestion\n    @mock.patch.object(time, \u0027sleep\u0027)\n```\n\nor do \n```\n    @mock.patch.object(time, \u0027sleep\u0027, new\u003dmock.Mock())\n```\n\nif you dont intend to use the mock in the test fucntio to assert something.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a3d0b31ee197be418487b1a9125c81c1e91895e6","unresolved":false,"context_lines":[{"line_number":443,"context_line":"        )"},{"line_number":444,"context_line":"        self.assertTrue(is_success)"},{"line_number":445,"context_line":""},{"line_number":446,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":447,"context_line":"    def test_watcher_non_live_migrate_instance_retry_default("},{"line_number":448,"context_line":"            self, mock_cinder, mock_nova):"},{"line_number":449,"context_line":"        \"\"\"Test that watcher_non_live_migrate uses config timeout by default\"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"c3a8d490_bd96711a","line":446,"range":{"start_line":446,"start_character":3,"end_line":446,"end_character":50},"in_reply_to":"219c7358_4e941f0f","updated":"2025-12-02 07:45:13.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"568f000c47e62b5288a06df9f58a2ab42dc8fb45","unresolved":true,"context_lines":[{"line_number":450,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":451,"context_line":"        server \u003d self.fake_server(self.instance_uuid)"},{"line_number":452,"context_line":"        setattr(server, \u0027OS-EXT-SRV-ATTR:host\u0027, self.source_node)"},{"line_number":453,"context_line":"        setattr(server, \u0027status\u0027, \u0027MIGRATING\u0027)  # Never reaches ACTIVE"},{"line_number":454,"context_line":""},{"line_number":455,"context_line":"        # Set config value"},{"line_number":456,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"c1435a1c_9c95dcf2","line":453,"updated":"2025-11-19 18:09:14.000000000","message":"you shoudl not use setatter in a test like this.\n```suggestion\n        server.status\u003d\u0027MIGRATING\u0027\n```\n\n or you shoudl change fake_server to build up the instnace object vai the kw args more completely.\n \n i have not checked if this is an instance object for nova vlient if it is you shoudl eb able to constuct it va passign the info dict like we dicussed on one fo your other patches.\n \n we really shoudl have our own instance object in \n \n https://github.com/openstack/watcher/tree/master/watcher/objects\n but that exsitng tech debth.\n \n thjis is testing the test helper so usign nova client instnace object for nwo is kind of ok but nothign outside that module shoudl do that.\n \n we do have \n \n https://github.com/openstack/watcher/blob/master/watcher/decision_engine/model/element/instance.py#L41-L59\n \n but while that is an ov its in the wrong location and the nova helper is not preprly converting the nova api respocen into our ovo form...","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a3d0b31ee197be418487b1a9125c81c1e91895e6","unresolved":false,"context_lines":[{"line_number":450,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":451,"context_line":"        server \u003d self.fake_server(self.instance_uuid)"},{"line_number":452,"context_line":"        setattr(server, \u0027OS-EXT-SRV-ATTR:host\u0027, self.source_node)"},{"line_number":453,"context_line":"        setattr(server, \u0027status\u0027, \u0027MIGRATING\u0027)  # Never reaches ACTIVE"},{"line_number":454,"context_line":""},{"line_number":455,"context_line":"        # Set config value"},{"line_number":456,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"275ea824_8d7f18eb","line":453,"in_reply_to":"592fdba3_3e2562c0","updated":"2025-12-02 07:45:13.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":true,"context_lines":[{"line_number":450,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":451,"context_line":"        server \u003d self.fake_server(self.instance_uuid)"},{"line_number":452,"context_line":"        setattr(server, \u0027OS-EXT-SRV-ATTR:host\u0027, self.source_node)"},{"line_number":453,"context_line":"        setattr(server, \u0027status\u0027, \u0027MIGRATING\u0027)  # Never reaches ACTIVE"},{"line_number":454,"context_line":""},{"line_number":455,"context_line":"        # Set config value"},{"line_number":456,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"592fdba3_3e2562c0","line":453,"in_reply_to":"c1435a1c_9c95dcf2","updated":"2025-11-20 10:26:16.000000000","message":"I\u0027d like to change the fake_server and fake_hypervisor to be real objects for all these tests but I guess that will take some more work and would prefer to do it in a separate patch.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":453,"context_line":"        setattr(server, \u0027status\u0027, \u0027MIGRATING\u0027)  # Never reaches ACTIVE"},{"line_number":454,"context_line":""},{"line_number":455,"context_line":"        # Set config value"},{"line_number":456,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":459,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"e72a3c2d_4d44f148","line":456,"updated":"2025-11-19 18:43:06.000000000","message":"note ot self we do not have the self.flags fuction yet.\nhttps://gist.github.com/SeanMooney/43afa55282d2286a312eae7f3c7709e2#phase-4-base-functional-test-class-1-commit\n\n```\n    def flags(self, **kw):\n        \"\"\"Override flag variables for a test.\n        \n        Example:\n            self.flags(periodic_interval\u003d10,\n                      group\u003d\u0027watcher_decision_engine\u0027)\n        \"\"\"\n        group \u003d kw.pop(\u0027group\u0027, None)\n        for k, v in kw.items():\n            CONF.set_override(k, v, group)\n            self.addCleanup(CONF.clear_override, k, group)\n```\nwe tehcnially shoudl be adding the self.addCleanup\n\nalthough the way we use the config fixture curerntly its reinitalised in every test. we are still correct when not using it because fo that.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"d7b3409cee0e6cbc8d43796ca26a30db60d65ef4","unresolved":false,"context_lines":[{"line_number":453,"context_line":"        setattr(server, \u0027status\u0027, \u0027MIGRATING\u0027)  # Never reaches ACTIVE"},{"line_number":454,"context_line":""},{"line_number":455,"context_line":"        # Set config value"},{"line_number":456,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":459,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"00b583ca_0b4e83f7","line":456,"in_reply_to":"34f3fc7f_d7947934","updated":"2026-02-09 17:44:09.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":true,"context_lines":[{"line_number":453,"context_line":"        setattr(server, \u0027status\u0027, \u0027MIGRATING\u0027)  # Never reaches ACTIVE"},{"line_number":454,"context_line":""},{"line_number":455,"context_line":"        # Set config value"},{"line_number":456,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":459,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"34f3fc7f_d7947934","line":456,"in_reply_to":"e72a3c2d_4d44f148","updated":"2025-11-20 10:26:16.000000000","message":"Adding the flags function.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":455,"context_line":"        # Set config value"},{"line_number":456,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":459,"context_line":""},{"line_number":460,"context_line":"        # Migration will timeout because status never changes"},{"line_number":461,"context_line":"        is_success \u003d nova_util.watcher_non_live_migrate_instance("},{"line_number":462,"context_line":"            self.instance_uuid, self.destination_node"}],"source_content_type":"text/x-python","patch_set":5,"id":"a48ce4bc_4a917f52","line":459,"range":{"start_line":458,"start_character":8,"end_line":459,"end_character":1},"updated":"2025-11-19 18:43:06.000000000","message":"as an aside i do not paritcaly like the api contract of this helper but its functionl\n\nit does not supprot returnign a list with more the one item...\n\nyou asked before why i dont partically like the untit test and this is a example of why i belive they are poorly designed.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"8d73ab52a217c21339f0029475135755994a11a7","unresolved":true,"context_lines":[{"line_number":455,"context_line":"        # Set config value"},{"line_number":456,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":459,"context_line":""},{"line_number":460,"context_line":"        # Migration will timeout because status never changes"},{"line_number":461,"context_line":"        is_success \u003d nova_util.watcher_non_live_migrate_instance("},{"line_number":462,"context_line":"            self.instance_uuid, self.destination_node"}],"source_content_type":"text/x-python","patch_set":5,"id":"04e0a80e_e95e0188","line":459,"range":{"start_line":458,"start_character":8,"end_line":459,"end_character":1},"in_reply_to":"3267430d_0acafef6","updated":"2025-11-27 15:17:55.000000000","message":"I was not familiar with this method so I looked at it and it has a glaring bug, https://github.com/openstack/watcher/blob/8a884e3d5166678b85eaf264e518ef24b30085e5/watcher/tests/common/test_nova_helper.py#L75 is wrongly using the `list` type instead of the `fake_list` parameter, and so does the `fake_nova_migration_list` https://github.com/openstack/watcher/blob/8a884e3d5166678b85eaf264e518ef24b30085e5/watcher/tests/common/test_nova_helper.py#L87. So calling these methods without `fake_list` will cause the mocked method to return `[None]`. We don\u0027t need to fix this in this patch but we probably should fix it","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":true,"context_lines":[{"line_number":455,"context_line":"        # Set config value"},{"line_number":456,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 25, group\u003d\u0027watcher_applier\u0027)"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":459,"context_line":""},{"line_number":460,"context_line":"        # Migration will timeout because status never changes"},{"line_number":461,"context_line":"        is_success \u003d nova_util.watcher_non_live_migrate_instance("},{"line_number":462,"context_line":"            self.instance_uuid, self.destination_node"}],"source_content_type":"text/x-python","patch_set":5,"id":"3267430d_0acafef6","line":459,"range":{"start_line":458,"start_character":8,"end_line":459,"end_character":1},"in_reply_to":"a48ce4bc_4a917f52","updated":"2025-11-20 10:26:16.000000000","message":"ack","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"568f000c47e62b5288a06df9f58a2ab42dc8fb45","unresolved":true,"context_lines":[{"line_number":466,"context_line":"        self.assertFalse(is_success)"},{"line_number":467,"context_line":"        # With 25 second timeout and 5 second intervals, should sleep 5 times"},{"line_number":468,"context_line":"        # (25/5 \u003d 5 iterations)"},{"line_number":469,"context_line":"        self.assertEqual(5, time.sleep.call_count)"},{"line_number":470,"context_line":""},{"line_number":471,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":472,"context_line":"    def test_watcher_non_live_migrate_instance_retry_explicit("}],"source_content_type":"text/x-python","patch_set":5,"id":"68d99cbd_54c43be1","line":469,"range":{"start_line":469,"start_character":28,"end_line":469,"end_character":32},"updated":"2025-11-19 18:09:14.000000000","message":"by convention you do\n```\n    @mock.patch.object(time, \u0027sleep\u0027)\n        def test_watcher_non_live_migrate_instance_retry_default(\n            self, mock_cinder, mock_nova, mock sleep)\n```\nand then update this to \n    \n```suggestion\n        self.assertEqual(5, mock_sleep.call_count)\n```\n\nif you are going to refence the mock and make asserts\n\nwhen your doing @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())\nyou are pass mock.Mock() as a postional argument to a keyword parmater\n\nhttps://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock\n\npatch.object(target, attribute, new\u003dDEFAULT, spec\u003dNone, create\u003dFalse, spec_set\u003dNone, autospec\u003dNone, new_callable\u003dNone, **kwargs)\n\nthat is an antipattern and it is fragile to tht addtion of new keyword arguemnts\n\nthe fact that other test are doing this is not a good reason to continue the bad partice\n\nyou do not need to fix them but you shoudl not contiue it.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a3d0b31ee197be418487b1a9125c81c1e91895e6","unresolved":false,"context_lines":[{"line_number":466,"context_line":"        self.assertFalse(is_success)"},{"line_number":467,"context_line":"        # With 25 second timeout and 5 second intervals, should sleep 5 times"},{"line_number":468,"context_line":"        # (25/5 \u003d 5 iterations)"},{"line_number":469,"context_line":"        self.assertEqual(5, time.sleep.call_count)"},{"line_number":470,"context_line":""},{"line_number":471,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":472,"context_line":"    def test_watcher_non_live_migrate_instance_retry_explicit("}],"source_content_type":"text/x-python","patch_set":5,"id":"827e948e_70f12675","line":469,"range":{"start_line":469,"start_character":28,"end_line":469,"end_character":32},"in_reply_to":"368913ac_f1b18dcb","updated":"2025-12-02 07:45:13.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":true,"context_lines":[{"line_number":466,"context_line":"        self.assertFalse(is_success)"},{"line_number":467,"context_line":"        # With 25 second timeout and 5 second intervals, should sleep 5 times"},{"line_number":468,"context_line":"        # (25/5 \u003d 5 iterations)"},{"line_number":469,"context_line":"        self.assertEqual(5, time.sleep.call_count)"},{"line_number":470,"context_line":""},{"line_number":471,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":472,"context_line":"    def test_watcher_non_live_migrate_instance_retry_explicit("}],"source_content_type":"text/x-python","patch_set":5,"id":"368913ac_f1b18dcb","line":469,"range":{"start_line":469,"start_character":28,"end_line":469,"end_character":32},"in_reply_to":"68d99cbd_54c43be1","updated":"2025-11-20 10:26:16.000000000","message":"Fixing it.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":495,"context_line":""},{"line_number":496,"context_line":"        # Verify all sleep calls used 5 second interval"},{"line_number":497,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":498,"context_line":"            self.assertEqual(call[0][0], 5)"},{"line_number":499,"context_line":""},{"line_number":500,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":501,"context_line":"    def test_watcher_non_live_migrate_instance_retry_custom_polling("}],"source_content_type":"text/x-python","patch_set":5,"id":"df0fc7be_3b8affd6","line":498,"updated":"2025-11-19 18:43:06.000000000","message":"this test is not required per my other coment","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":false,"context_lines":[{"line_number":495,"context_line":""},{"line_number":496,"context_line":"        # Verify all sleep calls used 5 second interval"},{"line_number":497,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":498,"context_line":"            self.assertEqual(call[0][0], 5)"},{"line_number":499,"context_line":""},{"line_number":500,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":501,"context_line":"    def test_watcher_non_live_migrate_instance_retry_custom_polling("}],"source_content_type":"text/x-python","patch_set":5,"id":"6c1f76e4_3c4f0131","line":498,"in_reply_to":"df0fc7be_3b8affd6","updated":"2025-11-20 10:26:16.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":525,"context_line":""},{"line_number":526,"context_line":"        # Verify all sleep calls used 3 second interval"},{"line_number":527,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":528,"context_line":"            self.assertEqual(call[0][0], 3)"},{"line_number":529,"context_line":""},{"line_number":530,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":531,"context_line":"    def test_live_migrate_instance_retry_default("}],"source_content_type":"text/x-python","patch_set":5,"id":"88d89020_a654ee24","line":528,"updated":"2025-11-19 18:43:06.000000000","message":"ack\n\nthis all makes sense.\n\nsame comment on setattr but thet rest is mostly fine.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a3d0b31ee197be418487b1a9125c81c1e91895e6","unresolved":false,"context_lines":[{"line_number":525,"context_line":""},{"line_number":526,"context_line":"        # Verify all sleep calls used 3 second interval"},{"line_number":527,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":528,"context_line":"            self.assertEqual(call[0][0], 3)"},{"line_number":529,"context_line":""},{"line_number":530,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":531,"context_line":"    def test_live_migrate_instance_retry_default("}],"source_content_type":"text/x-python","patch_set":5,"id":"4c85237c_1d3ba8c6","line":528,"in_reply_to":"88d89020_a654ee24","updated":"2025-12-02 07:45:13.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":527,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":528,"context_line":"            self.assertEqual(call[0][0], 3)"},{"line_number":529,"context_line":""},{"line_number":530,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":531,"context_line":"    def test_live_migrate_instance_retry_default("},{"line_number":532,"context_line":"            self, mock_cinder, mock_nova):"},{"line_number":533,"context_line":"        \"\"\"Test that live_migrate_instance uses config timeout by default\"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"135e0317_5b5a154b","line":530,"range":{"start_line":530,"start_character":3,"end_line":530,"end_character":50},"updated":"2025-11-19 18:43:06.000000000","message":"you are effectivly mockign this alwasy\n\nyou proably shoudl add self.time_mock \u003d self.useFixture(...) in the seupt fucntion \n\nso that you do not need to keep mockign this on every test.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":false,"context_lines":[{"line_number":527,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":528,"context_line":"            self.assertEqual(call[0][0], 3)"},{"line_number":529,"context_line":""},{"line_number":530,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":531,"context_line":"    def test_live_migrate_instance_retry_default("},{"line_number":532,"context_line":"            self, mock_cinder, mock_nova):"},{"line_number":533,"context_line":"        \"\"\"Test that live_migrate_instance uses config timeout by default\"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"d59543fe_1ad55262","line":530,"range":{"start_line":530,"start_character":3,"end_line":530,"end_character":50},"in_reply_to":"135e0317_5b5a154b","updated":"2025-11-20 10:26:16.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":537,"context_line":"        setattr(server, \u0027OS-EXT-STS:task_state\u0027, \u0027migrating\u0027)"},{"line_number":538,"context_line":""},{"line_number":539,"context_line":"        # Set config value"},{"line_number":540,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 30, group\u003d\u0027watcher_applier\u0027)"},{"line_number":541,"context_line":""},{"line_number":542,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":543,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"5db25742_046e495d","line":540,"updated":"2025-11-19 18:43:06.000000000","message":"so for test_live_migrate_instance_retry_default i waosul expect to use the config default","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a3d0b31ee197be418487b1a9125c81c1e91895e6","unresolved":false,"context_lines":[{"line_number":537,"context_line":"        setattr(server, \u0027OS-EXT-STS:task_state\u0027, \u0027migrating\u0027)"},{"line_number":538,"context_line":""},{"line_number":539,"context_line":"        # Set config value"},{"line_number":540,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 30, group\u003d\u0027watcher_applier\u0027)"},{"line_number":541,"context_line":""},{"line_number":542,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":543,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"a3d71b7d_137fdb22","line":540,"in_reply_to":"5db25742_046e495d","updated":"2025-12-02 07:45:13.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":563,"context_line":""},{"line_number":564,"context_line":"        # Set config value"},{"line_number":565,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 30, group\u003d\u0027watcher_applier\u0027)"},{"line_number":566,"context_line":"        CONF.set_override(\u0027migrate_polling\u0027, 1.5, group\u003d\u0027watcher_applier\u0027)"},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":569,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"4c66eab1_c0c5f7fa","line":566,"updated":"2025-11-19 18:43:06.000000000","message":"and then here you can test custom timeous and pooling interval.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a3d0b31ee197be418487b1a9125c81c1e91895e6","unresolved":false,"context_lines":[{"line_number":563,"context_line":""},{"line_number":564,"context_line":"        # Set config value"},{"line_number":565,"context_line":"        CONF.set_override(\u0027migrate_timeout\u0027, 30, group\u003d\u0027watcher_applier\u0027)"},{"line_number":566,"context_line":"        CONF.set_override(\u0027migrate_polling\u0027, 1.5, group\u003d\u0027watcher_applier\u0027)"},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"        self.fake_nova_find_list(nova_util, fake_find\u003dserver, fake_list\u003dserver)"},{"line_number":569,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"c0942ac0_fe157b83","line":566,"in_reply_to":"4c66eab1_c0c5f7fa","updated":"2025-12-02 07:45:13.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":609,"context_line":""},{"line_number":610,"context_line":"        # Verify all sleep calls used 5 second interval"},{"line_number":611,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":612,"context_line":"            self.assertEqual(call[0][0], 5)"},{"line_number":613,"context_line":""},{"line_number":614,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":615,"context_line":"    def test_live_migrate_instance_no_dest_retry_default("}],"source_content_type":"text/x-python","patch_set":5,"id":"bd7c4d64_ac16c445","line":612,"updated":"2025-11-19 18:43:06.000000000","message":"you can remove this if you remove the retry parmater since it never passed in production code.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":false,"context_lines":[{"line_number":609,"context_line":""},{"line_number":610,"context_line":"        # Verify all sleep calls used 5 second interval"},{"line_number":611,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":612,"context_line":"            self.assertEqual(call[0][0], 5)"},{"line_number":613,"context_line":""},{"line_number":614,"context_line":"    @mock.patch.object(time, \u0027sleep\u0027, mock.Mock())"},{"line_number":615,"context_line":"    def test_live_migrate_instance_no_dest_retry_default("}],"source_content_type":"text/x-python","patch_set":5,"id":"927df7d8_214a5507","line":612,"in_reply_to":"bd7c4d64_ac16c445","updated":"2025-11-20 10:26:16.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":false,"context_lines":[{"line_number":627,"context_line":""},{"line_number":628,"context_line":"        # Migration with no destination will timeout"},{"line_number":629,"context_line":"        is_success \u003d nova_util.live_migrate_instance("},{"line_number":630,"context_line":"            self.instance_uuid, None"},{"line_number":631,"context_line":"        )"},{"line_number":632,"context_line":""},{"line_number":633,"context_line":"        # Should fail due to timeout"}],"source_content_type":"text/x-python","patch_set":5,"id":"d9e150c6_6eae95ce","line":630,"range":{"start_line":630,"start_character":32,"end_line":630,"end_character":36},"updated":"2025-11-19 18:43:06.000000000","message":"this si corect as of today but this shoudl be a keywrod arguemnt if we are somethime passsing noe to it.\n\n\nbased on the fucntion signiture you shoud nto eneed to check for None here\n\nhttps://github.com/openstack/watcher/blob/8a884e3d5166678b85eaf264e518ef24b30085e5/watcher/common/nova_helper.py#L303-L340\n\nalthouhg there is more egreious issue with the code\nhttps://github.com/openstack/watcher/blob/8a884e3d5166678b85eaf264e518ef24b30085e5/watcher/common/nova_helper.py#L338-L379\n\nso im not suprised.\n\nthe test is valid the code just make me sad.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"118ec5d4d730068285c41bce50cb314fc3de1ede","unresolved":true,"context_lines":[{"line_number":639,"context_line":"        # Verify all sleep calls used 5 second interval"},{"line_number":640,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":641,"context_line":"            self.assertEqual(call[0][0], 5)"},{"line_number":642,"context_line":""},{"line_number":643,"context_line":"    def test_enable_service_nova_compute(self, mock_cinder, mock_nova):"},{"line_number":644,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":645,"context_line":"        nova_services \u003d nova_util.nova.services"}],"source_content_type":"text/x-python","patch_set":5,"id":"e134df79_80e1ac0c","line":642,"updated":"2025-11-19 18:43:06.000000000","message":"general comment, you didnt add any happy path test wher ethe time does not expire.\n\nto do that instead of using self.fake_nova_find_list\nyou woud need to do this\n\n\nupdated_server\u003d copy.deepCopy(server)\nupdate_server.status\u003d\u0027ACTIVE\u0027\nnova_util.nova.servers.get.side_efect \u003d (server, server, updated_server)\n\n\nhttps://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.side_effect\n\nonce we implemnt fucntionl testing this woudl be doen as a proper fucntiol test and we woudl properly emulate the state trasnation by modifying the state of the instnace object in the fixture when we call migrate.","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a3d0b31ee197be418487b1a9125c81c1e91895e6","unresolved":false,"context_lines":[{"line_number":639,"context_line":"        # Verify all sleep calls used 5 second interval"},{"line_number":640,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":641,"context_line":"            self.assertEqual(call[0][0], 5)"},{"line_number":642,"context_line":""},{"line_number":643,"context_line":"    def test_enable_service_nova_compute(self, mock_cinder, mock_nova):"},{"line_number":644,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":645,"context_line":"        nova_services \u003d nova_util.nova.services"}],"source_content_type":"text/x-python","patch_set":5,"id":"84aae0cf_39cdc06c","line":642,"in_reply_to":"ca2f47ad_b2d719b1","updated":"2025-12-02 07:45:13.000000000","message":"Done","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"225db053aa947c4bf3626bfb0b2cf2e998671cc3","unresolved":true,"context_lines":[{"line_number":639,"context_line":"        # Verify all sleep calls used 5 second interval"},{"line_number":640,"context_line":"        for call in time.sleep.call_args_list:"},{"line_number":641,"context_line":"            self.assertEqual(call[0][0], 5)"},{"line_number":642,"context_line":""},{"line_number":643,"context_line":"    def test_enable_service_nova_compute(self, mock_cinder, mock_nova):"},{"line_number":644,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":645,"context_line":"        nova_services \u003d nova_util.nova.services"}],"source_content_type":"text/x-python","patch_set":5,"id":"ca2f47ad_b2d719b1","line":642,"in_reply_to":"e134df79_80e1ac0c","updated":"2025-11-20 10:26:16.000000000","message":"Right, the pre-existing happy path tests are not simulating changing the state of the server. I will add one for live and one for non_live","commit_id":"9e002590744d73923a31bf145aa9de84bf019178"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"8d73ab52a217c21339f0029475135755994a11a7","unresolved":true,"context_lines":[{"line_number":468,"context_line":""},{"line_number":469,"context_line":"        self.flags(migration_max_retries\u003d20, migration_interval\u003d4,"},{"line_number":470,"context_line":"                   group\u003d\u0027nova\u0027)"},{"line_number":471,"context_line":"        # Migration will timeout because status never changes"},{"line_number":472,"context_line":"        is_success \u003d nova_util.watcher_non_live_migrate_instance("},{"line_number":473,"context_line":"            self.instance_uuid, self.destination_node"},{"line_number":474,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":10,"id":"ca4b79e3_ec9b1087","line":471,"updated":"2025-11-27 15:17:55.000000000","message":"this comment seems wrong, IIUC in this test the migration will succeed, not timeout","commit_id":"4d0d81ee5abe29e857d1a338a0d6d7c6b7297244"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"6332aac744b0cee240f804f23a2f9277a2f357be","unresolved":false,"context_lines":[{"line_number":468,"context_line":""},{"line_number":469,"context_line":"        self.flags(migration_max_retries\u003d20, migration_interval\u003d4,"},{"line_number":470,"context_line":"                   group\u003d\u0027nova\u0027)"},{"line_number":471,"context_line":"        # Migration will timeout because status never changes"},{"line_number":472,"context_line":"        is_success \u003d nova_util.watcher_non_live_migrate_instance("},{"line_number":473,"context_line":"            self.instance_uuid, self.destination_node"},{"line_number":474,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":10,"id":"54a822c2_58a56008","line":471,"in_reply_to":"ca4b79e3_ec9b1087","updated":"2025-11-27 16:36:39.000000000","message":"Done","commit_id":"4d0d81ee5abe29e857d1a338a0d6d7c6b7297244"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"8d73ab52a217c21339f0029475135755994a11a7","unresolved":true,"context_lines":[{"line_number":548,"context_line":"        nova_util.nova.servers.get.side_effect \u003d ("},{"line_number":549,"context_line":"            server, server, server, migrated_server)"},{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        # Migration will timeout because host never changes"},{"line_number":552,"context_line":"        is_success \u003d nova_util.live_migrate_instance("},{"line_number":553,"context_line":"            self.instance_uuid, self.destination_node"},{"line_number":554,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":10,"id":"0b6c0150_62b9907c","line":551,"updated":"2025-11-27 15:17:55.000000000","message":"this comment is also wrong","commit_id":"4d0d81ee5abe29e857d1a338a0d6d7c6b7297244"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"6332aac744b0cee240f804f23a2f9277a2f357be","unresolved":false,"context_lines":[{"line_number":548,"context_line":"        nova_util.nova.servers.get.side_effect \u003d ("},{"line_number":549,"context_line":"            server, server, server, migrated_server)"},{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        # Migration will timeout because host never changes"},{"line_number":552,"context_line":"        is_success \u003d nova_util.live_migrate_instance("},{"line_number":553,"context_line":"            self.instance_uuid, self.destination_node"},{"line_number":554,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":10,"id":"3ddab93e_bf1e276c","line":551,"in_reply_to":"0b6c0150_62b9907c","updated":"2025-11-27 16:36:39.000000000","message":"Done","commit_id":"4d0d81ee5abe29e857d1a338a0d6d7c6b7297244"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"453b58ad280ccb5cbea6e244c74a6aa4aad236bb","unresolved":true,"context_lines":[{"line_number":507,"context_line":"        for call in self.mock_sleep.call_args_list:"},{"line_number":508,"context_line":"            self.assertEqual(call[0][0], 5)"},{"line_number":509,"context_line":""},{"line_number":510,"context_line":"    def test_watcher_non_live_migrate_instance_retry_custom("},{"line_number":511,"context_line":"            self, mock_cinder, mock_nova):"},{"line_number":512,"context_line":"        \"\"\"Test that watcher_non_live_migrate respects explicit retry value\"\"\""},{"line_number":513,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"}],"source_content_type":"text/x-python","patch_set":11,"id":"35f70c13_ce07f0bd","line":510,"range":{"start_line":510,"start_character":8,"end_line":510,"end_character":59},"updated":"2025-11-28 18:25:38.000000000","message":"when tests are similar, you could try use ddt, to avoid duplicating code.","commit_id":"b1d2ac84bb54fae642921ec2b62dd9bd8f4239b1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"00c2a34edf6e1df6b4f5860087f6b1f3a1ad17f0","unresolved":true,"context_lines":[{"line_number":454,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":455,"context_line":"        server \u003d self.fake_server(self.instance_uuid)"},{"line_number":456,"context_line":"        server.status \u003d \u0027MIGRATING\u0027  # Never reaches ACTIVE"},{"line_number":457,"context_line":"        setattr(server, \u0027OS-EXT-SRV-ATTR:host\u0027, self.source_node)"},{"line_number":458,"context_line":""},{"line_number":459,"context_line":"        verify_server \u003d copy.deepcopy(server)"},{"line_number":460,"context_line":"        verify_server.status \u003d \u0027VERIFY_RESIZE\u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"6c1a5a0d_16a92d91","line":457,"range":{"start_line":457,"start_character":6,"end_line":457,"end_character":64},"updated":"2025-12-02 10:44:10.000000000","message":"you didnt actully adress this but we can move forward for now.\n\ni want use to stop using setattr in all new code including tests.\n\nit shoudl always be a last resort","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"d7b3409cee0e6cbc8d43796ca26a30db60d65ef4","unresolved":false,"context_lines":[{"line_number":454,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":455,"context_line":"        server \u003d self.fake_server(self.instance_uuid)"},{"line_number":456,"context_line":"        server.status \u003d \u0027MIGRATING\u0027  # Never reaches ACTIVE"},{"line_number":457,"context_line":"        setattr(server, \u0027OS-EXT-SRV-ATTR:host\u0027, self.source_node)"},{"line_number":458,"context_line":""},{"line_number":459,"context_line":"        verify_server \u003d copy.deepcopy(server)"},{"line_number":460,"context_line":"        verify_server.status \u003d \u0027VERIFY_RESIZE\u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"b329e076_a831489c","line":457,"range":{"start_line":457,"start_character":6,"end_line":457,"end_character":64},"in_reply_to":"6c1a5a0d_16a92d91","updated":"2026-02-09 17:44:09.000000000","message":"Acknowledged","commit_id":"13d73e9b4e64ac9fea8e301896a4dcabbbab4888"}]}