On Mon, Feb 17, 2025 at 8:07 PM Xavier Simonart <[email protected]> wrote:
> Hi Ales > > Thanks for the review and the comments. > Some comments below. > > On Mon, Feb 17, 2025 at 3:21 PM Ales Musil <[email protected]> wrote: > >> >> >> On Thu, Feb 13, 2025 at 5:01 PM Xavier Simonart <[email protected]> >> wrote: >> >>> Handle container ports migration in a similar way as vif migration. >>> When a container port is migrated (i.e. when requested-chassis is >>> configured >>> for its parent port), install flows in both src and dst hypervisors. >>> When migration completes, remove flows from src hypervisor. >>> >>> Reported-at: https://issues.redhat.com/browse/FDP-1037 >>> >>> Signed-off-by: Xavier Simonart <[email protected]> >>> --- >>> >> >> Hi Xavier, >> >> thank you for the patch, I have a couple of comments down below. >> >> >> controller/binding.c | 44 +++++--- >>> controller/physical.c | 4 +- >>> tests/multinode.at | 257 ++++++++++++++++++++++++++++++++++++++++++ >>> tests/ovn.at | 160 +++++++++++++++++++++++++- >>> 4 files changed, 447 insertions(+), 18 deletions(-) >>> >>> diff --git a/controller/binding.c b/controller/binding.c >>> index 7d4143008..7f1027b02 100644 >>> --- a/controller/binding.c >>> +++ b/controller/binding.c >>> @@ -1147,6 +1147,9 @@ set_pb_chassis_in_sbrec(const struct >>> sbrec_port_binding *pb, >>> VLOG_INFO("%s: Claiming %s", pb->logical_port, >>> pb->mac[i]); >>> } >>> sbrec_port_binding_set_chassis(pb, chassis_rec); >>> + if (is_additional_chassis(pb, chassis_rec)) { >>> + remove_additional_chassis(pb, chassis_rec); >>> + } >>> >> >> >> This feels like a separate issue or am I missing something? >> > You're right. In fact, as such this change is not strictly necessary as > the removal from additional chassis is done in claim_lport. However, moving > it to here would > avoid some potential recompute if sb is read only in claim_lport. But as > you said, this should not be part of this patch. > So, I'll remove it in v2. > Maybe we can have it as a separate change. It's still useful IMO. >> } >>> } else if (!is_set) { >>> sbrec_port_binding_set_chassis(pb, NULL); >>> @@ -1357,9 +1360,9 @@ claim_lport(const struct sbrec_port_binding *pb, >>> bool sb_readonly, bool is_vif, >>> struct hmap *tracked_datapaths, >>> struct if_status_mgr *if_mgr, >>> - struct sset *postponed_ports) >>> + struct sset *postponed_ports, >>> + enum can_bind can_bind) >>> { >>> - enum can_bind can_bind = >>> lport_can_bind_on_this_chassis(chassis_rec, pb); >>> bool update_tracked = false; >>> >>> if (can_bind == CAN_BIND_AS_MAIN) { >>> @@ -1561,7 +1564,7 @@ release_binding_lport(const struct sbrec_chassis >>> *chassis_rec, >>> >>> static bool >>> consider_vif_lport_(const struct sbrec_port_binding *pb, >>> - bool can_bind, >>> + enum can_bind can_bind, >>> struct binding_ctx_in *b_ctx_in, >>> struct binding_ctx_out *b_ctx_out, >>> struct binding_lport *b_lport) >>> @@ -1579,7 +1582,7 @@ consider_vif_lport_(const struct >>> sbrec_port_binding *pb, >>> !b_ctx_in->ovnsb_idl_txn, >>> !parent_pb, b_ctx_out->tracked_dp_bindings, >>> b_ctx_out->if_mgr, >>> - b_ctx_out->postponed_ports)) { >>> + b_ctx_out->postponed_ports, can_bind)) { >>> return false; >>> } >>> >>> @@ -1632,8 +1635,15 @@ consider_vif_lport_(const struct >>> sbrec_port_binding *pb, >>> b_ctx_out->if_mgr); >>> } >>> } >>> + >>> + struct binding_lport *parent_b_lport = b_lport && b_lport->lbinding >>> ? >>> + local_binding_get_primary_lport(b_lport->lbinding) : NULL; >>> + >>> if (pb->chassis && pb->chassis != b_ctx_in->chassis_rec >>> && !is_requested_additional_chassis(pb, >>> b_ctx_in->chassis_rec) >>> + && (!parent_b_lport >>> + || !is_requested_additional_chassis(parent_b_lport->pb, >>> + >>> b_ctx_in->chassis_rec)) >>> && if_status_is_port_claimed(b_ctx_out->if_mgr, >>> pb->logical_port)) { >>> update_lport_tracking(pb, b_ctx_out->tracked_dp_bindings, >>> false); >>> @@ -1651,7 +1661,8 @@ consider_vif_lport(const struct sbrec_port_binding >>> *pb, >>> struct binding_ctx_out *b_ctx_out, >>> struct local_binding *lbinding) >>> { >>> - bool can_bind = >>> lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb); >>> + enum can_bind can_bind = >>> + lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb); >>> >>> if (!lbinding) { >>> lbinding = >>> local_binding_find(&b_ctx_out->lbinding_data->bindings, >>> @@ -1767,10 +1778,12 @@ consider_container_lport(const struct >>> sbrec_port_binding *pb, >>> ovs_assert(parent_b_lport && parent_b_lport->pb); >>> /* cannot bind to this chassis if the parent_port cannot be >>> bounded. */ >>> /* Do not bind neither if parent is postponed */ >>> - bool can_bind = >>> lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, >>> - parent_b_lport->pb) >>> && >>> - >>> !is_postponed_port(parent_b_lport->pb->logical_port) && >>> - >>> lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb); >>> + >>> + enum can_bind can_bind = >>> + (!is_postponed_port(parent_b_lport->pb->logical_port) && >>> + lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb)) ? >>> + lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, >>> + parent_b_lport->pb) : >>> CANNOT_BIND; >>> >>> return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out, >>> container_b_lport); >>> @@ -1820,7 +1833,7 @@ consider_virtual_lport(const struct >>> sbrec_port_binding *pb, >>> } >>> } >>> >>> - if (!consider_vif_lport_(pb, true, b_ctx_in, b_ctx_out, >>> + if (!consider_vif_lport_(pb, CAN_BIND_AS_MAIN, b_ctx_in, b_ctx_out, >>> virtual_b_lport)) { >>> return false; >>> } >>> @@ -1916,11 +1929,13 @@ consider_nonvif_lport_(const struct >>> sbrec_port_binding *pb, >>> } >>> >>> update_related_lport(pb, b_ctx_out); >>> + enum can_bind can_bind = lport_can_bind_on_this_chassis( >>> + b_ctx_in->chassis_rec, pb); >>> return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL, >>> !b_ctx_in->ovnsb_idl_txn, false, >>> b_ctx_out->tracked_dp_bindings, >>> b_ctx_out->if_mgr, >>> - b_ctx_out->postponed_ports); >>> + b_ctx_out->postponed_ports, can_bind); >>> } >>> if (!is_ha_chassis) { >>> remove_related_lport(pb, b_ctx_out); >>> @@ -2900,7 +2915,8 @@ handle_updated_vif_lport(const struct >>> sbrec_port_binding *pb, >>> bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); >>> >>> if (lport_type == LP_VIRTUAL || lport_type == LP_CONTAINER || >>> - claimed == now_claimed) { >>> + (claimed == now_claimed && >>> + !is_additional_chassis(pb, b_ctx_in->chassis_rec))) { >>> return true; >>> } >>> >>> @@ -2917,7 +2933,9 @@ handle_updated_vif_lport(const struct >>> sbrec_port_binding *pb, >>> } >>> >>> struct binding_lport *b_lport; >>> + >>> LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { >>> + >>> >> >> nit: Unrelated changes. >> >> if (b_lport->type == LP_CONTAINER) { >>> handled = consider_container_lport(b_lport->pb, b_ctx_in, >>> b_ctx_out); >>> @@ -2987,7 +3005,7 @@ consider_patch_port_for_local_datapaths(const >>> struct sbrec_port_binding *pb, >>> !b_ctx_in->ovnsb_idl_txn, false, >>> b_ctx_out->tracked_dp_bindings, >>> b_ctx_out->if_mgr, >>> - b_ctx_out->postponed_ports); >>> + b_ctx_out->postponed_ports, >>> CAN_BIND_AS_MAIN); >>> } >>> /* If this chassis is claimed, but not requested to be; or >>> requested for >>> * some other chassis, but claimed by us - release. */ >>> diff --git a/controller/physical.c b/controller/physical.c >>> index 9cab3ae26..69bf05347 100644 >>> --- a/controller/physical.c >>> +++ b/controller/physical.c >>> @@ -1773,8 +1773,8 @@ consider_port_binding(const struct physical_ctx >>> *ctx, >>> ctx->sbrec_port_binding_by_name, binding->parent_port); >>> >>> if (parent_port >>> - && (lport_can_bind_on_this_chassis(ctx->chassis, >>> - parent_port) != CAN_BIND_AS_MAIN)) { >>> + && !lport_can_bind_on_this_chassis(ctx->chassis, >>> + parent_port)) { >>> /* Even though there is an ofport for this container >>> * parent port, it is requested on different chassis >>> ignore >>> * this container port. >>> diff --git a/tests/multinode.at b/tests/multinode.at >>> index c1bd3123a..6d0f2910d 100644 >>> --- a/tests/multinode.at >>> +++ b/tests/multinode.at >>> @@ -2776,3 +2776,260 @@ for i in 1 2; do >>> done >>> >>> AT_CLEANUP >>> + >>> +AT_SETUP([Migration of container ports]) >>> +# Migrate vif port between chassis-1 and chassis-3; send packets between >>> +# chassis-2 and chassis-1/chassis-3, and check that >>> +# - packet handing on src works before migration. >>> +# - packet handing on src & dst works during migration. >>> +# - packet handing on dst works after migration. >>> +# Do the same for container ports. >>> +# The container port migration is tested in two different orders, >>> +# setting iface-id on dst resp. before and after requested-chassis. >>> + >>> +# Check that ovn-fake-multinode setup is up and running >>> +# check_fake_multinode_setup >>> +check_fake_multinode_setup_by_nodes 'ovn-chassis-1 ovn-chassis-2 >>> ovn-chassis-3' >>> + >>> +# Delete the multinode NB and OVS resources before starting the test. >>> +cleanup_multinode_resources_by_nodes 'ovn-chassis-1 ovn-chassis-2 >>> ovn-chassis-3' >>> + >>> +OVS_WAIT_UNTIL([m_as ovn-chassis-1 ip link show | grep -q genev_sys]) >>> +OVS_WAIT_UNTIL([m_as ovn-chassis-2 ip link show | grep -q genev_sys]) >>> +OVS_WAIT_UNTIL([m_as ovn-chassis-3 ip link show | grep -q genev_sys]) >>> + >>> +check multinode_nbctl ls-add sw0 >>> +check multinode_nbctl lsp-add sw0 migrator >>> +check multinode_nbctl lsp-set-addresses migrator "50:54:00:00:00:03 >>> 10.0.0.3 1000::3" >>> +check multinode_nbctl lsp-add sw0 sw0-port2 >>> +check multinode_nbctl lsp-set-addresses sw0-port2 "50:54:00:00:00:04 >>> 10.0.0.4 1000::4" >>> + >>> +# Set requested chassis before creating migrator on chassis-3 >>> +check multinode_nbctl --wait=hv set Logical_Switch_Port migrator >>> options:requested-chassis=ovn-chassis-1 >>> + >>> +m_as ovn-chassis-1 /data/create_fake_vm.sh migrator migrator >>> 50:54:00:00:00:03 1342 10.0.0.3 24 10.0.0.1 1000::3/64 1000::a >>> +m_as ovn-chassis-3 /data/create_fake_vm.sh migrator migrator >>> 50:54:00:00:00:03 1342 10.0.0.3 24 10.0.0.1 1000::3/64 1000::a >>> +m_as ovn-chassis-2 /data/create_fake_vm.sh sw0-port2 sw0p2 >>> 50:54:00:00:00:04 1342 10.0.0.4 24 10.0.0.1 1000::4/64 1000::a >>> + >>> +m_wait_for_ports_up >>> + >>> +M_START_TCPDUMP([ovn-chassis-1], [-neei genev_sys_6081 arp or ip], >>> [ch1_genev]) >>> +M_START_TCPDUMP([ovn-chassis-1], [-neei migrator-p arp or ip], >>> [ch1_migrator]) >>> +M_START_TCPDUMP([ovn-chassis-2], [-neei genev_sys_6081 arp or ip], >>> [ch2_genev]) >>> +M_START_TCPDUMP([ovn-chassis-2], [-neei sw0p2-p arp or ip], [ch2_sw0p2]) >>> +M_START_TCPDUMP([ovn-chassis-3], [-neei genev_sys_6081 arp or ip], >>> [ch3_genev]) >>> +M_START_TCPDUMP([ovn-chassis-3], [-neei migrator-p arp or ip], >>> [ch3_migrator]) >>> + >>> +AS_BOX([Migration with vifs]) >>> +echo "Migrator on chassis-1 => sw0p2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 10.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "sw0p2 on chassis-2 => Migrator on chassis-1" >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ping -q -c 3 -i 0.3 -w 2 >>> 10.0.0.3 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "== Starting migration ==" >>> +check multinode_nbctl --wait=hv set Logical_Switch_Port migrator >>> options:requested-chassis=ovn-chassis-1,ovn-chassis-3 >>> + >>> +echo "Migrator on chassis-1 => sw0p2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 10.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> + >>> +echo "Migrator on chassis-3 => sw0p2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 10.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "sw0p2 on chassis-2 => migrator on ..." >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ping -q -c 3 -i 0.3 -w 2 >>> 10.0.0.3 | FORMAT_PING], \ >>> +[0], [stdout]) >>> + >>> +# Both VM are running ... We might get duplicates replies. >>> +AT_CHECK([cat stdout | grep "3 packets transmitted" | grep -c "3 >>> received"], [0],[dnl >>> +1 >>> +]) >>> + >>> +echo "== Finalizing migration ==" >>> +check multinode_nbctl --wait=hv set Logical_Switch_Port migrator >>> options:requested-chassis=ovn-chassis-3 >>> + >>> +echo "Migrator on chassis-1 => sw0p2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 10.0.0.4 | FORMAT_PING], \ >>> +[0], [stdout]) >>> + >>> +# VM still running on chassis-1 but flows should have been deleted as >>> migration completed. >>> +AT_CHECK([cat stdout | grep -c "100% packet loss"], [0],[dnl >>> +1 >>> +]) >>> + >>> +echo "Migrator on chassis-3 => sw0p2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 10.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +# We should not have duplicates anymore >>> +echo "sw0p2 on chassis-2 => migrator on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ping -q -c 3 -i 0.3 -w 2 >>> 10.0.0.3 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +AS_BOX([Migration with container ports]) >>> +# Create container ports. >>> +check multinode_nbctl ls-add sw1 >>> +check multinode_nbctl lsp-add sw1 mig-cont migrator 10 \ >>> + -- lsp-set-addresses mig-cont "f0:00:00:01:02:03 >>> 20.0.0.3" >>> +check multinode_nbctl lsp-add sw1 cont2 sw0-port2 10 \ >>> + -- lsp-set-addresses cont2 "f0:00:00:01:02:04 >>> 20.0.0.4" >>> + >>> +# Create the interface for lport mig-cont >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ip link add link migrator >>> name cont type vlan id 10], [0]) >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ip link set cont address >>> f0:00:00:01:02:03], [0]) >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ip link set cont up], [0]) >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ip addr add 20.0.0.3/24 >>> dev cont], [0]) >>> + >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ip link add link migrator >>> name cont type vlan id 10], [0]) >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ip link set cont address >>> f0:00:00:01:02:03], [0]) >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ip link set cont up], [0]) >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ip addr add 20.0.0.3/24 >>> dev cont], [0]) >>> + >>> +# Create the interface for lport sw1-port2 >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ip link add link sw0p2 name >>> cont2 type vlan id 10], [0]) >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ip link set cont2 address >>> f0:00:00:01:02:04], [0]) >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ip link set cont2 up], [0]) >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ip addr add 20.0.0.4/24 dev >>> cont2], [0]) >>> + >>> +echo "mig-cont on chassis-3 => cont2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "cont2 on chassis-2 => mig-count on chassis-3" >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.3 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "== Starting migration back ==" >>> +check multinode_nbctl --wait=hv set Logical_Switch_Port migrator >>> options:requested-chassis=ovn-chassis-3,ovn-chassis-1 >>> + >>> +echo "mig-cont on chassis-3 => cont2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "mig-count on chassis-1 => cont2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "cont2 on chassis-2 => migrator on ..." >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.3 | FORMAT_PING], \ >>> +[0], [stdout]) >>> + >>> +# Both VM are running ... We might get duplicates replies. >>> +AT_CHECK([cat stdout | grep "3 packets transmitted" | grep -c "3 >>> received"], [0],[dnl >>> +1 >>> +]) >>> + >>> +echo "== Finalizing migration ==" >>> +check multinode_nbctl --wait=hv set Logical_Switch_Port migrator >>> options:requested-chassis=ovn-chassis-1 >>> + >>> +echo "mig-cont on chassis-3 => cont2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.4 | FORMAT_PING], \ >>> +[0], [stdout]) >>> + >>> +# VM still running on chassis-3 but flows should have been deleted.... >>> +AT_CHECK([cat stdout | grep -c "100% packet loss"], [0],[dnl >>> +1 >>> +]) >>> + >>> +echo "mig-cont on chassis-1 => cont2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +# We should not have duplicates anymore >>> +echo "cont2 on chassis-2 => mig-cont on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.3 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "== Starting another migration, this time before starting dst VM >>> ==" >>> +# Unbind migrator from chassis-3 >>> +m_as ovn-chassis-3 ovs-vsctl -- set Interface migrator-p >>> external_ids:iface-id=foo >>> + >>> +check multinode_nbctl --wait=hv set Logical_Switch_Port migrator >>> options:requested-chassis=ovn-chassis-1,ovn-chassis-3 >>> +sleep 1 >>> +m_as ovn-chassis-3 ovs-vsctl -- set Interface migrator-p >>> external_ids:iface-id=migrator >>> + >>> + >>> +echo "mig-cont on chassis-3 => cont2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "mig-count on chassis-1 => cont2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +echo "cont2 on chassis-2 => migrator on ..." >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.3 | FORMAT_PING], \ >>> +[0], [stdout]) >>> + >>> +# Both VM are running ... We might get duplicates replies. >>> +AT_CHECK([cat stdout | grep "3 packets transmitted" | grep -c "3 >>> received"], [0],[dnl >>> +1 >>> +]) >>> + >>> +echo "== Finalizing migration ==" >>> +check multinode_nbctl --wait=hv set Logical_Switch_Port migrator >>> options:requested-chassis=ovn-chassis-3 >>> + >>> +echo "mig-cont on chassis-1 => cont2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-1], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.4 | FORMAT_PING], \ >>> +[0], [stdout]) >>> + >>> +# VM still running on chassis-1 but flows should have been deleted.... >>> +AT_CHECK([cat stdout | grep -c "100% packet loss"], [0],[dnl >>> +1 >>> +]) >>> + >>> +echo "mig-cont on chassis-3 => cont2 on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-3], [migrator], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.4 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +# We should not have duplicates anymore >>> +echo "cont2 on chassis-2 => mig-cont on chassis-2" >>> +M_NS_CHECK_EXEC([ovn-chassis-2], [sw0p2], [ping -q -c 3 -i 0.3 -w 2 >>> 20.0.0.3 | FORMAT_PING], \ >>> +[0], [dnl >>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >>> +]) >>> + >>> +m_as ovn-chassis-1 killall tcpdump >>> +m_as ovn-chassis-2 killall tcpdump >>> +m_as ovn-chassis-3 killall tcpdump >>> +rm -f *.tcpdump >>> +rm -f *.stderr >>> >> >> Shouldn't we keep those tcpdump logs for debugging purposes? >> > If the test reached those lines, it means that the test succeeded, so we > usually do not need much debugging. > However, I do not have a strong opinion, so we can keep them if we feel it > might help understanding (those dumps are not big). > I'll keep those logs in v2. > >> >> + >>> +AT_CLEANUP >>> + >>> + >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index d105ed253..08323f372 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -16785,6 +16785,160 @@ OVN_CLEANUP([hv1],[hv2]) >>> AT_CLEANUP >>> ]) >>> >>> +OVN_FOR_EACH_NORTHD([ >>> +AT_SETUP([No fight between two chassis for the same port]) >>> +ovn_start >>> + >>> +# lsp1 on hv1, lsp2 on hv2. >>> +# migrator1 (and its mig1-cont container) initially on hv1. >>> +# migrator2 (and its mig2-cont container) initially on hv2. >>> +# For migrator1, iface-id is set (in both hv) before starting migration. >>> +# For migrator2, iface-id is set (in dst hv) after initiating migration. >>> +check ovn-nbctl ls-add ls0 >>> +check ovn-nbctl lsp-add ls0 lsp1 >>> +check ovn-nbctl lsp-add ls0 lsp2 >>> +check ovn-nbctl lsp-add ls0 migrator1 >>> +check ovn-nbctl lsp-add ls0 migrator2 >>> +check ovn-nbctl lsp-add ls0 mig1-cont migrator1 1 >>> +check ovn-nbctl lsp-add ls0 mig2-cont migrator2 1 >>> +check ovn-nbctl set Logical_Switch_Port migrator1 >>> options:requested-chassis=hv1 >>> +check ovn-nbctl set Logical_Switch_Port migrator2 >>> options:requested-chassis=hv2 >>> + >>> +net_add n1 >>> +for i in 1 2; do >>> + sim_add hv$i >>> + as hv$i >>> + ovs-vsctl add-br br-phys >>> + ovn_attach n1 br-phys 192.168.0.$i >>> +done >>> + >>> +# Set iface-id in both hv1 and hv2. >>> +as hv2 ovs-vsctl -- add-port br-int vif-migrator2 \ >>> + -- set Interface vif-migrator2 >>> external-ids:iface-id=migrator2 >>> + >>> +for i in 1 2; do >>> + as hv$i >>> + ovs-vsctl -- add-port br-int vif-migrator1 \ >>> + -- set Interface vif-migrator1 >>> external-ids:iface-id=migrator1 >>> + ovs-vsctl -- add-port br-int vif$i \ >>> + -- set Interface vif$i external-ids:iface-id=lsp$i >>> +done >>> + >>> +hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1) >>> +hv2_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv2) >>> + >>> +check_in_port_flows_count() { >>> + hv=$1 >>> + of=$2 >>> + count=$3 >>> + AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int >>> table=OFTABLE_PHY_TO_LOG | grep "in_port=$of" | wc -l], [0], [dnl >>> +$count >>> +]) >>> +} >>> + >>> +check_claims() { >>> + port=$1 >>> + hv1_claims=$(grep -c "Claiming lport $port\|Changing chassis for >>> lport $port" hv1/ovn-controller.log) >>> + hv2_claims=$(grep -c "Claiming lport $port\|Changing chassis for >>> lport $port" hv2/ovn-controller.log) >>> + echo "$hv1_claims claims for port $port in hv1" >>> + echo "$hv2_claims claims for port $port in hv2" >>> + AT_CHECK([test "${hv1_claims}" -le "${max_claims}"], [0], []) >>> + AT_CHECK([test "${hv2_claims}" -le "${max_claims}"], [0], []) >>> +} >>> + >>> +# Waiting for flows in hv1 && hv2 for resp. lsp1 and lsp2. >>> +OVS_WAIT_UNTIL([ >>> + of_hv1=$(as hv1 ovs-vsctl --bare --columns ofport find Interface >>> name=vif1) >>> + of_hv2=$(as hv2 ovs-vsctl --bare --columns ofport find Interface >>> name=vif2) >>> >> >> We can request a unique ofport number, we should probably do that instead >> WDYT? >> > I usually avoid requesting a unique number as I feel it is not 100% > "reliable". > It works if we request a "high" number (e.g. 10), but one might wonder why > using high numbers... > If we request an ofport such as 1 or 2, which might clash with ofport > initially/internally attributed by ovn to interfaces such as ovn-hv1-0, the > requested ofport number might not be correct before some time. > That makes sense, I was so used to doing the ofport-request, maybe we should change that everywhere and add a nice macro for the retrieval as future follow up. > >> >>> + test -n of_hv1 && test -n $of_hv1 && >>> + test 1 -le $(as hv1 ovs-ofctl dump-flows br-int >>> table=OFTABLE_PHY_TO_LOG | grep "in_port=$of_hv1" | wc -l) && >>> + test 1 -le $(as hv2 ovs-ofctl dump-flows br-int >>> table=OFTABLE_PHY_TO_LOG | grep "in_port=$of_hv2" | wc -l) >>> +]) >>> + >>> +of1=$(as hv1 ovs-vsctl --bare --columns ofport find Interface >>> name=vif-migrator1) >>> +of2=$(as hv2 ovs-vsctl --bare --columns ofport find Interface >>> name=vif-migrator1) >>> + >>> +AS_BOX([$(date +%H:%M:%S.%03N) Before migrating migrator1]) >>> +wait_column "" Port_Binding additional_chassis logical_port=migrator1 >>> +wait_column "" Port_Binding additional_chassis logical_port=mig1-cont >>> +# We should have two flows in table OFTABLE_PHY_TO_LOG from >>> in_port=migrator1 in hv1 (one for vif and one for the container) and 0 >>> flows in hv2. >>> +check_in_port_flows_count hv1 $of1 2 >>> +check_in_port_flows_count hv2 $of2 0 >>> + >>> + >>> +AS_BOX([$(date +%H:%M:%S.%03N) Starting migrating migrator1]) >>> +check ovn-nbctl --wait=hv set Logical_Switch_Port migrator1 >>> options:requested-chassis=hv1,hv2 >>> +wait_column "$hv2_uuid" Port_Binding additional_chassis >>> logical_port=migrator1 >>> +wait_column "$hv2_uuid" Port_Binding additional_chassis >>> logical_port=mig1-cont >>> +# We should have two flows in table OFTABLE_PHY_TO_LOG from >>> in_port=migrator1 in hv1 and hv2. >>> +check_in_port_flows_count hv1 $of1 2 >>> +check_in_port_flows_count hv2 $of2 2 >>> + >>> +# While migration in underway, check that there is no fight between hv >>> +sleep 3 >>> +max_claims=2 >>> +check_claims migrator1 >>> +check_claims mig1-cont >>> + >>> +AS_BOX([$(date +%H:%M:%S.%03N) After migrating migrator1]) >>> +check ovn-nbctl --wait=hv set Logical_Switch_Port migrator1 >>> options:requested-chassis=hv2 >>> +wait_column "" Port_Binding additional_chassis logical_port=migrator1 >>> +wait_column "" Port_Binding additional_chassis logical_port=mig1-cont >>> +check_in_port_flows_count hv1 $of1 0 >>> +check_in_port_flows_count hv2 $of2 2 >>> + >>> +# Now do the same, but with iface-id set after starting migration >>> +of2=$(as hv2 ovs-vsctl --bare --columns ofport find Interface >>> name=vif-migrator2) >>> +# There is no ofport yet for hv1 => we cannot check/find a flows with >>> in_port=of1 >>> + >>> +AS_BOX([$(date +%H:%M:%S.%03N) Before migrating migrator2]) >>> +# We should have zero flows in table OFTABLE_PHY_TO_LOG from >>> in_port=migrator2 in hv1 and two flows in hv2. >>> +wait_column "" Port_Binding additional_chassis logical_port=migrator2 >>> +wait_column "" Port_Binding additional_chassis logical_port=mig2-cont >>> +check_in_port_flows_count hv2 $of2 2 >>> + >>> +check ovn-nbctl --wait=hv set Logical_Switch_Port migrator2 >>> options:requested-chassis=hv2,hv1 >>> +wait_column "" Port_Binding additional_chassis logical_port=migrator2 >>> +wait_column "" Port_Binding additional_chassis logical_port=mig2-cont >>> +# As no iface-id for migrator2 yet, migration not really started >>> +# We should have zero flows in table OFTABLE_PHY_TO_LOG from >>> in_port=migrator2 in hv1 and two flows in hv2. >>> +check_in_port_flows_count hv2 $of2 2 >>> + >>> +AS_BOX([$(date +%H:%M:%S.%03N) Starting migrating migrator2]) >>> +as hv1 ovs-vsctl -- add-port br-int vif-migrator2 \ >>> + -- set Interface vif-migrator2 >>> external-ids:iface-id=migrator2 >>> + >>> +wait_column "$hv1_uuid" Port_Binding additional_chassis >>> logical_port=migrator2 >>> +wait_column "$hv1_uuid" Port_Binding additional_chassis >>> logical_port=mig2-cont >>> +# We need for oflow to be created and handled by ovn-controller. Wait >>> sync would not be enough. >>> +OVS_WAIT_UNTIL([ >>> + of1=$(as hv1 ovs-vsctl --bare --columns ofport find Interface >>> name=vif-migrator2) >>> + test 1 -le "$of1" >>> +]) >>> + >>> +check ovn-nbctl --wait=hv sync >>> + >>> +# We should have two flows in table OFTABLE_PHY_TO_LOG from >>> in_port=migrator2 in hv1 and hv2. >>> +check_in_port_flows_count hv1 $of1 2 >>> +check_in_port_flows_count hv2 $of2 2 >>> + >>> +# While migration in underway, check that there is no fight between hv >>> +sleep 3 >>> +check_claims migrator2 >>> +check_claims mig2-cont >>> + >>> +AS_BOX([$(date +%H:%M:%S.%03N) After migrating migrator2]) >>> +check ovn-nbctl --wait=hv set Logical_Switch_Port migrator2 >>> options:requested-chassis=hv1 >>> +wait_column "" Port_Binding additional_chassis logical_port=migrator2 >>> +wait_column "" Port_Binding additional_chassis logical_port=mig2-cont >>> +check_in_port_flows_count hv1 $of1 2 >>> +check_in_port_flows_count hv2 $of2 0 >>> + >>> +OVN_CLEANUP([hv1],[hv2]) >>> + >>> +AT_CLEANUP >>> +]) >>> + >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([options:requested-chassis with hostname]) >>> >>> @@ -39199,7 +39353,7 @@ OVS_WAIT_FOR_OUTPUT([as hv2 ovs-ofctl dump-flows >>> br-int table=OFTABLE_PHY_TO_LOG >>> ]) >>> >>> # Add hv2 to lport Additional requested chassis as MAIN chassis >>> -# and check that no flows installed in table 0 in hv1 >>> +# and check that flows remain installed in table 0 in hv1 >>> check ovn-nbctl set Logical_Switch_Port lsp1 \ >>> options:requested-chassis=hv2,hv1 >>> >>> @@ -39214,8 +39368,8 @@ wait_column "$hv2_uuid" Port_Binding chassis >>> logical_port=lsp1 >>> OVS_WAIT_FOR_OUTPUT([as hv2 ovs-ofctl dump-flows br-int >>> table=OFTABLE_PHY_TO_LOG |grep priority=150|grep dl_vlan=7| grep -c >>> in_port=8], [0],[dnl >>> 1 >>> ]) >>> -OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int >>> table=OFTABLE_PHY_TO_LOG |grep priority=150|grep dl_vlan=7| grep -c >>> in_port=8], [1],[dnl >>> -0 >>> +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int >>> table=OFTABLE_PHY_TO_LOG |grep priority=150|grep dl_vlan=7| grep -c >>> in_port=8], [0],[dnl >>> +1 >>> ]) >>> >> >> This seems to be a "side effect" from the main chassis removal from >> the additional chassis. If that's the case we should keep that change >> separately. Would you agree? >> > I do not think so. This test tells that, when both hv are requested > chassis, so when hv1 is an additional_chassis, the flows remain installed > in hv1 - which is what we want as we are in the middle of the migration. > The change you refer to was somehow related to sb only, and was to avoid a > chassis to be main and additional chassis at the same time (which is > useless) > Ok that makes sense, thank you for the explanation. > >> >>> OVN_CLEANUP([hv1]) >>> -- >>> 2.47.1 >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> Thanks, >> Ales >> > Thanks > Xavier > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
