On 27.10.23 16:27, Eelco Chaudron wrote:
> On 25 Oct 2023, at 11:37, jm...@redhat.com wrote:
>> From: Jakob Meng <c...@jakobmeng.de>
>>
>> Add global option to output JSON from ovs-appctl cmds.
>>
>> This patch is an update of [0] with the following major changes:
>> * The JSON-RPC API change is now backward compatible. 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 are
>> updated.
>> * tests/pmd.at from forth patch now features an example of how the
>> output looks like when a command does not support JSON output.
>> * The patch has been split into a series of four. The first patch
>> introduces the '-f,--format' option for ovs-appctl/ovs-dpctl and
>> necessary changes to the JSON-RPC API. It does not yet pass the
>> output format to individual commands because that requires a lot
>> of changes. Those changes have been split out into the third patch
>> to increase readability of the series.
>> * The second patch introduces equivalent changes to the Python files.
>> * The third patch moves all commands to the updated functions in
>> lib/unixctl.*, in particular unixctl_command_register() and the
>> unixctl_cb_func type, as well as their Python counterparts. The
>> output is still text-only (no json) for all commands.
>> * The forth patch shows how JSON output could be implemented using
>> 'dpif/show' as an example.
>>
>> The following paragraphs are taken from the previous patch revision
>> and have been updated to changes mentioned above.
>>
>> For monitoring systems such as Prometheus it would be beneficial if OVS
>> and OVS-DPDK would expose statistics in a machine-readable format.
>> Several approaches like UNIX socket, OVSDB queries and JSON output from
>> ovs-xxx tools have been proposed [2],[3]. This proof of concept
>> describes one way how ovs-xxx tools could output JSON in addition to
>> plain-text for humans.
>>
>> This patch follows an alternative approach to RFC [1] which
>> implemented JSON output as a separate option for each command like
>> 'dpif/show'. The option was called '-o|--output' in the latter. It
>> has been renamed to '-f,--format' because ovs-appctl already has a
>> short option '-o' which prints the available ovs-appctl options
>> ('--option'). The new option name '-f,--format' is in line with
>> ovsdb-client where it controls output formatting, too.
>>
>> An example call would be 'ovs-appctl --format json dpif/show' as
>> shown in tests/pmd.at of the forth patch. By default, the output
>> format is plain-text as before.
>>
>> With this patch, all commands announce their support for output
>> formats when being registered with unixctl_command_register() from
>> lib/unixctl.*, e.g. OVS_OUTPUT_FMT_TEXT and/or OVS_OUTPUT_FMT_JSON.
>> When a requested output format is not supported by a command, then
>> process_command() in lib/unixctl.c will return an error. This is an
>> advantage over the previous approach [1] where each command would have
>> to parse the output format option and handle requests for unsupported
>> output formats on its own.
>>
>> The question whether JSON output should be pretty printed and sorted
>> remains. In most cases it would be unnecessary because machines
>> consume the output or humans could use jq for pretty printing. However,
>> it would make tests more readable (for humans) without having to use jq
>> (which would require us to introduce a dependency on jq).
> Hi Jakob,
>
> I had some code-related comments on V1, which I do not see addressed or
> replied to. Did you miss them? Anyway, I’ll go over my old review notes, and
> add them to the split-up patch.
Which one did I miss?
The note on JSON pretty printing I left here as a remainder for the discussion
on last thursday. With the consensus on adding an extra flag for pretty
printing I will remove it in the next patch version.
You asked about freeing args->target which is done a couple of lines above in
"cmdl_args_destroy()".
The rest of your comments has been incorporated in v3 in one way or another 😉
Best,
Jakob
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
