| Message ID | 20210908121640.3921795-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-northd: Fix update of a mac prefix. | 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 08/09/2021 13:16, Ilya Maximets wrote: > 'smap_add' doesn't check for duplicates. This leads to having > more than one entry with a 'mac_prefix' key in the 'options' smap. > > 'ovsdb_datum_sort_unique' removes duplicates before sending the > transaction. It does that by sorting values and keeping the first one > per key. In normal case 'mac_prefix' transitions from nothing to > random and it works fine. However, northd also tries to change > all-zero prefix to random one and this fails, because all-zero prefix > goes first in a sorted map and a new random value gets dropped from > the transaction. This makes northd to update macs of all ports > on every iteration with a new random mac prefix. > > Fix that by replacing smap value and not relying on IDL for removing > duplicates. Thanks, this looks good to me and I tested it and it worked. I couldn't find this behavior (ability to assign a random mac) documented anywhere (for example in ovn-nb.5). It might be worth documenting it. Acked-by: Mark D. Gray <mark.d.gray@redhat.com> > > Fixes: 39242c106eff ("northd: refactor and split some IPAM functions") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > northd/ovn-northd.c | 2 +- > tests/ovn.at | 13 +++++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index ee761cef0..eef752542 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -14212,7 +14212,7 @@ ovnnb_db_run(struct northd_context *ctx, > struct smap options; > smap_clone(&options, &nb->options); > > - smap_add(&options, "mac_prefix", mac_addr_prefix); > + smap_replace(&options, "mac_prefix", mac_addr_prefix); > > if (!monitor_mac) { > eth_addr_random(&svc_monitor_mac_ea); > diff --git a/tests/ovn.at b/tests/ovn.at > index 5104a6895..30625ec37 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -8030,6 +8030,19 @@ mac_prefix=$(ovn-nbctl --wait=sb get NB_Global . options:mac_prefix | tr -d \") > port_addr=$(ovn-nbctl get Logical-Switch-Port p91 dynamic_addresses | tr -d \") > AT_CHECK([test "$port_addr" = "${mac_prefix}:00:00:09"], [0], []) > > +# set mac_prefix to all-zeroes and check it is allocated in a random manner > +ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="00:00:00:00:00:00" > +ovn-nbctl ls-add sw14 > +ovn-nbctl --wait=sb set Logical-Switch sw14 other_config:mac_only=true > +ovn-nbctl --wait=sb lsp-add sw14 p141 -- lsp-set-addresses p141 dynamic > + > +mac_prefix=$(ovn-nbctl --wait=sb get NB_Global . options:mac_prefix | tr -d \") > +port_addr=$(ovn-nbctl get Logical-Switch-Port p141 dynamic_addresses | tr -d \") > +AT_CHECK([test "$mac_prefix" != "00:00:00:00:00:00"], [0], []) > +AT_CHECK([test "$port_addr" = "${mac_prefix}:00:00:0a"], [0], []) > +ovn-nbctl --wait=sb lsp-del sw14 p141 > +ovn-nbctl --wait=sb ls-del sw14 > + > ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="00:11:22" > ovn-nbctl ls-add sw10 > ovn-nbctl --wait=sb set Logical-Switch sw10 other_config:ipv6_prefix="ae01::" >
I merged this to master. Should this be backported as well? On 9/9/21 6:31 AM, Mark Gray wrote: > On 08/09/2021 13:16, Ilya Maximets wrote: >> 'smap_add' doesn't check for duplicates. This leads to having >> more than one entry with a 'mac_prefix' key in the 'options' smap. >> >> 'ovsdb_datum_sort_unique' removes duplicates before sending the >> transaction. It does that by sorting values and keeping the first one >> per key. In normal case 'mac_prefix' transitions from nothing to >> random and it works fine. However, northd also tries to change >> all-zero prefix to random one and this fails, because all-zero prefix >> goes first in a sorted map and a new random value gets dropped from >> the transaction. This makes northd to update macs of all ports >> on every iteration with a new random mac prefix. >> >> Fix that by replacing smap value and not relying on IDL for removing >> duplicates. > > Thanks, this looks good to me and I tested it and it worked. I couldn't > find this behavior (ability to assign a random mac) documented anywhere > (for example in ovn-nb.5). It might be worth documenting it. > > Acked-by: Mark D. Gray <mark.d.gray@redhat.com> > >> >> Fixes: 39242c106eff ("northd: refactor and split some IPAM functions") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> northd/ovn-northd.c | 2 +- >> tests/ovn.at | 13 +++++++++++++ >> 2 files changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index ee761cef0..eef752542 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -14212,7 +14212,7 @@ ovnnb_db_run(struct northd_context *ctx, >> struct smap options; >> smap_clone(&options, &nb->options); >> >> - smap_add(&options, "mac_prefix", mac_addr_prefix); >> + smap_replace(&options, "mac_prefix", mac_addr_prefix); >> >> if (!monitor_mac) { >> eth_addr_random(&svc_monitor_mac_ea); >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 5104a6895..30625ec37 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -8030,6 +8030,19 @@ mac_prefix=$(ovn-nbctl --wait=sb get NB_Global . options:mac_prefix | tr -d \") >> port_addr=$(ovn-nbctl get Logical-Switch-Port p91 dynamic_addresses | tr -d \") >> AT_CHECK([test "$port_addr" = "${mac_prefix}:00:00:09"], [0], []) >> >> +# set mac_prefix to all-zeroes and check it is allocated in a random manner >> +ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="00:00:00:00:00:00" >> +ovn-nbctl ls-add sw14 >> +ovn-nbctl --wait=sb set Logical-Switch sw14 other_config:mac_only=true >> +ovn-nbctl --wait=sb lsp-add sw14 p141 -- lsp-set-addresses p141 dynamic >> + >> +mac_prefix=$(ovn-nbctl --wait=sb get NB_Global . options:mac_prefix | tr -d \") >> +port_addr=$(ovn-nbctl get Logical-Switch-Port p141 dynamic_addresses | tr -d \") >> +AT_CHECK([test "$mac_prefix" != "00:00:00:00:00:00"], [0], []) >> +AT_CHECK([test "$port_addr" = "${mac_prefix}:00:00:0a"], [0], []) >> +ovn-nbctl --wait=sb lsp-del sw14 p141 >> +ovn-nbctl --wait=sb ls-del sw14 >> + >> ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="00:11:22" >> ovn-nbctl ls-add sw10 >> ovn-nbctl --wait=sb set Logical-Switch sw10 other_config:ipv6_prefix="ae01::" >> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 9/13/21 20:29, Mark Michelson wrote:
> I merged this to master. Should this be backported as well?
Setting options:mac_prefix="00:00:00:00:00:00" triggers
infinite mac changes on all dynamic ports, so IMO this
could be considered as a bug fix suitable for backporting.
Best regards, Ilya Maximets.
Thanks, I backported this to branch-21.03 and branch-21.06. On 9/13/21 2:32 PM, Ilya Maximets wrote: > On 9/13/21 20:29, Mark Michelson wrote: >> I merged this to master. Should this be backported as well? > > Setting options:mac_prefix="00:00:00:00:00:00" triggers > infinite mac changes on all dynamic ports, so IMO this > could be considered as a bug fix suitable for backporting. > > Best regards, Ilya Maximets. >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index ee761cef0..eef752542 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -14212,7 +14212,7 @@ ovnnb_db_run(struct northd_context *ctx, struct smap options; smap_clone(&options, &nb->options); - smap_add(&options, "mac_prefix", mac_addr_prefix); + smap_replace(&options, "mac_prefix", mac_addr_prefix); if (!monitor_mac) { eth_addr_random(&svc_monitor_mac_ea); diff --git a/tests/ovn.at b/tests/ovn.at index 5104a6895..30625ec37 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -8030,6 +8030,19 @@ mac_prefix=$(ovn-nbctl --wait=sb get NB_Global . options:mac_prefix | tr -d \") port_addr=$(ovn-nbctl get Logical-Switch-Port p91 dynamic_addresses | tr -d \") AT_CHECK([test "$port_addr" = "${mac_prefix}:00:00:09"], [0], []) +# set mac_prefix to all-zeroes and check it is allocated in a random manner +ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="00:00:00:00:00:00" +ovn-nbctl ls-add sw14 +ovn-nbctl --wait=sb set Logical-Switch sw14 other_config:mac_only=true +ovn-nbctl --wait=sb lsp-add sw14 p141 -- lsp-set-addresses p141 dynamic + +mac_prefix=$(ovn-nbctl --wait=sb get NB_Global . options:mac_prefix | tr -d \") +port_addr=$(ovn-nbctl get Logical-Switch-Port p141 dynamic_addresses | tr -d \") +AT_CHECK([test "$mac_prefix" != "00:00:00:00:00:00"], [0], []) +AT_CHECK([test "$port_addr" = "${mac_prefix}:00:00:0a"], [0], []) +ovn-nbctl --wait=sb lsp-del sw14 p141 +ovn-nbctl --wait=sb ls-del sw14 + ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="00:11:22" ovn-nbctl ls-add sw10 ovn-nbctl --wait=sb set Logical-Switch sw10 other_config:ipv6_prefix="ae01::"
'smap_add' doesn't check for duplicates. This leads to having more than one entry with a 'mac_prefix' key in the 'options' smap. 'ovsdb_datum_sort_unique' removes duplicates before sending the transaction. It does that by sorting values and keeping the first one per key. In normal case 'mac_prefix' transitions from nothing to random and it works fine. However, northd also tries to change all-zero prefix to random one and this fails, because all-zero prefix goes first in a sorted map and a new random value gets dropped from the transaction. This makes northd to update macs of all ports on every iteration with a new random mac prefix. Fix that by replacing smap value and not relying on IDL for removing duplicates. Fixes: 39242c106eff ("northd: refactor and split some IPAM functions") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- northd/ovn-northd.c | 2 +- tests/ovn.at | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)