Re: [ovs-dev] [PATCH v2 ovn] northd: support binding remote ports in ovn-northd
On Thu, Aug 10, 2023 at 7:51 PM Numan Siddique wrote: > > On Wed, Jul 26, 2023 at 3:58 PM Lorenzo Bianconi > 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 > > 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_co
Re: [ovs-dev] [PATCH v2 ovn] northd: support binding remote ports in ovn-northd
On Wed, Jul 26, 2023 at 3:58 PM Lorenzo Bianconi 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 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. - 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-nbct
[ovs-dev] [PATCH v2 ovn] northd: support binding remote ports in ovn-northd
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 --- 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