[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