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