)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"0cbdd93d391802fe5dd0ec6ab40f3ee1b1e30642","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     yuval \u003cyuval@lightbitslabs.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2022-02-07 08:27:30 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Followup - Reuse get_host_nqn code"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"- The lightos connectors is making use of the get_host_nqn function in"},{"line_number":10,"context_line":"  nvmeof, in order to reduce code duplication I extracted the function"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"f4fc80b8_b09f26cc","line":7,"updated":"2022-02-07 09:15:13.000000000","message":"Honestly I\u0027m not sure what\u0027s the exact os-brick convention but I suggest changing the subject to \u0027connectors: reuse get_host_nqn between LightOS and nvmeof\u0027.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"01d5c5f8862ee55143eb2230753006932cda9075","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     yuval \u003cyuval@lightbitslabs.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2022-02-07 08:27:30 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Followup - Reuse get_host_nqn code"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"- The lightos connectors is making use of the get_host_nqn function in"},{"line_number":10,"context_line":"  nvmeof, in order to reduce code duplication I extracted the function"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"6aa47e52_acbec664","line":7,"in_reply_to":"7722b3dc_65c99586","updated":"2022-02-08 16:46:13.000000000","message":"Done","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4d24231b4b087e1b123ffe132a15044eb4ea1b1e","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     yuval \u003cyuval@lightbitslabs.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2022-02-07 08:27:30 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Followup - Reuse get_host_nqn code"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"- The lightos connectors is making use of the get_host_nqn function in"},{"line_number":10,"context_line":"  nvmeof, in order to reduce code duplication I extracted the function"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"7722b3dc_65c99586","line":7,"in_reply_to":"f4fc80b8_b09f26cc","updated":"2022-02-07 23:51:00.000000000","message":"I agree with Muli\u0027s suggestion; you can add \"This patch is a followup to change I2e86fa840490.\" before line 20.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"0cbdd93d391802fe5dd0ec6ab40f3ee1b1e30642","unresolved":true,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":"- Lightos tests updated according the changes."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Signed-off-by: Yuval Brave  yuval@lightbitslabs.com"},{"line_number":21,"context_line":"Change-Id: Ia4d8c6fda875b4f4d32d511ca9282ca0fb5d6f12"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"c6144b04_f348c8a3","line":20,"updated":"2022-02-07 09:15:13.000000000","message":"For some reason your SOB is missing \u003c\u003e around the email.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"01d5c5f8862ee55143eb2230753006932cda9075","unresolved":false,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":"- Lightos tests updated according the changes."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Signed-off-by: Yuval Brave  yuval@lightbitslabs.com"},{"line_number":21,"context_line":"Change-Id: Ia4d8c6fda875b4f4d32d511ca9282ca0fb5d6f12"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"7aacf449_ecf9bd96","line":20,"in_reply_to":"c6144b04_f348c8a3","updated":"2022-02-08 16:46:13.000000000","message":"Done","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a45d11aa011af956be2ba357bf67e0f8335a90aa","unresolved":true,"context_lines":[{"line_number":12,"context_line":""},{"line_number":13,"context_line":"- The connection property \"hostnqn\" removed to only use single"},{"line_number":14,"context_line":"  key \"nqn\"."},{"line_number":15,"context_line":"  In the lightos connector there was a use of the key \"nqn\" in a"},{"line_number":16,"context_line":"  different context - so that was changed to \"subsysnqn\"."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"- Lightos tests updated according the changes."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"fee90e87_3754bbb1","line":16,"range":{"start_line":15,"start_character":0,"end_line":16,"end_character":57},"updated":"2022-02-08 15:00:45.000000000","message":"It\u0027s not necessary to change it, since both are the nqn, one is of the storage (present only in the connection_info) and the other is of the consumer (present only in the connector_info).\n\nThis would normally be considered a breaking change, since the connector would not work with older connection information, but since we haven\u0027t released yet we could let it slide.","commit_id":"5ee1197bba03066bd8bb0c3e19b15d5e6591f330"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"409b5e3f47d7cd7665ceec1a21b3410d90b3ce00","unresolved":false,"context_lines":[{"line_number":12,"context_line":""},{"line_number":13,"context_line":"- The connection property \"hostnqn\" removed to only use single"},{"line_number":14,"context_line":"  key \"nqn\"."},{"line_number":15,"context_line":"  In the lightos connector there was a use of the key \"nqn\" in a"},{"line_number":16,"context_line":"  different context - so that was changed to \"subsysnqn\"."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"- Lightos tests updated according the changes."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"d5148480_96d350f4","line":16,"range":{"start_line":15,"start_character":0,"end_line":16,"end_character":57},"in_reply_to":"9891225a_75cda09d","updated":"2022-02-09 10:04:22.000000000","message":"Done","commit_id":"5ee1197bba03066bd8bb0c3e19b15d5e6591f330"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"01d5c5f8862ee55143eb2230753006932cda9075","unresolved":true,"context_lines":[{"line_number":12,"context_line":""},{"line_number":13,"context_line":"- The connection property \"hostnqn\" removed to only use single"},{"line_number":14,"context_line":"  key \"nqn\"."},{"line_number":15,"context_line":"  In the lightos connector there was a use of the key \"nqn\" in a"},{"line_number":16,"context_line":"  different context - so that was changed to \"subsysnqn\"."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"- Lightos tests updated according the changes."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"9891225a_75cda09d","line":16,"range":{"start_line":15,"start_character":0,"end_line":16,"end_character":57},"in_reply_to":"fee90e87_3754bbb1","updated":"2022-02-08 16:46:13.000000000","message":"I\u0027m ok with letting it slide precisely because it hasn\u0027t been included in a release.","commit_id":"5ee1197bba03066bd8bb0c3e19b15d5e6591f330"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"9e3f386553ebfe90be02744cbb72d94cb8e8e91f","unresolved":true,"context_lines":[{"line_number":10,"context_line":"  nvmeof, in order to reduce code duplication I extracted the function"},{"line_number":11,"context_line":"  out of the nvmeof class."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"- The connection property \"hostnqn\" removed to only use single"},{"line_number":14,"context_line":"  key \"nqn\"."},{"line_number":15,"context_line":"  In the lightos connector there was a use of the key \"nqn\" in a"},{"line_number":16,"context_line":"  different context - so that was changed to \"subsysnqn\"."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"60bd2624_870094c2","line":13,"range":{"start_line":13,"start_character":36,"end_line":13,"end_character":43},"updated":"2022-02-14 15:54:14.000000000","message":"is replaced with key \"nqn\"","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"40128013ca51aa30b81df5ebeff798f0bd234ca3","unresolved":true,"context_lines":[{"line_number":10,"context_line":"  nvmeof, in order to reduce code duplication I extracted the function"},{"line_number":11,"context_line":"  out of the nvmeof class."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"- The connection property \"hostnqn\" removed to only use single"},{"line_number":14,"context_line":"  key \"nqn\"."},{"line_number":15,"context_line":"  In the lightos connector there was a use of the key \"nqn\" in a"},{"line_number":16,"context_line":"  different context - so that was changed to \"subsysnqn\"."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"b2f77f5e_ffb612ea","line":13,"range":{"start_line":13,"start_character":36,"end_line":13,"end_character":43},"in_reply_to":"60bd2624_870094c2","updated":"2022-02-14 19:38:18.000000000","message":"its not replaced since \"nqn\" is already exists","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4d24231b4b087e1b123ffe132a15044eb4ea1b1e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"3b5df89a_1e5444f2","updated":"2022-02-07 23:51:00.000000000","message":"A few comments inline.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"409ebb236d399b812c16be90fdc9d7c8c9fdaae2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"81124aa8_bd63ffb8","updated":"2022-02-07 10:51:08.000000000","message":"Lightbits ci fails because it needs https://review.opendev.org/c/openstack/cinder/+/828085\nWe will make an effort to make some flow where we cherry-pick this patch","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"0cbdd93d391802fe5dd0ec6ab40f3ee1b1e30642","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"01557fc5_a91d119d","updated":"2022-02-07 09:15:13.000000000","message":"Overall +1 to change in general but some nits should be fixed to make it cleaner.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"c5d3d9b7fb88d62c1b1d248b13c6525d8e0ac959","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"1718b720_4fc54fbb","updated":"2022-02-08 14:25:38.000000000","message":"Response inline.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"a055158cd2991339abe6e2844eb63df7116dc6aa","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"216c564b_3aa948ca","updated":"2022-02-07 08:55:02.000000000","message":"recheck 828087","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"7d54d87debc5d0838840888e3a1be6f90153af97","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"4aafcb86_a510ca5f","updated":"2022-02-08 16:00:59.000000000","message":"recheck 828087","commit_id":"77b4ea3a41dcf04177d345daa2d18ed7937d13ff"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"b2c2cf819ec621c86f1dd9dccae0fb093c5f8ec6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"0a349ad2_5b150317","updated":"2022-02-08 23:10:56.000000000","message":"See comments inline.","commit_id":"29659bcd5614f949235071642a7decd5f3f071ee"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"01d5c5f8862ee55143eb2230753006932cda9075","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"314c70ff_93d5347a","updated":"2022-02-08 16:46:13.000000000","message":"Some comments inline.","commit_id":"29659bcd5614f949235071642a7decd5f3f071ee"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"c1f4df1cb06eb0cab59592b053d3330f06db058c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"55375221_ef3b85b9","updated":"2022-02-09 13:40:52.000000000","message":"I think all the concerns have been addressed.  This patch changes the lightos connector and the Lightbits CI has passed.  It also touches the nvmeof connector, and the Kioxia CI has passed as well.  Since the nvmeof change was abstracting a function to a common location, it\u0027s not the kind of change that would pass Kioxia but fail Mellanox SPDK CI, so I won\u0027t wait for that CI to respond.","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f9b423f68e23bf1ae3b372dc5c551756c3684f06","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"69548342_93cda3d9","updated":"2022-02-10 11:35:56.000000000","message":"LGTM.  I haven\u0027t hit merge because this is a breaking change, and I think we should merge this and the cinder patch as close together as possible.","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"},{"author":{"_account_id":7198,"name":"Jay Bryant","email":"jungleboyj@electronicjungle.net","username":"jsbryant"},"change_message_id":"1ba7070e1b87d67900952c7ff3b2f088b84de19c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"26c2d585_89c55a37","updated":"2022-02-14 15:28:07.000000000","message":"Looks good to me.  Looks like we are holding this right now though.","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"bc0948a1a316847005ee3e318b0c8f5fc06b6f2d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"3d2f84c3_26981fa5","updated":"2022-02-14 21:51:34.000000000","message":"Verified that the function Rajat is worried about does have code coverage [0], so going ahead and approving.\n\n\n\n[0] https://2ab502b674131c685c16-07197e0a54baf445efd8154b2af608b4.ssl.cf2.rackcdn.com/828087/8/check/os-brick-code-coverage/15a6381/cover/d_f3b2fe0e95289380_lightos_py.html#t106","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"6431251d52858267e22456c36097ee39df54015b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"61abfe77_c61d9ea6","updated":"2022-02-09 13:41:14.000000000","message":"run-Mellanox CI","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"}],"os_brick/initiator/connectors/lightos.py":[{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"0cbdd93d391802fe5dd0ec6ab40f3ee1b1e30642","unresolved":true,"context_lines":[{"line_number":28,"context_line":"from os_brick import exception"},{"line_number":29,"context_line":"from os_brick.i18n import _"},{"line_number":30,"context_line":"from os_brick.initiator.connectors import base"},{"line_number":31,"context_line":"from os_brick.initiator.connectors.nvmeof import get_host_nqn"},{"line_number":32,"context_line":"from os_brick.privileged import lightos as priv_lightos"},{"line_number":33,"context_line":"from os_brick import utils"},{"line_number":34,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"17ca927a_cf0fe4bf","line":31,"updated":"2022-02-07 09:15:13.000000000","message":"I think importing specific functiouns might be frowned upon? I suggest to just import nvmeof and then call nvmeof.get_host_nqn","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4d24231b4b087e1b123ffe132a15044eb4ea1b1e","unresolved":true,"context_lines":[{"line_number":28,"context_line":"from os_brick import exception"},{"line_number":29,"context_line":"from os_brick.i18n import _"},{"line_number":30,"context_line":"from os_brick.initiator.connectors import base"},{"line_number":31,"context_line":"from os_brick.initiator.connectors.nvmeof import get_host_nqn"},{"line_number":32,"context_line":"from os_brick.privileged import lightos as priv_lightos"},{"line_number":33,"context_line":"from os_brick import utils"},{"line_number":34,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"b5b22899_a4a95ed3","line":31,"in_reply_to":"17ca927a_cf0fe4bf","updated":"2022-02-07 23:51:00.000000000","message":"Yes, what you suggest is the usual practice.\n\nSee my comment in nvmeof.py, however, about locating the function in os_brick/utils.py (which is already being imported into this file at line 33, so you won\u0027t even need this statement).","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"409b5e3f47d7cd7665ceec1a21b3410d90b3ce00","unresolved":false,"context_lines":[{"line_number":28,"context_line":"from os_brick import exception"},{"line_number":29,"context_line":"from os_brick.i18n import _"},{"line_number":30,"context_line":"from os_brick.initiator.connectors import base"},{"line_number":31,"context_line":"from os_brick.initiator.connectors.nvmeof import get_host_nqn"},{"line_number":32,"context_line":"from os_brick.privileged import lightos as priv_lightos"},{"line_number":33,"context_line":"from os_brick import utils"},{"line_number":34,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"eac4e288_06ff2a12","line":31,"in_reply_to":"b5b22899_a4a95ed3","updated":"2022-02-09 10:04:22.000000000","message":"Done","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"0cbdd93d391802fe5dd0ec6ab40f3ee1b1e30642","unresolved":true,"context_lines":[{"line_number":335,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":336,"context_line":"        return new_size"},{"line_number":337,"context_line":""},{"line_number":338,"context_line":"    def get_hostnqn(self):"},{"line_number":339,"context_line":"        return get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":3,"id":"35f170d4_66bc05b1","line":338,"updated":"2022-02-07 09:15:13.000000000","message":"no need for this added level of abstraction, just call the real function from the call sites.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"0a588786563d0759f83ecdf2d25fd714eb7c5c9d","unresolved":true,"context_lines":[{"line_number":335,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":336,"context_line":"        return new_size"},{"line_number":337,"context_line":""},{"line_number":338,"context_line":"    def get_hostnqn(self):"},{"line_number":339,"context_line":"        return get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":3,"id":"da419244_572b309a","line":338,"in_reply_to":"35f170d4_66bc05b1","updated":"2022-02-07 09:30:30.000000000","message":"I wanted to do as little code change I can. this is also used by the cinder lightos driver example:\nhttps://opendev.org/openstack/cinder/src/branch/master/cinder/volume/drivers/lightos.py#L817","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"409b5e3f47d7cd7665ceec1a21b3410d90b3ce00","unresolved":false,"context_lines":[{"line_number":335,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":336,"context_line":"        return new_size"},{"line_number":337,"context_line":""},{"line_number":338,"context_line":"    def get_hostnqn(self):"},{"line_number":339,"context_line":"        return get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":3,"id":"17a5fd63_c8fdcc7c","line":338,"in_reply_to":"41341488_0744d705","updated":"2022-02-09 10:04:22.000000000","message":"Done","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4d24231b4b087e1b123ffe132a15044eb4ea1b1e","unresolved":true,"context_lines":[{"line_number":335,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":336,"context_line":"        return new_size"},{"line_number":337,"context_line":""},{"line_number":338,"context_line":"    def get_hostnqn(self):"},{"line_number":339,"context_line":"        return get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":3,"id":"41341488_0744d705","line":338,"in_reply_to":"da419244_572b309a","updated":"2022-02-07 23:51:00.000000000","message":"OK. I think that you\u0027re probably right that the function should be exposed on the connector if your cinder driver is going to use it.  You should add a docstring or comment here so that people know it\u0027s not an unused function.  But in this file, you should call the function directly as Muli says.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a45d11aa011af956be2ba357bf67e0f8335a90aa","unresolved":true,"context_lines":[{"line_number":334,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":335,"context_line":"        return new_size"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"    def get_hostnqn(self):"},{"line_number":338,"context_line":"        \"\"\"used by cinder driver\"\"\""},{"line_number":339,"context_line":"        return utils.get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":4,"id":"d2bbd4a2_b93cfc36","line":337,"updated":"2022-02-08 15:00:45.000000000","message":"Following the conversation from patchset #3, the cinder driver could, instead of calling this method that is not part of the os-brick connector interface, just call the `get_connector_properties` method (which is part of the public API of any os-brick connector) and check for the presence of an nqn field in that dictionary.\n\nThat would give the connector freedom to change their own methods.","commit_id":"5ee1197bba03066bd8bb0c3e19b15d5e6591f330"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"409b5e3f47d7cd7665ceec1a21b3410d90b3ce00","unresolved":false,"context_lines":[{"line_number":334,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":335,"context_line":"        return new_size"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"    def get_hostnqn(self):"},{"line_number":338,"context_line":"        \"\"\"used by cinder driver\"\"\""},{"line_number":339,"context_line":"        return utils.get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":4,"id":"79f58821_d4809ccc","line":337,"in_reply_to":"582ad5f5_19954e3f","updated":"2022-02-09 10:04:22.000000000","message":"Done","commit_id":"5ee1197bba03066bd8bb0c3e19b15d5e6591f330"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"01d5c5f8862ee55143eb2230753006932cda9075","unresolved":true,"context_lines":[{"line_number":334,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":335,"context_line":"        return new_size"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"    def get_hostnqn(self):"},{"line_number":338,"context_line":"        \"\"\"used by cinder driver\"\"\""},{"line_number":339,"context_line":"        return utils.get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":4,"id":"582ad5f5_19954e3f","line":337,"in_reply_to":"9cc73484_99503b92","updated":"2022-02-08 16:46:13.000000000","message":"Agree with Gorka that we don\u0027t want to introduce functions that aren\u0027t part of the os-brick connector interface if it\u0027s not absolutely necessary (and it doesn\u0027t appear to be necessary in this case).  The lightos cinder driver hasn\u0027t been released yet, so there\u0027s still time to change the code over in cinder so it uses the standard function instead of this custom one.","commit_id":"5ee1197bba03066bd8bb0c3e19b15d5e6591f330"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"40beb07996537b4bb3c8054b015574a751435500","unresolved":true,"context_lines":[{"line_number":334,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":335,"context_line":"        return new_size"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"    def get_hostnqn(self):"},{"line_number":338,"context_line":"        \"\"\"used by cinder driver\"\"\""},{"line_number":339,"context_line":"        return utils.get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":4,"id":"9cc73484_99503b92","line":337,"in_reply_to":"abd260e0_88098f7a","updated":"2022-02-08 16:10:38.000000000","message":"anyway for the lightos connector I can add this change.","commit_id":"5ee1197bba03066bd8bb0c3e19b15d5e6591f330"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"b3aaf3938db3a3fef05aa1a760236e06eb9dd9df","unresolved":true,"context_lines":[{"line_number":334,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":335,"context_line":"        return new_size"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"    def get_hostnqn(self):"},{"line_number":338,"context_line":"        \"\"\"used by cinder driver\"\"\""},{"line_number":339,"context_line":"        return utils.get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":4,"id":"abd260e0_88098f7a","line":337,"in_reply_to":"d2bbd4a2_b93cfc36","updated":"2022-02-08 16:05:12.000000000","message":"Isn\u0027t that dangerous, if we miss a place that uses it - we will break a driver.\nin the lightos there is one appearance, but maybe there is a hidden usage somewhere else? (nova) also this change require a big change to the unittests in test_nvmeof and lightos_test.","commit_id":"5ee1197bba03066bd8bb0c3e19b15d5e6591f330"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"b2c2cf819ec621c86f1dd9dccae0fb093c5f8ec6","unresolved":true,"context_lines":[{"line_number":335,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":336,"context_line":"        return new_size"},{"line_number":337,"context_line":""},{"line_number":338,"context_line":"    def get_hostnqn(self):"},{"line_number":339,"context_line":"        try:"},{"line_number":340,"context_line":"            with open(\u0027/etc/nvme/hostnqn\u0027, \u0027r\u0027) as f:"},{"line_number":341,"context_line":"                host_nqn \u003d f.read().strip()"}],"source_content_type":"text/x-python","patch_set":6,"id":"23a2f0da_9d7c4ade","side":"PARENT","line":338,"range":{"start_line":338,"start_character":3,"end_line":338,"end_character":25},"updated":"2022-02-08 23:10:56.000000000","message":"Don\u0027t forget that the lightos driver is calling this function ... I agree with Gorka\u0027s comment on an earlier patch set that \"the cinder driver could, instead of calling this method that is not part of the os-brick connector interface, just call the `get_connector_properties` method (which is part of the public API of any os-brick connector) and check for the presence of an nqn field in that dictionary.\"\n\nI don\u0027t see a patch up for the cinder lightos driver yet; don\u0027t forget to propose one.","commit_id":"e761ab64230faf9b210a1457ceca592a51202fd6"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"57d52a917574cc0b25cffe098d19c8b5c24451bd","unresolved":true,"context_lines":[{"line_number":335,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":336,"context_line":"        return new_size"},{"line_number":337,"context_line":""},{"line_number":338,"context_line":"    def get_hostnqn(self):"},{"line_number":339,"context_line":"        try:"},{"line_number":340,"context_line":"            with open(\u0027/etc/nvme/hostnqn\u0027, \u0027r\u0027) as f:"},{"line_number":341,"context_line":"                host_nqn \u003d f.read().strip()"}],"source_content_type":"text/x-python","patch_set":6,"id":"aae43aca_3a8d7f26","side":"PARENT","line":338,"range":{"start_line":338,"start_character":3,"end_line":338,"end_character":25},"in_reply_to":"23a2f0da_9d7c4ade","updated":"2022-02-09 09:42:56.000000000","message":"Hey in this patch:\nhttps://review.opendev.org/c/openstack/cinder/+/828085\n\nI removed the use of \n     self.connector.get_hostnqn\nfor\n     self.connector.get_connector_properties(\n                utils.get_root_helper())[\u0027nqn\u0027])","commit_id":"e761ab64230faf9b210a1457ceca592a51202fd6"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"5c63efd46b03ed19fe742336baec60b8c424902f","unresolved":false,"context_lines":[{"line_number":335,"context_line":"        new_size \u003d self._get_size_by_uuid(uuid)"},{"line_number":336,"context_line":"        return new_size"},{"line_number":337,"context_line":""},{"line_number":338,"context_line":"    def get_hostnqn(self):"},{"line_number":339,"context_line":"        try:"},{"line_number":340,"context_line":"            with open(\u0027/etc/nvme/hostnqn\u0027, \u0027r\u0027) as f:"},{"line_number":341,"context_line":"                host_nqn \u003d f.read().strip()"}],"source_content_type":"text/x-python","patch_set":6,"id":"9b80a07f_f855c5ef","side":"PARENT","line":338,"range":{"start_line":338,"start_character":3,"end_line":338,"end_character":25},"in_reply_to":"aae43aca_3a8d7f26","updated":"2022-02-09 10:02:54.000000000","message":"Done","commit_id":"e761ab64230faf9b210a1457ceca592a51202fd6"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"9e3f386553ebfe90be02744cbb72d94cb8e8e91f","unresolved":true,"context_lines":[{"line_number":103,"context_line":"    def dsc_need_connect(self, connection_info):"},{"line_number":104,"context_line":"        return not os.path.isfile(self.dsc_file_name(connection_info[\u0027uuid\u0027]))"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"    def dsc_connect_volume(self, connection_info):"},{"line_number":107,"context_line":"        if not self.dsc_need_connect(connection_info):"},{"line_number":108,"context_line":"            return"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":"        subsysnqn \u003d connection_info[\u0027subsysnqn\u0027]"},{"line_number":111,"context_line":"        uuid \u003d connection_info[\u0027uuid\u0027]"},{"line_number":112,"context_line":"        hostnqn \u003d utils.get_host_nqn()"},{"line_number":113,"context_line":"        with tempfile.NamedTemporaryFile(mode\u003d\u0027w\u0027, delete\u003dFalse) as dscfile:"},{"line_number":114,"context_line":"            dscfile.write(\u0027# os_brick connector dsc file for LightOS\u0027"},{"line_number":115,"context_line":"                          \u0027 volume: {}\\n\u0027.format(uuid))"},{"line_number":116,"context_line":"            for (ip, node) in connection_info[\u0027lightos_nodes\u0027].items():"},{"line_number":117,"context_line":"                transport \u003d node[\u0027transport_type\u0027]"},{"line_number":118,"context_line":"                host \u003d node[\u0027target_portal\u0027]"},{"line_number":119,"context_line":"                port \u003d node[\u0027target_port\u0027]"},{"line_number":120,"context_line":"                dscfile.write(\u0027-t {} -a {} -s {} -q {} -n {}\\n\u0027.format("},{"line_number":121,"context_line":"                    transport, host, port, hostnqn, subsysnqn))"},{"line_number":122,"context_line":"            dscfile.flush()"},{"line_number":123,"context_line":"            try:"},{"line_number":124,"context_line":"                dest_name \u003d self.dsc_file_name(uuid)"},{"line_number":125,"context_line":"                priv_lightos.move_dsc_file(dscfile.name, dest_name)"},{"line_number":126,"context_line":"            except Exception:"},{"line_number":127,"context_line":"                LOG.warning("},{"line_number":128,"context_line":"                    \"LIGHTOS: Failed to create dsc file for connection with\""},{"line_number":129,"context_line":"                    f\" uuid:{uuid}\")"},{"line_number":130,"context_line":"                raise"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    def dsc_disconnect_volume(self, connection_info):"},{"line_number":133,"context_line":"        uuid \u003d connection_info[\u0027uuid\u0027]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3d239aaf_1d8e5c9f","line":130,"range":{"start_line":106,"start_character":0,"end_line":130,"end_character":21},"updated":"2022-02-14 15:54:14.000000000","message":"I don\u0027t see the unit test coverage for this method.\nThe hostnqn-\u003enqn change is covered by test_get_connector_properties but this change of key from nqn -\u003e subsysnqn isn\u0027t tested anywhere. The only occurrence of this method is mocking it in test_connect_volume[1] which has a return value None and isn\u0027t testing anything. This method is called from connect_volume so is an important method that is called everytime a connection is made and should be tested.\n\n[1] https://github.com/openstack/os-brick/blob/627da6a40b4679a41245502b12e06b8340f1ca05/os_brick/tests/initiator/connectors/test_lightos.py#L172-L173","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"5ef69e4fd6425ea736363b3f5f073430547c5c7b","unresolved":true,"context_lines":[{"line_number":103,"context_line":"    def dsc_need_connect(self, connection_info):"},{"line_number":104,"context_line":"        return not os.path.isfile(self.dsc_file_name(connection_info[\u0027uuid\u0027]))"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"    def dsc_connect_volume(self, connection_info):"},{"line_number":107,"context_line":"        if not self.dsc_need_connect(connection_info):"},{"line_number":108,"context_line":"            return"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":"        subsysnqn \u003d connection_info[\u0027subsysnqn\u0027]"},{"line_number":111,"context_line":"        uuid \u003d connection_info[\u0027uuid\u0027]"},{"line_number":112,"context_line":"        hostnqn \u003d utils.get_host_nqn()"},{"line_number":113,"context_line":"        with tempfile.NamedTemporaryFile(mode\u003d\u0027w\u0027, delete\u003dFalse) as dscfile:"},{"line_number":114,"context_line":"            dscfile.write(\u0027# os_brick connector dsc file for LightOS\u0027"},{"line_number":115,"context_line":"                          \u0027 volume: {}\\n\u0027.format(uuid))"},{"line_number":116,"context_line":"            for (ip, node) in connection_info[\u0027lightos_nodes\u0027].items():"},{"line_number":117,"context_line":"                transport \u003d node[\u0027transport_type\u0027]"},{"line_number":118,"context_line":"                host \u003d node[\u0027target_portal\u0027]"},{"line_number":119,"context_line":"                port \u003d node[\u0027target_port\u0027]"},{"line_number":120,"context_line":"                dscfile.write(\u0027-t {} -a {} -s {} -q {} -n {}\\n\u0027.format("},{"line_number":121,"context_line":"                    transport, host, port, hostnqn, subsysnqn))"},{"line_number":122,"context_line":"            dscfile.flush()"},{"line_number":123,"context_line":"            try:"},{"line_number":124,"context_line":"                dest_name \u003d self.dsc_file_name(uuid)"},{"line_number":125,"context_line":"                priv_lightos.move_dsc_file(dscfile.name, dest_name)"},{"line_number":126,"context_line":"            except Exception:"},{"line_number":127,"context_line":"                LOG.warning("},{"line_number":128,"context_line":"                    \"LIGHTOS: Failed to create dsc file for connection with\""},{"line_number":129,"context_line":"                    f\" uuid:{uuid}\")"},{"line_number":130,"context_line":"                raise"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    def dsc_disconnect_volume(self, connection_info):"},{"line_number":133,"context_line":"        uuid \u003d connection_info[\u0027uuid\u0027]"}],"source_content_type":"text/x-python","patch_set":8,"id":"778a9f24_afc384fe","line":130,"range":{"start_line":106,"start_character":0,"end_line":130,"end_character":21},"in_reply_to":"3d239aaf_1d8e5c9f","updated":"2022-02-14 19:37:07.000000000","message":"Hey, there is also test: https://github.com/openstack/os-brick/blob/627da6a40b4679a41245502b12e06b8340f1ca05/os_brick/tests/initiator/connectors/test_lightos.py#L119\n\nwhich doesnt mock dsc_connect_volume function.\nyou can also check the tox -e cover - its is tested.","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"ad7bf653085d1770bc254989e6ac2baa71cd9736","unresolved":true,"context_lines":[{"line_number":103,"context_line":"    def dsc_need_connect(self, connection_info):"},{"line_number":104,"context_line":"        return not os.path.isfile(self.dsc_file_name(connection_info[\u0027uuid\u0027]))"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"    def dsc_connect_volume(self, connection_info):"},{"line_number":107,"context_line":"        if not self.dsc_need_connect(connection_info):"},{"line_number":108,"context_line":"            return"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":"        subsysnqn \u003d connection_info[\u0027subsysnqn\u0027]"},{"line_number":111,"context_line":"        uuid \u003d connection_info[\u0027uuid\u0027]"},{"line_number":112,"context_line":"        hostnqn \u003d utils.get_host_nqn()"},{"line_number":113,"context_line":"        with tempfile.NamedTemporaryFile(mode\u003d\u0027w\u0027, delete\u003dFalse) as dscfile:"},{"line_number":114,"context_line":"            dscfile.write(\u0027# os_brick connector dsc file for LightOS\u0027"},{"line_number":115,"context_line":"                          \u0027 volume: {}\\n\u0027.format(uuid))"},{"line_number":116,"context_line":"            for (ip, node) in connection_info[\u0027lightos_nodes\u0027].items():"},{"line_number":117,"context_line":"                transport \u003d node[\u0027transport_type\u0027]"},{"line_number":118,"context_line":"                host \u003d node[\u0027target_portal\u0027]"},{"line_number":119,"context_line":"                port \u003d node[\u0027target_port\u0027]"},{"line_number":120,"context_line":"                dscfile.write(\u0027-t {} -a {} -s {} -q {} -n {}\\n\u0027.format("},{"line_number":121,"context_line":"                    transport, host, port, hostnqn, subsysnqn))"},{"line_number":122,"context_line":"            dscfile.flush()"},{"line_number":123,"context_line":"            try:"},{"line_number":124,"context_line":"                dest_name \u003d self.dsc_file_name(uuid)"},{"line_number":125,"context_line":"                priv_lightos.move_dsc_file(dscfile.name, dest_name)"},{"line_number":126,"context_line":"            except Exception:"},{"line_number":127,"context_line":"                LOG.warning("},{"line_number":128,"context_line":"                    \"LIGHTOS: Failed to create dsc file for connection with\""},{"line_number":129,"context_line":"                    f\" uuid:{uuid}\")"},{"line_number":130,"context_line":"                raise"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    def dsc_disconnect_volume(self, connection_info):"},{"line_number":133,"context_line":"        uuid \u003d connection_info[\u0027uuid\u0027]"}],"source_content_type":"text/x-python","patch_set":8,"id":"b40b6e86_978de195","line":130,"range":{"start_line":106,"start_character":0,"end_line":130,"end_character":21},"in_reply_to":"778a9f24_afc384fe","updated":"2022-02-15 04:01:22.000000000","message":"I see now the case is covered but i would still prefer a separate UT for this method.","commit_id":"71a6ea1f4df8b789ca0133a06c5b8dee7fdfa853"}],"os_brick/initiator/connectors/nvmeof.py":[{"author":{"_account_id":34459,"name":"Muli Ben-Yehuda","email":"muli@lightbitslabs.com","username":"muliby"},"change_message_id":"0cbdd93d391802fe5dd0ec6ab40f3ee1b1e30642","unresolved":true,"context_lines":[{"line_number":128,"context_line":"                \"Process execution error in _get_host_uuid: %s\" % str(e))"},{"line_number":129,"context_line":"            return None"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def _get_host_nqn(self):"},{"line_number":132,"context_line":"        return get_host_nqn()"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"    def _get_system_uuid(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"d93c53e7_ee8d5b21","line":131,"updated":"2022-02-07 09:15:13.000000000","message":"no need for this added level of abstraction, just change the callsites to call get_host_nqn() directly. Larger patch but cleaner code.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"57d52a917574cc0b25cffe098d19c8b5c24451bd","unresolved":true,"context_lines":[{"line_number":128,"context_line":"                \"Process execution error in _get_host_uuid: %s\" % str(e))"},{"line_number":129,"context_line":"            return None"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def _get_host_nqn(self):"},{"line_number":132,"context_line":"        return get_host_nqn()"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"    def _get_system_uuid(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"850a410e_61af51e6","line":131,"in_reply_to":"363595fc_94d54b12","updated":"2022-02-09 09:42:56.000000000","message":"ok, I removed it.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"b2c2cf819ec621c86f1dd9dccae0fb093c5f8ec6","unresolved":true,"context_lines":[{"line_number":128,"context_line":"                \"Process execution error in _get_host_uuid: %s\" % str(e))"},{"line_number":129,"context_line":"            return None"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def _get_host_nqn(self):"},{"line_number":132,"context_line":"        return get_host_nqn()"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"    def _get_system_uuid(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"363595fc_94d54b12","line":131,"in_reply_to":"7e243237_6db258b8","updated":"2022-02-08 23:10:56.000000000","message":"-1: I believe that for this connector, this function was only used in get_connector_properties(), so you no longer need to define this function at all.\n\nThe lightos driver [0] is the one currently calling an out-of-interface function on the lightos connector, not on this connector.  So I think this function isn\u0027t needed any more.\n\n[0] https://opendev.org/openstack/cinder/src/commit/638688b5bf47847369070f050ef6755972015927/cinder/volume/drivers/lightos.py#L817","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"5c63efd46b03ed19fe742336baec60b8c424902f","unresolved":false,"context_lines":[{"line_number":128,"context_line":"                \"Process execution error in _get_host_uuid: %s\" % str(e))"},{"line_number":129,"context_line":"            return None"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def _get_host_nqn(self):"},{"line_number":132,"context_line":"        return get_host_nqn()"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"    def _get_system_uuid(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"631dee5f_5a660245","line":131,"in_reply_to":"850a410e_61af51e6","updated":"2022-02-09 10:02:54.000000000","message":"Done","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4d24231b4b087e1b123ffe132a15044eb4ea1b1e","unresolved":true,"context_lines":[{"line_number":128,"context_line":"                \"Process execution error in _get_host_uuid: %s\" % str(e))"},{"line_number":129,"context_line":"            return None"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def _get_host_nqn(self):"},{"line_number":132,"context_line":"        return get_host_nqn()"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"    def _get_system_uuid(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7e243237_6db258b8","line":131,"in_reply_to":"d93c53e7_ee8d5b21","updated":"2022-02-07 23:51:00.000000000","message":"I agree with Muli.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4d24231b4b087e1b123ffe132a15044eb4ea1b1e","unresolved":true,"context_lines":[{"line_number":873,"context_line":"        host_nqn \u003d priv_nvme.create_hostnqn()"},{"line_number":874,"context_line":"    except Exception:"},{"line_number":875,"context_line":"        host_nqn \u003d None"},{"line_number":876,"context_line":"    return host_nqn"}],"source_content_type":"text/x-python","patch_set":3,"id":"c5e92baf_54c96e47","line":876,"updated":"2022-02-07 23:51:00.000000000","message":"Gorka\u0027s suggestion was that this function should go into os_brick/utils.py.  You could expose it here, but the advantage to it being in utils is that then it\u0027s obviously common code, and people will be more aware of that when refactoring happens.  Plus, this file and lightos.py already import os_brick.utils.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"c5d3d9b7fb88d62c1b1d248b13c6525d8e0ac959","unresolved":true,"context_lines":[{"line_number":873,"context_line":"        host_nqn \u003d priv_nvme.create_hostnqn()"},{"line_number":874,"context_line":"    except Exception:"},{"line_number":875,"context_line":"        host_nqn \u003d None"},{"line_number":876,"context_line":"    return host_nqn"}],"source_content_type":"text/x-python","patch_set":3,"id":"80d18b5e_20bed844","line":876,"in_reply_to":"768087e0_a2387d6e","updated":"2022-02-08 14:25:38.000000000","message":"The filenames are just there to organize the privileged functions for developers.  Any of the os_brick code can import and call any of the functions, so you don\u0027t need to worry about the file name.\n\nI think it makes sense to keep the privileged function in nvmeof.py because it\u0027s an nvme kind of thing, and in utils.py you can do\n\n  from os_brick.privileged import nvmeof as priv_nvme\n\nand then call priv_nvme.create_hostnqn() from within your utility function.\n\n(Of course, other reviewers may have different opinions.)","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a45d11aa011af956be2ba357bf67e0f8335a90aa","unresolved":true,"context_lines":[{"line_number":873,"context_line":"        host_nqn \u003d priv_nvme.create_hostnqn()"},{"line_number":874,"context_line":"    except Exception:"},{"line_number":875,"context_line":"        host_nqn \u003d None"},{"line_number":876,"context_line":"    return host_nqn"}],"source_content_type":"text/x-python","patch_set":3,"id":"a57fa6ae_7399e7c9","line":876,"in_reply_to":"80d18b5e_20bed844","updated":"2022-02-08 15:00:45.000000000","message":"I agree, having it in utils seems like easier to understand it\u0027s common code as well as make it harder for someone to do a refactoring in nvmeof.py and break the lightos.py connector.\n\nAlternatively we could create a os_brick/initiator/connectors/nvme_common.py file where we put the common things between these 2 connectors.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"57d52a917574cc0b25cffe098d19c8b5c24451bd","unresolved":true,"context_lines":[{"line_number":873,"context_line":"        host_nqn \u003d priv_nvme.create_hostnqn()"},{"line_number":874,"context_line":"    except Exception:"},{"line_number":875,"context_line":"        host_nqn \u003d None"},{"line_number":876,"context_line":"    return host_nqn"}],"source_content_type":"text/x-python","patch_set":3,"id":"d1152c61_625d16f7","line":876,"in_reply_to":"921dc4f0_3433238f","updated":"2022-02-09 09:42:56.000000000","message":"Its in os_brick/utils.py as requested - anything else is needed?","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"b2c2cf819ec621c86f1dd9dccae0fb093c5f8ec6","unresolved":true,"context_lines":[{"line_number":873,"context_line":"        host_nqn \u003d priv_nvme.create_hostnqn()"},{"line_number":874,"context_line":"    except Exception:"},{"line_number":875,"context_line":"        host_nqn \u003d None"},{"line_number":876,"context_line":"    return host_nqn"}],"source_content_type":"text/x-python","patch_set":3,"id":"921dc4f0_3433238f","line":876,"in_reply_to":"a57fa6ae_7399e7c9","updated":"2022-02-08 23:10:56.000000000","message":"Let\u0027s keep Gorka\u0027s suggestion in mind as development continues on the nvmeof and lightos connectors.  You should make a point of reviewing changes to the nvmeof connector, and people working on that one should make a point of reviewing yours.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"6d079c4b91aa344534586bfe15da94a427350ec1","unresolved":true,"context_lines":[{"line_number":873,"context_line":"        host_nqn \u003d priv_nvme.create_hostnqn()"},{"line_number":874,"context_line":"    except Exception:"},{"line_number":875,"context_line":"        host_nqn \u003d None"},{"line_number":876,"context_line":"    return host_nqn"}],"source_content_type":"text/x-python","patch_set":3,"id":"768087e0_a2387d6e","line":876,"in_reply_to":"c5e92baf_54c96e47","updated":"2022-02-08 07:53:21.000000000","message":"my thought of writing it here was - well its an nvme operation and the file is called nvmeof - so it made sense for me.\nalso the function use privileged access and was not sure how utils.py will handle it.\nshould I add a \"utils.py\" under priviliged dir?","commit_id":"df2874daec486818986e77a56314192aa9ffa819"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"c1f4df1cb06eb0cab59592b053d3330f06db058c","unresolved":true,"context_lines":[{"line_number":873,"context_line":"        host_nqn \u003d priv_nvme.create_hostnqn()"},{"line_number":874,"context_line":"    except Exception:"},{"line_number":875,"context_line":"        host_nqn \u003d None"},{"line_number":876,"context_line":"    return host_nqn"}],"source_content_type":"text/x-python","patch_set":3,"id":"f24028fd_3adb8983","line":876,"in_reply_to":"d1152c61_625d16f7","updated":"2022-02-09 13:40:52.000000000","message":"That\u0027s fine as far as I\u0027m concerned ... in utils for now, and into a nvme_common module if there turns out to be more code that could be re-used.","commit_id":"df2874daec486818986e77a56314192aa9ffa819"}],"os_brick/tests/initiator/test_connector.py":[{"author":{"_account_id":33431,"name":"Fábio Oliveira","email":"fabioaurelio1269@gmail.com","username":"fabiooliveira1"},"change_message_id":"91a637bac0926808f14e114b3769a42afad35460","unresolved":true,"context_lines":[{"line_number":48,"context_line":"                       return_value\u003dNone)"},{"line_number":49,"context_line":"    @mock.patch.object(nvmeof.NVMeOFConnector, \u0027_get_host_uuid\u0027,"},{"line_number":50,"context_line":"                       return_value\u003dNone)"},{"line_number":51,"context_line":"    @mock.patch.object(utils, \u0027get_host_nqn\u0027,"},{"line_number":52,"context_line":"                       return_value\u003dNone)"},{"line_number":53,"context_line":"    @mock.patch.object(iscsi.ISCSIConnector, \u0027get_initiator\u0027,"},{"line_number":54,"context_line":"                       return_value\u003d\u0027fakeinitiator\u0027)"},{"line_number":55,"context_line":"    @mock.patch.object(linuxfc.LinuxFibreChannel, \u0027get_fc_wwpns\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"83bfa8b0_23292d8e","line":52,"range":{"start_line":51,"start_character":4,"end_line":52,"end_character":41},"updated":"2022-02-08 19:28:08.000000000","message":"will you need it here? as you see, its the same parameters as the first one","commit_id":"29659bcd5614f949235071642a7decd5f3f071ee"},{"author":{"_account_id":33612,"name":"yuval","email":"yuval@lightbitslabs.com","username":"yuval"},"change_message_id":"5c63efd46b03ed19fe742336baec60b8c424902f","unresolved":false,"context_lines":[{"line_number":48,"context_line":"                       return_value\u003dNone)"},{"line_number":49,"context_line":"    @mock.patch.object(nvmeof.NVMeOFConnector, \u0027_get_host_uuid\u0027,"},{"line_number":50,"context_line":"                       return_value\u003dNone)"},{"line_number":51,"context_line":"    @mock.patch.object(utils, \u0027get_host_nqn\u0027,"},{"line_number":52,"context_line":"                       return_value\u003dNone)"},{"line_number":53,"context_line":"    @mock.patch.object(iscsi.ISCSIConnector, \u0027get_initiator\u0027,"},{"line_number":54,"context_line":"                       return_value\u003d\u0027fakeinitiator\u0027)"},{"line_number":55,"context_line":"    @mock.patch.object(linuxfc.LinuxFibreChannel, \u0027get_fc_wwpns\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"2bf0708d_9745a879","line":52,"range":{"start_line":51,"start_character":4,"end_line":52,"end_character":41},"in_reply_to":"83bfa8b0_23292d8e","updated":"2022-02-09 10:02:54.000000000","message":"you right, fixed","commit_id":"29659bcd5614f949235071642a7decd5f3f071ee"}]}