diff mbox series

[ovs-dev,2/5] controller: Nonvif related lports handling.

Message ID 20240423115333.2796843-3-xsimonar@redhat.com
State Accepted
Delegated to: Numan Siddique
Headers show
Series Fix I+P versus recompute differences. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Xavier Simonart April 23, 2024, 11:53 a.m. UTC
This patches fixes flows not properly deleted in scenarios similar to:
foo1 (hv1) - foo - R1 - join - R2 (chassis = hv2) - alice - alice1 (hv2).

When R2 is deleted, alice_R2 changed from l3gateway to patch, and, on hv1,
alice_R2 was added to related ports.
When R2 was added back (together with R2-alice and R2-join), alice_R2 changed
back from patch to l3gateway and alice_R2 remained in related_lports on hv1.

This is now fixed: a l3_gateway port is not a related_port if not for our chassis.

A test case has been modified to highlight the error, but also to
make the test easier to read (e.g avoid using same name for a port and
for a switch).

Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
---
 controller/binding.c | 12 ++++++++----
 tests/ovn.at         | 12 ++++++++----
 2 files changed, 16 insertions(+), 8 deletions(-)

Comments

Numan Siddique May 30, 2024, 3:27 p.m. UTC | #1
On Tue, Apr 23, 2024 at 7:54 AM Xavier Simonart <xsimonar@redhat.com> wrote:
>
> This patches fixes flows not properly deleted in scenarios similar to:
> foo1 (hv1) - foo - R1 - join - R2 (chassis = hv2) - alice - alice1 (hv2).
>
> When R2 is deleted, alice_R2 changed from l3gateway to patch, and, on hv1,
> alice_R2 was added to related ports.
> When R2 was added back (together with R2-alice and R2-join), alice_R2 changed
> back from patch to l3gateway and alice_R2 remained in related_lports on hv1.
>
> This is now fixed: a l3_gateway port is not a related_port if not for our chassis.
>
> A test case has been modified to highlight the error, but also to
> make the test easier to read (e.g avoid using same name for a port and
> for a switch).
>
> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>

Acked-by: Numan Siddique <numans@ovn.org>

Numan

> ---
>  controller/binding.c | 12 ++++++++----
>  tests/ovn.at         | 12 ++++++++----
>  2 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index c9658cb2a..3b36ed881 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1824,7 +1824,7 @@ consider_localport(const struct sbrec_port_binding *pb,
>   */
>  static bool
>  consider_nonvif_lport_(const struct sbrec_port_binding *pb,
> -                       bool our_chassis,
> +                       bool our_chassis, bool is_ha_chassis,
>                         struct binding_ctx_in *b_ctx_in,
>                         struct binding_ctx_out *b_ctx_out)
>  {
> @@ -1844,6 +1844,9 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb,
>                             b_ctx_out->if_mgr,
>                             b_ctx_out->postponed_ports);
>      }
> +    if (!is_ha_chassis) {
> +        remove_related_lport(pb, b_ctx_out);
> +    }
>
>      if (pb->chassis == b_ctx_in->chassis_rec
>              || is_additional_chassis(pb, b_ctx_in->chassis_rec)
> @@ -1867,7 +1870,7 @@ consider_l2gw_lport(const struct sbrec_port_binding *pb,
>      bool our_chassis = chassis_id && !strcmp(chassis_id,
>                                               b_ctx_in->chassis_rec->name);
>
> -    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out);
> +    return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
>  }
>
>  static bool
> @@ -1879,7 +1882,7 @@ consider_l3gw_lport(const struct sbrec_port_binding *pb,
>      bool our_chassis = chassis_id && !strcmp(chassis_id,
>                                               b_ctx_in->chassis_rec->name);
>
> -    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out);
> +    return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
>  }
>
>  static void
> @@ -1942,7 +1945,8 @@ consider_ha_lport(const struct sbrec_port_binding *pb,
>          update_related_lport(pb, b_ctx_out);
>      }
>
> -    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out);
> +    return consider_nonvif_lport_(pb, our_chassis, is_ha_chassis, b_ctx_in,
> +                                  b_ctx_out);
>  }
>
>  static bool
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f974cbb15..3c888aaf5 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -7781,9 +7781,9 @@ ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
>      type=router options:router-port=foo addresses=\"00:00:01:01:02:03\"
>
>  # Connect alice to R2
> -ovn-nbctl lrp-add R2 alice 00:00:02:01:02:03 172.16.1.1/24
> -ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port rp-alice \
> -    type=router options:router-port=alice addresses=\"00:00:02:01:02:03\"
> +ovn-nbctl lrp-add R2 R2-alice 00:00:02:01:02:03 172.16.1.1/24
> +ovn-nbctl lsp-add alice alice-R2 -- set Logical_Switch_Port alice-R2 \
> +    type=router options:router-port=R2-alice addresses=\"00:00:02:01:02:03\"
>
>  # Connect R1 to join
>  ovn-nbctl lrp-add R1 R1_join 00:00:04:01:02:03 20.0.0.1/24
> @@ -7871,11 +7871,13 @@ echo "----------------------------"
>  echo $expected > expected
>  OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
>
> +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1])
> +
>  # Delete the router and re-create it. Things should work as before.
>  ovn-nbctl  lr-del R2
>  ovn-nbctl create Logical_Router name=R2 options:chassis="hv2"
>  # Connect alice to R2
> -ovn-nbctl lrp-add R2 alice 00:00:02:01:02:03 172.16.1.1/24
> +ovn-nbctl lrp-add R2 R2-alice 00:00:02:01:02:03 172.16.1.1/24
>  # Connect R2 to join
>  ovn-nbctl lrp-add R2 R2_join 00:00:04:01:02:04 20.0.0.2/24
>
> @@ -7887,6 +7889,8 @@ R2 static_routes @lrt
>  wait_for_ports_up
>  check ovn-nbctl --wait=hv sync
>
> +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1])
> +
>  # Send the packet again.
>  as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet
>
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/controller/binding.c b/controller/binding.c
index c9658cb2a..3b36ed881 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1824,7 +1824,7 @@  consider_localport(const struct sbrec_port_binding *pb,
  */
 static bool
 consider_nonvif_lport_(const struct sbrec_port_binding *pb,
-                       bool our_chassis,
+                       bool our_chassis, bool is_ha_chassis,
                        struct binding_ctx_in *b_ctx_in,
                        struct binding_ctx_out *b_ctx_out)
 {
@@ -1844,6 +1844,9 @@  consider_nonvif_lport_(const struct sbrec_port_binding *pb,
                            b_ctx_out->if_mgr,
                            b_ctx_out->postponed_ports);
     }
+    if (!is_ha_chassis) {
+        remove_related_lport(pb, b_ctx_out);
+    }
 
     if (pb->chassis == b_ctx_in->chassis_rec
             || is_additional_chassis(pb, b_ctx_in->chassis_rec)
@@ -1867,7 +1870,7 @@  consider_l2gw_lport(const struct sbrec_port_binding *pb,
     bool our_chassis = chassis_id && !strcmp(chassis_id,
                                              b_ctx_in->chassis_rec->name);
 
-    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out);
+    return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
 }
 
 static bool
@@ -1879,7 +1882,7 @@  consider_l3gw_lport(const struct sbrec_port_binding *pb,
     bool our_chassis = chassis_id && !strcmp(chassis_id,
                                              b_ctx_in->chassis_rec->name);
 
-    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out);
+    return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
 }
 
 static void
@@ -1942,7 +1945,8 @@  consider_ha_lport(const struct sbrec_port_binding *pb,
         update_related_lport(pb, b_ctx_out);
     }
 
-    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out);
+    return consider_nonvif_lport_(pb, our_chassis, is_ha_chassis, b_ctx_in,
+                                  b_ctx_out);
 }
 
 static bool
diff --git a/tests/ovn.at b/tests/ovn.at
index f974cbb15..3c888aaf5 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -7781,9 +7781,9 @@  ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
     type=router options:router-port=foo addresses=\"00:00:01:01:02:03\"
 
 # Connect alice to R2
-ovn-nbctl lrp-add R2 alice 00:00:02:01:02:03 172.16.1.1/24
-ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port rp-alice \
-    type=router options:router-port=alice addresses=\"00:00:02:01:02:03\"
+ovn-nbctl lrp-add R2 R2-alice 00:00:02:01:02:03 172.16.1.1/24
+ovn-nbctl lsp-add alice alice-R2 -- set Logical_Switch_Port alice-R2 \
+    type=router options:router-port=R2-alice addresses=\"00:00:02:01:02:03\"
 
 # Connect R1 to join
 ovn-nbctl lrp-add R1 R1_join 00:00:04:01:02:03 20.0.0.1/24
@@ -7871,11 +7871,13 @@  echo "----------------------------"
 echo $expected > expected
 OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
 
+CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1])
+
 # Delete the router and re-create it. Things should work as before.
 ovn-nbctl  lr-del R2
 ovn-nbctl create Logical_Router name=R2 options:chassis="hv2"
 # Connect alice to R2
-ovn-nbctl lrp-add R2 alice 00:00:02:01:02:03 172.16.1.1/24
+ovn-nbctl lrp-add R2 R2-alice 00:00:02:01:02:03 172.16.1.1/24
 # Connect R2 to join
 ovn-nbctl lrp-add R2 R2_join 00:00:04:01:02:04 20.0.0.2/24
 
@@ -7887,6 +7889,8 @@  R2 static_routes @lrt
 wait_for_ports_up
 check ovn-nbctl --wait=hv sync
 
+CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1])
+
 # Send the packet again.
 as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet