)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"3da3b2fabe6a802dd3223db219f877d7f08f9e65","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Lightos connector - refactor disconnect volume"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Disconnect volume have the following actions:"},{"line_number":10,"context_line":"- flush the device"},{"line_number":11,"context_line":"- update queue"},{"line_number":12,"context_line":"- remove discovery client file"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"d504892b_c8894579","line":9,"updated":"2022-02-02 14:07:31.000000000","message":"has","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"1338ce5a51181f8b8c6005ba920a556bd418a356","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Lightos connector - refactor disconnect volume"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Disconnect volume have the following actions:"},{"line_number":10,"context_line":"- flush the device"},{"line_number":11,"context_line":"- update queue"},{"line_number":12,"context_line":"- remove discovery client file"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1b905ba1_fbe82bdd","line":9,"in_reply_to":"d504892b_c8894579","updated":"2022-02-03 09:59:49.000000000","message":"Done","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"3da3b2fabe6a802dd3223db219f877d7f08f9e65","unresolved":true,"context_lines":[{"line_number":11,"context_line":"- update queue"},{"line_number":12,"context_line":"- remove discovery client file"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The actual detach happens in the cinder driver when the ACL is changed."},{"line_number":15,"context_line":"in case we want to make sure the volume reach the cinder driver we can"},{"line_number":16,"context_line":"run it with \"ignore_errors\" flag active."},{"line_number":17,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"f0814c5f_de3db269","line":14,"updated":"2022-02-02 14:07:31.000000000","message":"this is not entirely accurate. I would phrase the commit message as follows:\n\nDisconnect volume needs to perform the following operations:\n1. Update our connection DB that the volume has been disconnected\n2. Flush any pending I/Os on the device file\n3. Update the discovery-client that the volume has been disconnected.\n\nDo these operations in order and ignore the force flag as other connectors appear to do (e.g., nvmeof).","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"1338ce5a51181f8b8c6005ba920a556bd418a356","unresolved":false,"context_lines":[{"line_number":11,"context_line":"- update queue"},{"line_number":12,"context_line":"- remove discovery client file"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The actual detach happens in the cinder driver when the ACL is changed."},{"line_number":15,"context_line":"in case we want to make sure the volume reach the cinder driver we can"},{"line_number":16,"context_line":"run it with \"ignore_errors\" flag active."},{"line_number":17,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"944761a8_276e910f","line":14,"in_reply_to":"f0814c5f_de3db269","updated":"2022-02-03 09:59:49.000000000","message":"Done","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"3da3b2fabe6a802dd3223db219f877d7f08f9e65","unresolved":true,"context_lines":[{"line_number":18,"context_line":"Removing the force flag and replacing it with try:except block"},{"line_number":19,"context_line":"using the \"ignore_errors\" flag"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: I5cd5456c4cc3e415fd08b678cc57fc794dc0621b"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"92b681ca_27689d3f","line":21,"updated":"2022-02-02 14:07:31.000000000","message":"Missing Signed-Off-By:","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"1338ce5a51181f8b8c6005ba920a556bd418a356","unresolved":true,"context_lines":[{"line_number":18,"context_line":"Removing the force flag and replacing it with try:except block"},{"line_number":19,"context_line":"using the \"ignore_errors\" flag"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: I5cd5456c4cc3e415fd08b678cc57fc794dc0621b"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"e8f28bc5_9e2582c6","line":21,"in_reply_to":"92b681ca_27689d3f","updated":"2022-02-03 09:59:49.000000000","message":"still missing :-)","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"eed67928a61b8f4fc489f25d858bdf2c7d3c4d6b","unresolved":false,"context_lines":[{"line_number":18,"context_line":"Removing the force flag and replacing it with try:except block"},{"line_number":19,"context_line":"using the \"ignore_errors\" flag"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: I5cd5456c4cc3e415fd08b678cc57fc794dc0621b"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"72738d66_2ee3df8c","line":21,"in_reply_to":"e8f28bc5_9e2582c6","updated":"2022-02-04 08:58:30.000000000","message":"Done","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"d187b7535d3649c2d053154c89f178f3e9a9fc78","unresolved":true,"context_lines":[{"line_number":11,"context_line":"2. Flush any pending I/Os on the device file"},{"line_number":12,"context_line":"3. Update the discovery-client that the volume has been disconnected."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"If force is True we avoid raising any error to reach the cinder ACL"},{"line_number":15,"context_line":"update."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Signed-off-by: Yuval Brave  \u003cyuval@lightbitslabs.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5b5d9814_5cfca113","line":14,"updated":"2022-02-11 09:03:27.000000000","message":"Comment here is no longer accurate. We can just remove it.","commit_id":"333d31b8021026c8c7635706f12244a422e11622"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"5a3169738b8a1f26745e2d61820bea043b5f3b33","unresolved":false,"context_lines":[{"line_number":11,"context_line":"2. Flush any pending I/Os on the device file"},{"line_number":12,"context_line":"3. Update the discovery-client that the volume has been disconnected."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"If force is True we avoid raising any error to reach the cinder ACL"},{"line_number":15,"context_line":"update."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Signed-off-by: Yuval Brave  \u003cyuval@lightbitslabs.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"af3a36d2_51eac42a","line":14,"in_reply_to":"5b5d9814_5cfca113","updated":"2022-02-11 11:59:25.000000000","message":"Done","commit_id":"333d31b8021026c8c7635706f12244a422e11622"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"3da3b2fabe6a802dd3223db219f877d7f08f9e65","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"96fa3c61_82b22f58","updated":"2022-02-02 14:07:31.000000000","message":"Thanks for handling this quickly Yuval. I think we need something a bit more involved, see inline comments.","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":29122,"name":"Raghavendra Tilay","email":"raghavendra-uddhav.tilay@hpe.com","username":"raghavendrat"},"change_message_id":"e63b67bfe34ca794bd36d2fa9c80b1c98a16f751","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"20325882_33ce7db6","updated":"2022-02-10 14:39:44.000000000","message":"Ack. Thanks Brian.","commit_id":"66d4d7a662350f2573a017b86e213e63b1c4d14c"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"d9681dddb14071efe2e54b7a4acc5a67a3c11151","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"21221acd_698f5d35","updated":"2022-02-10 13:26:43.000000000","message":"Answer to Raghavendra\u0027s question inline.","commit_id":"66d4d7a662350f2573a017b86e213e63b1c4d14c"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"eed67928a61b8f4fc489f25d858bdf2c7d3c4d6b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"c6c1c8c7_c3e0cdd5","updated":"2022-02-04 08:58:30.000000000","message":"LGTM and it would be great to get this merged so we can make progress on the nova side of things (requires an os-brick release).","commit_id":"66d4d7a662350f2573a017b86e213e63b1c4d14c"},{"author":{"_account_id":29122,"name":"Raghavendra Tilay","email":"raghavendra-uddhav.tilay@hpe.com","username":"raghavendrat"},"change_message_id":"4968512e6e8cb3dd8ea8adbedec2ecc624bfa0fd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"b0a2baf9_4a24c009","updated":"2022-02-10 12:35:59.000000000","message":"Minor query","commit_id":"66d4d7a662350f2573a017b86e213e63b1c4d14c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"5ee603bac368def9effbd3111bfb6ee792d79f30","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"1598f21b_5db2459e","updated":"2022-02-10 15:16:17.000000000","message":"Sorry, I forgot my comment on draft","commit_id":"66d4d7a662350f2573a017b86e213e63b1c4d14c"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"ab8e10529c9ec7ea9d3985d28833288cc9049f05","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"f0917f9b_9aeb4323","updated":"2022-02-11 09:02:15.000000000","message":"recheck","commit_id":"333d31b8021026c8c7635706f12244a422e11622"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"14353495ebc33ac4b7029802ea926ba442f8ae19","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"e47c92ad_dd7ceb3a","updated":"2022-02-14 15:32:19.000000000","message":"One comment inline","commit_id":"b0356db3d527dd5e5239adb92e3f26d0f71d1d70"},{"author":{"_account_id":29122,"name":"Raghavendra Tilay","email":"raghavendra-uddhav.tilay@hpe.com","username":"raghavendrat"},"change_message_id":"9d7cc925eae7d9e0d388b951a7fba3e5766a1a82","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"63c53c45_2e708696","updated":"2022-02-11 12:18:09.000000000","message":"The flag \"ignore_errors\" is now being used.","commit_id":"b0356db3d527dd5e5239adb92e3f26d0f71d1d70"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"276bea66d96c731f14a77692d20636478de1b5b9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"e6d44a3f_27c73243","updated":"2022-02-12 19:14:35.000000000","message":"This looks consistent with how we handle disconnect in the other connectors.  Zuul and third-party CI are green.","commit_id":"b0356db3d527dd5e5239adb92e3f26d0f71d1d70"},{"author":{"_account_id":7198,"name":"Jay Bryant","email":"jungleboyj@electronicjungle.net","username":"jsbryant"},"change_message_id":"ac63d948b6df03dc4cb8101a47ee85c6fc1bf8f9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"9a2e8666_9364b893","updated":"2022-02-14 15:22:37.000000000","message":"This looks like comments have been addressed and should be ok.","commit_id":"b0356db3d527dd5e5239adb92e3f26d0f71d1d70"}],"os_brick/initiator/connectors/lightos.py":[{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"3da3b2fabe6a802dd3223db219f877d7f08f9e65","unresolved":true,"context_lines":[{"line_number":313,"context_line":"        # bookkeeping lightos connections - delete connection"},{"line_number":314,"context_line":"        if self.message_queue:"},{"line_number":315,"context_line":"            self.message_queue.put((\u0027delete\u0027, connection_properties))"},{"line_number":316,"context_line":""},{"line_number":317,"context_line":"        uuid \u003d connection_properties[\u0027uuid\u0027]"},{"line_number":318,"context_line":"        LOG.debug(\u0027LIGHTOS: disconnect_volume called for volume %s\u0027, uuid)"},{"line_number":319,"context_line":"        device_path \u003d self._get_device_by_uuid(uuid)"}],"source_content_type":"text/x-python","patch_set":1,"id":"cf370afd_60e83f53","line":316,"updated":"2022-02-02 14:07:31.000000000","message":"+1","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"1338ce5a51181f8b8c6005ba920a556bd418a356","unresolved":false,"context_lines":[{"line_number":313,"context_line":"        # bookkeeping lightos connections - delete connection"},{"line_number":314,"context_line":"        if self.message_queue:"},{"line_number":315,"context_line":"            self.message_queue.put((\u0027delete\u0027, connection_properties))"},{"line_number":316,"context_line":""},{"line_number":317,"context_line":"        uuid \u003d connection_properties[\u0027uuid\u0027]"},{"line_number":318,"context_line":"        LOG.debug(\u0027LIGHTOS: disconnect_volume called for volume %s\u0027, uuid)"},{"line_number":319,"context_line":"        device_path \u003d self._get_device_by_uuid(uuid)"}],"source_content_type":"text/x-python","patch_set":1,"id":"e6d78954_f5dfcdb8","line":316,"in_reply_to":"cf370afd_60e83f53","updated":"2022-02-03 09:59:49.000000000","message":"Done","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79cfb434a503949cce79ccec57a360da79307025","unresolved":true,"context_lines":[{"line_number":321,"context_line":"            if device_path:"},{"line_number":322,"context_line":"                self._linuxscsi.flush_device_io(device_path)"},{"line_number":323,"context_line":"        except putils.ProcessExecutionError:"},{"line_number":324,"context_line":"            if not ignore_errors:"},{"line_number":325,"context_line":"                raise"},{"line_number":326,"context_line":"        try:"},{"line_number":327,"context_line":"            self.dsc_disconnect_volume(connection_properties)"}],"source_content_type":"text/x-python","patch_set":1,"id":"a09dcef1_30dc05f7","line":324,"updated":"2022-02-02 15:08:50.000000000","message":"-1: This is not technically correct, because if force is True then we should continue with the dsc_disconnect_volume regardless, and only after it if we failed the flush and ignore_errors is not True should we raise the exception. \n\n        :param force: Whether to forcefully disconnect even if flush fails.\n        :param ignore_errors: When force is True, this will decide whether to\n                              ignore errors or raise an exception once finished\n                              the operation.  Default is False.","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"b3c6d6d2e42ab70515e7c9a4519593fcd2807606","unresolved":true,"context_lines":[{"line_number":321,"context_line":"            if device_path:"},{"line_number":322,"context_line":"                self._linuxscsi.flush_device_io(device_path)"},{"line_number":323,"context_line":"        except putils.ProcessExecutionError:"},{"line_number":324,"context_line":"            if not ignore_errors:"},{"line_number":325,"context_line":"                raise"},{"line_number":326,"context_line":"        try:"},{"line_number":327,"context_line":"            self.dsc_disconnect_volume(connection_properties)"}],"source_content_type":"text/x-python","patch_set":1,"id":"c545d05a_937f1a7f","line":324,"in_reply_to":"a09dcef1_30dc05f7","updated":"2022-02-02 15:32:47.000000000","message":"Gorka, just to make sure - we checked nvmeof.py connector (closest to the LightOS connector) and it completely ignores force. Is force actually used/passed by upper levels and we should follow the comment you quote above verbatim?","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"1338ce5a51181f8b8c6005ba920a556bd418a356","unresolved":false,"context_lines":[{"line_number":321,"context_line":"            if device_path:"},{"line_number":322,"context_line":"                self._linuxscsi.flush_device_io(device_path)"},{"line_number":323,"context_line":"        except putils.ProcessExecutionError:"},{"line_number":324,"context_line":"            if not ignore_errors:"},{"line_number":325,"context_line":"                raise"},{"line_number":326,"context_line":"        try:"},{"line_number":327,"context_line":"            self.dsc_disconnect_volume(connection_properties)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f1bb53c_7f04a124","line":324,"in_reply_to":"c545d05a_937f1a7f","updated":"2022-02-03 09:59:49.000000000","message":"Done","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"3da3b2fabe6a802dd3223db219f877d7f08f9e65","unresolved":true,"context_lines":[{"line_number":327,"context_line":"            self.dsc_disconnect_volume(connection_properties)"},{"line_number":328,"context_line":"        except Exception:"},{"line_number":329,"context_line":"            if not ignore_errors:"},{"line_number":330,"context_line":"                raise"},{"line_number":331,"context_line":""},{"line_number":332,"context_line":"    @utils.trace"},{"line_number":333,"context_line":"    @synchronized(\u0027volume_op\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"4feb8eb8_e0bb3fd8","line":330,"updated":"2022-02-02 14:07:31.000000000","message":"In some cases, e.g., when \u0027blockdev\u0027 isn\u0027t found, this can leave us with dangling dsc files. I think we should remove the dsc file regardless of any errors thrown by flush_device_io. So we have two options: either we remember those errors and raise them if ignore_errors is False, but only after having removed the dsc file, or we just ignore any errors thrown by flush_device_io, which I think is the more reasonable approach.","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"1338ce5a51181f8b8c6005ba920a556bd418a356","unresolved":true,"context_lines":[{"line_number":327,"context_line":"            self.dsc_disconnect_volume(connection_properties)"},{"line_number":328,"context_line":"        except Exception:"},{"line_number":329,"context_line":"            if not ignore_errors:"},{"line_number":330,"context_line":"                raise"},{"line_number":331,"context_line":""},{"line_number":332,"context_line":"    @utils.trace"},{"line_number":333,"context_line":"    @synchronized(\u0027volume_op\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"54c21bd5_37fb5cd1","line":330,"in_reply_to":"3176c3d0_7b3e9acc","updated":"2022-02-03 09:59:49.000000000","message":"We narrowed down the behavior to:\nif force is False - raise errors as usual\nif force is True - don\u0027t raise errors so that cinder will do the ACL update. Errors here means either a problem with the flush (e.g., blockdev is not found) or with deleting the dsc file (RO FS?) and neither justifies not updating the ACL to remove the access to the volume, i.e., raising errors from the connector. This has the unfortunate effect of ignoring the \u0027ignore_errors\u0027 flag but it does provide the expected behavior.","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79cfb434a503949cce79ccec57a360da79307025","unresolved":true,"context_lines":[{"line_number":327,"context_line":"            self.dsc_disconnect_volume(connection_properties)"},{"line_number":328,"context_line":"        except Exception:"},{"line_number":329,"context_line":"            if not ignore_errors:"},{"line_number":330,"context_line":"                raise"},{"line_number":331,"context_line":""},{"line_number":332,"context_line":"    @utils.trace"},{"line_number":333,"context_line":"    @synchronized(\u0027volume_op\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"3176c3d0_7b3e9acc","line":330,"in_reply_to":"4feb8eb8_e0bb3fd8","updated":"2022-02-02 15:08:50.000000000","message":"That is what should happen if flush fails and force is True, but if force is False and ignore_errors is False as well then in theory we should leave the \"dangling dsc file\", because Nova shouldn\u0027t be calling Cinder to terminate the connection.","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"5ee603bac368def9effbd3111bfb6ee792d79f30","unresolved":true,"context_lines":[{"line_number":327,"context_line":"            self.dsc_disconnect_volume(connection_properties)"},{"line_number":328,"context_line":"        except Exception:"},{"line_number":329,"context_line":"            if not ignore_errors:"},{"line_number":330,"context_line":"                raise"},{"line_number":331,"context_line":""},{"line_number":332,"context_line":"    @utils.trace"},{"line_number":333,"context_line":"    @synchronized(\u0027volume_op\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"74c0ad0e_7657a13b","line":330,"in_reply_to":"54c21bd5_37fb5cd1","updated":"2022-02-10 15:16:17.000000000","message":"That\u0027s not the expected behavior.\n\nAs defined in the docstring these are two independent flags. Raising exceptions is controlled by the \"ignore_errors\", and it happens once the method finishes.  When it finishes depends on the \"force\" flag and whether there were errors or not.\n\nIf there are no error or if \"force\" is set then the method finishes once it has run all the commands, and if there are errors and \"force\" is not set then the method finishes once it encounters the first error.\n\nYou can look at the iSCSI connector for ideas of how to implement this using the `ExceptionChainer` class: https://github.com/openstack/os-brick/blob/e761ab64230faf9b210a1457ceca592a51202fd6/os_brick/initiator/connectors/iscsi.py#L915","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"92ae37b7b704b21ac5dc38de6d5c8cea2da48d02","unresolved":false,"context_lines":[{"line_number":327,"context_line":"            self.dsc_disconnect_volume(connection_properties)"},{"line_number":328,"context_line":"        except Exception:"},{"line_number":329,"context_line":"            if not ignore_errors:"},{"line_number":330,"context_line":"                raise"},{"line_number":331,"context_line":""},{"line_number":332,"context_line":"    @utils.trace"},{"line_number":333,"context_line":"    @synchronized(\u0027volume_op\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"dbd42b57_179b2af1","line":330,"in_reply_to":"74c0ad0e_7657a13b","updated":"2022-02-11 08:58:56.000000000","message":"Done","commit_id":"5cdc4d04ac5d8fd6916caeb4c4ff6113c463dc05"},{"author":{"_account_id":29122,"name":"Raghavendra Tilay","email":"raghavendra-uddhav.tilay@hpe.com","username":"raghavendrat"},"change_message_id":"4968512e6e8cb3dd8ea8adbedec2ecc624bfa0fd","unresolved":true,"context_lines":[{"line_number":293,"context_line":"    @utils.trace"},{"line_number":294,"context_line":"    @synchronized(\u0027volume_op\u0027)"},{"line_number":295,"context_line":"    def disconnect_volume(self, connection_properties, device_info,"},{"line_number":296,"context_line":"                          force\u003dFalse, ignore_errors\u003dFalse):"},{"line_number":297,"context_line":"        \"\"\"Disconnect a volume from the local host."},{"line_number":298,"context_line":""},{"line_number":299,"context_line":"        The connection_properties are the same as from connect_volume."}],"source_content_type":"text/x-python","patch_set":4,"id":"72295b55_1ed0dbd0","line":296,"range":{"start_line":296,"start_character":39,"end_line":296,"end_character":58},"updated":"2022-02-10 12:35:59.000000000","message":"In code of patchset 4, this variable ignore_errors isn\u0027t used at all.\nSo, can this variable be removed?","commit_id":"66d4d7a662350f2573a017b86e213e63b1c4d14c"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"d9681dddb14071efe2e54b7a4acc5a67a3c11151","unresolved":true,"context_lines":[{"line_number":293,"context_line":"    @utils.trace"},{"line_number":294,"context_line":"    @synchronized(\u0027volume_op\u0027)"},{"line_number":295,"context_line":"    def disconnect_volume(self, connection_properties, device_info,"},{"line_number":296,"context_line":"                          force\u003dFalse, ignore_errors\u003dFalse):"},{"line_number":297,"context_line":"        \"\"\"Disconnect a volume from the local host."},{"line_number":298,"context_line":""},{"line_number":299,"context_line":"        The connection_properties are the same as from connect_volume."}],"source_content_type":"text/x-python","patch_set":4,"id":"9b44df9f_ecb791bf","line":296,"range":{"start_line":296,"start_character":39,"end_line":296,"end_character":58},"in_reply_to":"72295b55_1ed0dbd0","updated":"2022-02-10 13:26:43.000000000","message":"Good question, but this function is a member of the initiator connector interface [0], so it needs to respect the function signature.  So we should ask your question in the other direction: why is this parameter that\u0027s part of the interface not being used in this function?\n\n[0] see os_brick.initiator.initiator_connector.InitiatorConnector","commit_id":"66d4d7a662350f2573a017b86e213e63b1c4d14c"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"92ae37b7b704b21ac5dc38de6d5c8cea2da48d02","unresolved":false,"context_lines":[{"line_number":293,"context_line":"    @utils.trace"},{"line_number":294,"context_line":"    @synchronized(\u0027volume_op\u0027)"},{"line_number":295,"context_line":"    def disconnect_volume(self, connection_properties, device_info,"},{"line_number":296,"context_line":"                          force\u003dFalse, ignore_errors\u003dFalse):"},{"line_number":297,"context_line":"        \"\"\"Disconnect a volume from the local host."},{"line_number":298,"context_line":""},{"line_number":299,"context_line":"        The connection_properties are the same as from connect_volume."}],"source_content_type":"text/x-python","patch_set":4,"id":"014350ed_739bb541","line":296,"range":{"start_line":296,"start_character":39,"end_line":296,"end_character":58},"in_reply_to":"9b44df9f_ecb791bf","updated":"2022-02-11 08:58:56.000000000","message":"I miss understood the \"ignore_errors\" flag - fixed","commit_id":"66d4d7a662350f2573a017b86e213e63b1c4d14c"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"d187b7535d3649c2d053154c89f178f3e9a9fc78","unresolved":false,"context_lines":[{"line_number":331,"context_line":"            exc.add_exception(type(e), e, traceback.format_exc())"},{"line_number":332,"context_line":"        if exc:"},{"line_number":333,"context_line":"            if not ignore_errors:"},{"line_number":334,"context_line":"                raise exc"},{"line_number":335,"context_line":""},{"line_number":336,"context_line":"    @utils.trace"},{"line_number":337,"context_line":"    @synchronized(\u0027volume_op\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"cdd79e82_a74ea2f8","line":334,"updated":"2022-02-11 09:03:27.000000000","message":"Logic checks out, thanks for taking care of this Yuval!","commit_id":"333d31b8021026c8c7635706f12244a422e11622"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"14353495ebc33ac4b7029802ea926ba442f8ae19","unresolved":true,"context_lines":[{"line_number":318,"context_line":"        LOG.debug(\u0027LIGHTOS: disconnect_volume called for volume %s\u0027, uuid)"},{"line_number":319,"context_line":"        device_path \u003d self._get_device_by_uuid(uuid)"},{"line_number":320,"context_line":"        exc \u003d exception.ExceptionChainer()"},{"line_number":321,"context_line":"        try:"},{"line_number":322,"context_line":"            if device_path:"},{"line_number":323,"context_line":"                self._linuxscsi.flush_device_io(device_path)"},{"line_number":324,"context_line":"        except putils.ProcessExecutionError as e:"},{"line_number":325,"context_line":"            exc.add_exception(type(e), e, traceback.format_exc())"},{"line_number":326,"context_line":"            if not (force or ignore_errors):"},{"line_number":327,"context_line":"                raise"},{"line_number":328,"context_line":"        try:"},{"line_number":329,"context_line":"            self.dsc_disconnect_volume(connection_properties)"},{"line_number":330,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"0a00d69c_e2caa04d","line":327,"range":{"start_line":321,"start_character":0,"end_line":327,"end_character":21},"updated":"2022-02-14 15:32:19.000000000","message":"Instead of the try except block, can we use the ExceptionChainer as a context manager and perform the flush operation under it since the __exit__ method already calls add_exception[1]\nSimilar to this[2]\n\n[1] https://github.com/openstack/os-brick/blob/master/os_brick/exception.py#L229\n[2] https://github.com/openstack/os-brick/blob/13516073d54058807432cd7dda88fb6ffc367c6e/os_brick/initiator/linuxscsi.py#L75-L76","commit_id":"b0356db3d527dd5e5239adb92e3f26d0f71d1d70"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"fd8b2f2019c9e493191afd6eaf05c2875d0aa81d","unresolved":true,"context_lines":[{"line_number":318,"context_line":"        LOG.debug(\u0027LIGHTOS: disconnect_volume called for volume %s\u0027, uuid)"},{"line_number":319,"context_line":"        device_path \u003d self._get_device_by_uuid(uuid)"},{"line_number":320,"context_line":"        exc \u003d exception.ExceptionChainer()"},{"line_number":321,"context_line":"        try:"},{"line_number":322,"context_line":"            if device_path:"},{"line_number":323,"context_line":"                self._linuxscsi.flush_device_io(device_path)"},{"line_number":324,"context_line":"        except putils.ProcessExecutionError as e:"},{"line_number":325,"context_line":"            exc.add_exception(type(e), e, traceback.format_exc())"},{"line_number":326,"context_line":"            if not (force or ignore_errors):"},{"line_number":327,"context_line":"                raise"},{"line_number":328,"context_line":"        try:"},{"line_number":329,"context_line":"            self.dsc_disconnect_volume(connection_properties)"},{"line_number":330,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"2712b2cc_1bb28558","line":327,"range":{"start_line":321,"start_character":0,"end_line":327,"end_character":21},"in_reply_to":"0a00d69c_e2caa04d","updated":"2022-02-14 16:18:19.000000000","message":"This is more readable for me - any special reason to use context manager?","commit_id":"b0356db3d527dd5e5239adb92e3f26d0f71d1d70"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"d972f106a11d01c8a9f62e47603b5c37dbc2a9a7","unresolved":true,"context_lines":[{"line_number":318,"context_line":"        LOG.debug(\u0027LIGHTOS: disconnect_volume called for volume %s\u0027, uuid)"},{"line_number":319,"context_line":"        device_path \u003d self._get_device_by_uuid(uuid)"},{"line_number":320,"context_line":"        exc \u003d exception.ExceptionChainer()"},{"line_number":321,"context_line":"        try:"},{"line_number":322,"context_line":"            if device_path:"},{"line_number":323,"context_line":"                self._linuxscsi.flush_device_io(device_path)"},{"line_number":324,"context_line":"        except putils.ProcessExecutionError as e:"},{"line_number":325,"context_line":"            exc.add_exception(type(e), e, traceback.format_exc())"},{"line_number":326,"context_line":"            if not (force or ignore_errors):"},{"line_number":327,"context_line":"                raise"},{"line_number":328,"context_line":"        try:"},{"line_number":329,"context_line":"            self.dsc_disconnect_volume(connection_properties)"},{"line_number":330,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"891ccc95_aa061a06","line":327,"range":{"start_line":321,"start_character":0,"end_line":327,"end_character":21},"in_reply_to":"2712b2cc_1bb28558","updated":"2022-02-14 16:46:11.000000000","message":"The ExceptionChainer class is defined in a way that we don\u0027t have to call add_exception manually and the context manager performs the work for us. Similar to what we do in file handling while opening a file and the context manager ensures that the file is closed in the end. That would be much cleaner and more pythonic way of writing this IMO. Code consistency is another reason since we use it as a context manager throughout os-brick.","commit_id":"b0356db3d527dd5e5239adb92e3f26d0f71d1d70"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"14353495ebc33ac4b7029802ea926ba442f8ae19","unresolved":true,"context_lines":[{"line_number":325,"context_line":"            exc.add_exception(type(e), e, traceback.format_exc())"},{"line_number":326,"context_line":"            if not (force or ignore_errors):"},{"line_number":327,"context_line":"                raise"},{"line_number":328,"context_line":"        try:"},{"line_number":329,"context_line":"            self.dsc_disconnect_volume(connection_properties)"},{"line_number":330,"context_line":"        except Exception as e:"},{"line_number":331,"context_line":"            exc.add_exception(type(e), e, traceback.format_exc())"},{"line_number":332,"context_line":"        if exc:"},{"line_number":333,"context_line":"            if not ignore_errors:"},{"line_number":334,"context_line":"                raise exc"},{"line_number":335,"context_line":""},{"line_number":336,"context_line":"    @utils.trace"},{"line_number":337,"context_line":"    @synchronized(\u0027volume_op\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"542e8402_ffc7227d","line":334,"range":{"start_line":328,"start_character":0,"end_line":334,"end_character":25},"updated":"2022-02-14 15:32:19.000000000","message":"same here","commit_id":"b0356db3d527dd5e5239adb92e3f26d0f71d1d70"}]}