On Wed, Jun 18, 2025 at 6:37 PM Mairtin O'Loingsigh via dev < [email protected]> wrote:
> Add support for using ovn-nbctl to list ports in a port group > > pg-get-ports: Get logical switch ports of a port group > > Reported-at: https://issues.redhat.com/browse/FDP-1463 > Signed-off-by: Mairtin O'Loingsigh <[email protected]> > --- > Hi Mairtin, I have a few minor comments that could be addressed during merge. v2: > * address code issues > * add NEWS entry > * update ovn-nbctl man page > > NEWS | 1 + > tests/ovn-nbctl.at | 13 +++++++---- > utilities/ovn-nbctl.8.xml | 6 +++++ > utilities/ovn-nbctl.c | 48 +++++++++++++++++++++++++++++++++++---- > 4 files changed, 60 insertions(+), 8 deletions(-) > > diff --git a/NEWS b/NEWS > index 4d84a456a..755e349a2 100644 > --- a/NEWS > +++ b/NEWS > @@ -26,6 +26,7 @@ Post v25.03.0 > - Add a new logical switch option - enable-stateless-acl-lb with > default > value of false. This option should be set to true for logical > switches > with stateless ACL to work with load balancer. > + - Added new nbctl command to get the ports on a port group > nit: We usually say what the command so I would rewrite it as follow: "Added new ovn-nbctl command 'pg-get-ports' to get the ports assigned to the port group. > > OVN v25.03.0 - 07 Mar 2025 > -------------------------- > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 447a0666f..6e1f3fd45 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -2791,22 +2791,27 @@ SW1P1=$(ovn-nbctl --bare --columns=_uuid list > logical_switch_port sw1-p1) > AT_CHECK([ovn-nbctl lsp-add sw1 sw1-p2], [0], [ignore]) > SW1P2=$(ovn-nbctl --bare --columns=_uuid list logical_switch_port sw1-p2) > > -AT_CHECK([ovn-nbctl pg-add pg1 sw1-p1], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-add pg1 sw1-p1 sw1-p2], [0], [ignore]) > AT_CHECK([ovn-nbctl --bare --columns=name list port_group pg1], [0],[dnl > pg1 > ]) > -AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group pg1], > [0], [dnl > -$SW1P1 > +AT_CHECK([ovn-nbctl pg-get-ports pg1],[0], [dnl > +sw1-p1 sw1-p2 > ]) > > AT_CHECK([ovn-nbctl pg-set-ports pg1 sw1-p2], [0], [ignore]) > AT_CHECK_UNQUOTED([ovn-nbctl --bare --columns=ports list port_group pg1], > [0], [dnl > $SW1P2 > ]) > +AT_CHECK([ovn-nbctl pg-get-ports pg1],[0], [dnl > +sw1-p2 > +]) > > AT_CHECK([ovn-nbctl pg-del pg1], [0], [ignore]) > AT_CHECK([ovn-nbctl list port_group], [0], []) > -]) > +AT_CHECK([ovn-nbctl pg-get-ports pg1], [1], [], > + [ovn-nbctl: pg1: port group name not found > +])]) > dnl --------------------------------------------------------------------- > > OVN_NBCTL_TEST([ovn_nbctl_fwd_groups], [fwd groups], [ > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > index 61ef70648..9f9547bdb 100644 > --- a/utilities/ovn-nbctl.8.xml > +++ b/utilities/ovn-nbctl.8.xml > @@ -1546,6 +1546,12 @@ > is an error if <code>group</code> does not exist. > </dd> > > + <dt><code>pg-get-ports</code> <var>group</var></dt> > + <dd> > + Gets <code>ports</code> on the port group named > <code>group</code>. It > + is an error if <code>group</code> does not exist. > + </dd> > + > <dt><code>pg-del</code> <var>group</var></dt> > <dd> > Deletes port group <code>group</code>. It is an error if > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 33e789523..2e9ff117b 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -483,6 +483,7 @@ Port group commands:\n\ > pg-add PG [PORTS] Create port group PG with optional PORTS\n\ > pg-set-ports PG PORTS Set PORTS on port group PG\n\ > pg-del PG Delete port group PG\n\ > + pg-get-ports PG Get PORTS on port group PG\n\ > HA chassis group commands:\n\ > ha-chassis-group-add GRP Create an HA chassis group GRP\n\ > ha-chassis-group-del GRP Delete the HA chassis group GRP\n\ > @@ -7445,10 +7446,9 @@ cmd_pg_add(struct ctl_context *ctx) > } > > static void > -cmd_pre_pg_set_ports(struct ctl_context *ctx) > +cmd_pre_pg_set_get_ports(struct ctl_context *ctx) > { > ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name); > - > nit: Unrelated. > ovsdb_idl_add_column(ctx->idl, &nbrec_port_group_col_name); > ovsdb_idl_add_column(ctx->idl, &nbrec_port_group_col_ports); > } > @@ -7489,6 +7489,44 @@ cmd_pg_del(struct ctl_context *ctx) > nbrec_port_group_delete(pg); > } > > +static int > +port_name_cmp(const void *s1_, const void *s2_) > +{ > + const char *s1 = *(char **) s1_; > + const char *s2 = *(char **) s2_; > + return strcmp(s1, s2); > +} > + > +static void > +cmd_pg_get_ports(struct ctl_context *ctx) > +{ > + const struct nbrec_port_group *pg; > + > + char *error = pg_by_name_or_uuid(ctx, ctx->argv[1], true, &pg); > + if (error) { > + ctx->error = error; > + return; > + } > + > + if (pg->n_ports == 0) { > nit: We don't compare to 0, we are using "!pg->n_ports". > + return; > + } > + > + char **port_names = xmalloc(sizeof *port_names * pg->n_ports); > + for (size_t i = 0; i < pg->n_ports; i++) { > + port_names[i] = pg->ports[i]->name; > + } > + > + qsort(port_names, pg->n_ports, sizeof *port_names, port_name_cmp); > nit: Missing empty line. > + ds_put_format(&ctx->output, "%s", port_names[0]); > + for (size_t i = 1; i < pg->n_ports; i++) { > + ds_put_format(&ctx->output, " %s", port_names[i]); > + } > + ds_put_format(&ctx->output, "\n"); > + > + free(port_names); > +} > + > static const struct nbrec_ha_chassis_group* > ha_chassis_group_by_name_or_uuid(struct ctl_context *ctx, const char *id, > bool must_exist) > @@ -8417,9 +8455,11 @@ static const struct ctl_command_syntax > nbctl_commands[] = { > > /* Port Group Commands */ > {"pg-add", 1, INT_MAX, "", cmd_pre_pg_add, cmd_pg_add, NULL, "", RW }, > - {"pg-set-ports", 2, INT_MAX, "", cmd_pre_pg_set_ports, > cmd_pg_set_ports, > - NULL, "", RW }, > + {"pg-set-ports", 2, INT_MAX, "", cmd_pre_pg_set_get_ports, > + cmd_pg_set_ports, NULL, "", RW }, > {"pg-del", 1, 1, "", cmd_pre_pg_del, cmd_pg_del, NULL, "", RW }, > + {"pg-get-ports", 1, 1, "PORT_GROUP", cmd_pre_pg_set_get_ports, > + cmd_pg_get_ports, NULL, "", RO }, > > /* HA chassis group commands. */ > {"ha-chassis-group-add", 1, 1, "[CHASSIS GROUP]", > -- > 2.49.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > With that addressed: Acked-by: Ales Musil <[email protected]> Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
