)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"a10de5c1aa3467d3221924660efb0a8128fb1d30","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Since microversion 3.44, we update `attachemnt_update` to return"},{"line_number":10,"context_line":"connector and make volume to \u0027attaching\u0027, and add a new api"},{"line_number":11,"context_line":"`attachment_comlete` to change volume status to \u0027in-use\u0027, that means"},{"line_number":12,"context_line":"the end of attachment is changed to `attachment_complete`."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"But the \u0027attach.end\u0027 notify is still send in attachment_update, which"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_e858028d","line":11,"range":{"start_line":11,"start_character":12,"end_line":11,"end_character":19},"updated":"2018-12-18 10:27:04.000000000","message":"complete","commit_id":"800977809176688caa50583ccb1ad2bf1dd87945"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"a10de5c1aa3467d3221924660efb0a8128fb1d30","unresolved":false,"context_lines":[{"line_number":16,"context_line":"`attachment_complete`."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Change-Id: I86dad7afb52d2d253799b543f484e2d55f6e9e6d"},{"line_number":19,"context_line":"Closes-bug: bug/1808941"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_c8550653","line":19,"range":{"start_line":19,"start_character":0,"end_line":19,"end_character":23},"updated":"2018-12-18 10:27:04.000000000","message":"Closes-Bug: #1808941","commit_id":"800977809176688caa50583ccb1ad2bf1dd87945"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"15e6a2ccf57094703ea93b17a0e1524f7f47a7c6","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Move \u0027attach.end\u0027 notify to attachment_complete"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Since microversion 3.44, we update `attachemnt_update` to return"},{"line_number":10,"context_line":"connector and make volume to \u0027attaching\u0027, and add a new api"},{"line_number":11,"context_line":"`attachment_complete` to change volume status to \u0027in-use\u0027, that means"},{"line_number":12,"context_line":"the end of attachment is changed to `attachment_complete`."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3f79a3b5_d3fccebd","line":9,"range":{"start_line":9,"start_character":36,"end_line":9,"end_character":46},"updated":"2018-12-18 15:54:57.000000000","message":"If you do another update: \"attachment\"","commit_id":"83e40c5b9a0b0006a193ae8d5501ab6a3f6f42c7"}],"cinder/api/v3/attachments.py":[{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"0448881c2d8acdfc9177292961df49e238d11987","unresolved":false,"context_lines":[{"line_number":287,"context_line":"        attachment_ref.save()"},{"line_number":288,"context_line":"        volume_ref.update({\u0027status\u0027: \u0027in-use\u0027, \u0027attach_status\u0027: \u0027attached\u0027})"},{"line_number":289,"context_line":"        volume_ref.save()"},{"line_number":290,"context_line":"        volume_utils.notify_about_volume_usage(context, volume_ref, \"attach.end\")"},{"line_number":291,"context_line":""},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"def create_resource(ext_mgr):"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_4ad247d2","line":290,"range":{"start_line":290,"start_character":79,"end_line":290,"end_character":81},"updated":"2018-12-18 13:57:07.000000000","message":"This is going to cause a pep8 failure. Put the args on a new line so it stays within 80 characters.","commit_id":"800977809176688caa50583ccb1ad2bf1dd87945"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"c9cdbea636c7432c90f49b1b48ab533995f0f1cf","unresolved":false,"context_lines":[{"line_number":287,"context_line":"        attachment_ref.save()"},{"line_number":288,"context_line":"        volume_ref.update({\u0027status\u0027: \u0027in-use\u0027, \u0027attach_status\u0027: \u0027attached\u0027})"},{"line_number":289,"context_line":"        volume_ref.save()"},{"line_number":290,"context_line":"        volume_utils.notify_about_volume_usage(context, volume_ref, \"attach.end\")"},{"line_number":291,"context_line":""},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"def create_resource(ext_mgr):"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_b35272d8","line":290,"range":{"start_line":290,"start_character":79,"end_line":290,"end_character":81},"in_reply_to":"3f79a3b5_4ad247d2","updated":"2019-01-04 01:27:47.000000000","message":"Done","commit_id":"800977809176688caa50583ccb1ad2bf1dd87945"}],"cinder/tests/unit/api/v3/test_attachments.py":[{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"a10de5c1aa3467d3221924660efb0a8128fb1d30","unresolved":false,"context_lines":[{"line_number":362,"context_line":"        self.controller.complete(req, self.attachment1.id,"},{"line_number":363,"context_line":"                                 body\u003dbody)"},{"line_number":364,"context_line":"        # Check notify has been called once"},{"line_number":365,"context_line":"        mock_notify.assert_called_once()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_e86fa22c","line":365,"range":{"start_line":365,"start_character":0,"end_line":365,"end_character":40},"updated":"2018-12-18 10:27:04.000000000","message":"IMO Mocking like [1], would be more specific here\n\n[1] https://github.com/openstack/cinder/blob/master/cinder/tests/unit/volume/test_volume.py#L320-L322","commit_id":"800977809176688caa50583ccb1ad2bf1dd87945"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"224f794f01068045070ff37bd2427347f0ea2c91","unresolved":false,"context_lines":[{"line_number":362,"context_line":"        self.controller.complete(req, self.attachment1.id,"},{"line_number":363,"context_line":"                                 body\u003dbody)"},{"line_number":364,"context_line":"        # Check notify has been called once"},{"line_number":365,"context_line":"        mock_notify.assert_called_once()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_7e3fc183","line":365,"range":{"start_line":365,"start_character":0,"end_line":365,"end_character":40},"in_reply_to":"3f79a3b5_53a67e10","updated":"2018-12-18 16:34:51.000000000","message":"Ok. I was considering something like asserting the status and calling assert_notify_called method separately but this works fine as well in my local env so i\u0027m good with it.","commit_id":"800977809176688caa50583ccb1ad2bf1dd87945"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"e95c776674436048ad2db17caf62d4dabcd3478f","unresolved":false,"context_lines":[{"line_number":362,"context_line":"        self.controller.complete(req, self.attachment1.id,"},{"line_number":363,"context_line":"                                 body\u003dbody)"},{"line_number":364,"context_line":"        # Check notify has been called once"},{"line_number":365,"context_line":"        mock_notify.assert_called_once()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_53a67e10","line":365,"range":{"start_line":365,"start_character":0,"end_line":365,"end_character":40},"in_reply_to":"3f79a3b5_e86fa22c","updated":"2018-12-18 15:53:08.000000000","message":"In L349, I checked the volume status is ‘in-use’ and also the event_suffix, Do you have some other point that we must use the way you mentioned? If not, I prefer left this. :)","commit_id":"800977809176688caa50583ccb1ad2bf1dd87945"}],"cinder/volume/manager.py":[{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"8ebe3bf0a1367ab1c23ff7eba18e0b518f443126","unresolved":false,"context_lines":[{"line_number":4449,"context_line":"                                mode,"},{"line_number":4450,"context_line":"                                False)"},{"line_number":4451,"context_line":"        vref.refresh()"},{"line_number":4452,"context_line":"        attachment_ref.refresh()"},{"line_number":4453,"context_line":"        LOG.info(\"attachment_update completed successfully.\","},{"line_number":4454,"context_line":"                 resource\u003dvref)"},{"line_number":4455,"context_line":"        return connection_info"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfdaf3ff_bcca7117","line":4452,"updated":"2019-01-15 02:53:15.000000000","message":"Hey, @hemna thanks for your feedback.\n\n \u003e This seems a bit odd to put the notification in the API.\n\nYes, before I submited this patch, I also think it seems an unusual way,\nBut I also saw [1], we already had the example of notification in API.\n\nAnd I think the notification mechanisms, is something to notify others\nsome information, but do not limited to have to happened in manager.\n\nIn addition, not only in Cinder, but also Nova has the same notification in API. [2]\n\n \u003e From what I remember in the attach v2 rewrite...\n\nNova calls attachment_update in [3], and calls attachment_complete in [4],\nand after attachment_complete the volume is changed to \u0027in-used\u0027\n\nThere are 2 main reason of proposing this patch:\n1. We should send notification after changing volume status to \u0027in-used\u0027.\nThe notification is a critical mechanism in metering or charging system, and\nwe should notify other system after we complete the all the attachment steps.\n\n2. We should keep the compatiable with attachment v1. \nAs I mentioned in issue description [5][6], in v1, we send the notification after\nthe volume is changed to \u0027in-used\u0027, but current the volume is still \u0027attaching\u0027\nwhen we send the final notificaiton.\n\n[1] https://github.com/openstack/cinder/blob/ee1ac54/cinder/api/v2/volumes.py#L302\n[2] https://github.com/openstack/nova/blob/183c3d5/nova/api/openstack/compute/server_tags.py\n\n[3] https://github.com/openstack/nova/blob/183c3d5f07f6bc73ba6d983158f0f77bbfa673c2/nova/virt/block_device.py#L444\n[4] https://github.com/openstack/nova/blob/183c3d5f07f6bc73ba6d983158f0f77bbfa673c2/nova/virt/block_device.py#L484.\n\n[5] https://launchpad.net/bugs/1808941\n[6] https://review.openstack.org/#/c/493262/3/cinder/volume/manager.py@4378\n\nI hope I could explain it clearly, any question you can raise here again. :) thanks!","commit_id":"283e1c2194b9fda1a6116df29b0644f449a69a76"}]}