Hi Numan,
On 4/14/25 8:39 PM, Mark Michelson wrote:
> On 4/14/25 11:22, Numan Siddique wrote:
>> On Mon, Apr 14, 2025 at 1:00 AM Han Zhou <hz...@ovn.org> wrote:
>>>
>>>
>>>
>>> On Sat, Apr 12, 2025 at 8:46 AM <num...@ovn.org> wrote:
>>>>
>>>> From: Numan Siddique <num...@ovn.org>
>>>>
>>>> Consider the below logical topology
>>>>
>>>> sw0-p1 -
>>>> | (distributed NATs configured)
>>>> sw0-p2 - -> sw0 -> lr0 ----
>>>> ... | |
>>>> sw0-pn - |
>>>> |
>>>> sw1-p1 - |
>>>> | |
>>>> sw1p-2 - -> sw1 -> lr1 ---- --- public (provider switch)
>>>> ... | |
>>>> sw1-pn- |
>>>> |
>>>> swn-p1 - |
>>>> | |
>>>> swn-p2- -> swn -> lrn ----
>>>> ... |
>>>> swn-pn -
>>>>
>>>> All the routers are connected to the provider switch via a ditributed
>>>> gateway port.
>>>>
>>>> If sw0-p1 is resident on the chassis C1, then since there is a path
>>>> to all the switches and the routers, ovn-controller will add all
>>>> these datapaths to its 'local_datapaths' map. This in turn results
>>>> in processing all the logical flows and installing all the openflows
>>>> and in turn wasting the CPU time. This can be very costly in
>>>> a highly scaled deployment.
>>>>
>>>> Note that for the same topology, if there are no distributed NATs
>>>> configured on lr0, thanks to the commit [1], ovn-northd will set
>>>> the option always-redirect=true in the chassis-redirect port.
>>>> If this option is set, then ovn-controller only adds [sw0, lr0]
>>>> in its local datapaths.
>>>>
>>>> However for deployments with distributed NATs, ovn-controller adds
>>>> all the datapaths to its local_datapaths map.
>>>>
>>>> With this patch, when ovn-controller claims sw0-p1, it only adds
>>>> [sw0, lr0, public] to its local_datapaths map. If it later claims
>>>> sw1-p1, it will add [sw1, lr1] to the map.
>>>>
>>>> This reduces the recompute time and the number of openflow rules
>>>> added to ovs-vswitchd significantly.
>>>>
>>>> If the public switch is connected to a router 'lr4' and and the
>>>> router port is not a distributed gateway port (DGP), then [lr4, sw4]
>>>> are also added to the map.
>>>>
>>>> This patch handles the scenario when a gateway port is configured as
>>>> a normal distributed router port (i.e its gateway_chassis or
>>>> ha_chassis_group is cleared). In this case it adds both the switch
>>>> and router datapaths to the local_datapaths map incrementally.
>>>> However it doesn't handle the other way - i.e a distributed router
>>>> port is configured as gateway port. ovn-controller will not delete
>>>> the switch and router datapaths from its map when handling these
>>>> changes incrementally. A subsequent full recompute will fix this.
>>>> Although we can handle this scenario incrementally, it comes up with
>>>> a bit of code complexity and we can handle it later if required.
>>>> Having the openflows of these datapaths is not going to be of any
>>>> harm.
>>>>
>>>> I tested this patch with a deployment of below logical resources:
>>>>
>>>> No of logical switches - 778
>>>> No of logical routers - 871
>>>> No of logical flows - 85626
>>>> No of 'ovn-sbctl dump-flows' - 208631
>>>>
>>>> Without this patch, afte claiming sw0-p1, ovn-controller adds 269098
>>>> openflow rules and it takes approx 2500 milli seconds for a
>>>> recompute.
>>>>
>>>> With this patch, after claiming sw0-p1, ovn-controller adds 21350
>>>> openflow rules and it takes approx 280 milli seconds for a recompute.
>>>>
>>>> There is approx 90% reduction in the openflow rules and 88% reduction
>>>> in recompute time when a compute node has VIFs from one logical
>>>> switch.
>>>>
>>>> [1] - 22298fd37 ("ovn-controller: Don't flood fill local datapaths
>>>> beyond DGP boundary.)
>>>>
>>>> Suggested-by: Han Zhou <hz...@ovn.org>
>>>> Acked-by: Mark Michelson <mmich...@redhat.com>
>>>> Co-authored-by: Xavier Simonart <xsimo...@redhat.com>
>>>> Signed-off-by: Xavier Simonart <xsimo...@redhat.com>
>>>> Signed-off-by: Numan Siddique <num...@ovn.org>
>>>
>>> Thanks Numan.
>>> Acked-by: Han Zhou <hz...@ovn.org>
>>
>>
>> Thank you Han, Mark, Dumitru and Xavier.
>>
>> I applied this patch series to main.
>>
>> After a week, I'm planning to backport this series to branch-25.03,
>> Please let me know if there are any objections.
>>
>> Numan
>
> No objections from me. Thanks for merging this!
>
Same from me, I think it's fine to have this in 25.03.
Regards,
Dumitru
>>
>>>
>>>> ---
>>>> controller/binding.c | 35 ++--
>>>> controller/local_data.c | 79 +++++----
>>>> controller/local_data.h | 6 +-
>>>> tests/ovn.at | 375 ++++++++++++++++++++++++++++++++++++
>>>> ++++
>>>> tests/system-ovn.at | 28 +++
>>>> 5 files changed, 475 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/controller/binding.c b/controller/binding.c
>>>> index b657de9e44..fdb0ad124b 100644
>>>> --- a/controller/binding.c
>>>> +++ b/controller/binding.c
>>>> @@ -1923,11 +1923,14 @@ consider_nonvif_lport_(const struct
>>>> sbrec_port_binding *pb,
>>>> /* Add the peer datapath to the local datapaths if it's
>>>> * not present yet.
>>>> */
>>>> - if (need_add_peer_to_local(
>>>> + const struct sbrec_port_binding *peer =
>>>> + lport_get_peer(pb, b_ctx_in-
>>>> >sbrec_port_binding_by_name);
>>>> +
>>>> + if (peer && need_add_peer_to_local(
>>>> b_ctx_in->sbrec_port_binding_by_name, pb,
>>>> - b_ctx_in->chassis_rec)) {
>>>> + peer, b_ctx_in->chassis_rec)) {
>>>> add_local_datapath_peer_port(
>>>> - pb, b_ctx_in->chassis_rec,
>>>> + pb, peer, b_ctx_in->chassis_rec,
>>>> b_ctx_in->sbrec_datapath_binding_by_key,
>>>> b_ctx_in->sbrec_port_binding_by_datapath,
>>>> b_ctx_in->sbrec_port_binding_by_name,
>>>> @@ -2958,27 +2961,27 @@
>>>> consider_patch_port_for_local_datapaths(const struct
>>>> sbrec_port_binding *pb,
>>>> struct binding_ctx_in
>>>> *b_ctx_in,
>>>> struct binding_ctx_out
>>>> *b_ctx_out)
>>>> {
>>>> - struct local_datapath *ld =
>>>> - get_local_datapath(b_ctx_out->local_datapaths,
>>>> - pb->datapath->tunnel_key);
>>>> + const struct sbrec_port_binding *peer;
>>>> + struct local_datapath *peer_ld = NULL;
>>>> + struct local_datapath *ld = NULL;
>>>> +
>>>> + ld = get_local_datapath(b_ctx_out->local_datapaths,
>>>> + pb->datapath->tunnel_key);
>>>>
>>>> + peer = lport_get_peer(pb, b_ctx_in->sbrec_port_binding_by_name);
>>>> if (!ld) {
>>>> /* If 'ld' for this lport is not present, then check if
>>>> * there is a peer for this lport. If peer is present
>>>> * and peer's datapath is already in the local datapaths,
>>>> * then add this lport's datapath to the local_datapaths.
>>>> * */
>>>> - const struct sbrec_port_binding *peer;
>>>> - struct local_datapath *peer_ld = NULL;
>>>> - peer = lport_get_patch_peer(pb, b_ctx_in-
>>>> >sbrec_port_binding_by_name);
>>>> if (peer) {
>>>> - peer_ld =
>>>> - get_local_datapath(b_ctx_out->local_datapaths,
>>>> - peer->datapath->tunnel_key);
>>>> + peer_ld = get_local_datapath(b_ctx_out->local_datapaths,
>>>> + peer->datapath->tunnel_key);
>>>> }
>>>> if (peer_ld && need_add_peer_to_local(
>>>> b_ctx_in->sbrec_port_binding_by_name, peer,
>>>> - b_ctx_in->chassis_rec)) {
>>>> + pb, b_ctx_in->chassis_rec)) {
>>>> add_local_datapath(
>>>> b_ctx_in->sbrec_datapath_binding_by_key,
>>>> b_ctx_in->sbrec_port_binding_by_datapath,
>>>> @@ -2991,11 +2994,11 @@
>>>> consider_patch_port_for_local_datapaths(const struct
>>>> sbrec_port_binding *pb,
>>>> /* Add the peer datapath to the local datapaths if it's
>>>> * not present yet.
>>>> */
>>>> - if (need_add_peer_to_local(
>>>> + if (peer && need_add_peer_to_local(
>>>> b_ctx_in->sbrec_port_binding_by_name, pb,
>>>> - b_ctx_in->chassis_rec)) {
>>>> + peer, b_ctx_in->chassis_rec)) {
>>>> add_local_datapath_peer_port(
>>>> - pb, b_ctx_in->chassis_rec,
>>>> + pb, peer, b_ctx_in->chassis_rec,
>>>> b_ctx_in->sbrec_datapath_binding_by_key,
>>>> b_ctx_in->sbrec_port_binding_by_datapath,
>>>> b_ctx_in->sbrec_port_binding_by_name,
>>>> diff --git a/controller/local_data.c b/controller/local_data.c
>>>> index 2df5cd3392..e256de5fa4 100644
>>>> --- a/controller/local_data.c
>>>> +++ b/controller/local_data.c
>>>> @@ -130,44 +130,68 @@ local_datapath_destroy(struct local_datapath *ld)
>>>> free(ld);
>>>> }
>>>>
>>>> -/* Checks if pb is running on local gw router or pb is a patch port
>>>> - * and the peer datapath should be added to local datapaths. */
>>>> +/* Checks if pb is running on local gw router or pb and peer
>>>> + * are patch ports and if the peer's datapath should be added to
>>>> + * local datapaths or not.
>>>> + *
>>>> + * Note that if 'pb' belongs to a logical switch and 'peer' to a
>>>> + * logical router datapath and if 'peer' has a chassis-redirect port,
>>>> + * then we add the 'peer' to the local datapaths only if the
>>>> + * chassis-redirect port is local.
>>>> + * */
>>>> bool
>>>> need_add_peer_to_local(
>>>> struct ovsdb_idl_index *sbrec_port_binding_by_name,
>>>> const struct sbrec_port_binding *pb,
>>>> + const struct sbrec_port_binding *peer,
>>>> const struct sbrec_chassis *chassis)
>>>> {
>>>> /* This port is running on local gw router. */
>>>> - if (!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) {
>>>> + if (!strcmp(pb->type, "l3gateway") && pb->chassis == chassis &&
>>>> + peer->chassis == chassis) {
>>>> return true;
>>>> }
>>>>
>>>> - /* If it is not a patch port, no peer to add. */
>>>> + /* If pb is not a patch port, no peer to add. */
>>>> if (strcmp(pb->type, "patch")) {
>>>> return false;
>>>> }
>>>>
>>>> - /* If it is a regular patch port, it is fully distributed, add
>>>> the peer. */
>>>> - const char *crp_name = smap_get(&pb->options, "chassis-
>>>> redirect-port");
>>>> - if (!crp_name) {
>>>> + const char *cr_pb_name = smap_get(&pb->options,
>>>> + "chassis-redirect-port");
>>>> + const char *cr_peer_name = smap_get(&peer->options,
>>>> + "chassis-redirect-port");
>>>> + if (!cr_pb_name && !cr_peer_name) {
>>>> + /* pb and peer are regular patch ports (fully distributed),
>>>> + * add the peer to local datapaths. */
>>>> return true;
>>>> }
>>>>
>>>> - /* A DGP, find its chassis-redirect-port pb. */
>>>> - const struct sbrec_port_binding *pb_crp =
>>>> - lport_get_cr_port(sbrec_port_binding_by_name, pb, crp_name);
>>>> + const struct sbrec_port_binding *cr_pb =
>>>> + lport_get_cr_port(sbrec_port_binding_by_name, pb, cr_pb_name);
>>>> + const struct sbrec_port_binding *cr_peer =
>>>> + lport_get_cr_port(sbrec_port_binding_by_name, peer,
>>>> cr_peer_name);
>>>>
>>>> - if (!pb_crp) {
>>>> + if (!cr_pb && !cr_peer) {
>>>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
>>>> 5);
>>>> VLOG_WARN_RL(&rl, "chassis-redirect-port %s for DGP %s is
>>>> not found.",
>>>> - crp_name, pb->logical_port);
>>>> + cr_pb_name ? cr_pb_name : cr_peer_name,
>>>> + cr_pb_name ? pb->logical_port :
>>>> + peer->logical_port);
>>>> return false;
>>>> }
>>>>
>>>> - /* Check if it is configured as "always-redirect". If not, then
>>>> we will
>>>> + if (cr_peer && datapath_is_switch(pb->datapath) &&
>>>> + !datapath_is_switch(peer->datapath)) {
>>>> + /* pb belongs to logical switch and peer belongs to
>>>> logical router.
>>>> + * Add the peer to local datapaths only if its chassis-
>>>> redirect-port
>>>> + * is local. */
>>>> + return ha_chassis_group_contains(cr_peer->ha_chassis_group,
>>>> chassis);
>>>> + }
>>>> +
>>>> + /* Check if cr-pb is configured as "always-redirect". If not,
>>>> then we will
>>>> * need to add the peer to local for distributed processing. */
>>>> - if (!smap_get_bool(&pb_crp->options, "always-redirect", false)) {
>>>> + if (cr_pb && !smap_get_bool(&cr_pb->options, "always-redirect",
>>>> false)) {
>>>> return true;
>>>> }
>>>>
>>>> @@ -175,9 +199,10 @@ need_add_peer_to_local(
>>>> * the peer to local, which could be the localnet network,
>>>> which doesn't
>>>> * have other chances to be added to local datapaths if there
>>>> is no VIF
>>>> * bindings. */
>>>> - if (pb_crp->ha_chassis_group) {
>>>> - return ha_chassis_group_contains(pb_crp->ha_chassis_group,
>>>> chassis);
>>>> + if (cr_pb && cr_pb->ha_chassis_group) {
>>>> + return ha_chassis_group_contains(cr_pb->ha_chassis_group,
>>>> chassis);
>>>> }
>>>> +
>>>> return false;
>>>> }
>>>>
>>>> @@ -200,6 +225,7 @@ add_local_datapath(struct ovsdb_idl_index
>>>> *sbrec_datapath_binding_by_key,
>>>> void
>>>> add_local_datapath_peer_port(
>>>> const struct sbrec_port_binding *pb,
>>>> + const struct sbrec_port_binding *peer,
>>>> const struct sbrec_chassis *chassis,
>>>> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
>>>> struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
>>>> @@ -208,25 +234,18 @@ add_local_datapath_peer_port(
>>>> struct hmap *local_datapaths,
>>>> struct hmap *tracked_datapaths)
>>>> {
>>>> - const struct sbrec_port_binding *peer =
>>>> - lport_get_peer(pb, sbrec_port_binding_by_name);
>>>> -
>>>> - if (!peer) {
>>>> - return;
>>>> - }
>>>> -
>>>> local_datapath_peer_port_add(ld, pb, peer);
>>>>
>>>> struct local_datapath *peer_ld =
>>>> get_local_datapath(local_datapaths,
>>>> peer->datapath->tunnel_key);
>>>> if (!peer_ld) {
>>>> - add_local_datapath__(sbrec_datapath_binding_by_key,
>>>> - sbrec_port_binding_by_datapath,
>>>> - sbrec_port_binding_by_name, 1,
>>>> - peer->datapath, chassis, local_datapaths,
>>>> - tracked_datapaths);
>>>> - return;
>>>> + peer_ld =
>>>> + add_local_datapath__(sbrec_datapath_binding_by_key,
>>>> + sbrec_port_binding_by_datapath,
>>>> + sbrec_port_binding_by_name, 1,
>>>> + peer->datapath, chassis,
>>>> local_datapaths,
>>>> + tracked_datapaths);
>>>> }
>>>>
>>>> local_datapath_peer_port_add(peer_ld, peer, pb);
>>>> @@ -626,7 +645,7 @@ add_local_datapath__(struct ovsdb_idl_index
>>>> *sbrec_datapath_binding_by_key,
>>>> const struct sbrec_port_binding *peer =
>>>> lport_get_peer(pb, sbrec_port_binding_by_name);
>>>> if (peer &&
>>>> need_add_peer_to_local(sbrec_port_binding_by_name,
>>>> - pb, chassis)) {
>>>> + pb, peer, chassis)) {
>>>> struct local_datapath *peer_ld =
>>>> add_local_datapath__(sbrec_datapath_binding_by_key,
>>>> sbrec_port_binding_by_datapath,
>>>> diff --git a/controller/local_data.h b/controller/local_data.h
>>>> index 1d60dada86..19d5582328 100644
>>>> --- a/controller/local_data.h
>>>> +++ b/controller/local_data.h
>>>> @@ -76,7 +76,8 @@ struct local_datapath *get_local_datapath_no_hash(
>>>> bool
>>>> need_add_peer_to_local(
>>>> struct ovsdb_idl_index *sbrec_port_binding_by_name,
>>>> - const struct sbrec_port_binding *,
>>>> + const struct sbrec_port_binding *pb,
>>>> + const struct sbrec_port_binding *peer,
>>>> const struct sbrec_chassis *);
>>>> void add_local_datapath(
>>>> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
>>>> @@ -90,7 +91,8 @@ void add_local_datapath(
>>>> void local_datapaths_destroy(struct hmap *local_datapaths);
>>>> void local_datapath_destroy(struct local_datapath *ld);
>>>> void add_local_datapath_peer_port(
>>>> - const struct sbrec_port_binding *,
>>>> + const struct sbrec_port_binding *pb,
>>>> + const struct sbrec_port_binding *peer,
>>>> const struct sbrec_chassis *,
>>>> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
>>>> struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
>>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>>> index bbaa653a82..a8b0770a34 100644
>>>> --- a/tests/ovn.at
>>>> +++ b/tests/ovn.at
>>>> @@ -42499,6 +42499,381 @@ wait_row_count ACL_ID 0
>>>> AT_CLEANUP
>>>> ])
>>>>
>>>> +OVN_FOR_EACH_NORTHD_NO_HV([
>>>> +AT_SETUP([ovn-controller -- local datapaths check])
>>>> +ovn_start
>>>> +net_add n1
>>>> +sim_add hv1
>>>> +as hv1
>>>> +check ovs-vsctl add-br br-phys
>>>> +ovn_attach n1 br-phys 192.168.0.1
>>>> +
>>>> +sim_add hv2
>>>> +as hv2
>>>> +check ovs-vsctl add-br br-phys
>>>> +ovn_attach n1 br-phys 192.168.0.2
>>>> +
>>>> +sim_add gw1
>>>> +as gw1
>>>> +check ovs-vsctl add-br br-phys
>>>> +ovn_attach n1 br-phys 192.168.0.3
>>>> +
>>>> +sim_add gw2
>>>> +as gw2
>>>> +check ovs-vsctl add-br br-phys
>>>> +ovn_attach n1 br-phys 192.168.0.4
>>>> +
>>>> +check ovn-nbctl ls-add sw0
>>>> +check ovn-nbctl lsp-add sw0 sw0-port1
>>>> +check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01
>>>> 10.0.0.3 1000::3"
>>>> +check ovn-nbctl lsp-add sw0 sw0-port2
>>>> +check ovn-nbctl lsp-set-addresses sw0-port2 "50:54:00:00:00:02
>>>> 10.0.0.4 1000::4"
>>>> +
>>>> +check ovn-nbctl lr-add lr0
>>>> +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
>>>> 1000::1/64
>>>> +check ovn-nbctl lsp-add sw0 sw0-lr0
>>>> +check ovn-nbctl lsp-set-type sw0-lr0 router
>>>> +check ovn-nbctl lsp-set-addresses sw0-lr0 router
>>>> +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
>>>> +
>>>> +check ovn-nbctl ls-add public
>>>> +check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13
>>>> 172.168.0.100/24 2000::1/64
>>>> +check ovn-nbctl lsp-add public public-lr0
>>>> +check ovn-nbctl lsp-set-type public-lr0 router
>>>> +check ovn-nbctl lsp-set-addresses public-lr0 router
>>>> +check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
>>>> +
>>>> +# localnet port
>>>> +check ovn-nbctl lsp-add public ln-public
>>>> +check ovn-nbctl lsp-set-type ln-public localnet
>>>> +check ovn-nbctl lsp-set-addresses ln-public unknown
>>>> +check ovn-nbctl lsp-set-options ln-public network_name=phys
>>>> +
>>>> +check ovn-nbctl lrp-set-gateway-chassis lr0-public gw1 20
>>>> +check ovn-nbctl lr-nat-add lr0 snat 172.168.0.100 10.0.0.0/24
>>>> +check ovn-nbctl lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4
>>>> sw0-port2 f0:00:00:01:02:04
>>>> +check ovn-nbctl lr-nat-add lr0 snat 2000::1 1000::/64
>>>> +check ovn-nbctl lr-nat-add lr0 dnat_and_snat 2000::2 1000::4 sw0-
>>>> port2 f0:00:00:01:02:04
>>>> +
>>>> +check ovn-nbctl --wait=hv sync
>>>> +
>>>> +check_local_datapath() {
>>>> + hv=$1
>>>> + expected_dps=$2
>>>> + OVS_WAIT_UNTIL(
>>>> + [check_expected $hv $expected_dps], [echo "Did not find flows
>>>> on $expected_dps on $hv"])
>>>> + OVS_WAIT_UNTIL(
>>>> + [check_unexpected $hv $expected_dps], [echo "Found unexpected
>>>> flows on $hv"])
>>>> + found=$(as $hv ovn-appctl -t ovn-controller debug/dump-local-
>>>> datapaths \
>>>> +| grep -v "Local" | sed 's/.*Datapath:\s*\([[a-z0-9-]]*\).*/\1/')
>>>> + dps=$(echo "$expected_dps" | tr ',' '\n' | sort | tr '\n' ' ')
>>>> + found=$(echo "$found" | sort | tr '\n' ' ')
>>>> + check test "$dps" = "$found"
>>>> +}
>>>> +
>>>> +check_unexpected() {
>>>> + hv=$1
>>>> + dps=$2
>>>> + flows=$(as $hv ovs-ofctl dump-flows br-int)
>>>> +
>>>> + # Check for flows from unexpected datapaths
>>>> + dps=$(echo "$dps" | tr ',' ' ')
>>>> + echo "Looking for unexpected flows on $hv"
>>>> + for dp in $dps
>>>> + do
>>>> + if [[ -n "${dp}" ]]; then
>>>> + key=$(printf "0x%x" $(fetch_column Datapath_Binding
>>>> tunnel_key external_ids:name=$dp))
>>>> + if [[ -n "${key}" ]]; then
>>>> + flows=$(echo "$flows" | grep -v "metadata=$key" )
>>>> + fi
>>>> + fi
>>>> + done
>>>> + n_flows=$(echo "$flows" | grep "metadata=" | wc -l)
>>>> + if [[ "${n_flows}" != "0" ]]; then
>>>> + #echo "hv $hv has $n_flows remaining flows"
>>>> + return 1
>>>> + fi
>>>> +}
>>>> +
>>>> +check_expected() {
>>>> + # Check for flows from expected datapaths
>>>> + hv=$1
>>>> + dps=$2
>>>> + dps=$(echo "$dps" | tr ',' ' ')
>>>> + for dp in $dps
>>>> + do
>>>> + if [[ -n "${dp}" ]]; then
>>>> + key=$(printf "0x%x" $(fetch_column Datapath_Binding
>>>> tunnel_key external_ids:name=$dp))
>>>> + if [[ -n "${key}" ]]; then
>>>> + echo "Looking for expected flows in $hv for $dp i.e.
>>>> tunnel_key
>>>> +$key"
>>>> + n_flows=$(as $hv ovs-ofctl dump-flows br-int | grep -c
>>>> "metadata=$key")
>>>> + if [[ "${n_flows}" -eq "0" ]]; then
>>>> + #echo "hv $hv has no flows on dp $dp"
>>>> + return 1
>>>> + fi
>>>> + fi
>>>> + fi
>>>> + done
>>>> +}
>>>> +
>>>> +trigger_recompute() {
>>>> + check ovn-nbctl --wait=hv sync
>>>> + for hv in hv1 hv2 gw1 gw2
>>>> + do
>>>> + as $hv ovn-appctl inc-engine/recompute
>>>> + done
>>>> + check ovn-nbctl --wait=hv sync
>>>> +}
>>>> +
>>>> +check_local_datapath hv1 ""
>>>> +check_local_datapath hv2 ""
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 ""
>>>> +
>>>> +# Create a VIF on hv1 for sw0-port1
>>>> +AS_BOX([create a VIF on hv1 for sw0-port1])
>>>> +
>>>> +as hv1
>>>> +ovs-vsctl -- add-port br-int hv1-vif1 -- \
>>>> + set interface hv1-vif1 external-ids:iface-id=sw0-port1 \
>>>> + options:tx_pcap=hv1/vif1-tx.pcap \
>>>> + options:rxq_pcap=hv1/vif1-rx.pcap \
>>>> + ofport-request=1
>>>> +
>>>> +wait_for_ports_up sw0-port1
>>>> +
>>>> +AS_BOX([Create a VIF on hv1 for sw0-port1 - hv1 should have flows
>>>> for sw0, lr0 and public])
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 ""
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 ""
>>>> +
>>>> +AS_BOX([create a switch sw1 and router lr1, attach both and attach
>>>> lr1 to public])
>>>> +
>>>> +check ovn-nbctl ls-add sw1
>>>> +check ovn-nbctl lsp-add sw1 sw1-port1
>>>> +check ovn-nbctl lsp-set-addresses sw1-port1 "60:54:00:00:00:01
>>>> 20.0.0.3"
>>>> +
>>>> +check ovn-nbctl lr-add lr1
>>>> +check ovn-nbctl lrp-add lr1 lr1-sw1 00:00:01:00:ef:01 20.0.0.1/24
>>>> +check ovn-nbctl lsp-add sw1 sw1-lr1
>>>> +check ovn-nbctl lsp-set-type sw1-lr1 router
>>>> +check ovn-nbctl lsp-set-addresses sw1-lr1 router
>>>> +check ovn-nbctl lsp-set-options sw1-lr1 router-port=lr1-sw1
>>>> +
>>>> +check ovn-nbctl lrp-add lr1 lr1-public 00:00:20:30:22:13
>>>> 172.168.0.101/24
>>>> +check ovn-nbctl lsp-add public public-lr1
>>>> +check ovn-nbctl lsp-set-type public-lr1 router
>>>> +check ovn-nbctl lsp-set-addresses public-lr1 router
>>>> +check ovn-nbctl lsp-set-options public-lr1 router-port=lr1-public
>>>> +
>>>> +check ovn-nbctl lr-nat-add lr1 snat 172.168.0.101 20.0.0.0/24
>>>> +check ovn-nbctl lr-nat-add lr1 dnat_and_snat 172.168.0.140 20.0.0.3
>>>> +
>>>> +AS_BOX([create a switch sw2 and router lr2, attach both and attach
>>>> lr2 to public])
>>>> +
>>>> +check ovn-nbctl ls-add sw2
>>>> +check ovn-nbctl lsp-add sw2 sw2-port1
>>>> +check ovn-nbctl lsp-set-addresses sw2-port1 "70:54:00:00:00:01
>>>> 30.0.0.3"
>>>> +
>>>> +check ovn-nbctl lr-add lr2
>>>> +check ovn-nbctl lrp-add lr2 lr2-sw2 00:00:02:00:ef:01 30.0.0.1/24
>>>> +check ovn-nbctl lsp-add sw2 sw2-lr2
>>>> +check ovn-nbctl lsp-set-type sw2-lr2 router
>>>> +check ovn-nbctl lsp-set-addresses sw2-lr2 router
>>>> +check ovn-nbctl lsp-set-options sw2-lr2 router-port=lr2-sw2
>>>> +
>>>> +check ovn-nbctl lrp-add lr2 lr2-public 00:00:20:40:22:53
>>>> 172.168.0.102/24
>>>> +check ovn-nbctl lsp-add public public-lr2
>>>> +check ovn-nbctl lsp-set-type public-lr2 router
>>>> +check ovn-nbctl lsp-set-addresses public-lr2 router
>>>> +check ovn-nbctl lsp-set-options public-lr2 router-port=lr2-public
>>>> +
>>>> +check ovn-nbctl lr-nat-add lr2 snat 172.168.0.102 30.0.0.0/24
>>>> +check ovn-nbctl lr-nat-add lr2 dnat_and_snat 172.168.0.150 30.0.0.3
>>>> +
>>>> +check ovn-nbctl --wait=hv sync
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public,sw1,lr1,sw2,lr2
>>>> +check_local_datapath hv2 ""
>>>> +check_local_datapath gw1 sw0,lr0,public,sw1,lr1,sw2,lr2
>>>> +check_local_datapath gw2 ""
>>>> +
>>>> +AS_BOX([Set gw2 as gateway chassis for lr1-public and lr2-public])
>>>> +check ovn-nbctl --wait=hv lrp-set-gateway-chassis lr1-public gw2 20
>>>> +check ovn-nbctl --wait=hv lrp-set-gateway-chassis lr2-public gw2 30
>>>> +wait_row_count Port_Binding 1 logical_port=cr-lr1-public
>>>> +wait_row_count Port_Binding 1 logical_port=cr-lr2-public
>>>> +
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 ""
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +AS_BOX([Create a VIF on hv2 for sw1-port1])
>>>> +
>>>> +as hv2
>>>> +ovs-vsctl -- add-port br-int hv2-vif1 -- \
>>>> + set interface hv2-vif1 external-ids:iface-id=sw1-port1 \
>>>> + options:tx_pcap=hv2/vif1-tx.pcap \
>>>> + options:rxq_pcap=hv2/vif1-rx.pcap \
>>>> + ofport-request=1
>>>> +
>>>> +wait_for_ports_up sw1-port1
>>>> +
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 sw1,lr1
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +# Add distributed dnat_and_snat in lr1. hv2 should have
>>>> +# public in its local datapaths.
>>>> +AS_BOX([ Add distributed dnat_and_snat in lr1])
>>>> +
>>>> +check ovn-nbctl lr-nat-del lr1 dnat_and_snat
>>>> +check ovn-nbctl --wait=hv lr-nat-add lr1 dnat_and_snat
>>>> 172.168.0.140 20.0.0.3 sw1-port1 10:00:00:01:02:14
>>>> +
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 sw1,lr1,public
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +AS_BOX([Create a VIF on hv2 for sw0-port2])
>>>> +
>>>> +as hv2
>>>> +ovs-vsctl -- add-port br-int hv2-vif2 -- \
>>>> + set interface hv2-vif2 external-ids:iface-id=sw0-port2 \
>>>> + options:tx_pcap=hv2/vif2-tx.pcap \
>>>> + options:rxq_pcap=hv2/vif2-rx.pcap \
>>>> + ofport-request=2
>>>> +
>>>> +wait_for_ports_up sw0-port2
>>>> +
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 sw0,lr0,sw1,lr1,public
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +AS_BOX([Delete the VIF for sw1-port1 in hv2])
>>>> +
>>>> +as hv2 ovs-vsctl del-port hv2-vif1
>>>> +check ovn-nbctl --wait=hv sync
>>>> +check_column "false" Port_Binding up logical_port=sw1-port1
>>>> +
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 sw0,lr0,public
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +AS_BOX([Delete the VIF for sw0-port2 in hv2])
>>>> +
>>>> +# Presently when a port binding is released we are not
>>>> +# deleting its datapath from the local_datapaths if it
>>>> +# is not relevant anymore.
>>>> +
>>>> +as hv2 ovs-vsctl del-port hv2-vif2
>>>> +check ovn-nbctl --wait=hv sync
>>>> +check_column "false" Port_Binding up logical_port=sw0-port2
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 sw0,lr0,public
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +AS_BOX([Trigger a recompute])
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 ""
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +AS_BOX([Disconnect sw2 from lr2])
>>>> +
>>>> +check ovn-nbctl --wait=hv lsp-set-options sw2-lr2 router-port=lr2-
>>>> sw2xxx
>>>> +
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 ""
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,lr2,public
>>>> +
>>>> +AS_BOX([Reconnect sw2 to lr2 again])
>>>> +
>>>> +check ovn-nbctl --wait=hv lsp-set-options sw2-lr2 router-port=lr2-sw2
>>>> +
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 ""
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +AS_BOX([Create a VIF on gw2 for sw1-port1])
>>>> +
>>>> +as gw2
>>>> +ovs-vsctl -- add-port br-int gw2-vif2 -- \
>>>> + set interface gw2-vif2 external-ids:iface-id=sw1-port1 \
>>>> + options:tx_pcap=gw2/vif2-tx.pcap \
>>>> + options:rxq_pcap=gw2/vif2-rx.pcap \
>>>> + ofport-request=2
>>>> +
>>>> +wait_for_ports_up sw1-port1
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 ""
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +AS_BOX([Delete the VIF for sw1-port1 in gw2])
>>>> +
>>>> +as gw2 ovs-vsctl del-port gw2-vif2
>>>> +check ovn-nbctl --wait=hv sync
>>>> +check_column "false" Port_Binding up logical_port=sw1-port1
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 ""
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +AS_BOX([Create a logical port for public and bind it on hv2])
>>>> +# hv2 will only have public in its local datapaths.
>>>> +check ovn-nbctl lsp-add public public-p1
>>>> +
>>>> +as hv2
>>>> +ovs-vsctl -- add-port br-int hv2-vif3 -- \
>>>> + set interface hv2-vif3 external-ids:iface-id=public-p1 \
>>>> + options:tx_pcap=hv2/vif3-tx.pcap \
>>>> + options:rxq_pcap=hv2/vif3-rx.pcap \
>>>> + ofport-request=2
>>>> +
>>>> +wait_for_ports_up public-p1
>>>> +trigger_recompute
>>>> +
>>>> +check_local_datapath hv1 sw0,lr0,public
>>>> +check_local_datapath hv2 public
>>>> +check_local_datapath gw1 sw0,lr0,public
>>>> +check_local_datapath gw2 sw1,lr1,sw2,lr2,public
>>>> +
>>>> +OVN_CLEANUP([hv1], [hv2], [gw1], [gw2])
>>>> +AT_CLEANUP
>>>> +])
>>>> +
>>>> # Test when a load balancer is added to two logical switches
>>>> # which are local to different hypervisors.
>>>> OVN_FOR_EACH_NORTHD([
>>>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>>>> index 7097e4155c..5fa740cfb1 100644
>>>> --- a/tests/system-ovn.at
>>>> +++ b/tests/system-ovn.at
>>>> @@ -16672,6 +16672,13 @@ OVN_ROUTE_EQUAL([ovnvrf$vrf], [dnl
>>>> blackhole 10.42.10.10 proto ovn metric 1000
>>>> blackhole 172.16.1.150 proto ovn metric 1000])
>>>>
>>>> +# Before cleanup of hv1 ovn-controller, trigger a recompute
>>>> +# to cleanup the local datapaths. Otherwise, the test will fail.
>>>> +# This is because we don't remove a datapath from
>>>> +# the local_datapaths hmap while handling the port binding
>>>> +# changes incrementally yet.
>>>> +check ovn-appctl inc-engine/recompute
>>>> +
>>>> OVN_CLEANUP_CONTROLLER([hv1])
>>>>
>>>> # Ensure system resources are cleaned up.
>>>> @@ -16810,6 +16817,13 @@ OVN_ROUTE_V6_EQUAL([ovnvrf$vrf], [dnl
>>>> blackhole 2001:db8:1001::150 dev lo proto ovn metric 1000 pref medium
>>>> blackhole 2001:db8:3001::150 dev lo proto ovn metric 1000 pref
>>>> medium])
>>>>
>>>> +# Before cleanup of hv1 ovn-controller, trigger a recompute
>>>> +# to cleanup the local datapaths. Otherwise, the test will fail.
>>>> +# This is because we don't remove a datapath from
>>>> +# the local_datapaths hmap while handling the port binding
>>>> +# changes incrementally yet.
>>>> +check ovn-appctl inc-engine/recompute
>>>> +
>>>> OVN_CLEANUP_CONTROLLER([hv1])
>>>>
>>>> # Ensure system resources are cleaned up.
>>>> @@ -17013,6 +17027,13 @@ blackhole 10.42.20.11 proto ovn metric 100
>>>> blackhole 172.16.1.10 proto ovn metric 1000
>>>> blackhole 172.16.1.11 proto ovn metric 1000])
>>>>
>>>> +# Before cleanup of hv1 ovn-controller, trigger a recompute
>>>> +# to cleanup the local datapaths. Otherwise, the test will fail.
>>>> +# This is because we don't remove a datapath from
>>>> +# the local_datapaths hmap while handling the port binding
>>>> +# changes incrementally yet.
>>>> +check ovn-appctl inc-engine/recompute
>>>> +
>>>> # Skip ls-join in flows comparison between I+P and recompute,
>>>> because R2 has multiple DGPs.
>>>> # This causes the following flows in sb
>>>> # table=xx(ls_in_l2_lkup ), priority=80 , match=(flags[1]
>>>> == 0 && arp.op == 1 && arp.tpa == 10.42.10.10), action=(outport =
>>>> "lsp-join-to-r2"; output;)
>>>> @@ -17220,6 +17241,13 @@ blackhole 2001:db8:1004::150 dev lo proto
>>>> ovn metric 1000 pref medium
>>>> blackhole 2001:db8:1004::151 dev lo proto ovn metric 1000 pref medium
>>>> blackhole 2001:db8:1005::150 dev lo proto ovn metric 100 pref
>>>> medium])
>>>>
>>>> +# Before cleanup of hv1 ovn-controller, trigger a recompute
>>>> +# to cleanup the local datapaths. Otherwise, the test will fail.
>>>> +# This is because we don't remove a datapath from
>>>> +# the local_datapaths hmap while handling the port binding
>>>> +# changes incrementally yet.
>>>> +check ovn-appctl inc-engine/recompute
>>>> +
>>>> OVN_CLEANUP_CONTROLLER([hv1], [], [], [ls-join])
>>>> # Ensure system resources are cleaned up.
>>>> AT_CHECK([ip link | grep -q ovnvrf1003:.*UP], [1])
>>>> --
>>>> 2.49.0
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
