[ovs-dev] [PATCH v3 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

Darrell Ball dlu998 at gmail.com
Wed Aug 14 20:25:27 UTC 2019


On Wed, Aug 14, 2019 at 9:47 AM Yi-Hung Wei <yihung.wei at gmail.com> wrote:

> On Mon, Aug 12, 2019 at 7:46 PM Darrell Ball <dlu998 at gmail.com> wrote:
> >> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> >> index f7c6eb8983cd..c0a2242ad345 100644
> >> --- a/vswitchd/vswitch.ovsschema
> >> +++ b/vswitchd/vswitch.ovsschema
> >> @@ -1,9 +1,14 @@
> >>  {"name": "Open_vSwitch",
> >> - "version": "8.0.0",
> >> - "cksum": "3962141869 23978",
> >> + "version": "8.1.0",
> >> + "cksum": "1635647160 26090",
> >>   "tables": {
> >>     "Open_vSwitch": {
> >>       "columns": {
> >> +       "datapaths": {
> >> +         "type": {"key": {"type": "string"},
> >
> >
> > I had a minor comment in V2 about using an enum here for key - 'system'
> or 'netdev'
> > Does it work or is there worry that other datapath types will likely
> develop
> > and we will need to update the enum ?
>
> Thanks for the review.
>
> I discussed this with Justin about this.  We currently do not limit
> the datapath type in the Bridge table in ovsdb schema. So it might
> just keep it as is to be consistent with the the Bridge table.
>

Lets defer for now.
It can be treated as a separate issue and fixed later in one shot.


>
> >> +                  "value": {"type": "uuid",
> >> +                            "refTable": "Datapath"},
> >> +                  "min": 0, "max": "unlimited"}},
> >>
> >>         "bridges": {
> >>           "type": {"key": {"type": "uuid",
> >>                            "refTable": "Bridge"},
> >> @@ -629,6 +634,48 @@
> >>                    "min": 0, "max": "unlimited"},
> >>           "ephemeral": true}},
> >>       "indexes": [["target"]]},
> >> +   "Datapath": {
> >> +     "columns": {
> >> +       "datapath_version": {
> >> +         "type": "string"},
> >> +       "ct_zones": {
> >> +         "type": {"key": {"type": "integer",
> >> +                          "minInteger": 0,
> >> +                          "maxInteger": 65535},
> >> +                  "value": {"type": "uuid",
> >> +                            "refTable": "CT_Zone"},
> >> +                  "min": 0, "max": "unlimited"}},
> >
> >
> > minor comment from V2; I think
> > +                  "min": 0, "max": "unlimited"}},
> > should be
> > +                  "min": 0, "max": "65536"}},
>
> Since ct_zones is a map, so  the maximum size is already limited the key
> range.
> +         "type": {"key": {"type": "integer",
> +                          "minInteger": 0,
> +                          "maxInteger": 65535},
>
> Keep "max" as "unlimited" also has the benefit that we do not need to
> update it when the range of value is changed.  There are other cases
> in ovsdb schema that has similar behavior, for  example:


>        "queues": {
>          "type": {"key": {"type": "integer",
>                           "minInteger": 0,
>                           "maxInteger": 4294967295},
>                   "value": {"type": "uuid",
>                             "refTable": "Queue"},
>                   "min": 0, "max": "unlimited"}},
>
>        "mappings": {
>          "type": {"key": {"type": "integer",
>                           "minInteger": 0,
>                           "maxInteger": 16777215},
>                   "value": {"type": "integer",
>                           "minInteger": 0,
>                           "maxInteger": 4095},
>                   "min": 0, "max": "unlimited"}}}}}}
>

lets defer


>
> >> +       "external_ids": {
> >> +         "type": {"key": "string", "value": "string",
> >> +                  "min": 0, "max": "unlimited"}}}},
> >> +   "CT_Zone": {
> >> +     "columns": {
> >> +       "timeout_policy": {
> >> +         "type": {"key": {"type": "uuid",
> >> +                          "refTable": "CT_Timeout_Policy"},
> >> +                  "min": 0, "max": 1}},
> >> +       "external_ids": {
> >> +         "type": {"key": "string", "value": "string",
> >> +                  "min": 0, "max": "unlimited"}}}},
> >> +   "CT_Timeout_Policy": {
> >> +     "columns": {
> >> +       "timeouts": {
> >> +         "type": {"key": {"type" : "string",
> >> +                          "enum": ["set", ["tcp_syn_sent",
> "tcp_syn_recv",
> >> +                                           "tcp_established",
> "tcp_fin_wait",
> >> +                                           "tcp_close_wait",
> "tcp_last_ack",
> >> +                                           "tcp_time_wait",
> "tcp_close",
> >> +                                           "tcp_syn_sent2",
> "tcp_retransmit",
> >> +                                           "tcp_unack", "udp_first",
> >> +                                           "udp_single",
> "udp_multiple",
> >> +                                           "icmp_first",
> "icmp_reply"]]},
> >> +                  "value": {"type" : "integer",
> >> +                            "minInteger" : 0,
> >> +                            "maxInteger" : 4294967295},
> >> +                  "min": 0, "max": "unlimited"}},
> >
> >
> > Should it be ?
> > +                  "min": 0, "max": "16"}},
> >
> > or will this create upgrade issues ?
>
> Similarly, I would keep "max" as "unlimited" here, or we will need to
> update "max" when more L4 protocol timeouts are supported.
>

If we ever supported L4 port number timers and we also used this table,
we would need to update the enum, anyways and also the ovs-vsctl command.

We already have protection in place thru. ovs-vsctl limiting the total
number of parameters
to 18, which includes 16 timeouts, so my suggestion is already in place if
the user is using
ovs-vsctl, which is likely.

{"add-zone-tp", 2, 18, "", pre_get_zone, cmd_add_zone_tp, NULL,
     "--may-exist", RW},


>
>
> >> +  <table name="Datapath">
> >> +    <p>
> >> +      Configuration for a datapath within <ref table="Open_vSwitch"/>.
> >> +    </p>
> >> +    <p>
> >> +      A datapath is responsible for providing the packet handling in
> Open
> >> +      vSwitch.  There are two primary datapath implementations used by
> >> +      Open vSwitch: kernel and userspace.  Kernel datapath
> >> +      implementations are available for Linux and Hyper-V, and selected
> >> +      as <code>system</code> in the <ref column="datapath_type"/>
> column
> >> +      of the <ref table="Bridge"/> table.  The userspace datapath is
> used
> >> +      by DPDK and AF-XDP, and is selected as <code>netdev</code> in the
> >> +      <ref column="datapath_type"/> column of the <ref table="Bridge"/>
> >> +      table.
> >> +    </p>
> >> +    <p>
> >> +      A datapath of a particular type is shared by all the bridges
> that use
> >> +      that datapath.  Thus, configurations applied to this table affect
> >> +      all bridges that use this datapath.
> >> +    </p>
> >> +
> >> +    <column name="datapath_version">
> >> +      <p>
> >> +        Reports the version number of the Open vSwitch datapath in use.
> >> +        This allows management software to detect and report
> discrepancies
> >> +        between Open vSwitch userspace and datapath versions.  (The
> <ref
> >> +        column="ovs_version" table="Open_vSwitch"/> column in the <ref
> >> +        table="Open_vSwitch"/> reports the Open vSwitch userspace
> version.)
> >> +        The version reported depends on the datapath in use:
> >
> >
> > What is the recommended usage here for the userspace datapath.
> > Should we just say to refer to  'ovs_version' in table="Open_vSwitch"/>
> for
> > the userspace datapath case or set the same value here ?
>
> I think we will set the same value here for userspace datapath, so
> that the management software can use the same logic to detect and
> report discrepancies between vswitchd and datapath versions.
>

As discussed, lets go with Option 1 as mentioned earler, which involves
just adding a comment to guide users.


>
> Thanks,
>
> -YI-Hung
>


More information about the dev mailing list