)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"dfd98162_0ab06cee","updated":"2025-06-17 11:21:43.000000000","message":"overall the direction looks good.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":5,"id":"bd1639f6_033ae27a","updated":"2025-06-18 14:24:25.000000000","message":"Hi Sean. Thank you so much for your detailed and thoughtful review! As a fresher and having the first time doing such contribution for open-source, I really value such feedback and study from others\u0027insights.\nI have made some changes and left some questions. Hope to learn more!","commit_id":"19a3f7592a878597c88ab387bd0808ae1e820608"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ee7f6671e6964ef2bcbe594672c2926de3dccfb6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"3b09fd4d_b58c956d","updated":"2025-06-26 01:16:47.000000000","message":"Hi Sean, hope you\u0027re doing well.\n\nI made some changes and replied to some of your comments. I would love to hear your feedback to push the progress further. Thank you so much!\n\nRegards,\nQuang","commit_id":"35084dc9e704f4d4a8e97ad1e32dc779595a2433"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"00aeca187c54cdcbdaddd1aa1a8e57adedddb4a1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"a8e97029_d7c3da11","updated":"2025-06-26 12:34:46.000000000","message":"The implementation looks good, but I think this needs more testing, at the very least we should cover the newly added parameters","commit_id":"35084dc9e704f4d4a8e97ad1e32dc779595a2433"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"13536bb0f9348d57170a7581c9c2b0bf7f2546b0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"686ca110_dc18c998","in_reply_to":"a8e97029_d7c3da11","updated":"2025-06-27 06:15:15.000000000","message":"Thanks for your feedback! I\u0027ve added some tests for test_host_maintenance.py to verify the new parameters.","commit_id":"35084dc9e704f4d4a8e97ad1e32dc779595a2433"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"13536bb0f9348d57170a7581c9c2b0bf7f2546b0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"a2b50621_2f6b3e09","updated":"2025-06-27 06:15:15.000000000","message":"Hi,\n\nSince the implementation is considered to be good, I\u0027ve just added several tests for the new stop action and the host maintenance strategy, mostly to verify it can cover the newly added parameters. \n\nLooking forward for your reviews regarding this. Please let me know if further implementation \u0026 testings should be done, or any modification is needed. For now, I will also start to do some local testing with my OpenStack cloud. \n\nRegards,\nQuang","commit_id":"71f8f7ddae95eeb643bfb5e9eb3d9f69e5fcf13f"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"5cc6058c9acdb99404b1c57e29ef9a077da32ee7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"9f28b23b_d41c0629","updated":"2025-06-27 11:23:29.000000000","message":"thanks for the tests! I left a comment about mocking in the stop action tests, and we should also add some testing for the `start_instance` method of the NovaHelper","commit_id":"71f8f7ddae95eeb643bfb5e9eb3d9f69e5fcf13f"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"d32ba5fe444279f9b186a139c09cc52ffbcc845a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"7d3bd068_c5ce2d00","updated":"2025-07-03 15:42:55.000000000","message":"Hi,\nThanks so much for your reviews. That\u0027s interesting and helpful for me to learn more about writing good test.\n\nI\u0027ve fixed the stop test as recommended. I also add a test for `start_instance` in TestNovaHelper, basically it\u0027s just a mirror of the existing `stop_instance` test.\n \nRecently I\u0027ve tried to deploy the current Watcher on my OpenStack deployment and do some checks, and find out it works quite as expected. There is a small issue that the action return a failed state when trying to do stop on a paused instance. Therefore I add a small condition for the `instance_handle` method in host maintenance, so now the stop action is only applied to active instances. I also verified back on my OpenStack and now no errors so far.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"552ce67deca9f860eb3a218d8b5c5e9c65cfab7e","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"d05beaf6_26aeea7a","updated":"2025-07-03 16:24:40.000000000","message":"In general this is moving in a good direction.\n\nI have not had time to review the test today but i have left some comments inline.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"14cfd69ffc0a2f39300d2eda30cf207661bc48e9","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"b1f3caab_6a801e86","in_reply_to":"d05beaf6_26aeea7a","updated":"2025-07-04 04:55:39.000000000","message":"Tysm!","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"14cfd69ffc0a2f39300d2eda30cf207661bc48e9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"f3549ef7_703d7f11","updated":"2025-07-04 04:55:39.000000000","message":"Thanks for the feedback!\n\nI\u0027ve applied the suggestions, make some changes for the table in `host_maintenance.rst` to match the current master branch, and left a comment for the revert in stop action to discuss further.\n\nThank you!","commit_id":"1a41bdd15ffc0048d63912d59d42d3c12ae6060f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4eddb2bef5f604baad6542cbb2bc56d06c8a95b2","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":10,"id":"b13fda11_9a93d91f","updated":"2025-07-04 11:51:41.000000000","message":"one ohter general comment\ncan you add a feature release note for this in a future revision.","commit_id":"1a41bdd15ffc0048d63912d59d42d3c12ae6060f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"eb21c241b65a3af27f5f610b67623ef50296c4d9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"5767f449_7d01f07f","updated":"2025-07-04 11:50:38.000000000","message":"you have addressed most of my feedback form my last review or responded with an explanation why \n\nim still +1 on this mainly because i have not had time to do a deep review of the test coverage.\n\nhave you started on tempest testing?\n\nit would be nice if there was a watcher-tempest-plugin patch that added a new testcase that used these new options via depends-on and we could demonstrate this working end to end.","commit_id":"1a41bdd15ffc0048d63912d59d42d3c12ae6060f"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"7bf52b8c632dff40422851313122a3be3993ecb6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"c64dd3a9_77edcd9f","in_reply_to":"5767f449_7d01f07f","updated":"2025-07-07 00:51:00.000000000","message":"Thanks so much for your feedback.\n\nI\u0027m not very familiar with tempest, so haven\u0027t really started on it though. I did the implementation with my custom Watcher image on my local OpenStack deployment, created several instances and apply the strategy with/without the new parameters, and it fully worked as expected. \n\nHowever it\u0027s a nice chance to learn more about tempest!\n\nI will start to learn and work on the patch for watcher-tempest-plugin and do the testing with tempest. Hope I can work it out soon.\n\nThank you!","commit_id":"1a41bdd15ffc0048d63912d59d42d3c12ae6060f"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"7bf52b8c632dff40422851313122a3be3993ecb6","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":10,"id":"64b4c379_e4a87e8f","in_reply_to":"b13fda11_9a93d91f","updated":"2025-07-07 00:51:00.000000000","message":"I have added the release note, hope you find it ok. Thank you!","commit_id":"1a41bdd15ffc0048d63912d59d42d3c12ae6060f"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"eb68442d1ffe503a9bb3e40d92533cc334a21f0a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"f2b6c570_4b51d3b6","updated":"2025-07-28 00:18:10.000000000","message":"Hi @smooney@redhat.com, it\u0027s been some times, so I would like to have some update about the status for this. \nAlso, I would like to ask whether it is possible for this feature to appear in  2025.02 release?\nThanks!","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"c990a4663a58452946305308c1f6d1042fd8d9f3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"01ccbb3f_7b64667e","updated":"2025-07-09 00:52:09.000000000","message":"Hi,\n\nThank you for the review and all your help to push the progress.\n\nI have a quick question for Ubuntu SRU: would this feature be acceptable for backport to any current stable branches (epoxy, caracal, etc.)?\n\nI expect the answer is no per OpenStack stable branch policy, but Ubuntu still\nrequires upstream confirmation for the SRU decision.\n\nThanks,","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"4dbbe85a0ed0cdd4f06b37d7372d478255b11ff3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"b386b1d1_b584aba4","updated":"2025-07-07 08:40:06.000000000","message":"Hi, I also update the patch to watcher-tempest-plugin here: \nhttps://review.opendev.org/c/openstack/watcher-tempest-plugin/+/954214\n\nHope it could be taken a look. Thank you so much!","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"115eb211_d09113b3","updated":"2025-08-06 20:57:36.000000000","message":"I think that we should update the post_condition implementation, based on discussions, unless there is a reason for checking the status again.\nSome other comments that we can discuss, like the instance not found scenario?\nThanks for your submission","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"7bf52b8c632dff40422851313122a3be3993ecb6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"b05ea2d6_0ec2f598","updated":"2025-07-07 00:51:00.000000000","message":"Thank you for the reviews.\n\nI have added the release note, and now will start looking at the tempest testing.\n\nThanks!","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"aad48f2f_6a74fb56","updated":"2025-08-09 09:55:52.000000000","message":"Thanks for the careful review!\n\nLooks like quite a lot need to be done, so I make quite considerable changes, and reply to the comments.\n\nHope to push this further and get more discussion!","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"fa21c16259337f6bae95b45e79d3a087fedddef1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"f8def852_85bb527b","updated":"2025-07-14 15:06:41.000000000","message":"Thanks for the feedback.\nI have given my thoughts to some questions. \nHi @smooney@redhat.com, there are some new questions and updates since your last review, so I would love to hear your thoughts as well. Thanks!","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"77b1d6cb4b739e693323e24377fcb4f6fd3a9c61","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"c8e7a2ce_d401ec65","updated":"2025-07-14 18:23:44.000000000","message":"i was on pto on friday and im still catching up with email/review so ill try and find time to do a proper end to end review of this code this week. \nfor now i just replied to the qustion inline.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"4c88fff071ad89728b9cf1ab0b488c83b52ee3aa","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"91e00c75_6ed5dee7","updated":"2025-07-08 15:03:09.000000000","message":"lgtm, thanks!","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":28647,"name":"David","display_name":"morenod","email":"dsanzmor@redhat.com","username":"morenod"},"change_message_id":"b78ac12f6e91a63b28596497dbcc798978a5ae52","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"18f08c7d_84cc21b6","updated":"2025-07-17 08:54:12.000000000","message":"recheck","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":12393,"name":"chandan kumar","display_name":"Chandan Kumar","email":"chkumar@redhat.com","username":"chkumar246"},"change_message_id":"87bc91a1b6f4564d94ea089fc5881580ea6059fa","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":11,"id":"a0da22ee_a44a9ce0","updated":"2025-07-18 06:12:31.000000000","message":"recheck after skipping tempest tests https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/955302","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"f5b0276ea72c470b7ef4e78926a289903e63b14f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"81fd4fa5_346d48a6","in_reply_to":"c8e7a2ce_d401ec65","updated":"2025-07-16 01:38:04.000000000","message":"Got it, please take your time. Thanks!","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"08de285bd56ed5f4d0d47e722642608783728281","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"92bb7a28_e6db8cd4","updated":"2025-08-10 04:00:07.000000000","message":"recheck","commit_id":"12fae0dcfe17cde0f4c5cc7bb8968f3b8ff6fa8b"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"d4c69a31dbe5bcd36e51d4c9d5df265f1b69c847","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"b4a67fc9_21634358","updated":"2025-08-18 17:43:38.000000000","message":"I think that is in a good shape, but I don\u0027t see a reason to change the post_condition at this moment, since we never discussed about that. Waiting for more feedbacks from reviewers, thanks!","commit_id":"23080cd3a1828cf7deae5788543295e02f6b5211"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"f40d2887d0b1d5c942f835302d3b33a132e0d76a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"9ee7ba96_583813db","updated":"2025-08-15 06:40:30.000000000","message":"Some small fixes in the document.Thanks!","commit_id":"23080cd3a1828cf7deae5788543295e02f6b5211"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ba7e730aa69e68562edd06b98ce6cef8fc993852","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"9fb27103_fabba548","updated":"2025-08-19 17:03:39.000000000","message":"i think i would be more or less ok with merging this as it is or with the refactor doug is asking for.\n\nfor now +1 as i want to review the tempest tests before froming an opion on +2 or not and i also want ot let doug respond.","commit_id":"23080cd3a1828cf7deae5788543295e02f6b5211"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"c6f64b41a191981b88a6dd9b995180325db3b1e2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"d69477d3_08d722c4","updated":"2025-08-18 13:58:05.000000000","message":"thanks for the work! I think this is good to go, I just spotted a tiny mistake in the docs","commit_id":"23080cd3a1828cf7deae5788543295e02f6b5211"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"be508dc87be64732b08d926e1b1d707e4654a67a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"6d3de170_f7fc1e75","updated":"2025-08-20 16:46:58.000000000","message":"+1: waiting tempest change to run again with latest PS. After that I will update to +2.\nThanks for updating the patch.","commit_id":"cc26b3b334e5d60bf04c927c771d572445e4a8bc"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"a99e42fbdf0a299b0cd6e6acb16d5053973de7f7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"f06b9813_ccf75dd4","updated":"2025-08-20 18:42:49.000000000","message":"LGTM, tempest test results:\nhttps://64d047e0a962fd5a15d9-b43d69092aed97a8232ffa660b1a59e6.ssl.cf1.rackcdn.com/openstack/3a26ba5f22014693b7e86fc056ec5248/testr_results.html","commit_id":"cc26b3b334e5d60bf04c927c771d572445e4a8bc"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"d39318b46ae7013bb868862257e4876e5204c2c2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"3c70832e_573e34ee","updated":"2025-08-20 12:40:54.000000000","message":"Thanks for many feedback!\nSo after several discussion, I think now we agree to keep the previous way the `post_condition()` is implemented to avoid further technical debt, and think of redesign in another scope. So I have reverted all the relevant changes, and just give a `pass` for the `post_condition()` in the stop action.","commit_id":"cc26b3b334e5d60bf04c927c771d572445e4a8bc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2367b54b8646585c3815ae6c1ef4c6c5e7d46774","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"a3f5ae50_f7fbe138","updated":"2025-08-21 13:34:25.000000000","message":"thanks there are some follow ups we could do but im happy to proceed with this as is for this cycle.","commit_id":"cc26b3b334e5d60bf04c927c771d572445e4a8bc"}],"doc/source/strategies/host_maintenance.rst":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":67,"context_line":"                                                  maintenance node. Optional."},{"line_number":68,"context_line":"``disable_live_migration`` Boolean  False         False: Active instances will"},{"line_number":69,"context_line":"                                                  be live migrated."},{"line_number":70,"context_line":"                                                  True: Active instances will"},{"line_number":71,"context_line":"                                                  be stopped. If"},{"line_number":72,"context_line":"                                                  ``disable_cold_migration``"},{"line_number":73,"context_line":"                                                  is not set, inactive"},{"line_number":74,"context_line":"                                                  instances including newly"}],"source_content_type":"text/x-rst","patch_set":4,"id":"939aa470_3437825f","line":71,"range":{"start_line":70,"start_character":56,"end_line":71,"end_character":61},"updated":"2025-06-17 11:21:43.000000000","message":"this is not correct. we shoudl not be stopping the isntnace.\n\ncold migration in nova can be used on an active instance and it will restore the instance to the active state when the cold migration is complete.\n\nwe should not modify the instnace statuse before calling cold migrate.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":67,"context_line":"                                                  maintenance node. Optional."},{"line_number":68,"context_line":"``disable_live_migration`` Boolean  False         False: Active instances will"},{"line_number":69,"context_line":"                                                  be live migrated."},{"line_number":70,"context_line":"                                                  True: Active instances will"},{"line_number":71,"context_line":"                                                  be stopped. If"},{"line_number":72,"context_line":"                                                  ``disable_cold_migration``"},{"line_number":73,"context_line":"                                                  is not set, inactive"},{"line_number":74,"context_line":"                                                  instances including newly"}],"source_content_type":"text/x-rst","patch_set":4,"id":"42074371_92951916","line":71,"range":{"start_line":70,"start_character":56,"end_line":71,"end_character":61},"in_reply_to":"939aa470_3437825f","updated":"2025-06-18 14:24:25.000000000","message":"Done","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"552ce67deca9f860eb3a218d8b5c5e9c65cfab7e","unresolved":true,"context_lines":[{"line_number":73,"context_line":"                                                  True: Inactive instances"},{"line_number":74,"context_line":"                                                  will not be cold migrated."},{"line_number":75,"context_line":"                                                  Optional."},{"line_number":76,"context_line":"\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"Efficacy Indicator"},{"line_number":79,"context_line":"------------------"}],"source_content_type":"text/x-rst","patch_set":9,"id":"147c85a8_42e9c1c4","line":76,"updated":"2025-07-03 16:24:40.000000000","message":"-1 We have changed the content of this table in parallel \n\nwe can have default be we need the required colume too.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"14cfd69ffc0a2f39300d2eda30cf207661bc48e9","unresolved":true,"context_lines":[{"line_number":73,"context_line":"                                                  True: Inactive instances"},{"line_number":74,"context_line":"                                                  will not be cold migrated."},{"line_number":75,"context_line":"                                                  Optional."},{"line_number":76,"context_line":"\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"Efficacy Indicator"},{"line_number":79,"context_line":"------------------"}],"source_content_type":"text/x-rst","patch_set":9,"id":"f4a57968_e09282cf","line":76,"in_reply_to":"147c85a8_42e9c1c4","updated":"2025-07-04 04:55:39.000000000","message":"I\u0027ve made some changes to the table. To adapt the line length check, I removed the default column and mentioned in the description instead, and also changed the column title `required/optional` to be `required` only (hope it still makes sense)","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"eb21c241b65a3af27f5f610b67623ef50296c4d9","unresolved":false,"context_lines":[{"line_number":73,"context_line":"                                                  True: Inactive instances"},{"line_number":74,"context_line":"                                                  will not be cold migrated."},{"line_number":75,"context_line":"                                                  Optional."},{"line_number":76,"context_line":"\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"Efficacy Indicator"},{"line_number":79,"context_line":"------------------"}],"source_content_type":"text/x-rst","patch_set":9,"id":"ca695061_7fae62e6","line":76,"in_reply_to":"f4a57968_e09282cf","updated":"2025-07-04 11:50:38.000000000","message":"ack, ya so what i think we are going to do in the medium term is convert this to either using one of sphinxs native table format or mor likely we will port\n\nhttps://github.com/openstack/nova/blob/master/doc/ext/feature_matrix.py\n\nor write our own so we can do this properly.\n\n\nmanually formating the table like this is not really correct espaically when we condier how this will render in pdf format ectra so im happy with the way you have adressed this","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":110,"context_line":"      -g cluster_maintaining -s host_maintenance \\"},{"line_number":111,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":112,"context_line":"      -p backup_node\u003dcompute02 \\"},{"line_number":113,"context_line":"      -p disable_live_migration\u003dTrue \\"},{"line_number":114,"context_line":"      -p disable_cold_migration\u003dTrue \\"},{"line_number":115,"context_line":"      --auto-trigger"},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"Note that after executing this strategy, the *maintenance_node* will be"}],"source_content_type":"text/x-rst","patch_set":11,"id":"6d8a5f27_e9a03c5f","line":114,"range":{"start_line":113,"start_character":0,"end_line":114,"end_character":38},"updated":"2025-08-06 20:57:36.000000000","message":"You should create a new example with the new parameters. The description of this example mentions that servers will move from compute01 to compute02. By disabling both migrations here, anything will be migrated.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":false,"context_lines":[{"line_number":110,"context_line":"      -g cluster_maintaining -s host_maintenance \\"},{"line_number":111,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":112,"context_line":"      -p backup_node\u003dcompute02 \\"},{"line_number":113,"context_line":"      -p disable_live_migration\u003dTrue \\"},{"line_number":114,"context_line":"      -p disable_cold_migration\u003dTrue \\"},{"line_number":115,"context_line":"      --auto-trigger"},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"Note that after executing this strategy, the *maintenance_node* will be"}],"source_content_type":"text/x-rst","patch_set":11,"id":"4bb9645c_0c34b65d","line":114,"range":{"start_line":113,"start_character":0,"end_line":114,"end_character":38},"in_reply_to":"6d8a5f27_e9a03c5f","updated":"2025-08-09 09:55:52.000000000","message":"Done","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"c95b05b4b70cee65984a1f7da3af08abbdda68dc","unresolved":true,"context_lines":[{"line_number":57,"context_line":"\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"},{"line_number":58,"context_line":"``maintenance_node``       String   The name of the            Required"},{"line_number":59,"context_line":"                                    compute node"},{"line_number":60,"context_line":"                                    which need maintenance."},{"line_number":61,"context_line":"``backup_node``            String   The name of the compute    Optional"},{"line_number":62,"context_line":"                                    node which will backup"},{"line_number":63,"context_line":"                                    the maintenance node."}],"source_content_type":"text/x-rst","patch_set":12,"id":"3b8b6b04_71ca3ecd","line":60,"updated":"2025-08-14 14:56:48.000000000","message":"typo\n```suggestion\n                                    which needs maintenance.\n```","commit_id":"12fae0dcfe17cde0f4c5cc7bb8968f3b8ff6fa8b"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"f40d2887d0b1d5c942f835302d3b33a132e0d76a","unresolved":false,"context_lines":[{"line_number":57,"context_line":"\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"},{"line_number":58,"context_line":"``maintenance_node``       String   The name of the            Required"},{"line_number":59,"context_line":"                                    compute node"},{"line_number":60,"context_line":"                                    which need maintenance."},{"line_number":61,"context_line":"``backup_node``            String   The name of the compute    Optional"},{"line_number":62,"context_line":"                                    node which will backup"},{"line_number":63,"context_line":"                                    the maintenance node."}],"source_content_type":"text/x-rst","patch_set":12,"id":"e72fe3b8_f6450ca0","line":60,"in_reply_to":"3b8b6b04_71ca3ecd","updated":"2025-08-15 06:40:30.000000000","message":"Acknowledged","commit_id":"12fae0dcfe17cde0f4c5cc7bb8968f3b8ff6fa8b"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"c95b05b4b70cee65984a1f7da3af08abbdda68dc","unresolved":true,"context_lines":[{"line_number":110,"context_line":"      -g cluster_maintaining -s host_maintenance \\"},{"line_number":111,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":112,"context_line":"      -p backup_node\u003dcompute02 \\"},{"line_number":113,"context_line":"      --auto-trigger"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Run an audit using Host Maintenance strategy with migrations disabled."},{"line_number":116,"context_line":"This will only stop active instances on compute01, useful for maintenance"}],"source_content_type":"text/x-rst","patch_set":12,"id":"79269f4e_fcde5732","line":113,"updated":"2025-08-14 14:56:48.000000000","message":"I don\u0027t think I would add this parameter here. While it is correct, I think we should keep these examples as simple as possible","commit_id":"12fae0dcfe17cde0f4c5cc7bb8968f3b8ff6fa8b"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"f40d2887d0b1d5c942f835302d3b33a132e0d76a","unresolved":false,"context_lines":[{"line_number":110,"context_line":"      -g cluster_maintaining -s host_maintenance \\"},{"line_number":111,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":112,"context_line":"      -p backup_node\u003dcompute02 \\"},{"line_number":113,"context_line":"      --auto-trigger"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Run an audit using Host Maintenance strategy with migrations disabled."},{"line_number":116,"context_line":"This will only stop active instances on compute01, useful for maintenance"}],"source_content_type":"text/x-rst","patch_set":12,"id":"9f4aa403_ed8f3b90","line":113,"in_reply_to":"79269f4e_fcde5732","updated":"2025-08-15 06:40:30.000000000","message":"Acknowledged","commit_id":"12fae0dcfe17cde0f4c5cc7bb8968f3b8ff6fa8b"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"c95b05b4b70cee65984a1f7da3af08abbdda68dc","unresolved":true,"context_lines":[{"line_number":123,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":124,"context_line":"      -p disable_live_migration\u003dTrue \\"},{"line_number":125,"context_line":"      -p disable_cold_migration\u003dTrue \\"},{"line_number":126,"context_line":"      --auto-trigger"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"Note that after executing this strategy, the *maintenance_node* will be"},{"line_number":129,"context_line":"marked as disabled, with the reason set to ``watcher_maintaining``."}],"source_content_type":"text/x-rst","patch_set":12,"id":"72bed488_d9ec9d2b","line":126,"updated":"2025-08-14 14:56:48.000000000","message":"same point as above","commit_id":"12fae0dcfe17cde0f4c5cc7bb8968f3b8ff6fa8b"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"f40d2887d0b1d5c942f835302d3b33a132e0d76a","unresolved":false,"context_lines":[{"line_number":123,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":124,"context_line":"      -p disable_live_migration\u003dTrue \\"},{"line_number":125,"context_line":"      -p disable_cold_migration\u003dTrue \\"},{"line_number":126,"context_line":"      --auto-trigger"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"Note that after executing this strategy, the *maintenance_node* will be"},{"line_number":129,"context_line":"marked as disabled, with the reason set to ``watcher_maintaining``."}],"source_content_type":"text/x-rst","patch_set":12,"id":"1a12b76e_a42e9108","line":126,"in_reply_to":"72bed488_d9ec9d2b","updated":"2025-08-15 06:40:30.000000000","message":"Acknowledged","commit_id":"12fae0dcfe17cde0f4c5cc7bb8968f3b8ff6fa8b"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"c6f64b41a191981b88a6dd9b995180325db3b1e2","unresolved":true,"context_lines":[{"line_number":109,"context_line":"    $ openstack optimize audit create \\"},{"line_number":110,"context_line":"      -g cluster_maintaining -s host_maintenance \\"},{"line_number":111,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":112,"context_line":"      -p backup_node\u003dcompute02 \\"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"Run an audit using Host Maintenance strategy with migration disabled."},{"line_number":115,"context_line":"This will only stop active instances on compute01, useful for maintenance"}],"source_content_type":"text/x-rst","patch_set":13,"id":"37481eef_0d519aed","line":112,"updated":"2025-08-18 13:58:05.000000000","message":"nit: the trailing slash is not needed in this line \n```suggestion\n      -p backup_node\u003dcompute02\n```","commit_id":"23080cd3a1828cf7deae5788543295e02f6b5211"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"d39318b46ae7013bb868862257e4876e5204c2c2","unresolved":false,"context_lines":[{"line_number":109,"context_line":"    $ openstack optimize audit create \\"},{"line_number":110,"context_line":"      -g cluster_maintaining -s host_maintenance \\"},{"line_number":111,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":112,"context_line":"      -p backup_node\u003dcompute02 \\"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"Run an audit using Host Maintenance strategy with migration disabled."},{"line_number":115,"context_line":"This will only stop active instances on compute01, useful for maintenance"}],"source_content_type":"text/x-rst","patch_set":13,"id":"cff97fb5_0ad4e539","line":112,"in_reply_to":"37481eef_0d519aed","updated":"2025-08-20 12:40:54.000000000","message":"Acknowledged","commit_id":"23080cd3a1828cf7deae5788543295e02f6b5211"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"c6f64b41a191981b88a6dd9b995180325db3b1e2","unresolved":true,"context_lines":[{"line_number":121,"context_line":"      -g cluster_maintaining -s host_maintenance \\"},{"line_number":122,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":123,"context_line":"      -p disable_live_migration\u003dTrue \\"},{"line_number":124,"context_line":"      -p disable_cold_migration\u003dTrue \\"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"Note that after executing this strategy, the *maintenance_node* will be"},{"line_number":127,"context_line":"marked as disabled, with the reason set to ``watcher_maintaining``."}],"source_content_type":"text/x-rst","patch_set":13,"id":"b3e51900_5f1e576a","line":124,"updated":"2025-08-18 13:58:05.000000000","message":"same point as above","commit_id":"23080cd3a1828cf7deae5788543295e02f6b5211"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"d39318b46ae7013bb868862257e4876e5204c2c2","unresolved":false,"context_lines":[{"line_number":121,"context_line":"      -g cluster_maintaining -s host_maintenance \\"},{"line_number":122,"context_line":"      -p maintenance_node\u003dcompute01 \\"},{"line_number":123,"context_line":"      -p disable_live_migration\u003dTrue \\"},{"line_number":124,"context_line":"      -p disable_cold_migration\u003dTrue \\"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"Note that after executing this strategy, the *maintenance_node* will be"},{"line_number":127,"context_line":"marked as disabled, with the reason set to ``watcher_maintaining``."}],"source_content_type":"text/x-rst","patch_set":13,"id":"38f683af_e0ef71c6","line":124,"in_reply_to":"b3e51900_5f1e576a","updated":"2025-08-20 12:40:54.000000000","message":"Acknowledged","commit_id":"23080cd3a1828cf7deae5788543295e02f6b5211"}],"watcher/applier/actions/base.py":[{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"00aeca187c54cdcbdaddd1aa1a8e57adedddb4a1","unresolved":true,"context_lines":[{"line_number":99,"context_line":"        raise NotImplementedError()"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    @abc.abstractmethod"},{"line_number":102,"context_line":"    def pre_condition(self):"},{"line_number":103,"context_line":"        \"\"\"Hook: called before the execution of an action"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"        This method can be used to perform some initializations or to make"}],"source_content_type":"text/x-python","patch_set":6,"id":"57c8aeef_a5af3a04","line":102,"updated":"2025-06-26 12:34:46.000000000","message":"I think it would be good to change the pre_condition to follow the same pattern as the post_condition. However, I would also be ok with it being in a follow-up patch to not make this bigger","commit_id":"35084dc9e704f4d4a8e97ad1e32dc779595a2433"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"13536bb0f9348d57170a7581c9c2b0bf7f2546b0","unresolved":false,"context_lines":[{"line_number":99,"context_line":"        raise NotImplementedError()"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    @abc.abstractmethod"},{"line_number":102,"context_line":"    def pre_condition(self):"},{"line_number":103,"context_line":"        \"\"\"Hook: called before the execution of an action"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"        This method can be used to perform some initializations or to make"}],"source_content_type":"text/x-python","patch_set":6,"id":"445f7199_cd0f03b4","line":102,"in_reply_to":"57c8aeef_a5af3a04","updated":"2025-06-27 06:15:15.000000000","message":"Yes, I think it should be better a follow-up patch for not extending the scope of this patch. The changes shouldn\u0027t be huge and I can take that after this one finishes.","commit_id":"35084dc9e704f4d4a8e97ad1e32dc779595a2433"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":108,"context_line":"        exception."},{"line_number":109,"context_line":"        \"\"\""},{"line_number":110,"context_line":"        raise NotImplementedError()"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def post_condition(self) -\u003e bool:"},{"line_number":113,"context_line":"        \"\"\"Hook: called after the execution of an action"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"        This function is called regardless of whether an action succeeded or"},{"line_number":116,"context_line":"        not. So you can use it to perform cleanup operations. It will return"},{"line_number":117,"context_line":"        `True` for a successful action (default) and `False` for a failed one."},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        :returns: A flag indicating whether or not the action succeeded"},{"line_number":120,"context_line":"        :rtype: bool"},{"line_number":121,"context_line":"        \"\"\""},{"line_number":122,"context_line":"        return True"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    @property"},{"line_number":125,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":11,"id":"7080fb30_1c2533a9","line":122,"range":{"start_line":111,"start_character":0,"end_line":122,"end_character":19},"updated":"2025-08-06 20:57:36.000000000","message":"We shouldn\u0027t change from abstractmethod, the reason is to make all actions aware of a mandatory method call, which they must implement.\nThe result of post_condition shouldn\u0027t do anything with the action, which already suceeded. Raising exceptions may give us more options to treat corner cases if needed.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":true,"context_lines":[{"line_number":108,"context_line":"        exception."},{"line_number":109,"context_line":"        \"\"\""},{"line_number":110,"context_line":"        raise NotImplementedError()"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def post_condition(self) -\u003e bool:"},{"line_number":113,"context_line":"        \"\"\"Hook: called after the execution of an action"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"        This function is called regardless of whether an action succeeded or"},{"line_number":116,"context_line":"        not. So you can use it to perform cleanup operations. It will return"},{"line_number":117,"context_line":"        `True` for a successful action (default) and `False` for a failed one."},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        :returns: A flag indicating whether or not the action succeeded"},{"line_number":120,"context_line":"        :rtype: bool"},{"line_number":121,"context_line":"        \"\"\""},{"line_number":122,"context_line":"        return True"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    @property"},{"line_number":125,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":11,"id":"c76da027_5131c516","line":122,"range":{"start_line":111,"start_character":0,"end_line":122,"end_character":19},"in_reply_to":"7080fb30_1c2533a9","updated":"2025-08-09 09:55:52.000000000","message":"Thanks for the concern, actually this change was discussed in patchset 4 with @smooney@redhat.com here: https://review.opendev.org/c/openstack/watcher/+/952538/comment/5147f607_289d7bf5/\n\nAs I take away from that discussion, the reasoning for this change is post_condition should have a sensible default (like return True) rather than forcing every action to implement an empty method as they currently do, as for now no action has post_condition logic.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ba7e730aa69e68562edd06b98ce6cef8fc993852","unresolved":true,"context_lines":[{"line_number":108,"context_line":"        exception."},{"line_number":109,"context_line":"        \"\"\""},{"line_number":110,"context_line":"        raise NotImplementedError()"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def post_condition(self) -\u003e bool:"},{"line_number":113,"context_line":"        \"\"\"Hook: called after the execution of an action"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"        This function is called regardless of whether an action succeeded or"},{"line_number":116,"context_line":"        not. So you can use it to perform cleanup operations. It will return"},{"line_number":117,"context_line":"        `True` for a successful action (default) and `False` for a failed one."},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        :returns: A flag indicating whether or not the action succeeded"},{"line_number":120,"context_line":"        :rtype: bool"},{"line_number":121,"context_line":"        \"\"\""},{"line_number":122,"context_line":"        return True"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    @property"},{"line_number":125,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":11,"id":"d0f5dbe0_5c2172e9","line":122,"range":{"start_line":111,"start_character":0,"end_line":122,"end_character":19},"in_reply_to":"b47e9fd8_a12c80ff","updated":"2025-08-19 17:03:39.000000000","message":"so i orgianlly asked for this because in generall i think the validation of the succses fo an action shoudl be done in the post condation not in execute itself.\n\nsince we have continued to do it in execute however i guess this si not needed for now.\n\ni do think we shoudl revisit that for all actions however.\n\nthat can be a seperate topic for the ptg.\n\nit makes sense in case where we have to pool for the compeltion of the action to do do the validation in execute in genreal but perhasp we will want to consider if we shoudl do that in post_condtions instead in the future.\n\ni agree that that is out of scope fo this change however so lets reviert this back for now.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"d4c69a31dbe5bcd36e51d4c9d5df265f1b69c847","unresolved":true,"context_lines":[{"line_number":108,"context_line":"        exception."},{"line_number":109,"context_line":"        \"\"\""},{"line_number":110,"context_line":"        raise NotImplementedError()"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def post_condition(self) -\u003e bool:"},{"line_number":113,"context_line":"        \"\"\"Hook: called after the execution of an action"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"        This function is called regardless of whether an action succeeded or"},{"line_number":116,"context_line":"        not. So you can use it to perform cleanup operations. It will return"},{"line_number":117,"context_line":"        `True` for a successful action (default) and `False` for a failed one."},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        :returns: A flag indicating whether or not the action succeeded"},{"line_number":120,"context_line":"        :rtype: bool"},{"line_number":121,"context_line":"        \"\"\""},{"line_number":122,"context_line":"        return True"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    @property"},{"line_number":125,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":11,"id":"b47e9fd8_a12c80ff","line":122,"range":{"start_line":111,"start_character":0,"end_line":122,"end_character":19},"in_reply_to":"c76da027_5131c516","updated":"2025-08-18 17:43:38.000000000","message":"But at this moment, this change is not needed and is not adding anything to this feature, since you don\u0027t implement post_condition anymore for stop action. So I think that we should remove it. I still prefer raising exception that allow us to update the status_message field that is being added here [1]. Waiting more feedbacks from other reviewers here (Sean, Joan, Alfredo).\n\n[1] https://review.opendev.org/c/openstack/watcher/+/954746/5/watcher/applier/workflow_engine/base.py#180","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"}],"watcher/applier/actions/stop.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":63,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":64,"context_line":"        LOG.debug(\"Stopping instance %s\", self.instance_uuid)"},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"        instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":67,"context_line":"        if not instance:"},{"line_number":68,"context_line":"            raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"8625b10c_5e034c81","line":66,"range":{"start_line":66,"start_character":19,"end_line":66,"end_character":57},"updated":"2025-06-17 11:21:43.000000000","message":"... i see your using this to be efficent becuase of technial debt in watcher.\n\nfind_isntance use get which is the correct way to get a single instnace\n```\n    def find_instance(self, instance_id):\n        return self.nova.servers.get(instance_id)\n```\n\nwhere as get_instance_by_uuid is using list and doing the filtering locally instead...\n\n```     \n    def get_instance_by_uuid(self, instance_uuid):\n        return [instance for instance in\n                self.nova.servers.list(search_opts\u003d{\"all_tenants\": True,\n                                                    \"uuid\": instance_uuid})]\n```\n\nthank you for looking at the implmetion and choosing the efficent method.\n\nwe will either fix this when we swap to the sdk or fix it as a low priorty performance bug sepreatly form this feature.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":63,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":64,"context_line":"        LOG.debug(\"Stopping instance %s\", self.instance_uuid)"},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"        instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":67,"context_line":"        if not instance:"},{"line_number":68,"context_line":"            raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"3e220784_64c22822","line":66,"range":{"start_line":66,"start_character":19,"end_line":66,"end_character":57},"in_reply_to":"8625b10c_5e034c81","updated":"2025-06-18 14:24:25.000000000","message":"Acknowledged","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":63,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":64,"context_line":"        LOG.debug(\"Stopping instance %s\", self.instance_uuid)"},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"        instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":67,"context_line":"        if not instance:"},{"line_number":68,"context_line":"            raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"f4ff5db4_bb717500","line":66,"range":{"start_line":66,"start_character":19,"end_line":66,"end_character":57},"in_reply_to":"8625b10c_5e034c81","updated":"2025-06-18 14:24:25.000000000","message":"Acknowledged","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":81,"context_line":"                )"},{"line_number":82,"context_line":"            return result"},{"line_number":83,"context_line":"        except Exception as exc:"},{"line_number":84,"context_line":"            LOG.exception(exc)"},{"line_number":85,"context_line":"            LOG.critical("},{"line_number":86,"context_line":"                \"Unexpected error occurred while stopping instance %(uuid)s: \""},{"line_number":87,"context_line":"                \"%(error)s\","},{"line_number":88,"context_line":"                {\u0027uuid\u0027: self.instance_uuid, \u0027error\u0027: str(exc)}"},{"line_number":89,"context_line":"            )"},{"line_number":90,"context_line":"            raise"},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"    def execute(self):"}],"source_content_type":"text/x-python","patch_set":4,"id":"cc09280a_12b5ed48","line":89,"range":{"start_line":84,"start_character":11,"end_line":89,"end_character":13},"updated":"2025-06-17 11:21:43.000000000","message":"logging the exception twice at both exception and critical is over kill\n\nwe shoudl avoid critical logs entirely that is normally not used in openstack.\n\na critical level log is only suable if a servcice cant start ro somehtign equally unrecoverable like db migrations failign to apply.\nif watcher is not exiting after a log critical then its the wrong log level to use.\n\nLOG.exception(exc) is a special case of LOG.error\n\nhttps://docs.python.org/3/library/logging.html#logging.Logger.exception\n\n\nlet just do this\n\n```suggestion\n            LOG.exception(f\"Unexpected error occurred while stopping instance \"\n                          f\"{self.instance_uuid}, error: {str(exc)}\")\n```\n\nhere and in all other places where you are logging at both excption and crtical.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":81,"context_line":"                )"},{"line_number":82,"context_line":"            return result"},{"line_number":83,"context_line":"        except Exception as exc:"},{"line_number":84,"context_line":"            LOG.exception(exc)"},{"line_number":85,"context_line":"            LOG.critical("},{"line_number":86,"context_line":"                \"Unexpected error occurred while stopping instance %(uuid)s: \""},{"line_number":87,"context_line":"                \"%(error)s\","},{"line_number":88,"context_line":"                {\u0027uuid\u0027: self.instance_uuid, \u0027error\u0027: str(exc)}"},{"line_number":89,"context_line":"            )"},{"line_number":90,"context_line":"            raise"},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"    def execute(self):"}],"source_content_type":"text/x-python","patch_set":4,"id":"4b4bd81e_fc82888d","line":89,"range":{"start_line":84,"start_character":11,"end_line":89,"end_character":13},"in_reply_to":"cc09280a_12b5ed48","updated":"2025-06-18 14:24:25.000000000","message":"Done","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":81,"context_line":"                )"},{"line_number":82,"context_line":"            return result"},{"line_number":83,"context_line":"        except Exception as exc:"},{"line_number":84,"context_line":"            LOG.exception(exc)"},{"line_number":85,"context_line":"            LOG.critical("},{"line_number":86,"context_line":"                \"Unexpected error occurred while stopping instance %(uuid)s: \""},{"line_number":87,"context_line":"                \"%(error)s\","},{"line_number":88,"context_line":"                {\u0027uuid\u0027: self.instance_uuid, \u0027error\u0027: str(exc)}"},{"line_number":89,"context_line":"            )"},{"line_number":90,"context_line":"            raise"},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"    def execute(self):"}],"source_content_type":"text/x-python","patch_set":4,"id":"d12cfa95_f9b315ff","line":89,"range":{"start_line":84,"start_character":11,"end_line":89,"end_character":13},"in_reply_to":"cc09280a_12b5ed48","updated":"2025-06-18 14:24:25.000000000","message":"Thank you for pointing that out. I just looked at some patterns in other actions like migration and resize, and saw LOG.exception \u0026 LOG.critical used together, so I had to admit that I kinda blindly copied that... My bad.\n\nIf possible, I would love to have more of your insights \u0026 experience on logging level setup, or any good docs you can share. I have to say that till now I\u0027m still confused many times for setting the level of logging, especially when I\u0027m a fresh dev and not really a frequent operator to actually observe the behaviors in practice @@","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":92,"context_line":"    def execute(self):"},{"line_number":93,"context_line":"        return self.stop()"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    def _revert_stop(self):"},{"line_number":96,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""},{"line_number":97,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":98,"context_line":"        LOG.debug(\"Starting instance %s\", self.instance_uuid)"},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"        try:"},{"line_number":101,"context_line":"            result \u003d nova.start_instance(instance_id\u003dself.instance_uuid)"},{"line_number":102,"context_line":"            if result:"},{"line_number":103,"context_line":"                LOG.debug("},{"line_number":104,"context_line":"                    \"Successfully reverted stop action and started instance \""},{"line_number":105,"context_line":"                    \"%(uuid)s\","},{"line_number":106,"context_line":"                    {\u0027uuid\u0027: self.instance_uuid}"},{"line_number":107,"context_line":"                )"},{"line_number":108,"context_line":"            else:"},{"line_number":109,"context_line":"                LOG.error("},{"line_number":110,"context_line":"                    \"Failed to start instance %(uuid)s\","},{"line_number":111,"context_line":"                    {\u0027uuid\u0027: self.instance_uuid}"},{"line_number":112,"context_line":"                )"},{"line_number":113,"context_line":"            return result"},{"line_number":114,"context_line":"        except Exception as exc:"},{"line_number":115,"context_line":"            LOG.exception(exc)"},{"line_number":116,"context_line":"            LOG.critical("},{"line_number":117,"context_line":"                \"Unexpected error occurred while starting instance %(uuid)s: \""},{"line_number":118,"context_line":"                \"%(error)s\","},{"line_number":119,"context_line":"                {\u0027uuid\u0027: self.instance_uuid, \u0027error\u0027: str(exc)}"},{"line_number":120,"context_line":"            )"},{"line_number":121,"context_line":"            raise"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"    def revert(self):"},{"line_number":124,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"}],"source_content_type":"text/x-python","patch_set":4,"id":"df61eca5_8e35e765","line":121,"range":{"start_line":95,"start_character":0,"end_line":121,"end_character":17},"updated":"2025-06-17 11:21:43.000000000","message":"This I\u0027m unsure about.\n\nIn general, you cannot assume that an admin can start a instance.\n\nSpecifically, if it\u0027s using vTPM or encrypted volumes.\nso im conflicted on if we should try to start the vm again on revert.\n\nIf we do try to start it, I don\u0027t think we should raise an exception if we get an api error. Just report that it failed by returning false and a log an info level message.\n\nFor some VMs, it\u0027s perfectly normal and should not be marked as critical or similar in the logs.\n\n\nIf an operator sees a critical log message, they should expect to need to call in a support engineer to resolve the issue in production immediately.\n\nwe shoudl use it very very sparingly and this does not rise to that level of urgency.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":92,"context_line":"    def execute(self):"},{"line_number":93,"context_line":"        return self.stop()"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    def _revert_stop(self):"},{"line_number":96,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""},{"line_number":97,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":98,"context_line":"        LOG.debug(\"Starting instance %s\", self.instance_uuid)"},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"        try:"},{"line_number":101,"context_line":"            result \u003d nova.start_instance(instance_id\u003dself.instance_uuid)"},{"line_number":102,"context_line":"            if result:"},{"line_number":103,"context_line":"                LOG.debug("},{"line_number":104,"context_line":"                    \"Successfully reverted stop action and started instance \""},{"line_number":105,"context_line":"                    \"%(uuid)s\","},{"line_number":106,"context_line":"                    {\u0027uuid\u0027: self.instance_uuid}"},{"line_number":107,"context_line":"                )"},{"line_number":108,"context_line":"            else:"},{"line_number":109,"context_line":"                LOG.error("},{"line_number":110,"context_line":"                    \"Failed to start instance %(uuid)s\","},{"line_number":111,"context_line":"                    {\u0027uuid\u0027: self.instance_uuid}"},{"line_number":112,"context_line":"                )"},{"line_number":113,"context_line":"            return result"},{"line_number":114,"context_line":"        except Exception as exc:"},{"line_number":115,"context_line":"            LOG.exception(exc)"},{"line_number":116,"context_line":"            LOG.critical("},{"line_number":117,"context_line":"                \"Unexpected error occurred while starting instance %(uuid)s: \""},{"line_number":118,"context_line":"                \"%(error)s\","},{"line_number":119,"context_line":"                {\u0027uuid\u0027: self.instance_uuid, \u0027error\u0027: str(exc)}"},{"line_number":120,"context_line":"            )"},{"line_number":121,"context_line":"            raise"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"    def revert(self):"},{"line_number":124,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5df7fa92_2cd3274d","line":121,"range":{"start_line":95,"start_character":0,"end_line":121,"end_character":17},"in_reply_to":"df61eca5_8e35e765","updated":"2025-06-18 14:24:25.000000000","message":"Done","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":92,"context_line":"    def execute(self):"},{"line_number":93,"context_line":"        return self.stop()"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    def _revert_stop(self):"},{"line_number":96,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""},{"line_number":97,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":98,"context_line":"        LOG.debug(\"Starting instance %s\", self.instance_uuid)"},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"        try:"},{"line_number":101,"context_line":"            result \u003d nova.start_instance(instance_id\u003dself.instance_uuid)"},{"line_number":102,"context_line":"            if result:"},{"line_number":103,"context_line":"                LOG.debug("},{"line_number":104,"context_line":"                    \"Successfully reverted stop action and started instance \""},{"line_number":105,"context_line":"                    \"%(uuid)s\","},{"line_number":106,"context_line":"                    {\u0027uuid\u0027: self.instance_uuid}"},{"line_number":107,"context_line":"                )"},{"line_number":108,"context_line":"            else:"},{"line_number":109,"context_line":"                LOG.error("},{"line_number":110,"context_line":"                    \"Failed to start instance %(uuid)s\","},{"line_number":111,"context_line":"                    {\u0027uuid\u0027: self.instance_uuid}"},{"line_number":112,"context_line":"                )"},{"line_number":113,"context_line":"            return result"},{"line_number":114,"context_line":"        except Exception as exc:"},{"line_number":115,"context_line":"            LOG.exception(exc)"},{"line_number":116,"context_line":"            LOG.critical("},{"line_number":117,"context_line":"                \"Unexpected error occurred while starting instance %(uuid)s: \""},{"line_number":118,"context_line":"                \"%(error)s\","},{"line_number":119,"context_line":"                {\u0027uuid\u0027: self.instance_uuid, \u0027error\u0027: str(exc)}"},{"line_number":120,"context_line":"            )"},{"line_number":121,"context_line":"            raise"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"    def revert(self):"},{"line_number":124,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"}],"source_content_type":"text/x-python","patch_set":4,"id":"8cee1d8e_b583bf5d","line":121,"range":{"start_line":95,"start_character":0,"end_line":121,"end_character":17},"in_reply_to":"df61eca5_8e35e765","updated":"2025-06-18 14:24:25.000000000","message":"Thank you for your insight! I kinda overlook that... So for now I\u0027ve made changed by setting log levels for the revert to be info \u0026 debug, not raising anything, and only returns True/False including for exception handling. I guess it can be considered fail-safe when the vm can\u0027t be restarted for any reasons,and it will just gracefully show a log and return False.\n\nBut for even further safety, then I guess we can simply make it not revertible, and simply return a False. I\u0027m just unsure what will be the resulted effect here.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def abort(self):"},{"line_number":128,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""},{"line_number":129,"context_line":"        LOG.warning(\"Abort operation is not applicable for stop action on \""},{"line_number":130,"context_line":"                    \"instance %s\", self.instance_uuid)"},{"line_number":131,"context_line":"        return False"},{"line_number":132,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"21f86683_ba1a1486","line":129,"range":{"start_line":129,"start_character":12,"end_line":129,"end_character":19},"updated":"2025-06-17 11:21:43.000000000","message":"info.\n\n\nwarning is the and higher log levels like error and critical are all call to action\n\nfor operators running a service.\n\nwarning should always have a recommendation for a human to go premtivly fix\nsomething before it turns into an error.\n\nthere is nothign for an operator to do here so it shoudl just be an info message.\n\nhttps://docs.python.org/3/library/logging.html#logging-levels","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def abort(self):"},{"line_number":128,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""},{"line_number":129,"context_line":"        LOG.warning(\"Abort operation is not applicable for stop action on \""},{"line_number":130,"context_line":"                    \"instance %s\", self.instance_uuid)"},{"line_number":131,"context_line":"        return False"},{"line_number":132,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"8c98c5c7_9ee3354e","line":129,"range":{"start_line":129,"start_character":12,"end_line":129,"end_character":19},"in_reply_to":"21f86683_ba1a1486","updated":"2025-06-18 14:24:25.000000000","message":"Done","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def abort(self):"},{"line_number":128,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""},{"line_number":129,"context_line":"        LOG.warning(\"Abort operation is not applicable for stop action on \""},{"line_number":130,"context_line":"                    \"instance %s\", self.instance_uuid)"},{"line_number":131,"context_line":"        return False"},{"line_number":132,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"c4b768d4_b62704c3","line":129,"range":{"start_line":129,"start_character":12,"end_line":129,"end_character":19},"in_reply_to":"21f86683_ba1a1486","updated":"2025-06-18 14:24:25.000000000","message":"Done","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":151,"context_line":"        except exception.InstanceNotFound:"},{"line_number":152,"context_line":"            raise"},{"line_number":153,"context_line":"        except Exception as exc:"},{"line_number":154,"context_line":"            LOG.exception(exc)"},{"line_number":155,"context_line":"            LOG.error(\"Pre-condition check failed for instance %s: %s\","},{"line_number":156,"context_line":"                      self.instance_uuid, str(exc))"},{"line_number":157,"context_line":"            raise"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def post_condition(self):"},{"line_number":160,"context_line":"        # Verify the instance is actually stopped"}],"source_content_type":"text/x-python","patch_set":4,"id":"ab254f63_8bde11f6","line":157,"range":{"start_line":154,"start_character":10,"end_line":157,"end_character":17},"updated":"2025-06-17 11:21:43.000000000","message":"again only log this once at error/exception level","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":151,"context_line":"        except exception.InstanceNotFound:"},{"line_number":152,"context_line":"            raise"},{"line_number":153,"context_line":"        except Exception as exc:"},{"line_number":154,"context_line":"            LOG.exception(exc)"},{"line_number":155,"context_line":"            LOG.error(\"Pre-condition check failed for instance %s: %s\","},{"line_number":156,"context_line":"                      self.instance_uuid, str(exc))"},{"line_number":157,"context_line":"            raise"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def post_condition(self):"},{"line_number":160,"context_line":"        # Verify the instance is actually stopped"}],"source_content_type":"text/x-python","patch_set":4,"id":"4ae1784b_413e17ef","line":157,"range":{"start_line":154,"start_character":10,"end_line":157,"end_character":17},"in_reply_to":"ab254f63_8bde11f6","updated":"2025-06-18 14:24:25.000000000","message":"Done","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":151,"context_line":"        except exception.InstanceNotFound:"},{"line_number":152,"context_line":"            raise"},{"line_number":153,"context_line":"        except Exception as exc:"},{"line_number":154,"context_line":"            LOG.exception(exc)"},{"line_number":155,"context_line":"            LOG.error(\"Pre-condition check failed for instance %s: %s\","},{"line_number":156,"context_line":"                      self.instance_uuid, str(exc))"},{"line_number":157,"context_line":"            raise"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def post_condition(self):"},{"line_number":160,"context_line":"        # Verify the instance is actually stopped"}],"source_content_type":"text/x-python","patch_set":4,"id":"bcdb63f5_30e2dad0","line":157,"range":{"start_line":154,"start_character":10,"end_line":157,"end_character":17},"in_reply_to":"ab254f63_8bde11f6","updated":"2025-06-18 14:24:25.000000000","message":"Done","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":162,"context_line":"        try:"},{"line_number":163,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":164,"context_line":"            if not instance:"},{"line_number":165,"context_line":"                LOG.warning("},{"line_number":166,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":167,"context_line":"                    self.instance_uuid"},{"line_number":168,"context_line":"                )"}],"source_content_type":"text/x-python","patch_set":4,"id":"5efff112_d7d5f649","line":165,"range":{"start_line":165,"start_character":20,"end_line":165,"end_character":27},"updated":"2025-06-17 11:21:43.000000000","message":"info, again this is normal, the inastnace may have been deleted by the end user.\n\nits good to recored this in the logs but it is not actully actionable by the operator.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":162,"context_line":"        try:"},{"line_number":163,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":164,"context_line":"            if not instance:"},{"line_number":165,"context_line":"                LOG.warning("},{"line_number":166,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":167,"context_line":"                    self.instance_uuid"},{"line_number":168,"context_line":"                )"}],"source_content_type":"text/x-python","patch_set":4,"id":"ca4cf7ca_70a1b55c","line":165,"range":{"start_line":165,"start_character":20,"end_line":165,"end_character":27},"in_reply_to":"5efff112_d7d5f649","updated":"2025-06-18 14:24:25.000000000","message":"Done","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":162,"context_line":"        try:"},{"line_number":163,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":164,"context_line":"            if not instance:"},{"line_number":165,"context_line":"                LOG.warning("},{"line_number":166,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":167,"context_line":"                    self.instance_uuid"},{"line_number":168,"context_line":"                )"}],"source_content_type":"text/x-python","patch_set":4,"id":"cdc3a92c_0bce76df","line":165,"range":{"start_line":165,"start_character":20,"end_line":165,"end_character":27},"in_reply_to":"5efff112_d7d5f649","updated":"2025-06-18 14:24:25.000000000","message":"Done","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":178,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":179,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":180,"context_line":"                ):"},{"line_number":181,"context_line":"                    LOG.warning("},{"line_number":182,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":183,"context_line":"                        self.instance_uuid, current_state"},{"line_number":184,"context_line":"                    )"}],"source_content_type":"text/x-python","patch_set":4,"id":"9585a09d_9f301c44","line":181,"range":{"start_line":181,"start_character":24,"end_line":181,"end_character":31},"updated":"2025-06-17 11:21:43.000000000","message":"this is more reasonable to log as a warning as it may indicate a problem with nova or that a user or something else has restarted the vm.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":178,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":179,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":180,"context_line":"                ):"},{"line_number":181,"context_line":"                    LOG.warning("},{"line_number":182,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":183,"context_line":"                        self.instance_uuid, current_state"},{"line_number":184,"context_line":"                    )"}],"source_content_type":"text/x-python","patch_set":4,"id":"5c8bcd94_de980075","line":181,"range":{"start_line":181,"start_character":24,"end_line":181,"end_character":31},"in_reply_to":"9585a09d_9f301c44","updated":"2025-06-18 14:24:25.000000000","message":"Acknowledged","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":178,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":179,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":180,"context_line":"                ):"},{"line_number":181,"context_line":"                    LOG.warning("},{"line_number":182,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":183,"context_line":"                        self.instance_uuid, current_state"},{"line_number":184,"context_line":"                    )"}],"source_content_type":"text/x-python","patch_set":4,"id":"d795296b_22f4a012","line":181,"range":{"start_line":181,"start_character":24,"end_line":181,"end_character":31},"in_reply_to":"9585a09d_9f301c44","updated":"2025-06-18 14:24:25.000000000","message":"Acknowledged","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":184,"context_line":"                    )"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.exception(exc)"},{"line_number":188,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":189,"context_line":"                        self.instance_uuid, str(exc))"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def get_description(self):"},{"line_number":192,"context_line":"        \"\"\"Description of the action\"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"5147f607_289d7bf5","line":189,"range":{"start_line":187,"start_character":11,"end_line":189,"end_character":53},"updated":"2025-06-17 11:21:43.000000000","message":"Again, log this once.\n\nIf the post condition failed the action should be marked as failed.\n\nHowever, currently, post_condition is not properly implemented for any other action.\n\nSo to actually do this, we would need to raise the excption again and capture the exception here:\n\nhttps://github.com/openstack/watcher/blob/master/watcher/applier/workflow_engine/default.py#L163\n\nand then do\n\nreturn self.engine.notify(self._db_action, objects.action.State.FAILED)\nlike here \nhttps://github.com/openstack/watcher/blob/master/watcher/applier/workflow_engine/default.py#L163\n\n\nwhere you are raisign a warning above you woudl also need to raise an excption.\n\n\na better approch may to define that post_condition shoudl return a bool \n\nTrue for succes and False for failed\n\nin that case line 169 shoudl return true\non line 185 you can return false\nand on line 189 if we catch an expction you can also return false.\n\nthen you woudl update do_post_execute to \n\ndef do_post_execute(self):\n    LOG.debug(\"Post-condition action: %s\", self.name)\n    if not self.action.post_condition():\n        return self.engine.notify(self._db_action,\n                                  objects.action.State.FAILED)\n                                  \nyou woudl laos need to update \n\nhttps://github.com/openstack/watcher/blob/master/watcher/applier/workflow_engine/base.py#L140-L142\n```\n  @abc.abstractmethod\n    def do_post_execute(self):\n        raise NotImplementedError()\n```\n\nto \n\n```\n    def do_post_execute(self):\n        return True\n```     \n        \nand remove the existing \n\n```\ndef post_condition(self):\n        # TODO(jed): check extra parameters (network response, etc.)\n        pass\n```\n\nfuncitons form all the other actions.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ee7f6671e6964ef2bcbe594672c2926de3dccfb6","unresolved":true,"context_lines":[{"line_number":184,"context_line":"                    )"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.exception(exc)"},{"line_number":188,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":189,"context_line":"                        self.instance_uuid, str(exc))"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def get_description(self):"},{"line_number":192,"context_line":"        \"\"\"Description of the action\"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"7f6a0333_e82ef595","line":189,"range":{"start_line":187,"start_character":11,"end_line":189,"end_character":53},"in_reply_to":"50062b54_5fb91da1","updated":"2025-06-26 01:16:47.000000000","message":"Also, the change for post_condition to return bool is made in patchset 5, as per the suggestion.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":true,"context_lines":[{"line_number":184,"context_line":"                    )"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.exception(exc)"},{"line_number":188,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":189,"context_line":"                        self.instance_uuid, str(exc))"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def get_description(self):"},{"line_number":192,"context_line":"        \"\"\"Description of the action\"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"50062b54_5fb91da1","line":189,"range":{"start_line":187,"start_character":11,"end_line":189,"end_character":53},"in_reply_to":"5147f607_289d7bf5","updated":"2025-06-18 14:24:25.000000000","message":"Thank you for the detailed suggestion. I also think to redefine post_condition to return bool is a better a simpler way. However I still have some concerns regarding that 2nd suggestion.\n\nFor this: \n\u003e then you woudl update do_post_execute to\n\u003e def do_post_execute(self):\n\u003e LOG.debug(\"Post-condition action: %s\", self.name)\n\u003e if not self.action.post_condition():\n\u003e return self.engine.notify(self._db_action,\n\u003e objects.action.State.FAILED)\n\nI\u0027m unsure we need to return the object by engine.notify, or just need to call that notify method. To me it\u0027s more reasonable for just calling it to mark a failed state for the action, otherwise it\u0027s inconsistent for the return type of do_post_execute.\n\nAlso, I\u0027m also confused on why we need to change the base do_post_execute in https://github.com/openstack/watcher/blob/master/watcher/applier/workflow_engine/base.py#L140-L142 to return True. It makes more sense to me to make the change to the post_condition in action base class https://github.com/openstack/watcher/blob/0f783864627260d98279ebc408af1e0ea58cad6c/watcher/applier/actions/base.py#L113 , especially when you mentioned:\n\n\u003e and remove the existing\n\u003e def post_condition(self):\n\u003e        # TODO(jed): check extra parameters (network response, etc.)\n\u003e        pass\n\u003e funcitons form all the other actions.\n\nI still need to learn a lot, and hope to get more clarification. Thank you so much!","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"aa13fbd9048c17ea6e71f385539eb03f85e50337","unresolved":false,"context_lines":[{"line_number":184,"context_line":"                    )"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.exception(exc)"},{"line_number":188,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":189,"context_line":"                        self.instance_uuid, str(exc))"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def get_description(self):"},{"line_number":192,"context_line":"        \"\"\"Description of the action\"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"9370190b_13607ba7","line":189,"range":{"start_line":187,"start_character":11,"end_line":189,"end_character":53},"in_reply_to":"7c3bd59d_e88f1383","updated":"2025-07-11 15:41:05.000000000","message":"Actually, we are already catching the exceptions in post_condition above _do_post_execute and setting actions as failed in:\n\nhttps://github.com/openstack/watcher/blob/master/watcher/applier/workflow_engine/base.py#L245-L256","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"552ce67deca9f860eb3a218d8b5c5e9c65cfab7e","unresolved":false,"context_lines":[{"line_number":184,"context_line":"                    )"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.exception(exc)"},{"line_number":188,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":189,"context_line":"                        self.instance_uuid, str(exc))"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def get_description(self):"},{"line_number":192,"context_line":"        \"\"\"Description of the action\"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"7c3bd59d_e88f1383","line":189,"range":{"start_line":187,"start_character":11,"end_line":189,"end_character":53},"in_reply_to":"7f6a0333_e82ef595","updated":"2025-07-03 16:24:40.000000000","message":"Acknowledged","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"00aeca187c54cdcbdaddd1aa1a8e57adedddb4a1","unresolved":true,"context_lines":[{"line_number":144,"context_line":"            if current_state \u003d\u003d \u0027stopped\u0027:"},{"line_number":145,"context_line":"                return True"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"            LOG.debug(\"Instance %s pre-condition check: state\u003d%s\","},{"line_number":148,"context_line":"                      self.instance_uuid, current_state)"},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"        except exception.InstanceNotFound:"}],"source_content_type":"text/x-python","patch_set":6,"id":"1e636d76_8ff16e05","line":147,"updated":"2025-06-26 12:34:46.000000000","message":"I would move this log to before the previous if so that when the instance it\u0027s found it\u0027s logged. It\u0027s a minor thing but I think it\u0027s better to have the debug logging consistent","commit_id":"35084dc9e704f4d4a8e97ad1e32dc779595a2433"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"13536bb0f9348d57170a7581c9c2b0bf7f2546b0","unresolved":false,"context_lines":[{"line_number":144,"context_line":"            if current_state \u003d\u003d \u0027stopped\u0027:"},{"line_number":145,"context_line":"                return True"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"            LOG.debug(\"Instance %s pre-condition check: state\u003d%s\","},{"line_number":148,"context_line":"                      self.instance_uuid, current_state)"},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"        except exception.InstanceNotFound:"}],"source_content_type":"text/x-python","patch_set":6,"id":"c8d02af7_d47f6a83","line":147,"in_reply_to":"1e636d76_8ff16e05","updated":"2025-06-27 06:15:15.000000000","message":"Done","commit_id":"35084dc9e704f4d4a8e97ad1e32dc779595a2433"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"552ce67deca9f860eb3a218d8b5c5e9c65cfab7e","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    def execute(self):"},{"line_number":92,"context_line":"        return self.stop()"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def _revert_stop(self):"},{"line_number":95,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""},{"line_number":96,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":97,"context_line":"        LOG.debug(\"Starting instance %s\", self.instance_uuid)"}],"source_content_type":"text/x-python","patch_set":9,"id":"b73a834e_6b2db4d4","line":94,"updated":"2025-07-03 16:24:40.000000000","message":"I\u0027m still not entirely convinced that we should try and start the VM as part of the revert, as we are not directly recording the state of the instance beforehand, although if we do not generate a stop action for a stopped VM, then I guess this might be ok. The VM could be in other states, like paused or suspended.\n\nI wonder what others think? I\u0027m incliend to make revert a noop, or otherwise only start the vm if it was previousy in the active state.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"14cfd69ffc0a2f39300d2eda30cf207661bc48e9","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    def execute(self):"},{"line_number":92,"context_line":"        return self.stop()"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def _revert_stop(self):"},{"line_number":95,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""},{"line_number":96,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":97,"context_line":"        LOG.debug(\"Starting instance %s\", self.instance_uuid)"}],"source_content_type":"text/x-python","patch_set":9,"id":"b96c1c21_d4b04ba3","line":94,"in_reply_to":"b73a834e_6b2db4d4","updated":"2025-07-04 04:55:39.000000000","message":"Thanks for the feedback. I\u0027m sharing the concern about this. So currently in the host maintenance strategy, I make it so that the `stop` action is only applied for instances in `active` status.\nI think, maybe in the `pre_condition` check of stop action, I can add another layer to the `stop` action itself, by raising an exception if the instance is not `active`. So with that we can ensure any instance getting to the revert step was previously in the active state. How do you think about this approach?","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"c3bf0c3e1a404e1cce3003b98825bfe3903eb567","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    def execute(self):"},{"line_number":92,"context_line":"        return self.stop()"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def _revert_stop(self):"},{"line_number":95,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""},{"line_number":96,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":97,"context_line":"        LOG.debug(\"Starting instance %s\", self.instance_uuid)"}],"source_content_type":"text/x-python","patch_set":9,"id":"d70f99e5_dc2097df","line":94,"in_reply_to":"b96c1c21_d4b04ba3","updated":"2025-07-04 11:24:00.000000000","message":"The approach in the PS 10 should be enough to ensure that we only added a `stop` action to instances that were originally `active`. My concern would be what would happen if some other service or the operator manually change the instance state between the action execution and the revert. Looking at the allowed transitions https://docs.openstack.org/nova/latest/reference/vm-states.html all states that can be reached from a `stopped` can go back to `active` except for `shelved_offloaded`. I\u0027m not sure what would happen in that case (it seems an unlike scenario), but probably nova would fail the start command, so we would probably be ok.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"eb21c241b65a3af27f5f610b67623ef50296c4d9","unresolved":false,"context_lines":[{"line_number":91,"context_line":"    def execute(self):"},{"line_number":92,"context_line":"        return self.stop()"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def _revert_stop(self):"},{"line_number":95,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""},{"line_number":96,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":97,"context_line":"        LOG.debug(\"Starting instance %s\", self.instance_uuid)"}],"source_content_type":"text/x-python","patch_set":9,"id":"895a3b52_4697cda2","line":94,"in_reply_to":"d70f99e5_dc2097df","updated":"2025-07-04 11:50:38.000000000","message":"i think start on an active vm woudl either return a 200 ok or a 409 conflict\n\ni.e. its alreay din the desgiered state so it reutrns 200 or its not in one of the defiend states that it can sart form so its reutrend a 409.\n\ni think we can acept this in its current form for now.\ni fell like as a project we need to recondisred/evalueate how revert works in general and look at all the actions.\n\nim happy to defer futher refinement of this to that larger effort.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"552ce67deca9f860eb3a218d8b5c5e9c65cfab7e","unresolved":true,"context_lines":[{"line_number":138,"context_line":"                raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"            # Check instance current state"},{"line_number":141,"context_line":"            current_state \u003d getattr(instance, \u0027OS-EXT-STS:vm_state\u0027)"},{"line_number":142,"context_line":"            LOG.debug(\"Instance %s pre-condition check: state\u003d%s\","},{"line_number":143,"context_line":"                      self.instance_uuid, current_state)"},{"line_number":144,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"351daff5_4c072339","line":141,"range":{"start_line":141,"start_character":28,"end_line":141,"end_character":35},"updated":"2025-07-03 16:24:40.000000000","message":"You should not be using getattr in production code.\n\nI think you can use instance.status instead\n\nhttps://github.com/openstack/watcher/blob/94d8676db8d652d8b252765ea720b27e96e2b6a7/watcher/applier/actions/volume_migration.py#L141","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"14cfd69ffc0a2f39300d2eda30cf207661bc48e9","unresolved":false,"context_lines":[{"line_number":138,"context_line":"                raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"            # Check instance current state"},{"line_number":141,"context_line":"            current_state \u003d getattr(instance, \u0027OS-EXT-STS:vm_state\u0027)"},{"line_number":142,"context_line":"            LOG.debug(\"Instance %s pre-condition check: state\u003d%s\","},{"line_number":143,"context_line":"                      self.instance_uuid, current_state)"},{"line_number":144,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"c19b6dce_9d80c70f","line":141,"range":{"start_line":141,"start_character":28,"end_line":141,"end_character":35},"in_reply_to":"351daff5_4c072339","updated":"2025-07-04 04:55:39.000000000","message":"Thanks for the notice. That\u0027s better indeed 👍","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":63,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":64,"context_line":"        LOG.debug(\"Stopping instance %s\", self.instance_uuid)"},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"        instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":67,"context_line":"        if not instance:"},{"line_number":68,"context_line":"            raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"        try:"},{"line_number":71,"context_line":"            result \u003d nova.stop_instance(instance_id\u003dself.instance_uuid)"}],"source_content_type":"text/x-python","patch_set":11,"id":"370df238_aa1d4065","line":68,"range":{"start_line":66,"start_character":0,"end_line":68,"end_character":69},"updated":"2025-08-06 20:57:36.000000000","message":"You could return True/False here.\nTBH, not sure what we should do with an Instance was not found. Should we really fail? I know that migration fails for that, but maybe is not the best thing to do either.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":true,"context_lines":[{"line_number":63,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":64,"context_line":"        LOG.debug(\"Stopping instance %s\", self.instance_uuid)"},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"        instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":67,"context_line":"        if not instance:"},{"line_number":68,"context_line":"            raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"        try:"},{"line_number":71,"context_line":"            result \u003d nova.stop_instance(instance_id\u003dself.instance_uuid)"}],"source_content_type":"text/x-python","patch_set":11,"id":"f483fe33_3ad3a1dc","line":68,"range":{"start_line":66,"start_character":0,"end_line":68,"end_character":69},"in_reply_to":"370df238_aa1d4065","updated":"2025-08-09 09:55:52.000000000","message":"I\u0027m also wondering this... \nIntuitively I don\u0027t feel a stop action should be marked as `failed` when the instance it tries to stop doesn\u0027t exist anymore. So for now I try to turn it into an idempotent behavior, where we still return True in case the False returned from `nova.stop_instance` is due to instance not found. Now the action only fails if the instance does exist but the actual attempt to stop it is failed.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":68,"context_line":"            raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"        try:"},{"line_number":71,"context_line":"            result \u003d nova.stop_instance(instance_id\u003dself.instance_uuid)"},{"line_number":72,"context_line":"            if result:"},{"line_number":73,"context_line":"                LOG.debug("},{"line_number":74,"context_line":"                    \"Successfully stopped instance %(uuid)s\","}],"source_content_type":"text/x-python","patch_set":11,"id":"6e1d1c13_665eeb9d","line":71,"range":{"start_line":71,"start_character":0,"end_line":71,"end_character":71},"updated":"2025-08-06 20:57:36.000000000","message":"this method already check if the instance exists and returns False if not, so you don\u0027t need the check above, unless if we decide to return True in case of instance not found?","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"d4c69a31dbe5bcd36e51d4c9d5df265f1b69c847","unresolved":false,"context_lines":[{"line_number":68,"context_line":"            raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"        try:"},{"line_number":71,"context_line":"            result \u003d nova.stop_instance(instance_id\u003dself.instance_uuid)"},{"line_number":72,"context_line":"            if result:"},{"line_number":73,"context_line":"                LOG.debug("},{"line_number":74,"context_line":"                    \"Successfully stopped instance %(uuid)s\","}],"source_content_type":"text/x-python","patch_set":11,"id":"9449b6d4_481185bf","line":71,"range":{"start_line":71,"start_character":0,"end_line":71,"end_character":71},"in_reply_to":"574a1080_4a1f4de6","updated":"2025-08-18 17:43:38.000000000","message":"ok, makes sense, otherwise we would need to change the logic of stop_instance method, which may not make sense.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":true,"context_lines":[{"line_number":68,"context_line":"            raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"        try:"},{"line_number":71,"context_line":"            result \u003d nova.stop_instance(instance_id\u003dself.instance_uuid)"},{"line_number":72,"context_line":"            if result:"},{"line_number":73,"context_line":"                LOG.debug("},{"line_number":74,"context_line":"                    \"Successfully stopped instance %(uuid)s\","}],"source_content_type":"text/x-python","patch_set":11,"id":"574a1080_4a1f4de6","line":71,"range":{"start_line":71,"start_character":0,"end_line":71,"end_character":71},"in_reply_to":"6e1d1c13_665eeb9d","updated":"2025-08-09 09:55:52.000000000","message":"You\u0027re right. I removed the above find_instance check. As follow the previous comment, now I change the logic to apply the `stop_instance` right away, and check `find_instance` only when the `stop_instance` returns False. So if the reason for failing is the non-existence of the instance then we can consider it a success, otherwise it will be an actual failure while trying to stop an existing instance.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":86,"context_line":"                \"%(error)s\","},{"line_number":87,"context_line":"                {\u0027uuid\u0027: self.instance_uuid, \u0027error\u0027: str(exc)}"},{"line_number":88,"context_line":"            )"},{"line_number":89,"context_line":"            raise"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    def execute(self):"},{"line_number":92,"context_line":"        return self.stop()"}],"source_content_type":"text/x-python","patch_set":11,"id":"4bf05bec_96a22b7d","line":89,"range":{"start_line":89,"start_character":0,"end_line":89,"end_character":17},"updated":"2025-08-06 20:57:36.000000000","message":"we should raise a known Watcher exception, and try/catch this method call in execute(). The taskflow itself should be able to catch exceptions but the BaseAction documentation defines a boolean as return type.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":true,"context_lines":[{"line_number":86,"context_line":"                \"%(error)s\","},{"line_number":87,"context_line":"                {\u0027uuid\u0027: self.instance_uuid, \u0027error\u0027: str(exc)}"},{"line_number":88,"context_line":"            )"},{"line_number":89,"context_line":"            raise"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    def execute(self):"},{"line_number":92,"context_line":"        return self.stop()"}],"source_content_type":"text/x-python","patch_set":11,"id":"0ff6e680_c7b590b7","line":89,"range":{"start_line":89,"start_character":0,"end_line":89,"end_character":17},"in_reply_to":"4bf05bec_96a22b7d","updated":"2025-08-09 09:55:52.000000000","message":"Taking a look through the existing exceptions from `watcher.common`, I couldn\u0027t find a suitable exception we can raise in the `stop` method... \nBasically, from what I understand it will get errors only from the Nova API calls (for `stop_instance`, `find_instance`), and I\u0027m unsure we should raise that here. So instead raising the exception, I just add more catches and logs, and make`stop` method only return boolean. \nBut tbh, I\u0027m not sure it\u0027s a good approach though...","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":89,"context_line":"            raise"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    def execute(self):"},{"line_number":92,"context_line":"        return self.stop()"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def _revert_stop(self):"},{"line_number":95,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"de428420_b73c4ef5","line":92,"range":{"start_line":92,"start_character":0,"end_line":92,"end_character":26},"updated":"2025-08-06 20:57:36.000000000","message":"if the stop method is raising exceptions, we may need to try/except here and return False in case of failure","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ba7e730aa69e68562edd06b98ce6cef8fc993852","unresolved":false,"context_lines":[{"line_number":89,"context_line":"            raise"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    def execute(self):"},{"line_number":92,"context_line":"        return self.stop()"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def _revert_stop(self):"},{"line_number":95,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"91311704_d77403e6","line":92,"range":{"start_line":92,"start_character":0,"end_line":92,"end_character":26},"in_reply_to":"8992c446_0b0eb0f7","updated":"2025-08-19 17:03:39.000000000","message":"Done the sotp function is internally catuch its excptions and marhaling the reults into booleans.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":true,"context_lines":[{"line_number":89,"context_line":"            raise"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    def execute(self):"},{"line_number":92,"context_line":"        return self.stop()"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def _revert_stop(self):"},{"line_number":95,"context_line":"        \"\"\"Revert the stop action by trying to start the instance\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"8992c446_0b0eb0f7","line":92,"range":{"start_line":92,"start_character":0,"end_line":92,"end_character":26},"in_reply_to":"de428420_b73c4ef5","updated":"2025-08-09 09:55:52.000000000","message":"Follow the change above, since now we don\u0027t raise exception in `stop` method, here I keep the current `execute()` so it looks consistent with the `execute()` in other actions.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    def revert(self):"},{"line_number":123,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"},{"line_number":124,"context_line":"        return self._revert_stop()"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def abort(self):"},{"line_number":127,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"7d006689_151cfdb1","line":124,"range":{"start_line":124,"start_character":0,"end_line":124,"end_character":34},"updated":"2025-08-06 20:57:36.000000000","message":"revert has no return type expected iiuc","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"f40d2887d0b1d5c942f835302d3b33a132e0d76a","unresolved":true,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    def revert(self):"},{"line_number":123,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"},{"line_number":124,"context_line":"        return self._revert_stop()"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def abort(self):"},{"line_number":127,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"28911d92_2cc0775b","line":124,"range":{"start_line":124,"start_character":0,"end_line":124,"end_character":34},"in_reply_to":"16666a63_cb08e1c4","updated":"2025-08-15 06:40:30.000000000","message":"Yes, so I was a bit hesitant earlier to implement revert having a return, though it\u0027s not mentioned a return type in the base method docstring like others.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ba7e730aa69e68562edd06b98ce6cef8fc993852","unresolved":true,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    def revert(self):"},{"line_number":123,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"},{"line_number":124,"context_line":"        return self._revert_stop()"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def abort(self):"},{"line_number":127,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"e84939fb_6185a79b","line":124,"range":{"start_line":124,"start_character":0,"end_line":124,"end_character":34},"in_reply_to":"17e05cca_1f97ea88","updated":"2025-08-19 17:03:39.000000000","message":"so all funciton in python implictly return the None object if they do not explcity return\n\nso this will retrun None\n```\ndef foo():\n  pass\n```\n\nand it correct type signiture would be \n```\ndef foo() -\u003e None:\n  pass\n```\n\nso rever does not have a return value today or rather its impliclty None.\n\nhowever in practhis tis is just another exampel of the docs string and the impelmetin being out of sycn\n\nfor sleep teh return type is bool\nhttps://github.com/openstack/watcher/blob/master/watcher/applier/actions/sleep.py#L65-L67\n\nmigrate is also bool\nhttps://github.com/openstack/watcher/blob/master/watcher/applier/actions/migration.py#L186-L187\nhttps://github.com/openstack/watcher/blob/master/watcher/common/nova_helper.py#L332\nhttps://github.com/openstack/watcher/blob/master/watcher/common/nova_helper.py#L308\n\n\nresize actully doe not return anything \nhttps://github.com/openstack/watcher/blob/master/watcher/applier/actions/resize.py#L97-L98\n\nbut change nova state and change node state also return bools\n\nhttps://github.com/openstack/watcher/blob/master/watcher/applier/actions/change_nova_service_state.py#L102\nhttps://github.com/openstack/watcher/blob/master/watcher/applier/actions/change_node_power_state.py#L88-L129\n\nas does nop\n\nhttps://github.com/openstack/watcher/blob/master/watcher/applier/actions/nop.py#L64\n\nso form looking at the existing usage it shoudl be bool and this is just another example of where the comments in the docs stings are wrong.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"c95b05b4b70cee65984a1f7da3af08abbdda68dc","unresolved":true,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    def revert(self):"},{"line_number":123,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"},{"line_number":124,"context_line":"        return self._revert_stop()"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def abort(self):"},{"line_number":127,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"16666a63_cb08e1c4","line":124,"range":{"start_line":124,"start_character":0,"end_line":124,"end_character":34},"in_reply_to":"272f1159_42159b2b","updated":"2025-08-14 14:56:48.000000000","message":"it looks like all actions that have actually a revert implemente do have a return value:\n- https://github.com/openstack/watcher/blob/355671e9791d20acba21ef4af64d42c658e00276/watcher/applier/actions/change_node_power_state.py#L88\n- https://github.com/openstack/watcher/blob/355671e9791d20acba21ef4af64d42c658e00276/watcher/applier/actions/change_nova_service_state.py#L102\n- https://github.com/openstack/watcher/blob/355671e9791d20acba21ef4af64d42c658e00276/watcher/applier/actions/migration.py#L186\nin this case even if not currently used I think it\u0027s better to keep the same pattern and in the future either remove them altogether or use them","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"d4c69a31dbe5bcd36e51d4c9d5df265f1b69c847","unresolved":true,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    def revert(self):"},{"line_number":123,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"},{"line_number":124,"context_line":"        return self._revert_stop()"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def abort(self):"},{"line_number":127,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"17e05cca_1f97ea88","line":124,"range":{"start_line":124,"start_character":0,"end_line":124,"end_character":34},"in_reply_to":"28911d92_2cc0775b","updated":"2025-08-18 17:43:38.000000000","message":"Yes, but what the others actions are doing today may not be correct too, or missed some updates in the way. Looking at the revert call here: \nhttps://opendev.org/openstack/watcher/src/commit/8309d9848addd3a9647269d533aa2486dd0eeea6/watcher/applier/workflow_engine/default.py#L175\nany return value is ignored, we can leave as it is","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":true,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    def revert(self):"},{"line_number":123,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"},{"line_number":124,"context_line":"        return self._revert_stop()"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def abort(self):"},{"line_number":127,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"272f1159_42159b2b","line":124,"range":{"start_line":124,"start_character":0,"end_line":124,"end_character":34},"in_reply_to":"7d006689_151cfdb1","updated":"2025-08-09 09:55:52.000000000","message":"You\u0027re right as we can see from the method documentation. But I also find out, there are many `revert()` from other actions (`nop`, `sleep`, `migrate`) currently can return boolean, so not sure why having this inconsistency :((","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2367b54b8646585c3815ae6c1ef4c6c5e7d46774","unresolved":false,"context_lines":[{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    def revert(self):"},{"line_number":123,"context_line":"        LOG.debug(\"Reverting stop action for instance %s\", self.instance_uuid)"},{"line_number":124,"context_line":"        return self._revert_stop()"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def abort(self):"},{"line_number":127,"context_line":"        \"\"\"Abort the stop action - not applicable for stop operations\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"b6908336_a9dd3578","line":124,"range":{"start_line":124,"start_character":0,"end_line":124,"end_character":34},"in_reply_to":"e84939fb_6185a79b","updated":"2025-08-21 13:34:25.000000000","message":"Acknowledged","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":133,"context_line":"        # Check for instance existence and its state"},{"line_number":134,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":135,"context_line":"        try:"},{"line_number":136,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":137,"context_line":"            if not instance:"},{"line_number":138,"context_line":"                raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"            # Check instance current state"},{"line_number":141,"context_line":"            current_state \u003d instance.status"}],"source_content_type":"text/x-python","patch_set":11,"id":"da4257d2_0119a921","line":138,"range":{"start_line":136,"start_character":0,"end_line":138,"end_character":73},"updated":"2025-08-06 20:57:36.000000000","message":"should we consider a success if the instance does not exist anymore? or a failure? not sure what other folks think","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"44a8dfed1021170dff8546053a694608d53c3f77","unresolved":true,"context_lines":[{"line_number":133,"context_line":"        # Check for instance existence and its state"},{"line_number":134,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":135,"context_line":"        try:"},{"line_number":136,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":137,"context_line":"            if not instance:"},{"line_number":138,"context_line":"                raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"            # Check instance current state"},{"line_number":141,"context_line":"            current_state \u003d instance.status"}],"source_content_type":"text/x-python","patch_set":11,"id":"d2041775_41dce8d3","line":138,"range":{"start_line":136,"start_character":0,"end_line":138,"end_character":73},"in_reply_to":"bbb610d0_40c1735d","updated":"2025-08-20 08:48:59.000000000","message":"IMO this should be a good case for the SKIPPED state which is WIP (see https://review.opendev.org/c/openstack/watcher/+/954746/ ). Once that\u0027s merged we can probably replace this by a ActionSkipped exception.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2367b54b8646585c3815ae6c1ef4c6c5e7d46774","unresolved":true,"context_lines":[{"line_number":133,"context_line":"        # Check for instance existence and its state"},{"line_number":134,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":135,"context_line":"        try:"},{"line_number":136,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":137,"context_line":"            if not instance:"},{"line_number":138,"context_line":"                raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"            # Check instance current state"},{"line_number":141,"context_line":"            current_state \u003d instance.status"}],"source_content_type":"text/x-python","patch_set":11,"id":"d765b0a1_fed4a326","line":138,"range":{"start_line":136,"start_character":0,"end_line":138,"end_character":73},"in_reply_to":"d2041775_41dce8d3","updated":"2025-08-21 13:34:25.000000000","message":"yep i think we should defer that to next cycle not to have both depend on each other or to a follow up.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":true,"context_lines":[{"line_number":133,"context_line":"        # Check for instance existence and its state"},{"line_number":134,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":135,"context_line":"        try:"},{"line_number":136,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":137,"context_line":"            if not instance:"},{"line_number":138,"context_line":"                raise exception.InstanceNotFound(name\u003dself.instance_uuid)"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"            # Check instance current state"},{"line_number":141,"context_line":"            current_state \u003d instance.status"}],"source_content_type":"text/x-python","patch_set":11,"id":"bbb610d0_40c1735d","line":138,"range":{"start_line":136,"start_character":0,"end_line":138,"end_character":73},"in_reply_to":"da4257d2_0119a921","updated":"2025-08-09 09:55:52.000000000","message":"I also update the `pre_condition` check to be consistent with the new approach. For now, it will pass the instance non-existence case and just produce a log, and just raise the exception when the API call to find the instance fails.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":144,"context_line":""},{"line_number":145,"context_line":"            # If already stopped, that\u0027s fine"},{"line_number":146,"context_line":"            if current_state \u003d\u003d \u0027stopped\u0027:"},{"line_number":147,"context_line":"                return True"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"        except exception.InstanceNotFound:"},{"line_number":150,"context_line":"            raise"}],"source_content_type":"text/x-python","patch_set":11,"id":"eab4b05d_dd95447c","line":147,"range":{"start_line":147,"start_character":23,"end_line":147,"end_character":27},"updated":"2025-08-06 20:57:36.000000000","message":"can be None, since the pre_condition method has no return type","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":false,"context_lines":[{"line_number":144,"context_line":""},{"line_number":145,"context_line":"            # If already stopped, that\u0027s fine"},{"line_number":146,"context_line":"            if current_state \u003d\u003d \u0027stopped\u0027:"},{"line_number":147,"context_line":"                return True"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"        except exception.InstanceNotFound:"},{"line_number":150,"context_line":"            raise"}],"source_content_type":"text/x-python","patch_set":11,"id":"aee38217_6455a981","line":147,"range":{"start_line":147,"start_character":23,"end_line":147,"end_character":27},"in_reply_to":"eab4b05d_dd95447c","updated":"2025-08-09 09:55:52.000000000","message":"Done","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"aa13fbd9048c17ea6e71f385539eb03f85e50337","unresolved":true,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"64e6003f_b029d75c","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"updated":"2025-07-11 15:41:05.000000000","message":"In execute, we verify that instance is stopped before returning success. Isn\u0027t this too redundant?","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2367b54b8646585c3815ae6c1ef4c6c5e7d46774","unresolved":false,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"c4d382ae_e91a22c8","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"in_reply_to":"1a54ede2_4c81f466","updated":"2025-08-21 13:34:25.000000000","message":"Acknowledged","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":true,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"ed9607d9_ce333746","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"in_reply_to":"51a39393_9f9b8d93","updated":"2025-08-09 09:55:52.000000000","message":"If we don\u0027t think there is anything to check after `execute()`, then for now I just removed it, so the default is True as inherited from `BaseAction`, similar to other actions.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"b3564aa5b1ce1d786b34b7e6b81ab0e5d06f6b86","unresolved":true,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"e83cccc2_6ce441bf","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"in_reply_to":"647013ec_fd4ad81a","updated":"2025-07-31 00:08:19.000000000","message":"That\u0027s indeed a valid point. I agree additional check in post_condition could potentially fail due to other transient issues even when the actual action in execute actually succeeded. \nIn case we don\u0027t have any other thing to check after the action is done, we can simply remove the post_condition here as said, and let it be default from the base class. \nI also would love to hear your thoughts @jgilaber@redhat.com @smooney@redhat.com","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"fa21c16259337f6bae95b45e79d3a087fedddef1","unresolved":true,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"db9ddd41_203942f9","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"in_reply_to":"64e6003f_b029d75c","updated":"2025-07-14 15:06:41.000000000","message":"Thank you for pointing out. \nIf I\u0027m not wrong here, the actual verification for instance to be stopped is in the existing `stop_instance` in Nova Helper (with `wait_for_instance_state` called), and the `execute` here (or actually `stop` function), only returns the result from Nova Helper\u0027s `stop_instance`. \nMaybe the logs in `stop` function is redundant here, as in the end we still also have the logs with quite similar meanings in `post_condition`?","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"eb68442d1ffe503a9bb3e40d92533cc334a21f0a","unresolved":true,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"9ecb1c93_112970d3","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"in_reply_to":"6addc34d_2815a22e","updated":"2025-07-28 00:18:10.000000000","message":"You\u0027re right, so I think, in `post_condition`, we can simplify to do a quick state check without calling the wait again. Can be like:\n```\n  if current_state !\u003d \u0027stopped\u0027:\n      LOG.warning(\n          \"Instance %s is not in stopped state (state: %s)\",\n          self.instance_uuid, current_state\n      )\n      return False\n  return True\n```\nDoes it address your concern? Please let me know.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"342b183cd018dc98032438265819e511e32fa30c","unresolved":true,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"647013ec_fd4ad81a","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"in_reply_to":"9ecb1c93_112970d3","updated":"2025-07-28 06:47:28.000000000","message":"I\u0027d keep post_condition empty. execute is correctly managing the action and making sure that it reachs the desired status, no post_condition is required at all. Adding unneeded calls to post_condition makes it simply more error prone IMO","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"661351f911043c45be0855f97b0c761d7461e9be","unresolved":true,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"6addc34d_2815a22e","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"in_reply_to":"db9ddd41_203942f9","updated":"2025-07-17 12:18:21.000000000","message":"The `stop_intance` methoid you are calling in stop method waits for the instances to be stopped in https://github.com/openstack/watcher/blob/master/watcher/common/nova_helper.py#L553-L557 unless i\u0027m missing something. If it returns True, we are sure that the status is stopped.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"51a39393_9f9b8d93","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"in_reply_to":"e83cccc2_6ce441bf","updated":"2025-08-06 20:57:36.000000000","message":"I don\u0027t think that this really fits here, once the instance is stopped at the execute(), there is nothing else to check afterwards, this post_conditions are more for clean-ups, according with the documentation.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ba7e730aa69e68562edd06b98ce6cef8fc993852","unresolved":true,"context_lines":[{"line_number":157,"context_line":"        # Verify the instance is actually stopped"},{"line_number":158,"context_line":"        nova \u003d nova_helper.NovaHelper(osc\u003dself.osc)"},{"line_number":159,"context_line":"        try:"},{"line_number":160,"context_line":"            instance \u003d nova.find_instance(self.instance_uuid)"},{"line_number":161,"context_line":"            if not instance:"},{"line_number":162,"context_line":"                LOG.info("},{"line_number":163,"context_line":"                    \"Instance %s not found during post-condition check\","},{"line_number":164,"context_line":"                    self.instance_uuid"},{"line_number":165,"context_line":"                )"},{"line_number":166,"context_line":"                # Instance not found is OK - it might have been deleted"},{"line_number":167,"context_line":"                return True"},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"            current_state \u003d instance.status"},{"line_number":170,"context_line":"            LOG.debug("},{"line_number":171,"context_line":"                \"Instance %s post-condition check: state\u003d%s\","},{"line_number":172,"context_line":"                self.instance_uuid, current_state"},{"line_number":173,"context_line":"            )"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            if current_state !\u003d \u0027stopped\u0027:"},{"line_number":176,"context_line":"                if not nova.wait_for_instance_state("},{"line_number":177,"context_line":"                    instance, \u0027stopped\u0027, 8, 10"},{"line_number":178,"context_line":"                ):"},{"line_number":179,"context_line":"                    LOG.warning("},{"line_number":180,"context_line":"                        \"Instance %s may not be fully stopped (state: %s)\","},{"line_number":181,"context_line":"                        self.instance_uuid, current_state"},{"line_number":182,"context_line":"                    )"},{"line_number":183,"context_line":"                    return False"},{"line_number":184,"context_line":"            return True"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"        except Exception as exc:"},{"line_number":187,"context_line":"            LOG.warning(\"Post-condition check failed for instance %s: %s\","},{"line_number":188,"context_line":"                        self.instance_uuid, str(exc))"}],"source_content_type":"text/x-python","patch_set":11,"id":"1a54ede2_4c81f466","line":185,"range":{"start_line":160,"start_character":0,"end_line":185,"end_character":1},"in_reply_to":"ed9607d9_ce333746","updated":"2025-08-19 17:03:39.000000000","message":"for now there is not but i actully dont thnk it correct in general to verify that the action completed succesfuly in execute\n\nwe shoudl eb doing that in psot_condtion however since none of the other actions work that way today lets descope this and bring that up for wider discussion at the next ptg and keep this following the logic of the other action by doing the check in execute and having an empty post_condtion.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"}],"watcher/applier/workflow_engine/default.py":[{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"1c243f5b0154eb4f29e7c8340b1a368ce034933a","unresolved":true,"context_lines":[{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    def do_post_execute(self):"},{"line_number":162,"context_line":"        LOG.debug(\"Post-condition action: %s\", self.name)"},{"line_number":163,"context_line":"        if not self.action.post_condition():"},{"line_number":164,"context_line":"            self.engine.notify(self._db_action,"},{"line_number":165,"context_line":"                               objects.action.State.FAILED)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def do_revert(self, *args, **kwargs):"},{"line_number":168,"context_line":"        # NOTE: Not rollback action plan"}],"source_content_type":"text/x-python","patch_set":11,"id":"12e90587_41ca864f","line":165,"range":{"start_line":163,"start_character":0,"end_line":165,"end_character":59},"updated":"2025-08-06 20:57:36.000000000","message":"I guess that based on the discusstion in the Stop action, we will not raise anything there, at post_condition the execute() already went fine.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"ad6f471ea417c84849588adc5767925e22542c68","unresolved":true,"context_lines":[{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    def do_post_execute(self):"},{"line_number":162,"context_line":"        LOG.debug(\"Post-condition action: %s\", self.name)"},{"line_number":163,"context_line":"        if not self.action.post_condition():"},{"line_number":164,"context_line":"            self.engine.notify(self._db_action,"},{"line_number":165,"context_line":"                               objects.action.State.FAILED)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def do_revert(self, *args, **kwargs):"},{"line_number":168,"context_line":"        # NOTE: Not rollback action plan"}],"source_content_type":"text/x-python","patch_set":11,"id":"e153339c_572b614b","line":165,"range":{"start_line":163,"start_character":0,"end_line":165,"end_character":59},"in_reply_to":"12e90587_41ca864f","updated":"2025-08-09 09:55:52.000000000","message":"Iiuc now we don\u0027t raise anything here, as the `execute()` should handle all verification, so `post_condition` is simply the default base class implementation (or at least it\u0027s just pure boolean return)","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"44a8dfed1021170dff8546053a694608d53c3f77","unresolved":true,"context_lines":[{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    def do_post_execute(self):"},{"line_number":162,"context_line":"        LOG.debug(\"Post-condition action: %s\", self.name)"},{"line_number":163,"context_line":"        if not self.action.post_condition():"},{"line_number":164,"context_line":"            self.engine.notify(self._db_action,"},{"line_number":165,"context_line":"                               objects.action.State.FAILED)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def do_revert(self, *args, **kwargs):"},{"line_number":168,"context_line":"        # NOTE: Not rollback action plan"}],"source_content_type":"text/x-python","patch_set":11,"id":"d96679a3_3ed0c426","line":165,"range":{"start_line":163,"start_character":0,"end_line":165,"end_character":59},"in_reply_to":"2e5b404c_a42a0b24","updated":"2025-08-20 08:48:59.000000000","message":"+1 to make this a topic for PTG.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ba7e730aa69e68562edd06b98ce6cef8fc993852","unresolved":true,"context_lines":[{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    def do_post_execute(self):"},{"line_number":162,"context_line":"        LOG.debug(\"Post-condition action: %s\", self.name)"},{"line_number":163,"context_line":"        if not self.action.post_condition():"},{"line_number":164,"context_line":"            self.engine.notify(self._db_action,"},{"line_number":165,"context_line":"                               objects.action.State.FAILED)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def do_revert(self, *args, **kwargs):"},{"line_number":168,"context_line":"        # NOTE: Not rollback action plan"}],"source_content_type":"text/x-python","patch_set":11,"id":"2e5b404c_a42a0b24","line":165,"range":{"start_line":163,"start_character":0,"end_line":165,"end_character":59},"in_reply_to":"32ff967c_43211456","updated":"2025-08-19 17:03:39.000000000","message":"you are right.\n\ni thinkthis si more correct but it is nolonger needed sue to reveriting the changes i previsioulsy asked for.\n\nim not going to block this eitehr way but i think we need to dicsus the relationship between pre consditon post constion and what is and shoudl not be contained in execute.\n\ntaht could be a good ptg topic but there is alot of tech debt in this area.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"d4c69a31dbe5bcd36e51d4c9d5df265f1b69c847","unresolved":true,"context_lines":[{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    def do_post_execute(self):"},{"line_number":162,"context_line":"        LOG.debug(\"Post-condition action: %s\", self.name)"},{"line_number":163,"context_line":"        if not self.action.post_condition():"},{"line_number":164,"context_line":"            self.engine.notify(self._db_action,"},{"line_number":165,"context_line":"                               objects.action.State.FAILED)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def do_revert(self, *args, **kwargs):"},{"line_number":168,"context_line":"        # NOTE: Not rollback action plan"}],"source_content_type":"text/x-python","patch_set":11,"id":"32ff967c_43211456","line":165,"range":{"start_line":163,"start_character":0,"end_line":165,"end_character":59},"in_reply_to":"e153339c_572b614b","updated":"2025-08-18 17:43:38.000000000","message":"I mean that, since you didn\u0027t implement anything on post_condition(), this change is not needed here. I am in favor of leaving the current implementation as it is. You are also missing the watcher notification about status change, see [1] for the current implementation.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"}],"watcher/common/nova_helper.py":[{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"aa13fbd9048c17ea6e71f385539eb03f85e50337","unresolved":true,"context_lines":[{"line_number":550,"context_line":"        else:"},{"line_number":551,"context_line":"            self.nova.servers.stop(instance_id)"},{"line_number":552,"context_line":""},{"line_number":553,"context_line":"            if self.wait_for_instance_state(instance, \"stopped\", 8, 10):"},{"line_number":554,"context_line":"                LOG.debug(\"Instance %s stopped.\", instance_id)"},{"line_number":555,"context_line":"                return True"},{"line_number":556,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":11,"id":"8b34f7a4_2f809bad","line":553,"updated":"2025-07-11 15:41:05.000000000","message":"Max wait time for stopping an instance is 80 secs, i wonder if that is too small to cover all cases.\n\nI\u0027d like to get Sean opinion about it.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ba7e730aa69e68562edd06b98ce6cef8fc993852","unresolved":false,"context_lines":[{"line_number":550,"context_line":"        else:"},{"line_number":551,"context_line":"            self.nova.servers.stop(instance_id)"},{"line_number":552,"context_line":""},{"line_number":553,"context_line":"            if self.wait_for_instance_state(instance, \"stopped\", 8, 10):"},{"line_number":554,"context_line":"                LOG.debug(\"Instance %s stopped.\", instance_id)"},{"line_number":555,"context_line":"                return True"},{"line_number":556,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":11,"id":"8366f1ff_75c325a7","line":553,"in_reply_to":"57ac8bff_d0ab9a32","updated":"2025-08-19 17:03:39.000000000","message":"Acknowledged","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"77b1d6cb4b739e693323e24377fcb4f6fd3a9c61","unresolved":true,"context_lines":[{"line_number":550,"context_line":"        else:"},{"line_number":551,"context_line":"            self.nova.servers.stop(instance_id)"},{"line_number":552,"context_line":""},{"line_number":553,"context_line":"            if self.wait_for_instance_state(instance, \"stopped\", 8, 10):"},{"line_number":554,"context_line":"                LOG.debug(\"Instance %s stopped.\", instance_id)"},{"line_number":555,"context_line":"                return True"},{"line_number":556,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":11,"id":"f2c85aea_61960905","line":553,"in_reply_to":"67b77695_4a6ec4f4","updated":"2025-07-14 18:23:44.000000000","message":"In nova the graceful shutdown timeout is 60 seconds by default\nhttps://docs.openstack.org/nova/latest/configuration/config.html#DEFAULT.shutdown_timeout\n\nafter that nova will escalate to effectively a sig_kill.\nIn the context of the libvirt driver, libvirt inernally has its own loop where it will escalate form SIGTERM to SIGKILL so its very rare that nova actuly need to escalate and directly kill the vm. when it does it generally means the vm is in a non killable state i.e. it blocked on hardware device io.\n\nim inclined ot say we may want to have a operational bug to address the hardcoding in general but i feel like that is out of scope of this patch to fix.\n\nim thinking we will want to extend watchers nova section with a dict of action to timeout.  probably 2 dicts to specify interval and count.\n\ni agree that on some cloud this may time out but i think the probability of that is low, as a nova core my expections is stoping the vm shoudl be single digit second on a healty cloud. slightly mroe if the nova instance is an ironic node but for libvirt vms we shoudl not expect it to take more then 80 seconds.\n\nin general hard-coding reties like this often is problematic out side fo a devstack environment so i do share the concern but i am ok with not addressing this for now\nespically given the time it has been this way.\n\n@quang.ngo@canonical.com unfortunetlly becuase the orginal core team is no longer active and we do not have feedback form a large user base we cant nessiarly rely on the fact that it appreas to have work well for 9 year entirly. but im not aware of any current buts or user feed back that its problematic so im inclide to think it works well enough for now.","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"fa21c16259337f6bae95b45e79d3a087fedddef1","unresolved":true,"context_lines":[{"line_number":550,"context_line":"        else:"},{"line_number":551,"context_line":"            self.nova.servers.stop(instance_id)"},{"line_number":552,"context_line":""},{"line_number":553,"context_line":"            if self.wait_for_instance_state(instance, \"stopped\", 8, 10):"},{"line_number":554,"context_line":"                LOG.debug(\"Instance %s stopped.\", instance_id)"},{"line_number":555,"context_line":"                return True"},{"line_number":556,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":11,"id":"67b77695_4a6ec4f4","line":553,"in_reply_to":"8b34f7a4_2f809bad","updated":"2025-07-14 15:06:41.000000000","message":"Thank you for the concern. I don\u0027t know about the practical duration for an instance to get fully stopped, or the time it may be up to in edge cases... I notice that 80-sec waiting duration was actually set from a commit of 9 years ago, so maybe it has worked quite well throughout time? \nAdditionally, for another safety layer, I already made the `post_condition` method in the stop action also call `wait_for_instance_state` with same duration, so in fact we now have up to 160s waiting for an instance to be stopped I guess.\nIt\u0027s nice to learn more about the cases where we actually need more time.\n\nWould love to hear your opinion @smooney@redhat.com","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"f5b0276ea72c470b7ef4e78926a289903e63b14f","unresolved":true,"context_lines":[{"line_number":550,"context_line":"        else:"},{"line_number":551,"context_line":"            self.nova.servers.stop(instance_id)"},{"line_number":552,"context_line":""},{"line_number":553,"context_line":"            if self.wait_for_instance_state(instance, \"stopped\", 8, 10):"},{"line_number":554,"context_line":"                LOG.debug(\"Instance %s stopped.\", instance_id)"},{"line_number":555,"context_line":"                return True"},{"line_number":556,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":11,"id":"57ac8bff_d0ab9a32","line":553,"in_reply_to":"f2c85aea_61960905","updated":"2025-07-16 01:38:04.000000000","message":"Thanks for sharing the info!","commit_id":"bfbb2680ef0a53eeec1eabef9bbee00a971749cf"}],"watcher/decision_engine/planner/weight.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"552ce67deca9f860eb3a218d8b5c5e9c65cfab7e","unresolved":true,"context_lines":[{"line_number":32,"context_line":"    \"\"\"Weight planner implementation"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    This implementation builds actions with parents in accordance with weights."},{"line_number":35,"context_line":"    Set of actions having a higher weight will be scheduled before"},{"line_number":36,"context_line":"    the other ones. There are two config options to configure:"},{"line_number":37,"context_line":"    action_weights and parallelization."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"    *Limitations*"}],"source_content_type":"text/x-python","patch_set":9,"id":"33b5e5d0_15a61cc1","line":36,"range":{"start_line":35,"start_character":3,"end_line":36,"end_character":18},"updated":"2025-07-03 16:24:40.000000000","message":"OK so based on this you are giving stop a higher priority to stop then migrate.\n\ni think this is OK. really i don\u0027t think there is a priority between them but since we cant have multiple actions with the same weight today either order is fine.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"14cfd69ffc0a2f39300d2eda30cf207661bc48e9","unresolved":true,"context_lines":[{"line_number":32,"context_line":"    \"\"\"Weight planner implementation"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    This implementation builds actions with parents in accordance with weights."},{"line_number":35,"context_line":"    Set of actions having a higher weight will be scheduled before"},{"line_number":36,"context_line":"    the other ones. There are two config options to configure:"},{"line_number":37,"context_line":"    action_weights and parallelization."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"    *Limitations*"}],"source_content_type":"text/x-python","patch_set":9,"id":"bcd1b6b6_147ea77a","line":36,"range":{"start_line":35,"start_character":3,"end_line":36,"end_character":18},"in_reply_to":"33b5e5d0_15a61cc1","updated":"2025-07-04 04:55:39.000000000","message":"You\u0027re right, I guess we now don\u0027t have a case where `stop` and `migrate` actions both exist in an action plan, so not really have a priority between them.  Intuitively, I just feel like a `stop` is somewhat less heavy-duty/impactful compare to a `migrate`, and a bit more than a`sleep`, that\u0027s why I put it in between `sleep` and `migrate`.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"eb21c241b65a3af27f5f610b67623ef50296c4d9","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    \"\"\"Weight planner implementation"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    This implementation builds actions with parents in accordance with weights."},{"line_number":35,"context_line":"    Set of actions having a higher weight will be scheduled before"},{"line_number":36,"context_line":"    the other ones. There are two config options to configure:"},{"line_number":37,"context_line":"    action_weights and parallelization."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"    *Limitations*"}],"source_content_type":"text/x-python","patch_set":9,"id":"7b2b8844_aad35ff5","line":36,"range":{"start_line":35,"start_character":3,"end_line":36,"end_character":18},"in_reply_to":"bcd1b6b6_147ea77a","updated":"2025-07-04 11:50:38.000000000","message":"i agree althou im not sure why    \u0027volume_migrate\u0027: 60, is considerd less closly then server migrage.\n\nvolume migrate involes transfering much much more data then server migrate does in general.\nim not gong to ask you to change that but i woudl\n\nhave palced  \u0027volume_migrate\u0027 at 15 not 60 so im not entirly sure what critia was orginally applied when selecting the existign weights\n\n\ni suspect orginaly that the were not usign cost as a metric and were instead thinking mroe about relitive orderign of what to do first i.e. they wanted volumes to be moved first before vms.\n\n\nin any case im ok with your current ordering/weight.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"}],"watcher/decision_engine/strategy/strategies/host_maintenance.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1c13f19891900d619d89ada88e8991fbff5125b5","unresolved":true,"context_lines":[{"line_number":229,"context_line":"                # Non-active instance, cold migration disabled, do nothing"},{"line_number":230,"context_line":"                return"},{"line_number":231,"context_line":"            # Cold migrate non-active instances"},{"line_number":232,"context_line":"            migration_type \u003d \u0027cold\u0027"},{"line_number":233,"context_line":""},{"line_number":234,"context_line":"        params \u003d {\u0027migration_type\u0027: migration_type,"},{"line_number":235,"context_line":"                  \u0027source_node\u0027: src_node.uuid,"}],"source_content_type":"text/x-python","patch_set":4,"id":"f8b5946a_72b29d03","line":232,"updated":"2025-06-17 11:21:43.000000000","message":"this is incorrect we shoudl not stop the instnace before cold migrating it.\n\nthat generall considerd an anti pattern in nova as it prevent verifyign that the instace is function in the verify_resize state before confirming the migration.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"aa0a279dc69aab8c34920040b75deb74b94643a3","unresolved":false,"context_lines":[{"line_number":229,"context_line":"                # Non-active instance, cold migration disabled, do nothing"},{"line_number":230,"context_line":"                return"},{"line_number":231,"context_line":"            # Cold migrate non-active instances"},{"line_number":232,"context_line":"            migration_type \u003d \u0027cold\u0027"},{"line_number":233,"context_line":""},{"line_number":234,"context_line":"        params \u003d {\u0027migration_type\u0027: migration_type,"},{"line_number":235,"context_line":"                  \u0027source_node\u0027: src_node.uuid,"}],"source_content_type":"text/x-python","patch_set":4,"id":"61e12da9_2c23c719","line":232,"in_reply_to":"f8b5946a_72b29d03","updated":"2025-06-18 14:24:25.000000000","message":"Thank you for mentioning that. I have changed the logic to set a cold migration for active instance in case disable_live_migration is set, but cold migration is still allowed.","commit_id":"f6af95adab99818dd081fddd436846723d32ff3e"}],"watcher/tests/applier/actions/test_stop.py":[{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"5cc6058c9acdb99404b1c57e29ef9a077da32ee7","unresolved":true,"context_lines":[{"line_number":32,"context_line":"    def setUp(self):"},{"line_number":33,"context_line":"        super(TestStop, self).setUp()"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"        self.m_osc_cls \u003d mock.Mock()"},{"line_number":36,"context_line":"        self.m_helper_cls \u003d mock.Mock()"},{"line_number":37,"context_line":"        self.m_helper \u003d mock.Mock(spec\u003dnova_helper.NovaHelper)"},{"line_number":38,"context_line":"        self.m_helper_cls.return_value \u003d self.m_helper"}],"source_content_type":"text/x-python","patch_set":8,"id":"ecae2f92_1407d16a","line":35,"updated":"2025-06-27 11:23:29.000000000","message":"I don\u0027t think we need so many mocks. I know this is probably following the existing pattern in watcher unit tests, but I think it\u0027s worth it to do testing better for new ones. I\u0027ve tried locally to replace lines 35-52 with the following snippet\n```suggestion\n        m_nova_helper \u003d mock.patch(\"watcher.common.nova_helper.NovaHelper\", autospec\u003dTrue)\n        self.m_helper \u003d m_nova_helper.start().return_value\n        self.addCleanup(m_nova_helper.stop)\n```\nthat seems to be enough to make the test pass, and is much simpler. I\u0027m not an expert on mocking tbh so there could be something I\u0027m missing. @smooney@redhat.com you know better than me about this topic, wdyt about my proposal?","commit_id":"71f8f7ddae95eeb643bfb5e9eb3d9f69e5fcf13f"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"09836293d7bd394fdf08d51893112f8dc82d2f2c","unresolved":true,"context_lines":[{"line_number":32,"context_line":"    def setUp(self):"},{"line_number":33,"context_line":"        super(TestStop, self).setUp()"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"        self.m_osc_cls \u003d mock.Mock()"},{"line_number":36,"context_line":"        self.m_helper_cls \u003d mock.Mock()"},{"line_number":37,"context_line":"        self.m_helper \u003d mock.Mock(spec\u003dnova_helper.NovaHelper)"},{"line_number":38,"context_line":"        self.m_helper_cls.return_value \u003d self.m_helper"}],"source_content_type":"text/x-python","patch_set":8,"id":"52932507_274a522b","line":35,"in_reply_to":"1c94557a_20e9dc1c","updated":"2025-06-27 15:03:35.000000000","message":"thanks for the link, I did not know about that fixture. I\u0027m curious as to why use `autospec\u003dFalse` though, isn\u0027t it better to set it to True to catch things like typos or changes in method names?\n\nAlso a note for anyone that might read this chain, to make it work with the tests as written in patchset 8, I needed to use\n```\n   self.m_helper \u003d self.useFixture(fixture: fixtures.MockPatch(\n       \"watcher.common.nova_helper.NovaHelper\", autospec\u003dFalse)).mock.return_value\n```","commit_id":"71f8f7ddae95eeb643bfb5e9eb3d9f69e5fcf13f"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"d32ba5fe444279f9b186a139c09cc52ffbcc845a","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    def setUp(self):"},{"line_number":33,"context_line":"        super(TestStop, self).setUp()"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"        self.m_osc_cls \u003d mock.Mock()"},{"line_number":36,"context_line":"        self.m_helper_cls \u003d mock.Mock()"},{"line_number":37,"context_line":"        self.m_helper \u003d mock.Mock(spec\u003dnova_helper.NovaHelper)"},{"line_number":38,"context_line":"        self.m_helper_cls.return_value \u003d self.m_helper"}],"source_content_type":"text/x-python","patch_set":8,"id":"7084f796_1bba2b08","line":35,"in_reply_to":"52932507_274a522b","updated":"2025-07-03 15:42:55.000000000","message":"Acknowledged","commit_id":"71f8f7ddae95eeb643bfb5e9eb3d9f69e5fcf13f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"97dbbb2701e0bdb3f80383dbf9d96334ab81ad43","unresolved":true,"context_lines":[{"line_number":32,"context_line":"    def setUp(self):"},{"line_number":33,"context_line":"        super(TestStop, self).setUp()"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"        self.m_osc_cls \u003d mock.Mock()"},{"line_number":36,"context_line":"        self.m_helper_cls \u003d mock.Mock()"},{"line_number":37,"context_line":"        self.m_helper \u003d mock.Mock(spec\u003dnova_helper.NovaHelper)"},{"line_number":38,"context_line":"        self.m_helper_cls.return_value \u003d self.m_helper"}],"source_content_type":"text/x-python","patch_set":8,"id":"840867a4_d9cdb457","line":35,"in_reply_to":"52932507_274a522b","updated":"2025-07-03 15:36:02.000000000","message":"oh that was just an example we can use true and shoudl in many cases","commit_id":"71f8f7ddae95eeb643bfb5e9eb3d9f69e5fcf13f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4fc3507d95b7cc3f49122f8047d29d6891d4a695","unresolved":true,"context_lines":[{"line_number":32,"context_line":"    def setUp(self):"},{"line_number":33,"context_line":"        super(TestStop, self).setUp()"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"        self.m_osc_cls \u003d mock.Mock()"},{"line_number":36,"context_line":"        self.m_helper_cls \u003d mock.Mock()"},{"line_number":37,"context_line":"        self.m_helper \u003d mock.Mock(spec\u003dnova_helper.NovaHelper)"},{"line_number":38,"context_line":"        self.m_helper_cls.return_value \u003d self.m_helper"}],"source_content_type":"text/x-python","patch_set":8,"id":"1c94557a_20e9dc1c","line":35,"in_reply_to":"ecae2f92_1407d16a","updated":"2025-06-27 14:31:17.000000000","message":"the way i would normally write this is like this\n\n```\nimport fixtures\n```\n\n```\n         self.m_helper \u003d self.useFixture(fixtures.MockPatch(\n           \"watcher.common.nova_helper.NovaHelper\", autospec\u003dFalse))\n```\nWe should not be manually creating, starting, and cleaning up the patcher objects.\n\nWatches current test quite is very outdated and using a lot of legacy patterns that we not considered a best practice even for a very long time.\n\nmanually strating the patcher and stoping it with addCleanup is long and more  error prone so we generally perfer to use \n\nhttps://testtools.readthedocs.io/en/latest/api.html#testtools.TestCase.useFixture\ncombinded with https://github.com/testing-cabal/fixtures/blob/master/fixtures/_fixtures/mockpatch.py\n\nto set up test fixtures/mocks in the setUp functions","commit_id":"71f8f7ddae95eeb643bfb5e9eb3d9f69e5fcf13f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2367b54b8646585c3815ae6c1ef4c6c5e7d46774","unresolved":true,"context_lines":[{"line_number":24,"context_line":"from watcher.tests import base"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"class TestStop(base.TestCase):"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    INSTANCE_UUID \u003d \"45a37aeb-95ab-4ddb-a305-7d9f62c2f5ba\""},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"c08619de_7090cc1d","line":27,"updated":"2025-08-21 13:34:25.000000000","message":"looking at the coverage report we have pretty good coverage over all\n\nhttps://7f7e5ab8a7159f9220a2-4781fed732bcaf2a49f1da3bb2ee8431.ssl.cf2.rackcdn.com/openstack/3b9aebf892b14f4e92d0781cdc16c2d1/cover/z_5d565cea790a511b_stop_py.html\n\nwe are missing coverave of abort \n\nand the case where nova client raises an excption.\n\ni am ok with adresing both of those in a follwo up.","commit_id":"cc26b3b334e5d60bf04c927c771d572445e4a8bc"}],"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":"552ce67deca9f860eb3a218d8b5c5e9c65cfab7e","unresolved":true,"context_lines":[{"line_number":241,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":242,"context_line":"        instance_id \u003d utils.generate_uuid()"},{"line_number":243,"context_line":"        server \u003d self.fake_server(instance_id)"},{"line_number":244,"context_line":"        setattr(server, \u0027OS-EXT-STS:vm_state\u0027, \u0027active\u0027)"},{"line_number":245,"context_line":"        self.fake_nova_find_list("},{"line_number":246,"context_line":"            nova_util,"},{"line_number":247,"context_line":"            fake_find\u003dserver,"}],"source_content_type":"text/x-python","patch_set":9,"id":"d34ee7d0_f246543a","line":244,"updated":"2025-07-03 16:24:40.000000000","message":"we shhould not be using setattr in general even in test code.\n\n\n\nin openstack we avoid using getattr,setattr or hasattr.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"2740977a699b533c07f3d2069262e2feee451acf","unresolved":false,"context_lines":[{"line_number":241,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":242,"context_line":"        instance_id \u003d utils.generate_uuid()"},{"line_number":243,"context_line":"        server \u003d self.fake_server(instance_id)"},{"line_number":244,"context_line":"        setattr(server, \u0027OS-EXT-STS:vm_state\u0027, \u0027active\u0027)"},{"line_number":245,"context_line":"        self.fake_nova_find_list("},{"line_number":246,"context_line":"            nova_util,"},{"line_number":247,"context_line":"            fake_find\u003dserver,"}],"source_content_type":"text/x-python","patch_set":9,"id":"81563027_bdcb7afb","line":244,"in_reply_to":"4e682cde_920b88d8","updated":"2025-07-04 05:15:38.000000000","message":"Oh, actually, not for this one... I mean, almost all the existing tests in `test_nova_helper.py`are using `setattr`, so here I kinda follow that pattern.\n\nI can have another patch in this topic to fix my own implementation on `test_start_instance`, but ideally I guess we can have another topic to refactor all the tests using `*attr`,since it\u0027s a different scope.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"eb21c241b65a3af27f5f610b67623ef50296c4d9","unresolved":false,"context_lines":[{"line_number":241,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":242,"context_line":"        instance_id \u003d utils.generate_uuid()"},{"line_number":243,"context_line":"        server \u003d self.fake_server(instance_id)"},{"line_number":244,"context_line":"        setattr(server, \u0027OS-EXT-STS:vm_state\u0027, \u0027active\u0027)"},{"line_number":245,"context_line":"        self.fake_nova_find_list("},{"line_number":246,"context_line":"            nova_util,"},{"line_number":247,"context_line":"            fake_find\u003dserver,"}],"source_content_type":"text/x-python","patch_set":9,"id":"301bb7cb_b2aac410","line":244,"in_reply_to":"81563027_bdcb7afb","updated":"2025-07-04 11:50:38.000000000","message":"i guess that\u0027s fair, i tend to avoid adding new technical debt when adding new tests but i will accept the consistency argument and that its not really in socope of your change to fix existing bad practices.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"14cfd69ffc0a2f39300d2eda30cf207661bc48e9","unresolved":false,"context_lines":[{"line_number":241,"context_line":"        nova_util \u003d nova_helper.NovaHelper()"},{"line_number":242,"context_line":"        instance_id \u003d utils.generate_uuid()"},{"line_number":243,"context_line":"        server \u003d self.fake_server(instance_id)"},{"line_number":244,"context_line":"        setattr(server, \u0027OS-EXT-STS:vm_state\u0027, \u0027active\u0027)"},{"line_number":245,"context_line":"        self.fake_nova_find_list("},{"line_number":246,"context_line":"            nova_util,"},{"line_number":247,"context_line":"            fake_find\u003dserver,"}],"source_content_type":"text/x-python","patch_set":9,"id":"4e682cde_920b88d8","line":244,"in_reply_to":"d34ee7d0_f246543a","updated":"2025-07-04 04:55:39.000000000","message":"Thanks for that insight! It\u0027s good to know about this practice. I have removed all the `*attr` in the test and my source code.","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"}],"watcher/tests/decision_engine/strategy/strategies/test_host_maintenance.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"552ce67deca9f860eb3a218d8b5c5e9c65cfab7e","unresolved":false,"context_lines":[{"line_number":226,"context_line":"        result \u003d self.strategy.post_execute()"},{"line_number":227,"context_line":"        self.assertIsNone(result)"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"    # \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"},{"line_number":230,"context_line":"    # Tests for disable migration parameters"},{"line_number":231,"context_line":"    # \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"    def test_schema_default_values(self):"},{"line_number":234,"context_line":"        \"\"\"Test that disable_* parameters default to False when not provided\"\"\""},{"line_number":235,"context_line":"        parameters \u003d {\"maintenance_node\": \"hostname_0\"}"}],"source_content_type":"text/x-python","patch_set":9,"id":"489c433d_1e936924","line":232,"range":{"start_line":229,"start_character":3,"end_line":232,"end_character":1},"updated":"2025-07-03 16:24:40.000000000","message":"in general we avoid using comment marker like this in open stack.\n\nwhile the can be useful they tend to get out of data and not age well so i would prefer if you removed this","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"},{"author":{"_account_id":38075,"name":"Ho Minh Quang Ngo","display_name":"Quang Ngo","email":"quang.ngo@canonical.com","username":"Quang-Ngo"},"change_message_id":"14cfd69ffc0a2f39300d2eda30cf207661bc48e9","unresolved":false,"context_lines":[{"line_number":226,"context_line":"        result \u003d self.strategy.post_execute()"},{"line_number":227,"context_line":"        self.assertIsNone(result)"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"    # \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"},{"line_number":230,"context_line":"    # Tests for disable migration parameters"},{"line_number":231,"context_line":"    # \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"    def test_schema_default_values(self):"},{"line_number":234,"context_line":"        \"\"\"Test that disable_* parameters default to False when not provided\"\"\""},{"line_number":235,"context_line":"        parameters \u003d {\"maintenance_node\": \"hostname_0\"}"}],"source_content_type":"text/x-python","patch_set":9,"id":"50197fcb_891da700","line":232,"range":{"start_line":229,"start_character":3,"end_line":232,"end_character":1},"in_reply_to":"489c433d_1e936924","updated":"2025-07-04 04:55:39.000000000","message":"I have removed it, thanks for reminding me!","commit_id":"a10fecbf8c144e4262821c4d5c4a8e6beee4fa6d"}]}