| Message ID | 20230927183003.3498939-1-mheib@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] controller: release container lport when releasing parent port | 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 | fail | github build: failed |
On 9/27/23 20:30, Mohammad Heib wrote: > Currently if the user sets the container parent_port:requested-chassis > option after the VIF/CIF is bonded to the chassis, this will migrate > the VIF/CIF flows to the new chassis but will still have the > container flows installed in the old chassis which can allow unwanted > tagged traffic to reach VMS/containers on the old chassis. > > This patch will resolve the above issue by remove the CIF flows > from the old chassis and prevent the CIF from being bonded to a > chassis different from the parent port VIF binding chassis. > > Rreported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2220938 > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- Hi Mohammad, Thanks for the patch! A few comments inline. > controller/binding.c | 18 ++++++++++++++ > controller/physical.c | 9 +++++++ > tests/ovn-controller.at | 55 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 82 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index fd08aaafa..2e58fb0cd 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1599,6 +1599,21 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr, > b_lport->lbinding->iface->name, > &b_lport->lbinding->iface->header_.uuid); > + > + LIST_FOR_EACH (b_lport, list_node, Please use a new variable here. Re-using 'b_lport' is risky IMO. What if in the future we use 'b_lport' after the loop? > + &b_lport->lbinding->binding_lports) { > + /* release children lports of type container if the primary > + * binding lport cannot be bind to this chassis. > + */ > + if (b_lport->type == LP_CONTAINER) { Isn't this valid for all types of "children lports"? So also for LP_VIRTUAL? > + if (!release_lport(b_lport->pb, b_ctx_in->chassis_rec, > + !b_ctx_in->ovnsb_idl_txn, > + b_ctx_out->tracked_dp_bindings, > + b_ctx_out->if_mgr)) { > + return false; > + } > + } > + } > } > > if (!lbinding_set || !can_bind) { > @@ -1733,6 +1748,9 @@ consider_container_lport(const struct sbrec_port_binding *pb, > > ovs_assert(parent_b_lport && parent_b_lport->pb); > bool can_bind = lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb); > + /* cannot bind to this chassis if the parent_port cannot be bounded. */ > + can_bind &= lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, > + parent_b_lport->pb); > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, > container_b_lport); > diff --git a/controller/physical.c b/controller/physical.c > index 75257bc85..5a5824f39 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -1573,6 +1573,15 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > nested_container = true; > parent_port = lport_lookup_by_name( > sbrec_port_binding_by_name, binding->parent_port); > + > + if (parent_port > + && !lport_can_bind_on_this_chassis(chassis, parent_port)) { > + /* Even though there is an ofport for this container > + * parent port, it is requested on different chassis ignore > + * this container port. > + */ > + return; > + } > } > } else if (!strcmp(binding->type, "localnet") > || !strcmp(binding->type, "l2gateway")) { > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 4212d601a..65c3aa84b 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -2651,3 +2651,58 @@ OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c in_por > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-controller - cleanup VIF/CIF related flows/fields when updating requested-chassis]) > +ovn_start > + > +net_add n1 > +sim_add hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +check ovs-vsctl -- add-port br-int vif1 -- \ > + set Interface vif1 external-ids:iface-id=lsp1 \ > + ofport-request=8 > + > +check ovn-nbctl ls-add lsw0 > + > +check ovn-nbctl lsp-add lsw0 lsp1 > +check ovn-nbctl lsp-set-addresses lsp1 "f0:00:00:00:00:01 172.16.0.101" Do we really need addresses? > +check ovn-nbctl lsp-add lsw0 sw0-port1.1 lsp1 7 > +check ovn-nbctl lsp-set-addresses sw0-port1.1 "f0:00:00:01:02:07 192.168.2.2" Same here. > + > +# wait for the VIF to be claimed to this chassis > +wait_row_count Chassis 1 name=hv1 > +hv1_uuid=$(fetch_column Chassis _uuid name=hv1) > +wait_for_ports_up lsp1 > +wait_for_ports_up sw0-port1.1 > +wait_column "$hv1_uuid" Port_Binding chassis logical_port=lsp1 > +wait_column "$hv1_uuid" Port_Binding chassis logical_port=sw0-port1.1 > + > +# check that flows is installed > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 |grep priority=100 | grep -c in_port=8], [0],[dnl > +1 > +]) > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 |grep priority=150|grep dl_vlan=7| grep -c in_port=8], [0],[dnl > +1 > +]) > + > +# set lport requested-chassis to differant chassis > +check ovn-nbctl set Logical_Switch_Port lsp1 \ > + options:requested-chassis=foo > + > +OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'false']) > +OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding sw0-port1.1 up` = 'false']) > +wait_column "" Port_Binding chassis logical_port=lsp1 > +wait_column "" Port_Binding chassis logical_port=sw0-port1.1 > + > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 |grep priority=100 |grep -c in_port=8], [1],[dnl > +0 > +]) > +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 |grep priority=150|grep dl_vlan=7| grep -c in_port=8], [1],[dnl > +0 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) Regards, Dumitru
diff --git a/controller/binding.c b/controller/binding.c index fd08aaafa..2e58fb0cd 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1599,6 +1599,21 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr, b_lport->lbinding->iface->name, &b_lport->lbinding->iface->header_.uuid); + + LIST_FOR_EACH (b_lport, list_node, + &b_lport->lbinding->binding_lports) { + /* release children lports of type container if the primary + * binding lport cannot be bind to this chassis. + */ + if (b_lport->type == LP_CONTAINER) { + if (!release_lport(b_lport->pb, b_ctx_in->chassis_rec, + !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings, + b_ctx_out->if_mgr)) { + return false; + } + } + } } if (!lbinding_set || !can_bind) { @@ -1733,6 +1748,9 @@ consider_container_lport(const struct sbrec_port_binding *pb, ovs_assert(parent_b_lport && parent_b_lport->pb); bool can_bind = lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb); + /* cannot bind to this chassis if the parent_port cannot be bounded. */ + can_bind &= lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, + parent_b_lport->pb); return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, container_b_lport); diff --git a/controller/physical.c b/controller/physical.c index 75257bc85..5a5824f39 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -1573,6 +1573,15 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, nested_container = true; parent_port = lport_lookup_by_name( sbrec_port_binding_by_name, binding->parent_port); + + if (parent_port + && !lport_can_bind_on_this_chassis(chassis, parent_port)) { + /* Even though there is an ofport for this container + * parent port, it is requested on different chassis ignore + * this container port. + */ + return; + } } } else if (!strcmp(binding->type, "localnet") || !strcmp(binding->type, "l2gateway")) { diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 4212d601a..65c3aa84b 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -2651,3 +2651,58 @@ OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c in_por OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller - cleanup VIF/CIF related flows/fields when updating requested-chassis]) +ovn_start + +net_add n1 +sim_add hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 +check ovs-vsctl -- add-port br-int vif1 -- \ + set Interface vif1 external-ids:iface-id=lsp1 \ + ofport-request=8 + +check ovn-nbctl ls-add lsw0 + +check ovn-nbctl lsp-add lsw0 lsp1 +check ovn-nbctl lsp-set-addresses lsp1 "f0:00:00:00:00:01 172.16.0.101" +check ovn-nbctl lsp-add lsw0 sw0-port1.1 lsp1 7 +check ovn-nbctl lsp-set-addresses sw0-port1.1 "f0:00:00:01:02:07 192.168.2.2" + +# wait for the VIF to be claimed to this chassis +wait_row_count Chassis 1 name=hv1 +hv1_uuid=$(fetch_column Chassis _uuid name=hv1) +wait_for_ports_up lsp1 +wait_for_ports_up sw0-port1.1 +wait_column "$hv1_uuid" Port_Binding chassis logical_port=lsp1 +wait_column "$hv1_uuid" Port_Binding chassis logical_port=sw0-port1.1 + +# check that flows is installed +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 |grep priority=100 | grep -c in_port=8], [0],[dnl +1 +]) +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 |grep priority=150|grep dl_vlan=7| grep -c in_port=8], [0],[dnl +1 +]) + +# set lport requested-chassis to differant chassis +check ovn-nbctl set Logical_Switch_Port lsp1 \ + options:requested-chassis=foo + +OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'false']) +OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding sw0-port1.1 up` = 'false']) +wait_column "" Port_Binding chassis logical_port=lsp1 +wait_column "" Port_Binding chassis logical_port=sw0-port1.1 + +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 |grep priority=100 |grep -c in_port=8], [1],[dnl +0 +]) +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 |grep priority=150|grep dl_vlan=7| grep -c in_port=8], [1],[dnl +0 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +])
Currently if the user sets the container parent_port:requested-chassis option after the VIF/CIF is bonded to the chassis, this will migrate the VIF/CIF flows to the new chassis but will still have the container flows installed in the old chassis which can allow unwanted tagged traffic to reach VMS/containers on the old chassis. This patch will resolve the above issue by remove the CIF flows from the old chassis and prevent the CIF from being bonded to a chassis different from the parent port VIF binding chassis. Rreported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2220938 Signed-off-by: Mohammad Heib <mheib@redhat.com> --- controller/binding.c | 18 ++++++++++++++ controller/physical.c | 9 +++++++ tests/ovn-controller.at | 55 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+)