| Message ID | 20210528192339.2707528-5-hzhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix ovn-controller I-P for multicast groups and lport changes | expand |
On 5/28/21 9:23 PM, Han Zhou wrote: > If a lflow has an lport name in the match, but when the lflow is > processed the port-binding is not seen by ovn-controller, the > corresponding openflow will not be created. Later if the port-binding is > created/monitored by ovn-controller, the lflow is not reprocessed > because the lflow didn't change and ovn-controller doesn't know that the > port-binding affects the lflow. This patch fixes the problem by tracking > the references when parsing the lflow, even if the port-binding is not > found when the lflow is firstly parsed. A test case is also added to > cover the scenario. > > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- > controller/lflow.c | 66 +++++++++++++++++++++++++++---------- > controller/lflow.h | 3 ++ > controller/ovn-controller.c | 17 ++++++++-- > tests/ovn.at | 48 +++++++++++++++++++++++++++ > 4 files changed, 114 insertions(+), 20 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 0abb6eb96..fa102e8d0 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -61,6 +61,7 @@ struct lookup_port_aux { > > struct condition_aux { > struct ovsdb_idl_index *sbrec_port_binding_by_name; > + const struct sbrec_datapath_binding *dp; > const struct sbrec_chassis *chassis; > const struct sset *active_tunnels; > const struct sbrec_logical_flow *lflow; > @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) > > const struct lookup_port_aux *aux = aux_; > > + /* Store the name that used to lookup the lport to lflow reference, so that > + * in the future when the lport's port binding changes, the logical flow > + * that references this lport can be reprocessed. */ > + lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name, > + &aux->lflow->header_.uuid); > + > const struct sbrec_port_binding *pb > = lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name); > if (pb && pb->datapath == aux->dp) { > @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name) > { > const struct condition_aux *c_aux = c_aux_; > > + /* Store the port name that used to lookup the lport to lflow reference, so > + * that in the future when the lport's port-binding changes the logical > + * flow that references this lport can be reprocessed. */ > + lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name, > + &c_aux->lflow->header_.uuid); > + > const struct sbrec_port_binding *pb > = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name); > if (!pb) { > return false; > } > > - /* Store the port_name to lflow reference. */ > - int64_t dp_id = pb->datapath->tunnel_key; > - char buf[16]; > - get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf)); > - lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf, > - &c_aux->lflow->header_.uuid); > - > if (strcmp(pb->type, "chassisredirect")) { > /* for non-chassisredirect ports */ > return pb->chassis && pb->chassis == c_aux->chassis; > @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > int64_t dp_id = dp->tunnel_key; > char buf[16]; > get_unique_lport_key(dp_id, port_id, buf, sizeof(buf)); > - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf, > - &lflow->header_.uuid); > if (!sset_contains(l_ctx_in->local_lport_ids, buf)) { > VLOG_DBG("lflow "UUID_FMT > " port %s in match is not local, skip", > @@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, > }; > struct condition_aux cond_aux = { > .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name, > + .dp = dp, > .chassis = l_ctx_in->chassis, > .active_tunnels = l_ctx_in->active_tunnels, > .lflow = lflow, > @@ -883,7 +888,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, > > /* Cache new entry if caching is enabled. */ > if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) { > - if (cached_expr && !is_cr_cond_present) { > + if (cached_expr > + && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table, > + &lflow->header_.uuid)) { > lflow_cache_add_matches(l_ctx_out->lflow_cache, > &lflow->header_.uuid, matches, > matches_size); > @@ -1746,19 +1753,42 @@ lflow_processing_end: > return handled; > } > > +/* Handles a port-binding change that is possibly related to a lport's > + * residence status on this chassis. */ > bool > lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out) > { > - bool changed; > - int64_t dp_id = pb->datapath->tunnel_key; > - char pb_ref_name[16]; > - get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name, > - sizeof(pb_ref_name)); > - > - return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name, > - l_ctx_in, l_ctx_out, &changed); > + bool changed, ret = true; Same nit as in patch 1, I'd personally split this in two declarations. > + > + if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port, > + l_ctx_in, l_ctx_out, &changed)) { > + ret = false; > + } > + return ret; > +} > + > +/* Handles port-binding add/deletions. */ > +bool > +lflow_handle_port_binding_add_del(struct lflow_ctx_in *l_ctx_in, > + struct lflow_ctx_out *l_ctx_out) For consistency with other functions that perform I-P in lflow.c we should probably rename this to lflow_handle_changed_port_bindings(). > +{ > + bool changed, ret = true; Same nit as in patch 1, I'd personally split this in two declarations. > + const struct sbrec_port_binding *pb; > + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > + l_ctx_in->port_binding_table) { > + if (!sbrec_port_binding_is_new(pb) > + && !sbrec_port_binding_is_deleted(pb)) { > + continue; > + } > + if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port, > + l_ctx_in, l_ctx_out, &changed)) { > + ret = false; > + break; > + } > + } > + return ret; > } > > bool > diff --git a/controller/lflow.h b/controller/lflow.h > index 07263ff47..1ae32b9c5 100644 > --- a/controller/lflow.h > +++ b/controller/lflow.h > @@ -130,6 +130,7 @@ struct lflow_ctx_in { > struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath; > struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group; > struct ovsdb_idl_index *sbrec_port_binding_by_name; > + const struct sbrec_port_binding_table *port_binding_table; > const struct sbrec_dhcp_options_table *dhcp_options_table; > const struct sbrec_dhcpv6_options_table *dhcpv6_options_table; > const struct sbrec_datapath_binding_table *dp_binding_table; > @@ -180,6 +181,8 @@ bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *, > bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *, > struct lflow_ctx_in *, > struct lflow_ctx_out *); > +bool lflow_handle_port_binding_add_del(struct lflow_ctx_in *, > + struct lflow_ctx_out *); > bool lflow_handle_mc_group_changes(struct lflow_ctx_in *, > struct lflow_ctx_out *); > #endif /* controller/lflow.h */ > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 949137c0a..6566f44be 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2000,6 +2000,10 @@ static void init_lflow_ctx(struct engine_node *node, > engine_get_input("SB_multicast_group", node), > "name_datapath"); > > + struct sbrec_port_binding_table *port_binding_table = > + (struct sbrec_port_binding_table *)EN_OVSDB_GET( > + engine_get_input("SB_port_binding", node)); > + > struct sbrec_dhcp_options_table *dhcp_table = > (struct sbrec_dhcp_options_table *)EN_OVSDB_GET( > engine_get_input("SB_dhcp_options", node)); > @@ -2063,6 +2067,7 @@ static void init_lflow_ctx(struct engine_node *node, > l_ctx_in->sbrec_logical_flow_by_logical_dp_group = > sbrec_logical_flow_by_dp_group; > l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name; > + l_ctx_in->port_binding_table = port_binding_table; > l_ctx_in->dhcp_options_table = dhcp_table; > l_ctx_in->dhcpv6_options_table = dhcpv6_table; > l_ctx_in->mac_binding_table = mac_binding_table; > @@ -2253,6 +2258,13 @@ flow_output_sb_port_binding_handler(struct engine_node *node, > struct ed_type_flow_output *fo = data; > struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > + struct lflow_ctx_in l_ctx_in; > + struct lflow_ctx_out l_ctx_out; > + init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); > + if (!lflow_handle_port_binding_add_del(&l_ctx_in, &l_ctx_out)) { > + return false; > + } > + > struct physical_ctx p_ctx; > init_physical_ctx(node, rt_data, &p_ctx); > > @@ -2356,8 +2368,9 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > break; > > case REF_TYPE_PORTBINDING: > - /* This ref type is handled in the > - * flow_output_runtime_data_handler. */ > + /* This ref type is handled in: > + * - flow_output_runtime_data_handler > + * - flow_output_sb_port_binding_handler. */ > case REF_TYPE_MC_GROUP: > /* This ref type is handled in the > * flow_output_sb_multicast_group_handler. */ > diff --git a/tests/ovn.at b/tests/ovn.at > index a21dbadac..5efd32d82 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -26749,3 +26749,51 @@ AT_CHECK([ovs-ofctl dump-flows br-int "nw_src=10.0.0.0/24" | \ > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +# Test when a logical port name is used in an ACL before it is created. When it > +# is created later, the ACL should be programmed as openflow rules by > +# ovn-controller. Although this is not likely to happen in real world use > +# cases, it is possible that a port-binding is observed by ovn-controller AFTER > +# an lflow that references the port is processed. So this test case is to make > +# sure the incremental processing in ovn-controller reprocesses the lflow when > +# the port-binding is observed. > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn -- ACL referencing lport before creation]) > +ovn_start > + > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +# Bind both lp1 and lp2 on the chasis. > +ovs-vsctl add-port br-int lp1 -- set interface lp1 external_ids:iface-id=lp1 > +ovs-vsctl add-port br-int lp2 -- set interface lp2 external_ids:iface-id=lp2 It's, probably, good to prepend "check " to all ovs-*ctl/ovn-*ctl calls. They might catch unexpected issues in the future. > + > +# Create only lport lp1, but not lp2. > +ovn-nbctl ls-add lsw0 > +ovn-nbctl lsp-add lsw0 lp1 \ > + -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11" > + > +# Each lport is referenced by an ACL. > +ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" && ip4.src == 10.0.0.111' allow-related > +ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" && ip4.src == 10.0.0.222' allow-related > + > +# The first ACL should be programmed. > +check ovn-nbctl --wait=hv sync > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111], [0], [ignore]) > + > +# Now create the lport lp2. > +ovn-nbctl ls-add lsw0 > +ovn-nbctl lsp-add lsw0 lp2 \ > + -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22" > + > +check ovn-nbctl --wait=hv sync > +# Now the second ACL should be programmed. > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222], [0], [ignore]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > Thanks, Dumitru
On Fri, Jun 4, 2021 at 6:56 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 5/28/21 9:23 PM, Han Zhou wrote: > > If a lflow has an lport name in the match, but when the lflow is > > processed the port-binding is not seen by ovn-controller, the > > corresponding openflow will not be created. Later if the port-binding is > > created/monitored by ovn-controller, the lflow is not reprocessed > > because the lflow didn't change and ovn-controller doesn't know that the > > port-binding affects the lflow. This patch fixes the problem by tracking > > the references when parsing the lflow, even if the port-binding is not > > found when the lflow is firstly parsed. A test case is also added to > > cover the scenario. > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > controller/lflow.c | 66 +++++++++++++++++++++++++++---------- > > controller/lflow.h | 3 ++ > > controller/ovn-controller.c | 17 ++++++++-- > > tests/ovn.at | 48 +++++++++++++++++++++++++++ > > 4 files changed, 114 insertions(+), 20 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 0abb6eb96..fa102e8d0 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -61,6 +61,7 @@ struct lookup_port_aux { > > > > struct condition_aux { > > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > + const struct sbrec_datapath_binding *dp; > > const struct sbrec_chassis *chassis; > > const struct sset *active_tunnels; > > const struct sbrec_logical_flow *lflow; > > @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) > > > > const struct lookup_port_aux *aux = aux_; > > > > + /* Store the name that used to lookup the lport to lflow reference, so that > > + * in the future when the lport's port binding changes, the logical flow > > + * that references this lport can be reprocessed. */ > > + lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name, > > + &aux->lflow->header_.uuid); > > + > > const struct sbrec_port_binding *pb > > = lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name); > > if (pb && pb->datapath == aux->dp) { > > @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name) > > { > > const struct condition_aux *c_aux = c_aux_; > > > > + /* Store the port name that used to lookup the lport to lflow reference, so > > + * that in the future when the lport's port-binding changes the logical > > + * flow that references this lport can be reprocessed. */ > > + lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name, > > + &c_aux->lflow->header_.uuid); > > + > > const struct sbrec_port_binding *pb > > = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name); > > if (!pb) { > > return false; > > } > > > > - /* Store the port_name to lflow reference. */ > > - int64_t dp_id = pb->datapath->tunnel_key; > > - char buf[16]; > > - get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf)); > > - lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf, > > - &c_aux->lflow->header_.uuid); > > - > > if (strcmp(pb->type, "chassisredirect")) { > > /* for non-chassisredirect ports */ > > return pb->chassis && pb->chassis == c_aux->chassis; > > @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > > int64_t dp_id = dp->tunnel_key; > > char buf[16]; > > get_unique_lport_key(dp_id, port_id, buf, sizeof(buf)); > > - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf, > > - &lflow->header_.uuid); > > if (!sset_contains(l_ctx_in->local_lport_ids, buf)) { > > VLOG_DBG("lflow "UUID_FMT > > " port %s in match is not local, skip", > > @@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, > > }; > > struct condition_aux cond_aux = { > > .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name, > > + .dp = dp, > > .chassis = l_ctx_in->chassis, > > .active_tunnels = l_ctx_in->active_tunnels, > > .lflow = lflow, > > @@ -883,7 +888,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, > > > > /* Cache new entry if caching is enabled. */ > > if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) { > > - if (cached_expr && !is_cr_cond_present) { > > + if (cached_expr > > + && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table, > > + &lflow->header_.uuid)) { > > lflow_cache_add_matches(l_ctx_out->lflow_cache, > > &lflow->header_.uuid, matches, > > matches_size); > > @@ -1746,19 +1753,42 @@ lflow_processing_end: > > return handled; > > } > > > > +/* Handles a port-binding change that is possibly related to a lport's > > + * residence status on this chassis. */ > > bool > > lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb, > > struct lflow_ctx_in *l_ctx_in, > > struct lflow_ctx_out *l_ctx_out) > > { > > - bool changed; > > - int64_t dp_id = pb->datapath->tunnel_key; > > - char pb_ref_name[16]; > > - get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name, > > - sizeof(pb_ref_name)); > > - > > - return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name, > > - l_ctx_in, l_ctx_out, &changed); > > + bool changed, ret = true; > > Same nit as in patch 1, I'd personally split this in two declarations. > Ack > > + > > + if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port, > > + l_ctx_in, l_ctx_out, &changed)) { > > + ret = false; > > + } > > + return ret; > > +} > > + > > +/* Handles port-binding add/deletions. */ > > +bool > > +lflow_handle_port_binding_add_del(struct lflow_ctx_in *l_ctx_in, > > + struct lflow_ctx_out *l_ctx_out) > > For consistency with other functions that perform I-P in lflow.c we > should probably rename this to lflow_handle_changed_port_bindings(). Ack > > > +{ > > + bool changed, ret = true; > > Same nit as in patch 1, I'd personally split this in two declarations. Ack > > > + const struct sbrec_port_binding *pb; > > + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > > + l_ctx_in->port_binding_table) { > > + if (!sbrec_port_binding_is_new(pb) > > + && !sbrec_port_binding_is_deleted(pb)) { > > + continue; > > + } > > + if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port, > > + l_ctx_in, l_ctx_out, &changed)) { > > + ret = false; > > + break; > > + } > > + } > > + return ret; > > } > > > > bool > > diff --git a/controller/lflow.h b/controller/lflow.h > > index 07263ff47..1ae32b9c5 100644 > > --- a/controller/lflow.h > > +++ b/controller/lflow.h > > @@ -130,6 +130,7 @@ struct lflow_ctx_in { > > struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath; > > struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group; > > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > + const struct sbrec_port_binding_table *port_binding_table; > > const struct sbrec_dhcp_options_table *dhcp_options_table; > > const struct sbrec_dhcpv6_options_table *dhcpv6_options_table; > > const struct sbrec_datapath_binding_table *dp_binding_table; > > @@ -180,6 +181,8 @@ bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *, > > bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *, > > struct lflow_ctx_in *, > > struct lflow_ctx_out *); > > +bool lflow_handle_port_binding_add_del(struct lflow_ctx_in *, > > + struct lflow_ctx_out *); > > bool lflow_handle_mc_group_changes(struct lflow_ctx_in *, > > struct lflow_ctx_out *); > > #endif /* controller/lflow.h */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 949137c0a..6566f44be 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -2000,6 +2000,10 @@ static void init_lflow_ctx(struct engine_node *node, > > engine_get_input("SB_multicast_group", node), > > "name_datapath"); > > > > + struct sbrec_port_binding_table *port_binding_table = > > + (struct sbrec_port_binding_table *)EN_OVSDB_GET( > > + engine_get_input("SB_port_binding", node)); > > + > > struct sbrec_dhcp_options_table *dhcp_table = > > (struct sbrec_dhcp_options_table *)EN_OVSDB_GET( > > engine_get_input("SB_dhcp_options", node)); > > @@ -2063,6 +2067,7 @@ static void init_lflow_ctx(struct engine_node *node, > > l_ctx_in->sbrec_logical_flow_by_logical_dp_group = > > sbrec_logical_flow_by_dp_group; > > l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name; > > + l_ctx_in->port_binding_table = port_binding_table; > > l_ctx_in->dhcp_options_table = dhcp_table; > > l_ctx_in->dhcpv6_options_table = dhcpv6_table; > > l_ctx_in->mac_binding_table = mac_binding_table; > > @@ -2253,6 +2258,13 @@ flow_output_sb_port_binding_handler(struct engine_node *node, > > struct ed_type_flow_output *fo = data; > > struct ovn_desired_flow_table *flow_table = &fo->flow_table; > > > > + struct lflow_ctx_in l_ctx_in; > > + struct lflow_ctx_out l_ctx_out; > > + init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); > > + if (!lflow_handle_port_binding_add_del(&l_ctx_in, &l_ctx_out)) { > > + return false; > > + } > > + > > struct physical_ctx p_ctx; > > init_physical_ctx(node, rt_data, &p_ctx); > > > > @@ -2356,8 +2368,9 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, > > break; > > > > case REF_TYPE_PORTBINDING: > > - /* This ref type is handled in the > > - * flow_output_runtime_data_handler. */ > > + /* This ref type is handled in: > > + * - flow_output_runtime_data_handler > > + * - flow_output_sb_port_binding_handler. */ > > case REF_TYPE_MC_GROUP: > > /* This ref type is handled in the > > * flow_output_sb_multicast_group_handler. */ > > diff --git a/tests/ovn.at b/tests/ovn.at > > index a21dbadac..5efd32d82 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -26749,3 +26749,51 @@ AT_CHECK([ovs-ofctl dump-flows br-int "nw_src= 10.0.0.0/24" | \ > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > ]) > > + > > +# Test when a logical port name is used in an ACL before it is created. When it > > +# is created later, the ACL should be programmed as openflow rules by > > +# ovn-controller. Although this is not likely to happen in real world use > > +# cases, it is possible that a port-binding is observed by ovn-controller AFTER > > +# an lflow that references the port is processed. So this test case is to make > > +# sure the incremental processing in ovn-controller reprocesses the lflow when > > +# the port-binding is observed. > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- ACL referencing lport before creation]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > + > > +# Bind both lp1 and lp2 on the chasis. > > +ovs-vsctl add-port br-int lp1 -- set interface lp1 external_ids:iface-id=lp1 > > +ovs-vsctl add-port br-int lp2 -- set interface lp2 external_ids:iface-id=lp2 > > It's, probably, good to prepend "check " to all ovs-*ctl/ovn-*ctl calls. > They might catch unexpected issues in the future. Ack. Thanks, Han > > > + > > +# Create only lport lp1, but not lp2. > > +ovn-nbctl ls-add lsw0 > > +ovn-nbctl lsp-add lsw0 lp1 \ > > + -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11" > > + > > +# Each lport is referenced by an ACL. > > +ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" && ip4.src == 10.0.0.111' allow-related > > +ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" && ip4.src == 10.0.0.222' allow-related > > + > > +# The first ACL should be programmed. > > +check ovn-nbctl --wait=hv sync > > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111], [0], [ignore]) > > + > > +# Now create the lport lp2. > > +ovn-nbctl ls-add lsw0 > > +ovn-nbctl lsp-add lsw0 lp2 \ > > + -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22" > > + > > +check ovn-nbctl --wait=hv sync > > +# Now the second ACL should be programmed. > > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222], [0], [ignore]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > > > Thanks, > Dumitru >
diff --git a/controller/lflow.c b/controller/lflow.c index 0abb6eb96..fa102e8d0 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -61,6 +61,7 @@ struct lookup_port_aux { struct condition_aux { struct ovsdb_idl_index *sbrec_port_binding_by_name; + const struct sbrec_datapath_binding *dp; const struct sbrec_chassis *chassis; const struct sset *active_tunnels; const struct sbrec_logical_flow *lflow; @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) const struct lookup_port_aux *aux = aux_; + /* Store the name that used to lookup the lport to lflow reference, so that + * in the future when the lport's port binding changes, the logical flow + * that references this lport can be reprocessed. */ + lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name, + &aux->lflow->header_.uuid); + const struct sbrec_port_binding *pb = lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name); if (pb && pb->datapath == aux->dp) { @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name) { const struct condition_aux *c_aux = c_aux_; + /* Store the port name that used to lookup the lport to lflow reference, so + * that in the future when the lport's port-binding changes the logical + * flow that references this lport can be reprocessed. */ + lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name, + &c_aux->lflow->header_.uuid); + const struct sbrec_port_binding *pb = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name); if (!pb) { return false; } - /* Store the port_name to lflow reference. */ - int64_t dp_id = pb->datapath->tunnel_key; - char buf[16]; - get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf)); - lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf, - &c_aux->lflow->header_.uuid); - if (strcmp(pb->type, "chassisredirect")) { /* for non-chassisredirect ports */ return pb->chassis && pb->chassis == c_aux->chassis; @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, int64_t dp_id = dp->tunnel_key; char buf[16]; get_unique_lport_key(dp_id, port_id, buf, sizeof(buf)); - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf, - &lflow->header_.uuid); if (!sset_contains(l_ctx_in->local_lport_ids, buf)) { VLOG_DBG("lflow "UUID_FMT " port %s in match is not local, skip", @@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, }; struct condition_aux cond_aux = { .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name, + .dp = dp, .chassis = l_ctx_in->chassis, .active_tunnels = l_ctx_in->active_tunnels, .lflow = lflow, @@ -883,7 +888,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, /* Cache new entry if caching is enabled. */ if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) { - if (cached_expr && !is_cr_cond_present) { + if (cached_expr + && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table, + &lflow->header_.uuid)) { lflow_cache_add_matches(l_ctx_out->lflow_cache, &lflow->header_.uuid, matches, matches_size); @@ -1746,19 +1753,42 @@ lflow_processing_end: return handled; } +/* Handles a port-binding change that is possibly related to a lport's + * residence status on this chassis. */ bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) { - bool changed; - int64_t dp_id = pb->datapath->tunnel_key; - char pb_ref_name[16]; - get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name, - sizeof(pb_ref_name)); - - return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name, - l_ctx_in, l_ctx_out, &changed); + bool changed, ret = true; + + if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port, + l_ctx_in, l_ctx_out, &changed)) { + ret = false; + } + return ret; +} + +/* Handles port-binding add/deletions. */ +bool +lflow_handle_port_binding_add_del(struct lflow_ctx_in *l_ctx_in, + struct lflow_ctx_out *l_ctx_out) +{ + bool changed, ret = true; + const struct sbrec_port_binding *pb; + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, + l_ctx_in->port_binding_table) { + if (!sbrec_port_binding_is_new(pb) + && !sbrec_port_binding_is_deleted(pb)) { + continue; + } + if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port, + l_ctx_in, l_ctx_out, &changed)) { + ret = false; + break; + } + } + return ret; } bool diff --git a/controller/lflow.h b/controller/lflow.h index 07263ff47..1ae32b9c5 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -130,6 +130,7 @@ struct lflow_ctx_in { struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath; struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group; struct ovsdb_idl_index *sbrec_port_binding_by_name; + const struct sbrec_port_binding_table *port_binding_table; const struct sbrec_dhcp_options_table *dhcp_options_table; const struct sbrec_dhcpv6_options_table *dhcpv6_options_table; const struct sbrec_datapath_binding_table *dp_binding_table; @@ -180,6 +181,8 @@ bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *, bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *, struct lflow_ctx_in *, struct lflow_ctx_out *); +bool lflow_handle_port_binding_add_del(struct lflow_ctx_in *, + struct lflow_ctx_out *); bool lflow_handle_mc_group_changes(struct lflow_ctx_in *, struct lflow_ctx_out *); #endif /* controller/lflow.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 949137c0a..6566f44be 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -2000,6 +2000,10 @@ static void init_lflow_ctx(struct engine_node *node, engine_get_input("SB_multicast_group", node), "name_datapath"); + struct sbrec_port_binding_table *port_binding_table = + (struct sbrec_port_binding_table *)EN_OVSDB_GET( + engine_get_input("SB_port_binding", node)); + struct sbrec_dhcp_options_table *dhcp_table = (struct sbrec_dhcp_options_table *)EN_OVSDB_GET( engine_get_input("SB_dhcp_options", node)); @@ -2063,6 +2067,7 @@ static void init_lflow_ctx(struct engine_node *node, l_ctx_in->sbrec_logical_flow_by_logical_dp_group = sbrec_logical_flow_by_dp_group; l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name; + l_ctx_in->port_binding_table = port_binding_table; l_ctx_in->dhcp_options_table = dhcp_table; l_ctx_in->dhcpv6_options_table = dhcpv6_table; l_ctx_in->mac_binding_table = mac_binding_table; @@ -2253,6 +2258,13 @@ flow_output_sb_port_binding_handler(struct engine_node *node, struct ed_type_flow_output *fo = data; struct ovn_desired_flow_table *flow_table = &fo->flow_table; + struct lflow_ctx_in l_ctx_in; + struct lflow_ctx_out l_ctx_out; + init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); + if (!lflow_handle_port_binding_add_del(&l_ctx_in, &l_ctx_out)) { + return false; + } + struct physical_ctx p_ctx; init_physical_ctx(node, rt_data, &p_ctx); @@ -2356,8 +2368,9 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, break; case REF_TYPE_PORTBINDING: - /* This ref type is handled in the - * flow_output_runtime_data_handler. */ + /* This ref type is handled in: + * - flow_output_runtime_data_handler + * - flow_output_sb_port_binding_handler. */ case REF_TYPE_MC_GROUP: /* This ref type is handled in the * flow_output_sb_multicast_group_handler. */ diff --git a/tests/ovn.at b/tests/ovn.at index a21dbadac..5efd32d82 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -26749,3 +26749,51 @@ AT_CHECK([ovs-ofctl dump-flows br-int "nw_src=10.0.0.0/24" | \ OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +# Test when a logical port name is used in an ACL before it is created. When it +# is created later, the ACL should be programmed as openflow rules by +# ovn-controller. Although this is not likely to happen in real world use +# cases, it is possible that a port-binding is observed by ovn-controller AFTER +# an lflow that references the port is processed. So this test case is to make +# sure the incremental processing in ovn-controller reprocesses the lflow when +# the port-binding is observed. +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn -- ACL referencing lport before creation]) +ovn_start + +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +# Bind both lp1 and lp2 on the chasis. +ovs-vsctl add-port br-int lp1 -- set interface lp1 external_ids:iface-id=lp1 +ovs-vsctl add-port br-int lp2 -- set interface lp2 external_ids:iface-id=lp2 + +# Create only lport lp1, but not lp2. +ovn-nbctl ls-add lsw0 +ovn-nbctl lsp-add lsw0 lp1 \ + -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11" + +# Each lport is referenced by an ACL. +ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" && ip4.src == 10.0.0.111' allow-related +ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" && ip4.src == 10.0.0.222' allow-related + +# The first ACL should be programmed. +check ovn-nbctl --wait=hv sync +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111], [0], [ignore]) + +# Now create the lport lp2. +ovn-nbctl ls-add lsw0 +ovn-nbctl lsp-add lsw0 lp2 \ + -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22" + +check ovn-nbctl --wait=hv sync +# Now the second ACL should be programmed. +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222], [0], [ignore]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +])
If a lflow has an lport name in the match, but when the lflow is processed the port-binding is not seen by ovn-controller, the corresponding openflow will not be created. Later if the port-binding is created/monitored by ovn-controller, the lflow is not reprocessed because the lflow didn't change and ovn-controller doesn't know that the port-binding affects the lflow. This patch fixes the problem by tracking the references when parsing the lflow, even if the port-binding is not found when the lflow is firstly parsed. A test case is also added to cover the scenario. Signed-off-by: Han Zhou <hzhou@ovn.org> --- controller/lflow.c | 66 +++++++++++++++++++++++++++---------- controller/lflow.h | 3 ++ controller/ovn-controller.c | 17 ++++++++-- tests/ovn.at | 48 +++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 20 deletions(-)