On 8/25/25 10:39 AM, Sragdhara Datta Chaudhuri wrote:
> Hi Dumitru,
>
Hi Sragdhara,
> Thanks for the comments. Have addressed all except one. Please see below.
>
> Thanks,
> Sragdhara
>
>
> From: Dumitru Ceara <dce...@redhat.com>
> Date: Thursday, August 21, 2025 at 3:23 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 v6 2/5] ovn-nbctl: Network Function
> insertion commands.
> !-------------------------------------------------------------------|
> CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> On 8/20/25 3:25 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> [<nf1> <nf2> ...] - Add a new NFG
>> 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,
>
>> utilities/ovn-nbctl.8.xml | 106 +++++++-
>> utilities/ovn-nbctl.c | 533 +++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 635 insertions(+), 4 deletions(-)
>>
>> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
>> index 60936a2b5..ce8f6f3a4 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,110 @@
>> </dd>
>> </dl>
>>
>> + <h2>Network Function Commands</h2>
>> +
>> + <dl>
>> + <dt>[<code>--may-exist</code>] | <code>--add-duplicate</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. With <code>--add-duplicate</code>, the command really
>> + creates a new network function with a duplicate name. It is an
>> + error to specify both options. If there are multiple network
>> + functions with the same name, configure the network functions
>> + using the UUID instead of the <var>nf</var> name.
>
> Why would we allow network functions with duplicate names? In any case,
> if we do want to allow that, please ignore my comment as adding the
> network function "name" as an unique index in the schema. We still need
> the "id" to be a unique index.
>
> In general, all new tables in the NB/SB schema should have at least one
> unique index. That's a good practice and avoids the (limited) risk of
> spurious duplicates in the database when running with raft, see the
> following examples for situations where we had to handle spurious
> records that were created due to lack of uniqueness constraints:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_9deb000&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=uXnTjPDrt8WYa8nbZqANTqL0TyzFTTKpPHphGFPgvBw&m=d2L3VNIWqglvWWyaeRvu_gD66zSbKUIIFck4XCsYyZJGC3bR6FcXKK3Z6vyz34pw&s=gT__RfP5hrGjMon1lcpiKymnuhFPIVVdj0tviEgAE5g&e=
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_b5387b3&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=uXnTjPDrt8WYa8nbZqANTqL0TyzFTTKpPHphGFPgvBw&m=d2L3VNIWqglvWWyaeRvu_gD66zSbKUIIFck4XCsYyZJGC3bR6FcXKK3Z6vyz34pw&s=QmPqb3CVN-FSQfy0_3Kh9aOoHIp3ziMRvxS3_kSEOb4&e=
>
> [Sragdhara] Not allowing duplicate any more after adding index in the schema.
>
>> + </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>--add-duplicate</code>]
>> <code>network-function-group-add</code> <var>nfg</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.
>> + </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. With <code>--add-duplicate</code>, the command
>> + really creates a new network function group with a duplicate name.
>
> I have here the same comment about name uniqueness as above.
>
>> + It is an error to specify both options. If there are multiple
>> + network function groups with the same name, configure the network
>> + function groups using the UUID instead of the <var>nfg</var> name.
>> + </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..5c0874106 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 [NETWORK-FUNCTION]...\n\
>> + create a network-functionr-group\n\
>
> Typo: network-functionr-group
>
>> + 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("\
>
> Why do we need a new printf() here? Why not just update the string?
>
> [Sragdhara] Needed to break it here, otherwise “line too long” error was
> coming from test.
>
I see, OK, let's leave it as in your patch.
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
