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

Ben Pfaff blp at ovn.org
Wed Jul 3 23:41:05 UTC 2019


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!

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.

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

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.

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

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.

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.

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.)?

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

Thanks,

Ben.


More information about the dev mailing list