Hi Dumitru, Thanks for your detailed review on the patch series once again. Regarding this patch, all comments are addressed in v9, except one. Please see inline.
Thanks, Sragdhara ________________________________ From: Dumitru Ceara <[email protected]> Sent: Wednesday, August 27, 2025 8:00 PM To: Sragdhara Datta Chaudhuri <[email protected]>; [email protected] <[email protected]> Cc: Numan Siddique <[email protected]> Subject: Re: [ovs-dev] [PATCH OVN v8 1/5] ovn-nb: Network Function insertion OVN-NB schema changes. !-------------------------------------------------------------------| CAUTION: External Email |-------------------------------------------------------------------! On 8/25/25 5:52 AM, Sragdhara Datta Chaudhuri wrote: > New tables: > Network_Function: Each row contains {inport, outport, health_check} > Network_Function_Group: Each row contains a list of Network_Function entities > and a unique id (between 1 and 255). > Min and max length of this list is 1. > Northd sets a reference to the current active NF. > The mode field is for future extension when we want > to support both inline and mirror modes. Currently > only inline is supported. > Network_Function_Health_Check: Each row contains configuration for probes in > options field: > {interval, timeout, success_count, failure_count} > > Modified table: > ACL: The ACL entity would have a new optional field that is a reference to a > Network_Function_Group entity. Only accepted for stateful allow ACLs. > > Signed-off-by: Sragdhara Datta Chaudhuri <[email protected]> > Acked-by: Naveen Yerramneni <[email protected]> > Acked-by: Numan Siddique <[email protected]> > --- Hi Sragdhara, Thanks for this new revision! > ovn-nb.ovsschema | 67 ++++++++++++++++++++++- > ovn-nb.xml | 135 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 200 insertions(+), 2 deletions(-) > > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > index f55930a2e..1e8f542b4 100644 > --- a/ovn-nb.ovsschema > +++ b/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > - "version": "7.12.0", > - "cksum": "2749576410 39903", > + "version": "7.13.0", > + "cksum": "3625876337 43148", > "tables": { > "NB_Global": { > "columns": { > @@ -184,6 +184,64 @@ > "min": 0, "max": "unlimited"}}}, > "indexes": [["name"]], > "isRoot": false}, > + "Network_Function_Health_Check": { > + "columns": { > + "name": {"type": "string"}, > + "options": { > + "type": {"key": "string", > + "value": "string", > + "min": 0, > + "max": "unlimited"}}, > + "external_ids": { > + "type": {"key": "string", "value": "string", > + "min": 0, "max": "unlimited"}}}, > + "indexes": [["name"]], > + "isRoot": true}, > + "Network_Function": { > + "columns": { > + "name": {"type": "string"}, > + "outport": {"type": {"key": {"type": "uuid", > + "refTable": > "Logical_Switch_Port", > + "refType": "strong"}, > + "min": 1, "max": 1}}, > + "inport": {"type": {"key": {"type": "uuid", > + "refTable": > "Logical_Switch_Port", > + "refType": "strong"}, > + "min": 1, "max": 1}}, > + "health_check": {"type": { > + "key": {"type": "uuid", > + "refTable": "Network_Function_Health_Check", > + "refType": "strong"}, > + "min": 0, "max": 1}}, > + "external_ids": { > + "type": {"key": "string", "value": "string", > + "min": 0, "max": "unlimited"}}}, > + "indexes": [["name"]], > + "isRoot": true}, Nit: I wonder if we should make this "isRoot": false. Numan what do you think? [sragdhara] Kept isRoot as true to allow CMS to create NF independent of the group, to exploit the health monitoring capability and build their logic on top of it. Updated isRoot to false for network_function_health_check though. > + "Network_Function_Group": { > + "columns": { > + "name": {"type": "string"}, > + "network_function": {"type": > + {"key": {"type": "uuid", > + "refTable": "Network_Function", > + "refType": "strong"}, > + "min": 0, "max": "unlimited"}}, > + "network_function_active": {"type": > + {"key": {"type": "uuid", > + "refTable": "Network_Function", > + "refType": "strong"}, > + "min": 0, "max": 1}}, > + "mode": {"type": {"key": {"type": "string", > + "enum": ["set", ["inline"]]}}}, > + "id": { > + "type": {"key": {"type": "integer", > + "minInteger": 1, > + "maxInteger": 255}}}, > + "external_ids": { > + "type": {"key": "string", "value": "string", > + "min": 0, "max": "unlimited"}}}, > + "indexes": [["name"], ["id"]], > + "isRoot": true}, > "Forwarding_Group": { > "columns": { > "name": {"type": "string"}, > @@ -297,6 +355,11 @@ > ["allow", "allow-related", > "allow-stateless", "drop", > "reject", "pass"]]}}}, > + "network_function_group": { > + "type": {"key": {"type": "uuid", > + "refTable": "Network_Function_Group", > + "refType": "strong"}, > + "min": 0, "max": 1}}, > "log": {"type": "boolean"}, > "severity": {"type": {"key": {"type": "string", > "enum": ["set", > diff --git a/ovn-nb.xml b/ovn-nb.xml > index b7b5b5c40..dbb031dab 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2716,6 +2716,13 @@ or > </p> > </column> > > + <column name="network_function_group"> > + <p> > + Group of network functions to which the traffic matching this ACL > + is redirected. > + </p> > + </column> > + > <group title="options"> > <p> > ACLs options. > @@ -6042,4 +6049,132 @@ or > </column> > </group> > </table> > + > + <table name="Network_Function_Group" > + title="network function group"> > + <p> > + Each row contains a list of <ref table="Network_Function"/>. Traffic > + redirection is achieved by referencing a > + <code>Network_Function_Group</code> from an <ref table="ACL"/>. Health > + monitoring of each <code>Network_Function</code> is performed based on > + parameters defined in <ref table="Network_Function_Health_Check"/>. > + Traffic matching the ACL is redirected to one of the active > + <code>Network_Functions</code>. If all are detected as down, traffic is > + redirected to one of the <code>Network_Functions</code> regardless of > + status. > + </p> > + > + <column name="name"> > + Name of the <ref table="Network_Function_Group"/>. Name should be > unique. > + </column> > + > + <column name="id"> > + A unique integer between 1 and 255 must be assigned to each > + <code>Network_Function_Group</code>. > + </column> > + > + <column name="network_function"> > + A list of network functions which belong to this group. > + </column> > + > + <column name="network_function_active"> > + Current active Network_Function. This column is populated by northd > + based on health monitoring status. > + </column> > + > + <column name="mode"> > + Traffic forwarding mode, with default and only value as "inline". The > + "inline" mode means that the network function is directly in the path > + of traffic, with traffic being redirected through it. > + </column> > + > + <group title="Common Columns"> > + <column name="external_ids"> > + See <em>External IDs</em> at the beginning of this document. > + </column> > + </group> > + </table> > + > + <table name="Network_Function" title="network function"> > + <p> > + Each row represents one network function entity. This contains a pair > + of logical_switch_ports. Traffic that matches the ACL is redirected to > + <code>inport</code> for from-lport ACLs and to <code>outport</code> > + for to-lport ACLs. Once the traffic is received on the other port, it > + continues through the standard OVN pipeline. > + Response traffic follows the reverse path: it is redirected to the > + <code>outport</code> for from-lport ACLs and to the <code>inport</code> > + for to-lport ACLs. Once the traffic is received on the other port, it > is > + processed by the regular OVN pipeline. > + > + <code>NOTE</code>: > + 1. The Network Function MUST NOT modify the packet headers. > + 2. The Network Function is not supported when used in conjunction > with > + Load Balancer. > + </p> > + > + <column name="name"> > + Name of the <ref table="Network_Function"/>. Name should be unique. > + </column> > + > + <column name="inport"> > + Logical port UUID where request traffic for from-lport ACL and response Nit: Logical switch port.. Or better: <ref table="Logical_Switch_Port"> where the.. > + traffic for to-lport ACL is redirected. > + </column> > + > + <column name="outport"> > + Logical port UUID where request traffic for to-lport ACL and response Nit: Logical switch port.. Or better: <ref table="Logical_Switch_Port"> where request.. > + traffic for from-lport ACL is redirected. > + </column> > + > + <column name="health_check"> > + Health check associated with this network function. Nit: maybe rephrase this to something like: "<ref table="Network_Function_Health_Check"/> associated with this network function." > + </column> > + > + <group title="Common Columns"> > + <column name="external_ids"> > + See <em>External IDs</em> at the beginning of this document. > + </column> > + </group> > + </table> > + > + <table name="Network_Function_Health_Check" > + title="network function health check"> > + <p> > + Each row represents one network function health check. > + </p> > + > + <column name="name"> > + Name of the <ref table="Network_Function_Health_Check"/>. > + Name should be unique. > + </column> > + > + > + <group title="Health check options"> > + <column name="options" key="interval" type='{"type": "integer"}'> > + The interval, in seconds, between health checks. Nit: I know we don't mention it for Load_Balancer_Health_Check but it would be good to list the default here. E.g., add "Default: 5s". > + </column> > + > + <column name="options" key="timeout" type='{"type": "integer"}'> > + The time, in seconds, after which a health check times out. Nit: add "Default: 3s". > + </column> > + > + <column name="options" key="success_count" type='{"type": "integer"}'> > + The number of successful checks after which the Network_Function is > + considered online. Nit: add "Default: 1". > + </column> > + > + <column name="options" key="failure_count" type='{"type": "integer"}'> > + The number of failure checks after which the Network_Function is > + considered offline. Nit: add "Default: 1". > + </column> > + </group> > + > + <group title="Common Columns"> > + <column name="external_ids"> > + See <em>External IDs</em> at the beginning of this document. > + </column> > + </group> > + </table> > + > </database> Aside from that the rest seems good to me. I am still reviewing the rest of the series so please wait with posting a v9. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
