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'.
>>
>> 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.
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
