| Message ID | 20221018091937.955476-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] ovn-controller: Fixed missing flows after interface deletion | expand |
| 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 |
Hi Xavier, I have a comment down below. On 10/18/22 05:19, Xavier Simonart wrote: > In the following scenario: > - interface "old" is created and external_ids:iface-id is set (to lp) > - interface "new" is created and external_ids:iface-id is set (to same lp) > - interface "old" is deleted > flows related to lp were deleted. > > Note that after "new" interface is created, flows use "new" ofport. > This patch only supports up to two ovs interfaces bound to the same iface-id. > > Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for pflow_output engine.") > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866 > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > v2: - Use bool instead of int for binding count to better reflect only > one additional binding is supported. > - Fix use after free. > - Remove debug logging from test case > --- > controller/binding.c | 85 +++++++++++++++++++++++++++++++---- > controller/binding.h | 1 + > tests/ovn.at | 105 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 182 insertions(+), 9 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index c3d2b2e42..b1dfb86b3 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -2155,8 +2155,12 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > if (!lbinding) { > lbinding = local_binding_create(iface_id, iface_rec); > local_binding_add(local_bindings, lbinding); > - } else { > + } else if (!lbinding->iface) { > + lbinding->iface = iface_rec; > + } else if (lbinding->iface != iface_rec) { I might be wrong, but it seems like this should also check that iface->multiple_bindings is false before entering this if block. Let's say you have logical switch port "lp". Someone does the following: 1) Create OVS interface lp0 with iface-id set to "lp". At this point, we claim the port and lbinding->multiple_interfaces == false. 2) Create OVS interface lp1 with iface-id set to "lp". At this point, we detect that the OVS interface is different, so we set lbinding->iface to lp1 and set lbinding->multiple_bindings to true. 3) Create OVS interface lp2 with iface-id set to "lp". Again, we detect that the OVS interace is different, so we set lbinding->iface to lp2. lbinding->multiple_bindings remains true. 4) Delete OVS interface lp2. At this point, consider_iface_release() will remove the current lbinding and will find either lp0 or lp1. For this example, we'll say that we find lp0. We create a new lbinding with lbinding->iface set to lp0 and lbinding->multiple_bindings is false. 5) Delete OVS interface lp0. Now consider_iface_release() will remove the current lbinding. Since lbinding->multiple_interfaces is false, there is no attempt to try to find an OVS interface to replace the old lbinding->iface. The result is that lp1 still exists but it's not bound anywhere. At this point you might be saying that this is expected, since you only intend to support the case where 2 interfaces have the same iface-id. However, I think that if the intent is to only support a maximum of 2 interfaces this way, then we should not update the lbinding in step (3) when interface lp2 is created, and we should print a warning. > lbinding->iface = iface_rec; > + lbinding->multiple_bindings = true; > + b_ctx_out->local_lports_changed = true; > } > > struct binding_lport *b_lport = > @@ -2219,13 +2223,23 @@ static bool > consider_iface_release(const struct ovsrec_interface *iface_rec, > const char *iface_id, > struct binding_ctx_in *b_ctx_in, > - struct binding_ctx_out *b_ctx_out) > + struct binding_ctx_out *b_ctx_out, > + bool *multiple_bindings) > { > struct local_binding *lbinding; > struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; > struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; > + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > > lbinding = local_binding_find(local_bindings, iface_id); > + if (lbinding && !lbinding->multiple_bindings && > + lbinding->iface != iface_rec && !ofport) { > + VLOG_DBG("Not releasing lport %s as %s was claimed " > + "and %s was never bound)", > + iface_id, lbinding->iface ? lbinding->iface->name:"", > + iface_rec->name); > + return true; > + } > struct binding_lport *b_lport = > local_binding_get_primary_or_localport_lport(lbinding); > if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) { > @@ -2254,8 +2268,10 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, > } > > if (lbinding) { > + *multiple_bindings = lbinding->multiple_bindings; > /* Clear the iface of the local binding. */ > lbinding->iface = NULL; > + lbinding->multiple_bindings = false; > } > > /* Check if the lbinding has children of type PB_CONTAINER. > @@ -2266,8 +2282,6 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, > } > > remove_local_lports(iface_id, b_ctx_out); > - smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > - > return true; > } > > @@ -2375,6 +2389,10 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > > bool handled = true; > > + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > + struct hmap *qos_map_ptr = > + sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > + > /* Run the tracked interfaces loop twice. One to handle deleted > * changes. And another to handle add/update changes. > * This will ensure correctness. > @@ -2435,8 +2453,59 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > } > > if (cleared_iface_id) { > + bool multiple_bindings = false; > handled = consider_iface_release(iface_rec, cleared_iface_id, > - b_ctx_in, b_ctx_out); > + b_ctx_in, b_ctx_out, > + &multiple_bindings); > + if (!handled) { > + break; > + } > + if (!multiple_bindings) { > + smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > + continue; > + } > + /* There is another OVS interface bound to this port. > + * First find its name > + */ > + VLOG_DBG("binding remaining for %s", cleared_iface_id); > + struct smap_node *node; > + bool found = false; > + SMAP_FOR_EACH (node, b_ctx_out->local_iface_ids) { > + if (strcmp(node->value, cleared_iface_id) || > + !strcmp(node->key, iface_rec->name)) { > + continue; > + } > + /* Then find its iface_rec */ > + char *other_iface_rec_name = node->key; > + const struct ovsrec_interface *other_iface_rec; > + OVSREC_INTERFACE_TABLE_FOR_EACH (other_iface_rec, > + b_ctx_in->iface_table) { > + if (!strcmp(other_iface_rec->name, other_iface_rec_name)) { > + found = true; > + break; > + } > + } > + if (found) { > + int64_t ofport = other_iface_rec->n_ofport ? > + *other_iface_rec->ofport : 0; > + if (cleared_iface_id && ofport > 0 && > + is_iface_in_int_bridge(other_iface_rec, > + b_ctx_in->br_int)) { > + handled = consider_iface_claim(other_iface_rec, > + cleared_iface_id, > + b_ctx_in, > + b_ctx_out, qos_map_ptr); > + } > + } > + break; > + } > + if (!found) { > + /* This is unexpected. Recompute to clean up */ > + VLOG_WARN("Did not find which OVS interface still bound to %s", > + cleared_iface_id); > + handled = false; > + } > + smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > } > > if (!handled) { > @@ -2448,13 +2517,10 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > /* This can happen if any non vif OVS interface is in the tracked > * list or if consider_iface_release() returned false. > * There is no need to process further. */ > + destroy_qos_map(&qos_map); > return false; > } > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > - struct hmap *qos_map_ptr = > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > - > /* > * We consider an OVS interface for claiming if the following > * 2 conditions are met: > @@ -3034,6 +3100,7 @@ local_binding_create(const char *name, const struct ovsrec_interface *iface) > struct local_binding *lbinding = xzalloc(sizeof *lbinding); > lbinding->name = xstrdup(name); > lbinding->iface = iface; > + lbinding->multiple_bindings = false; > ovs_list_init(&lbinding->binding_lports); > > return lbinding; > diff --git a/controller/binding.h b/controller/binding.h > index ad959a9e6..6c3a98b02 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -135,6 +135,7 @@ struct local_binding { > char *name; > const struct ovsrec_interface *iface; > struct ovs_list binding_lports; > + bool multiple_bindings; > }; > > struct local_binding_data { > diff --git a/tests/ovn.at b/tests/ovn.at > index f8b8db4df..d9b720a98 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -32975,3 +32975,108 @@ check ovn-nbctl --wait=hv sync > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical port]) > +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 > + > +get_flows() > +{ > + cookie=$1 > + ovs-ofctl dump-flows br-int | grep $cookie | > + sed -e 's/duration=[[0-9.]]*s, //g' | > + sed -e 's/idle_age=[[0-9]]*, //g' | > + sed -e 's/n_packets=[[0-9]]*, //g' | > + sed -e 's/n_bytes=[[0-9]]*, //g' > +} > + > +check ovn-nbctl ls-add ls > +check ovn-nbctl lsp-add ls lp > +check ovn-nbctl lsp-set-type lp localport > +check ovn-nbctl lsp-set-addresses lp "00:00:00:01:01:02 192.168.1.2" > + > +check ovn-nbctl lsp-add ls vm1 > +check ovn-nbctl lsp-set-addresses vm1 "00:00:00:01:01:11 192.168.1.11" > +check ovs-vsctl add-port br-int vm1 -- set interface vm1 type=internal external_ids:iface-id=vm1 > + > +check ovn-nbctl --wait=hv sync > + > +check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal > +check ovs-vsctl set interface lpold external_ids:iface-id=lp > + > +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns _uuid find port_binding logical_port=lp) != x]) > +echo ====================================================== > +echo === Flows after iface-id set for the old interface === > +echo ====================================================== > +COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut -d: -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//') > + > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpold) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" > +]) > +nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` > +echo $nb_flows "flows after iface-id set for old interface" > + > +echo ====================================================== > +echo === Flows after iface-id set for the new interface === > +echo ====================================================== > +check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal > +check ovs-vsctl set interface lpnew external_ids:iface-id=lp > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" > +]) > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) > +flows_lpnew=$(get_flows $COOKIE) > + > +echo ====================================================== > +echo ======= Flows after old interface is deleted ========= > +echo ====================================================== > +check ovs-vsctl del-port br-int lpold > +# We do not expect changes, so let's wait a few seconds to check nothing changed > +sleep 2 > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) > +flows_after_deletion=$(get_flows $COOKIE) > +check test "$flows_lpnew" = "$flows_after_deletion" > + > +echo ====================================================== > +echo ======= Flows after lptemp interface is created ==== > +echo ====================================================== > +check ovs-vsctl add-port br-int lptemp -- set Interface lptemp type=internal > +check ovs-vsctl set Interface lptemp external_ids:iface-id=lp > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lptemp) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" > +]) > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) > + > +echo ====================================================== > +echo ======= Flows after lptemp interface is deleted ====== > +echo ====================================================== > +check ovs-vsctl del-port br-int lptemp > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" > +]) > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) > +flows_after_deletion=$(get_flows $COOKIE) > +check test "$flows_lpnew" = "$flows_after_deletion" > + > +echo ====================================================== > +echo ======= Flows after new interface is deleted ========= > +echo ====================================================== > +check ovs-vsctl del-port br-int lpnew > +OVS_WAIT_UNTIL([ > + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` > + test "${nb_flows}" = 0 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +])
Hi Mark Thanks for your review. I agree that we need either to support this scenario with more than two interfaces bound to the same port, or warn. After some additional internal discussion, the idea is to support the scenario you pointed out, but use recompute, to avoid making the code even more complex. I'll send a v3. Thanks for the feedback On Tue, Oct 18, 2022 at 5:09 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Xavier, > > I have a comment down below. > > On 10/18/22 05:19, Xavier Simonart wrote: > > In the following scenario: > > - interface "old" is created and external_ids:iface-id is set (to lp) > > - interface "new" is created and external_ids:iface-id is set (to same > lp) > > - interface "old" is deleted > > flows related to lp were deleted. > > > > Note that after "new" interface is created, flows use "new" ofport. > > This patch only supports up to two ovs interfaces bound to the same > iface-id. > > > > Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for pflow_output > engine.") > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866 > > > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > > > --- > > v2: - Use bool instead of int for binding count to better reflect only > > one additional binding is supported. > > - Fix use after free. > > - Remove debug logging from test case > > --- > > controller/binding.c | 85 +++++++++++++++++++++++++++++++---- > > controller/binding.h | 1 + > > tests/ovn.at | 105 +++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 182 insertions(+), 9 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index c3d2b2e42..b1dfb86b3 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -2155,8 +2155,12 @@ consider_iface_claim(const struct > ovsrec_interface *iface_rec, > > if (!lbinding) { > > lbinding = local_binding_create(iface_id, iface_rec); > > local_binding_add(local_bindings, lbinding); > > - } else { > > + } else if (!lbinding->iface) { > > + lbinding->iface = iface_rec; > > + } else if (lbinding->iface != iface_rec) { > > I might be wrong, but it seems like this should also check that > iface->multiple_bindings is false before entering this if block. > > Let's say you have logical switch port "lp". Someone does the following: > > 1) Create OVS interface lp0 with iface-id set to "lp". At this point, we > claim the port and lbinding->multiple_interfaces == false. > > 2) Create OVS interface lp1 with iface-id set to "lp". At this point, we > detect that the OVS interface is different, so we set lbinding->iface to > lp1 and set lbinding->multiple_bindings to true. > > 3) Create OVS interface lp2 with iface-id set to "lp". Again, we detect > that the OVS interace is different, so we set lbinding->iface to lp2. > lbinding->multiple_bindings remains true. > > 4) Delete OVS interface lp2. At this point, consider_iface_release() > will remove the current lbinding and will find either lp0 or lp1. For > this example, we'll say that we find lp0. We create a new lbinding with > lbinding->iface set to lp0 and lbinding->multiple_bindings is false. > > 5) Delete OVS interface lp0. Now consider_iface_release() will remove > the current lbinding. Since lbinding->multiple_interfaces is false, > there is no attempt to try to find an OVS interface to replace the old > lbinding->iface. The result is that lp1 still exists but it's not bound > anywhere. > > At this point you might be saying that this is expected, since you only > intend to support the case where 2 interfaces have the same iface-id. > However, I think that if the intent is to only support a maximum of 2 > interfaces this way, then we should not update the lbinding in step (3) > when interface lp2 is created, and we should print a warning. > > > lbinding->iface = iface_rec; > > + lbinding->multiple_bindings = true; > > + b_ctx_out->local_lports_changed = true; > > } > > > > struct binding_lport *b_lport = > > @@ -2219,13 +2223,23 @@ static bool > > consider_iface_release(const struct ovsrec_interface *iface_rec, > > const char *iface_id, > > struct binding_ctx_in *b_ctx_in, > > - struct binding_ctx_out *b_ctx_out) > > + struct binding_ctx_out *b_ctx_out, > > + bool *multiple_bindings) > > { > > struct local_binding *lbinding; > > struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; > > struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; > > + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > > > > lbinding = local_binding_find(local_bindings, iface_id); > > + if (lbinding && !lbinding->multiple_bindings && > > + lbinding->iface != iface_rec && !ofport) { > > + VLOG_DBG("Not releasing lport %s as %s was claimed " > > + "and %s was never bound)", > > + iface_id, lbinding->iface ? lbinding->iface->name:"", > > + iface_rec->name); > > + return true; > > + } > > struct binding_lport *b_lport = > > local_binding_get_primary_or_localport_lport(lbinding); > > if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) > { > > @@ -2254,8 +2268,10 @@ consider_iface_release(const struct > ovsrec_interface *iface_rec, > > } > > > > if (lbinding) { > > + *multiple_bindings = lbinding->multiple_bindings; > > /* Clear the iface of the local binding. */ > > lbinding->iface = NULL; > > + lbinding->multiple_bindings = false; > > } > > > > /* Check if the lbinding has children of type PB_CONTAINER. > > @@ -2266,8 +2282,6 @@ consider_iface_release(const struct > ovsrec_interface *iface_rec, > > } > > > > remove_local_lports(iface_id, b_ctx_out); > > - smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > > - > > return true; > > } > > > > @@ -2375,6 +2389,10 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > > > bool handled = true; > > > > + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > + struct hmap *qos_map_ptr = > > + sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > + > > /* Run the tracked interfaces loop twice. One to handle deleted > > * changes. And another to handle add/update changes. > > * This will ensure correctness. > > @@ -2435,8 +2453,59 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > } > > > > if (cleared_iface_id) { > > + bool multiple_bindings = false; > > handled = consider_iface_release(iface_rec, > cleared_iface_id, > > - b_ctx_in, b_ctx_out); > > + b_ctx_in, b_ctx_out, > > + &multiple_bindings); > > + if (!handled) { > > + break; > > + } > > + if (!multiple_bindings) { > > + smap_remove(b_ctx_out->local_iface_ids, > iface_rec->name); > > + continue; > > + } > > + /* There is another OVS interface bound to this port. > > + * First find its name > > + */ > > + VLOG_DBG("binding remaining for %s", cleared_iface_id); > > + struct smap_node *node; > > + bool found = false; > > + SMAP_FOR_EACH (node, b_ctx_out->local_iface_ids) { > > + if (strcmp(node->value, cleared_iface_id) || > > + !strcmp(node->key, iface_rec->name)) { > > + continue; > > + } > > + /* Then find its iface_rec */ > > + char *other_iface_rec_name = node->key; > > + const struct ovsrec_interface *other_iface_rec; > > + OVSREC_INTERFACE_TABLE_FOR_EACH (other_iface_rec, > > + b_ctx_in->iface_table) > { > > + if (!strcmp(other_iface_rec->name, > other_iface_rec_name)) { > > + found = true; > > + break; > > + } > > + } > > + if (found) { > > + int64_t ofport = other_iface_rec->n_ofport ? > > + *other_iface_rec->ofport : 0; > > + if (cleared_iface_id && ofport > 0 && > > + is_iface_in_int_bridge(other_iface_rec, > > + b_ctx_in->br_int)) { > > + handled = consider_iface_claim(other_iface_rec, > > + cleared_iface_id, > > + b_ctx_in, > > + b_ctx_out, > qos_map_ptr); > > + } > > + } > > + break; > > + } > > + if (!found) { > > + /* This is unexpected. Recompute to clean up */ > > + VLOG_WARN("Did not find which OVS interface still bound > to %s", > > + cleared_iface_id); > > + handled = false; > > + } > > + smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); > > } > > > > if (!handled) { > > @@ -2448,13 +2517,10 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > /* This can happen if any non vif OVS interface is in the > tracked > > * list or if consider_iface_release() returned false. > > * There is no need to process further. */ > > + destroy_qos_map(&qos_map); > > return false; > > } > > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > > - struct hmap *qos_map_ptr = > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; > > - > > /* > > * We consider an OVS interface for claiming if the following > > * 2 conditions are met: > > @@ -3034,6 +3100,7 @@ local_binding_create(const char *name, const > struct ovsrec_interface *iface) > > struct local_binding *lbinding = xzalloc(sizeof *lbinding); > > lbinding->name = xstrdup(name); > > lbinding->iface = iface; > > + lbinding->multiple_bindings = false; > > ovs_list_init(&lbinding->binding_lports); > > > > return lbinding; > > diff --git a/controller/binding.h b/controller/binding.h > > index ad959a9e6..6c3a98b02 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -135,6 +135,7 @@ struct local_binding { > > char *name; > > const struct ovsrec_interface *iface; > > struct ovs_list binding_lports; > > + bool multiple_bindings; > > }; > > > > struct local_binding_data { > > diff --git a/tests/ovn.at b/tests/ovn.at > > index f8b8db4df..d9b720a98 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -32975,3 +32975,108 @@ check ovn-nbctl --wait=hv sync > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical > port]) > > +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 > > + > > +get_flows() > > +{ > > + cookie=$1 > > + ovs-ofctl dump-flows br-int | grep $cookie | > > + sed -e 's/duration=[[0-9.]]*s, //g' | > > + sed -e 's/idle_age=[[0-9]]*, //g' | > > + sed -e 's/n_packets=[[0-9]]*, //g' | > > + sed -e 's/n_bytes=[[0-9]]*, //g' > > +} > > + > > +check ovn-nbctl ls-add ls > > +check ovn-nbctl lsp-add ls lp > > +check ovn-nbctl lsp-set-type lp localport > > +check ovn-nbctl lsp-set-addresses lp "00:00:00:01:01:02 192.168.1.2" > > + > > +check ovn-nbctl lsp-add ls vm1 > > +check ovn-nbctl lsp-set-addresses vm1 "00:00:00:01:01:11 192.168.1.11" > > +check ovs-vsctl add-port br-int vm1 -- set interface vm1 type=internal > external_ids:iface-id=vm1 > > + > > +check ovn-nbctl --wait=hv sync > > + > > +check ovs-vsctl add-port br-int lpold -- set interface lpold > type=internal > > +check ovs-vsctl set interface lpold external_ids:iface-id=lp > > + > > +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns _uuid find > port_binding logical_port=lp) != x]) > > +echo ====================================================== > > +echo === Flows after iface-id set for the old interface === > > +echo ====================================================== > > +COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut -d: > -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//') > > + > > +OVS_WAIT_UNTIL([ > > + ofport=$(ovs-vsctl --bare --columns ofport find Interface > name=lpold) > > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > > +]) > > +nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` > > +echo $nb_flows "flows after iface-id set for old interface" > > + > > +echo ====================================================== > > +echo === Flows after iface-id set for the new interface === > > +echo ====================================================== > > +check ovs-vsctl add-port br-int lpnew -- set interface lpnew > type=internal > > +check ovs-vsctl set interface lpnew external_ids:iface-id=lp > > +OVS_WAIT_UNTIL([ > > + ofport=$(ovs-vsctl --bare --columns ofport find Interface > name=lpnew) > > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > > +]) > > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > > +flows_lpnew=$(get_flows $COOKIE) > > + > > +echo ====================================================== > > +echo ======= Flows after old interface is deleted ========= > > +echo ====================================================== > > +check ovs-vsctl del-port br-int lpold > > +# We do not expect changes, so let's wait a few seconds to check > nothing changed > > +sleep 2 > > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > > +flows_after_deletion=$(get_flows $COOKIE) > > +check test "$flows_lpnew" = "$flows_after_deletion" > > + > > +echo ====================================================== > > +echo ======= Flows after lptemp interface is created ==== > > +echo ====================================================== > > +check ovs-vsctl add-port br-int lptemp -- set Interface lptemp > type=internal > > +check ovs-vsctl set Interface lptemp external_ids:iface-id=lp > > +OVS_WAIT_UNTIL([ > > + ofport=$(ovs-vsctl --bare --columns ofport find Interface > name=lptemp) > > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > > +]) > > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > > + > > +echo ====================================================== > > +echo ======= Flows after lptemp interface is deleted ====== > > +echo ====================================================== > > +check ovs-vsctl del-port br-int lptemp > > +OVS_WAIT_UNTIL([ > > + ofport=$(ovs-vsctl --bare --columns ofport find Interface > name=lpnew) > > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > > +]) > > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > > +flows_after_deletion=$(get_flows $COOKIE) > > +check test "$flows_lpnew" = "$flows_after_deletion" > > + > > +echo ====================================================== > > +echo ======= Flows after new interface is deleted ========= > > +echo ====================================================== > > +check ovs-vsctl del-port br-int lpnew > > +OVS_WAIT_UNTIL([ > > + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` > > + test "${nb_flows}" = 0 > > +]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > >
diff --git a/controller/binding.c b/controller/binding.c index c3d2b2e42..b1dfb86b3 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2155,8 +2155,12 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, if (!lbinding) { lbinding = local_binding_create(iface_id, iface_rec); local_binding_add(local_bindings, lbinding); - } else { + } else if (!lbinding->iface) { + lbinding->iface = iface_rec; + } else if (lbinding->iface != iface_rec) { lbinding->iface = iface_rec; + lbinding->multiple_bindings = true; + b_ctx_out->local_lports_changed = true; } struct binding_lport *b_lport = @@ -2219,13 +2223,23 @@ static bool consider_iface_release(const struct ovsrec_interface *iface_rec, const char *iface_id, struct binding_ctx_in *b_ctx_in, - struct binding_ctx_out *b_ctx_out) + struct binding_ctx_out *b_ctx_out, + bool *multiple_bindings) { struct local_binding *lbinding; struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; lbinding = local_binding_find(local_bindings, iface_id); + if (lbinding && !lbinding->multiple_bindings && + lbinding->iface != iface_rec && !ofport) { + VLOG_DBG("Not releasing lport %s as %s was claimed " + "and %s was never bound)", + iface_id, lbinding->iface ? lbinding->iface->name:"", + iface_rec->name); + return true; + } struct binding_lport *b_lport = local_binding_get_primary_or_localport_lport(lbinding); if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) { @@ -2254,8 +2268,10 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, } if (lbinding) { + *multiple_bindings = lbinding->multiple_bindings; /* Clear the iface of the local binding. */ lbinding->iface = NULL; + lbinding->multiple_bindings = false; } /* Check if the lbinding has children of type PB_CONTAINER. @@ -2266,8 +2282,6 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, } remove_local_lports(iface_id, b_ctx_out); - smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); - return true; } @@ -2375,6 +2389,10 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, bool handled = true; + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); + struct hmap *qos_map_ptr = + sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; + /* Run the tracked interfaces loop twice. One to handle deleted * changes. And another to handle add/update changes. * This will ensure correctness. @@ -2435,8 +2453,59 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, } if (cleared_iface_id) { + bool multiple_bindings = false; handled = consider_iface_release(iface_rec, cleared_iface_id, - b_ctx_in, b_ctx_out); + b_ctx_in, b_ctx_out, + &multiple_bindings); + if (!handled) { + break; + } + if (!multiple_bindings) { + smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); + continue; + } + /* There is another OVS interface bound to this port. + * First find its name + */ + VLOG_DBG("binding remaining for %s", cleared_iface_id); + struct smap_node *node; + bool found = false; + SMAP_FOR_EACH (node, b_ctx_out->local_iface_ids) { + if (strcmp(node->value, cleared_iface_id) || + !strcmp(node->key, iface_rec->name)) { + continue; + } + /* Then find its iface_rec */ + char *other_iface_rec_name = node->key; + const struct ovsrec_interface *other_iface_rec; + OVSREC_INTERFACE_TABLE_FOR_EACH (other_iface_rec, + b_ctx_in->iface_table) { + if (!strcmp(other_iface_rec->name, other_iface_rec_name)) { + found = true; + break; + } + } + if (found) { + int64_t ofport = other_iface_rec->n_ofport ? + *other_iface_rec->ofport : 0; + if (cleared_iface_id && ofport > 0 && + is_iface_in_int_bridge(other_iface_rec, + b_ctx_in->br_int)) { + handled = consider_iface_claim(other_iface_rec, + cleared_iface_id, + b_ctx_in, + b_ctx_out, qos_map_ptr); + } + } + break; + } + if (!found) { + /* This is unexpected. Recompute to clean up */ + VLOG_WARN("Did not find which OVS interface still bound to %s", + cleared_iface_id); + handled = false; + } + smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); } if (!handled) { @@ -2448,13 +2517,10 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, /* This can happen if any non vif OVS interface is in the tracked * list or if consider_iface_release() returned false. * There is no need to process further. */ + destroy_qos_map(&qos_map); return false; } - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); - struct hmap *qos_map_ptr = - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; - /* * We consider an OVS interface for claiming if the following * 2 conditions are met: @@ -3034,6 +3100,7 @@ local_binding_create(const char *name, const struct ovsrec_interface *iface) struct local_binding *lbinding = xzalloc(sizeof *lbinding); lbinding->name = xstrdup(name); lbinding->iface = iface; + lbinding->multiple_bindings = false; ovs_list_init(&lbinding->binding_lports); return lbinding; diff --git a/controller/binding.h b/controller/binding.h index ad959a9e6..6c3a98b02 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -135,6 +135,7 @@ struct local_binding { char *name; const struct ovsrec_interface *iface; struct ovs_list binding_lports; + bool multiple_bindings; }; struct local_binding_data { diff --git a/tests/ovn.at b/tests/ovn.at index f8b8db4df..d9b720a98 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -32975,3 +32975,108 @@ check ovn-nbctl --wait=hv sync OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical port]) +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 + +get_flows() +{ + cookie=$1 + ovs-ofctl dump-flows br-int | grep $cookie | + sed -e 's/duration=[[0-9.]]*s, //g' | + sed -e 's/idle_age=[[0-9]]*, //g' | + sed -e 's/n_packets=[[0-9]]*, //g' | + sed -e 's/n_bytes=[[0-9]]*, //g' +} + +check ovn-nbctl ls-add ls +check ovn-nbctl lsp-add ls lp +check ovn-nbctl lsp-set-type lp localport +check ovn-nbctl lsp-set-addresses lp "00:00:00:01:01:02 192.168.1.2" + +check ovn-nbctl lsp-add ls vm1 +check ovn-nbctl lsp-set-addresses vm1 "00:00:00:01:01:11 192.168.1.11" +check ovs-vsctl add-port br-int vm1 -- set interface vm1 type=internal external_ids:iface-id=vm1 + +check ovn-nbctl --wait=hv sync + +check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal +check ovs-vsctl set interface lpold external_ids:iface-id=lp + +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns _uuid find port_binding logical_port=lp) != x]) +echo ====================================================== +echo === Flows after iface-id set for the old interface === +echo ====================================================== +COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut -d: -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//') + +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpold) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` +echo $nb_flows "flows after iface-id set for old interface" + +echo ====================================================== +echo === Flows after iface-id set for the new interface === +echo ====================================================== +check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal +check ovs-vsctl set interface lpnew external_ids:iface-id=lp +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_lpnew=$(get_flows $COOKIE) + +echo ====================================================== +echo ======= Flows after old interface is deleted ========= +echo ====================================================== +check ovs-vsctl del-port br-int lpold +# We do not expect changes, so let's wait a few seconds to check nothing changed +sleep 2 +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_after_deletion=$(get_flows $COOKIE) +check test "$flows_lpnew" = "$flows_after_deletion" + +echo ====================================================== +echo ======= Flows after lptemp interface is created ==== +echo ====================================================== +check ovs-vsctl add-port br-int lptemp -- set Interface lptemp type=internal +check ovs-vsctl set Interface lptemp external_ids:iface-id=lp +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lptemp) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) + +echo ====================================================== +echo ======= Flows after lptemp interface is deleted ====== +echo ====================================================== +check ovs-vsctl del-port br-int lptemp +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_after_deletion=$(get_flows $COOKIE) +check test "$flows_lpnew" = "$flows_after_deletion" + +echo ====================================================== +echo ======= Flows after new interface is deleted ========= +echo ====================================================== +check ovs-vsctl del-port br-int lpnew +OVS_WAIT_UNTIL([ + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` + test "${nb_flows}" = 0 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +])
In the following scenario: - interface "old" is created and external_ids:iface-id is set (to lp) - interface "new" is created and external_ids:iface-id is set (to same lp) - interface "old" is deleted flows related to lp were deleted. Note that after "new" interface is created, flows use "new" ofport. This patch only supports up to two ovs interfaces bound to the same iface-id. Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for pflow_output engine.") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866 Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- v2: - Use bool instead of int for binding count to better reflect only one additional binding is supported. - Fix use after free. - Remove debug logging from test case --- controller/binding.c | 85 +++++++++++++++++++++++++++++++---- controller/binding.h | 1 + tests/ovn.at | 105 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 9 deletions(-)