On Fri, Jan 31, 2025 at 01:17:38PM +0100, Dumitru Ceara wrote: > Hi Felix, > > After having a closer look at this patch I only have a few tiny comments > for v5.
Hi Dumitru, thanks for the review. All things will be addressed in the next version. Thanks a lot, Felix > > On 1/22/25 1:36 PM, Dumitru Ceara wrote: > > On 1/22/25 12:22 PM, Felix Huettner wrote: > >> On Wed, Jan 22, 2025 at 11:50:37AM +0100, Dumitru Ceara wrote: > >>> On 1/22/25 11:42 AM, Felix Huettner via dev wrote: > >>>> On Tue, Jan 21, 2025 at 04:47:40PM +0100, Felix Huettner via dev wrote: > >>>>> This allows the ovn-controller to later find all ports that > >>>>> participate in dynamic routing. > >>>>> > >>>>> Signed-off-by: Felix Huettner <[email protected]> > >>>>> --- > >>>>> v2->v3: > >>>>> * A lot of minor review comments. > >>>>> * Added more documentation and news > >>>>> > >>>>> NEWS | 8 ++++++++ > >>>>> northd/northd.c | 21 +++++++++++++++++++++ > >>>>> ovn-nb.xml | 42 ++++++++++++++++++++++++++++++++++++++++++ > >>>>> tests/ovn-northd.at | 32 ++++++++++++++++++++++++++++++++ > >>>>> 4 files changed, 103 insertions(+) > >>>>> > >>>>> diff --git a/NEWS b/NEWS > >>>>> index 289cdd7f7..14390a564 100644 > >>>>> --- a/NEWS > >>>>> +++ b/NEWS > >>>>> @@ -43,6 +43,14 @@ Post v24.09.0 > >>>>> * Add the option "dynamic-routing-connected-as-host-routes" to > >>>>> LRPs. If > >>>>> set to true then connected routes are announced as individual > >>>>> host > >>>>> routes. > >>>>> + * Add the option "dynamic-routing-maintain-vrf" to LRPs. If set > >>>>> the > >>>>> + ovn-controller will create a vrf named "ovnvrf" + datapath id > >>>>> that > >>>>> + includes all advertised and learned routes. > >>>>> + The vrf name can be overwritten with the > >>>>> "dynamic-routing-vrf-name" > >>>>> + setting. > >>>>> + * Add the option "dynamic-routing-ifname" to LRPs. If set only > >>>>> routes > >>>>> + learned from a linux iterfaces with that name are treated as > >>>>> relevant > >>>>> + routes for this LRP. > >>>>> > >>>>> OVN v24.09.0 - 13 Sep 2024 > >>>>> -------------------------- > >>>>> diff --git a/northd/northd.c b/northd/northd.c > >>>>> index c80049bfd..4d08cf850 100644 > >>>>> --- a/northd/northd.c > >>>>> +++ b/northd/northd.c > >>>>> @@ -4065,6 +4065,27 @@ sync_pb_for_lrp(struct ovn_port *op, > >>>>> } > >>>>> } > >>>>> > >>>>> + if (is_cr_port(op) || chassis_name) { > >>>>> + if (op->od->dynamic_routing) { > >>>>> + smap_add(&new, "dynamic-routing", "true"); > >>>>> + if (smap_get_bool(&op->nbrp->options, > >>>>> + "dynamic-routing-maintain-vrf", false)) { > >>>>> + smap_add(&new, "dynamic-routing-maintain-vrf", "true"); > >>>>> + } > >>>>> + const char *vrfname = smap_get(&op->nbrp->options, > >>>>> + "dynamic-routing-vrf-name"); > >>>>> + if (vrfname) { > >>>>> + smap_add(&new, "dynamic-routing-vrf-name", vrfname); > >>>>> + } > >>>>> + const char *ifname = smap_get(&op->nbrp->options, > >>>>> + "dynamic-routing-ifname"); > >>>>> + if (ifname) { > >>>>> + smap_add(&new, "dynamic-routing-ifname", ifname); > >>>>> + } > >>>>> + } > >>>>> + } > >>>>> + > >>>>> + > > Nit: no need for newline. > > >>>>> const char *ipv6_pd_list = smap_get(&op->sb->options, > >>>>> "ipv6_ra_pd_list"); > >>>>> if (ipv6_pd_list) { > >>>>> smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > >>>>> diff --git a/ovn-nb.xml b/ovn-nb.xml > >>>>> index 4d4105a21..793a4f6bc 100644 > >>>>> --- a/ovn-nb.xml > >>>>> +++ b/ovn-nb.xml > >>>>> @@ -3777,6 +3777,48 @@ or > >>>>> </li> > >>>>> </ul> > >>>>> </column> > >>>>> + > >>>>> + <column name="options" key="dynamic-routing-maintain-vrf" > >>>>> + type='{"type": "boolean"}'> > >>>>> + Only relevant if <ref column="options" key="dynamic-routing" > >>>>> + table="Logical_Router"/> on the respective Logical_Router is > >>>>> set > >>>>> + to <code>true</code>. > >>>>> + > >>>>> + If this LRP is bound to a specific chassis then the > >>>>> ovn-controller of > >>>>> + this chassis will maintain a vrf. > >>>>> + This vrf will contain all the routes that should be announced > >>>>> from > >>>>> + this LRP. > >>>>> + Unless <ref column="options" key="dynamic-routing-vrf-name"/> > >>>>> is set > >>>>> + the vrf will be named "ovnvrf" with the datapath id of the > >>>>> Logical > >>>>> + Router appended to it. > >>>>> + </column> > >>>>> + > >>>>> + <column name="options" key="dynamic-routing-vrf-name" > >>>>> + type='{"type": "string"}'> > >>>>> + Only relevant if <ref column="options" key="dynamic-routing" > >>>>> + table="Logical_Router"/> on the respective Logical_Router is > >>>>> set > >>>>> + to <code>true</code>. > >>>>> + > >>>>> + This defines the name of the vrf the ovn-controller will use to > >>>>> + advertise and learn routes. If not set the vrf will be named > >>>>> "ovnvrf" > >>>>> + with the datapath id of the Logical Router appended to it. > >>>>> + </column> > >>>> > >>>> Hi everyone, > >>>> > >>> > >>> Hi Felix, > >>> > >>>> Just wanted to share that i am currently thinking about if this should > >>>> rather be a setting on the Logical_Router. Otherwise a chassis with > >>>> multiple LRPs bound locally might have different vrfs for the same > >>>> datapath. > >>>> > >>>> What are your opinions? > >>>> > >>> > >>> If I understand correctly, with the current implementation, if > >>> dynamic-routing-vrf-name is _not_ set then we anyway program routes in a > >>> single VRF for all LRPs (vrf name will be "ovnvrf-<datapath-id>"). > >>> > >>> In my opinion it makes sense that the dynamic-routing-vrf-name is > >>> configured for the whole router. > >> > >> I just saw that we anyway use the datapath id as the vrf ID. So as long > >> as that is the case we anyway need to scope this to a per Logical_Router > >> basis. > >> > >> I would update that in the next version. But i would wait for reviews of > >> all of these patches. > >> > > > > Sounds good. I do plan to look at this version in the coming days. > > > >>> > >>> Regards, > >>> Dumitru > >>> > >>>> Thanks a lot > >>>> Felix > >>>> > >>>>> + > >>>>> + <column name="options" key="dynamic-routing-ifname" > >>>>> + type='{"type": "string"}'> > >>>>> + Only relevant if <ref column="options" key="dynamic-routing" > >>>>> + table="Logical_Router"/> on the respective Logical_Router is > >>>>> set > >>>>> + to <code>true</code>. > >>>>> + > >>>>> + Only learn routes associated with the interface specified here. > >>>>> + This allows a single chassis to learn different routes on > >>>>> separate > >>>>> + LRPs bound to this chassis. > >>>>> + > >>>>> + This is usefully e.g. in the case of a chassis with multiple > >>>>> links > >>>>> + towards the network fabric where all of them run BGP > >>>>> individually. > >>>>> + This option allows to have a 1:1 mapping between a single LRP > >>>>> and an > >>>>> + individual link. > >>>>> + </column> > >>>>> </group> > >>>>> > >>>>> <group title="Attachment"> > >>>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > >>>>> index fa664599a..e949eef6e 100644 > >>>>> --- a/tests/ovn-northd.at > >>>>> +++ b/tests/ovn-northd.at > >>>>> @@ -14741,6 +14741,38 @@ check_row_count Advertised_Route 1 > >>>>> datapath=$datapath logical_port=$sw0 ip_prefi > >>>>> AT_CLEANUP > >>>>> ]) > >>>>> > >>>>> +OVN_FOR_EACH_NORTHD_NO_HV([ > >>>>> +AT_SETUP([dynamic-routing - lrp options]) > >>>>> +AT_KEYWORDS([dynamic-routing]) > >>>>> +ovn_start > >>>>> + > >>>>> +check ovn-nbctl lr-add lr0 > >>>>> +check ovn-nbctl set Logical_Router lr0 option:dynamic-routing=true \ > >>>>> + option:dynamic-routing-connected=true > >>>>> \ > >>>>> + option:dynamic-routing-static=true > >>>>> +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > >>>>> +check ovn-nbctl set Logical_Router lr0 options:chassis=hv1 > >>>>> +check ovn-nbctl ls-add sw0 > >>>>> +check ovn-nbctl lsp-add sw0 sw0-lr0 > >>>>> +check ovn-nbctl --wait=sb set Logical_Switch_Port sw0-lr0 type=router > >>>>> options:router-port=lr0-sw0 > >>>>> + > >>>>> +AT_CHECK([fetch_column sb:Port_Binding options logical_port=lr0-sw0 > >>>>> |grep -q 'dynamic-routing=true']) > >>>>> +AT_CHECK([fetch_column sb:Port_Binding options logical_port=lr0-sw0 > >>>>> |grep -qv 'dynamic-routing-maintain-vrf']) > >>>>> +AT_CHECK([fetch_column sb:Port_Binding options logical_port=lr0-sw0 > >>>>> |grep -qv 'dynamic-routing-vrf-name']) > >>>>> +AT_CHECK([fetch_column sb:Port_Binding options logical_port=lr0-sw0 > >>>>> |grep -qv 'dynamic-routing-ifname']) > > Nit: Missing space before grep. > > >>>>> + > >>>>> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0 > >>>>> options:dynamic-routing-maintain-vrf=true \ > >>>>> + > >>>>> options:dynamic-routing-vrf-name=myvrf \ > >>>>> + > >>>>> options:dynamic-routing-ifname=myif > >>>>> + > >>>>> +AT_CHECK([fetch_column sb:Port_Binding options logical_port=lr0-sw0 > >>>>> |grep -q 'dynamic-routing=true']) > >>>>> +AT_CHECK([fetch_column sb:Port_Binding options logical_port=lr0-sw0 > >>>>> |grep -q 'dynamic-routing-maintain-vrf=true']) > >>>>> +AT_CHECK([fetch_column sb:Port_Binding options logical_port=lr0-sw0 > >>>>> |grep -q 'dynamic-routing-vrf-name=myvrf']) > >>>>> +AT_CHECK([fetch_column sb:Port_Binding options logical_port=lr0-sw0 > >>>>> |grep -q 'dynamic-routing-ifname=myif']) > > Nit: Here too. > > >>>>> + > >>>>> +AT_CLEANUP > >>>>> +]) > >>>>> + > >>>>> > >>>>> OVN_FOR_EACH_NORTHD_NO_HV([ > >>>>> AT_SETUP([dynamic-routing incremental processing]) > >>>>> -- > >>>>> 2.47.1 > >>>>> > > Thanks, > Dumitru > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
