On 8/25/25 5:52 AM, Sragdhara Datta Chaudhuri wrote:
> Network function:
> network-function-add <nf-name> <inport> <outport> - Add a new NF
> network-function-del <nf-name-or-uuid> - Delete a NF
> network-function-list - List all NFs
>
> Network function group:
> network-function-group-add <nfg-name> <id> <mode> [<nf1> <nf2> ...]
> - Add a new NFG (mode must be "inline", id a value between 1 and 255)
> network-function-group-del <nfg-name-or-uuid> - Delete a NFG
> network-function-group-list [<nfg-name-or-uuid>] - List all NFGs
> network-function-group-add-network-function <nfg-name-or-uuid>
> <nf-name-or-uuid>
> - Add a NF to a NFG
> network-function-group-del-network-function <nfg-name-or-uuid>
> <nf-name-or-uuid>
> - Remove a NF from a NFG
>
> ACL (new optional parameter added for NFG):
> acl-add <ls>|<pg> <direction> <priority> <match> <action> [<nfg-name-or-uuid>]
>
> Signed-off-by: Sragdhara Datta Chaudhuri <sragdha.chau...@nutanix.com>
> Acked-by: Naveen Yerramneni <naveen.yerramn...@nutanix.com>
> Acked-by:Numan Siddique <num...@ovn.org>
> ---
Hi Sragdhara,
Thanks for this new revision!
> utilities/ovn-nbctl.8.xml | 104 +++++++-
> utilities/ovn-nbctl.c | 518 +++++++++++++++++++++++++++++++++++++-
> 2 files changed, 618 insertions(+), 4 deletions(-)
>
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index 60936a2b5..fc069c048 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -399,7 +399,7 @@
> must be either <code>switch</code> or <code>port-group</code>.
> </p>
> <dl>
> - <dt>[<code>--type=</code>{<code>switch</code> |
> <code>port-group</code>}] [<code>--log</code>]
> [<code>--meter=</code><var>meter</var>]
> [<code>--severity=</code><var>severity</var>]
> [<code>--name=</code><var>name</var>] [<code>--label=</code><var>label</var>]
> [<code>--sample-new=</code><var>sample</var>]
> [<code>--sample-est=</code><var>sample</var>] [<code>--may-exist</code>]
> [<code>--apply-after-lb</code>] [<code>--tier</code>] <code>acl-add</code>
> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var>
> <var>verdict</var></dt>
> + <dt>[<code>--type=</code>{<code>switch</code> |
> <code>port-group</code>}] [<code>--log</code>]
> [<code>--meter=</code><var>meter</var>]
> [<code>--severity=</code><var>severity</var>]
> [<code>--name=</code><var>name</var>] [<code>--label=</code><var>label</var>]
> [<code>--sample-new=</code><var>sample</var>]
> [<code>--sample-est=</code><var>sample</var>] [<code>--may-exist</code>]
> [<code>--apply-after-lb</code>] [<code>--tier</code>] <code>acl-add</code>
> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var>
> <var>verdict</var> [<var>network-function-group</var>]</dt>
> <dd>
> <p>
> Adds the specified ACL to <var>entity</var>. <var>direction</var>
> @@ -472,6 +472,108 @@
> </dd>
> </dl>
>
> + <h2>Network Function Commands</h2>
> +
> + <dl>
> + <dt>[<code>--may-exist</code>] <code>network-function-add</code>
> <var>nf</var> <var>inport</var> <var>outport</var></dt>
> + <dd>
> + <p>
> + Creates a new network function named <var>nf</var> with logical
> + switch ports <var>inport</var> and <var>outport</var>. Both the
> + ports must be on the same logical switch and must be already
> + created. When used in an ACL action, traffic matching the ACL
> + are redirected to the <var>inport</var> if it is from-lport ACL
> + and to the <var>outport</var> if it is to-lport ACL. The response
> + packets are sent through the same ports in reverse order.
> + </p>
> +
> + <p>
> + Without any options, this command regards it as an error if
> + <var>nf</var> is a duplicate name. With <code>--may-exist</code>,
> + adding a duplicate name succeeds but does not create a new network
> + function.
Looking at the code in ovn-nbctl we do handle this as a "no-op". Which
makes me wonder about how useful it would be. With other CLI commands,
we treat this case (--may-exist + existing entry) as an update, e.g.,
"ovn-nbctl --may-exist meter-add". Should we be doing the same here?
> + </p>
> + </dd>
> +
> + <dt>[<code>--if-exists</code>] <code>network-function-del</code>
> <var>nf</var></dt>
> + <dd>
> + <p>
> + Deletes <var>nf</var> specified as name or uuid. It is an error if
> + <var>nf</var> does not exist, unless <code>--if-exists</code> is
> + specified.
> + </p>
> + </dd>
> +
> + <dt><code>network-function-list</code></dt>
> + <dd>
> + <p>
> + Lists all network functions.
> + </p>
> + </dd>
> +
> + <dt>[<code>--may-exist</code>] <code>network-function-group-add</code>
> <var>nfg</var> <var>id</var> <var>mode</var> [<var>nf</var>]...</dt>
> + <dd>
> + <p>
> + Creates a new network function group named <var>nfg</var> with
> + optionally one of more <code>nf</code> added to the group. The nfs
> + must be already created. Traffic redirection would be done towards
> + one of the active network functions, based on health monitoring.
> + If all are down, any one would be chosen for redirection.
Not necessarily a problem but would it be better to just not redirect
the traffic if all functions in a group are down? Also, wouldn't this
note fit better in ovn-nb.xml, in the "Network_Function_Group" part of
the man page?
> + </p>
> +
> + <p>
> + Each network function group must have a unique <var>id</var>
> between
> + <code>1</code> and <code>255</code>. The <var>mode</var> must be
> + <code>inline</code> which is currently the only supported mode.
> + </p>
> +
> + <p>
> + Without any options, this command regards it as an error if
> + <var>nfg</var> is a duplicate name. With <code>--may-exist</code>,
> + adding a duplicate name succeeds but does not create a new network
> + function group.
> + </p>
> + </dd>
> +
> + <dt>[<code>--if-exists</code>] <code>network-function-group-del</code>
> <var>nfg</var></dt>
> + <dd>
> + <p>
> + Deletes <var>nfg</var> specified as name or uuid. It is an error if
> + <var>nfg</var> does not exist, unless <code>--if-exists</code> is
> + specified.
> + </p>
> + </dd>
> +
> + <dt><code>network-function-group-list</code></dt>
> + <dd>
> + <p>
> + Lists all network function groups.
> + </p>
> + </dd>
> +
> + <dt>[<code>--may-exist</code>]
> <code>network-function-group-add-network-function</code> <var>nfg</var>
> <var>nf</var></dt>
> + <dd>
> + <p>
> + Add a network function named <var>nf</var> to a network function
> + group named <var>nfg</var>. It is an error if <var>nfg</var> or
> + <var>nf</var> does not exist. Unless <code>--may-exist</code> is
> + specified, it is an error if the <var>nf</var> being added is
> + already a part of the <var>nfg</var>.
> + </p>
> + </dd>
> +
> + <dt><code>network-function-group-del-network-function</code>
> <var>nfg</var> <var>nf</var></dt>
> + <dd>
> + <p>
> + Delete a network function named <var>nf</var> from a network
> function
> + group named <var>nfg</var>. It is an error if <var>nfg</var> or
> + <var>nf</var> does not exist. It is an error if <var>nf</var> is
> not
> + a part of the <var>nfg</var>, unless <code>--if-exists</code> is
> + specified.
> + </p>
> + </dd>
> + </dl>
> +
> <h2>Logical Switch QoS Rule Commands</h2>
> <dl>
> <dt>[<code>--may-exist</code>] <code>qos-add</code>
> <var>switch</var> <var>direction</var> <var>priority</var> <var>match</var>
> [<code>mark=</code><var>mark</var>] [<code>dscp=</code><var>dscp</var>]
> [<code>rate=</code><var>rate</var> [<code>burst=</code><var>burst</var>]]</dt>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 58517f966..d0a5595e4 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -283,7 +283,7 @@ Logical switch commands:\n\
> \n\
> ACL commands:\n\
> [--type={switch | port-group}] [--log] [--severity=SEVERITY] [--name=NAME]
> [--may-exist]\n\
> - acl-add {SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION\n\
> + acl-add {SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION
> [NETWORK-FUNCTION-GROUP]\n\
> add an ACL to SWITCH/PORTGROUP\n\
> [--type={switch | port-group}]\n\
> acl-del {SWITCH | PORTGROUP} [DIRECTION [PRIORITY MATCH]]\n\
> @@ -373,6 +373,26 @@ Forwarding group commands:\n\
> fwd-group-del GROUP delete a forwarding group\n\
> fwd-group-list [SWITCH] print forwarding groups\n\
> \n\
> +Network function group commands:\n\
> + network-function-group-add NETWORK-FUNCTION-GROUP ID MODE
> [NETWORK-FUNCTION]...\n\
> + create a network-function-group\n\
> + network-function-group-del NETWORK-FUNCTION-GROUP\n\
> + delete a network-function-group\n\
> + network-function-group-list print all network-function-groups\n\
> + network-function-group-add-network-function NETWORK-FUNCTION-GROUP
> NETWORK-FUNCTION\n\
> + add a network-function to a\n\
> + network-function-group\n\
> + network-function-group-del-network-function NETWORK-FUNCTION-GROUP
> NETWORK-FUNCTION\n\
> + delete a network-function from a\n\
> + network-function-group\n\
> +\n\
> +Network function commands:\n\
> + network-function-add NETWORK-FUNCTION PORT-IN PORT-OUT\n\
> + create a network-function\n\
> + network-function-del NETWORK-FUNCTION delete a network-function\n\
> + network-function-list print all network-functions\n\
> +\n\n",program_name, program_name);
> + printf("\
> Logical router commands:\n\
> lr-add [ROUTER] create a logical router named ROUTER\n\
> lr-del ROUTER delete ROUTER and all its ports\n\
> @@ -429,7 +449,7 @@ Policy commands:\n\
> lr-policy-del ROUTER [{PRIORITY | UUID} [MATCH]]\n\
> remove policies from ROUTER\n\
> lr-policy-list ROUTER print policies for ROUTER\n\
> -\n\n",program_name, program_name);
> ++\n\n");
> printf("\
> NAT commands:\n\
> [--stateless]\n\
> @@ -2135,6 +2155,447 @@ nbctl_lsp_get_ls(struct ctl_context *ctx)
> }
> }
>
> +
Nit: no need for this extra line.
> +static char * OVS_WARN_UNUSED_RESULT
> +nf_group_by_name_or_uuid(
> + struct ctl_context *ctx, const char *id,
> + bool must_exist,
> + const struct nbrec_network_function_group **nf_group_p)
> +{
> + struct uuid nf_group_uuid;
> + bool is_uuid = uuid_from_string(&nf_group_uuid, id);
> +
> + *nf_group_p = NULL;
> + if (is_uuid) {
> + *nf_group_p = nbrec_network_function_group_get_for_uuid(ctx->idl,
> + &nf_group_uuid);
Nit: indentation. A way to make it fit (not very pretty either but I
couldn't find anything better) might be:
if (is_uuid) {
*nf_group_p =
nbrec_network_function_group_get_for_uuid(ctx->idl,
&nf_group_uuid);
}
> + }
> +
> + if (!*nf_group_p) {
> + const struct nbrec_network_function_group *iter;
> + NBREC_NETWORK_FUNCTION_GROUP_FOR_EACH (iter, ctx->idl) {
> + if (strcmp(iter->name, id)) {
> + continue;
> + }
> + if (*nf_group_p) {
> + return xasprintf("Multiple network function group named
> '%s'. "
> + "Use a UUID.", id);
With the new DB schema name index restriction this can never be hit
anymore, right? So we could just break at the end of the loop, after
setting *nf_group_p = iter.
> + }
> + *nf_group_p = iter;
> + }
> + }
> + if (!*nf_group_p && must_exist) {
> + return xasprintf("%s: network function group %s not found", id,
> + is_uuid ? "UUID" : "name");
> + }
> + return NULL;
> +}
> +
> +static char * OVS_WARN_UNUSED_RESULT
> +nf_by_name_or_uuid(
> + struct ctl_context *ctx, const char *id,
> + bool must_exist, const struct nbrec_network_function **nf_p)
> +{
> + struct uuid nf_uuid;
> + bool is_uuid = uuid_from_string(&nf_uuid, id);
> +
> + *nf_p = NULL;
> + if (is_uuid) {
> + *nf_p = nbrec_network_function_get_for_uuid(ctx->idl, &nf_uuid);
> + }
> +
> + if (!*nf_p) {
> + const struct nbrec_network_function *iter;
> + NBREC_NETWORK_FUNCTION_FOR_EACH (iter, ctx->idl) {
> + if (strcmp(iter->name, id)) {
> + continue;
> + }
> + if (*nf_p) {
> + return xasprintf("Multiple network functions named '%s'. "
> + "Use a UUID.", id);
With the new DB schema name index restriction this can never be hit
anymore, right? We could just break after *nf_p = iter;
> + }
> + *nf_p = iter;
> + }
> + }
> + if (!*nf_p && must_exist) {
> + return xasprintf("%s: network function %s not found", id,
> + is_uuid ? "UUID" : "name");
> + }
> + return NULL;
> +}
> +
> +/*
> + * Network Function Groups CLI Functions
> + */
> +static void
> +nbctl_pre_nf_group_add(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_id);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_mode);
> + ovsdb_idl_add_column(
> + ctx->idl, &nbrec_network_function_group_col_network_function);
> +}
> +
> +static char *
> +set_network_function_in_network_function_group(
> + struct ctl_context *ctx,
> + const struct nbrec_network_function_group *nf_group,
> + char **new_network_function, size_t num_new_network_function)
> +{
> + struct nbrec_network_function **nfs =
> + xmalloc(sizeof *nfs * num_new_network_function);
Nit: indentation (should be 2 more spaces to the right).
> +
> + char *error = NULL;
> + for (size_t i = 0; i < num_new_network_function; i++) {
> + const struct nbrec_network_function *nf;
> + error = nf_by_name_or_uuid(ctx, new_network_function[i], true, &nf);
Nit: we could define 'char *error = nf_by_name_or_uuid()' here and just
return NULL at the end of the function.
> + if (error) {
> + free(nfs);
> + return error;
> + }
> + nfs[i] = CONST_CAST(struct nbrec_network_function *, nf);
> + }
I think we need this here:
nbrec_network_function_group_verify_network_function(nf_group);
> + nbrec_network_function_group_set_network_function(
> + nf_group, nfs, num_new_network_function);
> + free(nfs);
> + return error;
> +}
> +
> +static void
> +nbctl_nf_group_add(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function_group *nf_group;
> + const char *nfg_name = ctx->argv[1];
> + bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> + NBREC_NETWORK_FUNCTION_GROUP_FOR_EACH (nf_group, ctx->idl) {
> + if (!strcmp(nf_group->name, nfg_name)) {
> + if (may_exist) {
> + return;
> + }
> + ctl_error(ctx, "%s: a network-function-group with this name "
> + "already exists", nfg_name);
> + return;
> + }
> + }
> +
> + nf_group = nbrec_network_function_group_insert(ctx->txn);
> + nbrec_network_function_group_set_name(nf_group, nfg_name);
> +
> + int64_t nfg_id = 0;
> + if (!ovs_scan(ctx->argv[2], "%"SCNd64, &nfg_id)
> + || nfg_id < 1 || nfg_id > 255) {
> + ctl_error(ctx, "network-function-group id must be between 1 and
> 255");
> + return;
> + }
> + nbrec_network_function_group_set_id(nf_group, nfg_id);
> +
> + const char *nfg_mode = ctx->argv[3];
> + if (strcmp(nfg_mode,"inline")) {
Nit: missing space before "inline"
> + ctl_error(ctx, "Unsupported mode provided for "
> + "network-function-group:%s, supported values: inline",
> + nfg_name);
> + return;
> + }
> + nbrec_network_function_group_set_mode(nf_group, nfg_mode);
> +
> + if (ctx->argc > 4) {
> + ctx->error = set_network_function_in_network_function_group(
> + ctx, nf_group, ctx->argv + 4, ctx->argc - 4);
> + }
> +}
> +
> +static void
> +nbctl_pre_nf_group_del(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> +}
> +
> +static void
> +nbctl_nf_group_del(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function_group *nf_group;
> + bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +
> + char *error = nf_group_by_name_or_uuid(
> + ctx, ctx->argv[1], must_exist, &nf_group);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + if (!nf_group) {
> + return;
> + }
> +
> + nbrec_network_function_group_delete(nf_group);
> +}
> +
> +static void
> +nbctl_pre_nf_group_list(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_network_function_group_col_network_function);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> +}
> +
> +static void
> +nbctl_nf_group_list(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function_group *nf_group;
> + struct smap nf_groups = SMAP_INITIALIZER(&nf_groups);
> +
> + NBREC_NETWORK_FUNCTION_GROUP_FOR_EACH (nf_group, ctx->idl) {
> + smap_add_format(&nf_groups, nf_group->name,
> + UUID_FMT " (%s)",
> + UUID_ARGS(&nf_group->header_.uuid),
> + nf_group->name);
> + }
> + const struct smap_node **nodes = smap_sort(&nf_groups);
> + for (size_t i = 0; i < smap_count(&nf_groups); i++) {
> + const struct smap_node *node = nodes[i];
> + ds_put_format(&ctx->output, "%s\n", node->value);
> + }
> + smap_destroy(&nf_groups);
> + free(nodes);
> +}
> +
> +static void
> +nbctl_pre_nf_group_add_network_function(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_network_function_group_col_network_function);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> +}
> +
> +static void
> +nbctl_nf_group_add_network_function(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function_group *nf_group;
> + const struct nbrec_network_function *nf;
> + bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> + char * error = nf_group_by_name_or_uuid(ctx, ctx->argv[1], true,
Nit: should be "char *error".
> + &nf_group);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + if (!nf_group) {
> + return;
> + }
> +
> + /* Check that network-function exists */
> + error = nf_by_name_or_uuid(ctx, ctx->argv[2], true, &nf);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + /* Do not add network_function more than once in a given
> + * network-function-group */
> + for (size_t i = 0; i < nf_group->n_network_function; i++) {
> + if (nf_group->network_function[i] == nf) {
> + if (!may_exist) {
> + ctl_error(ctx, "network-function %s is already added to "
> + "network-function-group %s",
> + ctx->argv[2], ctx->argv[1]);
> + return;
> + }
> + return;
> + }
> + }
> +
> + /* Insert the logical network-function into the logical
> + * network-function-group. */
> + nbrec_network_function_group_verify_network_function(nf_group);
This whole block below
> + struct nbrec_network_function **new_network_function =
> + xmalloc(sizeof *new_network_function
> + * (nf_group->n_network_function + 1));
> + memcpy(new_network_function, nf_group->network_function,
> + sizeof *new_network_function * nf_group->n_network_function);
> + new_network_function[nf_group->n_network_function] =
> + CONST_CAST(struct nbrec_network_function *, nf);
> + nbrec_network_function_group_set_network_function(nf_group,
> + new_network_function, nf_group->n_network_function + 1);
> + free(new_network_function);
can be replaced with:
nbrec_network_function_group_update_network_function_addvalue(
nf_group, nf
);
> +}
> +
> +static void
> +remove_nf_from_network_function_group(
> + const struct nbrec_network_function_group *nf_group, size_t idx)
> +{
> + nf_group->network_function[idx] =
> + nf_group->network_function[nf_group->n_network_function - 1];
> + nbrec_network_function_group_verify_network_function(nf_group);
> + nbrec_network_function_group_set_network_function(
> + nf_group, nf_group->network_function,
> + nf_group->n_network_function - 1);
> +}
We don't need this function, please see below why.
> +
> +static void
> +nbctl_pre_nf_group_del_network_function(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_network_function_group_col_network_function);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> +}
> +
> +static void
> +nbctl_nf_group_del_network_function(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function *nf;
> + const struct nbrec_network_function_group *nf_group;
> + bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +
> + char * error = nf_group_by_name_or_uuid(ctx, ctx->argv[1], must_exist,
> + &nf_group);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + if (!nf_group) {
> + return;
> + }
> +
> + error = nf_by_name_or_uuid(ctx, ctx->argv[2], must_exist, &nf);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + /* Find the network-function_group that contains 'network-function',
> + * then delete it. */
> + for (size_t i = 0; i < nf_group->n_network_function; i++) {
> + if (nf_group->network_function[i] == nf) {
> + remove_nf_from_network_function_group(nf_group, i);
This can be replaced with:
nbrec_network_function_group_verify_network_function(nf_group);
nbrec_network_function_group_update_network_function_delvalue(nf_group, nf);
> + return;
> + }
> + }
> + if (must_exist) {
> + ctl_error(ctx, "network-function %s is not part of any logical
> switch",
> + ctx->argv[1]);
> + return;
> + }
> +}
> +/* End of network-function-group operations */
> +
> +/*
> + * network-function operations
> + */
> +static void
> +nbctl_pre_nf_add(struct ctl_context *ctx)
> +{
> + nbctl_pre_context(ctx);
> +
> + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_inport);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_outport);
> +}
> +
> +static void
> +nbctl_nf_add(struct ctl_context *ctx)
> +{
> + const struct nbrec_logical_switch_port *lsp_in, *lsp_out;
> + const struct nbrec_network_function *nf;
> +
> + bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> + char * error = lsp_by_name_or_uuid(ctx, ctx->argv[2], true, &lsp_in);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + error = lsp_by_name_or_uuid(ctx, ctx->argv[3], true, &lsp_out);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + const char *nf_name = ctx->argv[1];
> +
> + NBREC_NETWORK_FUNCTION_FOR_EACH (nf, ctx->idl) {
> + if (!strcmp(nf->name, nf_name)) {
> + if (may_exist) {
> + return;
> + }
> + ctl_error(ctx, "%s: same name network-function already exists",
> + nf_name);
> + return;
Please see the comment about updating on "--may-exist" and existing
network function. We might want to change this to break.
> + }
> + }
> +
> + /* create the logical network-function. */
> + nf = nbrec_network_function_insert(ctx->txn);
> + nbrec_network_function_set_inport(nf, lsp_in);
> + nbrec_network_function_set_outport(nf, lsp_out);
> + nbrec_network_function_set_name(nf, nf_name);
> +}
> +
> +static void
> +nbctl_pre_nf_del(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> +}
> +
> +static void
> +nbctl_nf_del(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function *nf;
> + bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +
> + char *error = nf_by_name_or_uuid(ctx, ctx->argv[1], must_exist, &nf);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + if (!nf) {
> + return;
> + }
> + nbrec_network_function_delete(nf);
> +}
> +
> +static void
> +nbctl_pre_nf_list(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_inport);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_outport);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name);
> +}
> +
> +static void
> +nbctl_nf_list(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function *nf;
> + struct smap nfs = SMAP_INITIALIZER(&nfs);
> +
> + NBREC_NETWORK_FUNCTION_FOR_EACH (nf, ctx->idl) {
> + const struct nbrec_logical_switch_port *linport = nf->inport;
> + const struct nbrec_logical_switch_port *loutport = nf->outport;
> + const char *linport_name = linport ? linport->name : "<not_set>";
> + const char *loutport_name = loutport ? loutport->name : "<not_set>";
> + smap_add_format(&nfs, nf->name,
> + UUID_FMT " (%s) in:%s out:%s",
> + UUID_ARGS(&nf->header_.uuid),
> + nf->name, linport_name, loutport_name);
> + }
> + const struct smap_node **nodes = smap_sort(&nfs);
> + for (size_t i = 0; i < smap_count(&nfs); i++) {
> + const struct smap_node *node = nodes[i];
> + ds_put_format(&ctx->output, "%s\n", node->value);
> + }
> + smap_destroy(&nfs);
> + free(nodes);
> +}
> +
> +/* End of network-function operations */
> +
> +
> enum {
> DIR_FROM_LPORT,
> DIR_TO_LPORT
> @@ -2232,6 +2693,10 @@ nbctl_acl_print(struct ctl_context *ctx, const struct
> nbrec_acl **acls,
> ds_put_format(&ctx->output, "%10s %5"PRId64" (%s) %s",
> acl->direction, acl->priority, acl->match,
> acl->action);
> + if (acl->network_function_group) {
> + ds_put_format(&ctx->output, " network-function-group=%s",
> + acl->network_function_group->name);
> + }
> if (acl->log) {
> ds_put_cstr(&ctx->output, " log(");
> if (acl->name) {
> @@ -2405,6 +2870,8 @@ nbctl_pre_acl(struct ctl_context *ctx)
>
> ovsdb_idl_add_table(ctx->idl, &nbrec_table_sample_collector);
> ovsdb_idl_add_table(ctx->idl, &nbrec_table_sample);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_network_function_group);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> }
>
> static void
> @@ -2461,10 +2928,21 @@ nbctl_acl_add(struct ctl_context *ctx)
>
> /* Create the acl. */
> struct nbrec_acl *acl = nbrec_acl_insert(ctx->txn);
> + const struct nbrec_network_function_group *network_function_group = NULL;
> nbrec_acl_set_priority(acl, priority);
> nbrec_acl_set_direction(acl, direction);
> nbrec_acl_set_match(acl, ctx->argv[4]);
> nbrec_acl_set_action(acl, action);
> + if (ctx->argc > 6) {
> + error = nf_group_by_name_or_uuid(ctx, ctx->argv[6], true,
> + &network_function_group);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + nbrec_acl_set_network_function_group(acl, network_function_group);
> + }
>
> /* Logging options. */
> bool log = shash_find(&ctx->options, "--log") != NULL;
> @@ -8266,7 +8744,8 @@ static const struct ctl_command_syntax nbctl_commands[]
> = {
> { "ls-list", 0, 0, "", nbctl_pre_ls_list, nbctl_ls_list, NULL, "", RO },
>
> /* acl commands. */
> - { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH
> ACTION",
> + { "acl-add", 5, 7,
> + "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION
> [NETWORK-FUNCTION-GROUP]",
> nbctl_pre_acl, nbctl_acl_add, NULL,
> "--log,--may-exist,--type=,--name=,--severity=,--meter=,--label=,"
> "--apply-after-lb,--tier=,--sample-new=,--sample-est=", RW },
> @@ -8358,6 +8837,39 @@ static const struct ctl_command_syntax
> nbctl_commands[] = {
> { "lsp-detach-mirror", 2, 2, "PORT MIRROR", nbctl_pre_lsp_mirror,
> nbctl_lsp_detach_mirror, NULL, "", RW },
>
> + /* network-function-group commands. */
> + { "network-function-group-add", 3, INT_MAX,
> + "NETWORK-FUNCTION-GROUP ID MODE [NETWORK-FUNCTION]",
> + nbctl_pre_nf_group_add,
> + nbctl_nf_group_add, NULL, "--may-exist", RW },
> + { "network-function-group-del", 1, 1, "NETWORK-FUNCTION-GROUP",
> + nbctl_pre_nf_group_del,
> + nbctl_nf_group_del,
> + NULL, "--if-exists", RW },
> + { "network-function-group-list", 0, 1, "[NETWORK-FUNCTION-GROUP]",
> + nbctl_pre_nf_group_list,
> + nbctl_nf_group_list,
> + NULL, "", RO },
> + { "network-function-group-add-network-function", 2, 2,
> + "NETWORK-FUNCTION-GROUP NETWORK-FUNCTION",
> + nbctl_pre_nf_group_add_network_function,
> + nbctl_nf_group_add_network_function, NULL, "--may-exist", RW },
> + { "network-function-group-del-network-function", 2, 2,
> + "NETWORK-FUNCTION-GROUP NETWORK-FUNCTION",
> + nbctl_pre_nf_group_del_network_function,
> + nbctl_nf_group_del_network_function, NULL, "--if-exists", RW },
> +
> + /* network-function commands. */
> + { "network-function-add", 3, 3, "NETWORK-FUNCTION PORT-IN PORT-OUT",
> + nbctl_pre_nf_add,
> + nbctl_nf_add,
> + NULL, "--may-exist", RW },
> + { "network-function-del", 1, 1, "NETWORK-FUNCTION", nbctl_pre_nf_del,
> + nbctl_nf_del,
> + NULL, "--if-exists", RW },
> + { "network-function-list", 0, 0, "", nbctl_pre_nf_list,
> + nbctl_nf_list, NULL, "", RO },
> +
I know it's quite late in the review cycle (v8) but maybe, if it's not
too much work, we could shorten these ovn-nbctl command names a bit.
Essentially just replace "network-function-" with "nf-" and
"network-function-group-" with "nfg-", e.g.:
ovn-nbctl nfg-add ...
ovn-nbctl nfg-add-nf ...
ovn-nbctl nfg-del-nf ...
What do you think?
> /* forwarding group commands. */
> { "fwd-group-add", 4, INT_MAX, "SWITCH GROUP VIP VMAC PORT...",
> nbctl_pre_fwd_group_add, nbctl_fwd_group_add, NULL, "--liveness", RW },
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
