[ovs-dev] [PATCH ovn] controller: Fall back to full recompute of pflow_output for vtep lport changes.

[Odintsov Vladislav]

Hi Numan,

for me this patch solves the issue.
Actually, it’s a good point about adding test for ovn-controller-vtep to eliminate this problem in future.
However there was another problem with HW VTEP support in OVN with using stateful services in the datapath.

I’ve sent a patch series with a test, which reproduces the mentioned problem. Bigfix is in a third patch. Please try those patches:
https://patchwork.ozlabs.org/project/ovn/cover/20210916000624.1609-1-odivlad@gmail.com/

Tested-by: Vladislav Odintsov <odivlad at gmail.com<mailto:odivlad at gmail.com>>

Regards,
Vladislav Odintsov

On 8 Sep 2021, at 20:14, Numan Siddique <numans at ovn.org<mailto:numans at ovn.org>> wrote:

On Wed, Sep 8, 2021 at 5:59 AM Mark Gray <mark.d.gray at redhat.com<mailto:mark.d.gray at redhat.com>> wrote:

On 07/09/2021 18:15, numans at ovn.org<mailto:numans at ovn.org> wrote:
From: Numan Siddique <numans at ovn.org<mailto:numans at ovn.org>>

When a vtep logical port changes, necessary flows are not added as
expected.  This is because the function physical_handle_flows_for_lport()
in physical.c does not add flows required for vtep logical ports.
These flows are added by physical_run(). So fall back to full recompute
of pflow_output engine node when vtep lports change.

Reported-by: Odintsov Vladislav <VlOdintsov at croc.ru<mailto:VlOdintsov at croc.ru>>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387435.html
Signed-off-by: Numan Siddique <numans at ovn.org<mailto:numans at ovn.org>>
---
controller/ovn-controller.c | 11 ++++++++---
controller/physical.c       |  9 ++++++++-
controller/physical.h       |  2 +-
3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 0031a1035..d98ebbbfa 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2867,7 +2867,10 @@ pflow_output_sb_port_binding_handler(struct engine_node *node,
    const struct sbrec_port_binding *pb;
    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, p_ctx.port_binding_table) {
        bool removed = sbrec_port_binding_is_deleted(pb);
-        physical_handle_flows_for_lport(pb, removed, &p_ctx, &pfo->flow_table);
+        if (!physical_handle_flows_for_lport(pb, removed, &p_ctx,
+                                             &pfo->flow_table)) {
+            return false;
+        }
    }

    engine_set_node_state(node, EN_UPDATED);
@@ -2930,8 +2933,10 @@ pflow_output_runtime_data_handler(struct engine_node *node, void *data)
            struct tracked_lport *lport = shash_node->data;
            bool removed =
                lport->tracked_type == TRACKED_RESOURCE_REMOVED ? true: false;
-            physical_handle_flows_for_lport(lport->pb, removed, &p_ctx,
-                                            &pfo->flow_table);
+            if (!physical_handle_flows_for_lport(lport->pb, removed, &p_ctx,
+                                                 &pfo->flow_table)) {
+                return false;
+            }
        }
    }

diff --git a/controller/physical.c b/controller/physical.c
index 6f2c1cea0..ffb9f9952 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1509,11 +1509,16 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
    sset_destroy(&remote_chassis);
}

-void
+bool
physical_handle_flows_for_lport(const struct sbrec_port_binding *pb,
                                bool removed, struct physical_ctx *p_ctx,
                                struct ovn_desired_flow_table *flow_table)
{
+    if (!strcmp(pb->type, "vtep")) {
+        /* Cannot handle changes to vtep lports (yet). */
+        return false;
+    }
+
    ofctrl_remove_flows(flow_table, &pb->header_.uuid);

    if (!strcmp(pb->type, "external")) {
@@ -1553,6 +1558,8 @@ physical_handle_flows_for_lport(const struct sbrec_port_binding *pb,
                              p_ctx->chassis, flow_table, &ofpacts);
        ofpbuf_uninit(&ofpacts);
    }
+
+    return true;
}

void
diff --git a/controller/physical.h b/controller/physical.h
index c4540ad7f..ee4b1ae1f 100644
--- a/controller/physical.h
+++ b/controller/physical.h
@@ -65,7 +65,7 @@ void physical_run(struct physical_ctx *,
                  struct ovn_desired_flow_table *);
void physical_handle_mc_group_changes(struct physical_ctx *,
                                      struct ovn_desired_flow_table *);
-void physical_handle_flows_for_lport(const struct sbrec_port_binding *,
+bool physical_handle_flows_for_lport(const struct sbrec_port_binding *,
                                     bool removed,
                                     struct physical_ctx *,
                                     struct ovn_desired_flow_table *);


The change looks ok to me. I am not really sure how to test it though?
Any suggestions?

Thanks for the review.  Vladislav will test this patch out.  He confirmed to me
in the thread where he reported the issue.

Numan


_______________________________________________
dev mailing list
dev at openvswitch.org<mailto:dev at openvswitch.org>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

_______________________________________________
dev mailing list
dev at openvswitch.org<mailto:dev at openvswitch.org>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev