Re: [SPDK] Changes and invocations to rpc/nvmf.py

Re: [SPDK] Changes and invocations to rpc/nvmf.py

[Verkamp, Daniel]

[-- Attachment #1: Type: text/plain, Size: 3924 bytes --]

Hi Helloway,

Thanks so much for picking up this task! Your patch is exactly the direction we want to move with the JSON-RPC library files in scripts/rpc/*.py.

I will post a few review comments (some minor formatting tweaks), but I’d like to discuss the API design a little bit here so that it gets wider visibility.

I think the goal should be for the scripts/rpc/*.py files to expose native Python-style interfaces as much as possible.  For example, the construct_nvmf_subsystem method currently takes a list of namespaces in “bdev:nsid bdev:nsid bdev:nsid …” pairs, whitespace separated, in a single string parameter.  I would prefer to make this parameter a list of Python objects, one per namespace, something like:

    namespaces = [
        {'bdev_name': 'Malloc0', 'nsid': 1},
        {'bdev_name': 'Malloc1', 'nsid': 2}
    ]

We would still keep the string parsing code, since that is the command-line interface exposed by scripts/rpc.py; it would just be moved out of scripts/rpc/nvmf.py and into the scripts/rpc.py parser function.

I’ve just chosen the namespace list as an example, but there are other parameters with similar issues; anything that is doing string parsing/splitting inside scripts/rpc/nvmf.py should probably be moved to scripts/rpc.py.

This should help make the API clearer and more extensible (and hopefully more like a normal Python module).  Does this make sense? Any comments or suggestions?

Thanks,
-- Daniel

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of helloway
Sent: Thursday, June 7, 2018 7:36 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: [SPDK] Changes and invocations to rpc/nvmf.py

Hi @Daniel, @Paul and @all

I reviewed the code you have updated about ‘pass named args to bdev.py [0]’. I think its functions are consistent with the py_spdk [1] I have proposed in the past. The difference is that you made the changes on the basis of rpc.py, while I considered more not to modify the rpc.py to make adjustments as much as possible. For this reason, I added one more layer (scripts/py_spdk/nvmf_client.py, which can invoke the /scripts/rpc/mvmf.py) on the /script/rpc/*.py module. Actually, this layer can be integrated into /scripts/rpc/nvmf.py by modifying rpc.py. However, I didn’t consider modifying rpc.py to integrate and remove this extra layer. I am afraid of influencing the use of rpc.py.

I think you did a better job, it is also applicable to my usage scenarios. Now I have modified the /rpc/nvmf. py by referring to your code and submitted the patch to the gerrit [2]. At the same time, I have written the call_spdk project about how the upper application calls these modified methods of the patch and put it into my github [3], which directly bypasses the rpc.py and  imports these /scripts/rpc/*. py modules, I hope you can spare some time to visit them together and give me some feedbacks.

Later, I plan to put these implementations into the openstack/cyborg project to realize the management of NVME accelerators. If there is no problem in the submitted code, I am willing to help SPDK community to update the other /scripts/rpc/*.py methods, for example: vhost.py, and then modify the SPDK Driver of openstack/cyborg to use these rpc modules. I don't know what you think about this?

I It is worth mentioning that the SPDK Driver I submitted has been integrated into the Openstack version Q [4]. I wonder if you guys are interested in adopting Cyborg into SPDK to enhance the ecology of SPDK.

Any idea is appreciated. I hope to receive your reply.

[0]https://review.gerrithub.io/c/spdk/spdk/+/411952
[1]https://review.gerrithub.io/c/spdk/spdk/+/405526/1/scripts/py_spdk/nvmf_client.py#93
[2]https://review.gerrithub.io/c/spdk/spdk/+/414296
[3]https://github.com/hellowaywewe/call_spdk
[4]https://review.openstack.org/#/c/538164/

Thx,
Helloway



[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 12086 bytes --]

Re: [SPDK] Changes and invocations to rpc/nvmf.py

[helloway]

[-- Attachment #1: Type: text/plain, Size: 4384 bytes --]

Hi Daniel,


Thanks you for your detailed reply and comments. You are totally right to move the some codes about string parsing/splitting inside scripts/rpc/nvmf.py to scripts/rpc.py. I have updated this part of the code，please visit it.


Thx,
Helloway


On 06/9/2018 00:25，Verkamp, Daniel<daniel.verkamp(a)intel.com> wrote：

Hi Helloway,

 

Thanks so much for picking up this task! Your patch is exactly the direction we want to move with the JSON-RPC library files in scripts/rpc/*.py.

 

I will post a few review comments (some minor formatting tweaks), but I’d like to discuss the API design a little bit here so that it gets wider visibility.

 

I think the goal should be for the scripts/rpc/*.py files to expose native Python-style interfaces as much as possible.  For example, the construct_nvmf_subsystem method currently takes a list of namespaces in “bdev:nsid bdev:nsid bdev:nsid …” pairs, whitespace separated, in a single string parameter.  I would prefer to make this parameter a list of Python objects, one per namespace, something like:

 

    namespaces = [

        {'bdev_name': 'Malloc0', 'nsid': 1},

        {'bdev_name': 'Malloc1', 'nsid': 2}

    ]

 

We would still keep the string parsing code, since that is the command-line interface exposed by scripts/rpc.py; it would just be moved out of scripts/rpc/nvmf.py and into the scripts/rpc.py parser function.

 

I’ve just chosen the namespace list as an example, but there are other parameters with similar issues; anything that is doing string parsing/splitting inside scripts/rpc/nvmf.py should probably be moved to scripts/rpc.py.

 

This should help make the API clearer and more extensible (and hopefully more like a normal Python module).  Does this make sense? Any comments or suggestions?

 

Thanks,

-- Daniel

 

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of helloway
Sent: Thursday, June 7, 2018 7:36 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: [SPDK] Changes and invocations to rpc/nvmf.py

 

Hi @Daniel, @Paul and @all

 

I reviewed the code you have updated about ‘pass named args to bdev.py [0]’. I think its functions are consistent with the py_spdk [1] I have proposed in the past. The difference is that you made the changes on the basis of rpc.py, while I considered more not to modify the rpc.py to make adjustments as much as possible. For this reason, I added one more layer (scripts/py_spdk/nvmf_client.py, which can invoke the /scripts/rpc/mvmf.py) on the /script/rpc/*.py module. Actually, this layer can be integrated into /scripts/rpc/nvmf.py by modifying rpc.py. However, I didn’t consider modifying rpc.py to integrate and remove this extra layer. I am afraid of influencing the use of rpc.py.

 

I think you did a better job, it is also applicable to my usage scenarios. Now I have modified the /rpc/nvmf. py by referring to your code and submitted the patch to the gerrit [2]. At the same time, I have written the call_spdk project about how the upper application calls these modified methods of the patch and put it into my github [3], which directly bypasses the rpc.py and  imports these /scripts/rpc/*. py modules, I hope you can spare some time to visit them together and give me some feedbacks.

 

Later, I plan to put these implementations into the openstack/cyborg project to realize the management of NVME accelerators. If there is no problem in the submitted code, I am willing to help SPDK community to update the other /scripts/rpc/*.py methods, for example: vhost.py, and then modify the SPDK Driver of openstack/cyborg to use these rpc modules. I don't know what you think about this?  

 

I It is worth mentioning that the SPDK Driver I submitted has been integrated into the Openstack version Q [4]. I wonder if you guys are interested in adopting Cyborg into SPDK to enhance the ecology of SPDK.

 

Any idea is appreciated. I hope to receive your reply.

 

[0]https://review.gerrithub.io/c/spdk/spdk/+/411952 

[1]https://review.gerrithub.io/c/spdk/spdk/+/405526/1/scripts/py_spdk/nvmf_client.py#93 

[2]https://review.gerrithub.io/c/spdk/spdk/+/414296 

[3]https://github.com/hellowaywewe/call_spdk 

[4]https://review.openstack.org/#/c/538164/

 

Thx,

Helloway    




[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 13214 bytes --]

[SPDK] Changes and invocations to rpc/nvmf.py

[helloway]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]

Hi @Daniel, @Paul and @all




I reviewed the code you have updated about ‘pass named args to bdev.py [0]’. I think its functions are consistent with the py_spdk [1] I have proposed in the past. The difference is that you made the changes on the basis of rpc.py, while I considered more not to modify the rpc.py to make adjustments as much as possible. For this reason, I added one more layer (scripts/py_spdk/nvmf_client.py, which can invoke the /scripts/rpc/mvmf.py) on the /script/rpc/*.py module. Actually, this layer can be integrated into /scripts/rpc/nvmf.py by modifying rpc.py. However, I didn’t consider modifying rpc.py to integrate and remove this extra layer. I am afraid of influencing the use of rpc.py.




I think you did a better job, it is also applicable to my usage scenarios. Now I have modified the /rpc/nvmf. py by referring to your code and submitted the patch to the gerrit [2]. At the same time, I have written the call_spdk project about how the upper application calls these modified methods of the patch and put it into my github [3], which directly bypasses the rpc.py and  imports these /scripts/rpc/*. py modules, I hope you can spare some time to visit them together and give me some feedbacks.




Later, I plan to put these implementations into the openstack/cyborg project to realize the management of NVME accelerators. If there is no problem in the submitted code, I am willing to help SPDK community to update the other /scripts/rpc/*.py methods, for example: vhost.py, and then modify the SPDK Driver of openstack/cyborg to use these rpc modules. I don't know what you think about this?  




I It is worth mentioning that the SPDK Driver I submitted has been integrated into the Openstack version Q [4]. I wonder if you guys are interested in adopting Cyborg into SPDK to enhance the ecology of SPDK.




Any idea is appreciated. I hope to receive your reply.

 

[0]https://review.gerrithub.io/c/spdk/spdk/+/411952 

[1]https://review.gerrithub.io/c/spdk/spdk/+/405526/1/scripts/py_spdk/nvmf_client.py#93 

[2]https://review.gerrithub.io/c/spdk/spdk/+/414296 

[3]https://github.com/hellowaywewe/call_spdk 

[4]https://review.openstack.org/#/c/538164/




Thx,

Helloway    


[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 41410 bytes --]