)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a2cbcca6f09d540a399529335d25b3c4b2fbe5b6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"710c817c_35899fd4","updated":"2023-02-14 08:20:48.000000000","message":"recheck tempest-integrated-compute env failed (unrelated)","commit_id":"f87275cbedb01d49c14bf5b89fd832eff2a4ae16"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"e750537690ba44129ef47c3ecfa2a1fbe5c50974","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"00bcfe00_f5623ef1","updated":"2023-02-16 14:42:50.000000000","message":"Much better, thanks! One not inline about putting this in the instance object.","commit_id":"617c3c14c560f9be9673f9a219b0ce5b20b45601"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"6bdd94f197410d5794ad5151874b964c3e77abb1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"8e17cebc_6f019fd9","updated":"2023-03-17 19:15:10.000000000","message":"I think we\u0027re close, as far as I can tell (though I want to do another last review after fixing this) we\u0027re only missing a target_cell() context","commit_id":"9a5ec249fdc2365d86455e7a4c512af86e1950ef"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"702255755cac1667d1640d8cb3ca7ff4758ef792","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"72b6437e_7f42e985","updated":"2023-03-20 10:58:56.000000000","message":"tested via id of context, and reference exists in lock_instance context manager.\nhope I understood and tested this correctly, please let me know if its not the correct way.\n","commit_id":"9a5ec249fdc2365d86455e7a4c512af86e1950ef"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"938bcc2aeda251750445299953f5d09d89721f02","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"6eba688b_29a4f7a7","updated":"2023-06-21 19:48:21.000000000","message":"I think what you\u0027ve done works, but for some reason the fact that you\u0027ve changed both refresh() and _refresh() makes it really hard for my brain to ascertain that nothing has functionally changed except for the new lock/unlock context manager.\n\nFor that reason, can I ask that you change the patch to leave as much code unchanged as possible, including refresh() and _refresh(), and just add the new context manager around lines 3022 through 3144?\n\nYou don\u0027t need to pass compute_api to the context manager, it can fetch it like it\u0027s currently done on L3044 (so that means that the current L3044 can be removed because compute_api is only needed to lock/unlock AFAICT).\n\nFor separation of concerns and to avoid coupling things, the lock/unlock context manager should probably redo the cell targeting, so L3077 can go inside there as well.","commit_id":"dbdeb091206bbaf1e5a131b5bbe4f14d02c57937"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"dbd4f19206e3e3d3d30dae58b28eb2d4e0c8890a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"b43c28e8_dd0b7a54","updated":"2023-03-21 11:21:40.000000000","message":"recheck tempest test basic ops failed because of timeout - unrelated","commit_id":"dbdeb091206bbaf1e5a131b5bbe4f14d02c57937"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"20a65ba65337991a05d9d0f245f047ca56c63d7c","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":8,"id":"c1772f59_2eb3c506","in_reply_to":"6eba688b_29a4f7a7","updated":"2023-06-27 20:25:58.000000000","message":"updated patch to leave as much code unchanged as possible,\n\nrefresh calls _refresh # as earlier\n_refresh verify instance state and if its locked and prceed to voume refresh\n\n#changes\n_refresh calls _do_refresh on locked instance\n\n_do_refresh perfom refresh operation.\n\ncompute_api is created inside locked_instance context-manager instead of passing as parameter.\nlock/unlock context manager redo the cell targeting.","commit_id":"dbdeb091206bbaf1e5a131b5bbe4f14d02c57937"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"637fbfd7436a297f4929cb07d828f5ceee4404ac","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"2c7592f7_742c1ff0","in_reply_to":"c1772f59_2eb3c506","updated":"2023-07-11 15:05:07.000000000","message":"Done","commit_id":"dbdeb091206bbaf1e5a131b5bbe4f14d02c57937"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"7c51377d4b12b9e0cac2311f186182de58dddf56","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"7920921f_f49bfb98","updated":"2023-07-10 21:13:41.000000000","message":"Ah, this is much easier to understand and follow, thanks!","commit_id":"760ae15eccfd2ffcb5850530a7ce188abc9c2a1d"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"20a65ba65337991a05d9d0f245f047ca56c63d7c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"684de8f3_2fb149d5","updated":"2023-06-27 20:25:58.000000000","message":"recheck nova-grenade-multinode failed deployment issue","commit_id":"760ae15eccfd2ffcb5850530a7ce188abc9c2a1d"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"1ae428415d0d2be962ae4364d53318f3584d49c3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"8f8d849e_a2189b2b","updated":"2023-06-27 20:29:32.000000000","message":"recheck nova-grenade-multinode failed deployment issue","commit_id":"760ae15eccfd2ffcb5850530a7ce188abc9c2a1d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"405c0a8f17de585c0c3b2a8f411fb57f2617eee1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"a0719c69_7972af0a","updated":"2023-07-14 14:28:53.000000000","message":"I still think this is wrong.","commit_id":"8e132e87259820d5835f6ba5d5863008846f461f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a95903e33780f797540880afe6b7bb5ad70b205c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"6da434a1_3c43bba8","updated":"2023-07-17 18:47:52.000000000","message":"-1 for some redundant code, the instance.locked check that raises InstanceInvalidState, and missing test coverage that didn\u0027t catch the InstanceInvalidState issue","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"808b5845ea965e0f7c4bb4bc1c047859b90a2800","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"b4af2e1b_a20d5e2f","updated":"2023-07-17 20:45:49.000000000","message":"Looks like we need multiple target_cell after all 😬","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"163dc126fd98c8276bc1f9862ba9b0bfa79abf21","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"9987eb29_3ca52f3c","updated":"2023-07-17 17:38:49.000000000","message":"recheck ssh timeout in tempest test","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"009cd064f93a94e4a15ca409585056a67854aeda","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":17,"id":"f752665c_e0178588","updated":"2023-07-21 15:57:25.000000000","message":"I guess I\u0027ve lost track of what the original goal was with this patch. I thought it was just to make the lock-if-locked-else-not behavior clean and in a context manager, but...what are we actually trying to improve here? The referenced bug just says that the problem we\u0027re fixing is that it\u0027s not currently a context manager...which isn\u0027t a bug.","commit_id":"50bebe5e08a930b9e3547b26eadfcf0acdf6c509"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"cbabfefb0e9dea164461254170b572d061ced88d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"beffd050_71ed24c4","updated":"2023-08-08 12:01:32.000000000","message":"recheck Skip test_image_tasks_create()   is merged\nhttps://review.opendev.org/c/openstack/tempest/+/890687","commit_id":"3a2231841e698576c81de3fde6f388129254fce9"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"fc170b3385b564cebcd081163d5a8f70a7fc5225","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"eb0ccc88_a929d751","updated":"2023-08-10 15:05:16.000000000","message":"recheck test_shelve_volume_backed_instance test request timeout on waiting on server create - unrelated","commit_id":"3a2231841e698576c81de3fde6f388129254fce9"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"8937139cac8ffc57864bdb39e5139cbcef92054a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":22,"id":"16a7eeb5_dd08176f","updated":"2023-09-05 03:02:28.000000000","message":"See inline - the previous behaviour was to abort if the instance was already locked. Do we want to continue doing that? If so, you don\u0027t need to remember the initial state (locked vs unlocked), because we\u0027ll only be doing that if the instance is unlocked.","commit_id":"c74e98495182000f3df56f26e3473e46aba1b2f1"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"6dbd87a715fcb228ca54ccdbae0495fe385678b1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"68d2047f_a11e9177","updated":"2023-11-20 05:01:38.000000000","message":"recheck fail logs are gone","commit_id":"fa7aa0c7cd623de9f31cf79a68e19710961f3b8a"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"017ae6bc3f4b8c648f64def6a712725d5e4b016e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"1e4a68e7_1db16680","updated":"2023-09-12 08:41:39.000000000","message":"recheck rare bug https://bugs.launchpad.net/nova/+bug/2035095","commit_id":"fa7aa0c7cd623de9f31cf79a68e19710961f3b8a"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"ee242bc9f1d85412535a468da08ccdd70e5c2ff4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"dd76ec77_89385ff1","updated":"2023-09-11 09:06:02.000000000","message":"recheck test_live_block_migration fail unrelated","commit_id":"fa7aa0c7cd623de9f31cf79a68e19710961f3b8a"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"f17215a2fd43bc02d8459d0b5bd1df0f95523434","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"1e0fa68f_dc689105","updated":"2023-11-21 03:19:57.000000000","message":"recheck unrelated fail","commit_id":"fa7aa0c7cd623de9f31cf79a68e19710961f3b8a"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"18d87e7a1d767338c3a7aa4c7445b082dfe00a9a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":24,"id":"ac6df702_6ed80d77","updated":"2024-02-12 00:49:33.000000000","message":"I think this is good. There are unit tests for the new context manager, the existing tests are unchanged (as they should be), except for the case when the instance was already and which used to throw an error, but we\u0027ve determined that was because the original implementation didn\u0027t want to end up unlocking an instance that was locked before we started,","commit_id":"8445082654644f338044034cf4636217110d2101"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"c8e4032afcc64350884eb237dbe39c007d05c0ca","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"79a468fd_1dc1d880","updated":"2024-02-13 05:04:28.000000000","message":"recheck grenade","commit_id":"ad14712c721681112ddf54a5e91349407b5e96a3"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2ab382cbc5334fbfbeb46ced3c61b13dc44b5d35","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"986c01b5_784c25fd","updated":"2024-02-13 12:53:06.000000000","message":"the return 0 annoys me but not enough to -1","commit_id":"ad14712c721681112ddf54a5e91349407b5e96a3"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"dd3d634e92f05927c648461458a0c45ed0cfddc7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":27,"id":"d88b4342_9ebe83b4","updated":"2024-02-29 12:15:46.000000000","message":"Sane fix","commit_id":"211737d0ce23a8a9b1f3953f4816150a9a3271fa"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"6e4440039a4af5af7e689a5e813215d255327b5f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":27,"id":"0d7f153b_234c8398","updated":"2024-02-16 10:50:23.000000000","message":"recheck unrelated","commit_id":"211737d0ce23a8a9b1f3953f4816150a9a3271fa"}],"nova/cmd/common.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"f872b108ba71d32877c1afe06f08b48fc7fae9f6","unresolved":true,"context_lines":[{"line_number":35,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def lock_and_unlock_instance(operation_on_instance):"},{"line_number":39,"context_line":"    def _decorator(*args):"},{"line_number":40,"context_line":"        instance_uuid \u003d args[1]"},{"line_number":41,"context_line":"        volume_id \u003d args[2]"}],"source_content_type":"text/x-python","patch_set":2,"id":"54efef32_153678ab","line":38,"updated":"2023-02-16 03:03:21.000000000","message":"I think I was too cavalier in my explanations on internal Slack. What I would do is to use the contextlib library [1] to write a new context manager. There are a few examples in the existing Nova code, the most useful for you is probably mutated_migration_context. I\u0027ve left comments so that you can see where it\u0027s defined and how it\u0027s used. The basic idea is:\n\n1. Do some stuff - in your case it would be locking the instance.\n2. try/yield - enter what\u0027s inside the `with` block when the context manager is called\n3. finally - do some more stuff, regardless of any exceptions thrown inside the `with` block. In your case, it would be unlocking the instance if necessary.\n\nHope that helps!\n\n[1] https://docs.python.org/3/library/contextlib.html","commit_id":"f87275cbedb01d49c14bf5b89fd832eff2a4ae16"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"db15dabeab0505a5a671f32c4d5c048710cf2062","unresolved":true,"context_lines":[{"line_number":35,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def lock_and_unlock_instance(operation_on_instance):"},{"line_number":39,"context_line":"    def _decorator(*args):"},{"line_number":40,"context_line":"        instance_uuid \u003d args[1]"},{"line_number":41,"context_line":"        volume_id \u003d args[2]"}],"source_content_type":"text/x-python","patch_set":2,"id":"e7806ab0_555f01f0","line":38,"in_reply_to":"54efef32_153678ab","updated":"2023-02-16 14:25:37.000000000","message":"Done","commit_id":"f87275cbedb01d49c14bf5b89fd832eff2a4ae16"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"e28a66c7d3bc3c158c0eb98470795758e7f672b3","unresolved":false,"context_lines":[{"line_number":35,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def lock_and_unlock_instance(operation_on_instance):"},{"line_number":39,"context_line":"    def _decorator(*args):"},{"line_number":40,"context_line":"        instance_uuid \u003d args[1]"},{"line_number":41,"context_line":"        volume_id \u003d args[2]"}],"source_content_type":"text/x-python","patch_set":2,"id":"9872437e_6ac688d2","line":38,"in_reply_to":"e7806ab0_555f01f0","updated":"2023-02-17 08:01:26.000000000","message":"Done","commit_id":"f87275cbedb01d49c14bf5b89fd832eff2a4ae16"}],"nova/cmd/manage.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"462fbec0745f8eab18d66a5b9f722cf826a100da","unresolved":true,"context_lines":[{"line_number":3022,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3023,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"},{"line_number":3024,"context_line":""},{"line_number":3025,"context_line":"            compute_api.lock("},{"line_number":3026,"context_line":"                cctxt, instance,"},{"line_number":3027,"context_line":"                reason\u003d("},{"line_number":3028,"context_line":"                    f\u0027Refreshing connection_info for BDM {bdm.uuid} \u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"cbf40b8f_25e4e071","side":"PARENT","line":3025,"updated":"2023-03-14 19:23:32.000000000","message":"The only thing that needs replacing with the context manager is this lock() call...","commit_id":"59ac94158179fc53a87cc3c717dcacff33f2781e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"20a65ba65337991a05d9d0f245f047ca56c63d7c","unresolved":false,"context_lines":[{"line_number":3022,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3023,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"},{"line_number":3024,"context_line":""},{"line_number":3025,"context_line":"            compute_api.lock("},{"line_number":3026,"context_line":"                cctxt, instance,"},{"line_number":3027,"context_line":"                reason\u003d("},{"line_number":3028,"context_line":"                    f\u0027Refreshing connection_info for BDM {bdm.uuid} \u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"80387c96_787827d1","side":"PARENT","line":3025,"in_reply_to":"4f86e902_21727dc1","updated":"2023-06-27 20:25:58.000000000","message":"Done","commit_id":"59ac94158179fc53a87cc3c717dcacff33f2781e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"20a65ba65337991a05d9d0f245f047ca56c63d7c","unresolved":false,"context_lines":[{"line_number":3022,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3023,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"},{"line_number":3024,"context_line":""},{"line_number":3025,"context_line":"            compute_api.lock("},{"line_number":3026,"context_line":"                cctxt, instance,"},{"line_number":3027,"context_line":"                reason\u003d("},{"line_number":3028,"context_line":"                    f\u0027Refreshing connection_info for BDM {bdm.uuid} \u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"ff2351a6_a09454b9","side":"PARENT","line":3025,"in_reply_to":"4f86e902_21727dc1","updated":"2023-06-27 20:25:58.000000000","message":"Done","commit_id":"59ac94158179fc53a87cc3c717dcacff33f2781e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"98fe73b89de8fcf7fa6027b23926e4f3747f85f2","unresolved":true,"context_lines":[{"line_number":3022,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3023,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"},{"line_number":3024,"context_line":""},{"line_number":3025,"context_line":"            compute_api.lock("},{"line_number":3026,"context_line":"                cctxt, instance,"},{"line_number":3027,"context_line":"                reason\u003d("},{"line_number":3028,"context_line":"                    f\u0027Refreshing connection_info for BDM {bdm.uuid} \u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"4f86e902_21727dc1","side":"PARENT","line":3025,"in_reply_to":"cbf40b8f_25e4e071","updated":"2023-03-16 13:39:04.000000000","message":"Do you mean we should send the compute_api object from the caller too?\nAlthough in the case where the caller is not from this module and does not need compute api for any other stuff, shouldn\u0027t we instantiate compute_api here to lock the instance","commit_id":"59ac94158179fc53a87cc3c717dcacff33f2781e"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"462fbec0745f8eab18d66a5b9f722cf826a100da","unresolved":true,"context_lines":[{"line_number":3121,"context_line":"                    context.get_admin_context(),"},{"line_number":3122,"context_line":"                    im.cell_mapping"},{"line_number":3123,"context_line":"                ) as u_cctxt:"},{"line_number":3124,"context_line":"                    compute_api.unlock(u_cctxt, instance)"},{"line_number":3125,"context_line":""},{"line_number":3126,"context_line":"    @action_description("},{"line_number":3127,"context_line":"        _(\"Refresh the connection info for a given volume attachment\"))"}],"source_content_type":"text/x-python","patch_set":6,"id":"55ea0d98_8b2bca1f","side":"PARENT","line":3124,"updated":"2023-03-14 19:23:32.000000000","message":"And this unlock() call. So the new context should wrap around everything between these two calls, and only do the lock()/unlock().","commit_id":"59ac94158179fc53a87cc3c717dcacff33f2781e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"20a65ba65337991a05d9d0f245f047ca56c63d7c","unresolved":false,"context_lines":[{"line_number":3121,"context_line":"                    context.get_admin_context(),"},{"line_number":3122,"context_line":"                    im.cell_mapping"},{"line_number":3123,"context_line":"                ) as u_cctxt:"},{"line_number":3124,"context_line":"                    compute_api.unlock(u_cctxt, instance)"},{"line_number":3125,"context_line":""},{"line_number":3126,"context_line":"    @action_description("},{"line_number":3127,"context_line":"        _(\"Refresh the connection info for a given volume attachment\"))"}],"source_content_type":"text/x-python","patch_set":6,"id":"15dbe34b_459d3d37","side":"PARENT","line":3124,"in_reply_to":"3570b2b8_16f04442","updated":"2023-06-27 20:25:58.000000000","message":"Done","commit_id":"59ac94158179fc53a87cc3c717dcacff33f2781e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"20a65ba65337991a05d9d0f245f047ca56c63d7c","unresolved":false,"context_lines":[{"line_number":3121,"context_line":"                    context.get_admin_context(),"},{"line_number":3122,"context_line":"                    im.cell_mapping"},{"line_number":3123,"context_line":"                ) as u_cctxt:"},{"line_number":3124,"context_line":"                    compute_api.unlock(u_cctxt, instance)"},{"line_number":3125,"context_line":""},{"line_number":3126,"context_line":"    @action_description("},{"line_number":3127,"context_line":"        _(\"Refresh the connection info for a given volume attachment\"))"}],"source_content_type":"text/x-python","patch_set":6,"id":"57ac8c19_fab219a0","side":"PARENT","line":3124,"in_reply_to":"3570b2b8_16f04442","updated":"2023-06-27 20:25:58.000000000","message":"Done","commit_id":"59ac94158179fc53a87cc3c717dcacff33f2781e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"98fe73b89de8fcf7fa6027b23926e4f3747f85f2","unresolved":true,"context_lines":[{"line_number":3121,"context_line":"                    context.get_admin_context(),"},{"line_number":3122,"context_line":"                    im.cell_mapping"},{"line_number":3123,"context_line":"                ) as u_cctxt:"},{"line_number":3124,"context_line":"                    compute_api.unlock(u_cctxt, instance)"},{"line_number":3125,"context_line":""},{"line_number":3126,"context_line":"    @action_description("},{"line_number":3127,"context_line":"        _(\"Refresh the connection info for a given volume attachment\"))"}],"source_content_type":"text/x-python","patch_set":6,"id":"3570b2b8_16f04442","side":"PARENT","line":3124,"in_reply_to":"55ea0d98_8b2bca1f","updated":"2023-03-16 13:39:04.000000000","message":"I have moved the instance verification part and it worked (locally).\nOther then that, as per the note from lyarwood we need to create a fresh context and request-id to unlock instance.\nnow we have \n    compute_api instantiation\n    api.lock\n       yeild\n    api.unlock with new context","commit_id":"59ac94158179fc53a87cc3c717dcacff33f2781e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"cf9a9ead8fff46e0ddc8949dd8f1a12a51ccdac2","unresolved":true,"context_lines":[{"line_number":158,"context_line":"    \"\"\""},{"line_number":159,"context_line":"    compute_api \u003d api.API()"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    if instance.vm_state !\u003d obj_fields.InstanceState.STOPPED:"},{"line_number":162,"context_line":"        raise exception.InstanceInvalidState("},{"line_number":163,"context_line":"            instance_uuid\u003dinstance.uuid, attr\u003d\u0027vm_state\u0027,"},{"line_number":164,"context_line":"            state\u003dinstance.vm_state,"},{"line_number":165,"context_line":"            method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    if instance.locked:"},{"line_number":168,"context_line":"        raise exception.InstanceInvalidState("}],"source_content_type":"text/x-python","patch_set":6,"id":"26630aba_a2c0d58e","line":165,"range":{"start_line":161,"start_character":0,"end_line":165,"end_character":50},"updated":"2023-03-15 12:05:09.000000000","message":"this should not be here.\nits not related to locking an instance its related to the specirfic command we are executing","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"20a65ba65337991a05d9d0f245f047ca56c63d7c","unresolved":false,"context_lines":[{"line_number":158,"context_line":"    \"\"\""},{"line_number":159,"context_line":"    compute_api \u003d api.API()"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    if instance.vm_state !\u003d obj_fields.InstanceState.STOPPED:"},{"line_number":162,"context_line":"        raise exception.InstanceInvalidState("},{"line_number":163,"context_line":"            instance_uuid\u003dinstance.uuid, attr\u003d\u0027vm_state\u0027,"},{"line_number":164,"context_line":"            state\u003dinstance.vm_state,"},{"line_number":165,"context_line":"            method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    if instance.locked:"},{"line_number":168,"context_line":"        raise exception.InstanceInvalidState("}],"source_content_type":"text/x-python","patch_set":6,"id":"e985676f_ed53adfb","line":165,"range":{"start_line":161,"start_character":0,"end_line":165,"end_character":50},"in_reply_to":"0857ca32_1f504e01","updated":"2023-06-27 20:25:58.000000000","message":"Done","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"98fe73b89de8fcf7fa6027b23926e4f3747f85f2","unresolved":true,"context_lines":[{"line_number":158,"context_line":"    \"\"\""},{"line_number":159,"context_line":"    compute_api \u003d api.API()"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    if instance.vm_state !\u003d obj_fields.InstanceState.STOPPED:"},{"line_number":162,"context_line":"        raise exception.InstanceInvalidState("},{"line_number":163,"context_line":"            instance_uuid\u003dinstance.uuid, attr\u003d\u0027vm_state\u0027,"},{"line_number":164,"context_line":"            state\u003dinstance.vm_state,"},{"line_number":165,"context_line":"            method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    if instance.locked:"},{"line_number":168,"context_line":"        raise exception.InstanceInvalidState("}],"source_content_type":"text/x-python","patch_set":6,"id":"0857ca32_1f504e01","line":165,"range":{"start_line":161,"start_character":0,"end_line":165,"end_character":50},"in_reply_to":"26630aba_a2c0d58e","updated":"2023-03-16 13:39:04.000000000","message":"ack, moved to caller\nI thought verification should be responsibility of context manager, so added checks here; just checked with open(), there also open do not verify if file is open already it allows to perform read/write operation","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"cf9a9ead8fff46e0ddc8949dd8f1a12a51ccdac2","unresolved":true,"context_lines":[{"line_number":164,"context_line":"            state\u003dinstance.vm_state,"},{"line_number":165,"context_line":"            method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    if instance.locked:"},{"line_number":168,"context_line":"        raise exception.InstanceInvalidState("},{"line_number":169,"context_line":"            instance_uuid\u003dinstance.uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":170,"context_line":"            method\u003d action + \u0027 (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f269a50_e604bc86","line":167,"updated":"2023-03-15 12:05:09.000000000","message":"this would be a correct precondition for this context manager however its not for what we want to do","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f69b4daab08d9ddfc3642655fe742d195f5a704e","unresolved":true,"context_lines":[{"line_number":164,"context_line":"            state\u003dinstance.vm_state,"},{"line_number":165,"context_line":"            method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    if instance.locked:"},{"line_number":168,"context_line":"        raise exception.InstanceInvalidState("},{"line_number":169,"context_line":"            instance_uuid\u003dinstance.uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":170,"context_line":"            method\u003d action + \u0027 (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"77478119_a6cc911c","line":167,"in_reply_to":"24628b32_c4bc604d","updated":"2023-07-10 17:16:17.000000000","message":"Sean, can you provide more information here? \"Correct but not what we want to do\" doesn\u0027t tell me what you\u0027re thinking. Do you just want to skip the lock attempt if it\u0027s already locked?","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"fdcfa20df2a6796cc37738bc68bb717cd87b9929","unresolved":true,"context_lines":[{"line_number":164,"context_line":"            state\u003dinstance.vm_state,"},{"line_number":165,"context_line":"            method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    if instance.locked:"},{"line_number":168,"context_line":"        raise exception.InstanceInvalidState("},{"line_number":169,"context_line":"            instance_uuid\u003dinstance.uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":170,"context_line":"            method\u003d action + \u0027 (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"a32d7329_6d2092a5","line":167,"in_reply_to":"5f269a50_e604bc86","updated":"2023-07-06 10:08:20.000000000","message":"removed condition to check if instance is locked already and locking irrespective of lock.","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"7f5cf45bb19d54dd6ae75e7f3fc938eac21f7fe3","unresolved":false,"context_lines":[{"line_number":164,"context_line":"            state\u003dinstance.vm_state,"},{"line_number":165,"context_line":"            method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    if instance.locked:"},{"line_number":168,"context_line":"        raise exception.InstanceInvalidState("},{"line_number":169,"context_line":"            instance_uuid\u003dinstance.uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":170,"context_line":"            method\u003d action + \u0027 (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"b9566539_2a078916","line":167,"in_reply_to":"77478119_a6cc911c","updated":"2023-07-19 13:58:55.000000000","message":"Done","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"6df73a9183a85a3f986b8d5a1bab2a6a23c1d634","unresolved":true,"context_lines":[{"line_number":164,"context_line":"            state\u003dinstance.vm_state,"},{"line_number":165,"context_line":"            method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    if instance.locked:"},{"line_number":168,"context_line":"        raise exception.InstanceInvalidState("},{"line_number":169,"context_line":"            instance_uuid\u003dinstance.uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":170,"context_line":"            method\u003d action + \u0027 (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"24628b32_c4bc604d","line":167,"in_reply_to":"a32d7329_6d2092a5","updated":"2023-07-06 10:49:22.000000000","message":"not at this line, please ignore the last reply.","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"cf9a9ead8fff46e0ddc8949dd8f1a12a51ccdac2","unresolved":true,"context_lines":[{"line_number":3176,"context_line":"                locking_reason \u003d (f\u0027Refreshing connection_info for BDM\u0027"},{"line_number":3177,"context_line":"                    f\u0027{bdm.uuid} associated with instance {instance.uuid}\u0027"},{"line_number":3178,"context_line":"                    f\u0027 and volume {volume_id}.\u0027)"},{"line_number":3179,"context_line":"                with lock_instance_context(instance,"},{"line_number":3180,"context_line":"                        cctxt, im.cell_mapping, action, locking_reason):"},{"line_number":3181,"context_line":"                    return self._refresh("},{"line_number":3182,"context_line":"                        cctxt, im, bdm, instance, volume_id, connector)"}],"source_content_type":"text/x-python","patch_set":6,"id":"665170cd_ffd6a6c8","line":3179,"updated":"2023-03-15 12:05:09.000000000","message":"so we need to support using this on instnaces that are locked allready so using a context manager or at least the one you wronte wont work","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f69b4daab08d9ddfc3642655fe742d195f5a704e","unresolved":true,"context_lines":[{"line_number":3176,"context_line":"                locking_reason \u003d (f\u0027Refreshing connection_info for BDM\u0027"},{"line_number":3177,"context_line":"                    f\u0027{bdm.uuid} associated with instance {instance.uuid}\u0027"},{"line_number":3178,"context_line":"                    f\u0027 and volume {volume_id}.\u0027)"},{"line_number":3179,"context_line":"                with lock_instance_context(instance,"},{"line_number":3180,"context_line":"                        cctxt, im.cell_mapping, action, locking_reason):"},{"line_number":3181,"context_line":"                    return self._refresh("},{"line_number":3182,"context_line":"                        cctxt, im, bdm, instance, volume_id, connector)"}],"source_content_type":"text/x-python","patch_set":6,"id":"4c6c57cd_6900b163","line":3179,"in_reply_to":"1cc4ebc1_2b78e22a","updated":"2023-07-10 17:16:17.000000000","message":"I *think* Sean is saying \"make sure it\u0027s locked, either because we locked it or because it was already locked\" ?","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"7f5cf45bb19d54dd6ae75e7f3fc938eac21f7fe3","unresolved":false,"context_lines":[{"line_number":3176,"context_line":"                locking_reason \u003d (f\u0027Refreshing connection_info for BDM\u0027"},{"line_number":3177,"context_line":"                    f\u0027{bdm.uuid} associated with instance {instance.uuid}\u0027"},{"line_number":3178,"context_line":"                    f\u0027 and volume {volume_id}.\u0027)"},{"line_number":3179,"context_line":"                with lock_instance_context(instance,"},{"line_number":3180,"context_line":"                        cctxt, im.cell_mapping, action, locking_reason):"},{"line_number":3181,"context_line":"                    return self._refresh("},{"line_number":3182,"context_line":"                        cctxt, im, bdm, instance, volume_id, connector)"}],"source_content_type":"text/x-python","patch_set":6,"id":"880a84f2_1b1763b6","line":3179,"in_reply_to":"4c6c57cd_6900b163","updated":"2023-07-19 13:58:55.000000000","message":"Done","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"fdcfa20df2a6796cc37738bc68bb717cd87b9929","unresolved":true,"context_lines":[{"line_number":3176,"context_line":"                locking_reason \u003d (f\u0027Refreshing connection_info for BDM\u0027"},{"line_number":3177,"context_line":"                    f\u0027{bdm.uuid} associated with instance {instance.uuid}\u0027"},{"line_number":3178,"context_line":"                    f\u0027 and volume {volume_id}.\u0027)"},{"line_number":3179,"context_line":"                with lock_instance_context(instance,"},{"line_number":3180,"context_line":"                        cctxt, im.cell_mapping, action, locking_reason):"},{"line_number":3181,"context_line":"                    return self._refresh("},{"line_number":3182,"context_line":"                        cctxt, im, bdm, instance, volume_id, connector)"}],"source_content_type":"text/x-python","patch_set":6,"id":"1cc4ebc1_2b78e22a","line":3179,"in_reply_to":"665170cd_ffd6a6c8","updated":"2023-07-06 10:08:20.000000000","message":"It took me time to understand this, thats why didn\u0027t reply on this earlier.\n\nso should we unlock the instance first and then lock it for particular caller ?\nwe are creating another context manager inside locked_instance.\n\ncan you please suggest test scenarios when it should pass and fail.","commit_id":"c1a01101bd5635a9f99ac269429c159046a041f0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"6bdd94f197410d5794ad5151874b964c3e77abb1","unresolved":true,"context_lines":[{"line_number":157,"context_line":"    \"\"\""},{"line_number":158,"context_line":"    compute_api \u003d api.API()"},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"    compute_api.lock(cctxt, instance, reason\u003dreason)"},{"line_number":161,"context_line":"    try:"},{"line_number":162,"context_line":"        yield"},{"line_number":163,"context_line":"    finally:"}],"source_content_type":"text/x-python","patch_set":7,"id":"96fa4e1e_fb102744","line":160,"updated":"2023-03-17 19:15:10.000000000","message":"The lock was being done under the target_cell() context, this was lost when it moved here.","commit_id":"9a5ec249fdc2365d86455e7a4c512af86e1950ef"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"702255755cac1667d1640d8cb3ca7ff4758ef792","unresolved":true,"context_lines":[{"line_number":157,"context_line":"    \"\"\""},{"line_number":158,"context_line":"    compute_api \u003d api.API()"},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"    compute_api.lock(cctxt, instance, reason\u003dreason)"},{"line_number":161,"context_line":"    try:"},{"line_number":162,"context_line":"        yield"},{"line_number":163,"context_line":"    finally:"}],"source_content_type":"text/x-python","patch_set":7,"id":"eaca2488_7884056a","line":160,"in_reply_to":"96fa4e1e_fb102744","updated":"2023-03-20 10:58:56.000000000","message":"Verified id of context variable cctxt, its same before and in lock_instance. \n\n\tstack@devstack-amit:~/nova$ nc localhost 4444\n\t\u003e /opt/stack/nova/nova/cmd/manage.py(3161)refresh()\n\t-\u003e instance \u003d objects.Instance.get_by_uuid(cctxt, instance_uuid)\n\t(Pdb) id(cctxt)\n\t140197822329904\n\t(Pdb) c\n\t^C\n\tstack@devstack-amit:~/nova$ nc localhost 4444\n\t\u003e /opt/stack/nova/nova/cmd/manage.py(159)lock_instance()\n\t-\u003e compute_api \u003d api.API()\n\t(Pdb) id(cctxt)\n\t140197822329904\n\t(Pdb)\n\nit gets updated at 172, when we create new cctxt.\n\t\u003e /opt/stack/nova/nova/cmd/manage.py(173)lock_instance()\n\t-\u003e compute_api.unlock(cctxt, instance)\n\t(Pdb) id(cctxt)\n\t140197817170624","commit_id":"9a5ec249fdc2365d86455e7a4c512af86e1950ef"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"20a65ba65337991a05d9d0f245f047ca56c63d7c","unresolved":false,"context_lines":[{"line_number":157,"context_line":"    \"\"\""},{"line_number":158,"context_line":"    compute_api \u003d api.API()"},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"    compute_api.lock(cctxt, instance, reason\u003dreason)"},{"line_number":161,"context_line":"    try:"},{"line_number":162,"context_line":"        yield"},{"line_number":163,"context_line":"    finally:"}],"source_content_type":"text/x-python","patch_set":7,"id":"96af5c3f_d867ef9b","line":160,"in_reply_to":"eaca2488_7884056a","updated":"2023-06-27 20:25:58.000000000","message":"Done","commit_id":"9a5ec249fdc2365d86455e7a4c512af86e1950ef"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"60c1fe96531ea7504495cafc282576d1e3209b9f","unresolved":true,"context_lines":[{"line_number":170,"context_line":"            with context.target_cell("},{"line_number":171,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":172,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"class DbCommands(object):"},{"line_number":176,"context_line":"    \"\"\"Class for managing the main database.\"\"\""}],"source_content_type":"text/x-python","patch_set":10,"id":"147cd315_9e2e1429","line":173,"updated":"2023-07-12 16:51:26.000000000","message":"I think you can simplify this method a lot by removing the target_cell calls. The request context is already targeted to a cell when you need use the context manager, so you can just use the already targeted context passed in.\n\nAlso, this isn\u0027t doing what the bug work item says, which is to restore the locked/unlocked state the instance was in before the refresh:\n\nhttps://bugzilla.redhat.com/show_bug.cgi?id\u003d2161733#c3\n\nThis is currently always unlocking the instance after the refresh.\n\nYou\u0027ll want to get the current locked state at the beginning of the context manager, if it\u0027s already locked, you don\u0027t need to lock it again and you also don\u0027t need to unlock it after the refresh. If it\u0027s unlocked, you\u0027ll need to lock it and unlock it after the refresh. You can check the instance locked state by instance.locked [1]. Even though the compute API will no-op if the instance is already locked, you can avoid the unnecessary RPC call roundtrip altogether by only calling compute_api.lock() if the instance is unlocked.\n\nFor clarity, this is what I mean about simplifying to use the already targeted context:\n```\n@contextmanager\ndef locked_instance(instance, cctxt, reason):\n    \"\"\"Context manager to lock and unlock instance\n    irrespective of instance current state\n\n    :param instance: vm to be locked and unlocked\n    :param cctxt: the context targeted to a cell\n    :param reason: reason, why lock is required\n    \"\"\"\n    compute_api \u003d api.API()\n\n    etc use compute_api.lock(cctxt, instance, reason\u003dreason) and compute_api.unlock(cctxt, instance) as needed\n```\n\n[1] https://github.com/openstack/nova/blob/f7ce4df51c080ff3b5a6280771f30a8488c7c2df/nova/compute/api.py#L4891","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"64eec130be9bd9d161cfade041d2cfb340338b00","unresolved":true,"context_lines":[{"line_number":170,"context_line":"            with context.target_cell("},{"line_number":171,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":172,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"class DbCommands(object):"},{"line_number":176,"context_line":"    \"\"\"Class for managing the main database.\"\"\""}],"source_content_type":"text/x-python","patch_set":10,"id":"fe7c520d_e268060e","line":173,"in_reply_to":"147cd315_9e2e1429","updated":"2023-07-13 11:32:04.000000000","message":"*I think you can simplify this method a lot by removing the target_cell calls.\nThe request context is already targeted to a cell when you need use the\ncontext manager, so you can just use the already targeted context passed in.*\n\n1 - the reason for having target_cell again was, to have all requirements of lock in same place.\nin case it gets called by caller where request context is not targeted to cell, unlike this call from _refresh.\nbut here locking instance is only required once so removed as per suggestion.\n\n2- fixed lock state w.r.t previous state.\n    ( missed the major point, I think Sean was also suggesting same in other comment.)\n\nthanks Melanie.","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"7f5cf45bb19d54dd6ae75e7f3fc938eac21f7fe3","unresolved":false,"context_lines":[{"line_number":170,"context_line":"            with context.target_cell("},{"line_number":171,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":172,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"class DbCommands(object):"},{"line_number":176,"context_line":"    \"\"\"Class for managing the main database.\"\"\""}],"source_content_type":"text/x-python","patch_set":10,"id":"be27e820_70638e2c","line":173,"in_reply_to":"46ed2faa_b0290804","updated":"2023-07-19 13:58:55.000000000","message":"Done","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a95903e33780f797540880afe6b7bb5ad70b205c","unresolved":true,"context_lines":[{"line_number":170,"context_line":"            with context.target_cell("},{"line_number":171,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":172,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"class DbCommands(object):"},{"line_number":176,"context_line":"    \"\"\"Class for managing the main database.\"\"\""}],"source_content_type":"text/x-python","patch_set":10,"id":"46ed2faa_b0290804","line":173,"in_reply_to":"fe7c520d_e268060e","updated":"2023-07-17 18:47:52.000000000","message":"I understand your reasoning but I guess I just think in general, \"make it more complicated when it needs to be more complicated\". You\u0027re adding locked_instance for refresh specifically right now and there\u0027s a fair chance no other command will ever need it. So the additional complexity would be redundant in that case.\n\nPersonally, I think if a future command needs locked_instance to do automatic cell targeting, the functionality can be added at that time. Just MHO.\n\nAside: I need to correct my earlier comment on this thread about RPC -- I realize the compute API lock() call does *not* go over RPC at all, so that part of what I said should be ignored.","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"60c1fe96531ea7504495cafc282576d1e3209b9f","unresolved":true,"context_lines":[{"line_number":3045,"context_line":""},{"line_number":3046,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":3047,"context_line":"        im \u003d objects.InstanceMapping.get_by_instance_uuid(ctxt, instance_uuid)"},{"line_number":3048,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3049,"context_line":""},{"line_number":3050,"context_line":"            instance \u003d objects.Instance.get_by_uuid(cctxt, instance_uuid)"},{"line_number":3051,"context_line":"            bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("}],"source_content_type":"text/x-python","patch_set":10,"id":"0bfa870e_1598e07d","line":3048,"range":{"start_line":3048,"start_character":59,"end_line":3048,"end_character":64},"updated":"2023-07-12 16:51:26.000000000","message":"This context is already targeted to a cell, so you don\u0027t need to target_cell again inside the context manager.","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"64eec130be9bd9d161cfade041d2cfb340338b00","unresolved":false,"context_lines":[{"line_number":3045,"context_line":""},{"line_number":3046,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":3047,"context_line":"        im \u003d objects.InstanceMapping.get_by_instance_uuid(ctxt, instance_uuid)"},{"line_number":3048,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3049,"context_line":""},{"line_number":3050,"context_line":"            instance \u003d objects.Instance.get_by_uuid(cctxt, instance_uuid)"},{"line_number":3051,"context_line":"            bdm \u003d objects.BlockDeviceMapping.get_by_volume_and_instance("}],"source_content_type":"text/x-python","patch_set":10,"id":"f95395e1_dd2d6212","line":3048,"range":{"start_line":3048,"start_character":59,"end_line":3048,"end_character":64},"in_reply_to":"0bfa870e_1598e07d","updated":"2023-07-13 11:32:04.000000000","message":"Done","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"64eec130be9bd9d161cfade041d2cfb340338b00","unresolved":true,"context_lines":[{"line_number":3057,"context_line":"                    state\u003dinstance.vm_state,"},{"line_number":3058,"context_line":"                    method\u003d\u0027refresh connection_info (must be stopped)\u0027)"},{"line_number":3059,"context_line":""},{"line_number":3060,"context_line":"            if instance.locked:"},{"line_number":3061,"context_line":"                raise exception.InstanceInvalidState("},{"line_number":3062,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3063,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"435573ae_d320a87e","line":3060,"updated":"2023-07-13 11:32:04.000000000","message":"because we are now restoring the instance lock/unlock state.\nright now control will only go there if instance is not locked.\n\nplease suggest, should we remove this check as well ?","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"41e490880be59a5858074281e103fe2a6220373f","unresolved":true,"context_lines":[{"line_number":3057,"context_line":"                    state\u003dinstance.vm_state,"},{"line_number":3058,"context_line":"                    method\u003d\u0027refresh connection_info (must be stopped)\u0027)"},{"line_number":3059,"context_line":""},{"line_number":3060,"context_line":"            if instance.locked:"},{"line_number":3061,"context_line":"                raise exception.InstanceInvalidState("},{"line_number":3062,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3063,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"47b145b1_26029fc5","line":3060,"in_reply_to":"02d22093_58900376","updated":"2023-07-18 10:16:43.000000000","message":"ack","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a95903e33780f797540880afe6b7bb5ad70b205c","unresolved":true,"context_lines":[{"line_number":3057,"context_line":"                    state\u003dinstance.vm_state,"},{"line_number":3058,"context_line":"                    method\u003d\u0027refresh connection_info (must be stopped)\u0027)"},{"line_number":3059,"context_line":""},{"line_number":3060,"context_line":"            if instance.locked:"},{"line_number":3061,"context_line":"                raise exception.InstanceInvalidState("},{"line_number":3062,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3063,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"02d22093_58900376","line":3060,"in_reply_to":"435573ae_d320a87e","updated":"2023-07-17 18:47:52.000000000","message":"Ah yeah, you\u0027re right, this should be removed now that we\u0027re allowing the instance to be already locked when refresh is called.\n\nThis also means we must be missing some test coverage ... we need a test scenario where refresh is called for an already locked instance. The refresh should succeed and the instance should still be in a locked state afterward.","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"7f5cf45bb19d54dd6ae75e7f3fc938eac21f7fe3","unresolved":false,"context_lines":[{"line_number":3057,"context_line":"                    state\u003dinstance.vm_state,"},{"line_number":3058,"context_line":"                    method\u003d\u0027refresh connection_info (must be stopped)\u0027)"},{"line_number":3059,"context_line":""},{"line_number":3060,"context_line":"            if instance.locked:"},{"line_number":3061,"context_line":"                raise exception.InstanceInvalidState("},{"line_number":3062,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3063,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff73b083_cecd960a","line":3060,"in_reply_to":"47b145b1_26029fc5","updated":"2023-07-19 13:58:55.000000000","message":"Done","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"60c1fe96531ea7504495cafc282576d1e3209b9f","unresolved":true,"context_lines":[{"line_number":3082,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3083,"context_line":"            instance_action \u003d None"},{"line_number":3084,"context_line":"            new_attachment_id \u003d None"},{"line_number":3085,"context_line":"            instance \u003d objects.Instance.get_by_uuid(cctxt, instance_uuid)"},{"line_number":3086,"context_line":"            try:"},{"line_number":3087,"context_line":"                # Log this as an instance action so operators and users are"},{"line_number":3088,"context_line":"                # aware that this has happened."}],"source_content_type":"text/x-python","patch_set":10,"id":"c7bd595e_11f6cb62","line":3085,"updated":"2023-07-12 16:51:26.000000000","message":"I think it would be good to avoid an unnecessary database query here and pass instance to _do_refresh instead of instance_uuid. To get the UUID you can use instance.uuid.","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"64eec130be9bd9d161cfade041d2cfb340338b00","unresolved":false,"context_lines":[{"line_number":3082,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3083,"context_line":"            instance_action \u003d None"},{"line_number":3084,"context_line":"            new_attachment_id \u003d None"},{"line_number":3085,"context_line":"            instance \u003d objects.Instance.get_by_uuid(cctxt, instance_uuid)"},{"line_number":3086,"context_line":"            try:"},{"line_number":3087,"context_line":"                # Log this as an instance action so operators and users are"},{"line_number":3088,"context_line":"                # aware that this has happened."}],"source_content_type":"text/x-python","patch_set":10,"id":"b06e0bd6_28534b38","line":3085,"in_reply_to":"c7bd595e_11f6cb62","updated":"2023-07-13 11:32:04.000000000","message":"Ack, Done","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"64eec130be9bd9d161cfade041d2cfb340338b00","unresolved":true,"context_lines":[{"line_number":3085,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3086,"context_line":"            instance_action \u003d None"},{"line_number":3087,"context_line":"            new_attachment_id \u003d None"},{"line_number":3088,"context_line":"            # instance \u003d objects.Instance.get_by_uuid(cctxt, instance_uuid)"},{"line_number":3089,"context_line":"            try:"},{"line_number":3090,"context_line":"                # Log this as an instance action so operators and users are"},{"line_number":3091,"context_line":"                # aware that this has happened."}],"source_content_type":"text/x-python","patch_set":11,"id":"04dcc603_0f3fdd1d","line":3088,"updated":"2023-07-13 11:32:04.000000000","message":"missed! will remove this in next update.","commit_id":"c9127edb323c9e9b2c9ca018e746254dbf0a8bc7"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"163dc126fd98c8276bc1f9862ba9b0bfa79abf21","unresolved":false,"context_lines":[{"line_number":3085,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3086,"context_line":"            instance_action \u003d None"},{"line_number":3087,"context_line":"            new_attachment_id \u003d None"},{"line_number":3088,"context_line":"            # instance \u003d objects.Instance.get_by_uuid(cctxt, instance_uuid)"},{"line_number":3089,"context_line":"            try:"},{"line_number":3090,"context_line":"                # Log this as an instance action so operators and users are"},{"line_number":3091,"context_line":"                # aware that this has happened."}],"source_content_type":"text/x-python","patch_set":11,"id":"1394ce26_757761ef","line":3088,"in_reply_to":"04dcc603_0f3fdd1d","updated":"2023-07-17 17:38:49.000000000","message":"Done","commit_id":"c9127edb323c9e9b2c9ca018e746254dbf0a8bc7"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"405c0a8f17de585c0c3b2a8f411fb57f2617eee1","unresolved":true,"context_lines":[{"line_number":162,"context_line":"        instance_was_not_locked \u003d True"},{"line_number":163,"context_line":"        # NOTE(auniyal): Lock the instance with same request context"},{"line_number":164,"context_line":"        # as the context is already targeted to a cell."},{"line_number":165,"context_line":"        compute_api.lock(cctxt, instance, reason\u003dreason)"},{"line_number":166,"context_line":"    try:"},{"line_number":167,"context_line":"        yield"},{"line_number":168,"context_line":"    finally:"}],"source_content_type":"text/x-python","patch_set":12,"id":"a11ce441_68c471a4","line":165,"updated":"2023-07-14 14:28:53.000000000","message":"Why are you not targeting here but are for the unlock? I think that\u0027s because as melwitt said, the context is already targeted. That means this works in the place you\u0027re using it but may not in other cases, which was your argument for keeping L174 IIRC.\n\nMaybe just put an assertion here that context.cell_uuid is not None, and document that the context must be targeted before calling this function?","commit_id":"8e132e87259820d5835f6ba5d5863008846f461f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"41e490880be59a5858074281e103fe2a6220373f","unresolved":false,"context_lines":[{"line_number":162,"context_line":"        instance_was_not_locked \u003d True"},{"line_number":163,"context_line":"        # NOTE(auniyal): Lock the instance with same request context"},{"line_number":164,"context_line":"        # as the context is already targeted to a cell."},{"line_number":165,"context_line":"        compute_api.lock(cctxt, instance, reason\u003dreason)"},{"line_number":166,"context_line":"    try:"},{"line_number":167,"context_line":"        yield"},{"line_number":168,"context_line":"    finally:"}],"source_content_type":"text/x-python","patch_set":12,"id":"e19189c3_ac92ce94","line":165,"in_reply_to":"a11ce441_68c471a4","updated":"2023-07-18 10:16:43.000000000","message":"done, added assert to check if context is set.","commit_id":"8e132e87259820d5835f6ba5d5863008846f461f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a95903e33780f797540880afe6b7bb5ad70b205c","unresolved":true,"context_lines":[{"line_number":146,"context_line":""},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"@contextmanager"},{"line_number":149,"context_line":"def locked_instance(instance, cctxt, cell_mapping, reason):"},{"line_number":150,"context_line":"    \"\"\"Context manager to lock and unlock instance,"},{"line_number":151,"context_line":"    lock state will be restored regardless of the success or failure"},{"line_number":152,"context_line":"    of target functionality."}],"source_content_type":"text/x-python","patch_set":13,"id":"55d752bb_d46624e6","line":149,"range":{"start_line":149,"start_character":37,"end_line":149,"end_character":49},"updated":"2023-07-17 18:47:52.000000000","message":"You don\u0027t need this anymore, so you can remove it.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"53d8928479d8cc3f09aeb1051239e595ddf6d0c4","unresolved":false,"context_lines":[{"line_number":146,"context_line":""},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"@contextmanager"},{"line_number":149,"context_line":"def locked_instance(instance, cctxt, cell_mapping, reason):"},{"line_number":150,"context_line":"    \"\"\"Context manager to lock and unlock instance,"},{"line_number":151,"context_line":"    lock state will be restored regardless of the success or failure"},{"line_number":152,"context_line":"    of target functionality."}],"source_content_type":"text/x-python","patch_set":13,"id":"3528f4a6_4a5ea099","line":149,"range":{"start_line":149,"start_character":37,"end_line":149,"end_character":49},"in_reply_to":"55d752bb_d46624e6","updated":"2023-07-21 11:07:14.000000000","message":"Done","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a95903e33780f797540880afe6b7bb5ad70b205c","unresolved":true,"context_lines":[{"line_number":153,"context_line":""},{"line_number":154,"context_line":"    :param instance: instance to be lock and unlock"},{"line_number":155,"context_line":"    :param cctxt: the admin context"},{"line_number":156,"context_line":"    :param cell_mapping: instance-cell-mapping"},{"line_number":157,"context_line":"    :param reason: reason, why lock is required"},{"line_number":158,"context_line":"    \"\"\""},{"line_number":159,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"66615f43_cae7fd57","line":156,"range":{"start_line":156,"start_character":11,"end_line":156,"end_character":23},"updated":"2023-07-17 18:47:52.000000000","message":"Same.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"53d8928479d8cc3f09aeb1051239e595ddf6d0c4","unresolved":false,"context_lines":[{"line_number":153,"context_line":""},{"line_number":154,"context_line":"    :param instance: instance to be lock and unlock"},{"line_number":155,"context_line":"    :param cctxt: the admin context"},{"line_number":156,"context_line":"    :param cell_mapping: instance-cell-mapping"},{"line_number":157,"context_line":"    :param reason: reason, why lock is required"},{"line_number":158,"context_line":"    \"\"\""},{"line_number":159,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"9a1cde89_2199da94","line":156,"range":{"start_line":156,"start_character":11,"end_line":156,"end_character":23},"in_reply_to":"66615f43_cae7fd57","updated":"2023-07-21 11:07:14.000000000","message":"Done","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a95903e33780f797540880afe6b7bb5ad70b205c","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"e8d05cbb_aa954e91","line":177,"updated":"2023-07-17 18:47:52.000000000","message":"You don\u0027t need to target_cell here anymore right? Since we\u0027re requiring (L160) the context already be targeted when this function is called.\n\nI guess alternatively, since we can detect whether the context is targeted by RequestContext.cell_uuid, we could only target if it\u0027s not targeted. But IMHO this is simpler.\n\nAlso, note that the variable name \u0027cctxt\u0027 stands for \"cell context\" i.e. targeted context. If we were to make locked_instance do automatic targeting, we should use the variable name \u0027ctxt\u0027 for a plain context instead, so as not to be confusing.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"83cc7a1bab2ca169642b424afe7e02949cea3d18","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"d8c3e20f_59c2fd34","line":177,"in_reply_to":"0a61e90f_307e9dcd","updated":"2023-07-17 21:57:04.000000000","message":"I agree with you but wasn\u0027t sure if it was just me. I would have thought all three actions: lock, refresh, unlock use the same request-id to group them together.\n\nIf we go that route, we\u0027ll need to remove the two get_admin_context() calls in here and instead pass in the targeted cell context from the _refresh() method.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f0cdcacbc401b851b59c5e0c0a454dcef5c6317c","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"c76c4002_48b29705","line":177,"in_reply_to":"1f673aa0_e8069eb4","updated":"2023-07-17 22:07:33.000000000","message":"I think Amit was just preserving the current behavior that was done intentionally when the commands were originally added. I went and looked in the spec and in the review for that change and couldn\u0027t find any explanation for wanting 3 separate request-id\u0027s. That said, maybe Amit knows why and I just missed it 🙂 \n\nIf we find we have consensus that we don\u0027t know the reason and don\u0027t have a good reason, I think we can go ahead with changing this to use one consistent request-id throughout the composite command.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"27c3d4f9f6004a21495dcd1b682d70664ff4c1b9","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"0a61e90f_307e9dcd","line":177,"in_reply_to":"3e40cb56_92775304","updated":"2023-07-17 21:14:48.000000000","message":"Maybe I missed it, but why do we need three separate request-ids? Doesn\u0027t that make it harder to determine that they were all related to this cleanup action? In general I think we try to keep the same request ID for everything that happens as a result of one request from a client, and this whole composite operation would fit that model, IMHO.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"cbabfefb0e9dea164461254170b572d061ced88d","unresolved":false,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"378913b3_241cd6ab","line":177,"in_reply_to":"66b09e58_820a81b3","updated":"2023-08-08 12:01:32.000000000","message":"Done","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"7f5cf45bb19d54dd6ae75e7f3fc938eac21f7fe3","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"fcffa46d_d10d2044","line":177,"in_reply_to":"95458bfd_467be197","updated":"2023-07-19 13:58:55.000000000","message":"the reason of 3 unique req-id is to have 3 unique event,  \nit will be helpful to debug errors.\n\n     $ openstack server event list vm_1 -c \"Request ID\" -c \"Action\"\n\n     +------------------------------------------+---------------------------+\n     | Request ID                               | Action                    |\n     +------------------------------------------+---------------------------+\n     | req-0e8bebcc-c22e-41ea-97fe-c9ceac64e282 | unlock                    |\n     | req-ee1808d6-e78a-4b4e-941d-7501d92320ef | refresh_volume_attachment |\n     | req-40d42553-8c79-494e-9ff6-9828f7b61224 | lock                      |\n     | req-81efd0b3-f68d-4c06-82f3-65e5bad11053 | stop                      |\n     | req-5cdd9897-a430-431b-8fc9-6507d1084f09 | attach_volume             |\n     | req-6b728e25-81ac-4f80-9893-0008f895931b | create                    |\n     +------------------------------------------+---------------------------+\n     \nI think we should continue with having same.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"41e490880be59a5858074281e103fe2a6220373f","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"95458bfd_467be197","line":177,"in_reply_to":"c76c4002_48b29705","updated":"2023-07-18 10:16:43.000000000","message":"primarily,I wanted to add context manager with minimal changes.\n\nsecond, I too had this doubt, why to have more then one req-id\u0027s for single operation, because mostly we track by req-id in logs.\nBut note from @lyarwood acknowledge it and says its required.\n\n     # NOTE(lyarwood): As above we need to unlock the instance with\n     # a fresh context and request-id to keep it unique. It\u0027s safe\n     # to assume that the instance is locked as this point as the\n     # earlier call to lock isn\u0027t part of this block.\n\nso I went with it.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4a477eb629c8c46035d3ed198e0f526d4f4843fa","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1f673aa0_e8069eb4","line":177,"in_reply_to":"d8c3e20f_59c2fd34","updated":"2023-07-17 22:00:06.000000000","message":"Yeah I mentioned earlier that we should be using the same context we were passed in, but I thought it was just an oversight and not an _intentional_ re-creation of the context. Maybe there\u0027s some good reason and Amit can enlighten us, but unless its really good, I think one context/req-id for this whole operation is most appropriate.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"808b5845ea965e0f7c4bb4bc1c047859b90a2800","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"3e40cb56_92775304","line":177,"in_reply_to":"e8d05cbb_aa954e91","updated":"2023-07-17 20:45:49.000000000","message":"OK, so Amit pointed out on IRC the above code comment about getting a fresh request-id that I did not make the connection with the target_cell and looking at the original commit that added the volume attachment commands:\n\nhttps://github.com/openstack/nova/commit/e906a8c0ec87b870b0ae75c20cf1d2da36433636\n\nit looks like the intention was to use 3 separate request-ids by using 3 different RequestContext\u0027s. One for lock, one for refresh, and one for unlock. I\u0027m guessing the rationale there is to mimic calls over the REST API if one were to lock and unlock, those actions would have different request-id\u0027s. I had been thinking we would want the opposite, to have all API calls related to the refresh under the same request-id for tracing and grouping related calls together, but maybe that\u0027s not a good thing to do?\n\nSo that pretty much brings us back around to the code from PS10 😩 I didn\u0027t know that\u0027s why you had repeated separate target_cell calls. The only other way I can think to do it would be to set the request-id before each call i.e.\n```\nfrom oslo_context import context as base_context\ncctxt.request_id \u003d base_context.generate_request_id()\n```\n\nbut that might be uglier than repeating target_cell.\n\nSo ... I think we probably need to go back to using target_cell twice in this function and removing the assert. The thing I would do differently is put the get_admin_context() call inline with target_cell, and then put a code comment above target_cell saying something like \"The context might already be targeted to a cell but we need a fresh RequestContext for a unique request-id, so we just use target_cell again to pass a new context.\"\n\nApologies for missing that ☹️","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"f6baf1bf10da1a99bfc0292f9b5d3b52bb02570e","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"66b09e58_820a81b3","line":177,"in_reply_to":"fb18e1ce_885a4619","updated":"2023-07-21 15:32:21.000000000","message":"if vm is locked already:\n\n    +------------------------------------------+---------------------------+\n    | Request ID                               | Action                    |\n    +------------------------------------------+---------------------------+\n    | req-af4bb211-8547-4fca-b516-4c66f0e95f8f | refresh_volume_attachment |\n    | req-8f937b45-e60a-4007-9d2c-b600b19e1ece | refresh_volume_attachment |\n    | req-9e40eb26-6e4b-4814-be68-f9d266a73bc8 | lock                      |","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"53d8928479d8cc3f09aeb1051239e595ddf6d0c4","unresolved":true,"context_lines":[{"line_number":174,"context_line":"            # to assume that the instance is locked at this point as the"},{"line_number":175,"context_line":"            # earlier call to lock isn\u0027t part of this block."},{"line_number":176,"context_line":"            with context.target_cell("},{"line_number":177,"context_line":"                context.get_admin_context(), cell_mapping) as cctxt:"},{"line_number":178,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"fb18e1ce_885a4619","line":177,"in_reply_to":"fcffa46d_d10d2044","updated":"2023-07-21 11:07:14.000000000","message":"updated to have single req-id for all 3 actions\n\n    +------------------------------------------+---------------------------+\n    | Request ID                               | Action                    |\n    +------------------------------------------+---------------------------+\n    | req-c118b7b4-2bce-46e2-a83c-79bb5f081a68 | unlock                    |\n    | req-c118b7b4-2bce-46e2-a83c-79bb5f081a68 | refresh_volume_attachment |\n    | req-c118b7b4-2bce-46e2-a83c-79bb5f081a68 | lock                      |\n    | req-f1fd72b1-a797-4a07-88b9-4742468f2d3e | stop                      |","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a95903e33780f797540880afe6b7bb5ad70b205c","unresolved":true,"context_lines":[{"line_number":3224,"context_line":"        except exception.InvalidInput as e:"},{"line_number":3225,"context_line":"            print(str(e))"},{"line_number":3226,"context_line":"            return 2"},{"line_number":3227,"context_line":"        except (Exception, AssertionError) as e:"},{"line_number":3228,"context_line":"            print(\u0027Unexpected error, see nova-manage.log for the full \u0027"},{"line_number":3229,"context_line":"                  \u0027trace: %s \u0027 % str(e))"},{"line_number":3230,"context_line":"            LOG.exception(\u0027Unexpected error\u0027)"}],"source_content_type":"text/x-python","patch_set":13,"id":"5b48a736_68a8f22f","line":3227,"range":{"start_line":3227,"start_character":27,"end_line":3227,"end_character":41},"updated":"2023-07-17 18:47:52.000000000","message":"I don\u0027t think you need to add this because AssertionError is a subclass of Exception. That is why you will pretty much never see other exception classes when there is a \"except Exception:\". It\u0027s used as a catchall when there is no other previous handling for a given exception.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"41e490880be59a5858074281e103fe2a6220373f","unresolved":false,"context_lines":[{"line_number":3224,"context_line":"        except exception.InvalidInput as e:"},{"line_number":3225,"context_line":"            print(str(e))"},{"line_number":3226,"context_line":"            return 2"},{"line_number":3227,"context_line":"        except (Exception, AssertionError) as e:"},{"line_number":3228,"context_line":"            print(\u0027Unexpected error, see nova-manage.log for the full \u0027"},{"line_number":3229,"context_line":"                  \u0027trace: %s \u0027 % str(e))"},{"line_number":3230,"context_line":"            LOG.exception(\u0027Unexpected error\u0027)"}],"source_content_type":"text/x-python","patch_set":13,"id":"5bde4cc8_e4dcafda","line":3227,"range":{"start_line":3227,"start_character":27,"end_line":3227,"end_character":41},"in_reply_to":"5b48a736_68a8f22f","updated":"2023-07-18 10:16:43.000000000","message":"Done","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"009cd064f93a94e4a15ca409585056a67854aeda","unresolved":true,"context_lines":[{"line_number":3046,"context_line":""},{"line_number":3047,"context_line":"        # NOTE(lyarwood): Yes this is weird but we need to recreate the admin"},{"line_number":3048,"context_line":"        # context here to ensure the lock above uses a unique request-id"},{"line_number":3049,"context_line":"        # versus the following refresh and eventual unlock."},{"line_number":3050,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":3051,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3052,"context_line":"            instance_action \u003d None"}],"source_content_type":"text/x-python","patch_set":17,"id":"cec6e433_14e490c2","side":"PARENT","line":3049,"updated":"2023-07-21 15:57:25.000000000","message":"Okay, I guess I\u0027ve lost track of where we started and where we\u0027re going here and that the existing code already uses three contexts purely to get unique request-ids. I understand why that was now being done but I think it\u0027s probably because we\u0027re hijacking operations that are meant to be used elsewhere for this process, likely incorrectly.\n\nAn action is supposed to be one operation on an instance, so `refresh_volume_connection` should be one action. If there are multiple steps to achieve that, those should be represented as action_events. The requirement for the actions to be one-per-request-id is specifically to delineate steps to achieve one goal from multiple goals.\n\nSo I think really the mistake here was jumping through these hoops to try to re-use compute_api.lock() as it is used from the actual API, where it will create its own action with the context it has been provided. That makes it look like someone did lock it through the API, and unlock it afterwards.\n\nSo really, if we did the meat of the lock ourselves (or factor out that helper) and then did the refresh, it would look like this and that\u0027s what I thought we were trying to get to:\n```\naction: refresh_volume_connection\n  event: locked temporarily because it was not locked already\n  event: updated connector\n  event: unlocked because it was originally unlocked\n```\n\nAll as one operation/action as viewed from the actions list on the instance, with multiple steps to achieve the goal of refreshing the volume connection. That would be the correct representation in my view, even though I know it requires a little more refactoring to get there. If the group decides that keeping the three contexts/request-ids is just easier, then so be it. But none of our other \"single operation on an instance\" things have to hack around it like this.","commit_id":"b6f4c57b433231ba922abaededeb5fef57a293d4"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"c9e54ea7da68bd147bb7e434a3952ccdc7d16733","unresolved":false,"context_lines":[{"line_number":3046,"context_line":""},{"line_number":3047,"context_line":"        # NOTE(lyarwood): Yes this is weird but we need to recreate the admin"},{"line_number":3048,"context_line":"        # context here to ensure the lock above uses a unique request-id"},{"line_number":3049,"context_line":"        # versus the following refresh and eventual unlock."},{"line_number":3050,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":3051,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3052,"context_line":"            instance_action \u003d None"}],"source_content_type":"text/x-python","patch_set":17,"id":"c9276737_402884c2","side":"PARENT","line":3049,"in_reply_to":"42f2234e_bf723b63","updated":"2024-02-14 10:28:37.000000000","message":"Done","commit_id":"b6f4c57b433231ba922abaededeb5fef57a293d4"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"cbabfefb0e9dea164461254170b572d061ced88d","unresolved":true,"context_lines":[{"line_number":3046,"context_line":""},{"line_number":3047,"context_line":"        # NOTE(lyarwood): Yes this is weird but we need to recreate the admin"},{"line_number":3048,"context_line":"        # context here to ensure the lock above uses a unique request-id"},{"line_number":3049,"context_line":"        # versus the following refresh and eventual unlock."},{"line_number":3050,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":3051,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3052,"context_line":"            instance_action \u003d None"}],"source_content_type":"text/x-python","patch_set":17,"id":"42f2234e_bf723b63","side":"PARENT","line":3049,"in_reply_to":"5e7b5af9_5e080e93","updated":"2023-08-08 12:01:32.000000000","message":"because lock and unlock itself creates action, IMHO there is no need to create event for lock and unlock and as they are actions, and we should have a new context for both lock and unlock as earlier code, reverted as earlier.","commit_id":"b6f4c57b433231ba922abaededeb5fef57a293d4"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"b19cebcb80d1eb009d447c7942f52fa088f48848","unresolved":true,"context_lines":[{"line_number":3046,"context_line":""},{"line_number":3047,"context_line":"        # NOTE(lyarwood): Yes this is weird but we need to recreate the admin"},{"line_number":3048,"context_line":"        # context here to ensure the lock above uses a unique request-id"},{"line_number":3049,"context_line":"        # versus the following refresh and eventual unlock."},{"line_number":3050,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":3051,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3052,"context_line":"            instance_action \u003d None"}],"source_content_type":"text/x-python","patch_set":17,"id":"5e7b5af9_5e080e93","side":"PARENT","line":3049,"in_reply_to":"a29b2549_3eaabfa7","updated":"2023-07-25 14:19:27.000000000","message":"fixed unit tests, functional tests are still failing, event.finish is not working for funcitonal tests.","commit_id":"b6f4c57b433231ba922abaededeb5fef57a293d4"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"c8ccdd669d4b93387b73976ecad1db9d1bdc152f","unresolved":true,"context_lines":[{"line_number":3046,"context_line":""},{"line_number":3047,"context_line":"        # NOTE(lyarwood): Yes this is weird but we need to recreate the admin"},{"line_number":3048,"context_line":"        # context here to ensure the lock above uses a unique request-id"},{"line_number":3049,"context_line":"        # versus the following refresh and eventual unlock."},{"line_number":3050,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":3051,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3052,"context_line":"            instance_action \u003d None"}],"source_content_type":"text/x-python","patch_set":17,"id":"a29b2549_3eaabfa7","side":"PARENT","line":3049,"in_reply_to":"cec6e433_14e490c2","updated":"2023-07-24 14:42:26.000000000","message":"updated code to have events for lock and unlock.\n\nis this OK ?\n- now in event-list has only refresh and lock and unlock\n- locking and unlocking are not recorded in instance_actions table or instace_action_events table.\n- is there a way we can see events too.\n\n\ncrrent issues:\n- unit test are failing because getting this exception in existing tests:\nException: This test uses methods that set internal                                                                    \n     oslo_db state, but it does not claim to use the database. This will conflict with the setup of tests that do use the database and cause failures later.\n\nits failing at locked_instance context-manager, we do not want to mock it so we can test if instance is locked already test case.","commit_id":"b6f4c57b433231ba922abaededeb5fef57a293d4"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"f6baf1bf10da1a99bfc0292f9b5d3b52bb02570e","unresolved":true,"context_lines":[{"line_number":3048,"context_line":"        # context here to ensure the lock above uses a unique request-id"},{"line_number":3049,"context_line":"        # versus the following refresh and eventual unlock."},{"line_number":3050,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":3051,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3052,"context_line":"            instance_action \u003d None"},{"line_number":3053,"context_line":"            new_attachment_id \u003d None"},{"line_number":3054,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":17,"id":"8276d28e_23155ce2","side":"PARENT","line":3051,"range":{"start_line":3051,"start_character":13,"end_line":3051,"end_character":20},"updated":"2023-07-21 15:32:21.000000000","message":"this context manager is removed to use same context which is coming from caller, to  use same req-id which is being used for lock and future unlock.","commit_id":"b6f4c57b433231ba922abaededeb5fef57a293d4"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"b19cebcb80d1eb009d447c7942f52fa088f48848","unresolved":false,"context_lines":[{"line_number":3048,"context_line":"        # context here to ensure the lock above uses a unique request-id"},{"line_number":3049,"context_line":"        # versus the following refresh and eventual unlock."},{"line_number":3050,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":3051,"context_line":"        with context.target_cell(ctxt, im.cell_mapping) as cctxt:"},{"line_number":3052,"context_line":"            instance_action \u003d None"},{"line_number":3053,"context_line":"            new_attachment_id \u003d None"},{"line_number":3054,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":17,"id":"b86e9846_9d737057","side":"PARENT","line":3051,"range":{"start_line":3051,"start_character":13,"end_line":3051,"end_character":20},"in_reply_to":"8276d28e_23155ce2","updated":"2023-07-25 14:19:27.000000000","message":"Done","commit_id":"b6f4c57b433231ba922abaededeb5fef57a293d4"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"f6baf1bf10da1a99bfc0292f9b5d3b52bb02570e","unresolved":true,"context_lines":[{"line_number":3079,"context_line":"            instance_action \u003d objects.InstanceAction.action_start("},{"line_number":3080,"context_line":"                cctxt, instance.uuid,"},{"line_number":3081,"context_line":"                instance_actions.NOVA_MANAGE_REFRESH_VOLUME_ATTACHMENT)"},{"line_number":3082,"context_line":""},{"line_number":3083,"context_line":"            # Create a blank attachment to keep the volume reserved"},{"line_number":3084,"context_line":"            new_attachment_id \u003d volume_api.attachment_create("},{"line_number":3085,"context_line":"                cctxt, volume_id, instance.uuid)[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":17,"id":"acf97a5b_6b6e9ef3","line":3082,"updated":"2023-07-21 15:32:21.000000000","message":"here action: refresh_volume_attachment, get listed in server event list\nwhich means action_start worked, also it did throw any error (tested with pdb as well).","commit_id":"50bebe5e08a930b9e3547b26eadfcf0acdf6c509"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"b19cebcb80d1eb009d447c7942f52fa088f48848","unresolved":false,"context_lines":[{"line_number":3079,"context_line":"            instance_action \u003d objects.InstanceAction.action_start("},{"line_number":3080,"context_line":"                cctxt, instance.uuid,"},{"line_number":3081,"context_line":"                instance_actions.NOVA_MANAGE_REFRESH_VOLUME_ATTACHMENT)"},{"line_number":3082,"context_line":""},{"line_number":3083,"context_line":"            # Create a blank attachment to keep the volume reserved"},{"line_number":3084,"context_line":"            new_attachment_id \u003d volume_api.attachment_create("},{"line_number":3085,"context_line":"                cctxt, volume_id, instance.uuid)[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":17,"id":"5568c240_99e45d0c","line":3082,"in_reply_to":"acf97a5b_6b6e9ef3","updated":"2023-07-25 14:19:27.000000000","message":"Done","commit_id":"50bebe5e08a930b9e3547b26eadfcf0acdf6c509"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"53d8928479d8cc3f09aeb1051239e595ddf6d0c4","unresolved":true,"context_lines":[{"line_number":3150,"context_line":"            if instance_action:"},{"line_number":3151,"context_line":"                try:"},{"line_number":3152,"context_line":"                    instance_action.finish()"},{"line_number":3153,"context_line":"                except Exception as e:"},{"line_number":3154,"context_line":"                    print(e)"},{"line_number":3155,"context_line":""},{"line_number":3156,"context_line":"    @action_description("}],"source_content_type":"text/x-python","patch_set":17,"id":"af765efc_a7161f39","line":3153,"updated":"2023-07-21 11:07:14.000000000","message":"on removing contex manager, instance_action_finish is failing while creating query. so it did not really ran query on DB.\nat https://github.com/openstack/nova/blob/master/nova/db/main/api.py#L3912\nerror:\nAction for request_id req-c118b7b4-2bce-46e2-a83c-79bb5f081a68 on instance 307375a3-f0c8-4ed9-855d-dc754cc862a7 not found\n\n- it did created query; but query.update(values) return 2 instead of 1\n- value of object context and values were same with and without change (i.e removing context manager).","commit_id":"50bebe5e08a930b9e3547b26eadfcf0acdf6c509"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"b19cebcb80d1eb009d447c7942f52fa088f48848","unresolved":false,"context_lines":[{"line_number":3150,"context_line":"            if instance_action:"},{"line_number":3151,"context_line":"                try:"},{"line_number":3152,"context_line":"                    instance_action.finish()"},{"line_number":3153,"context_line":"                except Exception as e:"},{"line_number":3154,"context_line":"                    print(e)"},{"line_number":3155,"context_line":""},{"line_number":3156,"context_line":"    @action_description("}],"source_content_type":"text/x-python","patch_set":17,"id":"29f8386d_a5e00caf","line":3153,"in_reply_to":"6ea03cb3_2487e8c9","updated":"2023-07-25 14:19:27.000000000","message":"Done","commit_id":"50bebe5e08a930b9e3547b26eadfcf0acdf6c509"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"f6baf1bf10da1a99bfc0292f9b5d3b52bb02570e","unresolved":true,"context_lines":[{"line_number":3150,"context_line":"            if instance_action:"},{"line_number":3151,"context_line":"                try:"},{"line_number":3152,"context_line":"                    instance_action.finish()"},{"line_number":3153,"context_line":"                except Exception as e:"},{"line_number":3154,"context_line":"                    print(e)"},{"line_number":3155,"context_line":""},{"line_number":3156,"context_line":"    @action_description("}],"source_content_type":"text/x-python","patch_set":17,"id":"6ea03cb3_2487e8c9","line":3153,"in_reply_to":"af765efc_a7161f39","updated":"2023-07-21 15:32:21.000000000","message":"if we manually lock the instance before running nova-mange this do not throw any error.\n\nSo does that mean that if nova-manage is locking, then locking (lock/unlock also calls action_start) is the first action associated with this context or req-id? Maybe that\u0027s why earlier they had three actions.\n\n    lock                          // calls action_start\n    refresh action_start and action.finish\n    unlock                        // calls action_start\n    \nI do not see any field in context or instance_action table that says for whom the action started.","commit_id":"50bebe5e08a930b9e3547b26eadfcf0acdf6c509"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"8937139cac8ffc57864bdb39e5139cbcef92054a","unresolved":true,"context_lines":[{"line_number":3034,"context_line":"                    state\u003dinstance.vm_state,"},{"line_number":3035,"context_line":"                    method\u003d\u0027refresh connection_info (must be stopped)\u0027)"},{"line_number":3036,"context_line":""},{"line_number":3037,"context_line":"            if instance.locked:"},{"line_number":3038,"context_line":"                raise exception.InstanceInvalidState("},{"line_number":3039,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3040,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":22,"id":"0b4779ed_2ceb1672","side":"PARENT","line":3037,"updated":"2023-09-05 03:02:28.000000000","message":"I think this paragraph got lost in the shuffle - looks like we\u0027re supposed to abort if the instance is already locked...","commit_id":"7e8e0dd1ab2e46c6f95746b47189e81b5a228c69"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2ab382cbc5334fbfbeb46ced3c61b13dc44b5d35","unresolved":false,"context_lines":[{"line_number":3034,"context_line":"                    state\u003dinstance.vm_state,"},{"line_number":3035,"context_line":"                    method\u003d\u0027refresh connection_info (must be stopped)\u0027)"},{"line_number":3036,"context_line":""},{"line_number":3037,"context_line":"            if instance.locked:"},{"line_number":3038,"context_line":"                raise exception.InstanceInvalidState("},{"line_number":3039,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3040,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":22,"id":"dddb3a18_27c3479c","side":"PARENT","line":3037,"in_reply_to":"03e85158_828cfdab","updated":"2024-02-13 12:53:06.000000000","message":"leving comments in gerrit unresolved does not really help with review.\n\nfuture us should look at comments in code.","commit_id":"7e8e0dd1ab2e46c6f95746b47189e81b5a228c69"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"5e7dbb2b463e4d7f49dd1eb340adb9cffa654dd9","unresolved":true,"context_lines":[{"line_number":3034,"context_line":"                    state\u003dinstance.vm_state,"},{"line_number":3035,"context_line":"                    method\u003d\u0027refresh connection_info (must be stopped)\u0027)"},{"line_number":3036,"context_line":""},{"line_number":3037,"context_line":"            if instance.locked:"},{"line_number":3038,"context_line":"                raise exception.InstanceInvalidState("},{"line_number":3039,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3040,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":22,"id":"1a11390f_dd1afcd7","side":"PARENT","line":3037,"in_reply_to":"0b4779ed_2ceb1672","updated":"2023-09-05 10:58:46.000000000","message":"https://review.opendev.org/c/openstack/nova/+/800634/8..13/nova/cmd/manage.py#b3027\n\nThe original intention for the precondition was to make sure we didn\u0027t accidentally a instance that was previously locked by something else. With this new context manager, we don\u0027t need this precondition anymore, and it can be removed.","commit_id":"7e8e0dd1ab2e46c6f95746b47189e81b5a228c69"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"923d93fb4c376d6933820b2a4769fba276e1a697","unresolved":true,"context_lines":[{"line_number":3034,"context_line":"                    state\u003dinstance.vm_state,"},{"line_number":3035,"context_line":"                    method\u003d\u0027refresh connection_info (must be stopped)\u0027)"},{"line_number":3036,"context_line":""},{"line_number":3037,"context_line":"            if instance.locked:"},{"line_number":3038,"context_line":"                raise exception.InstanceInvalidState("},{"line_number":3039,"context_line":"                    instance_uuid\u003dinstance_uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":3040,"context_line":"                    method\u003d\u0027refresh connection_info (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":22,"id":"03e85158_828cfdab","side":"PARENT","line":3037,"in_reply_to":"1a11390f_dd1afcd7","updated":"2023-09-05 15:37:29.000000000","message":"ack, thanks.\nwe can have this comment as reference for future, I\u0027ll let this comment unresolved.","commit_id":"7e8e0dd1ab2e46c6f95746b47189e81b5a228c69"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"bf2ba7df56d67857078996feab3c1268dcd83395","unresolved":true,"context_lines":[{"line_number":146,"context_line":""},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"@contextmanager"},{"line_number":149,"context_line":"def locked_instance(cell_mapping, instance, reason):"},{"line_number":150,"context_line":"    \"\"\"Context manager to lock and unlock instance,"},{"line_number":151,"context_line":"    lock state will be restored regardless of the success or failure"},{"line_number":152,"context_line":"    of target functionality."}],"source_content_type":"text/x-python","patch_set":22,"id":"f10087c9_1e25d909","line":149,"range":{"start_line":149,"start_character":4,"end_line":149,"end_character":19},"updated":"2023-09-06 11:46:23.000000000","message":"you need to also add unit test for this context manager.","commit_id":"c74e98495182000f3df56f26e3473e46aba1b2f1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2ab382cbc5334fbfbeb46ced3c61b13dc44b5d35","unresolved":false,"context_lines":[{"line_number":146,"context_line":""},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"@contextmanager"},{"line_number":149,"context_line":"def locked_instance(cell_mapping, instance, reason):"},{"line_number":150,"context_line":"    \"\"\"Context manager to lock and unlock instance,"},{"line_number":151,"context_line":"    lock state will be restored regardless of the success or failure"},{"line_number":152,"context_line":"    of target functionality."}],"source_content_type":"text/x-python","patch_set":22,"id":"5d21c280_2f17a649","line":149,"range":{"start_line":149,"start_character":4,"end_line":149,"end_character":19},"in_reply_to":"e7b5655b_3287fb54","updated":"2024-02-13 12:53:06.000000000","message":"Done","commit_id":"c74e98495182000f3df56f26e3473e46aba1b2f1"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"dd3175e1480e4aa904484da2ee89879cd89c8ad9","unresolved":true,"context_lines":[{"line_number":146,"context_line":""},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"@contextmanager"},{"line_number":149,"context_line":"def locked_instance(cell_mapping, instance, reason):"},{"line_number":150,"context_line":"    \"\"\"Context manager to lock and unlock instance,"},{"line_number":151,"context_line":"    lock state will be restored regardless of the success or failure"},{"line_number":152,"context_line":"    of target functionality."}],"source_content_type":"text/x-python","patch_set":22,"id":"e7b5655b_3287fb54","line":149,"range":{"start_line":149,"start_character":4,"end_line":149,"end_character":19},"in_reply_to":"f10087c9_1e25d909","updated":"2023-09-07 15:50:17.000000000","message":"Done","commit_id":"c74e98495182000f3df56f26e3473e46aba1b2f1"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"8937139cac8ffc57864bdb39e5139cbcef92054a","unresolved":true,"context_lines":[{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    compute_api \u003d api.API()"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    instance_was_not_locked \u003d False"},{"line_number":162,"context_line":"    if not instance.locked:"},{"line_number":163,"context_line":"        instance_was_not_locked \u003d True"},{"line_number":164,"context_line":"        with context.target_cell("}],"source_content_type":"text/x-python","patch_set":22,"id":"792ea5ed_a222cecb","line":161,"updated":"2023-09-05 03:02:28.000000000","message":"nit: a more Pythonic suggestion:\n\n    initial_state \u003d \u0027locked\u0027 if instance.locked else \u0027unlocked\u0027\n    \u003csnip\u003e\n    finally:\n      if initial_state \u003d\u003d \u0027unlocked\u0027:\n        \u003cunlock\u003e","commit_id":"c74e98495182000f3df56f26e3473e46aba1b2f1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"bf2ba7df56d67857078996feab3c1268dcd83395","unresolved":true,"context_lines":[{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    compute_api \u003d api.API()"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    instance_was_not_locked \u003d False"},{"line_number":162,"context_line":"    if not instance.locked:"},{"line_number":163,"context_line":"        instance_was_not_locked \u003d True"},{"line_number":164,"context_line":"        with context.target_cell("}],"source_content_type":"text/x-python","patch_set":22,"id":"ef31f872_68c20b11","line":161,"in_reply_to":"1a0b906a_f7606c2a","updated":"2023-09-06 11:46:23.000000000","message":"i agree with artom i think the inital sate way is more readable\n\nthe negation in instance_was_not_locked make it less readable\n\ninstance_was_locked\nor\ninstance_was_unlocked\n\nare both as readable as initial_state\n\ni have a prefernce for artoms version but i dont think this alone warrents a respin.","commit_id":"c74e98495182000f3df56f26e3473e46aba1b2f1"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"63a14ba1296d9eb103816ab6b59b774f40512e30","unresolved":false,"context_lines":[{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    compute_api \u003d api.API()"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    instance_was_not_locked \u003d False"},{"line_number":162,"context_line":"    if not instance.locked:"},{"line_number":163,"context_line":"        instance_was_not_locked \u003d True"},{"line_number":164,"context_line":"        with context.target_cell("}],"source_content_type":"text/x-python","patch_set":22,"id":"c8c28185_1a5450d2","line":161,"in_reply_to":"612d25ca_41ad7856","updated":"2024-01-19 05:45:01.000000000","message":"Done","commit_id":"c74e98495182000f3df56f26e3473e46aba1b2f1"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"923d93fb4c376d6933820b2a4769fba276e1a697","unresolved":true,"context_lines":[{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    compute_api \u003d api.API()"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    instance_was_not_locked \u003d False"},{"line_number":162,"context_line":"    if not instance.locked:"},{"line_number":163,"context_line":"        instance_was_not_locked \u003d True"},{"line_number":164,"context_line":"        with context.target_cell("}],"source_content_type":"text/x-python","patch_set":22,"id":"1a0b906a_f7606c2a","line":161,"in_reply_to":"792ea5ed_a222cecb","updated":"2023-09-05 15:37:29.000000000","message":"ack, IMHO var instance_was_not_locked seems more readable.","commit_id":"c74e98495182000f3df56f26e3473e46aba1b2f1"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"dd3175e1480e4aa904484da2ee89879cd89c8ad9","unresolved":true,"context_lines":[{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    compute_api \u003d api.API()"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    instance_was_not_locked \u003d False"},{"line_number":162,"context_line":"    if not instance.locked:"},{"line_number":163,"context_line":"        instance_was_not_locked \u003d True"},{"line_number":164,"context_line":"        with context.target_cell("}],"source_content_type":"text/x-python","patch_set":22,"id":"612d25ca_41ad7856","line":161,"in_reply_to":"ef31f872_68c20b11","updated":"2023-09-07 15:50:17.000000000","message":"ack, updated and fixed the merge conflict in top patch so respining.","commit_id":"c74e98495182000f3df56f26e3473e46aba1b2f1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2ab382cbc5334fbfbeb46ced3c61b13dc44b5d35","unresolved":true,"context_lines":[{"line_number":3049,"context_line":""},{"line_number":3050,"context_line":"        :param instance_uuid: UUID of instance"},{"line_number":3051,"context_line":"        :param volume_id: ID of volume attached to the instance"},{"line_number":3052,"context_line":"        :param connector: Connector with which to create the new attachment"},{"line_number":3053,"context_line":"        \"\"\""},{"line_number":3054,"context_line":""},{"line_number":3055,"context_line":"        ctxt \u003d context.get_admin_context()"}],"source_content_type":"text/x-python","patch_set":25,"id":"39dc0215_dd79e0ac","line":3052,"updated":"2024-02-13 12:53:06.000000000","message":"nit: this funcion returns a status code that is used as the status fo the command\nso we shoudl note that here.","commit_id":"ad14712c721681112ddf54a5e91349407b5e96a3"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"c9e54ea7da68bd147bb7e434a3952ccdc7d16733","unresolved":false,"context_lines":[{"line_number":3049,"context_line":""},{"line_number":3050,"context_line":"        :param instance_uuid: UUID of instance"},{"line_number":3051,"context_line":"        :param volume_id: ID of volume attached to the instance"},{"line_number":3052,"context_line":"        :param connector: Connector with which to create the new attachment"},{"line_number":3053,"context_line":"        \"\"\""},{"line_number":3054,"context_line":""},{"line_number":3055,"context_line":"        ctxt \u003d context.get_admin_context()"}],"source_content_type":"text/x-python","patch_set":25,"id":"72e5c671_7cf9b146","line":3052,"in_reply_to":"39dc0215_dd79e0ac","updated":"2024-02-14 10:28:37.000000000","message":"Acknowledged","commit_id":"ad14712c721681112ddf54a5e91349407b5e96a3"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2ab382cbc5334fbfbeb46ced3c61b13dc44b5d35","unresolved":true,"context_lines":[{"line_number":3127,"context_line":"            # status from attaching to in-use ahead of the instance"},{"line_number":3128,"context_line":"            # restarting"},{"line_number":3129,"context_line":"            volume_api.attachment_complete(cctxt, new_attachment_id)"},{"line_number":3130,"context_line":"            return 0"},{"line_number":3131,"context_line":""},{"line_number":3132,"context_line":"        finally:"},{"line_number":3133,"context_line":"            # If the bdm.attachment_id wasn\u0027t updated make sure we clean"}],"source_content_type":"text/x-python","patch_set":25,"id":"ff47d9ce_504b1095","line":3130,"updated":"2024-02-13 12:53:06.000000000","message":"... this is form the existing code but we are returning 0 because that is what will be returend form the calling funciton.\n\nhttps://github.com/openstack/nova/blob/35af4b345d997b63f999a090e236d91b78ea4304/nova/cmd/manage.py#L3181\n\ni would honestly prefer if we eiteher docuemented this or removed the return.\nand just returned 0  in refresh instead.","commit_id":"ad14712c721681112ddf54a5e91349407b5e96a3"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"be44a8e3dee98e7cc405506ed040ee7db835cf73","unresolved":false,"context_lines":[{"line_number":3127,"context_line":"            # status from attaching to in-use ahead of the instance"},{"line_number":3128,"context_line":"            # restarting"},{"line_number":3129,"context_line":"            volume_api.attachment_complete(cctxt, new_attachment_id)"},{"line_number":3130,"context_line":"            return 0"},{"line_number":3131,"context_line":""},{"line_number":3132,"context_line":"        finally:"},{"line_number":3133,"context_line":"            # If the bdm.attachment_id wasn\u0027t updated make sure we clean"}],"source_content_type":"text/x-python","patch_set":25,"id":"67806589_954bae75","line":3130,"in_reply_to":"7e4dedee_a1acd3fb","updated":"2024-02-23 12:13:54.000000000","message":"Done","commit_id":"ad14712c721681112ddf54a5e91349407b5e96a3"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"c9e54ea7da68bd147bb7e434a3952ccdc7d16733","unresolved":true,"context_lines":[{"line_number":3127,"context_line":"            # status from attaching to in-use ahead of the instance"},{"line_number":3128,"context_line":"            # restarting"},{"line_number":3129,"context_line":"            volume_api.attachment_complete(cctxt, new_attachment_id)"},{"line_number":3130,"context_line":"            return 0"},{"line_number":3131,"context_line":""},{"line_number":3132,"context_line":"        finally:"},{"line_number":3133,"context_line":"            # If the bdm.attachment_id wasn\u0027t updated make sure we clean"}],"source_content_type":"text/x-python","patch_set":25,"id":"7e4dedee_a1acd3fb","line":3130,"in_reply_to":"ff47d9ce_504b1095","updated":"2024-02-14 10:28:37.000000000","message":"Done, documented in _refresh","commit_id":"ad14712c721681112ddf54a5e91349407b5e96a3"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"c9e54ea7da68bd147bb7e434a3952ccdc7d16733","unresolved":true,"context_lines":[{"line_number":3173,"context_line":"        \"\"\"Refresh the connection_info associated with a volume attachment"},{"line_number":3174,"context_line":""},{"line_number":3175,"context_line":"        Return codes:"},{"line_number":3176,"context_line":"        * 0: Command completed successfully."},{"line_number":3177,"context_line":"        * 1: An unexpected error happened."},{"line_number":3178,"context_line":"        * 2: Connector path does not exist."},{"line_number":3179,"context_line":"        * 3: Failed to open connector path."}],"source_content_type":"text/x-python","patch_set":25,"id":"54d8831a_94880ef5","line":3176,"updated":"2024-02-14 10:28:37.000000000","message":"documented here too.","commit_id":"ad14712c721681112ddf54a5e91349407b5e96a3"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"be44a8e3dee98e7cc405506ed040ee7db835cf73","unresolved":false,"context_lines":[{"line_number":3173,"context_line":"        \"\"\"Refresh the connection_info associated with a volume attachment"},{"line_number":3174,"context_line":""},{"line_number":3175,"context_line":"        Return codes:"},{"line_number":3176,"context_line":"        * 0: Command completed successfully."},{"line_number":3177,"context_line":"        * 1: An unexpected error happened."},{"line_number":3178,"context_line":"        * 2: Connector path does not exist."},{"line_number":3179,"context_line":"        * 3: Failed to open connector path."}],"source_content_type":"text/x-python","patch_set":25,"id":"6b9b7bcd_22bb452d","line":3176,"in_reply_to":"54d8831a_94880ef5","updated":"2024-02-23 12:13:54.000000000","message":"Acknowledged","commit_id":"ad14712c721681112ddf54a5e91349407b5e96a3"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"dd3d634e92f05927c648461458a0c45ed0cfddc7","unresolved":true,"context_lines":[{"line_number":164,"context_line":"    initial_state \u003d \u0027locked\u0027 if instance.locked else \u0027unlocked\u0027"},{"line_number":165,"context_line":"    if not instance.locked:"},{"line_number":166,"context_line":"        with context.target_cell("},{"line_number":167,"context_line":"                    context.get_admin_context(),"},{"line_number":168,"context_line":"                    cell_mapping"},{"line_number":169,"context_line":"                ) as cctxt:"},{"line_number":170,"context_line":"            compute_api.lock(cctxt, instance, reason\u003dreason)"},{"line_number":171,"context_line":"    try:"},{"line_number":172,"context_line":"        yield"}],"source_content_type":"text/x-python","patch_set":27,"id":"3494e6e4_466ed5bc","line":169,"range":{"start_line":167,"start_character":0,"end_line":169,"end_character":27},"updated":"2024-02-29 12:15:46.000000000","message":"nit: dedent (x2)","commit_id":"211737d0ce23a8a9b1f3953f4816150a9a3271fa"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"9087d9b227a043b71445768f32e4548cdf8719b9","unresolved":false,"context_lines":[{"line_number":164,"context_line":"    initial_state \u003d \u0027locked\u0027 if instance.locked else \u0027unlocked\u0027"},{"line_number":165,"context_line":"    if not instance.locked:"},{"line_number":166,"context_line":"        with context.target_cell("},{"line_number":167,"context_line":"                    context.get_admin_context(),"},{"line_number":168,"context_line":"                    cell_mapping"},{"line_number":169,"context_line":"                ) as cctxt:"},{"line_number":170,"context_line":"            compute_api.lock(cctxt, instance, reason\u003dreason)"},{"line_number":171,"context_line":"    try:"},{"line_number":172,"context_line":"        yield"}],"source_content_type":"text/x-python","patch_set":27,"id":"547b9f81_3339f07d","line":169,"range":{"start_line":167,"start_character":0,"end_line":169,"end_character":27},"in_reply_to":"3494e6e4_466ed5bc","updated":"2024-02-29 19:53:13.000000000","message":"ack, fixed in later PS, thanks\nhttps://review.opendev.org/c/openstack/nova/+/877446","commit_id":"211737d0ce23a8a9b1f3953f4816150a9a3271fa"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"dd3d634e92f05927c648461458a0c45ed0cfddc7","unresolved":true,"context_lines":[{"line_number":173,"context_line":"    finally:"},{"line_number":174,"context_line":"        if initial_state \u003d\u003d \u0027unlocked\u0027:"},{"line_number":175,"context_line":"            with context.target_cell("},{"line_number":176,"context_line":"                    context.get_admin_context(),"},{"line_number":177,"context_line":"                    cell_mapping"},{"line_number":178,"context_line":"                ) as cctxt:"},{"line_number":179,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":""}],"source_content_type":"text/x-python","patch_set":27,"id":"668228e8_2bf5f183","line":178,"range":{"start_line":176,"start_character":0,"end_line":178,"end_character":27},"updated":"2024-02-29 12:15:46.000000000","message":"nit: dedent","commit_id":"211737d0ce23a8a9b1f3953f4816150a9a3271fa"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"9087d9b227a043b71445768f32e4548cdf8719b9","unresolved":false,"context_lines":[{"line_number":173,"context_line":"    finally:"},{"line_number":174,"context_line":"        if initial_state \u003d\u003d \u0027unlocked\u0027:"},{"line_number":175,"context_line":"            with context.target_cell("},{"line_number":176,"context_line":"                    context.get_admin_context(),"},{"line_number":177,"context_line":"                    cell_mapping"},{"line_number":178,"context_line":"                ) as cctxt:"},{"line_number":179,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":""}],"source_content_type":"text/x-python","patch_set":27,"id":"879787e8_74ae88d6","line":178,"range":{"start_line":176,"start_character":0,"end_line":178,"end_character":27},"in_reply_to":"668228e8_2bf5f183","updated":"2024-02-29 19:53:13.000000000","message":"Done","commit_id":"211737d0ce23a8a9b1f3953f4816150a9a3271fa"}],"nova/compute/manager.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"f872b108ba71d32877c1afe06f08b48fc7fae9f6","unresolved":true,"context_lines":[{"line_number":3687,"context_line":"        instance.save("},{"line_number":3688,"context_line":"            expected_task_state\u003d[task_states.REBUILD_BLOCK_DEVICE_MAPPING])"},{"line_number":3689,"context_line":""},{"line_number":3690,"context_line":"        with instance.mutated_migration_context():"},{"line_number":3691,"context_line":"            self.driver.spawn(context, instance, image_meta, injected_files,"},{"line_number":3692,"context_line":"                              admin_password, allocations,"},{"line_number":3693,"context_line":"                              network_info\u003dnetwork_info,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7bcf0d28_ecd223df","line":3690,"updated":"2023-02-16 03:03:21.000000000","message":"Used here.","commit_id":"f87275cbedb01d49c14bf5b89fd832eff2a4ae16"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"db15dabeab0505a5a671f32c4d5c048710cf2062","unresolved":false,"context_lines":[{"line_number":3687,"context_line":"        instance.save("},{"line_number":3688,"context_line":"            expected_task_state\u003d[task_states.REBUILD_BLOCK_DEVICE_MAPPING])"},{"line_number":3689,"context_line":""},{"line_number":3690,"context_line":"        with instance.mutated_migration_context():"},{"line_number":3691,"context_line":"            self.driver.spawn(context, instance, image_meta, injected_files,"},{"line_number":3692,"context_line":"                              admin_password, allocations,"},{"line_number":3693,"context_line":"                              network_info\u003dnetwork_info,"}],"source_content_type":"text/x-python","patch_set":2,"id":"0845287d_2f2ba99e","line":3690,"in_reply_to":"7bcf0d28_ecd223df","updated":"2023-02-16 14:25:37.000000000","message":"Ack","commit_id":"f87275cbedb01d49c14bf5b89fd832eff2a4ae16"}],"nova/objects/instance.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"f872b108ba71d32877c1afe06f08b48fc7fae9f6","unresolved":true,"context_lines":[{"line_number":1049,"context_line":"                setattr(self, inst_attr_name, attr_value)"},{"line_number":1050,"context_line":""},{"line_number":1051,"context_line":"    @contextlib.contextmanager"},{"line_number":1052,"context_line":"    def mutated_migration_context(self, revert\u003dFalse):"},{"line_number":1053,"context_line":"        \"\"\"Context manager to temporarily apply/revert the migration context."},{"line_number":1054,"context_line":""},{"line_number":1055,"context_line":"        Calling .save() from within the context manager means that the mutated"}],"source_content_type":"text/x-python","patch_set":2,"id":"c5304a0e_a25044f1","line":1052,"updated":"2023-02-16 03:03:21.000000000","message":"Defined here.","commit_id":"f87275cbedb01d49c14bf5b89fd832eff2a4ae16"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"db15dabeab0505a5a671f32c4d5c048710cf2062","unresolved":false,"context_lines":[{"line_number":1049,"context_line":"                setattr(self, inst_attr_name, attr_value)"},{"line_number":1050,"context_line":""},{"line_number":1051,"context_line":"    @contextlib.contextmanager"},{"line_number":1052,"context_line":"    def mutated_migration_context(self, revert\u003dFalse):"},{"line_number":1053,"context_line":"        \"\"\"Context manager to temporarily apply/revert the migration context."},{"line_number":1054,"context_line":""},{"line_number":1055,"context_line":"        Calling .save() from within the context manager means that the mutated"}],"source_content_type":"text/x-python","patch_set":2,"id":"4be89735_01d51bcd","line":1052,"in_reply_to":"c5304a0e_a25044f1","updated":"2023-02-16 14:25:37.000000000","message":"Ack","commit_id":"f87275cbedb01d49c14bf5b89fd832eff2a4ae16"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"e750537690ba44129ef47c3ecfa2a1fbe5c50974","unresolved":true,"context_lines":[{"line_number":1077,"context_line":"                setattr(self, attr_name, current_values[attr_name])"},{"line_number":1078,"context_line":""},{"line_number":1079,"context_line":"    @contextlib.contextmanager"},{"line_number":1080,"context_line":"    def lock_instance_context(self, compute_api, cctxt, im, action, reason):"},{"line_number":1081,"context_line":"        \"\"\"Context manager to lock and unlock instance"},{"line_number":1082,"context_line":"        on the basis of instance current state"},{"line_number":1083,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"9cc30a0e_3b749ffe","line":1080,"updated":"2023-02-16 14:42:50.000000000","message":"So I\u0027m not sure about the location of this method in the instance object - I used the mutated_migration_context() as an example of the mechanics that we need to implement, not necessarily an indication of where the new method should live. I think it\u0027s probably fine to keep this in cmd/manage.py.","commit_id":"617c3c14c560f9be9673f9a219b0ce5b20b45601"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"e28a66c7d3bc3c158c0eb98470795758e7f672b3","unresolved":false,"context_lines":[{"line_number":1077,"context_line":"                setattr(self, attr_name, current_values[attr_name])"},{"line_number":1078,"context_line":""},{"line_number":1079,"context_line":"    @contextlib.contextmanager"},{"line_number":1080,"context_line":"    def lock_instance_context(self, compute_api, cctxt, im, action, reason):"},{"line_number":1081,"context_line":"        \"\"\"Context manager to lock and unlock instance"},{"line_number":1082,"context_line":"        on the basis of instance current state"},{"line_number":1083,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"afdb269b_7ced2db4","line":1080,"in_reply_to":"9cc30a0e_3b749ffe","updated":"2023-02-17 08:01:26.000000000","message":"Moved to cmd/manage.py","commit_id":"617c3c14c560f9be9673f9a219b0ce5b20b45601"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"e750537690ba44129ef47c3ecfa2a1fbe5c50974","unresolved":true,"context_lines":[{"line_number":1088,"context_line":"                state\u003dself.vm_state,"},{"line_number":1089,"context_line":"                method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":1090,"context_line":""},{"line_number":1091,"context_line":"        if self.locked:"},{"line_number":1092,"context_line":"            raise exception.InstanceInvalidState("},{"line_number":1093,"context_line":"                instance_uuid\u003dself.uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":1094,"context_line":"                method\u003d action + \u0027 (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":4,"id":"efb06e08_710d3c44","line":1091,"updated":"2023-02-16 14:42:50.000000000","message":"Ah OK, so we only lock it if it\u0027s not already locked...","commit_id":"617c3c14c560f9be9673f9a219b0ce5b20b45601"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"e28a66c7d3bc3c158c0eb98470795758e7f672b3","unresolved":false,"context_lines":[{"line_number":1088,"context_line":"                state\u003dself.vm_state,"},{"line_number":1089,"context_line":"                method\u003d action + \u0027 (must be stopped)\u0027)"},{"line_number":1090,"context_line":""},{"line_number":1091,"context_line":"        if self.locked:"},{"line_number":1092,"context_line":"            raise exception.InstanceInvalidState("},{"line_number":1093,"context_line":"                instance_uuid\u003dself.uuid, attr\u003d\u0027locked\u0027, state\u003d\u0027True\u0027,"},{"line_number":1094,"context_line":"                method\u003d action + \u0027 (must be unlocked)\u0027)"}],"source_content_type":"text/x-python","patch_set":4,"id":"b1d87685_24552e10","line":1091,"in_reply_to":"efb06e08_710d3c44","updated":"2023-02-17 08:01:26.000000000","message":"Done","commit_id":"617c3c14c560f9be9673f9a219b0ce5b20b45601"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"e750537690ba44129ef47c3ecfa2a1fbe5c50974","unresolved":true,"context_lines":[{"line_number":1105,"context_line":"                    context.get_admin_context(),"},{"line_number":1106,"context_line":"                    im.cell_mapping"},{"line_number":1107,"context_line":"                ) as cctxt:"},{"line_number":1108,"context_line":"                compute_api.unlock(cctxt, self)"},{"line_number":1109,"context_line":""},{"line_number":1110,"context_line":"    @base.remotable"},{"line_number":1111,"context_line":"    def drop_migration_context(self):"}],"source_content_type":"text/x-python","patch_set":4,"id":"3529aac9_80298960","line":1108,"updated":"2023-02-16 14:42:50.000000000","message":"... meaning here we\u0027re good to unlock it regarldess, as you\u0027re doing.","commit_id":"617c3c14c560f9be9673f9a219b0ce5b20b45601"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"e28a66c7d3bc3c158c0eb98470795758e7f672b3","unresolved":false,"context_lines":[{"line_number":1105,"context_line":"                    context.get_admin_context(),"},{"line_number":1106,"context_line":"                    im.cell_mapping"},{"line_number":1107,"context_line":"                ) as cctxt:"},{"line_number":1108,"context_line":"                compute_api.unlock(cctxt, self)"},{"line_number":1109,"context_line":""},{"line_number":1110,"context_line":"    @base.remotable"},{"line_number":1111,"context_line":"    def drop_migration_context(self):"}],"source_content_type":"text/x-python","patch_set":4,"id":"3c550e9a_8c5e2024","line":1108,"in_reply_to":"3529aac9_80298960","updated":"2023-02-17 08:01:26.000000000","message":"Done","commit_id":"617c3c14c560f9be9673f9a219b0ce5b20b45601"}],"nova/tests/unit/cmd/test_manage.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a95903e33780f797540880afe6b7bb5ad70b205c","unresolved":true,"context_lines":[{"line_number":3507,"context_line":"        ) as (mock_get_context, mock_target_cell, _, _):"},{"line_number":3508,"context_line":"            fake_target_cell_mock \u003d mock.MagicMock()"},{"line_number":3509,"context_line":"            fake_target_cell_mock.__enter__.return_value \u003d cell_ctxt"},{"line_number":3510,"context_line":"            cell_ctxt.cell_uuid \u003d \u002739fd7a1f-db62-45bc-a7b6-8137cef87c9d\u0027"},{"line_number":3511,"context_line":"            mock_target_cell.return_value \u003d fake_target_cell_mock"},{"line_number":3512,"context_line":""},{"line_number":3513,"context_line":"            ret \u003d self.commands.refresh("}],"source_content_type":"text/x-python","patch_set":13,"id":"5f40ee15_743e5256","line":3510,"updated":"2023-07-17 18:47:52.000000000","message":"Nit: it might make more sense for readability to put this under L3494 where cell_ctxt is defined.","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"41e490880be59a5858074281e103fe2a6220373f","unresolved":false,"context_lines":[{"line_number":3507,"context_line":"        ) as (mock_get_context, mock_target_cell, _, _):"},{"line_number":3508,"context_line":"            fake_target_cell_mock \u003d mock.MagicMock()"},{"line_number":3509,"context_line":"            fake_target_cell_mock.__enter__.return_value \u003d cell_ctxt"},{"line_number":3510,"context_line":"            cell_ctxt.cell_uuid \u003d \u002739fd7a1f-db62-45bc-a7b6-8137cef87c9d\u0027"},{"line_number":3511,"context_line":"            mock_target_cell.return_value \u003d fake_target_cell_mock"},{"line_number":3512,"context_line":""},{"line_number":3513,"context_line":"            ret \u003d self.commands.refresh("}],"source_content_type":"text/x-python","patch_set":13,"id":"ae66d34f_11951cc9","line":3510,"in_reply_to":"5f40ee15_743e5256","updated":"2023-07-18 10:16:43.000000000","message":"Done","commit_id":"296db6e73e5df5c23b1ab7f2e11bf25c2ccf7e9f"}]}