| Message ID | 20241114125312.1411928-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: Make sure that we keep explicit chassis for remote PB. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Hi Ales Thanks for the patch! On Thu, Nov 14, 2024 at 1:53 PM Ales Musil <amusil@redhat.com> wrote: > Ensure the chassis for remote port binding is not cleared when it's > set explicitly by CMS. The requested-chassis is still preferred > method to direct changes to SB DB, this option remains for > backward compatibility. > As a side note, ovn-ic also sets the chassis for remote pb. Hence, without this patch, we had ovn-ic fighting to set pb->chassis while northd fights to clear it (e.g. run "grep -c chassis ./az1/ovn-sb/ovn-sb.db" in tests such as "duplicate NB route adv/learn", and you will see chassis being set and cleared 100s of times w/o the patch. This patch fixes it! > > Fixes: 0688d7e834b9 ("northd: Allow multichassis port to be bound on remote > chassis.") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > northd/northd.c | 5 ++++- > tests/ovn.at | 34 ++++++++++++++++++++++++++++++++++ > 2 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 64b2e3859..aed1d425f 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3209,8 +3209,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > "is-remote", false)) { > sbrec_port_binding_set_chassis(op->sb, > > op->sb->requested_chassis); > - } else { > + smap_add(&options, "is-remote-nb-bound", "true"); > + } else if (smap_get_bool(&op->sb->options, > + "is-remote-nb-bound", false)) { > sbrec_port_binding_set_chassis(op->sb, NULL); > + smap_add(&options, "is-remote-nb-bound", "false"); > } > } else if (op->sb->chassis && > smap_get_bool(&op->sb->chassis->other_config, > diff --git a/tests/ovn.at b/tests/ovn.at > index 92e1ffadc..865feab2c 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -39686,3 +39686,37 @@ OVN_CLEANUP([hv1]) > > AT_CLEANUP > ]) > + > +# ovn-kubernetes used to explicitly set chassis, > +# test if we don't accidentally remove it. > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([Remote port with explicit SB chassis]) > +ovn_start > + > +net_add n1 > + > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.11 > + > +check ovn-sbctl chassis-add hv2 geneve 192.168.0.12 \ > + -- set chassis hv2 other_config:is-remote=true > + > +check ovn-nbctl ls-add ls > +check ovn-nbctl lsp-add ls lsp > +check ovn-nbctl lsp-set-type lsp remote > + > It's missing --wait=sb or the following fetch_column Port_Binding might fail(e.g. if northd is slow) > +remote_chassis=$(fetch_column Chassis _uuid name=hv2) > +lsp_uuid=$(fetch_column Port_Binding _uuid logical_port=lsp) > + > +check ovn-sbctl set Port_Binding $lsp_uuid chassis=$remote_chassis > + > +check ovn-nbctl --wait=hv sync > + > +check_column $remote_chassis Port_Binding chassis logical_port=lsp > + > +OVN_CLEANUP([hv1]) > + > +AT_CLEANUP > +]) > -- > 2.47.0 > > Other than the missing --wait=sb, looks good to me, thanks. Acked-by: Xavier Simonart <xsimonar@redhat.com> Thanks Xavier > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Thu, Nov 14, 2024 at 10:21 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > Hi Ales > > Thanks for the patch! > > On Thu, Nov 14, 2024 at 1:53 PM Ales Musil <amusil@redhat.com> wrote: > > > Ensure the chassis for remote port binding is not cleared when it's > > set explicitly by CMS. The requested-chassis is still preferred > > method to direct changes to SB DB, this option remains for > > backward compatibility. > > > As a side note, ovn-ic also sets the chassis for remote pb. > Hence, without this patch, we had ovn-ic fighting to set pb->chassis while > northd fights to clear it (e.g. run "grep -c chassis > ./az1/ovn-sb/ovn-sb.db" in tests such as "duplicate NB route adv/learn", > and you will see chassis being set and cleared 100s of times w/o the patch. > This patch fixes it! > > > > > > Fixes: 0688d7e834b9 ("northd: Allow multichassis port to be bound on remote > > chassis.") > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > northd/northd.c | 5 ++++- > > tests/ovn.at | 34 ++++++++++++++++++++++++++++++++++ > > 2 files changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 64b2e3859..aed1d425f 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -3209,8 +3209,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > > *ovnsb_txn, > > "is-remote", false)) { > > sbrec_port_binding_set_chassis(op->sb, > > > > op->sb->requested_chassis); > > - } else { > > + smap_add(&options, "is-remote-nb-bound", "true"); > > + } else if (smap_get_bool(&op->sb->options, > > + "is-remote-nb-bound", false)) { > > sbrec_port_binding_set_chassis(op->sb, NULL); > > + smap_add(&options, "is-remote-nb-bound", "false"); > > } > > } else if (op->sb->chassis && > > smap_get_bool(&op->sb->chassis->other_config, > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 92e1ffadc..865feab2c 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -39686,3 +39686,37 @@ OVN_CLEANUP([hv1]) > > > > AT_CLEANUP > > ]) > > + > > +# ovn-kubernetes used to explicitly set chassis, > > +# test if we don't accidentally remove it. > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([Remote port with explicit SB chassis]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +check ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.11 > > + > > +check ovn-sbctl chassis-add hv2 geneve 192.168.0.12 \ > > + -- set chassis hv2 other_config:is-remote=true > > + > > +check ovn-nbctl ls-add ls > > +check ovn-nbctl lsp-add ls lsp > > +check ovn-nbctl lsp-set-type lsp remote > > + > > > It's missing --wait=sb or the following fetch_column Port_Binding might > fail(e.g. if northd is slow) > Thanks. I added the missing "--wait=sb" as suggested by Xavier and applied this patch to main. Numan > > +remote_chassis=$(fetch_column Chassis _uuid name=hv2) > > +lsp_uuid=$(fetch_column Port_Binding _uuid logical_port=lsp) > > + > > +check ovn-sbctl set Port_Binding $lsp_uuid chassis=$remote_chassis > > + > > +check ovn-nbctl --wait=hv sync > > + > > +check_column $remote_chassis Port_Binding chassis logical_port=lsp > > + > > +OVN_CLEANUP([hv1]) > > + > > +AT_CLEANUP > > +]) > > -- > > 2.47.0 > > > > Other than the missing --wait=sb, looks good to me, thanks. > Acked-by: Xavier Simonart <xsimonar@redhat.com> > > Thanks > Xavier > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/northd/northd.c b/northd/northd.c index 64b2e3859..aed1d425f 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3209,8 +3209,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, "is-remote", false)) { sbrec_port_binding_set_chassis(op->sb, op->sb->requested_chassis); - } else { + smap_add(&options, "is-remote-nb-bound", "true"); + } else if (smap_get_bool(&op->sb->options, + "is-remote-nb-bound", false)) { sbrec_port_binding_set_chassis(op->sb, NULL); + smap_add(&options, "is-remote-nb-bound", "false"); } } else if (op->sb->chassis && smap_get_bool(&op->sb->chassis->other_config, diff --git a/tests/ovn.at b/tests/ovn.at index 92e1ffadc..865feab2c 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -39686,3 +39686,37 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +# ovn-kubernetes used to explicitly set chassis, +# test if we don't accidentally remove it. +OVN_FOR_EACH_NORTHD([ +AT_SETUP([Remote port with explicit SB chassis]) +ovn_start + +net_add n1 + +sim_add hv1 +as hv1 +check ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.11 + +check ovn-sbctl chassis-add hv2 geneve 192.168.0.12 \ + -- set chassis hv2 other_config:is-remote=true + +check ovn-nbctl ls-add ls +check ovn-nbctl lsp-add ls lsp +check ovn-nbctl lsp-set-type lsp remote + +remote_chassis=$(fetch_column Chassis _uuid name=hv2) +lsp_uuid=$(fetch_column Port_Binding _uuid logical_port=lsp) + +check ovn-sbctl set Port_Binding $lsp_uuid chassis=$remote_chassis + +check ovn-nbctl --wait=hv sync + +check_column $remote_chassis Port_Binding chassis logical_port=lsp + +OVN_CLEANUP([hv1]) + +AT_CLEANUP +])
Ensure the chassis for remote port binding is not cleared when it's set explicitly by CMS. The requested-chassis is still preferred method to direct changes to SB DB, this option remains for backward compatibility. Fixes: 0688d7e834b9 ("northd: Allow multichassis port to be bound on remote chassis.") Signed-off-by: Ales Musil <amusil@redhat.com> --- northd/northd.c | 5 ++++- tests/ovn.at | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-)