> Hi Lorenzo, Hi Mark,
thx for the fast review. > > Please add a NEWS item about this new feature. ack, I will add it in v2. > > 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? iiuc this code is executed for 'remote' ports (not router ones) but I would say we can keep the code and avoid to overwrite the configuration if the CMS sets requested-chassis options in lsp table. > > > } > > 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? ack, I will fix it in v2. > > 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. ack, I think we can use requested_chassis column and not requested_additional_chassis one. What do you think? Regards, Lorenzo > > > /* 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
