On Thu, Aug 10, 2023 at 7:51 PM Numan Siddique <num...@ovn.org> wrote:
>
> On Wed, Jul 26, 2023 at 3:58 PM Lorenzo Bianconi
> <lorenzo.bianc...@redhat.com> wrote:
> >
> > ovn supports creating remote logical ports. An user
> > can set requested-chassis option for a logical switch port
> > to the remote chassis and ovn-northd can bind it to that chassis.
> > This is required for OVN IC in ovn-k8s. Right now ovn-k8s
> > ovnkube-controller after creating a remote logical port, sets the
> > chassis column of the corresponding port binding in SB DB to the
> > remote chassis. This process can be implemented in ovn-northd.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2217930
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
>
> Hi Lorenzo,
>
> Thanks for the patch. I applied this patch to the main branch with
> the below changes.
> There were still few cases which the patch didn't cover. So I added
> those and the corresponding tests.
Hi Lorenzo and others,
I'm sorry. I introduced a bug with the below changes. If a CMS
creates a remote port and binds the port binding manually without
setting the requested-chassis, then ovn-northd will set the chassis to
NULL now. This would break ovn-kubernetes and also ovn-ic since both
don't set the requested-chassis column of remote ports.
So I reverted this comment.
@Lorenzo Bianconi Please submit another version fixing this scenario
and other test scenarios which your original patch missed.
Thanks
Numan
>
> -------------------------
> diff --git a/northd/northd.c b/northd/northd.c
> index 4b08ab2fd..35d149ddc 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3517,6 +3517,33 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>
> sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
> } else {
> + if (lsp_is_remote(op->nbsp)) {
> + /* ovn-northd is suppose to set port_binding for remote ports
> + * if requested chassis is a remote chassis. */
> + const struct sbrec_chassis *remote_chassis = NULL;
> + bool is_remote_chassis = false;
> + if (op->sb->requested_chassis) {
> + is_remote_chassis =
> + smap_get_bool(&op->sb->requested_chassis->other_config,
> + "is-remote", false);
> + }
> + if (is_remote_chassis) {
> + remote_chassis = op->sb->requested_chassis;
> + }
> + sbrec_port_binding_set_chassis(op->sb, remote_chassis);
> + } else {
> + /* Its not a remote port but if the chassis is set and if its a
> + * remote chassis then clear it. */
> + if (op->sb->chassis) {
> + bool is_remote_chassis =
> + smap_get_bool(&op->sb->chassis->other_config,
> + "is-remote", false);
> + if (is_remote_chassis) {
> + sbrec_port_binding_set_chassis(op->sb, NULL);
> + }
> + }
> + }
> +
> if (!lsp_is_router(op->nbsp)) {
> uint32_t queue_id = smap_get_int(
> &op->sb->options, "qdisc_queue_id", 0);
> @@ -3601,14 +3628,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
> * ha_chassis_group cleared in the same transaction. */
> sbrec_port_binding_set_ha_chassis_group(op->sb, NULL);
> }
> -
> - /* ovn-northd is supposed to set port_binding for remote ports
> - * if requested chassis is marked as remote
> - */
> - if (lsp_is_remote(op->nbsp)) {
> - sbrec_port_binding_set_chassis(op->sb,
> - op->sb->requested_chassis);
> - }
> } else {
> const char *chassis = NULL;
> if (op->peer && op->peer->od && op->peer->od->nbr) {
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index edcb062f2..804d988d1 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9736,15 +9736,63 @@ ovn_start
>
> check ovn-sbctl chassis-add remote-ch0 geneve 127.0.0.1
> check ovn-sbctl set chassis remote-ch0 other_config:is-remote=true
> -wait_row_count Chassis 1
> +
> +check ovn-sbctl chassis-add local-ch0 geneve 127.0.0.2
> +wait_row_count Chassis 2
> +
> +remote_ch_uuid=$(fetch_column Chassis _uuid name=remote-ch0)
> +local_ch_uuid=$(fetch_column Chassis _uuid name=local-ch0)
> +
> +as northd ovn-appctl -t NORTHD_TYPE vlog/set dbg
>
> check ovn-nbctl ls-add sw0
> check ovn-nbctl lsp-add sw0 sw0-r1 -- lsp-set-type sw0-r1 remote
> check ovn-nbctl lsp-set-options sw0-r1 requested-chassis=remote-ch0
> wait_for_ports_up sw0-r1
>
> +# Make sure it is bound by remote-ch0
> +check_column $remote_ch_uuid Port_Binding chassis logical_port=sw0-r1
> +
> check ovn-nbctl remove logical_switch_port sw0-r1 options requested-chassis
> wait_row_count nb:Logical_Switch_Port 1 up=false name=sw0-r1
> +check_column '' Port_Binding chassis logical_port=sw0-r1
> +
> +# Set the requested-chassis to local-ch0. ovn-northd should not
> +# bind it. But before that bind again to remote-ch0. This becomes
> +# easier to test for the local-ch0 scenario.
> +check ovn-nbctl lsp-set-options sw0-r1 requested-chassis=remote-ch0
> +wait_for_ports_up sw0-r1
> +check_column $remote_ch_uuid Port_Binding chassis logical_port=sw0-r1
> +check ovn-nbctl lsp-set-options sw0-r1 requested-chassis=local-ch0
> +wait_row_count nb:Logical_Switch_Port 1 up=false name=sw0-r1
> +check_column '' Port_Binding chassis logical_port=sw0-r1
> +
> +# Set the requested-chassis to unknown chassis. ovn-northd should not
> +# bind it. But before that bind again to remote-ch0. This becomes
> +# easier to test for the local-ch0 scenario.
> +check ovn-nbctl lsp-set-options sw0-r1 requested-chassis=remote-ch0
> +wait_for_ports_up sw0-r1
> +check_column $remote_ch_uuid Port_Binding chassis logical_port=sw0-r1
> +check ovn-nbctl lsp-set-options sw0-r1 requested-chassis=foo
> +wait_row_count nb:Logical_Switch_Port 1 up=false name=sw0-r1
> +check_column '' Port_Binding chassis logical_port=sw0-r1
> +
> +# Change the port type to normal and ovn-northd should not bind it.
> +check ovn-nbctl lsp-set-options sw0-r1 requested-chassis=remote-ch0
> +wait_for_ports_up sw0-r1
> +check_column $remote_ch_uuid Port_Binding chassis logical_port=sw0-r1
> +check ovn-nbctl lsp-set-type sw0-r1 ''
> +wait_row_count nb:Logical_Switch_Port 1 up=false name=sw0-r1
> +check_column '' Port_Binding chassis logical_port=sw0-r1
> +
> +# Change back to type to remote and ovn-northd should bind it.
> +check ovn-nbctl lsp-set-type sw0-r1 remote
> +wait_for_ports_up sw0-r1
> +check_column $remote_ch_uuid Port_Binding chassis logical_port=sw0-r1
> +
> +# Set the type to router and ovn-northd should not claim it.
> +check ovn-nbctl lsp-set-type sw0-r1 router
> +check_column '' Port_Binding chassis logical_port=sw0-r1
>
> AT_CLEANUP
> ])
>
> ------------------------------------------------------------------------------------------------------------------
>
> Thanks
> Numan
>
> > ---
> > Changes since v1:
> > - add NEWS entry
> > - do not remove ovn-ic code to bind sb to gw chassis
> > - simplify codebase
> > ---
> > NEWS | 2 ++
> > ic/ovn-ic.c | 5 +++++
> > northd/northd.c | 8 ++++++++
> > tests/ovn-ic.at | 2 ++
> > tests/ovn-northd.at | 20 ++++++++++++++++++++
> > tests/ovn.at | 27 +++++++++++++++++++++++++++
> > 6 files changed, 64 insertions(+)
> >
> > diff --git a/NEWS b/NEWS
> > index 8275877f9..be900c95b 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -10,6 +10,8 @@ Post v23.06.0
> > - To allow optimizing ovn-controller's monitor conditions for the regular
> > VIF case, ovn-controller now unconditionally monitors all sub-ports
> > (ports with parent_port set).
> > + - Introduce support for binding remote ports in ovn-northd if the CMS
> > sets
> > + requested-chassis option for a remote logical switch port.
> >
> > OVN v23.06.0 - 01 Jun 2023
> > --------------------------
> > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> > index 6f31037ec..72709ce78 100644
> > --- a/ic/ovn-ic.c
> > +++ b/ic/ovn-ic.c
> > @@ -646,6 +646,11 @@ sync_remote_port(struct ic_context *ctx,
> > /* Sync tunnel key from ISB to NB */
> > sync_lsp_tnl_key(lsp, isb_pb->tunnel_key);
> >
> > + /* Skip port binding if it is already requested by the CMS. */
> > + if (smap_get(&lsp->options, "requested-chassis")) {
> > + return;
> > + }
> > +
> > /* Sync gateway from ISB to SB */
> > if (isb_pb->gateway[0]) {
> > if (!sb_pb->chassis || strcmp(sb_pb->chassis->name,
> > isb_pb->gateway)) {
> > diff --git a/northd/northd.c b/northd/northd.c
> > index b9605862e..e5cd6b6ab 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -3601,6 +3601,14 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> > *ovnsb_txn,
> > * ha_chassis_group cleared in the same transaction. */
> > sbrec_port_binding_set_ha_chassis_group(op->sb, NULL);
> > }
> > +
> > + /* ovn-northd is supposed to set port_binding for remote ports
> > + * if requested chassis is marked as remote
> > + */
> > + if (lsp_is_remote(op->nbsp)) {
> > + sbrec_port_binding_set_chassis(op->sb,
> > + op->sb->requested_chassis);
> > + }
> > } else {
> > const char *chassis = NULL;
> > if (op->peer && op->peer->od && op->peer->od->nbr) {
> > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
> > index ceee45092..05c9b2825 100644
> > --- a/tests/ovn-ic.at
> > +++ b/tests/ovn-ic.at
> > @@ -337,6 +337,7 @@ ovn-nbctl lsp-set-addresses lsp-ts1-lr1 router
> > ovn-nbctl lsp-set-type lsp-ts1-lr1 router
> > ovn-nbctl --wait=hv lsp-set-options lsp-ts1-lr1 router-port=lrp-lr1-ts1
> >
> > +ovn_as az2 ovn-nbctl lsp-set-options lsp-ts1-lr1 requested-chassis=gw1
> > AT_CHECK([ovn_as az2 ovn-nbctl show | uuidfilt], [0], [dnl
> > switch <0> (ts1)
> > port lsp-ts1-lr1
> > @@ -351,6 +352,7 @@ lsp-ts1-lr1,remote
> > ovn-nbctl lrp-set-gateway-chassis lrp-lr1-ts1 gw1
> > OVS_WAIT_UNTIL([ovn_as az2 ovn-sbctl show | grep lsp-ts1-lr1])
> >
> > +ovn_as az2 ovn-nbctl lsp-set-options lsp-ts1-lr1 requested-chassis=""
> > ovn-nbctl lrp-del-gateway-chassis lrp-lr1-ts1 gw1
> > OVS_WAIT_WHILE([ovn_as az2 ovn-sbctl show | grep lsp-ts1-lr1])
> >
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 3e06f14c9..69569f3a7 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -9688,3 +9688,23 @@ AT_CHECK([grep "lr_in_gw_redirect" R1flows |sed
> > s'/table=../table=??/' |sort], [
> >
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([Remote port binding])
> > +AT_KEYWORDS([remote-port-binding])
> > +ovn_start
> > +
> > +check ovn-sbctl chassis-add remote-ch0 geneve 127.0.0.1
> > +check ovn-sbctl set chassis remote-ch0 other_config:is-remote=true
> > +wait_row_count Chassis 1
> > +
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl lsp-add sw0 sw0-r1 -- lsp-set-type sw0-r1 remote
> > +check ovn-nbctl lsp-set-options sw0-r1 requested-chassis=remote-ch0
> > +wait_for_ports_up sw0-r1
> > +
> > +check ovn-nbctl remove logical_switch_port sw0-r1 options requested-chassis
> > +wait_row_count nb:Logical_Switch_Port 1 up=false name=sw0-r1
> > +
> > +AT_CLEANUP
> > +])
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 24da9174e..a27e3eec2 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -26129,6 +26129,19 @@ done
> > # XXX This should be more systematic.
> > sleep 2
> >
> > +# Populate requested-chassis options for remote lsps
> > +for az in $(seq 1 $n_az); do
> > + ovn_as az${az}
> > + for ts in $(seq 1 $n_ts); do
> > + for i in $(seq 1 $n_ts); do
> > + if [[ $i -eq ${az} ]]; then
> > + continue
> > + fi
> > + check ovn-nbctl lsp-set-options lsp-ts${ts}-lr${i}-${ts}
> > requested-chassis=gw$i
> > + done
> > + done
> > +done
> > +
> > ovn-ic-nbctl show > ic-nbctl.dump
> > AT_CAPTURE_FILE([ic-nbctl.dump])
> >
> > @@ -26352,6 +26365,13 @@ check ovn-nbctl lsp-add ts ts-lr3 \
> >
> > wait_for_ports_up
> >
> > +ovn_as az1
> > +check ovn-nbctl lsp-set-options ts-lr2 requested-chassis=hv2
> > +check ovn-nbctl lsp-set-options ts-lr3 requested-chassis=hv2
> > +
> > +ovn_as az2
> > +check ovn-nbctl lsp-set-options ts-lr1 requested-chassis=hv1
> > +
> > dnl Enable unregistered IP multicast flooding and IP multicast relay.
> > ovn_as az1
> > check ovn-nbctl set logical_switch ls1 other_config:mcast_snoop="true" \
> > @@ -26557,6 +26577,13 @@ check ovn-nbctl lsp-add ts ts-lr3 \
> >
> > wait_for_ports_up
> >
> > +ovn_as az1
> > +check ovn-nbctl lsp-set-options ts-lr2 requested-chassis=hv2
> > +check ovn-nbctl lsp-set-options ts-lr3 requested-chassis=hv2
> > +
> > +ovn_as az2
> > +check ovn-nbctl lsp-set-options ts-lr1 requested-chassis=hv1
> > +
> > dnl Enable IP multicast snooping and IP multicast relay. Reports are
> > dnl forwarded statically.
> > ovn_as az1
> > --
> > 2.41.0
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
