Hi Sragdhara,
Thanks for the patch!
On 8/20/25 3:25 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.
> Min and max length of this list is 1.
> It also contains a unique id (1 and 255) generated by
> northd and and a reference to the current active NF.
It's a bit odd that northd generates this unique ID in the northbound
database. For most features, the NB database is not changed by
ovn-northd (there are some exceptions like reporting logical_port state
but those are not the norm).
Why can't the CMS generate and write this unqiue ID?
> The mode field is for future extension when we want
> to support both inline and mirror modes.
> Network_Function_Health_Check: Each row contains configuration for probes in
> options field:
> {interval, timeout, success_count, failure_count}
I have a related comment in patch 5/5: shouldn't we store the target IP
the NF health check should use? Instead of having a global config?
>
> 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 <sragdha.chau...@nutanix.com>
> Acked-by: Naveen Yerramneni <naveen.yerramn...@nutanix.com>
> Acked-by: Numan Siddique <num...@ovn.org>
> ---
> ovn-nb.ovsschema | 64 +++++++++++++++++++++++-
> ovn-nb.xml | 126 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 188 insertions(+), 2 deletions(-)
>
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index f55930a2e..7dc63021d 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": "80358356 43035",
> "tables": {
> "NB_Global": {
> "columns": {
> @@ -184,6 +184,61 @@
> "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"}}},
> + "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"}}},
> + "isRoot": true},
> + "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": 0,
> + "maxInteger": 255}}},
The commit message states that valid IDs are in the 1-255 range, we
should change minInteger here.
> + "external_ids": {
> + "type": {"key": "string", "value": "string",
> + "min": 0, "max": "unlimited"}}},
> + "isRoot": true},
With the comment I had at the top, that it's probably the CMS who should
generate the unique IDs, I think we should also add unique indexes to
this table.
I think we should have two indexes:
a. one to ensure unique names
b. one to ensure unique ids
E.g.:
"indexes": [["name"], ["id"]]
> "Forwarding_Group": {
> "columns": {
> "name": {"type": "string"},
> @@ -297,6 +352,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 cbe9c40bb..c6144067f 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2648,6 +2648,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.
> @@ -5974,4 +5981,123 @@ or
> </column>
> </group>
> </table>
> +
> +
Nit: one empty line too many.
> + <table name="Network_Function_Group"
> + title="network function group">
> + <p>
> + Each row contains a list of <ref table="Network_Function"/>. Health
> + monitoring of each Network_Function in the list would be done based
> + on parameters defined in <ref table="Network_Function_Health_Check"/>.
> + Traffic redirection would be done towards one of the active
> + Network_Functions. If all are detected to be down, one of the
> + Network_Functions is chosen for redirection.
> + </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, assigned to the
> + network function group.
> + </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.
> + </column>
> +
> + <column name="mode">
> + Network-function mode. The "inline" mode means the network-function
> + is directly in the path of network traffic - 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 matching the ACL are redirected to the
s/are redirected/is redirected/
> + inport for from-lport and to the outport for the to-lport ACLs. Once
> + it comes out of the other port, it gets forwarded nomally. The response
Typo: "nomally". But maybe we should rephrase to "Once traffic is
received on the 'outport' it is forwarded according to the regular OVN
pipeline."
> + traffic gets redirected to the outport for from-lport and to the inport
> + for the to-lport ACLs, and when it comes out of the other port, it
Same here, I think I'd rephrase this too.
> + gets forwarded. The packet header must not be modified by the network
> + function.
This is a very important restriction, it would be good to stress that,
e.g.: "NOTE: The packet headers MUST NOT be modified by the network
function."
> + </p>
> +
> + <column name="name">
> + Name of the <ref table="Network_Function"/>. Name should be unique.
We should enforce this into the schema, i.e., add an index for "name".
> + </column>
> +
> + <column name="inport">
> + Logical port UUID where request traffic for from-lport ACL and response
> + traffic for to-lport ACL are redirected.
> + </column>
> +
> + <column name="outport">
> + Logical port UUID where request traffic for to-lport ACL and response
> + traffic for from-lport ACL are redirected.
> + </column>
> +
> + <column name="health_check">
> + 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.
We should enforce this into the schema, i.e., add an index for "name".
> + </column>
> +
> +
> + <group title="Health check options">
> + <column name="options" key="interval" type='{"type": "integer"}'>
> + The interval, in seconds, between health checks.
> + </column>
> +
> + <column name="options" key="timeout" type='{"type": "integer"}'>
> + The time, in seconds, after which a health check times out.
> + </column>
> +
> + <column name="options" key="success_count" type='{"type": "integer"}'>
> + The number of successful checks after which the Network_Function is
> + considered online.
> + </column>
> +
> + <column name="options" key="failure_count" type='{"type": "integer"}'>
> + The number of failure checks after which the Network_Function is
> + considered offline.
> + </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>
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
