On Mon, Nov 6, 2023 at 5:55 AM Mohammad Heib <[email protected]> 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 <[email protected]>
Hi Mohammad,
Thanks for the fix. Really sorry for the delay in reviewing the patch.
> ---
> controller/binding.c | 19 +++++++++++++++
> controller/physical.c | 9 +++++++
> tests/ovn-controller.at | 53 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 81 insertions(+)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 8020d052f..0b64a1226 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1600,6 +1600,22 @@ 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);
> +
> + struct binding_lport *tmp_b_lport = b_lport;
> + LIST_FOR_EACH (tmp_b_lport, list_node,
> + &tmp_b_lport->lbinding->binding_lports) {
> + /* release children lports of type container if the primary
> + * binding lport cannot be bind to this chassis.
> + */
> + if (tmp_b_lport->type == LP_CONTAINER) {
> + if (!release_lport(tmp_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;
> + }
> + }
> + }
> }
>
Although the above code fixes the issue, I don't think this is the
right place to fix it.
I think it should be fixed in consider_container_lport().
When the parent port gets updated with a different requested-chassis
---
check ovn-nbctl set Logical_Switch_Port lsp1 options:requested-chassis=foo
---
The function binding_handle_port_binding_changes() calls
handle_updated_port() and it calls handle_updated_vif_lport().
If you look at this function here [1], it iterates its binding lports
and it calls consider_container_lport() for its children.
Can you please look into why consider_container_lport() is not
releasing it ? I think it's better to fix it in this function
rather than in consider_vif_lport_().
Thanks
Numan
i.e
> if (!lbinding_set || !can_bind) {
> @@ -1734,6 +1750,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 2338561ec..4e169e35f 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 e72438fbf..8a1904378 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2700,3 +2700,56 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=40
> | grep -q controller], [1]
> 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-add lsw0 sw0-port1.1 lsp1 7
> +
> +# 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
> +])
> --
> 2.34.3
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
