On Thu, Nov 14, 2024 at 10:21 AM Xavier Simonart <[email protected]> wrote:
>
> Hi Ales
>
> Thanks for the patch!
>
> On Thu, Nov 14, 2024 at 1:53 PM Ales Musil <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
> Thanks
> Xavier
>
> > _______________________________________________
> > 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
