)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"043aafefeee45fb10a75f4b74795f7fa5002e1e1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"34308876_0735fdf7","updated":"2023-08-29 15:01:38.000000000","message":"Please urgently fix the list thing, and I fast-approve quickly, given Dan was already +2.","commit_id":"de7f181aa3c0cb2a989b7a35c3752c2c53ba9f86"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"6c6c772fca6d19a01faeb63683a12e97d0515f2d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"56660737_58dd44c1","updated":"2023-08-30 09:39:29.000000000","message":"Thanks Amit for the quick rework, please see my note for improving your Python knowledge.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"771a39f09cc5a64a01c4a0079396088ee557ac66","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"edee689d_1e079865","updated":"2023-08-30 19:39:04.000000000","message":"i was going to remove my -1 but i think my other questions are still valid i.e. why are you waiting for the instce to be active","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"232e586e4b8c929180e395a342e5f2138efccc07","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"b8fd72de_b9642179","updated":"2023-08-31 10:39:11.000000000","message":"So, the scope of this series is very close to what Amit wrote in his spec and that we agreed : https://specs.openstack.org/openstack/nova-specs/specs/2023.2/approved/cleanup-dangling-volume-attachments.html\n\nIt\u0027s not resolving all of the Cinder attachments issues but at least it provides one solution for users.\n\n+2 based on that.","commit_id":"c33a9ccf4c2c996f5bb2e43fb6107072a9d2c66e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a31114920606813b8d91c7f3ca728f36c738eec6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"d57f6b3f_af008167","updated":"2023-08-31 11:07:49.000000000","message":"i still have some nits with this but there is nothing that can be corrected at a later date and nothing that is technial going to break anything as this si just test code so +2w","commit_id":"c33a9ccf4c2c996f5bb2e43fb6107072a9d2c66e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a31114920606813b8d91c7f3ca728f36c738eec6","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":7,"id":"ddd234a7_016d3827","in_reply_to":"b8fd72de_b9642179","updated":"2023-08-31 11:07:49.000000000","message":"right but i didn\u0027t review the spec after it was change form a nova manage command and i woudl have been -1 on it if it was not for the service token changes for the cve.\n\nso i was raisng my concern with it since i was asked to reivew.\nif we did not have the service user token for the attachment api now i would be -1 bordering on a -2 for the approch.\n\ngiven that is now a hard requirement when an instance is specified in a attachment im ok with proceeding with this even if i still think this is not the behvaior i woudl generally expect as an end user. its one we can document and explain.i would have expected use to still have an error in the case the volume extis but is not attachned and make detach work in that case. this is more automatic but im less sure if that is a good thing.\n\nwhat im afraid of is peopel who have messed with attachment, (or whare admins have),   may suddenly find there vm that was working fine stops work after a reboot because a volume disappeard. they can just attach it back but that might have other implications for there workload.","commit_id":"c33a9ccf4c2c996f5bb2e43fb6107072a9d2c66e"}],"nova/tests/functional/compute/test_attached_volumes.py":[{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"0a905029c87cd0fdf9d0a3831169e5bd96afacb6","unresolved":true,"context_lines":[{"line_number":28,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":29,"context_line":"        self.start_compute()"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    def _attach_volumes(self, server, vol_ids\u003dlist):"},{"line_number":32,"context_line":"        # attach volumes to server"},{"line_number":33,"context_line":"        for vol_id in vol_ids:"},{"line_number":34,"context_line":"            self.api.post_server_volume("}],"source_content_type":"text/x-python","patch_set":3,"id":"18b6336f_934032cc","line":31,"updated":"2023-07-31 11:36:58.000000000","message":"these attachments are done by nova api, that means nova know about these attachments and so they are valid ones.","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"7673d8dc5afdad401d8c85df7bfb8c581d05f78b","unresolved":true,"context_lines":[{"line_number":28,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":29,"context_line":"        self.start_compute()"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    def _attach_volumes(self, server, vol_ids\u003dlist):"},{"line_number":32,"context_line":"        # attach volumes to server"},{"line_number":33,"context_line":"        for vol_id in vol_ids:"},{"line_number":34,"context_line":"            self.api.post_server_volume("}],"source_content_type":"text/x-python","patch_set":3,"id":"cccdc333_f08a83fe","line":31,"in_reply_to":"18b6336f_934032cc","updated":"2023-08-25 15:56:11.000000000","message":"These are both good to point out. Can you add these as a docstring or comment for people reading this code later?","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"2ac35a3fdf9019724799c36d028d1033c3f08ee4","unresolved":false,"context_lines":[{"line_number":28,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":29,"context_line":"        self.start_compute()"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    def _attach_volumes(self, server, vol_ids\u003dlist):"},{"line_number":32,"context_line":"        # attach volumes to server"},{"line_number":33,"context_line":"        for vol_id in vol_ids:"},{"line_number":34,"context_line":"            self.api.post_server_volume("}],"source_content_type":"text/x-python","patch_set":3,"id":"a3e2c12c_bf7c20bf","line":31,"in_reply_to":"cccdc333_f08a83fe","updated":"2023-08-25 16:50:46.000000000","message":"Done","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"7673d8dc5afdad401d8c85df7bfb8c581d05f78b","unresolved":true,"context_lines":[{"line_number":43,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":44,"context_line":"            ctxt, server[\u0027id\u0027])"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"        # filter [(vol.id and vol.attachment_id)]"},{"line_number":47,"context_line":"        return [("},{"line_number":48,"context_line":"            bdm.volume_id, bdm.attachment_id) for bdm in bdms if bdm.volume_id]"},{"line_number":49,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"6aa280b9_20be626f","line":46,"updated":"2023-08-25 15:56:11.000000000","message":"Remove leftover debug?","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"2ac35a3fdf9019724799c36d028d1033c3f08ee4","unresolved":false,"context_lines":[{"line_number":43,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":44,"context_line":"            ctxt, server[\u0027id\u0027])"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"        # filter [(vol.id and vol.attachment_id)]"},{"line_number":47,"context_line":"        return [("},{"line_number":48,"context_line":"            bdm.volume_id, bdm.attachment_id) for bdm in bdms if bdm.volume_id]"},{"line_number":49,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"c8c72793_3ecdc9c8","line":46,"in_reply_to":"6aa280b9_20be626f","updated":"2023-08-25 16:50:46.000000000","message":"Done","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"0a905029c87cd0fdf9d0a3831169e5bd96afacb6","unresolved":true,"context_lines":[{"line_number":47,"context_line":"        return [("},{"line_number":48,"context_line":"            bdm.volume_id, bdm.attachment_id) for bdm in bdms if bdm.volume_id]"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def _create_vol_attachments_by_cinder(self, volume_id, server, attach\u003d1):"},{"line_number":51,"context_line":"        for _ in range(attach):"},{"line_number":52,"context_line":"            self.cinder.create_vol_attachment("},{"line_number":53,"context_line":"                volume_id, server[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":3,"id":"fc11045c_c9282ba5","line":50,"updated":"2023-07-31 11:36:58.000000000","message":"these attachments are done by cinder api, and nova is not aware of them.","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"2ac35a3fdf9019724799c36d028d1033c3f08ee4","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        return [("},{"line_number":48,"context_line":"            bdm.volume_id, bdm.attachment_id) for bdm in bdms if bdm.volume_id]"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def _create_vol_attachments_by_cinder(self, volume_id, server, attach\u003d1):"},{"line_number":51,"context_line":"        for _ in range(attach):"},{"line_number":52,"context_line":"            self.cinder.create_vol_attachment("},{"line_number":53,"context_line":"                volume_id, server[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":3,"id":"ec755c3a_3c20ab38","line":50,"in_reply_to":"fc11045c_c9282ba5","updated":"2023-08-25 16:50:46.000000000","message":"Done","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"0a905029c87cd0fdf9d0a3831169e5bd96afacb6","unresolved":true,"context_lines":[{"line_number":147,"context_line":"        # get attachment id by server from cinder"},{"line_number":148,"context_line":"        attached_volume_ids \u003d self.cinder.attachment_ids_for_instance("},{"line_number":149,"context_line":"            server[\u0027id\u0027])"},{"line_number":150,"context_line":"        self.assertEqual(attachments + 1, len(attached_volume_ids))"},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"        for _id in attached_volume_ids:"},{"line_number":153,"context_line":"            _ \u003d self.cinder.get_vol_attachment(_id)"}],"source_content_type":"text/x-python","patch_set":3,"id":"2b1bc954_682c72fd","line":150,"updated":"2023-07-31 11:36:58.000000000","message":"here we are checking with +1 because we had attached a valid attachment at line 140. this is to differentiate that reboot should not remove a valid attachment and only remove dangling one.","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"7673d8dc5afdad401d8c85df7bfb8c581d05f78b","unresolved":true,"context_lines":[{"line_number":147,"context_line":"        # get attachment id by server from cinder"},{"line_number":148,"context_line":"        attached_volume_ids \u003d self.cinder.attachment_ids_for_instance("},{"line_number":149,"context_line":"            server[\u0027id\u0027])"},{"line_number":150,"context_line":"        self.assertEqual(attachments + 1, len(attached_volume_ids))"},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"        for _id in attached_volume_ids:"},{"line_number":153,"context_line":"            _ \u003d self.cinder.get_vol_attachment(_id)"}],"source_content_type":"text/x-python","patch_set":3,"id":"f237903e_acc490e3","line":150,"in_reply_to":"2b1bc954_682c72fd","updated":"2023-08-25 15:56:11.000000000","message":"Please add this to the comment!","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"2ac35a3fdf9019724799c36d028d1033c3f08ee4","unresolved":false,"context_lines":[{"line_number":147,"context_line":"        # get attachment id by server from cinder"},{"line_number":148,"context_line":"        attached_volume_ids \u003d self.cinder.attachment_ids_for_instance("},{"line_number":149,"context_line":"            server[\u0027id\u0027])"},{"line_number":150,"context_line":"        self.assertEqual(attachments + 1, len(attached_volume_ids))"},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"        for _id in attached_volume_ids:"},{"line_number":153,"context_line":"            _ \u003d self.cinder.get_vol_attachment(_id)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ce697e36_e4ba17c4","line":150,"in_reply_to":"f237903e_acc490e3","updated":"2023-08-25 16:50:46.000000000","message":"Done","commit_id":"cab0c95a415b8f1587efd2ba2784a38cb4800b42"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"043aafefeee45fb10a75f4b74795f7fa5002e1e1","unresolved":true,"context_lines":[{"line_number":28,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":29,"context_line":"        self.start_compute()"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    def _attach_volumes(self, server, vol_ids\u003dlist):"},{"line_number":32,"context_line":"        # attach volumes to server"},{"line_number":33,"context_line":"        # these attachments are done by nova api, that means"},{"line_number":34,"context_line":"        # nova know about these attachments and so they are valid ones."}],"source_content_type":"text/x-python","patch_set":5,"id":"34e10b03_a5d53174","line":31,"range":{"start_line":31,"start_character":45,"end_line":31,"end_character":50},"updated":"2023-08-29 15:01:38.000000000","message":"please remove this, you\u0027re defaulting the parameter to the python list object, which can create some weird behaviour.\n\nIf you want to do annotations, please do it this way : \n\n    def _attach_volumes(self, server: ty.Dict, vol_ids: ty.List[str])","commit_id":"de7f181aa3c0cb2a989b7a35c3752c2c53ba9f86"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"417665de11f4329ef83bfe3b6a96d4bec7d2871e","unresolved":true,"context_lines":[{"line_number":28,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":29,"context_line":"        self.start_compute()"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    def _attach_volumes(self, server, vol_ids\u003dlist):"},{"line_number":32,"context_line":"        # attach volumes to server"},{"line_number":33,"context_line":"        # these attachments are done by nova api, that means"},{"line_number":34,"context_line":"        # nova know about these attachments and so they are valid ones."}],"source_content_type":"text/x-python","patch_set":5,"id":"5270c592_5d819618","line":31,"range":{"start_line":31,"start_character":45,"end_line":31,"end_character":50},"in_reply_to":"34e10b03_a5d53174","updated":"2023-08-30 07:53:23.000000000","message":"nice catch actually I meant \n```def _attach_volumes(self, server, vol_ids\u003dlist()):```\n\nbut this seems better","commit_id":"de7f181aa3c0cb2a989b7a35c3752c2c53ba9f86"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"1971a9432e535c8694360a082bd6a0878d03dba5","unresolved":false,"context_lines":[{"line_number":28,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":29,"context_line":"        self.start_compute()"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    def _attach_volumes(self, server, vol_ids\u003dlist):"},{"line_number":32,"context_line":"        # attach volumes to server"},{"line_number":33,"context_line":"        # these attachments are done by nova api, that means"},{"line_number":34,"context_line":"        # nova know about these attachments and so they are valid ones."}],"source_content_type":"text/x-python","patch_set":5,"id":"00831e61_7d033931","line":31,"range":{"start_line":31,"start_character":45,"end_line":31,"end_character":50},"in_reply_to":"515d91cb_768276e7","updated":"2023-08-30 09:51:44.000000000","message":"Ack, thanks","commit_id":"de7f181aa3c0cb2a989b7a35c3752c2c53ba9f86"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"6c6c772fca6d19a01faeb63683a12e97d0515f2d","unresolved":true,"context_lines":[{"line_number":28,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":29,"context_line":"        self.start_compute()"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    def _attach_volumes(self, server, vol_ids\u003dlist):"},{"line_number":32,"context_line":"        # attach volumes to server"},{"line_number":33,"context_line":"        # these attachments are done by nova api, that means"},{"line_number":34,"context_line":"        # nova know about these attachments and so they are valid ones."}],"source_content_type":"text/x-python","patch_set":5,"id":"515d91cb_768276e7","line":31,"range":{"start_line":31,"start_character":45,"end_line":31,"end_character":50},"in_reply_to":"5270c592_5d819618","updated":"2023-08-30 09:39:29.000000000","message":"No, as I said elsewhere, you can\u0027t default a parameter with a mutable object like list().\n\nExample why: \n```\n\u003e\u003e\u003e def foo(baz\u003dlist()):\n...     baz.append(1)\n...     return baz\n... \n\u003e\u003e\u003e foo()\n[1]\n\u003e\u003e\u003e foo()\n[1, 1]\n```\n\nSince the parameter is evaluated once at definition time, you will only once have baz as an empty list.\n\nPlease don\u0027t get confused by annotations (that are optional and marked using a colon) and defaulted args (using the equal sign)\nYou can mix both :\n\n```\ndef foo(baz: ty.List[int] \u003d None):\n   if baz is None:\n       baz \u003d []\n   ...\n```","commit_id":"de7f181aa3c0cb2a989b7a35c3752c2c53ba9f86"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"043aafefeee45fb10a75f4b74795f7fa5002e1e1","unresolved":false,"context_lines":[{"line_number":49,"context_line":"        return [("},{"line_number":50,"context_line":"            bdm.volume_id, bdm.attachment_id) for bdm in bdms if bdm.volume_id]"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    def _create_vol_attachments_by_cinder(self, volume_id, server, attach\u003d1):"},{"line_number":53,"context_line":"        # these attachments are done by cinder api,"},{"line_number":54,"context_line":"        # and nova is not aware of them."},{"line_number":55,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"975ed40c_01441b18","line":52,"range":{"start_line":52,"start_character":67,"end_line":52,"end_character":75},"updated":"2023-08-29 15:01:38.000000000","message":"you\u0027re fortunate, ints are considered immutable in Python.\nIn general, please avoid to default some arguments if those arguments are mutable, given the defaults are set at method definition time, not method call time.\n\nhttps://docs.python-guide.org/writing/gotchas/#mutable-default-arguments\n\nThat\u0027s why I usually recommend to do defaults this way :\n\n    def foo(var\u003dNone):\n        if var is None:\n            var\u003d1","commit_id":"de7f181aa3c0cb2a989b7a35c3752c2c53ba9f86"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"043aafefeee45fb10a75f4b74795f7fa5002e1e1","unresolved":true,"context_lines":[{"line_number":53,"context_line":"        # these attachments are done by cinder api,"},{"line_number":54,"context_line":"        # and nova is not aware of them."},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"        for _ in range(attach):"},{"line_number":57,"context_line":"            self.cinder.create_vol_attachment("},{"line_number":58,"context_line":"                volume_id, server[\u0027id\u0027])"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"f1a688fa_219d4bd0","line":56,"range":{"start_line":56,"start_character":23,"end_line":56,"end_character":29},"updated":"2023-08-29 15:01:38.000000000","message":"nit: I\u0027d prefer having a meaningful parameter name, like nb_attach","commit_id":"de7f181aa3c0cb2a989b7a35c3752c2c53ba9f86"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"6c6c772fca6d19a01faeb63683a12e97d0515f2d","unresolved":false,"context_lines":[{"line_number":53,"context_line":"        # these attachments are done by cinder api,"},{"line_number":54,"context_line":"        # and nova is not aware of them."},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"        for _ in range(attach):"},{"line_number":57,"context_line":"            self.cinder.create_vol_attachment("},{"line_number":58,"context_line":"                volume_id, server[\u0027id\u0027])"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"ab828493_9fa767f1","line":56,"range":{"start_line":56,"start_character":23,"end_line":56,"end_character":29},"in_reply_to":"3e2ef4eb_009b93cf","updated":"2023-08-30 09:39:29.000000000","message":"Sorry, I meant \u0027number of attachments\u0027, hence nb_attach.\n\nAnyway, won\u0027t bother more.","commit_id":"de7f181aa3c0cb2a989b7a35c3752c2c53ba9f86"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"417665de11f4329ef83bfe3b6a96d4bec7d2871e","unresolved":true,"context_lines":[{"line_number":53,"context_line":"        # these attachments are done by cinder api,"},{"line_number":54,"context_line":"        # and nova is not aware of them."},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"        for _ in range(attach):"},{"line_number":57,"context_line":"            self.cinder.create_vol_attachment("},{"line_number":58,"context_line":"                volume_id, server[\u0027id\u0027])"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3e2ef4eb_009b93cf","line":56,"range":{"start_line":56,"start_character":23,"end_line":56,"end_character":29},"in_reply_to":"f1a688fa_219d4bd0","updated":"2023-08-30 07:53:23.000000000","message":"Done\nwas not sure of meaning nb_attach, updated as new_attachments","commit_id":"de7f181aa3c0cb2a989b7a35c3752c2c53ba9f86"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5e92f5545f2bef75fec360e6fa50f0408d10e547","unresolved":true,"context_lines":[{"line_number":29,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":30,"context_line":"        self.start_compute()"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    def _attach_volumes(self, server: ty.Dict, vol_ids: ty.List[str]):"},{"line_number":33,"context_line":"        # attach volumes to server"},{"line_number":34,"context_line":"        # these attachments are done by nova api, that means"},{"line_number":35,"context_line":"        # nova know about these attachments and so they are valid ones."},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"        for vol_id in vol_ids:"},{"line_number":38,"context_line":"            self.api.post_server_volume("},{"line_number":39,"context_line":"                server[\u0027id\u0027], {\u0027volumeAttachment\u0027: {\u0027volumeId\u0027: vol_id}})"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # wait for last vol to get attached"},{"line_number":42,"context_line":"        self._wait_for_volume_attach(server[\u0027id\u0027], vol_id)"},{"line_number":43,"context_line":"        return self._wait_for_state_change(server, expected_status\u003d\u0027ACTIVE\u0027)"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    def _get_bdm_list(self, server):"}],"source_content_type":"text/x-python","patch_set":6,"id":"819aefbc_6bee4622","line":42,"range":{"start_line":32,"start_character":0,"end_line":42,"end_character":58},"updated":"2023-08-30 18:01:05.000000000","message":"you shoudl really add an _attach_volume function to integrated_helpers\nand call that instead in a loop here.\n\nlike _attach_interface and _wait_for_volume_attach\nhttps://github.com/openstack/nova/blob/master/nova/tests/functional/integrated_helpers.py#L495-L505\nhttps://github.com/openstack/nova/blob/master/nova/tests/functional/integrated_helpers.py#L205C9-L205C32\n\n\nthat can be refactored late but we already have this code multiple tiems in the functional tests\n\nhttps://github.com/search?q\u003drepo%3Aopenstack%2Fnova+volumeAttachment+language%3APython+path%3A%2F%5Enova%5C%2Ftests%5C%2Ffunctional%5C%2F%2F\u0026type\u003dcode\n\nso it woudl be good to refactor this out into a common funciton as a followup.\n\n\nthe other thing is you really shoudl be waiting for alll the voluem to be attached not jsut the final one. it will proably work but this could cause a race.\nthe eventlets handeling the reqeust may not complete in order.\nso i woudl like you to fix that i this patch.\n\ni think they will most of the time but i dont think you can actully rely on that unless we are usign cast as call.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"8121d7743fd7f17a0275d3580fdc4b0561c9c5a8","unresolved":true,"context_lines":[{"line_number":29,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":30,"context_line":"        self.start_compute()"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    def _attach_volumes(self, server: ty.Dict, vol_ids: ty.List[str]):"},{"line_number":33,"context_line":"        # attach volumes to server"},{"line_number":34,"context_line":"        # these attachments are done by nova api, that means"},{"line_number":35,"context_line":"        # nova know about these attachments and so they are valid ones."},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"        for vol_id in vol_ids:"},{"line_number":38,"context_line":"            self.api.post_server_volume("},{"line_number":39,"context_line":"                server[\u0027id\u0027], {\u0027volumeAttachment\u0027: {\u0027volumeId\u0027: vol_id}})"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # wait for last vol to get attached"},{"line_number":42,"context_line":"        self._wait_for_volume_attach(server[\u0027id\u0027], vol_id)"},{"line_number":43,"context_line":"        return self._wait_for_state_change(server, expected_status\u003d\u0027ACTIVE\u0027)"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    def _get_bdm_list(self, server):"}],"source_content_type":"text/x-python","patch_set":6,"id":"cebe1153_fbe84cf0","line":42,"range":{"start_line":32,"start_character":0,"end_line":42,"end_character":58},"in_reply_to":"819aefbc_6bee4622","updated":"2023-08-31 06:39:11.000000000","message":"move attach_vol - done\nrefactor exsting  VolumeAttachment - ack\nwait for only _last_vol_ to get attached - done\n\n\nas this is a one-by-one request, and the body we are sending is same, for all the requests, I believed the last sent request will finish last.\n\nbut as Sean explained, this could cause a race condition.\nthanks Sean.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"5d83d91ecc2ec2872657b2a8f50a46127a18a931","unresolved":false,"context_lines":[{"line_number":29,"context_line":"        self.admin_api \u003d self.api_fixture.admin_api"},{"line_number":30,"context_line":"        self.start_compute()"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    def _attach_volumes(self, server: ty.Dict, vol_ids: ty.List[str]):"},{"line_number":33,"context_line":"        # attach volumes to server"},{"line_number":34,"context_line":"        # these attachments are done by nova api, that means"},{"line_number":35,"context_line":"        # nova know about these attachments and so they are valid ones."},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"        for vol_id in vol_ids:"},{"line_number":38,"context_line":"            self.api.post_server_volume("},{"line_number":39,"context_line":"                server[\u0027id\u0027], {\u0027volumeAttachment\u0027: {\u0027volumeId\u0027: vol_id}})"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # wait for last vol to get attached"},{"line_number":42,"context_line":"        self._wait_for_volume_attach(server[\u0027id\u0027], vol_id)"},{"line_number":43,"context_line":"        return self._wait_for_state_change(server, expected_status\u003d\u0027ACTIVE\u0027)"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    def _get_bdm_list(self, server):"}],"source_content_type":"text/x-python","patch_set":6,"id":"edcb3e1c_58c37add","line":42,"range":{"start_line":32,"start_character":0,"end_line":42,"end_character":58},"in_reply_to":"cebe1153_fbe84cf0","updated":"2023-09-04 07:40:59.000000000","message":"refactor exsting VolumeAttachment  https://review.opendev.org/c/openstack/nova/+/893585","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5e92f5545f2bef75fec360e6fa50f0408d10e547","unresolved":true,"context_lines":[{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # wait for last vol to get attached"},{"line_number":42,"context_line":"        self._wait_for_volume_attach(server[\u0027id\u0027], vol_id)"},{"line_number":43,"context_line":"        return self._wait_for_state_change(server, expected_status\u003d\u0027ACTIVE\u0027)"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    def _get_bdm_list(self, server):"},{"line_number":46,"context_line":"        ctxt \u003d context.get_admin_context()"}],"source_content_type":"text/x-python","patch_set":6,"id":"55c93b98_c33950c1","line":43,"range":{"start_line":43,"start_character":15,"end_line":43,"end_character":1},"updated":"2023-08-30 18:01:05.000000000","message":"why are you doing this?\n\nthe vm status shoudl remain ACTIVE for the entirty fo the attchment.\n\nthe task_state may change but the status shoudl not.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a31114920606813b8d91c7f3ca728f36c738eec6","unresolved":true,"context_lines":[{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # wait for last vol to get attached"},{"line_number":42,"context_line":"        self._wait_for_volume_attach(server[\u0027id\u0027], vol_id)"},{"line_number":43,"context_line":"        return self._wait_for_state_change(server, expected_status\u003d\u0027ACTIVE\u0027)"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    def _get_bdm_list(self, server):"},{"line_number":46,"context_line":"        ctxt \u003d context.get_admin_context()"}],"source_content_type":"text/x-python","patch_set":6,"id":"9066cfa7_e7414d18","line":43,"range":{"start_line":43,"start_character":15,"end_line":43,"end_character":1},"in_reply_to":"53cdba62_6707837f","updated":"2023-08-31 11:07:49.000000000","message":"right that is not the correct way to do that.\ni was guessing you were using it for that but you hsould just do a sever show.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"8121d7743fd7f17a0275d3580fdc4b0561c9c5a8","unresolved":true,"context_lines":[{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # wait for last vol to get attached"},{"line_number":42,"context_line":"        self._wait_for_volume_attach(server[\u0027id\u0027], vol_id)"},{"line_number":43,"context_line":"        return self._wait_for_state_change(server, expected_status\u003d\u0027ACTIVE\u0027)"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    def _get_bdm_list(self, server):"},{"line_number":46,"context_line":"        ctxt \u003d context.get_admin_context()"}],"source_content_type":"text/x-python","patch_set":6,"id":"53cdba62_6707837f","line":43,"range":{"start_line":43,"start_character":15,"end_line":43,"end_character":1},"in_reply_to":"55c93b98_c33950c1","updated":"2023-08-31 06:39:11.000000000","message":"its kind of refreshing server object, but if we don\u0027t do it, attached volumes won\u0027t be showing in server object.\n\nAs its been added in helpers, I think its better to send back server having all whats required.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"5d83d91ecc2ec2872657b2a8f50a46127a18a931","unresolved":false,"context_lines":[{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # wait for last vol to get attached"},{"line_number":42,"context_line":"        self._wait_for_volume_attach(server[\u0027id\u0027], vol_id)"},{"line_number":43,"context_line":"        return self._wait_for_state_change(server, expected_status\u003d\u0027ACTIVE\u0027)"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    def _get_bdm_list(self, server):"},{"line_number":46,"context_line":"        ctxt \u003d context.get_admin_context()"}],"source_content_type":"text/x-python","patch_set":6,"id":"8ab75a40_690b9c7b","line":43,"range":{"start_line":43,"start_character":15,"end_line":43,"end_character":1},"in_reply_to":"9066cfa7_e7414d18","updated":"2023-09-04 07:40:59.000000000","message":"Assed show instead of calling wait_to_active https://review.opendev.org/c/openstack/nova/+/893584","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5e92f5545f2bef75fec360e6fa50f0408d10e547","unresolved":true,"context_lines":[{"line_number":76,"context_line":"        self.assertEqual(volume_id, bdm_list[0][0])"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"        # delete volume attachment using cinder api"},{"line_number":79,"context_line":"        self.cinder.delete_vol_attachment(bdm_list[0][0])"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"        # rebooting server should remove stale attachments"},{"line_number":82,"context_line":"        server \u003d self._reboot_server(server, hard\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5a7466be_3fab8ae8","line":79,"updated":"2023-08-30 18:01:05.000000000","message":"so this is simulating the user preformaing the unsupproted operation.\n\ni.e. atempting to detach a volume form cinder that is attached to a nova instnce.\n\nthis is not blocked without a service_user token.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a31114920606813b8d91c7f3ca728f36c738eec6","unresolved":false,"context_lines":[{"line_number":76,"context_line":"        self.assertEqual(volume_id, bdm_list[0][0])"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"        # delete volume attachment using cinder api"},{"line_number":79,"context_line":"        self.cinder.delete_vol_attachment(bdm_list[0][0])"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"        # rebooting server should remove stale attachments"},{"line_number":82,"context_line":"        server \u003d self._reboot_server(server, hard\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":6,"id":"2717b1c0_e38ad127","line":79,"in_reply_to":"5a7466be_3fab8ae8","updated":"2023-08-31 11:07:49.000000000","message":"Ack","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5e92f5545f2bef75fec360e6fa50f0408d10e547","unresolved":true,"context_lines":[{"line_number":78,"context_line":"        # delete volume attachment using cinder api"},{"line_number":79,"context_line":"        self.cinder.delete_vol_attachment(bdm_list[0][0])"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"        # rebooting server should remove stale attachments"},{"line_number":82,"context_line":"        server \u003d self._reboot_server(server, hard\u003dTrue)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"        # verify if volume attachment is present at cinder"}],"source_content_type":"text/x-python","patch_set":6,"id":"0a8d4d1d_351efba4","line":81,"range":{"start_line":81,"start_character":47,"end_line":81,"end_character":58},"updated":"2023-08-30 18:01:05.000000000","message":"BDMs","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"8121d7743fd7f17a0275d3580fdc4b0561c9c5a8","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        # delete volume attachment using cinder api"},{"line_number":79,"context_line":"        self.cinder.delete_vol_attachment(bdm_list[0][0])"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"        # rebooting server should remove stale attachments"},{"line_number":82,"context_line":"        server \u003d self._reboot_server(server, hard\u003dTrue)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":"        # verify if volume attachment is present at cinder"}],"source_content_type":"text/x-python","patch_set":6,"id":"6eccb594_a0a1c8b1","line":81,"range":{"start_line":81,"start_character":47,"end_line":81,"end_character":58},"in_reply_to":"0a8d4d1d_351efba4","updated":"2023-08-31 06:39:11.000000000","message":"Done","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5e92f5545f2bef75fec360e6fa50f0408d10e547","unresolved":true,"context_lines":[{"line_number":92,"context_line":"        # TODO(auniyal): Reboot should remove stale attachments"},{"line_number":93,"context_line":"        # after fix bdm should not have any volume"},{"line_number":94,"context_line":"        self.assertEqual(1, len(bdm_list))"},{"line_number":95,"context_line":"        self.assertEqual(volume_id, bdm_list[0][0])"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"    def test_delete_multiple_stale_attachment_from_nova(self):"},{"line_number":98,"context_line":"        volumes \u003d ["}],"source_content_type":"text/x-python","patch_set":6,"id":"b53cd21f_5b386470","line":95,"updated":"2023-08-30 18:01:05.000000000","message":"so i dont think this is correct.\n\nwe only want to remove the bdm if the voluem is deelted.\nwe are not trying to support volume detach by manually deleting the attachments in cidner.\n\nwere the voluem still exits and is in the bdm and is avaiable im not sure we shoudl eb deleteign the bdm on reboot.\n\nit woudl be more correct to recreate the attachment in cinder in that case.\n\nthe nova bdms are the souce of truth for what volumes shoudl be attached to an instnace.\n\n\n@sylvain is there a reason why we are going to delete the bdm.\n\ni know we cant just recreat the attachment if the volume has been attached to another instnace in the intervinging time but here the volume still exists so this feels like backdooring a feature to allow you to detach volumes form cinder.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"771a39f09cc5a64a01c4a0079396088ee557ac66","unresolved":true,"context_lines":[{"line_number":92,"context_line":"        # TODO(auniyal): Reboot should remove stale attachments"},{"line_number":93,"context_line":"        # after fix bdm should not have any volume"},{"line_number":94,"context_line":"        self.assertEqual(1, len(bdm_list))"},{"line_number":95,"context_line":"        self.assertEqual(volume_id, bdm_list[0][0])"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"    def test_delete_multiple_stale_attachment_from_nova(self):"},{"line_number":98,"context_line":"        volumes \u003d ["}],"source_content_type":"text/x-python","patch_set":6,"id":"f8f1b7cf_241f686b","line":95,"in_reply_to":"b53cd21f_5b386470","updated":"2023-08-30 19:39:04.000000000","message":"my -1 was because of this but after talking to dansmith abotu it i guess im ok with it in the context of the recent service token changes in cinder.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"8121d7743fd7f17a0275d3580fdc4b0561c9c5a8","unresolved":true,"context_lines":[{"line_number":92,"context_line":"        # TODO(auniyal): Reboot should remove stale attachments"},{"line_number":93,"context_line":"        # after fix bdm should not have any volume"},{"line_number":94,"context_line":"        self.assertEqual(1, len(bdm_list))"},{"line_number":95,"context_line":"        self.assertEqual(volume_id, bdm_list[0][0])"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"    def test_delete_multiple_stale_attachment_from_nova(self):"},{"line_number":98,"context_line":"        volumes \u003d ["}],"source_content_type":"text/x-python","patch_set":6,"id":"a51c6a7f_2736af55","line":95,"in_reply_to":"f8f1b7cf_241f686b","updated":"2023-08-31 06:39:11.000000000","message":"ack, thanks.\n\nfor other reviewers: what we are doing at cinder side:\nissue is volume is deleted or not available but nova thinks it is present because it knows about attachment which is present in cinder DB.\n\nwe are only deleting the attachment of the target instance, for which we do not find attached volumes.\nwe are not detaching volume which we are attached to or not aware of.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5e92f5545f2bef75fec360e6fa50f0408d10e547","unresolved":true,"context_lines":[{"line_number":148,"context_line":""},{"line_number":149,"context_line":"        attachments \u003d 4"},{"line_number":150,"context_line":"        # create stale attachments at cinder"},{"line_number":151,"context_line":"        self._create_vol_attachments_by_cinder(volume_id, server, attachments)"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"        # verify if volume attachment created at cinder"},{"line_number":154,"context_line":"        # get attachment id by server from cinder"}],"source_content_type":"text/x-python","patch_set":6,"id":"a9ed82d1_76342478","line":151,"updated":"2023-08-30 18:01:05.000000000","message":"so here your faking that something else has created attachment for the instnace that were not created by nova. i.e. ironic.\nagain this is unsupproted but this is replicating the broken state.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a31114920606813b8d91c7f3ca728f36c738eec6","unresolved":false,"context_lines":[{"line_number":148,"context_line":""},{"line_number":149,"context_line":"        attachments \u003d 4"},{"line_number":150,"context_line":"        # create stale attachments at cinder"},{"line_number":151,"context_line":"        self._create_vol_attachments_by_cinder(volume_id, server, attachments)"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"        # verify if volume attachment created at cinder"},{"line_number":154,"context_line":"        # get attachment id by server from cinder"}],"source_content_type":"text/x-python","patch_set":6,"id":"bbffd43c_23f70e5c","line":151,"in_reply_to":"a9ed82d1_76342478","updated":"2023-08-31 11:07:49.000000000","message":"Ack","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5e92f5545f2bef75fec360e6fa50f0408d10e547","unresolved":true,"context_lines":[{"line_number":177,"context_line":"        # verify if how many volume attachments are present at cinder"},{"line_number":178,"context_line":"        attached_volume_ids \u003d self.cinder.attachment_ids_for_instance("},{"line_number":179,"context_line":"            server[\u0027id\u0027])"},{"line_number":180,"context_line":"        self.assertEqual(attachments + 1, len(attached_volume_ids))"}],"source_content_type":"text/x-python","patch_set":6,"id":"532cd234_231f0b32","line":180,"updated":"2023-08-30 18:01:05.000000000","message":"ack.","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a31114920606813b8d91c7f3ca728f36c738eec6","unresolved":false,"context_lines":[{"line_number":177,"context_line":"        # verify if how many volume attachments are present at cinder"},{"line_number":178,"context_line":"        attached_volume_ids \u003d self.cinder.attachment_ids_for_instance("},{"line_number":179,"context_line":"            server[\u0027id\u0027])"},{"line_number":180,"context_line":"        self.assertEqual(attachments + 1, len(attached_volume_ids))"}],"source_content_type":"text/x-python","patch_set":6,"id":"f013411e_9c638e79","line":180,"in_reply_to":"532cd234_231f0b32","updated":"2023-08-31 11:07:49.000000000","message":"Ack","commit_id":"0c97b6524b57289ca45a3d90bef091cd3d3bd5f9"}],"nova/tests/functional/integrated_helpers.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a31114920606813b8d91c7f3ca728f36c738eec6","unresolved":true,"context_lines":[{"line_number":641,"context_line":"            {\u0027createImage\u0027: {\u0027name\u0027: snapshot_name}}"},{"line_number":642,"context_line":"        )"},{"line_number":643,"context_line":""},{"line_number":644,"context_line":"    def _attach_volumes(self, server, vol_ids: ty.List[str]):"},{"line_number":645,"context_line":"        # attach volumes to server"},{"line_number":646,"context_line":"        # these attachments are done by nova api, that means"},{"line_number":647,"context_line":"        # nova know about these attachments and so they are valid ones."}],"source_content_type":"text/x-python","patch_set":7,"id":"0e25d9c9_6c01040f","line":644,"updated":"2023-08-31 11:07:49.000000000","message":"i guess this will do\ni had hpped you woudl add _attach_volume and then call that in a loop form\n\n_attach_volumes in the other class but this is fine i guess.\n\nwe can add _attach_volume at a later date.","commit_id":"c33a9ccf4c2c996f5bb2e43fb6107072a9d2c66e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a31114920606813b8d91c7f3ca728f36c738eec6","unresolved":true,"context_lines":[{"line_number":657,"context_line":"        # here server is already in an active state"},{"line_number":658,"context_line":"        # this is just to refresh server object"},{"line_number":659,"context_line":"        # so attached volumes can be shown in server"},{"line_number":660,"context_line":"        return self._wait_for_state_change(server, expected_status\u003d\u0027ACTIVE\u0027)"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"    def _create_vol_attachments_by_cinder("},{"line_number":663,"context_line":"            self, volume_id, server, new_attachments\u003d1):"}],"source_content_type":"text/x-python","patch_set":7,"id":"e3ab11a7_eafddeee","line":660,"updated":"2023-08-31 11:07:49.000000000","message":"you can fix this later but you should not use the sideefect fo _wait_for_state_change to just get a new server object.\n\nthe correct way to do this is to do server show.\nwe may want to resue this funciton to attach volume to shutoff instances so we should not be asserting active as it makes this function less reusable.\n\nthis can be fixed in a followup to move this forward but _wait_for_state_change shoudl only be used when you are waiting for a state change.\n\nthis should just be \"return self.api.get_server(server[\u0027id\u0027])\"","commit_id":"c33a9ccf4c2c996f5bb2e43fb6107072a9d2c66e"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"5d83d91ecc2ec2872657b2a8f50a46127a18a931","unresolved":false,"context_lines":[{"line_number":657,"context_line":"        # here server is already in an active state"},{"line_number":658,"context_line":"        # this is just to refresh server object"},{"line_number":659,"context_line":"        # so attached volumes can be shown in server"},{"line_number":660,"context_line":"        return self._wait_for_state_change(server, expected_status\u003d\u0027ACTIVE\u0027)"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"    def _create_vol_attachments_by_cinder("},{"line_number":663,"context_line":"            self, volume_id, server, new_attachments\u003d1):"}],"source_content_type":"text/x-python","patch_set":7,"id":"48a7a014_a17f38d4","line":660,"in_reply_to":"e3ab11a7_eafddeee","updated":"2023-09-04 07:40:59.000000000","message":"Ack","commit_id":"c33a9ccf4c2c996f5bb2e43fb6107072a9d2c66e"}]}