)]}'
{"os_brick/cmd/nvmeof_agent/agent.py":[{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"af39d5eb2872e27cf550453605f50ff43593faab","unresolved":true,"context_lines":[{"line_number":26,"context_line":"from oslo_utils import importutils"},{"line_number":27,"context_line":"from oslo_utils.secretutils import md5"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"from os_brick.cmd.nvmeof_agent.kioxia.rest_client import \\"},{"line_number":30,"context_line":"    kioxia_rest_client_opts"},{"line_number":31,"context_line":"import os_brick.initiator.connectors.nvmeof as nvmeof"},{"line_number":32,"context_line":"from os_brick.privileged import nvmeof as priv_nvme"}],"source_content_type":"text/x-python","patch_set":1,"id":"c57e3ecf_da2bbae5","line":29,"updated":"2021-08-18 10:33:13.000000000","message":"Rather than keep these OpenStack conf standards in the vendor-specific provisioner rest client (which is a mere lib, that is used outside of OpenStack context too) - I prefer keeping them in a vendor driver class (coming up in next patchset) - with the long term approach being that this vendor driver class is an \"agent driver\" that inherits/overrides an \"agent driver interface\" (rather than a \"rest client inteface\")\n\nMore on this in the comments in kioxia/rest_client.py","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"af39d5eb2872e27cf550453605f50ff43593faab","unresolved":true,"context_lines":[{"line_number":51,"context_line":""},{"line_number":52,"context_line":"nvmeof_agent_opts \u003d ["},{"line_number":53,"context_line":"    # this should be a class, or better yet, maybe this should just be"},{"line_number":54,"context_line":"    # handled by the rest client instead of the agent"},{"line_number":55,"context_line":"    cfg.StrOpt(\u0027provisioner_entities\u0027,"},{"line_number":56,"context_line":"               sample_default\u003d\u0027os_brick.cmd.nmveof_agent.kioxia.entities\u0027,"},{"line_number":57,"context_line":"               help\u003d(\u0027Python module that exposes provisioner entities \u0027"}],"source_content_type":"text/x-python","patch_set":1,"id":"662e3687_a1fa8d30","line":54,"updated":"2021-08-18 10:33:13.000000000","message":"good idea, but rather than doing it in the rest client, I am handling this in the kumoscale agent driver coming up in next patchset.","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"}],"os_brick/cmd/nvmeof_agent/kioxia/rest_client.py":[{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"af39d5eb2872e27cf550453605f50ff43593faab","unresolved":true,"context_lines":[{"line_number":26,"context_line":"RUN_COMMAND_TRIALS \u003d 20"},{"line_number":27,"context_line":"RUN_COMMAND_SLEEP \u003d 0.5"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"CONF \u003d cfg.CONF"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"kioxia_rest_client_opts \u003d ["},{"line_number":32,"context_line":"    cfg.IPOpt(\u0027kioxia_prov_ip\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"1217fbae_8b62f73a","line":29,"updated":"2021-08-18 10:33:13.000000000","message":"I understand the need for this, but I rather keep all this out of the vendor-specific rest client class. Instead, I believe the best approach would be to wrap it up in an \"agent driver\" class (which will be the inheritable interface for other drivers, who will handle their own provisioner communication methods in whichever way they choose).\n\nMore on this in my response on line 287","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"af39d5eb2872e27cf550453605f50ff43593faab","unresolved":true,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":"CONF \u003d cfg.CONF"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"kioxia_rest_client_opts \u003d ["},{"line_number":32,"context_line":"    cfg.IPOpt(\u0027kioxia_prov_ip\u0027,"},{"line_number":33,"context_line":"              help\u003d\u0027IP address of the Kioxia provider\u0027),"},{"line_number":34,"context_line":"    cfg.PortOpt(\u0027kioxia_prov_port\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f004842_c8f913d5","line":31,"updated":"2021-08-18 10:33:13.000000000","message":"I could not find anywhere that these kioxia_rest_client_opts get registered (ie. using CONF.register_opts()) - which is why I think I was getting \"oslo_config.cfg.NoSuchOptError: no such option kioxia_prov_ip\"\n\nTo solve this in the coming patchset, I added CONF.register_opts(kioxia_opts) below here in the vendor specific agent driver.","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"780cd946f47a89b1dff9357c864d1d14b28666fb","unresolved":true,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":"CONF \u003d cfg.CONF"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"kioxia_rest_client_opts \u003d ["},{"line_number":32,"context_line":"    cfg.IPOpt(\u0027kioxia_prov_ip\u0027,"},{"line_number":33,"context_line":"              help\u003d\u0027IP address of the Kioxia provider\u0027),"},{"line_number":34,"context_line":"    cfg.PortOpt(\u0027kioxia_prov_port\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"766a18e5_4cf1703e","line":31,"in_reply_to":"1f004842_c8f913d5","updated":"2021-08-18 23:10:55.000000000","message":"You\u0027re right, I forgot to register the options!","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"af39d5eb2872e27cf550453605f50ff43593faab","unresolved":true,"context_lines":[{"line_number":284,"context_line":"        return r"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"# ideally you would define an interface class that each vendor class"},{"line_number":288,"context_line":"# (including yours right here) would inherit from"},{"line_number":289,"context_line":"class KioxiaProvisioner(object):"},{"line_number":290,"context_line":"    #"}],"source_content_type":"text/x-python","patch_set":1,"id":"94101f1a_e5b4cde6","line":287,"updated":"2021-08-18 10:33:13.000000000","message":"Understood, this definitely should be the general direction, what I was thinking is: a good long term approach (which I am working on now) is to create an interface class that wraps the logic calling these vendor-specific provisioner menthods, rather than inheriting and overriding a provisioner rest client class.\n\nMeaning, rather than a common provisioner rest client interface (where methods such as ProvisionerRestClientClass.get_volume_by_uuid() will need to be inherited and overriden by each driver) - instead, wrapper methods in the agent that rely on provisioner communication will be the inheritable/overrideable interface methods, and it will be up to the driver devs to decide how to implement their provisioner communication parts (rather than being forced to follow the same \"provisioner rest client\" format as ours here)\n\nSo, to summarize what I am trying to say: rather than having a \"rest client interface\", the long term goal is to have an \"agent driver interface\".\n\nI am making an attempt to create this agent driver interface in the following patchset, for now it is only dealing with the import and initialization of the provisioner rest client and entities, but over time, certain agent methods (the ones that rely on the provisioner communication which is vendor specific) - will be refactored into this interface class. (while the vendor-agnostic nvme and mdadm stuff will remain in the base agent).","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"780cd946f47a89b1dff9357c864d1d14b28666fb","unresolved":true,"context_lines":[{"line_number":284,"context_line":"        return r"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"# ideally you would define an interface class that each vendor class"},{"line_number":288,"context_line":"# (including yours right here) would inherit from"},{"line_number":289,"context_line":"class KioxiaProvisioner(object):"},{"line_number":290,"context_line":"    #"}],"source_content_type":"text/x-python","patch_set":1,"id":"d3f6adf0_c7f2b4cb","line":287,"in_reply_to":"94101f1a_e5b4cde6","updated":"2021-08-18 23:10:55.000000000","message":"Your approach sounds good.  It\u0027s more like dependency injection than inheritance, and I like that.","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"af39d5eb2872e27cf550453605f50ff43593faab","unresolved":true,"context_lines":[{"line_number":299,"context_line":""},{"line_number":300,"context_line":"    def __init__(self, ips, cert, token, port\u003d8090):"},{"line_number":301,"context_line":"        # is this always going to be one value? you could make the"},{"line_number":302,"context_line":"        # config option type be a list of IPs"},{"line_number":303,"context_line":"        self.mgmt_ips \u003d [CONF.kioxia_prov_ip]"},{"line_number":304,"context_line":"        self.port \u003d CONF.kioxia_prov_port"},{"line_number":305,"context_line":"        self.user \u003d None"}],"source_content_type":"text/x-python","patch_set":1,"id":"251d380b_9fff36eb","line":302,"updated":"2021-08-18 10:33:13.000000000","message":"For now it was always used as a one value, mostly because high availability is achieved using routing (kumoscale provisioner address being a HA VIP).\n\nI will have to double check if putting multiple addresses in is actually working, but it is what the kumoscale rest client expects. I think this improvement could be added as a followup that is kumoscale specific, for now i\u0027m focusing on the generic agent architecture. But thank you for the observation and great suggestion, made a note to revisit this later.","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"780cd946f47a89b1dff9357c864d1d14b28666fb","unresolved":false,"context_lines":[{"line_number":299,"context_line":""},{"line_number":300,"context_line":"    def __init__(self, ips, cert, token, port\u003d8090):"},{"line_number":301,"context_line":"        # is this always going to be one value? you could make the"},{"line_number":302,"context_line":"        # config option type be a list of IPs"},{"line_number":303,"context_line":"        self.mgmt_ips \u003d [CONF.kioxia_prov_ip]"},{"line_number":304,"context_line":"        self.port \u003d CONF.kioxia_prov_port"},{"line_number":305,"context_line":"        self.user \u003d None"}],"source_content_type":"text/x-python","patch_set":1,"id":"31bb5689_c881bf74","line":302,"in_reply_to":"251d380b_9fff36eb","updated":"2021-08-18 23:10:55.000000000","message":"Ack","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"}],"tools/config/nvmeof_agent-config-generator.conf":[{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"af39d5eb2872e27cf550453605f50ff43593faab","unresolved":true,"context_lines":[{"line_number":3,"context_line":"wrap_width \u003d 79"},{"line_number":4,"context_line":"namespace \u003d nvmeof_agent"},{"line_number":5,"context_line":"# not sure if we want this or not"},{"line_number":6,"context_line":"#namespace \u003d oslo.log"}],"source_content_type":"text/plain","patch_set":1,"id":"2192fd96_0a5c0755","line":6,"updated":"2021-08-18 10:33:13.000000000","message":"what does having this here mean?","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"780cd946f47a89b1dff9357c864d1d14b28666fb","unresolved":true,"context_lines":[{"line_number":3,"context_line":"wrap_width \u003d 79"},{"line_number":4,"context_line":"namespace \u003d nvmeof_agent"},{"line_number":5,"context_line":"# not sure if we want this or not"},{"line_number":6,"context_line":"#namespace \u003d oslo.log"}],"source_content_type":"text/plain","patch_set":1,"id":"db8df149_aa9c271b","line":6,"in_reply_to":"2192fd96_0a5c0755","updated":"2021-08-18 23:10:55.000000000","message":"It will include all the options for oslo.log.  It can be useful so that they\u0027re there for operators to see what they can do, but (1) there\u0027s a lot of them, so they kind of overwhelm the 5 or so options that we want to expose for the agent, and (2) operators probably already know about them from configuring other openstack services, so there\u0027s not much value-add from including them.","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"647306b60905a1e931f3b485b784210d61b797db","unresolved":false,"context_lines":[{"line_number":3,"context_line":"wrap_width \u003d 79"},{"line_number":4,"context_line":"namespace \u003d nvmeof_agent"},{"line_number":5,"context_line":"# not sure if we want this or not"},{"line_number":6,"context_line":"#namespace \u003d oslo.log"}],"source_content_type":"text/plain","patch_set":1,"id":"256a95f0_238c2794","line":6,"in_reply_to":"db8df149_aa9c271b","updated":"2021-08-19 02:57:37.000000000","message":"That is good to know, thank you, leaving it out for the reasons you mentioned.","commit_id":"4175dcf943867a4a58d0880d30852dc9c56698ea"}]}