[ovs-dev] [PATCH 2/4] OVN: Add IGMP SB definitions and ovn-controller support

Dumitru Ceara dceara at redhat.com
Thu Jul 4 07:55:45 UTC 2019


On Thu, Jul 4, 2019 at 1:41 AM Ben Pfaff <blp at ovn.org> wrote:
>
> On Tue, Jul 02, 2019 at 09:07:45AM +0200, Dumitru Ceara wrote:
> > A new IP_Multicast table is added to Southbound DB. This table stores the
> > multicast related configuration for each datapath. Each row will be
> > populated by ovn-northd and will control:
> > - if IGMP Snooping is enabled or not, the snooping table size and multicast
> >   group idle timeout.
> > - if IGMP Querier is enabled or not (only if snooping is enabled too), query
> >   interval, query source addresses (Ethernet and IP) and the max-response
> >   field to be stored in outgoing queries.
> > - an additional "seq_no" column is added such that ovn-sbctl or if needed a
> >   CMS can flush currently learned groups. This can be achieved by incrementing
> >   the "seq_no" value.
>
> Thank you for the patches!

Thanks for reviewing the patches!

>
> In the IP_Multicast table schema, I would consider whether most of the
> columns should be optional ("min": 0, "max": 1).  Optional columns are
> the usual way to allow for defaults in OVSDB, so that an empty value
> means to use the default.  This is important if we ever want to change a
> default.  For example, consider "enabled".  A database client that
> inserts a row without setting "enabled" will get the default value.  The
> default value for a Boolean column is false, and this default can't be
> changed, so if we ever wanted the default to be true, it would be
> difficult.  On the other hand, for an "optional Boolean" column, the
> default is the empty set [], which the reader can interpret as whatever
> default is appropriate.

Makes sense indeed, I'll make the columns optional where applicable.

>
> For documenting the "seq_no" column, I recommend specifying *what* uses
> the column internally.  ovn-controller perhaps?

Ack. I'll add more documentation in v2.

>
> There's a lot of passive voice in the documentation.  Passive voice is
> ambiguous, especially to people without enough context:  For example:
>
>     <column name="eth_src">
>       Ethernet address to be used as source for self originated IGMP queries.
>     </column>
>
> I *think* this means that ovn-controller uses this value.  But it could
> be written in active voice to say so:
>
>     <column name="eth_src">
>       Hypervisor nodes use this Ethernet address as the source for
>       self-originated IGMP queries.
>     </column>
>
> Or there could be a <group> that contains multiple columns with a header
> like this (I'm just guessing):
>
>   The <code>ovn-controller</code> process that runs on OVN hypervisor
>   nodes uses the following columns to determine field values in IGMP
>   queries that it originates.
>
> etc.

Thanks for pointing this out. I'll rework the documentation and try to
make it clearer in v2.

>
> I don't see any tests for parsing the new igmp action.  Those should go
> into the "ovn -- action parsing" test in ovn.at.

Sure, will do.

>
> I don't see any documentation for what the igmp action does.  This
> should go into the documentation for the "actions" column in the
> Logical_Flow table.

Sure, will do.

>
> The bit-fields in struct ip_mcast_snoop_cfg seem odd.  Any reason not to
> use plain "bool"s?  They would not take more space.
>

When I first wrote the structure i wasn't completely sure how many
bools i will need in the end and it felt like a waste. But indeed it
doesn't make sense now. I'll change it in v2 and if ever needed in the
future we can move back to bit fields.

> The new static variables in pinctrl.c have synchronization annotations
> in comments.  Any reason not to use OVS_GUARDED or OVS_GUARDED_BY (and
> then annotate functions with OVS_REQUIRES etc.)?

No real reason, I just forgot to add the annotations. I'll fix it in v2.

>
> The new VLOG_*() calls should probably be rate-limited like the existing
> ones in pinctrl.c.

Sure, i'll add rate limiting in v2.

Thanks,
Dumitru

>
> Thanks,
>
> Ben.


More information about the dev mailing list