)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8d9f3cfd4c5440061cefed2210efbe1b67975bae","unresolved":true,"context_lines":[{"line_number":17,"context_line":"attached (because the target and lun matched an existing volume on the"},{"line_number":18,"context_line":"host) and would try to disconnect, resulting in errors on the compute"},{"line_number":19,"context_line":"logs."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: I21109752ff1c56d3cefa58fcd36c68bf468e0a73"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"ba17725f_b67ba2e1","line":20,"updated":"2023-03-20 19:51:09.000000000","message":"this needs a bug or specless blueprint by the way and you should not use bz-numebr as topics.\n\nthe topic is ment to be bug/\u003claunchpad bug number\u003e or bp/\u003cbluepirnt name\u003e oir spec/\u003cblueprint name\u003e \n\nbasically we try to align the topic ot the launchpad url \n\n\nwe do not enforce that but we do not use downstream references in upstream commits.","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"055bbc1baf355f633d3b83ab0e26f8ab24892a74","unresolved":false,"context_lines":[{"line_number":17,"context_line":"attached (because the target and lun matched an existing volume on the"},{"line_number":18,"context_line":"host) and would try to disconnect, resulting in errors on the compute"},{"line_number":19,"context_line":"logs."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: I21109752ff1c56d3cefa58fcd36c68bf468e0a73"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"046290f6_9b799877","line":20,"in_reply_to":"ba17725f_b67ba2e1","updated":"2023-03-21 11:22:00.000000000","message":"Ack, created upstream bug for same.","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a9788f60f57373b3765f22d65d067e4bc81131ae","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"b351c9a2_5fd1954f","updated":"2023-03-17 18:35:48.000000000","message":"I think the extra check is good, and I don\u0027t see us doing anything else... We should probably word the error message in a way that prompts the admin to do some digging. *most likely* they just ran get_connector on the wrong host, but perhaps instance.host is wrong after a failed live migration or something, and further cleanup is necessary.","commit_id":"a558211e390ebe9d6501642217c71de6f78efaf0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d04c4d33373aa9885b0ba28f057e85a4fba44ab7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"74ec10ca_82d67fce","updated":"2023-07-12 18:04:27.000000000","message":"I think there\u0027s some unnecessary complexity in here and this PS is also losing handling of OSError when opening the connector file. I think we probably need to add a new return code because none of the existing ones fit? Otherwise return code 1 is the only generic one and \"unexpected\" usually means an exception we didn\u0027t account for.","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4ddfc8e11a0c58dbd78deb7b193228c2d7819b3f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"2ac00586_d643ca14","updated":"2023-07-14 13:44:51.000000000","message":"Looks okay to me now.","commit_id":"3b9a275b8f107e36a598dd4cd78333e04bb18894"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"8ce16bd47ea1c5f316262a7f02b9946db3a907c8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"9acd397c_1f08d3aa","updated":"2023-07-14 09:21:55.000000000","message":"recheck oslo_db.exception.DBConnectionError unrealted","commit_id":"3b9a275b8f107e36a598dd4cd78333e04bb18894"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"6115391cfd1210d95bb13596f623ade21008a16e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":22,"id":"510b077f_9c968345","updated":"2023-11-30 17:14:56.000000000","message":"recheck logs are gone","commit_id":"e340f786419193ff267c75fd025d787eb3b51cb1"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"631e20308947f24dce40dcf9900fef679525d6b3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"6f753dcb_5c669788","updated":"2024-02-12 00:56:59.000000000","message":"Soft -1, but I think the way you\u0027ve done this (moving the OSError around and adding a test for ValueError) means is should be 2 patches: one for the improved error handling and messages, and one for the connector host check.","commit_id":"32424a14877e14b272db32c70b66bdb3d0041b56"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"01406b82225bd045f55604c53a5e72bb517d406d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":24,"id":"2a378095_50fa983c","updated":"2024-02-13 12:43:37.000000000","message":"this patch looks ok. i dont really think we need to hold this longer to adress the remaining nits.","commit_id":"9c980798fe7e1bb5f9f6e173457a9b0d22642bdd"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"eb489b7233acaf03d060f0b72b97289daeb67684","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":27,"id":"8f1feee3_a0f693ad","updated":"2024-02-29 12:17:31.000000000","message":"Looks mostly good but it\u0027s in merge conflict and you forgot to update the return code table in `doc/source/cli/nova-manage.rst`. Fix both and I\u0027m +2","commit_id":"460d6394325ca530919603c8f2edf749dd96a58a"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"bcba6d06e3fb0e644d512da1a357dfaf5f3a6d74","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":27,"id":"ca4fc0b9_f2aef995","updated":"2024-02-19 12:33:39.000000000","message":"recheck grenade script failed","commit_id":"460d6394325ca530919603c8f2edf749dd96a58a"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"523df2a57050e72bdb7a0f45cabebbd317407251","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":27,"id":"9b7195da_313553dc","updated":"2024-02-19 16:20:40.000000000","message":"recheck nova-grenade-multinode","commit_id":"460d6394325ca530919603c8f2edf749dd96a58a"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"d01720bbccd61c33fda89b2c0f7a57990e72ecde","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":27,"id":"bb837b3d_37065296","updated":"2024-02-19 09:21:04.000000000","message":"recheck tempest-integrated-compute-rbac-old-defaults timeout","commit_id":"460d6394325ca530919603c8f2edf749dd96a58a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"7344dd2395e20ee1c4f30ebd6b8d25455a2e8141","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":28,"id":"5cddc1c5_b2b00b5d","updated":"2024-02-29 20:22:00.000000000","message":"You still haven\u0027t updated the doc","commit_id":"7fb1cf9e73fc041a2582fcb67fe8b925f1c76d87"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"fd69d4c2717e598cf0e67ef37fbf2ca05cf5b049","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":28,"id":"bccb2354_36ee3cc3","in_reply_to":"5cddc1c5_b2b00b5d","updated":"2024-03-01 09:15:50.000000000","message":"missed it earlier, updated, thanks","commit_id":"7fb1cf9e73fc041a2582fcb67fe8b925f1c76d87"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"393ea50525510117ef01dd08c5fcfe523370d619","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":29,"id":"322d4f63_7176b572","updated":"2024-03-05 16:50:20.000000000","message":"my previous comments and dans have been addressed so i think this is ok to proceed with.","commit_id":"a8f81d5f08692c16d538af3197460d9426cc00a1"}],"nova/cmd/manage.py":[{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"62eb7d7c5957fed122fdc200b9d81e90b550e127","unresolved":true,"context_lines":[{"line_number":3067,"context_line":"                # remove_volume_connection as is available in the private"},{"line_number":3068,"context_line":"                # method within the manager."},{"line_number":3069,"context_line":"                compute_rpcapi.remove_volume_connection("},{"line_number":3070,"context_line":"                    cctxt, instance, volume_id, connector[\u0027host\u0027])"},{"line_number":3071,"context_line":""},{"line_number":3072,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3073,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":1,"id":"554d1b06_2f15425a","line":3070,"updated":"2023-03-15 05:53:00.000000000","message":"@reviewers need suggestion here.\nThe connector is retrieved from the compute host, so every time value of connector[\"host\"] depends on the compute host from which we retrieved the connector.\nex:\n```\n{\"platform\": \"x86_64\", \"os_type\": \"linux\", \"ip\": \"172.17.1.149\", \n\n\"host\": \"compute-1\", \n\n\"multipath\": false, \"initiator\": \"iqn.1994-05.com.redhat:7566153f257\", \"do_local_attach\": false, \"system uuid\": \"33cc35bb-b550-49fd-b52c-6397b79d5753\", \"nqn\": \"nqn.2014-08.org.nvmexpress:uuid:d1d3b4f8-388b-4b92-bfb4-f68a7618ce25\", \"nvme_native_multipath\": false}\n```\n\nAs per suggestion from @Gorka, If we use the \"host\" from the connector properties, we are dependent on connector which user/operator sent, I feel we should have some check in place to verify if we get the correct connector.","commit_id":"717519ed8be9c1518e64b64bcb7e96f3bcdcb134"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e2f70c4104d94cd7d3dfbecdbb0eefca9b87c47a","unresolved":false,"context_lines":[{"line_number":3067,"context_line":"                # remove_volume_connection as is available in the private"},{"line_number":3068,"context_line":"                # method within the manager."},{"line_number":3069,"context_line":"                compute_rpcapi.remove_volume_connection("},{"line_number":3070,"context_line":"                    cctxt, instance, volume_id, connector[\u0027host\u0027])"},{"line_number":3071,"context_line":""},{"line_number":3072,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3073,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":1,"id":"98ae8b6b_f97074d2","line":3070,"in_reply_to":"00c66a7f_713f3b31","updated":"2023-03-20 19:53:49.000000000","message":"Done","commit_id":"717519ed8be9c1518e64b64bcb7e96f3bcdcb134"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"60c09fe1fdaed59e3e0362736d221a0c8626d474","unresolved":true,"context_lines":[{"line_number":3067,"context_line":"                # remove_volume_connection as is available in the private"},{"line_number":3068,"context_line":"                # method within the manager."},{"line_number":3069,"context_line":"                compute_rpcapi.remove_volume_connection("},{"line_number":3070,"context_line":"                    cctxt, instance, volume_id, connector[\u0027host\u0027])"},{"line_number":3071,"context_line":""},{"line_number":3072,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3073,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":1,"id":"f0b6c063_6e5bbf32","line":3070,"in_reply_to":"554d1b06_2f15425a","updated":"2023-03-15 05:54:21.000000000","message":"please correct me if I did not understood correctly.","commit_id":"717519ed8be9c1518e64b64bcb7e96f3bcdcb134"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1214ac23fd32aab2e5d9004bd19c1d2cd78ddf4f","unresolved":true,"context_lines":[{"line_number":3067,"context_line":"                # remove_volume_connection as is available in the private"},{"line_number":3068,"context_line":"                # method within the manager."},{"line_number":3069,"context_line":"                compute_rpcapi.remove_volume_connection("},{"line_number":3070,"context_line":"                    cctxt, instance, volume_id, connector[\u0027host\u0027])"},{"line_number":3071,"context_line":""},{"line_number":3072,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3073,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":1,"id":"00c66a7f_713f3b31","line":3070,"in_reply_to":"e9230450_2e07ca9e","updated":"2023-03-15 12:07:48.000000000","message":"we shoudl probaly still use instance.host but check if instance.host \u003d\u003d connector[\u0027host\u0027] and raise an excpetion if they dont match.","commit_id":"717519ed8be9c1518e64b64bcb7e96f3bcdcb134"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"6dfd7289273e9b38a60a12b1ac49790e662d2a6a","unresolved":true,"context_lines":[{"line_number":3067,"context_line":"                # remove_volume_connection as is available in the private"},{"line_number":3068,"context_line":"                # method within the manager."},{"line_number":3069,"context_line":"                compute_rpcapi.remove_volume_connection("},{"line_number":3070,"context_line":"                    cctxt, instance, volume_id, connector[\u0027host\u0027])"},{"line_number":3071,"context_line":""},{"line_number":3072,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3073,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":1,"id":"e9230450_2e07ca9e","line":3070,"in_reply_to":"f0b6c063_6e5bbf32","updated":"2023-03-15 08:47:14.000000000","message":"\u0027foo\u0027 !\u003d \u0027fake-host\u0027\nhttps://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_cde/877446/1/check/openstack-tox-py310/cdef5e9/testr_results.html","commit_id":"717519ed8be9c1518e64b64bcb7e96f3bcdcb134"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a9788f60f57373b3765f22d65d067e4bc81131ae","unresolved":true,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise exception.TooManyComputesForHost("},{"line_number":3062,"context_line":"                        num_computes\u003d2, host\u003dinstance.host)"},{"line_number":3063,"context_line":""},{"line_number":3064,"context_line":"                # Delete the existing volume attachment if present in the bdm."}],"source_content_type":"text/x-python","patch_set":3,"id":"7bdbd8a5_6f575d7a","line":3061,"updated":"2023-03-17 18:35:48.000000000","message":"I don\u0027t think this exception is appropriate, probably something like ValueError would be better, because that\u0027s really happening is that the user ran the get_connector command on the wrong compute...","commit_id":"a558211e390ebe9d6501642217c71de6f78efaf0"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"9ac9ec4a82eeb6504ce1aacb21267f3efcc941eb","unresolved":false,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise exception.TooManyComputesForHost("},{"line_number":3062,"context_line":"                        num_computes\u003d2, host\u003dinstance.host)"},{"line_number":3063,"context_line":""},{"line_number":3064,"context_line":"                # Delete the existing volume attachment if present in the bdm."}],"source_content_type":"text/x-python","patch_set":3,"id":"6c8cdb09_513532ac","line":3061,"in_reply_to":"1ff38239_d81d213a","updated":"2023-03-20 13:29:04.000000000","message":"Done","commit_id":"a558211e390ebe9d6501642217c71de6f78efaf0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2742c532aa239b537e8e0e9adb6a3a39e26bf843","unresolved":true,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise exception.TooManyComputesForHost("},{"line_number":3062,"context_line":"                        num_computes\u003d2, host\u003dinstance.host)"},{"line_number":3063,"context_line":""},{"line_number":3064,"context_line":"                # Delete the existing volume attachment if present in the bdm."}],"source_content_type":"text/x-python","patch_set":3,"id":"1ff38239_d81d213a","line":3061,"in_reply_to":"7bdbd8a5_6f575d7a","updated":"2023-03-17 22:09:35.000000000","message":"+1, this condition means that the user provided mismatched host connector file and instance UUID when they ran \u0027nova-manage volume_attachment refresh\u0027. A ValueError would be consistent with the way other input errors are handled in this file and the message might say something like \"The host connector does not match the instance host. The \u0027nova-manage volume_attachment get_connector\u0027 command may have been run on the wrong compute host or the instance.host may be wrong and in need of repair.\" Or something better that I haven\u0027t thought of.","commit_id":"a558211e390ebe9d6501642217c71de6f78efaf0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"ce67dc7c6b5f6ed22a7b171b99697f9e10d3c9a9","unresolved":true,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise ValueError"},{"line_number":3062,"context_line":""},{"line_number":3063,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3064,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":4,"id":"af2d50ff_3df19838","line":3061,"updated":"2023-03-20 19:31:17.000000000","message":"Pass the error message to the ValueError, as such:\n\n  ValueError(\u0027The compute host etc...\u0027)","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"055bbc1baf355f633d3b83ab0e26f8ab24892a74","unresolved":false,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise ValueError"},{"line_number":3062,"context_line":""},{"line_number":3063,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3064,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":4,"id":"e986b80f_49f0f824","line":3061,"in_reply_to":"38dcf4ad_167fced6","updated":"2023-03-21 11:22:00.000000000","message":"Done","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e2f70c4104d94cd7d3dfbecdbb0eefca9b87c47a","unresolved":true,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise ValueError"},{"line_number":3062,"context_line":""},{"line_number":3063,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3064,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":4,"id":"38dcf4ad_167fced6","line":3061,"in_reply_to":"af2d50ff_3df19838","updated":"2023-03-20 19:53:49.000000000","message":"+1 we should set a message in the value error","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"ce67dc7c6b5f6ed22a7b171b99697f9e10d3c9a9","unresolved":true,"context_lines":[{"line_number":3196,"context_line":"        ) as e:"},{"line_number":3197,"context_line":"            print(str(e))"},{"line_number":3198,"context_line":"            return 4"},{"line_number":3199,"context_line":"        except (ValueError, OSError):"},{"line_number":3200,"context_line":"            try:"},{"line_number":3201,"context_line":"                print("},{"line_number":3202,"context_line":"                    f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the connector \""}],"source_content_type":"text/x-python","patch_set":4,"id":"1740a5e5_a81b503f","line":3199,"updated":"2023-03-20 19:31:17.000000000","message":"Ah, I see why you did it this way. So if you do what I suggested above, here you would need to split ValueError and OSError, and something like do:\n\n  except ValueError as e:\n    print(e)\n  except OSError:\n    etc","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"055bbc1baf355f633d3b83ab0e26f8ab24892a74","unresolved":true,"context_lines":[{"line_number":3196,"context_line":"        ) as e:"},{"line_number":3197,"context_line":"            print(str(e))"},{"line_number":3198,"context_line":"            return 4"},{"line_number":3199,"context_line":"        except (ValueError, OSError):"},{"line_number":3200,"context_line":"            try:"},{"line_number":3201,"context_line":"                print("},{"line_number":3202,"context_line":"                    f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the connector \""}],"source_content_type":"text/x-python","patch_set":4,"id":"292bb44e_88a3d404","line":3199,"in_reply_to":"1740a5e5_a81b503f","updated":"2023-03-21 11:22:00.000000000","message":"yes, also I did like this because, (one) here ValueError can be raised from 2 places, when we are reading connector file and when we are removing-volume-connection, (second) I didn\u0027t want make lot of change in try-block structure (also as it was returning the same error code).\n\n try:\n   read_connector_file\n except (ValueError, OSError):\n   print()\n   return 3\n   \nI have moved above (reading) in first try-block and returned the status code from their itself, as we must need connector for refresh, and latter except only ValueError and again return 3, which might come while removing-volume-connection.","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"edfac689eac398c03ef535c0dfba55b0d14c3500","unresolved":false,"context_lines":[{"line_number":3196,"context_line":"        ) as e:"},{"line_number":3197,"context_line":"            print(str(e))"},{"line_number":3198,"context_line":"            return 4"},{"line_number":3199,"context_line":"        except (ValueError, OSError):"},{"line_number":3200,"context_line":"            try:"},{"line_number":3201,"context_line":"                print("},{"line_number":3202,"context_line":"                    f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the connector \""}],"source_content_type":"text/x-python","patch_set":4,"id":"29f60804_aa39a7f3","line":3199,"in_reply_to":"292bb44e_88a3d404","updated":"2023-06-21 14:15:06.000000000","message":"Done","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e2f70c4104d94cd7d3dfbecdbb0eefca9b87c47a","unresolved":true,"context_lines":[{"line_number":3204,"context_line":"                    f\"The cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3205,"context_line":"                    f\"may have been run on the wrong compute host. Or the \""},{"line_number":3206,"context_line":"                    f\"instance host may be wrong and in need of repair.\")"},{"line_number":3207,"context_line":"            except NameError:"},{"line_number":3208,"context_line":"                print("},{"line_number":3209,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3210,"context_line":"                    f\u0027connector_info data?\u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"7e88f1e0_22d7bac4","line":3207,"updated":"2023-03-20 19:53:49.000000000","message":"we should never need to wrap a print in a try except like this we shoudl not be getting a NameError here.","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"055bbc1baf355f633d3b83ab0e26f8ab24892a74","unresolved":false,"context_lines":[{"line_number":3204,"context_line":"                    f\"The cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3205,"context_line":"                    f\"may have been run on the wrong compute host. Or the \""},{"line_number":3206,"context_line":"                    f\"instance host may be wrong and in need of repair.\")"},{"line_number":3207,"context_line":"            except NameError:"},{"line_number":3208,"context_line":"                print("},{"line_number":3209,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3210,"context_line":"                    f\u0027connector_info data?\u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"879fb2ea_30cf16a2","line":3207,"in_reply_to":"7e88f1e0_22d7bac4","updated":"2023-03-21 11:22:00.000000000","message":"Ack","commit_id":"22d1a15f15fc0107b050c9cd425f9a3a8669f438"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"89f55d9f65a1485d0e74f3aa229323e538d9a302","unresolved":true,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise ValueError("},{"line_number":3062,"context_line":"                        f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the \""},{"line_number":3063,"context_line":"                        f\"connector does not match the instance host \""},{"line_number":3064,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""},{"line_number":3065,"context_line":"                        f\"cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3066,"context_line":"                        f\"may have been run on the wrong compute host. Or the \""},{"line_number":3067,"context_line":"                        f\"instance host may be wrong and in need of repair.\")"},{"line_number":3068,"context_line":""},{"line_number":3069,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3070,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":5,"id":"b3802e72_4547bff3","line":3067,"range":{"start_line":3061,"start_character":0,"end_line":3067,"end_character":77},"updated":"2023-03-28 07:37:24.000000000","message":"The problem I see is that we don\u0027t have a \"clean\" way to fix this situation.\nThis tool is meant to help us fix things, but will fail and tell us to fix things ourselves.\nThis means that we\u0027ll have to manually disconnect the volume on the host, call cinder ourselves, and edit the nova DB records, right?\nAnd that\u0027s probably not what we want sysadmins to actually do.","commit_id":"e2264355a18fb0315b05d365b6f83262dffda12f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"34286f71a69233157bd24da5ad442da4d45f5e9b","unresolved":false,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise ValueError("},{"line_number":3062,"context_line":"                        f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the \""},{"line_number":3063,"context_line":"                        f\"connector does not match the instance host \""},{"line_number":3064,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""},{"line_number":3065,"context_line":"                        f\"cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3066,"context_line":"                        f\"may have been run on the wrong compute host. Or the \""},{"line_number":3067,"context_line":"                        f\"instance host may be wrong and in need of repair.\")"},{"line_number":3068,"context_line":""},{"line_number":3069,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3070,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":5,"id":"660a2363_5b4d49dd","line":3067,"range":{"start_line":3061,"start_character":0,"end_line":3067,"end_character":77},"in_reply_to":"653cf31b_4a113e1f","updated":"2023-07-10 15:19:22.000000000","message":"Done","commit_id":"e2264355a18fb0315b05d365b6f83262dffda12f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4d6ec14284da0dc3b857863fd02697dfbf1ba7c2","unresolved":true,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise ValueError("},{"line_number":3062,"context_line":"                        f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the \""},{"line_number":3063,"context_line":"                        f\"connector does not match the instance host \""},{"line_number":3064,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""},{"line_number":3065,"context_line":"                        f\"cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3066,"context_line":"                        f\"may have been run on the wrong compute host. Or the \""},{"line_number":3067,"context_line":"                        f\"instance host may be wrong and in need of repair.\")"},{"line_number":3068,"context_line":""},{"line_number":3069,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3070,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":5,"id":"c749d1da_faaab913","line":3067,"range":{"start_line":3061,"start_character":0,"end_line":3067,"end_character":77},"in_reply_to":"b3802e72_4547bff3","updated":"2023-03-28 10:45:28.000000000","message":"no it means you use the wrong input file\n\nthis if is there to detect if you use a connector file that was created for a host that does not  match the host of the vm","commit_id":"e2264355a18fb0315b05d365b6f83262dffda12f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"903f631bfde676a30cc29be4a319143eba6e67d7","unresolved":true,"context_lines":[{"line_number":3058,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3059,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3060,"context_line":"                else:"},{"line_number":3061,"context_line":"                    raise ValueError("},{"line_number":3062,"context_line":"                        f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the \""},{"line_number":3063,"context_line":"                        f\"connector does not match the instance host \""},{"line_number":3064,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""},{"line_number":3065,"context_line":"                        f\"cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3066,"context_line":"                        f\"may have been run on the wrong compute host. Or the \""},{"line_number":3067,"context_line":"                        f\"instance host may be wrong and in need of repair.\")"},{"line_number":3068,"context_line":""},{"line_number":3069,"context_line":"                # Delete the existing volume attachment if present in the bdm."},{"line_number":3070,"context_line":"                # This isn\u0027t present when the original attachment was made"}],"source_content_type":"text/x-python","patch_set":5,"id":"653cf31b_4a113e1f","line":3067,"range":{"start_line":3061,"start_character":0,"end_line":3067,"end_character":77},"in_reply_to":"c749d1da_faaab913","updated":"2023-03-28 11:48:41.000000000","message":"if it the later case \n\"instance host may be wrong and in need of repair\"\n\nwe do not want this command to fix that. that needs to be fixed possibelve via the db beofre you try to repair the instance.\n\nwe want to hard fail in this case as that means you have db currption and you shoudl stop making changes until that is resolved.","commit_id":"e2264355a18fb0315b05d365b6f83262dffda12f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"4a798b3bd734aa7803e44886cb4c4384ed6d032a","unresolved":true,"context_lines":[{"line_number":3158,"context_line":"            try:"},{"line_number":3159,"context_line":"                with open(connector_path, \u0027rb\u0027) as connector_file:"},{"line_number":3160,"context_line":"                    connector \u003d jsonutils.load(connector_file)"},{"line_number":3161,"context_line":"            except (ValueError, OSError):"},{"line_number":3162,"context_line":"                print("},{"line_number":3163,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3164,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"36b6326e_b485c4d8","line":3161,"updated":"2023-03-21 14:35:11.000000000","message":"Ah I see, you catch them here to print the correct error message, then catch the ValueError that _refresh() can raise further down. I think this works.","commit_id":"e2264355a18fb0315b05d365b6f83262dffda12f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"e9ce6f4e8f4ee1280d4a4a59b59f3cc8fd0abc85","unresolved":false,"context_lines":[{"line_number":3158,"context_line":"            try:"},{"line_number":3159,"context_line":"                with open(connector_path, \u0027rb\u0027) as connector_file:"},{"line_number":3160,"context_line":"                    connector \u003d jsonutils.load(connector_file)"},{"line_number":3161,"context_line":"            except (ValueError, OSError):"},{"line_number":3162,"context_line":"                print("},{"line_number":3163,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3164,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"112aa3a0_5beda04b","line":3161,"in_reply_to":"36b6326e_b485c4d8","updated":"2023-06-21 14:15:35.000000000","message":"Done","commit_id":"e2264355a18fb0315b05d365b6f83262dffda12f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"492076ce1cb0c3c96513ecf81fff835553a95d6c","unresolved":true,"context_lines":[{"line_number":3188,"context_line":"            try:"},{"line_number":3189,"context_line":"                with open(connector_path, \u0027rb\u0027) as connector_file:"},{"line_number":3190,"context_line":"                    connector \u003d jsonutils.load(connector_file)"},{"line_number":3191,"context_line":"            except (ValueError, OSError):"},{"line_number":3192,"context_line":"                print("},{"line_number":3193,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3194,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"f8d6c981_60492763","line":3191,"updated":"2023-07-10 17:40:23.000000000","message":"Why did you move this up here? IMHO, nesting exception handlers like this should be avoided unless it\u0027s really necessary and I don\u0027t think it is here. I think re-using `ValueError` for the host mismatch is not only the wrong exception, but also muddies your handling here. I say use something else for that and then it will be easier to make sure the right thing gets printed to the user.","commit_id":"ba7dd04e0cd95a69a902e9be129ac0980ce8e6aa"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"25cb74b04de6ac9c3b2e59e74d062c9b462c382b","unresolved":true,"context_lines":[{"line_number":3188,"context_line":"            try:"},{"line_number":3189,"context_line":"                with open(connector_path, \u0027rb\u0027) as connector_file:"},{"line_number":3190,"context_line":"                    connector \u003d jsonutils.load(connector_file)"},{"line_number":3191,"context_line":"            except (ValueError, OSError):"},{"line_number":3192,"context_line":"                print("},{"line_number":3193,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3194,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f194ba6_87024666","line":3191,"in_reply_to":"0bf9cc6d_6715587a","updated":"2023-07-11 15:04:23.000000000","message":"`regarding nesting exception handling:`\n-------------------------------------\nit\u0027s because of 2 different messages for the same type of Exception - ValueError.\nFirst one we can not change as if connector file content is not in correct format jsonutils.load only throws ValueError.\n\nMoved reading connector_file outside try-catch block as a function.\n\n\n`regarding using ValueError for host check:`\n----------------------------\nin addition to Melanie\u0027s suggestion, if need to add another exception, we can call it `InvalidConnectorError`.\nalso, we have one `InvalidNodeConfiguration: Invalid node identity configuration`. it\u0027s mostly used with the virt driver","commit_id":"ba7dd04e0cd95a69a902e9be129ac0980ce8e6aa"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a8a923bf726aef7ad71d58d4ae6eeb9c6a9305d7","unresolved":false,"context_lines":[{"line_number":3188,"context_line":"            try:"},{"line_number":3189,"context_line":"                with open(connector_path, \u0027rb\u0027) as connector_file:"},{"line_number":3190,"context_line":"                    connector \u003d jsonutils.load(connector_file)"},{"line_number":3191,"context_line":"            except (ValueError, OSError):"},{"line_number":3192,"context_line":"                print("},{"line_number":3193,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3194,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"f88fdad1_c8bae86e","line":3191,"in_reply_to":"5f194ba6_87024666","updated":"2023-07-13 11:39:33.000000000","message":"Done","commit_id":"ba7dd04e0cd95a69a902e9be129ac0980ce8e6aa"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"935f1f2d95a0730d249018d46d9e3bb9ed66a805","unresolved":true,"context_lines":[{"line_number":3188,"context_line":"            try:"},{"line_number":3189,"context_line":"                with open(connector_path, \u0027rb\u0027) as connector_file:"},{"line_number":3190,"context_line":"                    connector \u003d jsonutils.load(connector_file)"},{"line_number":3191,"context_line":"            except (ValueError, OSError):"},{"line_number":3192,"context_line":"                print("},{"line_number":3193,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3194,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"0bf9cc6d_6715587a","line":3191,"in_reply_to":"897af73c_d37d27b2","updated":"2023-07-10 18:13:02.000000000","message":"Yeah I mean I can see that, but it seems like a stretch to use it for this case to me. Here\u0027s the explanation of this exception:\n\nhttps://docs.python.org/3/library/exceptions.html#ValueError\n\nWhich basically describes a situation where you pass `int(\u0027a\u0027)` - it expects a string, but you gave it one where not all the digits are numbers. In this case, we\u0027re passing a connector blob, which we got from another service, and we\u0027re comparing something in there to something in our database. It\u0027s not a \"you passed me the wrong sort of thing\" or \"that value isn\u0027t acceptable\" sort of situation, because both values were clearly acceptable at some point in time.\n\nEither way, to answer artom\u0027s thing, I think barfing out the very confusing (IMHO) JSON parser error would be a substantially less helpful response, and it\u0027s also a regression in usability from what we have now.\n\nAnyway, it\u0027s not a huge deal, but choosing the questionably-correct ValueError for this makes this uglier and a bit harder to eye-parse IMHO.","commit_id":"ba7dd04e0cd95a69a902e9be129ac0980ce8e6aa"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3e36bb60de5e030a76ca952dcd66cb8de269d16c","unresolved":true,"context_lines":[{"line_number":3188,"context_line":"            try:"},{"line_number":3189,"context_line":"                with open(connector_path, \u0027rb\u0027) as connector_file:"},{"line_number":3190,"context_line":"                    connector \u003d jsonutils.load(connector_file)"},{"line_number":3191,"context_line":"            except (ValueError, OSError):"},{"line_number":3192,"context_line":"                print("},{"line_number":3193,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3194,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"897af73c_d37d27b2","line":3191,"in_reply_to":"897cc138_e15a71ac","updated":"2023-07-10 17:54:34.000000000","message":"(started writing my comment ^ before Artom\u0027s comment landed 😄)","commit_id":"ba7dd04e0cd95a69a902e9be129ac0980ce8e6aa"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"28999855b16780192157eecb5a07f4e3a582074a","unresolved":true,"context_lines":[{"line_number":3188,"context_line":"            try:"},{"line_number":3189,"context_line":"                with open(connector_path, \u0027rb\u0027) as connector_file:"},{"line_number":3190,"context_line":"                    connector \u003d jsonutils.load(connector_file)"},{"line_number":3191,"context_line":"            except (ValueError, OSError):"},{"line_number":3192,"context_line":"                print("},{"line_number":3193,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3194,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"ec9f1418_169ac31b","line":3191,"in_reply_to":"f8d6c981_60492763","updated":"2023-07-10 17:49:43.000000000","message":"So Mel and I suggested ValueError, because it does fit the use case: the value of the host is wrong (presumably because the `get_connector` subcommand was ran on the wrong compute host).\n\nI agree that it does muddy the handling though - you could get a ValueError from malformed JSON, or from a host mismatch.\n\nWhy can\u0027t we just catch all ValueErrors and just print(str(e))? Would it be so bad if the details of the JSON malformation are made visible to the user?","commit_id":"ba7dd04e0cd95a69a902e9be129ac0980ce8e6aa"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d005dc80993486713190ff613a0284b4c60124dc","unresolved":true,"context_lines":[{"line_number":3188,"context_line":"            try:"},{"line_number":3189,"context_line":"                with open(connector_path, \u0027rb\u0027) as connector_file:"},{"line_number":3190,"context_line":"                    connector \u003d jsonutils.load(connector_file)"},{"line_number":3191,"context_line":"            except (ValueError, OSError):"},{"line_number":3192,"context_line":"                print("},{"line_number":3193,"context_line":"                    f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3194,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"897cc138_e15a71ac","line":3191,"in_reply_to":"f8d6c981_60492763","updated":"2023-07-10 17:52:18.000000000","message":"We had suggested using `ValueError` on PS3 bc it seemed consistent with how other nova-manage commands are using it.\n\nNone of the existing nova/exception.py seem to fit, so maybe we should add a new one as part of this patch, like HostMismatch or HostConflict or something?","commit_id":"ba7dd04e0cd95a69a902e9be129ac0980ce8e6aa"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"67bb41827f6b40b2a1edc736bb8d19adf93dfa32","unresolved":true,"context_lines":[{"line_number":3103,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3104,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3105,"context_line":"                else:"},{"line_number":3106,"context_line":"                    raise ValueError("},{"line_number":3107,"context_line":"                        f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the \""},{"line_number":3108,"context_line":"                        f\"connector does not match the instance host \""},{"line_number":3109,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""}],"source_content_type":"text/x-python","patch_set":7,"id":"16f7cd47_d2c8aef9","line":3106,"updated":"2023-07-12 13:45:32.000000000","message":"I still think this is wrong, even though it\u0027s visually less bad the way you have it here. Even `RuntimeError` would be better to reflect some condition that is not met.\n\nAdding `InvalidConnectorError` to the top of this file would be a few lines of change to make this much cleaner, IMHO. It doesn\u0027t need to be in `nova.exception` or even inherit from `NovaException`.","commit_id":"ce96d40f0c9ed84404e0ce9cdd13dadbe741207e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a8a923bf726aef7ad71d58d4ae6eeb9c6a9305d7","unresolved":false,"context_lines":[{"line_number":3103,"context_line":"                    compute_rpcapi.remove_volume_connection("},{"line_number":3104,"context_line":"                        cctxt, instance, volume_id, instance.host)"},{"line_number":3105,"context_line":"                else:"},{"line_number":3106,"context_line":"                    raise ValueError("},{"line_number":3107,"context_line":"                        f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the \""},{"line_number":3108,"context_line":"                        f\"connector does not match the instance host \""},{"line_number":3109,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""}],"source_content_type":"text/x-python","patch_set":7,"id":"3770482b_fd1d9669","line":3106,"in_reply_to":"16f7cd47_d2c8aef9","updated":"2023-07-13 11:39:33.000000000","message":"Done","commit_id":"ce96d40f0c9ed84404e0ce9cdd13dadbe741207e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"25cb74b04de6ac9c3b2e59e74d062c9b462c382b","unresolved":true,"context_lines":[{"line_number":3198,"context_line":"            try:"},{"line_number":3199,"context_line":"                with open(json_file_path, \u0027rb\u0027) as connector_file:"},{"line_number":3200,"context_line":"                    return jsonutils.load(connector_file)"},{"line_number":3201,"context_line":"            except (ValueError, OSError):"},{"line_number":3202,"context_line":"                raise ValueError("},{"line_number":3203,"context_line":"                    f\u0027Failed to open {json_file_path}. Does it contain valid \u0027"},{"line_number":3204,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":7,"id":"c91f3b1c_5c7c1ca9","line":3201,"range":{"start_line":3201,"start_character":32,"end_line":3201,"end_character":39},"updated":"2023-07-11 15:04:23.000000000","message":"I think OSError is not really required. it only raised if given file path is not correct, which is already handled by InvalidInput","commit_id":"ce96d40f0c9ed84404e0ce9cdd13dadbe741207e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a8a923bf726aef7ad71d58d4ae6eeb9c6a9305d7","unresolved":false,"context_lines":[{"line_number":3198,"context_line":"            try:"},{"line_number":3199,"context_line":"                with open(json_file_path, \u0027rb\u0027) as connector_file:"},{"line_number":3200,"context_line":"                    return jsonutils.load(connector_file)"},{"line_number":3201,"context_line":"            except (ValueError, OSError):"},{"line_number":3202,"context_line":"                raise ValueError("},{"line_number":3203,"context_line":"                    f\u0027Failed to open {json_file_path}. Does it contain valid \u0027"},{"line_number":3204,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":7,"id":"a2ffe1ac_e9cc6c68","line":3201,"range":{"start_line":3201,"start_character":32,"end_line":3201,"end_character":39},"in_reply_to":"0430dc43_2dfbe66f","updated":"2023-07-13 11:39:33.000000000","message":"reverted","commit_id":"ce96d40f0c9ed84404e0ce9cdd13dadbe741207e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"67bb41827f6b40b2a1edc736bb8d19adf93dfa32","unresolved":true,"context_lines":[{"line_number":3198,"context_line":"            try:"},{"line_number":3199,"context_line":"                with open(json_file_path, \u0027rb\u0027) as connector_file:"},{"line_number":3200,"context_line":"                    return jsonutils.load(connector_file)"},{"line_number":3201,"context_line":"            except (ValueError, OSError):"},{"line_number":3202,"context_line":"                raise ValueError("},{"line_number":3203,"context_line":"                    f\u0027Failed to open {json_file_path}. Does it contain valid \u0027"},{"line_number":3204,"context_line":"                    f\u0027connector_info data?\u0027)"}],"source_content_type":"text/x-python","patch_set":7,"id":"0430dc43_2dfbe66f","line":3201,"range":{"start_line":3201,"start_character":32,"end_line":3201,"end_character":39},"in_reply_to":"c91f3b1c_5c7c1ca9","updated":"2023-07-12 13:45:32.000000000","message":"IMHO, converting OSError to ValueError is very much not correct anyway.","commit_id":"ce96d40f0c9ed84404e0ce9cdd13dadbe741207e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d04c4d33373aa9885b0ba28f057e85a4fba44ab7","unresolved":true,"context_lines":[{"line_number":3213,"context_line":"            print("},{"line_number":3214,"context_line":"                f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3215,"context_line":"                f\u0027connector_info data?\u0027"},{"line_number":3216,"context_line":"            )"},{"line_number":3217,"context_line":"            return 3"},{"line_number":3218,"context_line":"        except exception.InvalidInput as e:"},{"line_number":3219,"context_line":"            print(str(e))"}],"source_content_type":"text/x-python","patch_set":8,"id":"8e32eacb_412fcfe6","side":"PARENT","line":3216,"updated":"2023-07-12 18:04:27.000000000","message":"By doing this you are discarding the handling of OSError which was there and IMHO this code should be left as it is. I think you just need to add handling of HostConflict which is unrelated to the file opening right?","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"de7e8be8ffc0d3a2ee3bcffd878fc179ea8e96db","unresolved":false,"context_lines":[{"line_number":3213,"context_line":"            print("},{"line_number":3214,"context_line":"                f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3215,"context_line":"                f\u0027connector_info data?\u0027"},{"line_number":3216,"context_line":"            )"},{"line_number":3217,"context_line":"            return 3"},{"line_number":3218,"context_line":"        except exception.InvalidInput as e:"},{"line_number":3219,"context_line":"            print(str(e))"}],"source_content_type":"text/x-python","patch_set":8,"id":"37f13dce_50e24309","side":"PARENT","line":3216,"in_reply_to":"723c26db_28ecbfba","updated":"2023-07-13 15:40:30.000000000","message":"Done","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a8a923bf726aef7ad71d58d4ae6eeb9c6a9305d7","unresolved":true,"context_lines":[{"line_number":3213,"context_line":"            print("},{"line_number":3214,"context_line":"                f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3215,"context_line":"                f\u0027connector_info data?\u0027"},{"line_number":3216,"context_line":"            )"},{"line_number":3217,"context_line":"            return 3"},{"line_number":3218,"context_line":"        except exception.InvalidInput as e:"},{"line_number":3219,"context_line":"            print(str(e))"}],"source_content_type":"text/x-python","patch_set":8,"id":"723c26db_28ecbfba","side":"PARENT","line":3216,"in_reply_to":"8e32eacb_412fcfe6","updated":"2023-07-13 11:39:33.000000000","message":"Reason of removing OSError here: OSError is only raised by jsonutils.load if file path is not correct or not able to read because of permission (which is unlikely), otherwise it raises ValueError.\nalso in this block, I believe only thing related to OS/IO we are doing is reading connector file, apart from that all other are cinder DB ops.\nVerifying file path is already done with `os.path.exists(connector_path)` which handled as InvalidInput exception, so I think OSError may not be raised at all. please do suggest on this.\n\nyes, added handling for HostConflict.","commit_id":"9211a9557c9636e1f9f505bd61cc80bc9e65de90"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d04c4d33373aa9885b0ba28f057e85a4fba44ab7","unresolved":true,"context_lines":[{"line_number":180,"context_line":"            self.message \u003d self.msg_fmt % kwargs"},{"line_number":181,"context_line":"        else:"},{"line_number":182,"context_line":"            self.message \u003d str(message)"},{"line_number":183,"context_line":"        super(HostConflict, self).__init__(message)"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"class DbCommands(object):"}],"source_content_type":"text/x-python","patch_set":8,"id":"2f6fe13a_f075d12e","line":183,"updated":"2023-07-12 18:04:27.000000000","message":"FWIW as Dan mentioned on IRC, you don\u0027t need to do all this, it can just be:\n```\nclass HostConflict(Exception):\n    pass\n```\nor\n```\nclass InvalidConnectorError(Exception):\n    pass\n```\n\nand then when you raise it it\u0027s just ```raise HostConflict(message)``` or `raise InvalidConnectorError(message)`","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a8a923bf726aef7ad71d58d4ae6eeb9c6a9305d7","unresolved":false,"context_lines":[{"line_number":180,"context_line":"            self.message \u003d self.msg_fmt % kwargs"},{"line_number":181,"context_line":"        else:"},{"line_number":182,"context_line":"            self.message \u003d str(message)"},{"line_number":183,"context_line":"        super(HostConflict, self).__init__(message)"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"class DbCommands(object):"}],"source_content_type":"text/x-python","patch_set":8,"id":"b98e3b59_612541ef","line":183,"in_reply_to":"2f6fe13a_f075d12e","updated":"2023-07-13 11:39:33.000000000","message":"yes.\nthe reason was of doing like is to have a default message.\nbut sure we do not need to add that because right now its being used in only one place and there we are passing msg.\nIf in case its used in future, then also a custom msg should be passed.\n\nupdated, thanks.","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d04c4d33373aa9885b0ba28f057e85a4fba44ab7","unresolved":true,"context_lines":[{"line_number":3119,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""},{"line_number":3120,"context_line":"                        f\"cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3121,"context_line":"                        f\"may have been run on the wrong compute host. Or the \""},{"line_number":3122,"context_line":"                        f\"instance host may be wrong and in need of repair.\")"},{"line_number":3123,"context_line":"                    raise HostConflict(message\u003dmsg)"},{"line_number":3124,"context_line":""},{"line_number":3125,"context_line":"                # Delete the existing volume attachment if present in the bdm."}],"source_content_type":"text/x-python","patch_set":8,"id":"8710d41c_e61f98ae","line":3122,"updated":"2023-07-12 18:04:27.000000000","message":"I\u0027m not 100% sure this is still being done but if you wrap the message string in `_()` it will translate the string for other languages for end users. There are a lot of places in this file where it\u0027s used, for reference.\nExample:\n`\nmsg \u003d _(\u0027text string\u0027)\n`","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a8a923bf726aef7ad71d58d4ae6eeb9c6a9305d7","unresolved":false,"context_lines":[{"line_number":3119,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""},{"line_number":3120,"context_line":"                        f\"cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3121,"context_line":"                        f\"may have been run on the wrong compute host. Or the \""},{"line_number":3122,"context_line":"                        f\"instance host may be wrong and in need of repair.\")"},{"line_number":3123,"context_line":"                    raise HostConflict(message\u003dmsg)"},{"line_number":3124,"context_line":""},{"line_number":3125,"context_line":"                # Delete the existing volume attachment if present in the bdm."}],"source_content_type":"text/x-python","patch_set":8,"id":"47400d0f_99808f90","line":3122,"in_reply_to":"8710d41c_e61f98ae","updated":"2023-07-13 11:39:33.000000000","message":"Done.\nadding like _(\"the {connector} sting\") failed because it was not able to recognise the connector and instance (other variables) object.\nso first formed the string and then converted/translated it.","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d04c4d33373aa9885b0ba28f057e85a4fba44ab7","unresolved":true,"context_lines":[{"line_number":3198,"context_line":"        * 0: Command completed successfully."},{"line_number":3199,"context_line":"        * 1: An unexpected error happened."},{"line_number":3200,"context_line":"        * 2: Connector path does not exist."},{"line_number":3201,"context_line":"        * 3: Failed to open connector path."},{"line_number":3202,"context_line":"        * 4: Instance does not exist."},{"line_number":3203,"context_line":"        * 5: Instance state invalid."},{"line_number":3204,"context_line":"        * 6: Volume is not attached to instance."}],"source_content_type":"text/x-python","patch_set":8,"id":"94ac9364_409b5bca","line":3201,"updated":"2023-07-12 18:04:27.000000000","message":"What you currently have would return 3 which is supposed to mean a failure to open the connector path, which isn\u0027t really accurate and could mislead the end user.\n\nAFAICT I think we need to add a new return code 7 to represent the host conflict and add it to the docstring here.","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a8a923bf726aef7ad71d58d4ae6eeb9c6a9305d7","unresolved":false,"context_lines":[{"line_number":3198,"context_line":"        * 0: Command completed successfully."},{"line_number":3199,"context_line":"        * 1: An unexpected error happened."},{"line_number":3200,"context_line":"        * 2: Connector path does not exist."},{"line_number":3201,"context_line":"        * 3: Failed to open connector path."},{"line_number":3202,"context_line":"        * 4: Instance does not exist."},{"line_number":3203,"context_line":"        * 5: Instance state invalid."},{"line_number":3204,"context_line":"        * 6: Volume is not attached to instance."}],"source_content_type":"text/x-python","patch_set":8,"id":"059cc475_6c56cd3c","line":3201,"in_reply_to":"94ac9364_409b5bca","updated":"2023-07-13 11:39:33.000000000","message":"Done","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d04c4d33373aa9885b0ba28f057e85a4fba44ab7","unresolved":true,"context_lines":[{"line_number":3213,"context_line":"                raise ValueError("},{"line_number":3214,"context_line":"                    f\u0027Failed to open {json_file_path}. Does it contain valid \u0027"},{"line_number":3215,"context_line":"                    f\u0027connector_info data?\u0027)"},{"line_number":3216,"context_line":""},{"line_number":3217,"context_line":"        try:"},{"line_number":3218,"context_line":"            # TODO(lyarwood): Make this optional and provide a rpcapi capable"},{"line_number":3219,"context_line":"            # of pulling this down from the target compute during this flow."}],"source_content_type":"text/x-python","patch_set":8,"id":"2803a2e3_55ea991d","line":3216,"updated":"2023-07-12 18:04:27.000000000","message":"Same here, IMHO I don\u0027t think this change is improving anything and could be left as it was without a new helper functon.","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a8a923bf726aef7ad71d58d4ae6eeb9c6a9305d7","unresolved":true,"context_lines":[{"line_number":3213,"context_line":"                raise ValueError("},{"line_number":3214,"context_line":"                    f\u0027Failed to open {json_file_path}. Does it contain valid \u0027"},{"line_number":3215,"context_line":"                    f\u0027connector_info data?\u0027)"},{"line_number":3216,"context_line":""},{"line_number":3217,"context_line":"        try:"},{"line_number":3218,"context_line":"            # TODO(lyarwood): Make this optional and provide a rpcapi capable"},{"line_number":3219,"context_line":"            # of pulling this down from the target compute during this flow."}],"source_content_type":"text/x-python","patch_set":8,"id":"43af48f0_e7ced48d","line":3216,"in_reply_to":"2803a2e3_55ea991d","updated":"2023-07-13 11:39:33.000000000","message":"this is to remove nesting exception handling, only imprvement may be its somwhat more readble.","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4f644860b07f2832b8836bf0cdf0a2859dda1323","unresolved":true,"context_lines":[{"line_number":3213,"context_line":"                raise ValueError("},{"line_number":3214,"context_line":"                    f\u0027Failed to open {json_file_path}. Does it contain valid \u0027"},{"line_number":3215,"context_line":"                    f\u0027connector_info data?\u0027)"},{"line_number":3216,"context_line":""},{"line_number":3217,"context_line":"        try:"},{"line_number":3218,"context_line":"            # TODO(lyarwood): Make this optional and provide a rpcapi capable"},{"line_number":3219,"context_line":"            # of pulling this down from the target compute during this flow."}],"source_content_type":"text/x-python","patch_set":8,"id":"60a50ccb_3256abeb","line":3216,"in_reply_to":"43af48f0_e7ced48d","updated":"2023-07-13 14:23:09.000000000","message":"I don\u0027t think you\u0027ll have the nested handling anymore now that you have specific exceptions for each of the cases. I agree with melwitt that this is less readable than it could be. It\u0027s also weird to catch OSError and re-raise with a different message. IMHO, you should do that in your exception handler where you catch, print, and return an error code.","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"de7e8be8ffc0d3a2ee3bcffd878fc179ea8e96db","unresolved":false,"context_lines":[{"line_number":3213,"context_line":"                raise ValueError("},{"line_number":3214,"context_line":"                    f\u0027Failed to open {json_file_path}. Does it contain valid \u0027"},{"line_number":3215,"context_line":"                    f\u0027connector_info data?\u0027)"},{"line_number":3216,"context_line":""},{"line_number":3217,"context_line":"        try:"},{"line_number":3218,"context_line":"            # TODO(lyarwood): Make this optional and provide a rpcapi capable"},{"line_number":3219,"context_line":"            # of pulling this down from the target compute during this flow."}],"source_content_type":"text/x-python","patch_set":8,"id":"a0fcbd9d_81ee2f00","line":3216,"in_reply_to":"60a50ccb_3256abeb","updated":"2023-07-13 15:40:30.000000000","message":"oh yes, because now we do not have 2 ValueError exception as we added HostConflict, so it can go back to as it is.","commit_id":"fa40a53bd45f3019c7dcd95a556ce9d67f6addda"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4f644860b07f2832b8836bf0cdf0a2859dda1323","unresolved":true,"context_lines":[{"line_number":3115,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""},{"line_number":3116,"context_line":"                        f\"cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3117,"context_line":"                        f\"may have been run on the wrong compute host. Or the \""},{"line_number":3118,"context_line":"                        f\"instance host may be wrong and in need of repair.\")"},{"line_number":3119,"context_line":"                    raise HostConflict(_(msg))"},{"line_number":3120,"context_line":""},{"line_number":3121,"context_line":"                # Delete the existing volume attachment if present in the bdm."}],"source_content_type":"text/x-python","patch_set":9,"id":"2182cf29_828af81a","line":3118,"updated":"2023-07-13 14:23:09.000000000","message":"This alignment is wrong. I also don\u0027t think it\u0027s nice to have (os-specific) newlines in an exception message. IMHO, you should keep this to just the data you need here (i.e. everything up to the newline) and then format it in the print statement where you handle it.","commit_id":"fd4f2307850d90f36d7018b4a70e18424d7f89f8"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"de7e8be8ffc0d3a2ee3bcffd878fc179ea8e96db","unresolved":false,"context_lines":[{"line_number":3115,"context_line":"                        f\"\u0027{instance.host}\u0027.\\nThe \""},{"line_number":3116,"context_line":"                        f\"cmd \u0027nova-manage volume_attachment get_connector\u0027 \""},{"line_number":3117,"context_line":"                        f\"may have been run on the wrong compute host. Or the \""},{"line_number":3118,"context_line":"                        f\"instance host may be wrong and in need of repair.\")"},{"line_number":3119,"context_line":"                    raise HostConflict(_(msg))"},{"line_number":3120,"context_line":""},{"line_number":3121,"context_line":"                # Delete the existing volume attachment if present in the bdm."}],"source_content_type":"text/x-python","patch_set":9,"id":"50dcfe0d_863dcf84","line":3118,"in_reply_to":"2182cf29_828af81a","updated":"2023-07-13 15:40:30.000000000","message":"Done","commit_id":"fd4f2307850d90f36d7018b4a70e18424d7f89f8"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4f644860b07f2832b8836bf0cdf0a2859dda1323","unresolved":true,"context_lines":[{"line_number":3239,"context_line":"        ) as e:"},{"line_number":3240,"context_line":"            print(str(e))"},{"line_number":3241,"context_line":"            return 4"},{"line_number":3242,"context_line":"        except (ValueError, OSError) as e:"},{"line_number":3243,"context_line":"            print(e)"},{"line_number":3244,"context_line":"            return 3"},{"line_number":3245,"context_line":"        except exception.InvalidInput as e:"}],"source_content_type":"text/x-python","patch_set":9,"id":"78731448_64463fd2","line":3242,"updated":"2023-07-13 14:23:09.000000000","message":"I know this isn\u0027t your fault, but this is the case where the file *is* open-able but it does not contain a valid connector, right? According to the list above, exit code 3 *only* means that the file couldn\u0027t be opened. Or if 3 means it couldn\u0027t be *parsed* then 2 is more accurate for the OSError case I think.","commit_id":"fd4f2307850d90f36d7018b4a70e18424d7f89f8"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"de7e8be8ffc0d3a2ee3bcffd878fc179ea8e96db","unresolved":false,"context_lines":[{"line_number":3239,"context_line":"        ) as e:"},{"line_number":3240,"context_line":"            print(str(e))"},{"line_number":3241,"context_line":"            return 4"},{"line_number":3242,"context_line":"        except (ValueError, OSError) as e:"},{"line_number":3243,"context_line":"            print(e)"},{"line_number":3244,"context_line":"            return 3"},{"line_number":3245,"context_line":"        except exception.InvalidInput as e:"}],"source_content_type":"text/x-python","patch_set":9,"id":"f3bf2319_08954c9f","line":3242,"in_reply_to":"78731448_64463fd2","updated":"2023-07-13 15:40:30.000000000","message":"Done","commit_id":"fd4f2307850d90f36d7018b4a70e18424d7f89f8"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6108b1e0cc6d4e3dcf5486c6315220148dcc0e65","unresolved":true,"context_lines":[{"line_number":3215,"context_line":"            return self._refresh(instance_uuid, volume_id, connector)"},{"line_number":3216,"context_line":""},{"line_number":3217,"context_line":"        except HostConflict as e:"},{"line_number":3218,"context_line":"            e \u003d str(e).replace(\u0027. \u0027, \u0027.\\n\u0027, 1)"},{"line_number":3219,"context_line":"            print(e)"},{"line_number":3220,"context_line":"            return 7"},{"line_number":3221,"context_line":"        except exception.VolumeBDMNotFound as e:"}],"source_content_type":"text/x-python","patch_set":10,"id":"03a1aae8_241b12d4","line":3218,"updated":"2023-07-13 17:24:18.000000000","message":"While this literally addresses my comment, it\u0027s a bit of a naive understanding of the spirit. Further, if these are ever translated this might not behave properly. So, I\u0027ll try again and use more detail.\n\nI think the exception string should be the most basic problem (i.e. hostA !\u003d hostB) and the \"you\u0027re using this command wrong\" string should be generated in the context of the actual command. If the `_do_refresh` method is ever re-used as part of another command in here, the implication that it was used for volume refresh will be wrong and confusing.\n\nI think the raise should look like this:\n```\nraise HostConflict(f\"The compute host \u0027{connector[\u0027host\u0027]}\u0027 in the \"\n                   f\"connector does not match the instance host \"\n                   f\"\u0027{instance.host}\u0027)\n```                   \nand the exception handler should be:\n```\nexcept HostConflict as e:\n    print(f\"The cmd \u0027nova-manage volume_attachment get_connector\u0027 \"\n          f\"may have been run on the wrong compute host. Or the \"\n          f\"instance host may be wrong and in need of repair: {e}\")\n```\nAlso, I kept the above strings mostly as you have them, but IMHO \"cmd\" should be spelled out as \"command\".","commit_id":"74b8efd76a43ad90e2fb7ffd060ebe7dad78a10f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"8ce16bd47ea1c5f316262a7f02b9946db3a907c8","unresolved":true,"context_lines":[{"line_number":3215,"context_line":"            return self._refresh(instance_uuid, volume_id, connector)"},{"line_number":3216,"context_line":""},{"line_number":3217,"context_line":"        except HostConflict as e:"},{"line_number":3218,"context_line":"            e \u003d str(e).replace(\u0027. \u0027, \u0027.\\n\u0027, 1)"},{"line_number":3219,"context_line":"            print(e)"},{"line_number":3220,"context_line":"            return 7"},{"line_number":3221,"context_line":"        except exception.VolumeBDMNotFound as e:"}],"source_content_type":"text/x-python","patch_set":10,"id":"b45d32fd_5cfe203a","line":3218,"in_reply_to":"03a1aae8_241b12d4","updated":"2023-07-14 09:21:55.000000000","message":"Done","commit_id":"74b8efd76a43ad90e2fb7ffd060ebe7dad78a10f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4ddfc8e11a0c58dbd78deb7b193228c2d7819b3f","unresolved":false,"context_lines":[{"line_number":3215,"context_line":"            return self._refresh(instance_uuid, volume_id, connector)"},{"line_number":3216,"context_line":""},{"line_number":3217,"context_line":"        except HostConflict as e:"},{"line_number":3218,"context_line":"            e \u003d str(e).replace(\u0027. \u0027, \u0027.\\n\u0027, 1)"},{"line_number":3219,"context_line":"            print(e)"},{"line_number":3220,"context_line":"            return 7"},{"line_number":3221,"context_line":"        except exception.VolumeBDMNotFound as e:"}],"source_content_type":"text/x-python","patch_set":10,"id":"b05ec955_2d41217a","line":3218,"in_reply_to":"b45d32fd_5cfe203a","updated":"2023-07-14 13:44:51.000000000","message":"Looks better now.","commit_id":"74b8efd76a43ad90e2fb7ffd060ebe7dad78a10f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4ddfc8e11a0c58dbd78deb7b193228c2d7819b3f","unresolved":true,"context_lines":[{"line_number":3237,"context_line":"                f\u0027connector_info data?\u0027"},{"line_number":3238,"context_line":"            )"},{"line_number":3239,"context_line":"            return 3"},{"line_number":3240,"context_line":"        except (exception.InvalidInput, OSError) as e:"},{"line_number":3241,"context_line":"            print(str(e))"},{"line_number":3242,"context_line":"            return 2"},{"line_number":3243,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":11,"id":"62ce6f74_7e042c09","line":3240,"updated":"2023-07-14 13:44:51.000000000","message":"I\u0027m guessing there\u0027s no coverage for this case since you didn\u0027t have to change a test\u0027s expected result.","commit_id":"3b9a275b8f107e36a598dd4cd78333e04bb18894"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"27e3a8e2734eeade3c2bad4d0471d8eb23308125","unresolved":true,"context_lines":[{"line_number":3237,"context_line":"                f\u0027connector_info data?\u0027"},{"line_number":3238,"context_line":"            )"},{"line_number":3239,"context_line":"            return 3"},{"line_number":3240,"context_line":"        except (exception.InvalidInput, OSError) as e:"},{"line_number":3241,"context_line":"            print(str(e))"},{"line_number":3242,"context_line":"            return 2"},{"line_number":3243,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":11,"id":"92994616_f659cb12","line":3240,"in_reply_to":"24f7037c_793999db","updated":"2023-07-21 12:01:03.000000000","message":"Added a test for OSError","commit_id":"3b9a275b8f107e36a598dd4cd78333e04bb18894"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"2d70ad5c4f2e2c958dda43e2b63541c56b36b8a8","unresolved":true,"context_lines":[{"line_number":3237,"context_line":"                f\u0027connector_info data?\u0027"},{"line_number":3238,"context_line":"            )"},{"line_number":3239,"context_line":"            return 3"},{"line_number":3240,"context_line":"        except (exception.InvalidInput, OSError) as e:"},{"line_number":3241,"context_line":"            print(str(e))"},{"line_number":3242,"context_line":"            return 2"},{"line_number":3243,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":11,"id":"24f7037c_793999db","line":3240,"in_reply_to":"62ce6f74_7e042c09","updated":"2023-07-14 14:11:04.000000000","message":"right now there are 2 if file exists\n\ntest_refresh_missing_connector_path_file, throws 2\ntest_refresh_invalid_connector_path_file, throws 3\n\nhttps://github.com/openstack/nova/blob/master/nova/tests/unit/cmd/test_manage.py#L3462","commit_id":"3b9a275b8f107e36a598dd4cd78333e04bb18894"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a0194109920d99aa09e797602dcd77fd99be9113","unresolved":false,"context_lines":[{"line_number":3237,"context_line":"                f\u0027connector_info data?\u0027"},{"line_number":3238,"context_line":"            )"},{"line_number":3239,"context_line":"            return 3"},{"line_number":3240,"context_line":"        except (exception.InvalidInput, OSError) as e:"},{"line_number":3241,"context_line":"            print(str(e))"},{"line_number":3242,"context_line":"            return 2"},{"line_number":3243,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":11,"id":"8020da5b_6041e658","line":3240,"in_reply_to":"92994616_f659cb12","updated":"2023-07-25 14:19:34.000000000","message":"Done","commit_id":"3b9a275b8f107e36a598dd4cd78333e04bb18894"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"01406b82225bd045f55604c53a5e72bb517d406d","unresolved":false,"context_lines":[{"line_number":178,"context_line":""},{"line_number":179,"context_line":""},{"line_number":180,"context_line":"class HostConflict(Exception):"},{"line_number":181,"context_line":"    pass"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"class DbCommands(object):"}],"source_content_type":"text/x-python","patch_set":21,"id":"a35c364a_8693d136","line":181,"updated":"2024-02-13 12:43:37.000000000","message":"we do not define excptions in moduels outside of nova.excpetions\n\nso this is not in the correct place.","commit_id":"a9143e1a2d6a5233847d0541f7b4691af7c59a3b"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"631e20308947f24dce40dcf9900fef679525d6b3","unresolved":true,"context_lines":[{"line_number":179,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"class HostConflict(Exception):"},{"line_number":183,"context_line":"    pass"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":""}],"source_content_type":"text/x-python","patch_set":23,"id":"2eed5771_391cc0c9","line":182,"updated":"2024-02-12 00:56:59.000000000","message":"nit: we generally try to put exceptions in nova/exception.py, I can\u0027t find any precedence for exception classes in this file (or others under nova/cmd)","commit_id":"32424a14877e14b272db32c70b66bdb3d0041b56"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"01406b82225bd045f55604c53a5e72bb517d406d","unresolved":false,"context_lines":[{"line_number":179,"context_line":"                compute_api.unlock(cctxt, instance)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"class HostConflict(Exception):"},{"line_number":183,"context_line":"    pass"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":""}],"source_content_type":"text/x-python","patch_set":23,"id":"f31ae919_c2d941dc","line":182,"in_reply_to":"2eed5771_391cc0c9","updated":"2024-02-13 12:43:37.000000000","message":"Done","commit_id":"32424a14877e14b272db32c70b66bdb3d0041b56"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"631e20308947f24dce40dcf9900fef679525d6b3","unresolved":true,"context_lines":[{"line_number":3225,"context_line":"        ) as e:"},{"line_number":3226,"context_line":"            print(str(e))"},{"line_number":3227,"context_line":"            return 4"},{"line_number":3228,"context_line":"        except ValueError:"},{"line_number":3229,"context_line":"            print("},{"line_number":3230,"context_line":"                f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3231,"context_line":"                f\u0027connector_info data?\u0027"}],"source_content_type":"text/x-python","patch_set":23,"id":"1660defe_fa9f32bf","line":3228,"updated":"2024-02-12 00:56:59.000000000","message":"Looks like you\u0027re trying to do 2 things in the same patch.\n\nIf I understand this correctly, ValueError will get thrown if the JSON is invalid, which is different from OSError, indicating something wrong at the file system level. Splitting OSError into a different except block makes sense, but maybe do it in a separate patch?","commit_id":"32424a14877e14b272db32c70b66bdb3d0041b56"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"01406b82225bd045f55604c53a5e72bb517d406d","unresolved":true,"context_lines":[{"line_number":3225,"context_line":"        ) as e:"},{"line_number":3226,"context_line":"            print(str(e))"},{"line_number":3227,"context_line":"            return 4"},{"line_number":3228,"context_line":"        except ValueError:"},{"line_number":3229,"context_line":"            print("},{"line_number":3230,"context_line":"                f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3231,"context_line":"                f\u0027connector_info data?\u0027"}],"source_content_type":"text/x-python","patch_set":23,"id":"1edb1803_e30eeffb","line":3228,"in_reply_to":"1660defe_fa9f32bf","updated":"2024-02-13 12:43:37.000000000","message":"this patch is only adding a check for the host value not bing the same.\nvalue error would still be caught here and OSError if the file does not exist would be on line 3230\n---later---\nah this was split out to the previous patch","commit_id":"32424a14877e14b272db32c70b66bdb3d0041b56"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"b3d5221b157e38e4e75fd405b623374365edf407","unresolved":false,"context_lines":[{"line_number":3225,"context_line":"        ) as e:"},{"line_number":3226,"context_line":"            print(str(e))"},{"line_number":3227,"context_line":"            return 4"},{"line_number":3228,"context_line":"        except ValueError:"},{"line_number":3229,"context_line":"            print("},{"line_number":3230,"context_line":"                f\u0027Failed to open {connector_path}. Does it contain valid \u0027"},{"line_number":3231,"context_line":"                f\u0027connector_info data?\u0027"}],"source_content_type":"text/x-python","patch_set":23,"id":"dd2e534f_a99436b1","line":3228,"in_reply_to":"1edb1803_e30eeffb","updated":"2024-02-14 10:27:39.000000000","message":"Done","commit_id":"32424a14877e14b272db32c70b66bdb3d0041b56"}],"nova/tests/unit/cmd/test_manage.py":[{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"4a798b3bd734aa7803e44886cb4c4384ed6d032a","unresolved":true,"context_lines":[{"line_number":3625,"context_line":"        mock_get_instance.return_value \u003d objects.Instance("},{"line_number":3626,"context_line":"            uuid\u003duuidsentinel.instance,"},{"line_number":3627,"context_line":"            vm_state\u003dobj_fields.InstanceState.STOPPED,"},{"line_number":3628,"context_line":"            host\u003d\u0027old-host\u0027, locked\u003dFalse)"},{"line_number":3629,"context_line":"        mock_get_bdm.return_value \u003d objects.BlockDeviceMapping("},{"line_number":3630,"context_line":"            uuid\u003duuidsentinel.bdm, volume_id\u003duuidsentinel.volume,"},{"line_number":3631,"context_line":"            attachment_id\u003duuidsentinel.instance,"}],"source_content_type":"text/x-python","patch_set":5,"id":"2df4d4ef_224bbd04","line":3628,"updated":"2023-03-21 14:35:11.000000000","message":"Ah, \u0027old-host\u0027 is different that the \u0027fake-host\u0027 that _get_fake_connector_info() returns further down.","commit_id":"e2264355a18fb0315b05d365b6f83262dffda12f"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"e9ce6f4e8f4ee1280d4a4a59b59f3cc8fd0abc85","unresolved":false,"context_lines":[{"line_number":3625,"context_line":"        mock_get_instance.return_value \u003d objects.Instance("},{"line_number":3626,"context_line":"            uuid\u003duuidsentinel.instance,"},{"line_number":3627,"context_line":"            vm_state\u003dobj_fields.InstanceState.STOPPED,"},{"line_number":3628,"context_line":"            host\u003d\u0027old-host\u0027, locked\u003dFalse)"},{"line_number":3629,"context_line":"        mock_get_bdm.return_value \u003d objects.BlockDeviceMapping("},{"line_number":3630,"context_line":"            uuid\u003duuidsentinel.bdm, volume_id\u003duuidsentinel.volume,"},{"line_number":3631,"context_line":"            attachment_id\u003duuidsentinel.instance,"}],"source_content_type":"text/x-python","patch_set":5,"id":"0e804507_4767d38c","line":3628,"in_reply_to":"2df4d4ef_224bbd04","updated":"2023-06-21 14:15:35.000000000","message":"Done","commit_id":"e2264355a18fb0315b05d365b6f83262dffda12f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"01406b82225bd045f55604c53a5e72bb517d406d","unresolved":true,"context_lines":[{"line_number":3677,"context_line":"            \u0027id\u0027: uuidsentinel.new_attachment,"},{"line_number":3678,"context_line":"        }"},{"line_number":3679,"context_line":"        fake_volume_api.attachment_update.return_value \u003d {"},{"line_number":3680,"context_line":"            \u0027connection_info\u0027: self._get_fake_connector_info(),"},{"line_number":3681,"context_line":"        }"},{"line_number":3682,"context_line":""},{"line_number":3683,"context_line":"        ret \u003d self._test_refresh()"}],"source_content_type":"text/x-python","patch_set":24,"id":"105e87c5_60c20bb1","line":3680,"updated":"2024-02-13 12:43:37.000000000","message":"def _get_fake_connector_info():\n        return {\n            \u0027ip\u0027: \u0027192.168.7.8\u0027,\n            \u0027host\u0027: \u0027fake-host\u0027,\n            \u0027initiator\u0027: \u0027fake.initiator.iqn\u0027,\n            \u0027wwpns\u0027: \u0027100010604b019419\u0027,\n            \u0027wwnns\u0027: \u0027200010604b019419\u0027,\n            \u0027multipath\u0027: False,\n            \u0027platform\u0027: \u0027x86_64\u0027,\n            \u0027os_type\u0027: \u0027linux2\u0027,\n        }\n        \nthe host is fake-host in the connector info but old-host in the instnace.\n\nthat is really not obvious without looking that op so it wouldbe nice to add a comment for that.","commit_id":"9c980798fe7e1bb5f9f6e173457a9b0d22642bdd"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"b3d5221b157e38e4e75fd405b623374365edf407","unresolved":false,"context_lines":[{"line_number":3677,"context_line":"            \u0027id\u0027: uuidsentinel.new_attachment,"},{"line_number":3678,"context_line":"        }"},{"line_number":3679,"context_line":"        fake_volume_api.attachment_update.return_value \u003d {"},{"line_number":3680,"context_line":"            \u0027connection_info\u0027: self._get_fake_connector_info(),"},{"line_number":3681,"context_line":"        }"},{"line_number":3682,"context_line":""},{"line_number":3683,"context_line":"        ret \u003d self._test_refresh()"}],"source_content_type":"text/x-python","patch_set":24,"id":"804956e1_f074aa34","line":3680,"in_reply_to":"105e87c5_60c20bb1","updated":"2024-02-14 10:27:39.000000000","message":"Done","commit_id":"9c980798fe7e1bb5f9f6e173457a9b0d22642bdd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"393ea50525510117ef01dd08c5fcfe523370d619","unresolved":true,"context_lines":[{"line_number":3684,"context_line":"        }"},{"line_number":3685,"context_line":""},{"line_number":3686,"context_line":"        ret \u003d self._test_refresh()"},{"line_number":3687,"context_line":"        self.assertEqual(7, ret)"},{"line_number":3688,"context_line":""},{"line_number":3689,"context_line":"    @mock.patch(\u0027nova.compute.rpcapi.ComputeAPI\u0027, autospec\u003dTrue)"},{"line_number":3690,"context_line":"    @mock.patch(\u0027nova.volume.cinder.API\u0027, autospec\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":29,"id":"58773ef6_d65f696c","line":3687,"updated":"2024-03-05 16:50:20.000000000","message":"+1","commit_id":"a8f81d5f08692c16d538af3197460d9426cc00a1"}]}