diff mbox series

[ovs-dev,v9,6/6] ofproto: Add JSON output for 'dpif/show' command.

Message ID 20240412072638.712622-7-jmeng@redhat.com
State Under Review
Delegated to: Ilya Maximets
Headers show
Series Add global option to output JSON from ovs-appctl cmds. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation fail test: fail

Commit Message

Jakob Meng April 12, 2024, 7:26 a.m. UTC
From: Jakob Meng <code@jakobmeng.de>

The 'dpif/show' command now supports machine-readable JSON output in
addition to the plain-text output for humans. An example would be:

  ovs-appctl --format json dpif/show

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng <code@jakobmeng.de>
---
 NEWS                   |   1 +
 ofproto/ofproto-dpif.c | 124 +++++++++++++++++++++++++++++++++++++----
 tests/pmd.at           |  28 ++++++++++
 3 files changed, 142 insertions(+), 11 deletions(-)

Comments

Ilya Maximets April 12, 2024, 10:55 p.m. UTC | #1
On 4/12/24 09:26, jmeng@redhat.com wrote:
> From: Jakob Meng <code@jakobmeng.de>
> 
> The 'dpif/show' command now supports machine-readable JSON output in
> addition to the plain-text output for humans. An example would be:
> 
>   ovs-appctl --format json dpif/show
> 
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng <code@jakobmeng.de>
> ---
>  NEWS                   |   1 +
>  ofproto/ofproto-dpif.c | 124 +++++++++++++++++++++++++++++++++++++----
>  tests/pmd.at           |  28 ++++++++++
>  3 files changed, 142 insertions(+), 11 deletions(-)
> 

Hi, Jakob.  Thanks for v9!

The general approach in the set seems reasonable, however I didn't
read the code carefully enough.  I hope to do that once I'm back
from PTO in one week.

I didn't read the code in this patch either, but I have a couple
of general comments for the formatting below.

> diff --git a/NEWS b/NEWS
> index af83623b3..97b30233c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,7 @@ Post-v3.3.0
>         E.g. members of objects and elements of arrays are printed one per line,
>         with indentation.
>       * 'list-commands' now supports output in JSON format.
> +     * 'dpif/show' now supports output in JSON format.
>     - Python:
>       * Added support for choosing the output format, e.g. 'json' or 'text'.
>       * Added new option [--pretty] to print JSON output in a readable fashion.

These NEWS entries look strange.  "Output format for python" sounds
strange.  Is it for every python library we have?  The section should
be more specific on what it means.

<snip>

> diff --git a/tests/pmd.at b/tests/pmd.at
> index 35a44b4df..cff80da15 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -112,6 +112,34 @@ dummy@ovs-dummy: hit:0 missed:0
>      p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
>  ])
>  
> +AT_CHECK([ovs-appctl --format json --pretty dpif/show], [0], [dnl
> +[[
> +  {
> +    "name": "dummy@ovs-dummy",
> +    "ofprotos": [
> +      {
> +        "name": "br0",
> +        "ports": [
> +          {
> +            "netdev_config": {
> +              },
> +            "netdev_name": "br0",
> +            "netdev_type": "dummy-internal",
> +            "odp_port": "100",
> +            "ofp_port": "65534"},
> +          {
> +            "netdev_config": {
> +              "n_rxq": "1",
> +              "n_txq": "1",
> +              "numa_id": "0"},
> +            "netdev_name": "p0",
> +            "netdev_type": "dummy-pmd",
> +            "odp_port": "1",
> +            "ofp_port": "1"}]}],
> +    "stats": {
> +      "n_hit": "0",
> +      "n_missed": "0"}}]]])

I'd suggest a different format for this command, e.g.:

{
  "dummy@ovs-dummy": {
    "bridges": {
      "br0": {
        "br0": {
           "config": {},
           "type": "dummy-internal",
           "port_no": "100",
           "ofport": "65534"},
        "p0": {
           "config": {
               "n_rxq": "1",
               "n_txq": "1",
               "numa_id": "0"},
           "type": "dummy-pmd",
           "port_no": "1",
           "ofport": "1"}}},
    "stats": {
      "n_hit": "0",
      "n_missed": "0"}}
}

Using dictionaries with names as keys saves us from some of the strange
naming.  "ofprotos" is a strange word and is not understandable by users.
It's an internal word.  The user-facing entity is a bridge, not ofproto.
We don't need to have "netdev_" prefixes, it's already clear that it is
a port configuration if it is a part of port values.  All datapaths,
bridges and ports have unique names, so they should be keys in JSON objects,
no need to use arrays.  "ofp_port" means "OpenFlow port port", no need to
repeat.  In the database we have "ofport" name, so we can use it here as
well.  "dpif" commands operate with "port_no" as a datapath port number,
so that should be used instead of "odp_port".

Does that make sense?

Also, I noticed that non-pretty output is not sorted, this may cause
random CI failures, because the order depends on a hash function for
smap.  I'd say the output should always be sorted, or we'll have to
somehow sort the values in tests.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index af83623b3..97b30233c 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,7 @@  Post-v3.3.0
        E.g. members of objects and elements of arrays are printed one per line,
        with indentation.
      * 'list-commands' now supports output in JSON format.
+     * 'dpif/show' now supports output in JSON format.
    - Python:
      * Added support for choosing the output format, e.g. 'json' or 'text'.
      * Added new option [--pretty] to print JSON output in a readable fashion.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 32d037be6..67e1df550 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6519,8 +6519,104 @@  done:
     return changed;
 }
 
+static struct json *
+dpif_show_backer_json(const struct dpif_backer *backer)
+{
+    struct json *json_backer = json_object_create();
+
+    /* Add dpif name to JSON. */
+    json_object_put_string(json_backer, "name", dpif_name(backer->dpif));
+
+    /* Add dpif stats to JSON. */
+    struct dpif_dp_stats dp_stats;
+    struct json *json_dp_stats = json_object_create();
+
+    dpif_get_dp_stats(backer->dpif, &dp_stats);
+    json_object_put_format(json_dp_stats, "n_hit", "%"PRIu64, dp_stats.n_hit);
+    json_object_put_format(json_dp_stats, "n_missed", "%"PRIu64,
+                           dp_stats.n_missed);
+    json_object_put(json_backer, "stats", json_dp_stats);
+
+    /* Add ofprotos to JSON. */
+    struct json *json_ofprotos = json_array_create_empty();
+    struct shash ofproto_shash;
+    shash_init(&ofproto_shash);
+    const struct shash_node **ofprotos = get_ofprotos(&ofproto_shash);
+    for (size_t i = 0; i < shash_count(&ofproto_shash); i++) {
+        struct ofproto_dpif *ofproto = ofprotos[i]->data;
+
+        if (ofproto->backer != backer) {
+            continue;
+        }
+
+        struct json *json_ofproto = json_object_create();
+
+        /* Add ofproto name to JSON array. */
+        json_object_put_string(json_ofproto, "name", ofproto->up.name);
+
+        /* Add ofproto ports to JSON array. */
+        struct json *json_ofproto_ports = json_array_create_empty();
+        const struct shash_node **ports;
+        ports = shash_sort(&ofproto->up.port_by_name);
+        for (size_t j = 0; j < shash_count(&ofproto->up.port_by_name); j++) {
+            const struct shash_node *port = ports[j];
+            struct ofport *ofport = port->data;
+
+            struct json * json_ofproto_port = json_object_create();
+            /* Add ofproto port netdev name to JSON sub array. */
+            json_object_put_string(json_ofproto_port, "netdev_name",
+                                   netdev_get_name(ofport->netdev));
+            /* Add ofproto port ofp port to JSON sub array. */
+            json_object_put_format(json_ofproto_port, "ofp_port", "%u",
+                                   ofport->ofp_port);
+
+            /* Add ofproto port odp port to JSON sub array. */
+            odp_port_t odp_port = ofp_port_to_odp_port(ofproto,
+                                                       ofport->ofp_port);
+            if (odp_port != ODPP_NONE) {
+                json_object_put_format(json_ofproto_port, "odp_port",
+                                       "%"PRIu32, odp_port);
+            } else {
+                json_object_put_string(json_ofproto_port, "odp_port", "none");
+            }
+
+            /* Add ofproto port netdev type to JSON sub array. */
+            json_object_put_string(json_ofproto_port, "netdev_type",
+                                   netdev_get_type(ofport->netdev));
+
+            /* Add ofproto port config to JSON sub array. */
+            struct json *json_port_config = json_object_create();
+            struct smap config;
+            smap_init(&config);
+            if (!netdev_get_config(ofport->netdev, &config)) {
+                struct smap_node *node;
+
+                SMAP_FOR_EACH (node, &config) {
+                    json_object_put_string(json_port_config, node->key,
+                                           node->value);
+                }
+            }
+            smap_destroy(&config);
+            json_object_put(json_ofproto_port, "netdev_config",
+                            json_port_config);
+
+            json_array_add(json_ofproto_ports, json_ofproto_port);
+        } /* End of ofproto port(s). */
+
+        free(ports);
+        json_object_put(json_ofproto, "ports", json_ofproto_ports);
+
+        json_array_add(json_ofprotos, json_ofproto);
+    } /* End of ofproto(s). */
+    shash_destroy(&ofproto_shash);
+    free(ofprotos);
+
+    json_object_put(json_backer, "ofprotos", json_ofprotos);
+    return json_backer;
+}
+
 static void
-dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
+dpif_show_backer_text(const struct dpif_backer *backer, struct ds *ds)
 {
     const struct shash_node **ofprotos;
     struct dpif_dp_stats dp_stats;
@@ -6587,18 +6683,24 @@  static void
 ofproto_unixctl_dpif_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
                           const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
-    struct ds ds = DS_EMPTY_INITIALIZER;
-    const struct shash_node **backers;
-    int i;
+    if (unixctl_command_get_output_format(conn) == OVS_OUTPUT_FMT_JSON) {
+        struct json *backers = json_array_create_empty();
+        const struct shash_node *backer;
+        SHASH_FOR_EACH (backer, &all_dpif_backers) {
+            json_array_add(backers, dpif_show_backer_json(backer->data));
+        }
+        unixctl_command_reply_json(conn, backers);
+    } else {
+        struct ds ds = DS_EMPTY_INITIALIZER;
+        const struct shash_node **backers = shash_sort(&all_dpif_backers);
+        for (int i = 0; i < shash_count(&all_dpif_backers); i++) {
+            dpif_show_backer_text(backers[i]->data, &ds);
+        }
+        free(backers);
 
-    backers = shash_sort(&all_dpif_backers);
-    for (i = 0; i < shash_count(&all_dpif_backers); i++) {
-        dpif_show_backer(backers[i]->data, &ds);
+        unixctl_command_reply(conn, ds_cstr(&ds));
+        ds_destroy(&ds);
     }
-    free(backers);
-
-    unixctl_command_reply(conn, ds_cstr(&ds));
-    ds_destroy(&ds);
 }
 
 static void
diff --git a/tests/pmd.at b/tests/pmd.at
index 35a44b4df..cff80da15 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -112,6 +112,34 @@  dummy@ovs-dummy: hit:0 missed:0
     p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
 ])
 
+AT_CHECK([ovs-appctl --format json --pretty dpif/show], [0], [dnl
+[[
+  {
+    "name": "dummy@ovs-dummy",
+    "ofprotos": [
+      {
+        "name": "br0",
+        "ports": [
+          {
+            "netdev_config": {
+              },
+            "netdev_name": "br0",
+            "netdev_type": "dummy-internal",
+            "odp_port": "100",
+            "ofp_port": "65534"},
+          {
+            "netdev_config": {
+              "n_rxq": "1",
+              "n_txq": "1",
+              "numa_id": "0"},
+            "netdev_name": "p0",
+            "netdev_type": "dummy-pmd",
+            "odp_port": "1",
+            "ofp_port": "1"}]}],
+    "stats": {
+      "n_hit": "0",
+      "n_missed": "0"}}]]])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP