Hi Ales, Lorenzo,
On 2/4/25 1:33 PM, Ales Musil wrote:
> On Tue, Feb 4, 2025 at 1:30 PM Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> wrote:
>
I rephrased a bit the NEWS entry:
- Add concept of Transit Routers, users are now allowed to specify
options:requested-chassis for router ports; if the chassis is remote
then the router port will behave as a remote port.
Hope that's fine.
>> [...]
>>>
>>> /* If the router is for l3 gateway, it resides on a chassis
>>> * and its port type is "l3gateway". */
>>> - const char *chassis_name = smap_get(&op->od->nbr->options,
>> "chassis");
>>> + const char *lr_chassis = smap_get(&op->od->nbr->options,
>> "chassis");
>>> +
>>> + /* If the LRP has requested chassis, that is remote, set the
>> type to
>>> + * remote and add the appropriate chassis. */
>>> + const char *req_chassis = smap_get(&op->nbrp->options,
>>> + "requested-chassis");
>>> + const struct sbrec_chassis *sb_chassis = NULL;
>>> + const char *type = "patch";
>>> if (is_cr_port(op)) {
>>> - sbrec_port_binding_set_type(op->sb, "chassisredirect");
>>> - } else if (chassis_name) {
>>> - sbrec_port_binding_set_type(op->sb, "l3gateway");
>>> - } else {
>>> - sbrec_port_binding_set_type(op->sb, "patch");
>>> + type = "chassisredirect";
>>> + } else if (lr_chassis) {
>>> + type = "l3gateway";
>>> + } else if (req_chassis) {
>>> + const struct sbrec_chassis *tr_chassis = chassis_lookup(
>>> + sbrec_chassis_by_name, sbrec_chassis_by_hostname,
>> req_chassis);
>>> + bool trp = tr_chassis &&
>> smap_get_bool(&tr_chassis->other_config,
>>> + "is-remote", false);
>>> + sb_chassis = trp ? tr_chassis : NULL;
>>> + type = trp ? "remote" : "patch";
>>> + }
>>> +
>>> + sbrec_port_binding_set_type(op->sb, type);
>>> + sbrec_port_binding_set_requested_chassis(op->sb, sb_chassis);
>>
>
> Hi Lorenzo,
>
> thank you for the review.
>
>
>> nit: is it ok to set sbrec_port_binding_set_requested_chassis() even if
>> sb_chassis is NULL? (since you check it for
>> sbrec_port_binding_set_chassis()).
>>
>
> It is, because this is the only use case within LRP for requested-chassis,
> however it needs to be checked for chassis because we might otherwise
> overwrite what was set by ovn-controller. Does that make sense?
>
>
>>
>> Regards,
>> Lorenzo
>>
>>
> Thanks,
> Ales
>
>
>>> + if (sb_chassis) {
>>> + sbrec_port_binding_set_chassis(op->sb, sb_chassis);
>>> }
>>>
>>> if (is_cr_port(op)) {
>>> @@ -4167,6 +4185,14 @@ ovn_port_assign_requested_tnl_id(struct ovn_port
>> *op)
>>> return true;
>>> }
>>>
>>> +static bool
>>> +ovn_port_is_trp(struct ovn_port *op)
>>> +{
>>> + return op->nbrp &&
>>> + op->sb->chassis &&
>>> + smap_get_bool(&op->sb->chassis->other_config, "is-remote",
>> false);
>>> +}
>>> +
I found 'trp' too short as a name. I renamed this function to
is_transit_router_port(), moved it next to the other is_*_port()
functions and added a comment:
/* This function returns true if 'op' is a router port that has as
* requested chassis a remote chassis, i.e., if 'op' is a transit router
* port. */
static bool
is_transit_router_port(struct ovn_port *op)
{
[...]
}
>>> static bool
>>> ovn_port_allocate_key(struct ovn_port *op)
>>> {
>>> @@ -4272,6 +4298,7 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
>>> sbrec_mirror_table,
>>> op, queue_id_bitmap,
>>> &active_ha_chassis_grps);
>>> + op->od->is_transit_router |= ovn_port_is_trp(op);
>>> ovs_list_remove(&op->list);
>>> }
>>>
>>> @@ -4285,6 +4312,7 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
>>> op, queue_id_bitmap,
>>> &active_ha_chassis_grps);
>>> sbrec_port_binding_set_logical_port(op->sb, op->key);
>>> + op->od->is_transit_router |= ovn_port_is_trp(op);
>>> ovs_list_remove(&op->list);
>>> }
>>>
>>> diff --git a/northd/northd.h b/northd/northd.h
>>> index 6bca6bb1a..0da05d518 100644
>>> --- a/northd/northd.h
>>> +++ b/northd/northd.h
>>> @@ -356,6 +356,10 @@ struct ovn_datapath {
>>> * If this is true, then 'l3dgw_ports' will be ignored. */
>>> bool is_gw_router;
>>>
>>> + /* Indicates whether the router should be considered a transit
>> router.
>>> + * This is applicable only to routers with "remote" ports. */
>>> + bool is_transit_router;
>>> +
>>> /* OVN northd only needs to know about logical router gateway ports
>> for
>>> * NAT/LB on a distributed router. The "distributed gateway ports"
>> are
>>> * populated only when there is a gateway chassis or ha chassis
>> group
>>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>>> index d82f9872b..07bd24fce 100644
>>> --- a/ovn-nb.xml
>>> +++ b/ovn-nb.xml
>>> @@ -3104,6 +3104,32 @@ or
>>> <column name="external_ids">
>>> See <em>External IDs</em> at the beginning of this document.
>>> </column>
>>> +
>>> + <group title="Transit router">
>>> + <p>
>>> + In order to achieve status of <code>Transit Router</code> for
>>> + <ref table="Logical_Router"/> there needs to be at least one
>>> + <ref table="Logical_Router_Port"/> that is considered remote.
>>> + The LRP can be <code>remote</code> only if it has
>>> + <code>options:requested-chassis</code> set to chassis that is
>>> + considered remote. See <ref table="Logical_Router_Port"/> for
>> more
>>> + details.
>>> + </p>
>>> +
>>> + <p>
>>> + In order for the <code>Transit Router</code> to work properly
>> all the
>>> + tunnel keys for the <code>Transit Router</code> itself and the
>> remote
>>> + ports keys needs to match in all AZs e.g. TR in AZ1 and AZ2
>> needs to
>>> + have the same tunnel key. Remote port for AZ2 in AZ1 needs to
>> have the
>>> + same tunnel key as local port in AZ2 and vice vers.
>>> + </p>
>>> +
>>> + <p>
>>> + The <code>Transit Router</code> behaves as distributed router
>> which
>>> + means that it has the same limitations for stateful flows like
>>> + <code>NAT and LBs</code> and it will lose the CT state between
>> AZs.
>>> + </p>
>>> + </group>
>>> </table>
>>>
>>> <table name="Meter" title="Meter entry">
>>> @@ -3680,6 +3706,23 @@ or
>>> learned by the <code>ovn-ic</code> daemon.
>>> </p>
>>> </column>
>>> +
>>> + <column name="options" key="requested-chassis">
>>> + <p>
>>> + If set, identifies a specific chassis (by name or hostname)
>> that
>>> + is allowed to bind this port. This option is valid only for
>> chassis
>>> + that have <code>options:is-remote=true</code>, in other words
>> for
>>> + chassis that are in different Availability zone. The option
>> accepts
>>> + only single value.
>>> + </p>
>>> +
>>> + <p>
>>> + By assigning remote chassis the <ref table="Logical_Router"/>
>> gains
>>> + status of <code>Transit Router</code> see
>>> + <ref table="Logical_Router"/> table for more details.
>>> + </p>
>>> +
>>> + </column>
>>> </group>
>>>
>>> <group title="Attachment">
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index e64b5a660..40dcf338a 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -14560,3 +14560,54 @@ wait_row_count Multicast_Group 0 name=239.0.1.68
>>> OVN_CLEANUP([hv1], [hv2])
>>> AT_CLEANUP
>>> ])
>>> +
>>> +OVN_FOR_EACH_NORTHD_NO_HV([
>>> +AT_SETUP([Transit router - remote ports])
>>> +ovn_start NORTHD_TYPE
>>> +
>>> +check ovn-sbctl chassis-add hv2 geneve 192.168.0.12 \
>>> + -- set chassis hv2 other_config:is-remote=true
>>> +
>>> +check ovn-sbctl chassis-add hv3 geneve 192.168.0.13 \
>>> + -- set chassis hv3 other_config:is-remote=true
>>> +
>>> +check ovn-sbctl chassis-add hv4 geneve 192.168.0.14
>>> +
>>> +check ovn-nbctl lr-add tr
>>> +ovn-nbctl lrp-add tr tr-local 00:00:00:00:30:1 192.168.100.1/31
>>> +
>>> +ovn-nbctl lrp-add tr tr-az2 00:00:00:00:30:03 192.168.100.3/31 \
>>> + -- set Logical_Router_Port tr-az2 options:requested-chassis=hv2
>>> +
>>> +ovn-nbctl lrp-add tr tr-az3 00:00:00:00:30:04 192.168.100.4/31 \
>>> + -- set Logical_Router_Port tr-az3 options:requested-chassis=hv3
>>> +
>>> +ovn-nbctl lrp-add tr tr-az4 00:00:00:00:30:04 192.168.100.4/31 \
>>> + -- set Logical_Router_Port tr-az4 options:requested-chassis=hv4
>>> +
>>> +check ovn-nbctl --wait=sb sync
>>> +
>>> +hv2_uuid=$(fetch_column Chassis _uuid name=hv2)
>>> +hv3_uuid=$(fetch_column Chassis _uuid name=hv3)
>>> +hv4_uuid=$(fetch_column Chassis _uuid name=hv4)
>>> +
>>> +check_row_count Port_Binding 1 logical_port=tr-az2 type=remote
>> chassis=$hv2_uuid requested-chassis=$hv2_uuid
>>> +check_row_count Port_Binding 1 logical_port=tr-az3 type=remote
>> chassis=$hv3_uuid requested-chassis=$hv3_uuid
>>> +
>>> +# hv4 is not set as remote, so the port should remain as patch port
>>> +check_row_count Port_Binding 1 logical_port=tr-az4 type=patch
>>> +
>>> +check ovn-sbctl set chassis hv4 other_config:is-remote=true
>>> +check ovn-nbctl --wait=sb sync
>>> +
>>> +# Setting hv4 as remote should change the tr-az4 to remote too
>>> +check_row_count Port_Binding 1 logical_port=tr-az4 type=remote
>> chassis=$hv4_uuid requested-chassis=$hv4_uuid
>>> +
>>> +check ovn-sbctl remove chassis hv4 other_config is-remote
>>> +check ovn-nbctl --wait=sb sync
>>> +
>>> +# Removing "is-remote" from hv4 should change the tr-az4 back to patch
>> port
>>> +check_row_count Port_Binding 1 logical_port=tr-az4 type=patch
>>> +
>>> +AT_CLEANUP
>>> +])
>>> --
>>> 2.48.1
>>>
>>
Thanks, Ales, Numan, Lorenzo and Quique! I added Quique's "Tested-by"
[0] and pushed this patch to main.
Regards,
Dumitru
[0] https://mail.openvswitch.org/pipermail/ovs-dev/2025-February/420423.html
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
