On Thu, Aug 26, 2021 at 8:24 AM Han Zhou <[email protected]> wrote: > > > > On Wed, Aug 25, 2021 at 11:00 PM Frode Nordahl <[email protected]> > wrote: > > > > On Thu, Aug 26, 2021 at 7:38 AM Han Zhou <[email protected]> wrote: > > > > > > > > > > > > On Thu, Aug 19, 2021 at 4:09 AM Frode Nordahl > > > <[email protected]> wrote: > > > > > > > > To allow for use of optional plugging support we add a new > > > > plugged_by column with weakRef to the Chassis table. The > > > > ovn-controller can monitor this column and only process events > > > > for its chassis UUID. > > > > > > > > ovn-northd will fill this column with UUID of Chassis referenced > > > > in Logical_Switch_Port options:requested-chassis when > > > > options:plug-type is defined. > > > > > > Thanks Frode. Please see my comments below. > > > > Thank you for taking the time to review Han, much appreciated. > > > > > > > > > > Signed-off-by: Frode Nordahl <[email protected]> > > > > --- > > > > northd/ovn-northd.c | 31 ++++++++++++++++++++++++++++++ > > > > ovn-nb.xml | 38 ++++++++++++++++++++++++++++++++++++ > > > > ovn-sb.ovsschema | 10 +++++++--- > > > > ovn-sb.xml | 15 +++++++++++++++ > > > > tests/ovn-northd.at | 47 +++++++++++++++++++++++++++++++++++++++++++++ > > > > 5 files changed, 138 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > > > index 3d8e21a4f..a66f92ddd 100644 > > > > --- a/northd/ovn-northd.c > > > > +++ b/northd/ovn-northd.c > > > > @@ -3222,6 +3222,35 @@ ovn_port_update_sbrec(struct northd_context *ctx, > > > > * ha_chassis_group cleared in the same transaction. */ > > > > sbrec_port_binding_set_ha_chassis_group(op->sb, NULL); > > > > } > > > > + > > > > + const char *plug_type; /* May be NULL. */ > > > > + const char *requested_chassis; /* May be NULL. */ > > > > + bool reset_plugged_by = false; > > > > + plug_type = smap_get(&op->nbsp->options, "plug-type"); > > > > + requested_chassis = smap_get(&op->nbsp->options, > > > > + "requested-chassis"); > > > > + if (plug_type && requested_chassis) { > > > > + const struct sbrec_chassis *chassis; /* May be NULL. */ > > > > + chassis = chassis_lookup_by_name(sbrec_chassis_by_name, > > > > + requested_chassis); > > > > + if (chassis) { > > > > + sbrec_port_binding_set_plugged_by(op->sb, chassis); > > > > > > Why do we need a separate column for this while we already have the value > > > in the SB port-binding options? > > > > The challenge is that we want the ovn-controllers to do as little > > processing as possible, and only react to unbound ports that are > > destined for them. In the initial RFC [0] I did the lookup based on > > Port_Binding:options, but that proved to not be very effective as you > > have to monitor all unbound ports and you can't use an index (you > > would only get the records with exactly and only the option you look > > for, which is normally not the case). In deployments with lots of load > > balancers there will be a lot of unbound ports. > > > Makes sense. Thanks for the explanation. To avoid the redundant information > in SB (and the confusion), it seems the "requested-chassis" option can be > removed from the SB and ovn-controller can instead use the new column for the > "requested-chassis" validation. But this can be a follow-up enhancement.
That is an interesting idea, maybe we should give the column a more generic name to prepare for that then, for example "requested_chassis" instead of the now proposed "plugged_by"? -- Frode Nordahl > > I also think you suggested using a separate column in the review of > > the initial RFC ;) > > It was not me :) but I am on the same page now. > > > > > 0: > > https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > > > > > > > > + } else { > > > > + reset_plugged_by = true; > > > > + static struct vlog_rate_limit rl = > > > > VLOG_RATE_LIMIT_INIT( > > > > + 1, 1); > > > > + VLOG_WARN_RL( > > > > + &rl, > > > > + "Unknown chassis '%s' set as " > > > > + "options:requested-chassis on LSP '%s'.", > > > > + requested_chassis, op->nbsp->name); > > > > > > Would it be normal that the chassis is not registered to SB when the NB > > > option is set? > > > > I would not think so, but it is a safeguard in the event the CMS puts > > an invalid value into the requested-chassis option. We need to make > > sure chassis != NULL and I thought it would be reasonable to log the > > fact if it ever happened. > > > > OK, I am fine with it. > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
