When generating Port_Binding records ovn-northd tries to reuse the tunnel_key value from the original SB record, if any available.
However, there's no check for tunnel_keys that would conflict with newly allocated keys for new records. In order to avoid that, we don't reuse stale Port_Binding entries, i.e., their "datapath" field doesn't match the Datapath_Binding record associated with the logical switch/router they're part of. One way to reproduce the issue is: $ ovn-nbctl ls-add ls1 $ ovn-nbctl ls-add ls2 $ ovn-nbctl lsp-add ls1 lsp1 $ ovn-nbctl lsp-add ls2 lsp2 $ ovn-nbctl --wait=sb sync $ ovn-nbctl lsp-del lsp2 -- lsp-add ls1 lsp2 Another option to reproduce the issue is with HA_Chassis_Group: $ ovn-nbctl ls-add ls1 $ ovn-nbctl ls-add ls2 $ ovn-nbctl lsp-add ls1 lsp1 $ ovn-nbctl lsp-add ls2 lsp2 $ ovn-nbctl lsp-set-type lsp2 external $ ovn-nbctl ha-chassis-group-add chg1 $ ovn-nbctl ha-chassis-group-add-chassis chg1 chassis-1 30 $ chg1_uuid=$(ovn-nbctl --bare --columns _uuid list ha_Chassis_Group .) $ ovn-nbctl set logical_switch_port lsp2 ha_chassis_group=${chg1_uuid} $ ovn-nbctl lsp-del lsp2 -- lsp-add ls1 lsp2 Reported-by: Dan Williams <d...@redhat.com> Reported-at: https://bugzilla.redhat.com/1828637 Signed-off-by: Dumitru Ceara <dce...@redhat.com> --- northd/ovn-northd.c | 6 +++-- tests/ovn-northd.at | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index de59452..e37b346 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -2029,7 +2029,7 @@ join_logical_ports(struct northd_context *ctx, const struct nbrec_logical_switch_port *nbsp = od->nbs->ports[i]; struct ovn_port *op = ovn_port_find(ports, nbsp->name); - if (op) { + if (op && op->sb->datapath == od->sb) { if (op->nbsp || op->nbrp) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); @@ -2123,7 +2123,7 @@ join_logical_ports(struct northd_context *ctx, } struct ovn_port *op = ovn_port_find(ports, nbrp->name); - if (op) { + if (op && op->sb->datapath == od->sb) { if (op->nbsp || op->nbrp) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); @@ -2175,7 +2175,7 @@ join_logical_ports(struct northd_context *ctx, char *redirect_name = ovn_chassis_redirect_name(nbrp->name); struct ovn_port *crp = ovn_port_find(ports, redirect_name); - if (crp) { + if (crp && crp->sb->datapath == od->sb) { crp->derived = true; ovn_port_set_nb(crp, NULL, nbrp); ovs_list_remove(&crp->list); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index a4469c7..a0109ae 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1414,3 +1414,59 @@ lr_uuid=$(ovn-nbctl --columns _uuid list Logical_Router .) AT_CHECK[test ${nb_uuid} = ${lr_uuid}] AT_CLEANUP + +AT_SETUP([ovn -- check reconcile stale tunnel keys]) +ovn_start + +ovn-nbctl ls-add ls1 +ovn-nbctl ls-add ls2 +ovn-nbctl lsp-add ls1 lsp1 +ovn-nbctl lsp-add ls2 lsp2 +AT_CHECK([ovn-nbctl --wait=sb sync], [0]) + +# Ports are bound on different datapaths so it's expected that they both +# get tunnel_key == 1. +AT_CHECK([test 1 = $(ovn-sbctl --bare --columns tunnel_key find \ +port_binding logical_port=lsp1)]) +AT_CHECK([test 1 = $(ovn-sbctl --bare --columns tunnel_key find \ +port_binding logical_port=lsp2)]) + +ovn-nbctl lsp-del lsp2 -- lsp-add ls1 lsp2 +AT_CHECK([ovn-nbctl --wait=sb sync], [0]) + +AT_CHECK([test 1 = $(ovn-sbctl --bare --columns tunnel_key find \ +port_binding logical_port=lsp1)]) +AT_CHECK([test 2 = $(ovn-sbctl --bare --columns tunnel_key find \ +port_binding logical_port=lsp2)]) + +# ovn-northd should allocate a new tunnel_key for lsp1 or lsp2 to maintain +# unique DB indices. +AT_CHECK([test ${pb1_key} != ${pb2_key}]) + +AT_CLEANUP + +AT_SETUP([ovn -- check reconcile stale Ha_Chassis_Group]) +ovn_start + +ovn-nbctl ls-add ls1 +ovn-nbctl ls-add ls2 +ovn-nbctl lsp-add ls1 lsp1 +ovn-nbctl lsp-add ls2 lsp2 + +ovn-nbctl lsp-set-type lsp2 external + +ovn-nbctl ha-chassis-group-add chg1 +ovn-nbctl ha-chassis-group-add-chassis chg1 chassis-1 30 + +chg1_uuid=$(ovn-nbctl --bare --columns _uuid list Ha_Chassis_Group .) +ovn-nbctl set logical_switch_port lsp2 ha_chassis_group=${chg1_uuid} +AT_CHECK([ovn-nbctl --wait=sb sync], [0]) + +# Move lsp2 from ls2 to ls1. This should also remove the SB HA_Chassis_Group +# record. +ovn-nbctl lsp-del lsp2 -- lsp-add ls1 lsp2 +AT_CHECK([ovn-nbctl --wait=sb sync], [0]) + +AT_CHECK([test 0 = $(ovn-sbctl list Ha_Chassis_Group | wc -l)]) + +AT_CLEANUP _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev