[ovs-dev,v2,1/1] ovn-sb: Introduce Route table.

The Route table will be used in the future to coordinate routing
information between northd and ovn-controller.
Northd will insert routes that should be advertised to the outside
fabric.
Ovn-controller will insert routes that have been learned from the
outside fabric.

Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
---
 ovn-sb.ovsschema | 29 ++++++++++++++++--
 ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 2 deletions(-)

Not sure what happened here, but this should have no effect on the ECMP
routes test case.

Recheck-request: github-robot

On Tue, Dec 03, 2024 at 03:58:27PM +0100, Felix Huettner via dev wrote:
> The Route table will be used in the future to coordinate routing
> information between northd and ovn-controller.
> Northd will insert routes that should be advertised to the outside
> fabric.
> Ovn-controller will insert routes that have been learned from the
> outside fabric.
> 
> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> ---
>  ovn-sb.ovsschema | 29 ++++++++++++++++--
>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index 73abf2c8d..76d32197d 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Southbound",
> -    "version": "20.37.0",
> -    "cksum": "1950136776 31493",
> +    "version": "20.38.0",
> +    "cksum": "550338719 32889",
>      "tables": {
>          "SB_Global": {
>              "columns": {
> @@ -617,6 +617,31 @@
>                      "type": {"key": "string", "value": "string",
>                               "min": 0, "max": "unlimited"}}},
>              "indexes": [["chassis"]],
> +            "isRoot": true},
> +        "Route": {
> +            "columns": {
> +                "datapath":
> +                    {"type": {"key": {"type": "uuid",
> +                                      "refTable": "Datapath_Binding"}}},
> +                "logical_port": {"type": {"key": {"type": "uuid",
> +                                                  "refTable": "Port_Binding",
> +                                                  "refType": "weak"}}},
> +                "ip_prefix": {"type": "string"},
> +                "nexthop": {"type": "string"},
> +                "tracked_port": {"type": {"key": {"type": "uuid",
> +                                                  "refTable": "Port_Binding",
> +                                                  "refType": "weak"},
> +                                          "min": 0,
> +                                          "max": 1}},
> +                "type": {"type": {"key": {"type": "string",
> +                                          "enum": ["set", ["advertise",
> +                                                           "receive"]]},
> +                                    "min": 1, "max": 1}},
> +                "external_ids": {
> +                    "type": {"key": "string", "value": "string",
> +                             "min": 0, "max": "unlimited"}}},
> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
> +                         "type"]],
>              "isRoot": true}
>      }
>  }
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index ea4adc1c3..fa9e7b4ba 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -5217,4 +5217,84 @@ tcp.flags = RST;
>        The set of variable values for a given chassis.
>      </column>
>    </table>
> +
> +  <table name="Route">
> +    <p>
> +      Each record represents a route thas is export from ovn or imported to ovn
> +      using some dynamic routing logic outside of ovn.
> +      It is populated on the one hand by <code>ovn-northd</code> based on the
> +      addresses, routes and NAT Entries of a
> +      <code>OVN_Northbound.Logical_Router_Port</code>.
> +      On the other hand <code>ovn-controller</code> populates it with routes
> +      learned from outside of OVN.
> +    </p>
> +
> +    <column name="datapath">
> +      The datapath belonging to the
> +      <code>OVN_Northbound.Logical_Router</code> that this route is valid
> +      for.
> +    </column>
> +
> +    <column name="logical_port">
> +      <p>
> +        If the type is <code>advertise</code> then this is the logical_port
> +        the router will send packets out.
> +      </p>
> +
> +      <p>
> +        If the type is <code>receive</code> then this is the logical_port
> +        the route was learned on.
> +      </p>
> +    </column>
> +
> +    <column name="ip_prefix">
> +      <p>
> +        IP prefix of this route (e.g. 192.168.100.0/24).
> +      </p>
> +    </column>
> +
> +    <column name="nexthop">
> +      <p>
> +        If the type is <code>advertise</code> then this is empty.
> +      </p>
> +
> +      <p>
> +        If the type is <code>receive</code> then this is the nexthop ip we
> +        from the outside.
> +      </p>
> +    </column>
> +
> +    <column name="tracked_port">
> +      <p>
> +        Only relevant for type <code>advertise</code>.
> +
> +        In combination with a host <code>ip_prefix</code> this trackes the port
> +        OVN will forward the packets for this destination to.
> +
> +        An announcing chassis can use this information to check if this
> +        destination is local and adjust the route priorities based on that.
> +      </p>
> +    </column>
> +
> +    <column name="type">
> +      <p>
> +        If the route is to be exported from OVN to the outside network or if
> +        it is imported from the outside network.
> +      </p>
> +      <ul>
> +        <li>
> +          <code>advertise</code>: This route should be advertised to the
> +          outside network.
> +       </li>
> +        <li>
> +          <code>receive</code>: This route has been learned from the outside
> +          network.
> +        </li>
> +      </ul>
> +    </column>
> +
> +    <column name="external_ids">
> +      See <em>External IDs</em> at the beginning of this document.
> +    </column>
> +  </table>
>  </database>
> -- 
> 2.47.0
> 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
> The Route table will be used in the future to coordinate routing
> information between northd and ovn-controller.
> Northd will insert routes that should be advertised to the outside
> fabric.
> Ovn-controller will insert routes that have been learned from the
> outside fabric.
> 
> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> ---

Hi Felix,

I have a few minor comments, otherwise, this looks ok to me.

>  ovn-sb.ovsschema | 29 ++++++++++++++++--
>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index 73abf2c8d..76d32197d 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Southbound",
> -    "version": "20.37.0",
> -    "cksum": "1950136776 31493",
> +    "version": "20.38.0",
> +    "cksum": "550338719 32889",
>      "tables": {
>          "SB_Global": {
>              "columns": {
> @@ -617,6 +617,31 @@
>                      "type": {"key": "string", "value": "string",
>                               "min": 0, "max": "unlimited"}}},
>              "indexes": [["chassis"]],
> +            "isRoot": true},
> +        "Route": {
> +            "columns": {
> +                "datapath":
> +                    {"type": {"key": {"type": "uuid",
> +                                      "refTable": "Datapath_Binding"}}},
> +                "logical_port": {"type": {"key": {"type": "uuid",
> +                                                  "refTable": "Port_Binding",
> +                                                  "refType": "weak"}}},

It's probably fine to keep this as a strong reference, right?

> +                "ip_prefix": {"type": "string"},
> +                "nexthop": {"type": "string"},
> +                "tracked_port": {"type": {"key": {"type": "uuid",
> +                                                  "refTable": "Port_Binding",
> +                                                  "refType": "weak"},

Same here, I guess.

> +                                          "min": 0,
> +                                          "max": 1}},
> +                "type": {"type": {"key": {"type": "string",
> +                                          "enum": ["set", ["advertise",
> +                                                           "receive"]]},
> +                                    "min": 1, "max": 1}},
> +                "external_ids": {
> +                    "type": {"key": "string", "value": "string",
> +                             "min": 0, "max": "unlimited"}}},
> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
> +                         "type"]],
>              "isRoot": true}

Do we really need this table to be root?

>      }
>  }
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index ea4adc1c3..fa9e7b4ba 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -5217,4 +5217,84 @@ tcp.flags = RST;
>        The set of variable values for a given chassis.
>      </column>
>    </table>
> +
> +  <table name="Route">
> +    <p>
> +      Each record represents a route thas is export from ovn or imported to ovn

Typo: thas
Maybe: s/export/exported/

> +      using some dynamic routing logic outside of ovn.
> +      It is populated on the one hand by <code>ovn-northd</code> based on the
> +      addresses, routes and NAT Entries of a
> +      <code>OVN_Northbound.Logical_Router_Port</code>.
> +      On the other hand <code>ovn-controller</code> populates it with routes
> +      learned from outside of OVN.
> +    </p>
> +
> +    <column name="datapath">
> +      The datapath belonging to the
> +      <code>OVN_Northbound.Logical_Router</code> that this route is valid
> +      for.
> +    </column>
> +
> +    <column name="logical_port">
> +      <p>
> +        If the type is <code>advertise</code> then this is the logical_port
> +        the router will send packets out.
> +      </p>
> +
> +      <p>
> +        If the type is <code>receive</code> then this is the logical_port
> +        the route was learned on.
> +      </p>
> +    </column>
> +
> +    <column name="ip_prefix">
> +      <p>
> +        IP prefix of this route (e.g. 192.168.100.0/24).
> +      </p>
> +    </column>
> +
> +    <column name="nexthop">
> +      <p>
> +        If the type is <code>advertise</code> then this is empty.
> +      </p>
> +
> +      <p>
> +        If the type is <code>receive</code> then this is the nexthop ip we
> +        from the outside.
> +      </p>
> +    </column>
> +
> +    <column name="tracked_port">
> +      <p>
> +        Only relevant for type <code>advertise</code>.
> +
> +        In combination with a host <code>ip_prefix</code> this trackes the port

Typo: trackes

> +        OVN will forward the packets for this destination to.
> +
> +        An announcing chassis can use this information to check if this
> +        destination is local and adjust the route priorities based on that.
> +      </p>
> +    </column>
> +
> +    <column name="type">
> +      <p>
> +        If the route is to be exported from OVN to the outside network or if
> +        it is imported from the outside network.
> +      </p>
> +      <ul>
> +        <li>
> +          <code>advertise</code>: This route should be advertised to the
> +          outside network.
> +       </li>
> +        <li>
> +          <code>receive</code>: This route has been learned from the outside
> +          network.
> +        </li>
> +      </ul>
> +    </column>
> +
> +    <column name="external_ids">
> +      See <em>External IDs</em> at the beginning of this document.
> +    </column>
> +  </table>
>  </database>

Regards,
Dumitru

On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote:
> On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
> > The Route table will be used in the future to coordinate routing
> > information between northd and ovn-controller.
> > Northd will insert routes that should be advertised to the outside
> > fabric.
> > Ovn-controller will insert routes that have been learned from the
> > outside fabric.
> > 
> > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> > ---
> 
> Hi Felix,
> 
> I have a few minor comments, otherwise, this looks ok to me.

Hi Dimitru,

thanks a lot for the review.

> 
> >  ovn-sb.ovsschema | 29 ++++++++++++++++--
> >  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 107 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> > index 73abf2c8d..76d32197d 100644
> > --- a/ovn-sb.ovsschema
> > +++ b/ovn-sb.ovsschema
> > @@ -1,7 +1,7 @@
> >  {
> >      "name": "OVN_Southbound",
> > -    "version": "20.37.0",
> > -    "cksum": "1950136776 31493",
> > +    "version": "20.38.0",
> > +    "cksum": "550338719 32889",
> >      "tables": {
> >          "SB_Global": {
> >              "columns": {
> > @@ -617,6 +617,31 @@
> >                      "type": {"key": "string", "value": "string",
> >                               "min": 0, "max": "unlimited"}}},
> >              "indexes": [["chassis"]],
> > +            "isRoot": true},
> > +        "Route": {
> > +            "columns": {
> > +                "datapath":
> > +                    {"type": {"key": {"type": "uuid",
> > +                                      "refTable": "Datapath_Binding"}}},
> > +                "logical_port": {"type": {"key": {"type": "uuid",
> > +                                                  "refTable": "Port_Binding",
> > +                                                  "refType": "weak"}}},
> 
> It's probably fine to keep this as a strong reference, right?
> 
> > +                "ip_prefix": {"type": "string"},
> > +                "nexthop": {"type": "string"},
> > +                "tracked_port": {"type": {"key": {"type": "uuid",
> > +                                                  "refTable": "Port_Binding",
> > +                                                  "refType": "weak"},
> 
> Same here, I guess.

Yes, will both be changed in v3.

> 
> > +                                          "min": 0,
> > +                                          "max": 1}},
> > +                "type": {"type": {"key": {"type": "string",
> > +                                          "enum": ["set", ["advertise",
> > +                                                           "receive"]]},
> > +                                    "min": 1, "max": 1}},
> > +                "external_ids": {
> > +                    "type": {"key": "string", "value": "string",
> > +                             "min": 0, "max": "unlimited"}}},
> > +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
> > +                         "type"]],
> >              "isRoot": true}
> 
> Do we really need this table to be root?

As i understood the isRoot flag it keeps the records around even if
there is no incoming reference to any of the rows. As there are no
incoming references to the Route table at all we need this to be true.

Thanks a lot
Felix

> 
> >      }
> >  }
> > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > index ea4adc1c3..fa9e7b4ba 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -5217,4 +5217,84 @@ tcp.flags = RST;
> >        The set of variable values for a given chassis.
> >      </column>
> >    </table>
> > +
> > +  <table name="Route">
> > +    <p>
> > +      Each record represents a route thas is export from ovn or imported to ovn
> 
> Typo: thas
> Maybe: s/export/exported/
> 
> > +      using some dynamic routing logic outside of ovn.
> > +      It is populated on the one hand by <code>ovn-northd</code> based on the
> > +      addresses, routes and NAT Entries of a
> > +      <code>OVN_Northbound.Logical_Router_Port</code>.
> > +      On the other hand <code>ovn-controller</code> populates it with routes
> > +      learned from outside of OVN.
> > +    </p>
> > +
> > +    <column name="datapath">
> > +      The datapath belonging to the
> > +      <code>OVN_Northbound.Logical_Router</code> that this route is valid
> > +      for.
> > +    </column>
> > +
> > +    <column name="logical_port">
> > +      <p>
> > +        If the type is <code>advertise</code> then this is the logical_port
> > +        the router will send packets out.
> > +      </p>
> > +
> > +      <p>
> > +        If the type is <code>receive</code> then this is the logical_port
> > +        the route was learned on.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="ip_prefix">
> > +      <p>
> > +        IP prefix of this route (e.g. 192.168.100.0/24).
> > +      </p>
> > +    </column>
> > +
> > +    <column name="nexthop">
> > +      <p>
> > +        If the type is <code>advertise</code> then this is empty.
> > +      </p>
> > +
> > +      <p>
> > +        If the type is <code>receive</code> then this is the nexthop ip we
> > +        from the outside.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="tracked_port">
> > +      <p>
> > +        Only relevant for type <code>advertise</code>.
> > +
> > +        In combination with a host <code>ip_prefix</code> this trackes the port
> 
> Typo: trackes
> 
> > +        OVN will forward the packets for this destination to.
> > +
> > +        An announcing chassis can use this information to check if this
> > +        destination is local and adjust the route priorities based on that.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="type">
> > +      <p>
> > +        If the route is to be exported from OVN to the outside network or if
> > +        it is imported from the outside network.
> > +      </p>
> > +      <ul>
> > +        <li>
> > +          <code>advertise</code>: This route should be advertised to the
> > +          outside network.
> > +       </li>
> > +        <li>
> > +          <code>receive</code>: This route has been learned from the outside
> > +          network.
> > +        </li>
> > +      </ul>
> > +    </column>
> > +
> > +    <column name="external_ids">
> > +      See <em>External IDs</em> at the beginning of this document.
> > +    </column>
> > +  </table>
> >  </database>
> 
> Regards,
> Dumitru
> 
>

On 12/5/24 1:59 PM, Felix Huettner wrote:
> On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote:
>> On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
>>> The Route table will be used in the future to coordinate routing
>>> information between northd and ovn-controller.
>>> Northd will insert routes that should be advertised to the outside
>>> fabric.
>>> Ovn-controller will insert routes that have been learned from the
>>> outside fabric.
>>>
>>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
>>> ---
>>
>> Hi Felix,
>>
>> I have a few minor comments, otherwise, this looks ok to me.
> 
> Hi Dimitru,
> 
> thanks a lot for the review.
> 
>>
>>>  ovn-sb.ovsschema | 29 ++++++++++++++++--
>>>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 107 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>>> index 73abf2c8d..76d32197d 100644
>>> --- a/ovn-sb.ovsschema
>>> +++ b/ovn-sb.ovsschema
>>> @@ -1,7 +1,7 @@
>>>  {
>>>      "name": "OVN_Southbound",
>>> -    "version": "20.37.0",
>>> -    "cksum": "1950136776 31493",
>>> +    "version": "20.38.0",
>>> +    "cksum": "550338719 32889",
>>>      "tables": {
>>>          "SB_Global": {
>>>              "columns": {
>>> @@ -617,6 +617,31 @@
>>>                      "type": {"key": "string", "value": "string",
>>>                               "min": 0, "max": "unlimited"}}},
>>>              "indexes": [["chassis"]],
>>> +            "isRoot": true},
>>> +        "Route": {
>>> +            "columns": {
>>> +                "datapath":
>>> +                    {"type": {"key": {"type": "uuid",
>>> +                                      "refTable": "Datapath_Binding"}}},
>>> +                "logical_port": {"type": {"key": {"type": "uuid",
>>> +                                                  "refTable": "Port_Binding",
>>> +                                                  "refType": "weak"}}},
>>
>> It's probably fine to keep this as a strong reference, right?
>>
>>> +                "ip_prefix": {"type": "string"},
>>> +                "nexthop": {"type": "string"},
>>> +                "tracked_port": {"type": {"key": {"type": "uuid",
>>> +                                                  "refTable": "Port_Binding",
>>> +                                                  "refType": "weak"},
>>
>> Same here, I guess.
> 
> Yes, will both be changed in v3.
> 
>>
>>> +                                          "min": 0,
>>> +                                          "max": 1}},
>>> +                "type": {"type": {"key": {"type": "string",
>>> +                                          "enum": ["set", ["advertise",
>>> +                                                           "receive"]]},
>>> +                                    "min": 1, "max": 1}},
>>> +                "external_ids": {
>>> +                    "type": {"key": "string", "value": "string",
>>> +                             "min": 0, "max": "unlimited"}}},
>>> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
>>> +                         "type"]],
>>>              "isRoot": true}
>>
>> Do we really need this table to be root?
> 
> As i understood the isRoot flag it keeps the records around even if
> there is no incoming reference to any of the rows. As there are no
> incoming references to the Route table at all we need this to be true.
> 

Ah, you're right about that.  But shouldn't actually routes be referred
by Datapath_Binding instead of routes pointing to Datapath_binding?
Maybe that's a better schema definition?  In that case isRoot can be
false for the Routes table.

It doesn't really make sense to have route records for routers that
don't exist in the DB anymore.

Thanks,
Dumitru

On Thu, Dec 05, 2024 at 03:23:23PM +0100, Dumitru Ceara wrote:
> On 12/5/24 1:59 PM, Felix Huettner wrote:
> > On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote:
> >> On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
> >>> The Route table will be used in the future to coordinate routing
> >>> information between northd and ovn-controller.
> >>> Northd will insert routes that should be advertised to the outside
> >>> fabric.
> >>> Ovn-controller will insert routes that have been learned from the
> >>> outside fabric.
> >>>
> >>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> >>> ---
> >>
> >> Hi Felix,
> >>
> >> I have a few minor comments, otherwise, this looks ok to me.
> > 
> > Hi Dimitru,
> > 
> > thanks a lot for the review.
> > 
> >>
> >>>  ovn-sb.ovsschema | 29 ++++++++++++++++--
> >>>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 107 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> >>> index 73abf2c8d..76d32197d 100644
> >>> --- a/ovn-sb.ovsschema
> >>> +++ b/ovn-sb.ovsschema
> >>> @@ -1,7 +1,7 @@
> >>>  {
> >>>      "name": "OVN_Southbound",
> >>> -    "version": "20.37.0",
> >>> -    "cksum": "1950136776 31493",
> >>> +    "version": "20.38.0",
> >>> +    "cksum": "550338719 32889",
> >>>      "tables": {
> >>>          "SB_Global": {
> >>>              "columns": {
> >>> @@ -617,6 +617,31 @@
> >>>                      "type": {"key": "string", "value": "string",
> >>>                               "min": 0, "max": "unlimited"}}},
> >>>              "indexes": [["chassis"]],
> >>> +            "isRoot": true},
> >>> +        "Route": {
> >>> +            "columns": {
> >>> +                "datapath":
> >>> +                    {"type": {"key": {"type": "uuid",
> >>> +                                      "refTable": "Datapath_Binding"}}},
> >>> +                "logical_port": {"type": {"key": {"type": "uuid",
> >>> +                                                  "refTable": "Port_Binding",
> >>> +                                                  "refType": "weak"}}},
> >>
> >> It's probably fine to keep this as a strong reference, right?
> >>
> >>> +                "ip_prefix": {"type": "string"},
> >>> +                "nexthop": {"type": "string"},
> >>> +                "tracked_port": {"type": {"key": {"type": "uuid",
> >>> +                                                  "refTable": "Port_Binding",
> >>> +                                                  "refType": "weak"},
> >>
> >> Same here, I guess.
> > 
> > Yes, will both be changed in v3.
> > 
> >>
> >>> +                                          "min": 0,
> >>> +                                          "max": 1}},
> >>> +                "type": {"type": {"key": {"type": "string",
> >>> +                                          "enum": ["set", ["advertise",
> >>> +                                                           "receive"]]},
> >>> +                                    "min": 1, "max": 1}},
> >>> +                "external_ids": {
> >>> +                    "type": {"key": "string", "value": "string",
> >>> +                             "min": 0, "max": "unlimited"}}},
> >>> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
> >>> +                         "type"]],
> >>>              "isRoot": true}
> >>
> >> Do we really need this table to be root?
> > 
> > As i understood the isRoot flag it keeps the records around even if
> > there is no incoming reference to any of the rows. As there are no
> > incoming references to the Route table at all we need this to be true.
> > 
> 
> Ah, you're right about that.  But shouldn't actually routes be referred
> by Datapath_Binding instead of routes pointing to Datapath_binding?
> Maybe that's a better schema definition?  In that case isRoot can be
> false for the Routes table.
> 
> It doesn't really make sense to have route records for routers that
> don't exist in the DB anymore.

Hi Dumitru,

I tried this out and noticed something that might stand in the way of
this change. However it is quite likely that it might be just my limited
understanding of ovsdb :)

I changed logical_port and tracked_port above to be strong references.
Also i removed the datapath key and added in Datapath_Binding a
reference to Routes. Additionally i set isRoot to false.

If we now assume we have a working setup with LR, LRPs and some created
routes by northd.
Now if we want to delete a LRP we need to:
* Delete the Port_Binding in the southbound
* Delete all Routes referencing the Port_Binding
* Remove these routes from the list in the Datapath_Binding

The problem i encountered is that this creates a "referential integrity
violation" because Routes are still referencing the deleted
Port_Bindings.

As i understood it this is caused by:
1. the ovsdb idl not actually sending deletes for nonRoot tables
   (https://github.com/openvswitch/ovs/blob/main/lib/ovsdb-idl.c#L3290)
2. the ovsdb server first checking referential integrity and afterwards
   deleting orphaned rows
   (https://github.com/openvswitch/ovs/blob/main/ovsdb/transaction.c#L1099-L1109)

So from a northd perspective it sends the (from my understanding)
correct changes:
1. Deleting the Port_Binding
2. Removing any refernce to the Route entries
It does not send a delete for the Route entries, as that is a result of
the references.

However due to the order of ovsdb server evaluating these things we get
an "integrity violation".

I am not sure how to best address this issue.
I guess having multiple transactions in northd would work, but is quite
complex and i don't think we do this anywhere else.
Alternatively we would need to remove one of the problematic parts from
the schema, so we could:
* Define Route as a root table
* Remove the reference from logical_port/tracked_port to Port_Binding
* Maybe changing the references of logical_port/tracked_port to be weak
  might also work.

I am quite unsure what the most appropriate solution is or if i just
missunderstood something somewhere.
So any ideas would be appreciated.

Thanks a lot
Felix

> 
> Thanks,
> Dumitru
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 12/9/24 9:05 AM, Felix Huettner wrote:
> On Thu, Dec 05, 2024 at 03:23:23PM +0100, Dumitru Ceara wrote:
>> On 12/5/24 1:59 PM, Felix Huettner wrote:
>>> On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote:
>>>> On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
>>>>> The Route table will be used in the future to coordinate routing
>>>>> information between northd and ovn-controller.
>>>>> Northd will insert routes that should be advertised to the outside
>>>>> fabric.
>>>>> Ovn-controller will insert routes that have been learned from the
>>>>> outside fabric.
>>>>>
>>>>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
>>>>> ---
>>>>
>>>> Hi Felix,
>>>>
>>>> I have a few minor comments, otherwise, this looks ok to me.
>>>
>>> Hi Dimitru,
>>>
>>> thanks a lot for the review.
>>>
>>>>
>>>>>  ovn-sb.ovsschema | 29 ++++++++++++++++--
>>>>>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  2 files changed, 107 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>>>>> index 73abf2c8d..76d32197d 100644
>>>>> --- a/ovn-sb.ovsschema
>>>>> +++ b/ovn-sb.ovsschema
>>>>> @@ -1,7 +1,7 @@
>>>>>  {
>>>>>      "name": "OVN_Southbound",
>>>>> -    "version": "20.37.0",
>>>>> -    "cksum": "1950136776 31493",
>>>>> +    "version": "20.38.0",
>>>>> +    "cksum": "550338719 32889",
>>>>>      "tables": {
>>>>>          "SB_Global": {
>>>>>              "columns": {
>>>>> @@ -617,6 +617,31 @@
>>>>>                      "type": {"key": "string", "value": "string",
>>>>>                               "min": 0, "max": "unlimited"}}},
>>>>>              "indexes": [["chassis"]],
>>>>> +            "isRoot": true},
>>>>> +        "Route": {
>>>>> +            "columns": {
>>>>> +                "datapath":
>>>>> +                    {"type": {"key": {"type": "uuid",
>>>>> +                                      "refTable": "Datapath_Binding"}}},
>>>>> +                "logical_port": {"type": {"key": {"type": "uuid",
>>>>> +                                                  "refTable": "Port_Binding",
>>>>> +                                                  "refType": "weak"}}},
>>>>
>>>> It's probably fine to keep this as a strong reference, right?
>>>>
>>>>> +                "ip_prefix": {"type": "string"},
>>>>> +                "nexthop": {"type": "string"},
>>>>> +                "tracked_port": {"type": {"key": {"type": "uuid",
>>>>> +                                                  "refTable": "Port_Binding",
>>>>> +                                                  "refType": "weak"},
>>>>
>>>> Same here, I guess.
>>>
>>> Yes, will both be changed in v3.
>>>
>>>>
>>>>> +                                          "min": 0,
>>>>> +                                          "max": 1}},
>>>>> +                "type": {"type": {"key": {"type": "string",
>>>>> +                                          "enum": ["set", ["advertise",
>>>>> +                                                           "receive"]]},
>>>>> +                                    "min": 1, "max": 1}},
>>>>> +                "external_ids": {
>>>>> +                    "type": {"key": "string", "value": "string",
>>>>> +                             "min": 0, "max": "unlimited"}}},
>>>>> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
>>>>> +                         "type"]],
>>>>>              "isRoot": true}
>>>>
>>>> Do we really need this table to be root?
>>>
>>> As i understood the isRoot flag it keeps the records around even if
>>> there is no incoming reference to any of the rows. As there are no
>>> incoming references to the Route table at all we need this to be true.
>>>
>>
>> Ah, you're right about that.  But shouldn't actually routes be referred
>> by Datapath_Binding instead of routes pointing to Datapath_binding?
>> Maybe that's a better schema definition?  In that case isRoot can be
>> false for the Routes table.
>>
>> It doesn't really make sense to have route records for routers that
>> don't exist in the DB anymore.
> 
> Hi Dumitru,
> 

Hi Felix,

> I tried this out and noticed something that might stand in the way of
> this change. However it is quite likely that it might be just my limited
> understanding of ovsdb :)
> 
> I changed logical_port and tracked_port above to be strong references.
> Also i removed the datapath key and added in Datapath_Binding a
> reference to Routes. Additionally i set isRoot to false.
> 
> If we now assume we have a working setup with LR, LRPs and some created
> routes by northd.
> Now if we want to delete a LRP we need to:
> * Delete the Port_Binding in the southbound
> * Delete all Routes referencing the Port_Binding
> * Remove these routes from the list in the Datapath_Binding
> 
> The problem i encountered is that this creates a "referential integrity
> violation" because Routes are still referencing the deleted
> Port_Bindings.
> 
> As i understood it this is caused by:
> 1. the ovsdb idl not actually sending deletes for nonRoot tables
>    (https://github.com/openvswitch/ovs/blob/main/lib/ovsdb-idl.c#L3290)

I wasn't aware of this, thanks for pointing it out (CC Ilya).
Introduced a long time ago by:
https://github.com/openvswitch/ovs/commit/dcd1dbc

This is actually a bigger problem than you described here.  These
records are to be created/deleted by ovn-controller.  But ovn-controller
effectively can't delete any non-root Route records if they're still
referenced by Datapath_Binding.

> 2. the ovsdb server first checking referential integrity and afterwards
>    deleting orphaned rows
>    (https://github.com/openvswitch/ovs/blob/main/ovsdb/transaction.c#L1099-L1109)
> 
> So from a northd perspective it sends the (from my understanding)
> correct changes:
> 1. Deleting the Port_Binding
> 2. Removing any refernce to the Route entries
> It does not send a delete for the Route entries, as that is a result of
> the references.
> 
> However due to the order of ovsdb server evaluating these things we get
> an "integrity violation".
> 
> I am not sure how to best address this issue.
> I guess having multiple transactions in northd would work, but is quite
> complex and i don't think we do this anywhere else.

I don't think we should do that, it sounds too risky.

> Alternatively we would need to remove one of the problematic parts from
> the schema, so we could:
> * Define Route as a root table
> * Remove the reference from logical_port/tracked_port to Port_Binding
> * Maybe changing the references of logical_port/tracked_port to be weak
>   might also work.
> 
> I am quite unsure what the most appropriate solution is or if i just
> missunderstood something somewhere.
> So any ideas would be appreciated.
> 

It seems to me that the only reasonable way forward is what you had in
your patch originally:

- Route table as "root"
- Route records with (strong) references to port bindings
- Route records with (strong) references to datapath_bindings

Which means whenever:
- a route is added by ovn-controller, northd only gets IDL updates for
that new route record

- a route is deleted by ovn-controller, northd only gets IDL updates for
the removed route record

- northd needs to delete a port binding it must also delete all route
records referencing that port binding - in the same transaction.

- northd needs to delete a datapath binding it must also delete all
route records referencing that datapath binding - in the same transaction

- northd updates a datapath binding (e.g., a port is added or deleted) -
all route records referencing that datapath will be marked as "updated"
by the IDL in ovn-controller -> assuming forwarding rules for routes are
created by ovn-northd through logical flows, ovn-controller can probably
ignore the Route IDL updates (ovsdb_idl_omit_alert() for all columns of
the Route table).

- northd updates a port binding (e.g., a network is added or deleted
to/from the LRP config) - all route records referencing that port
binding will be marked as "updated" by the IDL in ovn-controller -> same
as above, ovn-controller can probably ignore these updates

Regards,
Dumitru

> Thanks a lot
> Felix
> 
>>
>> Thanks,
>> Dumitru
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On 12/9/24 11:09, Dumitru Ceara wrote:
> On 12/9/24 9:05 AM, Felix Huettner wrote:
>> On Thu, Dec 05, 2024 at 03:23:23PM +0100, Dumitru Ceara wrote:
>>> On 12/5/24 1:59 PM, Felix Huettner wrote:
>>>> On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote:
>>>>> On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
>>>>>> The Route table will be used in the future to coordinate routing
>>>>>> information between northd and ovn-controller.
>>>>>> Northd will insert routes that should be advertised to the outside
>>>>>> fabric.
>>>>>> Ovn-controller will insert routes that have been learned from the
>>>>>> outside fabric.
>>>>>>
>>>>>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
>>>>>> ---
>>>>>
>>>>> Hi Felix,
>>>>>
>>>>> I have a few minor comments, otherwise, this looks ok to me.
>>>>
>>>> Hi Dimitru,
>>>>
>>>> thanks a lot for the review.
>>>>
>>>>>
>>>>>>  ovn-sb.ovsschema | 29 ++++++++++++++++--
>>>>>>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  2 files changed, 107 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>>>>>> index 73abf2c8d..76d32197d 100644
>>>>>> --- a/ovn-sb.ovsschema
>>>>>> +++ b/ovn-sb.ovsschema
>>>>>> @@ -1,7 +1,7 @@
>>>>>>  {
>>>>>>      "name": "OVN_Southbound",
>>>>>> -    "version": "20.37.0",
>>>>>> -    "cksum": "1950136776 31493",
>>>>>> +    "version": "20.38.0",
>>>>>> +    "cksum": "550338719 32889",
>>>>>>      "tables": {
>>>>>>          "SB_Global": {
>>>>>>              "columns": {
>>>>>> @@ -617,6 +617,31 @@
>>>>>>                      "type": {"key": "string", "value": "string",
>>>>>>                               "min": 0, "max": "unlimited"}}},
>>>>>>              "indexes": [["chassis"]],
>>>>>> +            "isRoot": true},
>>>>>> +        "Route": {
>>>>>> +            "columns": {
>>>>>> +                "datapath":
>>>>>> +                    {"type": {"key": {"type": "uuid",
>>>>>> +                                      "refTable": "Datapath_Binding"}}},
>>>>>> +                "logical_port": {"type": {"key": {"type": "uuid",
>>>>>> +                                                  "refTable": "Port_Binding",
>>>>>> +                                                  "refType": "weak"}}},
>>>>>
>>>>> It's probably fine to keep this as a strong reference, right?
>>>>>
>>>>>> +                "ip_prefix": {"type": "string"},
>>>>>> +                "nexthop": {"type": "string"},
>>>>>> +                "tracked_port": {"type": {"key": {"type": "uuid",
>>>>>> +                                                  "refTable": "Port_Binding",
>>>>>> +                                                  "refType": "weak"},
>>>>>
>>>>> Same here, I guess.
>>>>
>>>> Yes, will both be changed in v3.
>>>>
>>>>>
>>>>>> +                                          "min": 0,
>>>>>> +                                          "max": 1}},
>>>>>> +                "type": {"type": {"key": {"type": "string",
>>>>>> +                                          "enum": ["set", ["advertise",
>>>>>> +                                                           "receive"]]},
>>>>>> +                                    "min": 1, "max": 1}},
>>>>>> +                "external_ids": {
>>>>>> +                    "type": {"key": "string", "value": "string",
>>>>>> +                             "min": 0, "max": "unlimited"}}},
>>>>>> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
>>>>>> +                         "type"]],
>>>>>>              "isRoot": true}
>>>>>
>>>>> Do we really need this table to be root?
>>>>
>>>> As i understood the isRoot flag it keeps the records around even if
>>>> there is no incoming reference to any of the rows. As there are no
>>>> incoming references to the Route table at all we need this to be true.
>>>>
>>>
>>> Ah, you're right about that.  But shouldn't actually routes be referred
>>> by Datapath_Binding instead of routes pointing to Datapath_binding?
>>> Maybe that's a better schema definition?  In that case isRoot can be
>>> false for the Routes table.
>>>
>>> It doesn't really make sense to have route records for routers that
>>> don't exist in the DB anymore.
>>
>> Hi Dumitru,
>>
> 
> Hi Felix,
> 
>> I tried this out and noticed something that might stand in the way of
>> this change. However it is quite likely that it might be just my limited
>> understanding of ovsdb :)
>>
>> I changed logical_port and tracked_port above to be strong references.
>> Also i removed the datapath key and added in Datapath_Binding a
>> reference to Routes. Additionally i set isRoot to false.
>>
>> If we now assume we have a working setup with LR, LRPs and some created
>> routes by northd.
>> Now if we want to delete a LRP we need to:
>> * Delete the Port_Binding in the southbound
>> * Delete all Routes referencing the Port_Binding
>> * Remove these routes from the list in the Datapath_Binding
>>
>> The problem i encountered is that this creates a "referential integrity
>> violation" because Routes are still referencing the deleted
>> Port_Bindings.
>>
>> As i understood it this is caused by:
>> 1. the ovsdb idl not actually sending deletes for nonRoot tables
>>    (https://github.com/openvswitch/ovs/blob/main/lib/ovsdb-idl.c#L3290)
> 
> I wasn't aware of this, thanks for pointing it out (CC Ilya).
> Introduced a long time ago by:
> https://github.com/openvswitch/ovs/commit/dcd1dbc
> 
> This is actually a bigger problem than you described here.  These
> records are to be created/deleted by ovn-controller.  But ovn-controller
> effectively can't delete any non-root Route records if they're still
> referenced by Datapath_Binding.

In general, there is no point sending deletions for non-root tables.
Because they will either be garbage collected anyway, or your transaction
had a referential integrity violation in the first place and wouldn't
go through anyway.

I'll take a closer look at what the database server is doing...

> 
>> 2. the ovsdb server first checking referential integrity and afterwards
>>    deleting orphaned rows
>>    (https://github.com/openvswitch/ovs/blob/main/ovsdb/transaction.c#L1099-L1109)
>>
>> So from a northd perspective it sends the (from my understanding)
>> correct changes:
>> 1. Deleting the Port_Binding
>> 2. Removing any refernce to the Route entries
>> It does not send a delete for the Route entries, as that is a result of
>> the references.
>>
>> However due to the order of ovsdb server evaluating these things we get
>> an "integrity violation".
>>
>> I am not sure how to best address this issue.
>> I guess having multiple transactions in northd would work, but is quite
>> complex and i don't think we do this anywhere else.
> 
> I don't think we should do that, it sounds too risky.
> 
>> Alternatively we would need to remove one of the problematic parts from
>> the schema, so we could:
>> * Define Route as a root table
>> * Remove the reference from logical_port/tracked_port to Port_Binding
>> * Maybe changing the references of logical_port/tracked_port to be weak
>>   might also work.
>>
>> I am quite unsure what the most appropriate solution is or if i just
>> missunderstood something somewhere.
>> So any ideas would be appreciated.
>>
> 
> It seems to me that the only reasonable way forward is what you had in
> your patch originally:
> 
> - Route table as "root"
> - Route records with (strong) references to port bindings
> - Route records with (strong) references to datapath_bindings
> 
> Which means whenever:
> - a route is added by ovn-controller, northd only gets IDL updates for
> that new route record
> 
> - a route is deleted by ovn-controller, northd only gets IDL updates for
> the removed route record
> 
> - northd needs to delete a port binding it must also delete all route
> records referencing that port binding - in the same transaction.
> 
> - northd needs to delete a datapath binding it must also delete all
> route records referencing that datapath binding - in the same transaction
> 
> - northd updates a datapath binding (e.g., a port is added or deleted) -
> all route records referencing that datapath will be marked as "updated"
> by the IDL in ovn-controller -> assuming forwarding rules for routes are
> created by ovn-northd through logical flows, ovn-controller can probably
> ignore the Route IDL updates (ovsdb_idl_omit_alert() for all columns of
> the Route table).
> 
> - northd updates a port binding (e.g., a network is added or deleted
> to/from the LRP config) - all route records referencing that port
> binding will be marked as "updated" by the IDL in ovn-controller -> same
> as above, ovn-controller can probably ignore these updates
> 
> Regards,
> Dumitru
> 
>> Thanks a lot
>> Felix
>>
>>>
>>> Thanks,
>>> Dumitru
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>

On 12/9/24 11:54 AM, Ilya Maximets wrote:
> On 12/9/24 11:09, Dumitru Ceara wrote:
>> On 12/9/24 9:05 AM, Felix Huettner wrote:
>>> On Thu, Dec 05, 2024 at 03:23:23PM +0100, Dumitru Ceara wrote:
>>>> On 12/5/24 1:59 PM, Felix Huettner wrote:
>>>>> On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote:
>>>>>> On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
>>>>>>> The Route table will be used in the future to coordinate routing
>>>>>>> information between northd and ovn-controller.
>>>>>>> Northd will insert routes that should be advertised to the outside
>>>>>>> fabric.
>>>>>>> Ovn-controller will insert routes that have been learned from the
>>>>>>> outside fabric.
>>>>>>>
>>>>>>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
>>>>>>> ---
>>>>>>
>>>>>> Hi Felix,
>>>>>>
>>>>>> I have a few minor comments, otherwise, this looks ok to me.
>>>>>
>>>>> Hi Dimitru,
>>>>>
>>>>> thanks a lot for the review.
>>>>>
>>>>>>
>>>>>>>  ovn-sb.ovsschema | 29 ++++++++++++++++--
>>>>>>>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  2 files changed, 107 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>>>>>>> index 73abf2c8d..76d32197d 100644
>>>>>>> --- a/ovn-sb.ovsschema
>>>>>>> +++ b/ovn-sb.ovsschema
>>>>>>> @@ -1,7 +1,7 @@
>>>>>>>  {
>>>>>>>      "name": "OVN_Southbound",
>>>>>>> -    "version": "20.37.0",
>>>>>>> -    "cksum": "1950136776 31493",
>>>>>>> +    "version": "20.38.0",
>>>>>>> +    "cksum": "550338719 32889",
>>>>>>>      "tables": {
>>>>>>>          "SB_Global": {
>>>>>>>              "columns": {
>>>>>>> @@ -617,6 +617,31 @@
>>>>>>>                      "type": {"key": "string", "value": "string",
>>>>>>>                               "min": 0, "max": "unlimited"}}},
>>>>>>>              "indexes": [["chassis"]],
>>>>>>> +            "isRoot": true},
>>>>>>> +        "Route": {
>>>>>>> +            "columns": {
>>>>>>> +                "datapath":
>>>>>>> +                    {"type": {"key": {"type": "uuid",
>>>>>>> +                                      "refTable": "Datapath_Binding"}}},
>>>>>>> +                "logical_port": {"type": {"key": {"type": "uuid",
>>>>>>> +                                                  "refTable": "Port_Binding",
>>>>>>> +                                                  "refType": "weak"}}},
>>>>>>
>>>>>> It's probably fine to keep this as a strong reference, right?
>>>>>>
>>>>>>> +                "ip_prefix": {"type": "string"},
>>>>>>> +                "nexthop": {"type": "string"},
>>>>>>> +                "tracked_port": {"type": {"key": {"type": "uuid",
>>>>>>> +                                                  "refTable": "Port_Binding",
>>>>>>> +                                                  "refType": "weak"},
>>>>>>
>>>>>> Same here, I guess.
>>>>>
>>>>> Yes, will both be changed in v3.
>>>>>
>>>>>>
>>>>>>> +                                          "min": 0,
>>>>>>> +                                          "max": 1}},
>>>>>>> +                "type": {"type": {"key": {"type": "string",
>>>>>>> +                                          "enum": ["set", ["advertise",
>>>>>>> +                                                           "receive"]]},
>>>>>>> +                                    "min": 1, "max": 1}},
>>>>>>> +                "external_ids": {
>>>>>>> +                    "type": {"key": "string", "value": "string",
>>>>>>> +                             "min": 0, "max": "unlimited"}}},
>>>>>>> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
>>>>>>> +                         "type"]],
>>>>>>>              "isRoot": true}
>>>>>>
>>>>>> Do we really need this table to be root?
>>>>>
>>>>> As i understood the isRoot flag it keeps the records around even if
>>>>> there is no incoming reference to any of the rows. As there are no
>>>>> incoming references to the Route table at all we need this to be true.
>>>>>
>>>>
>>>> Ah, you're right about that.  But shouldn't actually routes be referred
>>>> by Datapath_Binding instead of routes pointing to Datapath_binding?
>>>> Maybe that's a better schema definition?  In that case isRoot can be
>>>> false for the Routes table.
>>>>
>>>> It doesn't really make sense to have route records for routers that
>>>> don't exist in the DB anymore.
>>>
>>> Hi Dumitru,
>>>
>>
>> Hi Felix,
>>
>>> I tried this out and noticed something that might stand in the way of
>>> this change. However it is quite likely that it might be just my limited
>>> understanding of ovsdb :)
>>>
>>> I changed logical_port and tracked_port above to be strong references.
>>> Also i removed the datapath key and added in Datapath_Binding a
>>> reference to Routes. Additionally i set isRoot to false.
>>>
>>> If we now assume we have a working setup with LR, LRPs and some created
>>> routes by northd.
>>> Now if we want to delete a LRP we need to:
>>> * Delete the Port_Binding in the southbound
>>> * Delete all Routes referencing the Port_Binding
>>> * Remove these routes from the list in the Datapath_Binding
>>>
>>> The problem i encountered is that this creates a "referential integrity
>>> violation" because Routes are still referencing the deleted
>>> Port_Bindings.
>>>
>>> As i understood it this is caused by:
>>> 1. the ovsdb idl not actually sending deletes for nonRoot tables
>>>    (https://github.com/openvswitch/ovs/blob/main/lib/ovsdb-idl.c#L3290)
>>
>> I wasn't aware of this, thanks for pointing it out (CC Ilya).
>> Introduced a long time ago by:
>> https://github.com/openvswitch/ovs/commit/dcd1dbc
>>
>> This is actually a bigger problem than you described here.  These
>> records are to be created/deleted by ovn-controller.  But ovn-controller
>> effectively can't delete any non-root Route records if they're still
>> referenced by Datapath_Binding.
> 
> In general, there is no point sending deletions for non-root tables.
> Because they will either be garbage collected anyway, or your transaction
> had a referential integrity violation in the first place and wouldn't
> go through anyway.
>

I'm not sure I follow completely.  For clarity, in this case, the
behavior I was hoping for is:

- allow explicit deletes of isRoot=false records, i.e., Route records
when ovn-controller (the one who created them) detects they should be
removed (individually, regardless if they're referred or not).
- support automatic garbage collection of routes when the router
(Datapath_Binding) goes away.

> I'll take a closer look at what the database server is doing...
> 
>>
>>> 2. the ovsdb server first checking referential integrity and afterwards
>>>    deleting orphaned rows
>>>    (https://github.com/openvswitch/ovs/blob/main/ovsdb/transaction.c#L1099-L1109)
>>>
>>> So from a northd perspective it sends the (from my understanding)
>>> correct changes:
>>> 1. Deleting the Port_Binding
>>> 2. Removing any refernce to the Route entries
>>> It does not send a delete for the Route entries, as that is a result of
>>> the references.
>>>
>>> However due to the order of ovsdb server evaluating these things we get
>>> an "integrity violation".
>>>
>>> I am not sure how to best address this issue.
>>> I guess having multiple transactions in northd would work, but is quite
>>> complex and i don't think we do this anywhere else.
>>
>> I don't think we should do that, it sounds too risky.
>>
>>> Alternatively we would need to remove one of the problematic parts from
>>> the schema, so we could:
>>> * Define Route as a root table
>>> * Remove the reference from logical_port/tracked_port to Port_Binding
>>> * Maybe changing the references of logical_port/tracked_port to be weak
>>>   might also work.
>>>
>>> I am quite unsure what the most appropriate solution is or if i just
>>> missunderstood something somewhere.
>>> So any ideas would be appreciated.
>>>
>>
>> It seems to me that the only reasonable way forward is what you had in
>> your patch originally:
>>
>> - Route table as "root"
>> - Route records with (strong) references to port bindings
>> - Route records with (strong) references to datapath_bindings
>>
>> Which means whenever:
>> - a route is added by ovn-controller, northd only gets IDL updates for
>> that new route record
>>
>> - a route is deleted by ovn-controller, northd only gets IDL updates for
>> the removed route record
>>
>> - northd needs to delete a port binding it must also delete all route
>> records referencing that port binding - in the same transaction.
>>
>> - northd needs to delete a datapath binding it must also delete all
>> route records referencing that datapath binding - in the same transaction
>>
>> - northd updates a datapath binding (e.g., a port is added or deleted) -
>> all route records referencing that datapath will be marked as "updated"
>> by the IDL in ovn-controller -> assuming forwarding rules for routes are
>> created by ovn-northd through logical flows, ovn-controller can probably
>> ignore the Route IDL updates (ovsdb_idl_omit_alert() for all columns of
>> the Route table).
>>
>> - northd updates a port binding (e.g., a network is added or deleted
>> to/from the LRP config) - all route records referencing that port
>> binding will be marked as "updated" by the IDL in ovn-controller -> same
>> as above, ovn-controller can probably ignore these updates
>>
>> Regards,
>> Dumitru
>>
>>> Thanks a lot
>>> Felix
>>>
>>>>
>>>> Thanks,
>>>> Dumitru
>>>>
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
>

On 12/9/24 13:05, Dumitru Ceara wrote:
> On 12/9/24 11:54 AM, Ilya Maximets wrote:
>> On 12/9/24 11:09, Dumitru Ceara wrote:
>>> On 12/9/24 9:05 AM, Felix Huettner wrote:
>>>> On Thu, Dec 05, 2024 at 03:23:23PM +0100, Dumitru Ceara wrote:
>>>>> On 12/5/24 1:59 PM, Felix Huettner wrote:
>>>>>> On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote:
>>>>>>> On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
>>>>>>>> The Route table will be used in the future to coordinate routing
>>>>>>>> information between northd and ovn-controller.
>>>>>>>> Northd will insert routes that should be advertised to the outside
>>>>>>>> fabric.
>>>>>>>> Ovn-controller will insert routes that have been learned from the
>>>>>>>> outside fabric.
>>>>>>>>
>>>>>>>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
>>>>>>>> ---
>>>>>>>
>>>>>>> Hi Felix,
>>>>>>>
>>>>>>> I have a few minor comments, otherwise, this looks ok to me.
>>>>>>
>>>>>> Hi Dimitru,
>>>>>>
>>>>>> thanks a lot for the review.
>>>>>>
>>>>>>>
>>>>>>>>  ovn-sb.ovsschema | 29 ++++++++++++++++--
>>>>>>>>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  2 files changed, 107 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>>>>>>>> index 73abf2c8d..76d32197d 100644
>>>>>>>> --- a/ovn-sb.ovsschema
>>>>>>>> +++ b/ovn-sb.ovsschema
>>>>>>>> @@ -1,7 +1,7 @@
>>>>>>>>  {
>>>>>>>>      "name": "OVN_Southbound",
>>>>>>>> -    "version": "20.37.0",
>>>>>>>> -    "cksum": "1950136776 31493",
>>>>>>>> +    "version": "20.38.0",
>>>>>>>> +    "cksum": "550338719 32889",
>>>>>>>>      "tables": {
>>>>>>>>          "SB_Global": {
>>>>>>>>              "columns": {
>>>>>>>> @@ -617,6 +617,31 @@
>>>>>>>>                      "type": {"key": "string", "value": "string",
>>>>>>>>                               "min": 0, "max": "unlimited"}}},
>>>>>>>>              "indexes": [["chassis"]],
>>>>>>>> +            "isRoot": true},
>>>>>>>> +        "Route": {
>>>>>>>> +            "columns": {
>>>>>>>> +                "datapath":
>>>>>>>> +                    {"type": {"key": {"type": "uuid",
>>>>>>>> +                                      "refTable": "Datapath_Binding"}}},
>>>>>>>> +                "logical_port": {"type": {"key": {"type": "uuid",
>>>>>>>> +                                                  "refTable": "Port_Binding",
>>>>>>>> +                                                  "refType": "weak"}}},
>>>>>>>
>>>>>>> It's probably fine to keep this as a strong reference, right?
>>>>>>>
>>>>>>>> +                "ip_prefix": {"type": "string"},
>>>>>>>> +                "nexthop": {"type": "string"},
>>>>>>>> +                "tracked_port": {"type": {"key": {"type": "uuid",
>>>>>>>> +                                                  "refTable": "Port_Binding",
>>>>>>>> +                                                  "refType": "weak"},
>>>>>>>
>>>>>>> Same here, I guess.
>>>>>>
>>>>>> Yes, will both be changed in v3.
>>>>>>
>>>>>>>
>>>>>>>> +                                          "min": 0,
>>>>>>>> +                                          "max": 1}},
>>>>>>>> +                "type": {"type": {"key": {"type": "string",
>>>>>>>> +                                          "enum": ["set", ["advertise",
>>>>>>>> +                                                           "receive"]]},
>>>>>>>> +                                    "min": 1, "max": 1}},
>>>>>>>> +                "external_ids": {
>>>>>>>> +                    "type": {"key": "string", "value": "string",
>>>>>>>> +                             "min": 0, "max": "unlimited"}}},
>>>>>>>> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
>>>>>>>> +                         "type"]],
>>>>>>>>              "isRoot": true}
>>>>>>>
>>>>>>> Do we really need this table to be root?
>>>>>>
>>>>>> As i understood the isRoot flag it keeps the records around even if
>>>>>> there is no incoming reference to any of the rows. As there are no
>>>>>> incoming references to the Route table at all we need this to be true.
>>>>>>
>>>>>
>>>>> Ah, you're right about that.  But shouldn't actually routes be referred
>>>>> by Datapath_Binding instead of routes pointing to Datapath_binding?
>>>>> Maybe that's a better schema definition?  In that case isRoot can be
>>>>> false for the Routes table.
>>>>>
>>>>> It doesn't really make sense to have route records for routers that
>>>>> don't exist in the DB anymore.
>>>>
>>>> Hi Dumitru,
>>>>
>>>
>>> Hi Felix,
>>>
>>>> I tried this out and noticed something that might stand in the way of
>>>> this change. However it is quite likely that it might be just my limited
>>>> understanding of ovsdb :)
>>>>
>>>> I changed logical_port and tracked_port above to be strong references.
>>>> Also i removed the datapath key and added in Datapath_Binding a
>>>> reference to Routes. Additionally i set isRoot to false.
>>>>
>>>> If we now assume we have a working setup with LR, LRPs and some created
>>>> routes by northd.
>>>> Now if we want to delete a LRP we need to:
>>>> * Delete the Port_Binding in the southbound
>>>> * Delete all Routes referencing the Port_Binding
>>>> * Remove these routes from the list in the Datapath_Binding
>>>>
>>>> The problem i encountered is that this creates a "referential integrity
>>>> violation" because Routes are still referencing the deleted
>>>> Port_Bindings.
>>>>
>>>> As i understood it this is caused by:
>>>> 1. the ovsdb idl not actually sending deletes for nonRoot tables
>>>>    (https://github.com/openvswitch/ovs/blob/main/lib/ovsdb-idl.c#L3290)
>>>
>>> I wasn't aware of this, thanks for pointing it out (CC Ilya).
>>> Introduced a long time ago by:
>>> https://github.com/openvswitch/ovs/commit/dcd1dbc
>>>
>>> This is actually a bigger problem than you described here.  These
>>> records are to be created/deleted by ovn-controller.  But ovn-controller
>>> effectively can't delete any non-root Route records if they're still
>>> referenced by Datapath_Binding.
>>
>> In general, there is no point sending deletions for non-root tables.
>> Because they will either be garbage collected anyway, or your transaction
>> had a referential integrity violation in the first place and wouldn't
>> go through anyway.
>>
> 
> I'm not sure I follow completely.  For clarity, in this case, the
> behavior I was hoping for is:
> 
> - allow explicit deletes of isRoot=false records, i.e., Route records
> when ovn-controller (the one who created them) detects they should be
> removed (individually, regardless if they're referred or not).

You can't delete a record if there is a strong reference to it,
as that will violate referential integrity.
And if there is no reference, then you don't need to remove it,
because it will be just garbage-collected.

So, there should be no scenario where you actually need to explicitly
delete a record from a non-root table.

> - support automatic garbage collection of routes when the router
> (Datapath_Binding) goes away.

This should already work.

The problem is that those garbage-cllected records hold strong
references to other table (port bindings) and that, as Felix says,
breaks referential integrity check.  That is likely a bug in
ovsdb-server, unless we document somewhere that non-root tables
can't hold strong references to root ones (I don't remember if
that's the case).  That part needs checking and potentially fixing.

Best regards, Ilya Maximets.

On 12/9/24 1:27 PM, Ilya Maximets wrote:
> On 12/9/24 13:05, Dumitru Ceara wrote:
>> On 12/9/24 11:54 AM, Ilya Maximets wrote:
>>> On 12/9/24 11:09, Dumitru Ceara wrote:
>>>> On 12/9/24 9:05 AM, Felix Huettner wrote:
>>>>> On Thu, Dec 05, 2024 at 03:23:23PM +0100, Dumitru Ceara wrote:
>>>>>> On 12/5/24 1:59 PM, Felix Huettner wrote:
>>>>>>> On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote:
>>>>>>>> On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
>>>>>>>>> The Route table will be used in the future to coordinate routing
>>>>>>>>> information between northd and ovn-controller.
>>>>>>>>> Northd will insert routes that should be advertised to the outside
>>>>>>>>> fabric.
>>>>>>>>> Ovn-controller will insert routes that have been learned from the
>>>>>>>>> outside fabric.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
>>>>>>>>> ---
>>>>>>>>
>>>>>>>> Hi Felix,
>>>>>>>>
>>>>>>>> I have a few minor comments, otherwise, this looks ok to me.
>>>>>>>
>>>>>>> Hi Dimitru,
>>>>>>>
>>>>>>> thanks a lot for the review.
>>>>>>>
>>>>>>>>
>>>>>>>>>  ovn-sb.ovsschema | 29 ++++++++++++++++--
>>>>>>>>>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>  2 files changed, 107 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>>>>>>>>> index 73abf2c8d..76d32197d 100644
>>>>>>>>> --- a/ovn-sb.ovsschema
>>>>>>>>> +++ b/ovn-sb.ovsschema
>>>>>>>>> @@ -1,7 +1,7 @@
>>>>>>>>>  {
>>>>>>>>>      "name": "OVN_Southbound",
>>>>>>>>> -    "version": "20.37.0",
>>>>>>>>> -    "cksum": "1950136776 31493",
>>>>>>>>> +    "version": "20.38.0",
>>>>>>>>> +    "cksum": "550338719 32889",
>>>>>>>>>      "tables": {
>>>>>>>>>          "SB_Global": {
>>>>>>>>>              "columns": {
>>>>>>>>> @@ -617,6 +617,31 @@
>>>>>>>>>                      "type": {"key": "string", "value": "string",
>>>>>>>>>                               "min": 0, "max": "unlimited"}}},
>>>>>>>>>              "indexes": [["chassis"]],
>>>>>>>>> +            "isRoot": true},
>>>>>>>>> +        "Route": {
>>>>>>>>> +            "columns": {
>>>>>>>>> +                "datapath":
>>>>>>>>> +                    {"type": {"key": {"type": "uuid",
>>>>>>>>> +                                      "refTable": "Datapath_Binding"}}},
>>>>>>>>> +                "logical_port": {"type": {"key": {"type": "uuid",
>>>>>>>>> +                                                  "refTable": "Port_Binding",
>>>>>>>>> +                                                  "refType": "weak"}}},
>>>>>>>>
>>>>>>>> It's probably fine to keep this as a strong reference, right?
>>>>>>>>
>>>>>>>>> +                "ip_prefix": {"type": "string"},
>>>>>>>>> +                "nexthop": {"type": "string"},
>>>>>>>>> +                "tracked_port": {"type": {"key": {"type": "uuid",
>>>>>>>>> +                                                  "refTable": "Port_Binding",
>>>>>>>>> +                                                  "refType": "weak"},
>>>>>>>>
>>>>>>>> Same here, I guess.
>>>>>>>
>>>>>>> Yes, will both be changed in v3.
>>>>>>>
>>>>>>>>
>>>>>>>>> +                                          "min": 0,
>>>>>>>>> +                                          "max": 1}},
>>>>>>>>> +                "type": {"type": {"key": {"type": "string",
>>>>>>>>> +                                          "enum": ["set", ["advertise",
>>>>>>>>> +                                                           "receive"]]},
>>>>>>>>> +                                    "min": 1, "max": 1}},
>>>>>>>>> +                "external_ids": {
>>>>>>>>> +                    "type": {"key": "string", "value": "string",
>>>>>>>>> +                             "min": 0, "max": "unlimited"}}},
>>>>>>>>> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
>>>>>>>>> +                         "type"]],
>>>>>>>>>              "isRoot": true}
>>>>>>>>
>>>>>>>> Do we really need this table to be root?
>>>>>>>
>>>>>>> As i understood the isRoot flag it keeps the records around even if
>>>>>>> there is no incoming reference to any of the rows. As there are no
>>>>>>> incoming references to the Route table at all we need this to be true.
>>>>>>>
>>>>>>
>>>>>> Ah, you're right about that.  But shouldn't actually routes be referred
>>>>>> by Datapath_Binding instead of routes pointing to Datapath_binding?
>>>>>> Maybe that's a better schema definition?  In that case isRoot can be
>>>>>> false for the Routes table.
>>>>>>
>>>>>> It doesn't really make sense to have route records for routers that
>>>>>> don't exist in the DB anymore.
>>>>>
>>>>> Hi Dumitru,
>>>>>
>>>>
>>>> Hi Felix,
>>>>
>>>>> I tried this out and noticed something that might stand in the way of
>>>>> this change. However it is quite likely that it might be just my limited
>>>>> understanding of ovsdb :)
>>>>>
>>>>> I changed logical_port and tracked_port above to be strong references.
>>>>> Also i removed the datapath key and added in Datapath_Binding a
>>>>> reference to Routes. Additionally i set isRoot to false.
>>>>>
>>>>> If we now assume we have a working setup with LR, LRPs and some created
>>>>> routes by northd.
>>>>> Now if we want to delete a LRP we need to:
>>>>> * Delete the Port_Binding in the southbound
>>>>> * Delete all Routes referencing the Port_Binding
>>>>> * Remove these routes from the list in the Datapath_Binding
>>>>>
>>>>> The problem i encountered is that this creates a "referential integrity
>>>>> violation" because Routes are still referencing the deleted
>>>>> Port_Bindings.
>>>>>
>>>>> As i understood it this is caused by:
>>>>> 1. the ovsdb idl not actually sending deletes for nonRoot tables
>>>>>    (https://github.com/openvswitch/ovs/blob/main/lib/ovsdb-idl.c#L3290)
>>>>
>>>> I wasn't aware of this, thanks for pointing it out (CC Ilya).
>>>> Introduced a long time ago by:
>>>> https://github.com/openvswitch/ovs/commit/dcd1dbc
>>>>
>>>> This is actually a bigger problem than you described here.  These
>>>> records are to be created/deleted by ovn-controller.  But ovn-controller
>>>> effectively can't delete any non-root Route records if they're still
>>>> referenced by Datapath_Binding.
>>>
>>> In general, there is no point sending deletions for non-root tables.
>>> Because they will either be garbage collected anyway, or your transaction
>>> had a referential integrity violation in the first place and wouldn't
>>> go through anyway.
>>>
>>
>> I'm not sure I follow completely.  For clarity, in this case, the
>> behavior I was hoping for is:
>>
>> - allow explicit deletes of isRoot=false records, i.e., Route records
>> when ovn-controller (the one who created them) detects they should be
>> removed (individually, regardless if they're referred or not).
> 
> You can't delete a record if there is a strong reference to it,
> as that will violate referential integrity.
> And if there is no reference, then you don't need to remove it,
> because it will be just garbage-collected.
> 
> So, there should be no scenario where you actually need to explicitly
> delete a record from a non-root table.
> 

Ack I see, we should just remove the reference to the non-root table
record, but in this case that record has a reference to a different
table too (e.g., Route points to Port_Binding in the OVN case).  If that
reference target is also removed in the same transaction then we hit the
referential integrity violation problem that Felix pointed out on the
server side - integrity check happening before garbage collection.

>> - support automatic garbage collection of routes when the router
>> (Datapath_Binding) goes away.
> 
> This should already work.
> 
> The problem is that those garbage-cllected records hold strong
> references to other table (port bindings) and that, as Felix says,
> breaks referential integrity check.  That is likely a bug in
> ovsdb-server, unless we document somewhere that non-root tables
> can't hold strong references to root ones (I don't remember if
> that's the case).  That part needs checking and potentially fixing.
> 
> Best regards, Ilya Maximets.
> 

Thanks,
Dumitru

On Mon, Dec 09, 2024 at 02:12:14PM +0100, Dumitru Ceara wrote:
> On 12/9/24 1:27 PM, Ilya Maximets wrote:
> > On 12/9/24 13:05, Dumitru Ceara wrote:
> >> On 12/9/24 11:54 AM, Ilya Maximets wrote:
> >>> On 12/9/24 11:09, Dumitru Ceara wrote:
> >>>> On 12/9/24 9:05 AM, Felix Huettner wrote:
> >>>>> On Thu, Dec 05, 2024 at 03:23:23PM +0100, Dumitru Ceara wrote:
> >>>>>> On 12/5/24 1:59 PM, Felix Huettner wrote:
> >>>>>>> On Wed, Dec 04, 2024 at 02:18:31PM +0100, Dumitru Ceara wrote:
> >>>>>>>> On 12/3/24 3:58 PM, Felix Huettner via dev wrote:
> >>>>>>>>> The Route table will be used in the future to coordinate routing
> >>>>>>>>> information between northd and ovn-controller.
> >>>>>>>>> Northd will insert routes that should be advertised to the outside
> >>>>>>>>> fabric.
> >>>>>>>>> Ovn-controller will insert routes that have been learned from the
> >>>>>>>>> outside fabric.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> >>>>>>>>> ---
> >>>>>>>>
> >>>>>>>> Hi Felix,
> >>>>>>>>
> >>>>>>>> I have a few minor comments, otherwise, this looks ok to me.
> >>>>>>>
> >>>>>>> Hi Dimitru,
> >>>>>>>
> >>>>>>> thanks a lot for the review.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>>  ovn-sb.ovsschema | 29 ++++++++++++++++--
> >>>>>>>>>  ovn-sb.xml       | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>  2 files changed, 107 insertions(+), 2 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> >>>>>>>>> index 73abf2c8d..76d32197d 100644
> >>>>>>>>> --- a/ovn-sb.ovsschema
> >>>>>>>>> +++ b/ovn-sb.ovsschema
> >>>>>>>>> @@ -1,7 +1,7 @@
> >>>>>>>>>  {
> >>>>>>>>>      "name": "OVN_Southbound",
> >>>>>>>>> -    "version": "20.37.0",
> >>>>>>>>> -    "cksum": "1950136776 31493",
> >>>>>>>>> +    "version": "20.38.0",
> >>>>>>>>> +    "cksum": "550338719 32889",
> >>>>>>>>>      "tables": {
> >>>>>>>>>          "SB_Global": {
> >>>>>>>>>              "columns": {
> >>>>>>>>> @@ -617,6 +617,31 @@
> >>>>>>>>>                      "type": {"key": "string", "value": "string",
> >>>>>>>>>                               "min": 0, "max": "unlimited"}}},
> >>>>>>>>>              "indexes": [["chassis"]],
> >>>>>>>>> +            "isRoot": true},
> >>>>>>>>> +        "Route": {
> >>>>>>>>> +            "columns": {
> >>>>>>>>> +                "datapath":
> >>>>>>>>> +                    {"type": {"key": {"type": "uuid",
> >>>>>>>>> +                                      "refTable": "Datapath_Binding"}}},
> >>>>>>>>> +                "logical_port": {"type": {"key": {"type": "uuid",
> >>>>>>>>> +                                                  "refTable": "Port_Binding",
> >>>>>>>>> +                                                  "refType": "weak"}}},
> >>>>>>>>
> >>>>>>>> It's probably fine to keep this as a strong reference, right?
> >>>>>>>>
> >>>>>>>>> +                "ip_prefix": {"type": "string"},
> >>>>>>>>> +                "nexthop": {"type": "string"},
> >>>>>>>>> +                "tracked_port": {"type": {"key": {"type": "uuid",
> >>>>>>>>> +                                                  "refTable": "Port_Binding",
> >>>>>>>>> +                                                  "refType": "weak"},
> >>>>>>>>
> >>>>>>>> Same here, I guess.
> >>>>>>>
> >>>>>>> Yes, will both be changed in v3.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> +                                          "min": 0,
> >>>>>>>>> +                                          "max": 1}},
> >>>>>>>>> +                "type": {"type": {"key": {"type": "string",
> >>>>>>>>> +                                          "enum": ["set", ["advertise",
> >>>>>>>>> +                                                           "receive"]]},
> >>>>>>>>> +                                    "min": 1, "max": 1}},
> >>>>>>>>> +                "external_ids": {
> >>>>>>>>> +                    "type": {"key": "string", "value": "string",
> >>>>>>>>> +                             "min": 0, "max": "unlimited"}}},
> >>>>>>>>> +            "indexes": [["datapath", "logical_port", "ip_prefix", "nexthop",
> >>>>>>>>> +                         "type"]],
> >>>>>>>>>              "isRoot": true}
> >>>>>>>>
> >>>>>>>> Do we really need this table to be root?
> >>>>>>>
> >>>>>>> As i understood the isRoot flag it keeps the records around even if
> >>>>>>> there is no incoming reference to any of the rows. As there are no
> >>>>>>> incoming references to the Route table at all we need this to be true.
> >>>>>>>
> >>>>>>
> >>>>>> Ah, you're right about that.  But shouldn't actually routes be referred
> >>>>>> by Datapath_Binding instead of routes pointing to Datapath_binding?
> >>>>>> Maybe that's a better schema definition?  In that case isRoot can be
> >>>>>> false for the Routes table.
> >>>>>>
> >>>>>> It doesn't really make sense to have route records for routers that
> >>>>>> don't exist in the DB anymore.
> >>>>>
> >>>>> Hi Dumitru,
> >>>>>
> >>>>
> >>>> Hi Felix,
> >>>>
> >>>>> I tried this out and noticed something that might stand in the way of
> >>>>> this change. However it is quite likely that it might be just my limited
> >>>>> understanding of ovsdb :)
> >>>>>
> >>>>> I changed logical_port and tracked_port above to be strong references.
> >>>>> Also i removed the datapath key and added in Datapath_Binding a
> >>>>> reference to Routes. Additionally i set isRoot to false.
> >>>>>
> >>>>> If we now assume we have a working setup with LR, LRPs and some created
> >>>>> routes by northd.
> >>>>> Now if we want to delete a LRP we need to:
> >>>>> * Delete the Port_Binding in the southbound
> >>>>> * Delete all Routes referencing the Port_Binding
> >>>>> * Remove these routes from the list in the Datapath_Binding
> >>>>>
> >>>>> The problem i encountered is that this creates a "referential integrity
> >>>>> violation" because Routes are still referencing the deleted
> >>>>> Port_Bindings.
> >>>>>
> >>>>> As i understood it this is caused by:
> >>>>> 1. the ovsdb idl not actually sending deletes for nonRoot tables
> >>>>>    (https://github.com/openvswitch/ovs/blob/main/lib/ovsdb-idl.c#L3290)
> >>>>
> >>>> I wasn't aware of this, thanks for pointing it out (CC Ilya).
> >>>> Introduced a long time ago by:
> >>>> https://github.com/openvswitch/ovs/commit/dcd1dbc
> >>>>
> >>>> This is actually a bigger problem than you described here.  These
> >>>> records are to be created/deleted by ovn-controller.  But ovn-controller
> >>>> effectively can't delete any non-root Route records if they're still
> >>>> referenced by Datapath_Binding.
> >>>
> >>> In general, there is no point sending deletions for non-root tables.
> >>> Because they will either be garbage collected anyway, or your transaction
> >>> had a referential integrity violation in the first place and wouldn't
> >>> go through anyway.
> >>>
> >>
> >> I'm not sure I follow completely.  For clarity, in this case, the
> >> behavior I was hoping for is:
> >>
> >> - allow explicit deletes of isRoot=false records, i.e., Route records
> >> when ovn-controller (the one who created them) detects they should be
> >> removed (individually, regardless if they're referred or not).
> > 
> > You can't delete a record if there is a strong reference to it,
> > as that will violate referential integrity.
> > And if there is no reference, then you don't need to remove it,
> > because it will be just garbage-collected.
> > 
> > So, there should be no scenario where you actually need to explicitly
> > delete a record from a non-root table.
> > 
> 
> Ack I see, we should just remove the reference to the non-root table
> record, but in this case that record has a reference to a different
> table too (e.g., Route points to Port_Binding in the OVN case).  If that
> reference target is also removed in the same transaction then we hit the
> referential integrity violation problem that Felix pointed out on the
> server side - integrity check happening before garbage collection.

Thanks a lot for all that clarification.
I'll go ahead for now and just make these tables root tables.

> 
> >> - support automatic garbage collection of routes when the router
> >> (Datapath_Binding) goes away.
> > 
> > This should already work.
> > 
> > The problem is that those garbage-cllected records hold strong
> > references to other table (port bindings) and that, as Felix says,
> > breaks referential integrity check.  That is likely a bug in
> > ovsdb-server, unless we document somewhere that non-root tables
> > can't hold strong references to root ones (I don't remember if
> > that's the case).  That part needs checking and potentially fixing.
> > 
> > Best regards, Ilya Maximets.
> > 
> 
> Thanks,
> Dumitru
>