Re: [SPDK] About dump/load (or export/edit/import) JSON file by SPDK

[Harris, James R <james.r.harris at intel.com>]

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



From: SPDK <spdk-bounces(a)lists.01.org> on behalf of "Wodkowski, PawelX" <pawelx.wodkowski(a)intel.com>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org>
Date: Friday, February 2, 2018 at 7:19 AM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] About dump/load (or export/edit/import) JSON file by SPDK

Hi Shuhei,

<trim>

About your questions:


> - the top layer of a JSON configuration file is a JSON object

I vote for this



I agree – making the JSON configuration file a JSON object seems like a good idea.



I would like to consider allowing comments + something like JSON-minify though.  https://github.com/getify/JSON.minify/tree/python



I’d love Daniel’s input on this – 50/50 chance he’ll either like the minify idea, or will flame me for suggesting it.



> - the object has the following key-value pairs:

>  - key = "apptype", value = "name of application type",

I don’t see any value in adding this. If we decide to merge all apps into one capable of serving iSCSI, vhost,

NVMeF what to do with this type of field. It will be dead anyway.



I agree with Pawel here.  I could see cases for testing where we may even describe the configuration using multiple JSON-RPC files.  For example, one configuration file to construct a bunch of block devices that are common across a bunch of different tests.  Then each test could provide a second configuration file that only provides specifics for the upper layer target (iSCSI, NVMe-oF, vhost, etc.)



>  - key = "jsonrpc", value = "2.0",

>  - key = "sequence", value = an array of request objects which is made of ID, method, and params

>  - Currently ID of JSON-RPC request is always 1.

>  - Change it to the sequence number of each JSON-RPC request to call JSON-RPC requests in order.

User app might want to read many config files as well want to save config of each subsystem to different file.

I would vote against embedding “jsonrpc”, “id” etc in any JSON config file. Only data needed to fully restore

application configuration should be needed in JSON config file.

> Do you think any general metadata should be included in the file or just sequence of requests is enough?
For other “metadata” reported by RPC calls there could be some additional call like “get_info”.

I think a sequence of requests is enough – plus ability to add comments in the JSON.

I think a new RPC that accepts a batch of other JSON-RPC objects would be challenging from an error handling perspective.  For example, what happens if the 4th RPC out of a batch of 20 fails?  Sure – we could try to define a protocol for this but I think having the client just send one RPC at a time is sufficient for now.

I don’t hide that I’m under impression of the QEMU QMP and QAPI. They did excellent work in similar area,
maybe we could base our JSON(-RPC) API on their ideas in some way. What do you think?

And last topic: are we ready to describe those changes on trello?

Yes – I think starting to get some details in Trello would be excellent.

Regarding Shuhei’s question on maintaining old INI config file capability – yes, we will need to keep both for some period of time.  We will need to officially deprecate the INI config file format first, but cannot do that until JSON-RPC configuration is fully functional.  If JSON-RPC is fully functional for the 18.04 release, we could deprecate it for 18.04 and remove it in 18.07 possibly or more likely 18.10.

Paweł

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of ???? / MATSUMOTO,SHUUHEI
Sent: Friday, February 2, 2018 10:04 AM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] About dump/load (or export/edit/import) JSON file by SPDK


Hi Jim, Daniel and All,



I found another questions and would like to put them on the discussion in this week.



The batch request format may be better as the format of JSON config file.


[
    { "jsonrpc": "2.0", "id": 1, "method": "add_portal_group", "params": { ... } },
    { "jsonrpc": "2.0", "id": 2, "method": "add_portal_group", "params": { ... } }
]

Do you think any general metadata should be included in the file or just sequence of requests is enough?

Additionally do you think which is reasonable? I think the former is enough for now.
- batch requests is managed by the python RPC client and the python RPC client send one of each.
- SPDK support batch requests and the python RPC client send batch requests, SPDK RPC handler processes one of each.

Any of your feedback is valuable.

Thank you,
Shuhei

________________________________
差出人: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> が 松本周平 / MATSUMOTO，SHUUHEI <shuhei.matsumoto.xt(a)hitachi.com<mailto:shuhei.matsumoto.xt(a)hitachi.com>> の代理で送信
送信日時: 2018年2月2日 10:22
宛先: Storage Performance Development Kit
件名: [!]Re: [SPDK] About dump/load (or export/edit/import) JSON file by SPDK


Hi Jim and All,



Sorry I forgot to bring an item onto the discussion.



We should keep the current config file implementation for compatibility?

Or is it OK to replace the whole to the new one once everything is prepared?



Thank you,

Shuhei

________________________________
差出人: 松本周平 / MATSUMOTO，SHUUHEI
送信日時: 2018年2月2日 10:12:23
宛先: Storage Performance Development Kit
件名: RE: [SPDK] About dump/load (or export/edit/import) JSON file by SPDK


Hi Jim and All,



Thank you for valuable feedback to go forward.

Based on your feedback, I'm happy if we can share the idea of the format of the SPDK JSON configuration file.



One key question is whether the configuration file should be a set of JSON objects which are just SPDK JSON-RPC requests, or if they are set of some new type of SPDK JSON objects.



My thought is that a JSON configuration file could just be a sequence of JSON-RPC requests.  Each SPDK subsystem/module would need to know how to generate a sequence of JSON-RPC requests to rebuild its current state.  This would be similar to the get_running_config() routines that many of the subsystems and modules have today which generate an INI config file when a process receives a USR1 signal.

Make sense to me. How do you think the following:



- the top layer of a JSON configuration file is a JSON object

- the object has the following key-value pairs:

  - key = "apptype", value = "name of application type",

  - key = "jsonrpc", value = "2.0",

  - key = "sequence", value = an array of request objects which is made of ID, method, and params

  - Currently ID of JSON-RPC request is always 1.

  - Change it to the sequence number of each JSON-RPC request to call JSON-RPC requests in order.


{
    "apptype": "iscsi-tgt",
    "jsonrpc": "2.0",
    "sequence" : [
        {
            "id": 1,
            "method": "add_portal_group",
            "params": {
                "tag": 2,
                "portals": [
                    {
                        "host": "192.168.2,21",
                        "port": "3260"
                    }
                ]
            }
        },
        {
            "id": 2,
            "method": "add_portal_group",
            "params": {
                "tag": 2,
                "portals": [
                    {
                        "host": "192.168.2,22",
                        "port": "3260"
                    }
                ]
            }
        },
    ]
}


I think this should be straightforward.  spdk_app_start() and spdk_subsystem_init() currently use the following order:

1)      Initialize subsystems

2)      Initialize RPC

3)      Start reactors



It basically needs to change to:

1)      Initialize RPC

2)      Start reactors

3)      Initialize subsystems

Thank you, very clear for me now.





However I think only python implementation is not enough and  importing/exporting JSON file by SPDK C library will be valuable at least

- for unit tests

- for iSCSI CHAP shared secret file. (Sending secrets written in plain text by RPC is not comfortable for me.)

Yes – I agree, we will want a small C library that could read the JSON-RPC objects and execute them internally.
OK, I got it.

Thank you,
Shuhei



________________________________
差出人: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> が Harris, James R <james.r.harris(a)intel.com<mailto:james.r.harris(a)intel.com>> の代理で送信
送信日時: 2018年2月2日 6:00
宛先: Storage Performance Development Kit
件名: [!]Re: [SPDK] About dump/load (or export/edit/import) JSON file by SPDK


Hi Shuhei,



Thank you for starting this discussion – I have some comments inline below.



-Jim





From: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of 松本周平 / MATSUMOTO，SHUUHEI <shuhei.matsumoto.xt(a)hitachi.com<mailto:shuhei.matsumoto.xt(a)hitachi.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Wednesday, January 31, 2018 at 6:13 PM
To: "spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>" <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: [SPDK] About dump/load (or export/edit/import) JSON file by SPDK



Hi All,



As you know, current variant of INI configuration file is dated and SPDK have great JSON-RPC stack.



I think as KVM utilize effectively XML configuration file well, utilizing JSON configuration file will be valuable for SPDK.

By doing this SPDK will be able to have persistent configuration by itself.



As long as I investigate, now SPDK support
(1) create and dump JSON object to fixed size JSON-RPC buffer
(2) load JSON object from JSON-RPC and then parse/convert it to a spdk_json_vals table to decode it



Adding the following may be helpful:

(3) create and dump JSON object to resizable string buffer and then to file/fd
(4) load JSON object from file/fd and then parse/convert it to a spdk_json_vals table to decode it



About (3), having pretty print function in which indent/newlines/whitespace can be tuned will be desirable.



One key question is whether the configuration file should be a set of JSON objects which are just SPDK JSON-RPC requests, or if they are set of some new type of SPDK JSON objects.



My thought is that a JSON configuration file could just be a sequence of JSON-RPC requests.  Each SPDK subsystem/module would need to know how to generate a sequence of JSON-RPC requests to rebuild its current state.  This would be similar to the get_running_config() routines that many of the subsystems and modules have today which generate an INI config file when a process receives a USR1 signal.



Furthermore,
I would like to add the following in Python JSON-RPC code,
(5) for reply, dump response to JSON file
(6) for send, load JSON file to Python object, and convert (dump) to JSON object request, and then send to SPDK.

((3) and (5)) or ((4) and (6)) may be duplicated each other, respectively,
and the following approach make sense as I talked with Jim shortly.

- SPDK start only JSON-RPC first
- and then SPDK python handles all configuration files



I think this should be straightforward.  spdk_app_start() and spdk_subsystem_init() currently use the following order:

1)      Initialize subsystems

2)      Initialize RPC

3)      Start reactors



It basically needs to change to:

1)      Initialize RPC

2)      Start reactors

3)      Initialize subsystems



Where step #3 (initialize subsystems) is triggered by a new RPC.



However I think only python implementation is not enough and  importing/exporting JSON file by SPDK C library will be valuable at least

- for unit tests

- for iSCSI CHAP shared secret file. (Sending secrets written in plain text by RPC is not comfortable for me.)



Yes – I agree, we will want a small C library that could read the JSON-RPC objects and execute them internally.

I can't say I have mastered JSON but as long as I read and learn JSON and SPDK's JSON implementation,
we will be able to do the above without difficulty.



The first major use case may be to replace SPDK subsystem's config dump function from the variant of INI format to JSON format.



I would like to post patch one by one or series of patches if prepared but discussion in Trello first may be better because each will have own idea in this area.



I'm happy for your any feedback.



Best Regards,

Shuhei



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