On 10/27/23 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.
BTW, there is no such separate thing as OVS-DPDK, it's just OVS.
>>>
>>> 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'.
>>>
>>> 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).
It has to be an array, not an object. Parameters are positional.
Unless you want to change API for every existing command and give
each argument a unique name.
And that will not help with supporting older servers, because they
expect an array.
>> 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.
>
> 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
