On 10/30/23 10:52, Eelco Chaudron wrote:
>
>
> On 27 Oct 2023, at 23:51, Ilya Maximets wrote:
>
>> On 10/26/23 13:44, Jakob Meng wrote:
>>> On 25.10.23 11:37, jm...@redhat.com wrote:
>>>> From: Jakob Meng <c...@jakobmeng.de>
>>>>
>>>> For monitoring systems such as Prometheus it would be beneficial if
>>>> OVS and OVS-DPDK would expose statistics in a machine-readable format.
>>>>
>>>> This patch introduces support for different output formats to ovs-xxx
>>>> tools. They gain a global option '-f,--format' which allows users to
>>>> request JSON instead of plain-text for humans. An example call
>>>> implemented in a later patch is 'ovs-appctl --format json dpif/show'.
>
> I started to look at some of the code, but I guess some of the comments from
> Ilya could change the implementation, so I’ll stop for now.
>
> Please take a look at my commands from the v1 review, and incorporate them if
> still valid after potential changes (and flake8 warnings).
>
> Also, instead of TODO use use 'XXX' or 'FIXME' as mentioned in the coding
> style.
>
> On the ‘pretty print or not discussion’, I added the following:
>
> I think we should use the same option as ovs-vsctl has for the dbase.
>
> -f, --format=FORMAT set output formatting to FORMAT
> ("table", "html", "csv", or "json")
> --pretty pretty-print JSON in output
>
> This looks like the following:
>
> ovs-vsctl --format=json list Open_vSwitch
> {"data":[[["uuid","8acf0aa8-7b3c-4b2a-9a15-6a4143ef63f2"],["uuid","630c4d69-c905-4645-ba59-66fd0bfa66c7"],32,["set",["netdev","system"]],["map",[]],"8.3.1",true,"DPDK
>
> 22.11.1",["map",[["hostname","wsfd-advnetlab224.anl.lab.eng.bos.redhat.com"],["rundir","/var/run/openvswitch"],["system-id","5ee96c43-4823-4456-9bc3-9616c1acdce3"]]],["set",["bareudp","dpdk","dpdkvhostuser","dpdkvhostuserclient","erspan","geneve","gre","gtpu","internal","ip6erspan","ip6gre","lisp","patch","stt","system","tap","vxlan"]],["set",[]],32,["map",[["dpdk-init","true"],["dpdk-socket-mem","2048"],["pmd-cpu-mask","0x4000000040000000"]]],"3.1.4",["set",[]],["map",[]],"rhel","9.2"]],"headings":["_uuid","bridges","cur_cfg","datapath_types","datapaths","db_version","dpdk_initialized","dpdk_version","external_ids","iface_types","manager_options","next_cfg","other_config","ovs_version","ssl","statistics","system_type","system_version"]}
> [wsfd-advnetlab224:~]$ ovs-vsctl --format=json list Open_vSwitch
>
> [wsfd-advnetlab224:~]$ ovs-vsctl --format=json --pretty list Open_vSwitch
> {
> "data": [
> ...
> }
>
>>>> For that it is necessary to change the JSON-RPC API lib/unixctl.*
>>>> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
>>>> sockets. It now allows to transport the requested output format
>>>> besides the command and its args. This change has been implemented in
>>>> a backward compatible way. One can use an updated client
>>>> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
>>>> Of course, JSON output only works when both sides have been updated.
>>>>
>>>> Previously, the command was copied to the 'method' parameter in
>>>> JSON-RPC while the args were copied to the 'params' parameter. Without
>>>> any other means to transport parameters via JSON-RPC left, the meaning
>>>> of 'method' and 'params' had to be changed: 'method' will now be
>>>> 'execute/v1' when an output format other than 'text' is requested. In
>>>> that case, the first parameter of the JSON array 'params' will now be
>>>> the designated command, the second one the output format and the rest
>>>> will be command args.
>>>
>>> Ilya brought up the question why I changed the meaning of 'method' and
>>> 'params' instead of adding the output format as an addition argument to the
>>> command arguments in 'params'. The server side would then interpret and
>>> filter out this argument before passing the remaining arguments to the
>>> command callbacks.
>>>
>>> I decided against this approach because the code would get more involved,
>>> in particular we would have to implement option/argument parsing inside
>>> process_command() in lib/unixctl.c. (Besides, using getopt*() functions in
>>> a safe way would be difficult in general because their global state.)
>>> The current implementation is based purely on JSON objects/arrays which are
>>> nicely supported by OVS with functions from lib/json.*.
>>
>> I'm not sure I got this point, but see below.
>>
>>>
>>> Ilya also voiced concerns about the limited extensibility of the proposed
>>> API. To fix this, this patch series could be tweaked in the follow way:
>>>
>>> (1.) Instead of passing the output format as second entry in the JSON array
>>> 'params', turn the second entry into a JSON object (shash). The output
>>> format would be one entry in this JSON object, e.g. called 'format'.
>>>
>>> The server (process_command() from lib/unixctl.c) will parse the content of
>>> this JSON object into known options. Clients would only add non-default
>>> options to this JSON object to keep compatibility with older servers.
>>> Options which are supported by the server but not transferred via this JSON
>>> object would be initialized with default values to keep compatibility with
>>> older clients. Unknown entries would cause the server to return an error.
>>
>> This kind of implements what I had in mind, but in a slightly different
>> manner. How about something like this:
>>
>> * ovs-appctl has a global parameter --format passed to it, not part of
>> the requested command paramaters. (I think this is implemented in the
>> code, below, but I didn't read very carefully.)
>>
>> * ovs-appctl adds this paramater as an argument to a 'params' JSON array.
>> i.e. separate element in the array to every command it executes, if
>> provided.
>>
>> * Server (ovs-vswitchd, ovsdb-server, etc.) receives the request, finds
>> that argument, removes it from the list of parameters before passing
>> to the actual handler.
>>
>> * Before calling a handler, server stores these special parameters
>> into struct unixctl_conn (maybe a sub-struct inside it, but it doesn't
>> really matter).
>>
>> * Handler always has the conn pointer. Handler can ask in which format
>> the user prefers the output and generate one. E.g.:
>>
>> if (unixctl_command_reply_format(conn) == UNIXCTL_REPLY_FMT_JSON) {
>> unixctl_command_reply_json(conn, ...);
>> } else {
>> unixctl_command_reply(conn, ...);
>> }
>>
>> Notice that nothing bad happens if command is not aware of the formats,
>> because unixctl_command_reply() is a JSON reply, but with a simple
>> JSON string instead of any fancy structure.
>>
>> * Result is sent back to the ovs-appctl.
>>
>> * If the --format=json was originally requested, just dump the result
>> as JSON to the output in the unixctl_client_transact(). If format
>> wasn't requested, parse it and print out the same way as it is done
>> right now, if the result is not a simple JSON string - error.
>>
>> This approach requires no changes in the code that doesn't want to provide
>> JSON output and no changes to the command registering API.
>>
>> All commands support JSON output automatically. But commands that do
>> not handle it specially will just return a simple JSON string. We will
>> print it out as a JSON string, there is nothing bad in this.
>
> I think we should report an error in this case, as it will make it clear that
> the requested JSON format is not supported. This way it might be easy to
> differentiate between an implementation with or without explicit JSON output
> support.
I'm not sure it's actually possible to detect. There is no difference
between JSON string as an actual JSON result and a JSON string created
from a simple output. I guess, the only way could be is to require that
the argument of unixctl_command_reply_json() has to be a JSON_OBJECT.
>
>> Prettyfication can be done on the ovs-appctl side by an extra argument
>> that doesn't need to be transferred to the server (already done this way).
>>
>> The only issue is that it's hard to tell what happened when a new
>> ovs-appctl --format=json communicates with the old server and gets a
>> number of arguments error. But we could add a hint 'Can be caused by an
>> older <target> not supporting JSON format' to every such reply if format
>> was requested and let user try without format and get a proper error if
>> there is one. In any case, that should not be a frequent case, as I
>> would expect the most users of JSON format will be scripts with their
>> own JSON-RPC implementation, not ovs-appctl.
>>
>> Thoughts?
>>
>> Some more comments below.
>>
>>>
>>> For (2.) see below.
>>>
>>>>
>>>> unixctl_command_register() in lib/unixctl.* has been cloned as
>>>> unixctl_command_register_fmt() in order to demonstrate the new API.
>>>> The latter will be replaced with the former in a later patch. The
>>>> new function gained an int argument called 'output_fmts' with which
>>>> commands have to announce their support for output formats.
>>>>
>>>> Function type unixctl_cb_func in lib/unixctl.h has gained a new arg
>>>> 'enum ovs_output_fmt fmt'. For now, it has been added as a comment
>>>> only for the huge changes reason mentioned earlier. The output format
>>>> which is passed via unix socket to ovs-vswitchd will be converted into
>>>> a value of type 'enum ovs_output_fmt' in lib/unixctl.c and then passed
>>>> to said 'fmt' arg of the choosen command handler (in a future patch).
>>>> When a requested output format is not supported by a command,
>>>> then process_command() in lib/unixctl.c will return an error.
>>>
>>> (2.) Instead of adding 'enum ovs_output_fmt fmt' as a new arg to function
>>> type unixctl_cb_func, an indirection will be used:
>>>
>>> The enum value will be put into a struct and the struct will be added to
>>> unixctl_cb_func. This would allow us to add new options easily without
>>> having to change all command callbacks again.
>>
>> We already have strunc unixctl_conn where we can store generic things
>> and have functions-accessors.
>>
>>>
>>> Wdyt?
>>>
>>>> This patch does not yet pass the choosen output format to commands.
>>>> Doing so requires changes to all unixctl_command_register() calls and
>>>> all command callbacks. To improve readibility those changes have been
>>>> split out into a follow up patch. Respectively, whenever an output
>>>> format other than 'text' is choosen for ovs-xxx tools, they will fail.
>>>> By default, the output format is plain-text as before.
>>>>
>>>> In popular tools like kubectl the option for output control is usually
>>>> called '-o|--output' instead of '-f,--format'. But ovs-appctl already
>>>> has an short option '-o' which prints the available ovs-appctl options
>>>> ('--option'). The now choosen name also better alines with ovsdb-client
>>>> where '-f,--format' controls output formatting.
>>>>
>>>> Reported-at: https://bugzilla.redhat.com/1824861
>>>> Signed-off-by: Jakob Meng <c...@jakobmeng.de>
>>>> ---
>>>> lib/command-line.c | 36 ++++++
>>>> lib/command-line.h | 10 ++
>>>> lib/dpctl.h | 4 +
>>>> lib/unixctl.c | 260 ++++++++++++++++++++++++++++++++---------
>>>> lib/unixctl.h | 17 ++-
>>>> tests/pmd.at | 5 +
>>>> utilities/ovs-appctl.c | 65 ++++++++---
>>>> utilities/ovs-dpctl.c | 12 ++
>>
>> Don't touch interface of the ovs-dpctl binary. It doesn't need to have
>> JSON output. ovs-appctl dpctl/ should pretty much always be used instead.
>>
>> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
