On Wed, Aug 25, 2021 at 11:32 PM Frode Nordahl <[email protected]>
wrote:
>
> 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"?
>
Yes, I agree.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
