diff mbox series

[ovs-dev] ovn-controller: processing matches precedes processing action in logical flow.

Message ID 20180202025029.14096-1-ligs@dtdream.com
State Accepted
Headers show
Series [ovs-dev] ovn-controller: processing matches precedes processing action in logical flow. | expand

Commit Message

Guoshuai Li Feb. 2, 2018, 2:50 a.m. UTC
This is to fix such a problem:
When the match field has "is_chassis_resident", and the chassis has no resident,
and the action has meter or group, the group/meter ID is assigned.

I hope to parse match field with the first, if there is no matches field,
we do not need to deal with the action field.

Signed-off-by: Guoshuai Li <ligs@dtdream.com>
---
 ovn/controller/lflow.c | 63 +++++++++++++++++++++++++++++---------------------
 tests/ovn.at           | 15 +++++++++++-
 2 files changed, 51 insertions(+), 27 deletions(-)

Comments

Ben Pfaff Feb. 2, 2018, 10:29 p.m. UTC | #1
On Fri, Feb 02, 2018 at 10:50:29AM +0800, Guoshuai Li wrote:
> This is to fix such a problem:
> When the match field has "is_chassis_resident", and the chassis has no resident,
> and the action has meter or group, the group/meter ID is assigned.
> 
> I hope to parse match field with the first, if there is no matches field,
> we do not need to deal with the action field.
> 
> Signed-off-by: Guoshuai Li <ligs@dtdream.com>

Thank you for noticing the problem and fixing it.  I applied this to
master.  I edited the commit message and I removed the code that logged
a warning for this case; I think that this is a normal situation that
doesn't warrant logging.

Thanks,

Ben.
diff mbox series

Patch

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 1e79a5355..0d4a3d6d5 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -254,31 +254,6 @@  consider_logical_flow(struct controller_ctx *ctx,
         return;
     }
 
-    /* Encode OVN logical actions into OpenFlow. */
-    uint64_t ofpacts_stub[1024 / 8];
-    struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
-    struct lookup_port_aux aux = {
-        .ovnsb_idl = ctx->ovnsb_idl,
-        .dp = lflow->logical_datapath
-    };
-    struct ovnact_encode_params ep = {
-        .lookup_port = lookup_port_cb,
-        .aux = &aux,
-        .is_switch = is_switch(ldp),
-        .is_gateway_router = is_gateway_router(ldp, local_datapaths),
-        .group_table = group_table,
-        .meter_table = meter_table,
-
-        .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
-        .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
-        .egress_ptable = OFTABLE_LOG_EGRESS_PIPELINE,
-        .output_ptable = output_ptable,
-        .mac_bind_ptable = OFTABLE_MAC_BINDING,
-    };
-    ovnacts_encode(ovnacts.data, ovnacts.size, &ep, &ofpacts);
-    ovnacts_free(ovnacts.data, ovnacts.size);
-    ofpbuf_uninit(&ovnacts);
-
     /* Translate OVN match into table of OpenFlow matches. */
     struct hmap matches;
     struct expr *expr;
@@ -296,11 +271,16 @@  consider_logical_flow(struct controller_ctx *ctx,
         VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s",
                      lflow->match, error);
         expr_destroy(prereqs);
-        ofpbuf_uninit(&ofpacts);
         free(error);
+        ovnacts_free(ovnacts.data, ovnacts.size);
+        ofpbuf_uninit(&ovnacts);
         return;
     }
 
+    struct lookup_port_aux aux = {
+        .ovnsb_idl = ctx->ovnsb_idl,
+        .dp = lflow->logical_datapath
+    };
     struct condition_aux cond_aux = { ctx->ovnsb_idl, chassis, active_tunnels,
                                       chassis_index};
     expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux);
@@ -309,6 +289,37 @@  consider_logical_flow(struct controller_ctx *ctx,
                                        &matches);
     expr_destroy(expr);
 
+    if (hmap_is_empty(&matches)) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_WARN_RL(&rl, " \"%s\": matches is empty", lflow->match);
+        free(error);
+        ovnacts_free(ovnacts.data, ovnacts.size);
+        ofpbuf_uninit(&ovnacts);
+        expr_matches_destroy(&matches);
+        return;
+    }
+
+    /* Encode OVN logical actions into OpenFlow. */
+    uint64_t ofpacts_stub[1024 / 8];
+    struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
+    struct ovnact_encode_params ep = {
+        .lookup_port = lookup_port_cb,
+        .aux = &aux,
+        .is_switch = is_switch(ldp),
+        .is_gateway_router = is_gateway_router(ldp, local_datapaths),
+        .group_table = group_table,
+        .meter_table = meter_table,
+
+        .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
+        .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
+        .egress_ptable = OFTABLE_LOG_EGRESS_PIPELINE,
+        .output_ptable = output_ptable,
+        .mac_bind_ptable = OFTABLE_MAC_BINDING,
+    };
+    ovnacts_encode(ovnacts.data, ovnacts.size, &ep, &ofpacts);
+    ovnacts_free(ovnacts.data, ovnacts.size);
+    ofpbuf_uninit(&ovnacts);
+
     /* Prepare the OpenFlow matches for adding to the flow table. */
     struct expr_match *m;
     HMAP_FOR_EACH (m, hmap_node, &matches) {
diff --git a/tests/ovn.at b/tests/ovn.at
index 734dc6c1d..1632a981b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5972,8 +5972,10 @@  ovn_start
 ovn-nbctl ls-add lsw0
 ovn-nbctl --wait=sb lsp-add lsw0 lp1
 ovn-nbctl --wait=sb lsp-add lsw0 lp2
+ovn-nbctl --wait=sb lsp-add lsw0 lp3
 ovn-nbctl lsp-set-addresses lp1 f0:00:00:00:00:01
 ovn-nbctl lsp-set-addresses lp2 f0:00:00:00:00:02
+ovn-nbctl lsp-set-addresses lp3 f0:00:00:00:00:03
 ovn-nbctl lsp-set-port-security lp1 f0:00:00:00:00:01
 ovn-nbctl lsp-set-port-security lp2 f0:00:00:00:00:02
 ovn-nbctl --wait=sb sync
@@ -6032,7 +6034,7 @@  AT_CHECK([get_final_nw_tos], [0], [none
 check_tos 0
 
 # Mark DSCP with a valid value
-qos_id=$(ovn-nbctl --wait=hv -- --id=@lp1-qos create QoS priority=100 action=dscp=48 match="inport\=\=\"lp1\"" direction="from-lport" -- set Logical_Switch lsw0 qos_rules=@lp1-qos)
+qos_id=$(ovn-nbctl --wait=hv -- --id=@lp1-qos create QoS priority=100 action=dscp=48 match="inport\=\=\"lp1\"\ &&\ is_chassis_resident(\"lp1\")" direction="from-lport" -- set Logical_Switch lsw0 qos_rules=@lp1-qos)
 AT_CHECK([as hv ovn-nbctl qos-list lsw0 | wc -l], [0], [1
 ])
 check_tos 48
@@ -6076,6 +6078,17 @@  check_tos 0
 AT_CHECK([as hv ovs-ofctl dump-flows br-int -O OpenFlow13 | grep meter | wc -l], [0], [0
 ])
 
+# check meter with chassis not resident
+ovn-nbctl qos-add lsw0 to-lport 1001 'inport=="lp3" && is_chassis_resident("lp3")' rate=11123 burst=111230
+AT_CHECK([as hv ovn-nbctl qos-list lsw0 | wc -l], [0], [1
+])
+
+# check no meter table
+AT_CHECK([as hv ovs-ofctl dump-flows br-int -O OpenFlow13 | grep meter | wc -l], [0], [0
+])
+AT_CHECK([as hv ovs-ofctl dump-meters br-int -O OpenFlow13 | grep rate=11123 | wc -l], [0], [0
+])
+
 OVN_CLEANUP([hv])
 AT_CLEANUP