)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"8f39ab852088e5121dc01ce53fc23468996aacf7","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"b20f7afd_c8baf35c","updated":"2022-08-23 01:36:51.000000000","message":"What tool does use tools/config/nvmeof_agent-config-generator.conf?\nWhy is it necessary to save its output into etc/ of the source tree?\n","commit_id":"b790ea40ac10ff968f3cd38fbef44873416ae1da"}],"os_brick/cmd/nvmeof_agent/agent.py":[{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":82,"context_line":""},{"line_number":83,"context_line":"    @staticmethod"},{"line_number":84,"context_line":"    def get_nvme_state(md_name, search_path):"},{"line_number":85,"context_line":"        \"\"\"get nvme devices state. returns: dictionary of device:state\"\"\""},{"line_number":86,"context_line":"        dev_path \u003d \u0027/dev/\u0027"},{"line_number":87,"context_line":"        devices \u003d []"},{"line_number":88,"context_line":"        states \u003d []"}],"source_content_type":"text/x-python","patch_set":3,"id":"a2686733_e3dceb38","line":85,"updated":"2021-08-12 11:39:16.000000000","message":"Should the returns be on a new line for readability. And perhaps add the params also.\n\n\"\"\"get nvme devices state.\n\n:param md_name:\n:param search_path:\n:returns: dictionary of device:state\n\"\"\"\n\nThere are a few places below also that could be changed, but just a suggestion","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":false,"context_lines":[{"line_number":82,"context_line":""},{"line_number":83,"context_line":"    @staticmethod"},{"line_number":84,"context_line":"    def get_nvme_state(md_name, search_path):"},{"line_number":85,"context_line":"        \"\"\"get nvme devices state. returns: dictionary of device:state\"\"\""},{"line_number":86,"context_line":"        dev_path \u003d \u0027/dev/\u0027"},{"line_number":87,"context_line":"        devices \u003d []"},{"line_number":88,"context_line":"        states \u003d []"}],"source_content_type":"text/x-python","patch_set":3,"id":"9e6530f3_9ca7a92d","line":85,"in_reply_to":"a2686733_e3dceb38","updated":"2021-08-13 05:37:33.000000000","message":"Done","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":161,"context_line":"        return True"},{"line_number":162,"context_line":""},{"line_number":163,"context_line":"    def fail_md(self, md_name, nvme_device):"},{"line_number":164,"context_line":"        \"\"\"fail md device. returns: command error, exit code on failure\"\"\""},{"line_number":165,"context_line":"        try:"},{"line_number":166,"context_line":"            cmd \u003d [\u0027mdadm\u0027, \u0027--manage\u0027, \u0027/dev/md/\u0027 + md_name, \u0027--fail\u0027,"},{"line_number":167,"context_line":"                   nvme_device]"}],"source_content_type":"text/x-python","patch_set":3,"id":"4e0bb8fe_6c1c8f31","line":164,"updated":"2021-08-12 11:39:16.000000000","message":"Is the return type correct.  Should it be a boolean.","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":false,"context_lines":[{"line_number":161,"context_line":"        return True"},{"line_number":162,"context_line":""},{"line_number":163,"context_line":"    def fail_md(self, md_name, nvme_device):"},{"line_number":164,"context_line":"        \"\"\"fail md device. returns: command error, exit code on failure\"\"\""},{"line_number":165,"context_line":"        try:"},{"line_number":166,"context_line":"            cmd \u003d [\u0027mdadm\u0027, \u0027--manage\u0027, \u0027/dev/md/\u0027 + md_name, \u0027--fail\u0027,"},{"line_number":167,"context_line":"                   nvme_device]"}],"source_content_type":"text/x-python","patch_set":3,"id":"a79960af_e23aa0f4","line":164,"in_reply_to":"4e0bb8fe_6c1c8f31","updated":"2021-08-13 05:37:33.000000000","message":"Done","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":173,"context_line":"        return True"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def modify_device_count(self, md_name, device_count):"},{"line_number":176,"context_line":"        \"\"\"grow mdadm device. returns: command error, exit code on failure\"\"\""},{"line_number":177,"context_line":"        try:"},{"line_number":178,"context_line":"            raid_devices \u003d \u0027--raid-devices\u003d\u0027 + str(device_count)"},{"line_number":179,"context_line":"            LOG.debug(\"[!] raid_devices: %s\", raid_devices)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a2592c3_ca0b2324","line":176,"updated":"2021-08-12 11:39:16.000000000","message":"again, method is returning a boolean.","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":false,"context_lines":[{"line_number":173,"context_line":"        return True"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def modify_device_count(self, md_name, device_count):"},{"line_number":176,"context_line":"        \"\"\"grow mdadm device. returns: command error, exit code on failure\"\"\""},{"line_number":177,"context_line":"        try:"},{"line_number":178,"context_line":"            raid_devices \u003d \u0027--raid-devices\u003d\u0027 + str(device_count)"},{"line_number":179,"context_line":"            LOG.debug(\"[!] raid_devices: %s\", raid_devices)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ea6fc464_9909c120","line":176,"in_reply_to":"5a2592c3_ca0b2324","updated":"2021-08-13 05:37:33.000000000","message":"Done","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":210,"context_line":"    def get_volume_by_alias(self, md_name):"},{"line_number":211,"context_line":"        ks_volume \u003d None"},{"line_number":212,"context_line":"        result \u003d self.prov_rest.get_volumes_by_alias(md_name)"},{"line_number":213,"context_line":"        if result.status \u003d\u003d \"Success\":"},{"line_number":214,"context_line":"            if len(result.prov_entities) \u003d\u003d 0:"},{"line_number":215,"context_line":"                return None"},{"line_number":216,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"dc89c1af_1f2dbb9e","line":213,"updated":"2021-08-12 11:39:16.000000000","message":"nit: shorthand\n\n    def get_volume_by_alias(self, md_name):\n        result \u003d self.prov_rest.get_volumes_by_alias(md_name)\n        if result.status \u003d\u003d \"Success\":\n            return result.prov_entities[0] if len(\n                    result.prov_entities) \u003e 0 else None\n        return None","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":true,"context_lines":[{"line_number":210,"context_line":"    def get_volume_by_alias(self, md_name):"},{"line_number":211,"context_line":"        ks_volume \u003d None"},{"line_number":212,"context_line":"        result \u003d self.prov_rest.get_volumes_by_alias(md_name)"},{"line_number":213,"context_line":"        if result.status \u003d\u003d \"Success\":"},{"line_number":214,"context_line":"            if len(result.prov_entities) \u003d\u003d 0:"},{"line_number":215,"context_line":"                return None"},{"line_number":216,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"02df27d2_21bef977","line":213,"in_reply_to":"dc89c1af_1f2dbb9e","updated":"2021-08-13 05:37:33.000000000","message":"I am actually like you preferring shorthand over more bloated-looking code, however, I am not the only one working on this code, and they have their preferences and standards too. For this reason in this case I rather leave it as it is.","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":264,"context_line":"    def get_uuid_by_nvme_device(nvme_device):"},{"line_number":265,"context_line":"        \"\"\"get volume uuid by nvme device\"\"\""},{"line_number":266,"context_line":"        uuid_path \u003d \u0027/sys/block/{0}/uuid\u0027.format(nvme_device)"},{"line_number":267,"context_line":"        # LOG.debug(\"[!] uuid_path: %s\", uuid_path)"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"        try:"},{"line_number":270,"context_line":"            with open(uuid_path) as f:"}],"source_content_type":"text/x-python","patch_set":3,"id":"2397bf47_36f735fd","line":267,"updated":"2021-08-12 11:39:16.000000000","message":"nit: remove or uncomment","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":false,"context_lines":[{"line_number":264,"context_line":"    def get_uuid_by_nvme_device(nvme_device):"},{"line_number":265,"context_line":"        \"\"\"get volume uuid by nvme device\"\"\""},{"line_number":266,"context_line":"        uuid_path \u003d \u0027/sys/block/{0}/uuid\u0027.format(nvme_device)"},{"line_number":267,"context_line":"        # LOG.debug(\"[!] uuid_path: %s\", uuid_path)"},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"        try:"},{"line_number":270,"context_line":"            with open(uuid_path) as f:"}],"source_content_type":"text/x-python","patch_set":3,"id":"d0e1b384_3734fc43","line":267,"in_reply_to":"2397bf47_36f735fd","updated":"2021-08-13 05:37:33.000000000","message":"Done","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":343,"context_line":"                    host_uuid,"},{"line_number":344,"context_line":"                    result.status)"},{"line_number":345,"context_line":"        except Exception as ex:"},{"line_number":346,"context_line":"            LOG.error(\"[!] host_probe exception: %s\", str(ex))"},{"line_number":347,"context_line":"            traceback.print_exc()"},{"line_number":348,"context_line":"            LOG.debug(\"[!] Exception %s\", traceback.format_exc())"},{"line_number":349,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"9cdd84da_07642c5b","line":346,"updated":"2021-08-12 11:39:16.000000000","message":"nit: not sure if the str() is necessary.  It\u0027s missing from line 358","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":true,"context_lines":[{"line_number":343,"context_line":"                    host_uuid,"},{"line_number":344,"context_line":"                    result.status)"},{"line_number":345,"context_line":"        except Exception as ex:"},{"line_number":346,"context_line":"            LOG.error(\"[!] host_probe exception: %s\", str(ex))"},{"line_number":347,"context_line":"            traceback.print_exc()"},{"line_number":348,"context_line":"            LOG.debug(\"[!] Exception %s\", traceback.format_exc())"},{"line_number":349,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"1dcf1ec2_353ffd83","line":346,"in_reply_to":"9cdd84da_07642c5b","updated":"2021-08-13 05:37:33.000000000","message":"I am also not sure.\nHowever, I saw this is a common standard in a lot of code, including recent code by other maintainers. Also, this looks like the safer approch (to avoid a possible exception during a simple log message)\nFor this reason and to keep to this perceived standard, I actually rather add the str() around the ex in line 358. Hope that\u0027s ok, and thank you for the catch!","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":375,"context_line":"            if backend.persistentID \u003d\u003d persistent_id:"},{"line_number":376,"context_line":"                LOG.debug(\"[!] backend: %s\", str(backend))"},{"line_number":377,"context_line":"                if backend.portals[0] is None:"},{"line_number":378,"context_line":"                    msg \u003d \"no portals found on KumoScale\""},{"line_number":379,"context_line":"                    raise Exception(msg)"},{"line_number":380,"context_line":"                portals \u003d backend.portals"},{"line_number":381,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"b4afd6f7_21d72f28","line":378,"updated":"2021-08-12 11:39:16.000000000","message":"General comment.  When I started reviewing first this had a look and feel of a generic agent for NVMe and not specific to Kumoscale.  is this new connection agent exclusive to Kumoscale, it\u0027s difficult to tell","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":false,"context_lines":[{"line_number":375,"context_line":"            if backend.persistentID \u003d\u003d persistent_id:"},{"line_number":376,"context_line":"                LOG.debug(\"[!] backend: %s\", str(backend))"},{"line_number":377,"context_line":"                if backend.portals[0] is None:"},{"line_number":378,"context_line":"                    msg \u003d \"no portals found on KumoScale\""},{"line_number":379,"context_line":"                    raise Exception(msg)"},{"line_number":380,"context_line":"                portals \u003d backend.portals"},{"line_number":381,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"e00c1d0b_6e046d51","line":378,"in_reply_to":"b4afd6f7_21d72f28","updated":"2021-08-13 05:37:33.000000000","message":"This is a good find, thank you! This likely got overlooked, I will change the message to be more proper (because this format is not specific to KumoScale, it is the same connection_properties format that is used by the NVMeOF connector replicated mode, this format is implemented by the KumoScale driver, but it can also be implemented by other drivers as well)\n\nTo more fully answer your question, a current best attempt was made to make this agent as generic as possible, yet there are likely more changes needed to fully abstract out the vendor-specific bits (this was mentioned at the PTG, a best effort making sure the core non-vendor related nvmeof operations are all generic - but, there is still a lot of wiggle room in provisioner communication, so it is not perfect. As other vendors decide to implement drivers that use replicated nvmeof connector and this agent, the potentially still missing pieces of the vendor-abstraction will surface and then be addressed)","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":521,"context_line":"        volume_replicas \u003d volume.location"},{"line_number":522,"context_line":"        LOG.debug(\"[!] host_id: %s\", host_id)"},{"line_number":523,"context_line":"        targets \u003d self.get_targets(host_id, volume.uuid)"},{"line_number":524,"context_line":"        if targets is None or len(targets) \u003c 1:"},{"line_number":525,"context_line":"            LOG.warning(\"[!] Could not find all targets for volume: %s\","},{"line_number":526,"context_line":"                        volume.uuid)"},{"line_number":527,"context_line":"            return False"}],"source_content_type":"text/x-python","patch_set":3,"id":"d4804a91_593e6020","line":524,"updated":"2021-08-12 11:39:16.000000000","message":"Do we need both conditions.  \nif targets is None should mean its empty and len \u003d\u003d 0.  These little things always get me.","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":false,"context_lines":[{"line_number":521,"context_line":"        volume_replicas \u003d volume.location"},{"line_number":522,"context_line":"        LOG.debug(\"[!] host_id: %s\", host_id)"},{"line_number":523,"context_line":"        targets \u003d self.get_targets(host_id, volume.uuid)"},{"line_number":524,"context_line":"        if targets is None or len(targets) \u003c 1:"},{"line_number":525,"context_line":"            LOG.warning(\"[!] Could not find all targets for volume: %s\","},{"line_number":526,"context_line":"                        volume.uuid)"},{"line_number":527,"context_line":"            return False"}],"source_content_type":"text/x-python","patch_set":3,"id":"d477e135_1b3fe4ec","line":524,"in_reply_to":"d4804a91_593e6020","updated":"2021-08-13 05:37:33.000000000","message":"In general such similar little things get to me too and I get nitty on those with the team as well (you should see our inner workings 😊)\n\nBut, in this case, I think that len(None) throws an exception, that is why this one actually makes sense.\n\n\u003e\u003e\u003e nonevar \u003d None\n\u003e\u003e\u003e len(nonevar)\nTraceback (most recent call last):\n  File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":812,"context_line":"                volume_alias_from_md \u003d md_name[device_index + 1:]"},{"line_number":813,"context_line":""},{"line_number":814,"context_line":"                LOG.debug(\"[!] nvme_live_legs %s\", str(nvme_live_legs))"},{"line_number":815,"context_line":"                if len(nvme_live_legs) \u003c 1:"},{"line_number":816,"context_line":"                    volume_alias \u003d self.get_volume_alias(\"\","},{"line_number":817,"context_line":"                                                         volume_alias_from_md)"},{"line_number":818,"context_line":"                    volume \u003d self.get_volume_if_connected_to_me(volume_alias)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5c05e529_26fa14b0","line":815,"updated":"2021-08-12 11:39:16.000000000","message":"maybe check to see if nvme_live_legs is not None in case it is None and throws a \nTypeError: object of type \u0027NoneType\u0027 has no len()","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":true,"context_lines":[{"line_number":812,"context_line":"                volume_alias_from_md \u003d md_name[device_index + 1:]"},{"line_number":813,"context_line":""},{"line_number":814,"context_line":"                LOG.debug(\"[!] nvme_live_legs %s\", str(nvme_live_legs))"},{"line_number":815,"context_line":"                if len(nvme_live_legs) \u003c 1:"},{"line_number":816,"context_line":"                    volume_alias \u003d self.get_volume_alias(\"\","},{"line_number":817,"context_line":"                                                         volume_alias_from_md)"},{"line_number":818,"context_line":"                    volume \u003d self.get_volume_if_connected_to_me(volume_alias)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1ac590ff_b13f30d9","line":815,"in_reply_to":"5c05e529_26fa14b0","updated":"2021-08-13 05:37:33.000000000","message":"That truely makes sense to be safe, and it would be 100% necessary if there was a code path that could lead to it being None.\n\nI did take a look and traced it back: handle_leg_failure() assigns [] to the nvme_live_legs that it returns and only appends to it, so im pretty sure with current code it will never be None.\n\nI would change it to be safe, but there are many other similar occurnces of len() being used on a variable without checking if it is None, so for the above reasons and to minimize code agitation, I rather keep this one the way it is.","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"f6eb7ff66a3afe6f58b3d8e2954e878bb5f54673","unresolved":true,"context_lines":[{"line_number":24,"context_line":"from oslo_log import log as logging"},{"line_number":25,"context_line":"from oslo_utils.secretutils import md5"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"import os_brick.cmd.nvmeof_agent.kioxia.entities as provisioner_entities"},{"line_number":28,"context_line":"import os_brick.cmd.nvmeof_agent.kioxia.rest_client as provisioner_rest_client"},{"line_number":29,"context_line":"import os_brick.initiator.connectors.nvmeof as nvmeof"},{"line_number":30,"context_line":"from os_brick.privileged import nvmeof as priv_nvme"},{"line_number":31,"context_line":"from os_brick.privileged import rootwrap as priv_rootwrap"}],"source_content_type":"text/x-python","patch_set":4,"id":"ec9f2f26_a497296a","line":28,"range":{"start_line":27,"start_character":0,"end_line":28,"end_character":78},"updated":"2021-08-15 23:23:52.000000000","message":"Since this is a generic agent, you should make the provisioner_entities and provisioner_rest_client configurable in the conf file, and then use importutils from oslo.utils to import the classes.  (You can look at cinder for an example, I think the backup manager does a lot of this.)","commit_id":"1a513ca13d182c769e6ba1d640568afa6efc40fc"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"31c7c148166aa33354e675d21cf62d603d306324","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from oslo_log import log as logging"},{"line_number":25,"context_line":"from oslo_utils.secretutils import md5"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"import os_brick.cmd.nvmeof_agent.kioxia.entities as provisioner_entities"},{"line_number":28,"context_line":"import os_brick.cmd.nvmeof_agent.kioxia.rest_client as provisioner_rest_client"},{"line_number":29,"context_line":"import os_brick.initiator.connectors.nvmeof as nvmeof"},{"line_number":30,"context_line":"from os_brick.privileged import nvmeof as priv_nvme"},{"line_number":31,"context_line":"from os_brick.privileged import rootwrap as priv_rootwrap"}],"source_content_type":"text/x-python","patch_set":4,"id":"cdd41847_9039c03c","line":28,"range":{"start_line":27,"start_character":0,"end_line":28,"end_character":78},"in_reply_to":"ec9f2f26_a497296a","updated":"2021-08-16 15:46:08.000000000","message":"Thank you, good point and great suggestion, this is a good initial approach for making the agent generic.\n\nWith this, it is already modular enough for any other \"agent driver\" implementations using custom rest_api and entitie.\n\nHowever, just to mention here for completion, there could still some work to be done abstracting out driver-specific methods, such that logic can be changed (or omitted) by other drivers, other than just implementing their own copy of the provisioner rest api. This will be added in a followup over time, based on new drivers (or changes in current driver) needs.","commit_id":"1a513ca13d182c769e6ba1d640568afa6efc40fc"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"bc2a837986917d788315a5e523fb1b93535fd3e2","unresolved":true,"context_lines":[{"line_number":60,"context_line":"            CONF.provisioner_entities)"},{"line_number":61,"context_line":"        self.provisioner_rest_client \u003d importutils.import_module("},{"line_number":62,"context_line":"            CONF.provisioner_rest_client)"},{"line_number":63,"context_line":"        self.prov_rest \u003d self.provisioner_rest_client.KioxiaProvisioner("},{"line_number":64,"context_line":"            [CONF.prov_ip], CONF.cert_file, CONF.token, CONF.prov_port)"},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"        scheduler.enter(1, 1, self.call_host_monitor, (30,))"}],"source_content_type":"text/x-python","patch_set":5,"id":"f93f0909_2fea8ace","line":63,"range":{"start_line":63,"start_character":25,"end_line":63,"end_character":71},"updated":"2021-08-17 04:21:48.000000000","message":"you can\u0027t really do this ... this agent class shouldn\u0027t have any knowledge of the internals of the rest client module\n\n(see https://review.opendev.org/c/openstack/os-brick/+/804813 for a different way to handle this)","commit_id":"6ad181acf97ad628c203e1f9f2fa873e63be670f"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"a3f347ccc4548885b3d48dfaa7e2f1c1206c9286","unresolved":false,"context_lines":[{"line_number":60,"context_line":"            CONF.provisioner_entities)"},{"line_number":61,"context_line":"        self.provisioner_rest_client \u003d importutils.import_module("},{"line_number":62,"context_line":"            CONF.provisioner_rest_client)"},{"line_number":63,"context_line":"        self.prov_rest \u003d self.provisioner_rest_client.KioxiaProvisioner("},{"line_number":64,"context_line":"            [CONF.prov_ip], CONF.cert_file, CONF.token, CONF.prov_port)"},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"        scheduler.enter(1, 1, self.call_host_monitor, (30,))"}],"source_content_type":"text/x-python","patch_set":5,"id":"46c5120a_28a97743","line":63,"range":{"start_line":63,"start_character":25,"end_line":63,"end_character":71},"in_reply_to":"22156ad7_8335dedf","updated":"2021-08-18 23:45:45.000000000","message":"Ack","commit_id":"6ad181acf97ad628c203e1f9f2fa873e63be670f"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"892ad9b1c01b6ee767e9eb5667a4bbf087ffb186","unresolved":true,"context_lines":[{"line_number":60,"context_line":"            CONF.provisioner_entities)"},{"line_number":61,"context_line":"        self.provisioner_rest_client \u003d importutils.import_module("},{"line_number":62,"context_line":"            CONF.provisioner_rest_client)"},{"line_number":63,"context_line":"        self.prov_rest \u003d self.provisioner_rest_client.KioxiaProvisioner("},{"line_number":64,"context_line":"            [CONF.prov_ip], CONF.cert_file, CONF.token, CONF.prov_port)"},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"        scheduler.enter(1, 1, self.call_host_monitor, (30,))"}],"source_content_type":"text/x-python","patch_set":5,"id":"22156ad7_8335dedf","line":63,"range":{"start_line":63,"start_character":25,"end_line":63,"end_character":71},"in_reply_to":"f93f0909_2fea8ace","updated":"2021-08-18 10:41:51.000000000","message":"resolving this using a vendor-specific \"agent driver\" class","commit_id":"6ad181acf97ad628c203e1f9f2fa873e63be670f"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"a3f347ccc4548885b3d48dfaa7e2f1c1206c9286","unresolved":true,"context_lines":[{"line_number":62,"context_line":"        (\u0027DEFAULT\u0027, itertools.chain("},{"line_number":63,"context_line":"            nvmeof_agent_opts,"},{"line_number":64,"context_line":"            # each vendor will add their specific opts here"},{"line_number":65,"context_line":"            kioxia_opts,"},{"line_number":66,"context_line":"        )),"},{"line_number":67,"context_line":"    ]"},{"line_number":68,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"109a2f5e_c0adf2ee","line":65,"range":{"start_line":65,"start_character":12,"end_line":65,"end_character":23},"updated":"2021-08-18 23:45:45.000000000","message":"I know this is kind of ugly, but I\u0027m not sure how else we can do this.  Cinder has a script that crawls all the drivers and generates a python file, but I think that\u0027s overkill until we reach the point of there being more than 4 or so agent implementations.","commit_id":"5a45671f4f636a0c8b6bae7b6fe74c0e4df045ca"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"0190af246f9f524e116a85d333311db09b908cb2","unresolved":false,"context_lines":[{"line_number":62,"context_line":"        (\u0027DEFAULT\u0027, itertools.chain("},{"line_number":63,"context_line":"            nvmeof_agent_opts,"},{"line_number":64,"context_line":"            # each vendor will add their specific opts here"},{"line_number":65,"context_line":"            kioxia_opts,"},{"line_number":66,"context_line":"        )),"},{"line_number":67,"context_line":"    ]"},{"line_number":68,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"7124a9b7_c30f4378","line":65,"range":{"start_line":65,"start_character":12,"end_line":65,"end_character":23},"in_reply_to":"109a2f5e_c0adf2ee","updated":"2021-08-19 03:08:51.000000000","message":"I also thought this is kinda ugly, so I did look around for how it\u0027s done in other services, specifically Cinder.\n\nI found https://opendev.org/openstack/cinder/src/branch/master/cinder/opts.py\nWhich essentially does the same thing as here; it imports every driver one by one (not even automatically, they are all explicitly \"hardcoded\" in) - and adds each driver\u0027s custom opts to the output of list_opts()\n\nFor this reason, I found that this is fully consistent with the way Cinder already does it, so I think it should be done this way here too (at least until this whole opts importing and listing approach, including on the Cinder side, is revisited)","commit_id":"5a45671f4f636a0c8b6bae7b6fe74c0e4df045ca"}],"os_brick/cmd/nvmeof_agent/rest_client.py":[{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":1,"context_line":"#    (c)  Copyright  Kioxia Corporation 2021 All Rights Reserved."},{"line_number":2,"context_line":"#"},{"line_number":3,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":4,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":3,"id":"236fe78f_024f45e4","line":1,"updated":"2021-08-12 11:39:16.000000000","message":"If this is exclusive to Kioxia, should it be more apparent in the directory structure or file name?","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":false,"context_lines":[{"line_number":1,"context_line":"#    (c)  Copyright  Kioxia Corporation 2021 All Rights Reserved."},{"line_number":2,"context_line":"#"},{"line_number":3,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":4,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":3,"id":"e08c6f34_bf9672a2","line":1,"in_reply_to":"236fe78f_024f45e4","updated":"2021-08-13 05:37:33.000000000","message":"Great point, thank you! Addressing in patchset 4.","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":334,"context_line":"        KioxiaProvisioner.mgmt_ips[0] \u003d KioxiaProvisioner.mgmt_ips[ip_idx]"},{"line_number":335,"context_line":"        KioxiaProvisioner.mgmt_ips[ip_idx] \u003d temp"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"    # Call Provisioner with get request"},{"line_number":338,"context_line":"    def provisioner_get_request(self, api_name):"},{"line_number":339,"context_line":"        get_visitor \u003d ProvisionerGetVisitor(self.http, api_name)"},{"line_number":340,"context_line":"        provisioner_connector \u003d ProvisionerConnector("}],"source_content_type":"text/x-python","patch_set":3,"id":"8a7eb2c3_6d863cc9","line":337,"updated":"2021-08-12 11:39:16.000000000","message":"Perhaps this comment should be a docstring? Comment for methods below too.","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":true,"context_lines":[{"line_number":334,"context_line":"        KioxiaProvisioner.mgmt_ips[0] \u003d KioxiaProvisioner.mgmt_ips[ip_idx]"},{"line_number":335,"context_line":"        KioxiaProvisioner.mgmt_ips[ip_idx] \u003d temp"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"    # Call Provisioner with get request"},{"line_number":338,"context_line":"    def provisioner_get_request(self, api_name):"},{"line_number":339,"context_line":"        get_visitor \u003d ProvisionerGetVisitor(self.http, api_name)"},{"line_number":340,"context_line":"        provisioner_connector \u003d ProvisionerConnector("}],"source_content_type":"text/x-python","patch_set":3,"id":"aa1eda59_fa4619e7","line":337,"in_reply_to":"8a7eb2c3_6d863cc9","updated":"2021-08-13 05:37:33.000000000","message":"In general I agree and I could go through and make these comment changes, but I have some reasons outlined for why I prefer to keep it the way it is for now in my response on line 378","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"b320adc643befe990ce656e05b4632dbf668fe2a","unresolved":true,"context_lines":[{"line_number":375,"context_line":"        return self.result_support(r)"},{"line_number":376,"context_line":""},{"line_number":377,"context_line":"    def get_info(self):"},{"line_number":378,"context_line":"        # Call to Get Info API"},{"line_number":379,"context_line":""},{"line_number":380,"context_line":"        # @rtype: ProvisionerResponse"},{"line_number":381,"context_line":"        # @returns: Provisioner response data contain Provisioner information"}],"source_content_type":"text/x-python","patch_set":3,"id":"39e4bb88_7ce0526b","line":378,"updated":"2021-08-12 11:39:16.000000000","message":"\"\"\"Call to Get Info API\n\n:returns: Provisioner response data contain Provisioner information\n\"\"\"\n\nSuggestion only.  There may be a reason you have it this way","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"35d66e23911b4d024c1ac19f80b5585508c0eae9","unresolved":true,"context_lines":[{"line_number":375,"context_line":"        return self.result_support(r)"},{"line_number":376,"context_line":""},{"line_number":377,"context_line":"    def get_info(self):"},{"line_number":378,"context_line":"        # Call to Get Info API"},{"line_number":379,"context_line":""},{"line_number":380,"context_line":"        # @rtype: ProvisionerResponse"},{"line_number":381,"context_line":"        # @returns: Provisioner response data contain Provisioner information"}],"source_content_type":"text/x-python","patch_set":3,"id":"bc5adaa0_4c426e0a","line":378,"in_reply_to":"39e4bb88_7ce0526b","updated":"2021-08-13 05:37:33.000000000","message":"This is the standard that our dev team uses, it is not always kept, or kept in the best form, but it is so far the best effort.\n\nAnother thing about this rest_client is that it is Kioxia KumoScale specific, and it appears in the same way in the Cinder repo - in KumoScale cinder driver.\n\nFor this reason I rather keep it the same, to avoid divergence (which will be less confusing for KumoScale team, since this is code that is only specific to Kioxia KumoScale anyway)\n\nFor this os-brick nvmeof connection agent, this KumoScale rest_client is the first \"agent driver\" (other vendors would need to add and maintain their own equivalent)\n\nFinally, we are not the happiest with the duplication of this rest_client (which is now being used both by cinder driver and os-brick agent) - and although this is the current approach, we are looking into solutions of a way to package it as a dependency (that is also \"openstack certified\") for both brick and cinder. Once that is figured out and done, this code should be taken out from here.\n\nHopefully this is a good overview of what\u0027s going on in this file!","commit_id":"ecae9415580693ad5a8e5132d493f6944bfb4dd0"}],"os_brick/tests/privileged/test_nvmeof.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"f6eb7ff66a3afe6f58b3d8e2954e878bb5f54673","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"00e5bc6c_bff30a32","line":137,"updated":"2021-08-15 23:23:52.000000000","message":"It\u0027s worth adding a test similar to test_run_mdadm_err() but that calls run_mdadm() with raise_exception\u003dTrue to guard against someone refactoring the code at some point and breaking that.","commit_id":"1a513ca13d182c769e6ba1d640568afa6efc40fc"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"31c7c148166aa33354e675d21cf62d603d306324","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"25901560_cf8ff605","line":137,"in_reply_to":"00e5bc6c_bff30a32","updated":"2021-08-16 15:46:08.000000000","message":"Done","commit_id":"1a513ca13d182c769e6ba1d640568afa6efc40fc"}],"releasenotes/notes/nvmeof-connection-agent-9d9e45ffb3975c47.yaml":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"f6eb7ff66a3afe6f58b3d8e2954e878bb5f54673","unresolved":true,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    `https://blueprints.launchpad.net/cinder/+spec/nvmeof-connection-agent`"},{"line_number":5,"context_line":"    NVMeOF Connection Agent"},{"line_number":6,"context_line":"    A console script that when launched, is responsible for initiator-side"},{"line_number":7,"context_line":"    monitoring and self healing of NVMeoF connections and replicated mdraid"},{"line_number":8,"context_line":"    volumes."}],"source_content_type":"text/x-yaml","patch_set":4,"id":"44e25311_33f07bfa","line":5,"range":{"start_line":4,"start_character":0,"end_line":5,"end_character":27},"updated":"2021-08-15 23:23:52.000000000","message":"this should be\n\n    `NVMeOF Connection Agent\n    \u003chttps://blueprints.launchpad.net/cinder/+spec/nvmeof-connection-agent\u003e`_:\n    A console script that ...","commit_id":"1a513ca13d182c769e6ba1d640568afa6efc40fc"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"31c7c148166aa33354e675d21cf62d603d306324","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    `https://blueprints.launchpad.net/cinder/+spec/nvmeof-connection-agent`"},{"line_number":5,"context_line":"    NVMeOF Connection Agent"},{"line_number":6,"context_line":"    A console script that when launched, is responsible for initiator-side"},{"line_number":7,"context_line":"    monitoring and self healing of NVMeoF connections and replicated mdraid"},{"line_number":8,"context_line":"    volumes."}],"source_content_type":"text/x-yaml","patch_set":4,"id":"667b3111_09276e50","line":5,"range":{"start_line":4,"start_character":0,"end_line":5,"end_character":27},"in_reply_to":"44e25311_33f07bfa","updated":"2021-08-16 15:46:08.000000000","message":"Done","commit_id":"1a513ca13d182c769e6ba1d640568afa6efc40fc"}],"tox.ini":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"a3f347ccc4548885b3d48dfaa7e2f1c1206c9286","unresolved":true,"context_lines":[{"line_number":148,"context_line":"[testenv:genconfig]"},{"line_number":149,"context_line":"sitepackages \u003d False"},{"line_number":150,"context_line":"envdir \u003d {toxworkdir}/pep8"},{"line_number":151,"context_line":"commands \u003d oslo-config-generator --config-file\u003dtools/config/nvmeof_agent-config-generator.conf"}],"source_content_type":"text/x-properties","patch_set":7,"id":"3dc83c2a_be91c118","line":151,"range":{"start_line":151,"start_character":47,"end_line":151,"end_character":53},"updated":"2021-08-18 23:45:45.000000000","message":"The config generator can\u0027t find this file (you have it in \u0027config\u0027).  You can edit this, or probably better would be to put the file in \u0027tools/config\u0027, which would be consistent with how it\u0027s done in cinder.","commit_id":"5a45671f4f636a0c8b6bae7b6fe74c0e4df045ca"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"0190af246f9f524e116a85d333311db09b908cb2","unresolved":true,"context_lines":[{"line_number":148,"context_line":"[testenv:genconfig]"},{"line_number":149,"context_line":"sitepackages \u003d False"},{"line_number":150,"context_line":"envdir \u003d {toxworkdir}/pep8"},{"line_number":151,"context_line":"commands \u003d oslo-config-generator --config-file\u003dtools/config/nvmeof_agent-config-generator.conf"}],"source_content_type":"text/x-properties","patch_set":7,"id":"fa84d6d6_c72ecb27","line":151,"range":{"start_line":151,"start_character":47,"end_line":151,"end_character":53},"in_reply_to":"3dc83c2a_be91c118","updated":"2021-08-19 03:08:51.000000000","message":"Ack, working on this now, patch coming up shortly.","commit_id":"5a45671f4f636a0c8b6bae7b6fe74c0e4df045ca"}]}