Hi Lorenzo,
Please add a NEWS item about this new feature.
I have more down below.
On 7/24/23 11:52, 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 authomized in ovn-northd.
I've never heard the word "authomized" before. I don't know if this is
jargon that I have never heard before or if it is a typo :)
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2217930
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
ic/ovn-ic.c | 42 +++---------------------------------------
northd/northd.c | 21 ++++++++++++++++++++-
tests/ovn-ic.at | 2 ++
tests/ovn-northd.at | 20 ++++++++++++++++++++
tests/ovn.at | 27 +++++++++++++++++++++++++++
5 files changed, 72 insertions(+), 40 deletions(-)
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 6f31037ec..c12e345ed 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -506,20 +506,6 @@ get_lrp_address_for_sb_pb(struct ic_context *ctx,
return peer->n_mac ? *peer->mac : NULL;
}
-static const struct sbrec_chassis *
-find_sb_chassis(struct ic_context *ctx, const char *name)
-{
- const struct sbrec_chassis *key =
- sbrec_chassis_index_init_row(ctx->sbrec_chassis_by_name);
- sbrec_chassis_index_set_name(key, name);
-
- const struct sbrec_chassis *chassis =
- sbrec_chassis_index_find(ctx->sbrec_chassis_by_name, key);
- sbrec_chassis_index_destroy_row(key);
-
- return chassis;
-}
-
static void
sync_lsp_tnl_key(const struct nbrec_logical_switch_port *lsp,
int64_t isb_tnl_key)
@@ -622,13 +608,10 @@ sync_local_port(struct ic_context *ctx,
/* For each remote port:
* - Sync from ISB to NB
- * - Sync gateway from ISB to SB
*/
static void
-sync_remote_port(struct ic_context *ctx,
- const struct icsbrec_port_binding *isb_pb,
- const struct nbrec_logical_switch_port *lsp,
- const struct sbrec_port_binding *sb_pb)
+sync_remote_port(const struct icsbrec_port_binding *isb_pb,
+ const struct nbrec_logical_switch_port *lsp)
{
/* Sync address from ISB to NB */
if (isb_pb->address[0]) {
@@ -645,25 +628,6 @@ sync_remote_port(struct ic_context *ctx,
/* Sync tunnel key from ISB to NB */
sync_lsp_tnl_key(lsp, isb_pb->tunnel_key);
-
- /* Sync gateway from ISB to SB */
- if (isb_pb->gateway[0]) {
- if (!sb_pb->chassis || strcmp(sb_pb->chassis->name, isb_pb->gateway)) {
- const struct sbrec_chassis *chassis =
- find_sb_chassis(ctx, isb_pb->gateway);
- if (!chassis) {
- VLOG_DBG("Chassis %s is not found in SB, syncing from ISB "
- "to SB skipped for logical port %s.",
- isb_pb->gateway, lsp->name);
- return;
- }
- sbrec_port_binding_set_chassis(sb_pb, chassis);
- }
- } else {
- if (sb_pb->chassis) {
- sbrec_port_binding_set_chassis(sb_pb, NULL);
- }
- }
I don't feel great about removing this. This will break existing
configurations that expect the port to automatically be bound to the
gateway chassis.
I also don't understand why this needs to be removed. The removed
behavior only works on logical switch ports of type "router". Typically
"requested-chassis" is set on VIF ports, correct?
}
static void
@@ -813,7 +777,7 @@ port_binding_run(struct ic_context *ctx,
if (!sb_pb) {
continue;
}
- sync_remote_port(ctx, isb_pb, lsp, sb_pb);
+ sync_remote_port(isb_pb, lsp);
}
} else {
VLOG_DBG("Ignore lsp %s on ts %s with type %s.",
diff --git a/northd/northd.c b/northd/northd.c
index b9605862e..618c79c61 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3773,11 +3773,30 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
sbrec_port_binding_set_tunnel_key(op->sb, op->tunnel_key);
}
+ bool up = false;
+ /* ovn-northd is supposed to set port_binding for remote ports
+ * if requested chassis is marked as remote
+ */
+ if (op->nbsp && lsp_is_remote(op->nbsp)) {
+ const struct sbrec_chassis *chassis_sb = NULL;
+ const char *requested_chassis = smap_get(
+ &op->nbsp->options, "requested-chassis");
+ if (requested_chassis) {
+ chassis_sb = chassis_lookup(
+ sbrec_chassis_by_name, sbrec_chassis_by_hostname,
+ requested_chassis);
+ if (chassis_sb) {
+ up = smap_get_bool(&chassis_sb->other_config,
+ "is-remote", false);
+ }
+ }
+ sbrec_port_binding_set_chassis(op->sb, chassis_sb);
+ }
+
Two things:
1) This code has been inserted into a section where op may be a logical
router port or a logical switch port. The commit message only mentions
logical switch ports, so should this code be moved?
2) "requested-chassis" can accept a comma-separated list of values. The
first in this list is the intended primary chassis for the logical
switch port. Right now, the code will not work properly if a
comma-separated list of requested-chassis is provided. You should
probably also add this to your new test below.
/* ovn-controller will update 'Port_Binding.up' only if it was explicitly
* set to 'false'.
*/
if (!op->sb->n_up) {
- bool up = false;
sbrec_port_binding_set_up(op->sb, &up, 1);
}
}
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 882a548db..8334b3ad4 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -26106,6 +26106,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])
@@ -26329,6 +26342,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" \
@@ -26534,6 +26554,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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
