Thanks Dumitru. Addressed all comments in V9. Pls see inline for some responses.
*
Sragdhara
________________________________
From: Dumitru Ceara <dce...@redhat.com>
Sent: Thursday, August 28, 2025 1:52 AM
To: Sragdhara Datta Chaudhuri <sragdha.chau...@nutanix.com>;
ovs-dev@openvswitch.org <ovs-dev@openvswitch.org>
Cc: Numan Siddique <num...@ovn.org>
Subject: Re: [ovs-dev] [PATCH OVN v8 2/5] ovn-nbctl: Network Function insertion
commands.
!-------------------------------------------------------------------|
CAUTION: External Email
|-------------------------------------------------------------------!
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?
[sragdhara] changed the may-exist handling from no-op to overwrite, as you
suggested.
> + </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
[Sragdhara] We plan to add a new config knob to let user choose between
fail-open and fail-close behaviours. The former would let traffic flow through,
bypassing the NF, in case all NFs are down. The latter would drop the traffic
under the same condition. If neither mode is chosen, the behaviour will be what
is there in this diff, ie send to the first NF in the group.
note fit better in ovn-nb.xml, in the "Network_Function_Group" part of
the man page?
[sragdhara] I think this is already present in ovn-nb.xml. Let me know if
something is missing.
> + </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.
[sragdhara] that’s right. Thanks.
> + }
> + *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
);
[sragdhara] thanks. Changed it.
> +}
> +
> +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?
[sragdhara] done.
> /* 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
