Re: [ovs-dev] [PATCH v2 ovn] northd: support binding remote ports in ovn-northd

2023-08-10 Thread Numan Siddique 
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(>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(>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(
>  >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 ''

Re: [ovs-dev] [PATCH v2 ovn] northd: support binding remote ports in ovn-northd

2023-08-10 Thread Numan Siddique 
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(>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(>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(
 >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

[ovs-dev] [PATCH v2 ovn] northd: support binding remote ports in ovn-northd

2023-07-26 Thread Lorenzo Bianconi 
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(>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