[ovs-dev] [PATCH] ovn: Support chassis option in "l2gateway" logical ports

Numan Siddique nusiddiq at redhat.com
Fri Jul 8 04:47:53 UTC 2016


Thanks for the review Russel.
Please see comments inline.

On Fri, Jul 8, 2016 at 1:19 AM, Russell Bryant <russell at ovn.org> wrote:

> Thanks for working on this!  A few comments ...
>
> On Mon, Jul 4, 2016 at 7:10 AM, Numan Siddique <nusiddiq at redhat.com>
> wrote:
>
>> ovn-controller will now bind the l2gateway logical ports.
>>
>> Signed-Off-by: Numan Siddique <nusiddiq at redhat.com>
>> ---
>>  ovn/TODO                 |  9 ---------
>>  ovn/controller/binding.c | 33 +++++++++++++++++++++++----------
>>  ovn/ovn-nb.xml           |  7 +++++++
>>  ovn/ovn-sb.xml           |  6 ++++++
>>  tests/ovn.at             |  5 +----
>>  5 files changed, 37 insertions(+), 23 deletions(-)
>>
>> diff --git a/ovn/TODO b/ovn/TODO
>> index 3f358c2..5b9d829 100644
>> --- a/ovn/TODO
>> +++ b/ovn/TODO
>> @@ -249,12 +249,3 @@ large.
>>  ** Support log option.
>>
>>  * Software L2 gateway
>>
>
> You can remove this line, as well.
>
>
>> -
>> -** Support "chassis" option in Logical_Switch_Port with type of
>> "l2gateway".
>> -
>> -   Right now an "l2gateway" port is bound to a chassis by setting the
>> "chassis"
>> -   column of the port binding in the southbound database directly.  We
>> should
>> -   support a "chassis" option in the "options" column of the
>> -   "Logical_Switch_Port" in the northbound database.  This would bring
>> -   "l2gateway" into alignment with how chassis binding is done for L3
>> gateways
>> -   (a "chassis" option for Logical_Router).
>> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
>> index 4e5c1df..d28cd80 100644
>> --- a/ovn/controller/binding.c
>> +++ b/ovn/controller/binding.c
>> @@ -222,16 +222,29 @@ consider_local_datapath(struct controller_ctx *ctx,
>> struct shash *lports,
>>              }
>>              sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
>>          }
>> -    } else if (!strcmp(binding_rec->type, "l2gateway")
>> -               && binding_rec->chassis == chassis_rec) {
>> -        /* A locally bound L2 gateway port.
>> -         *
>> -         * ovn-controller does not bind gateway ports itself.
>> -         * Choosing a chassis for a gateway port is left
>> -         * up to an entity external to OVN. */
>> -        sset_add(&all_lports, binding_rec->logical_port);
>> -        add_local_datapath(local_datapaths, binding_rec,
>> -                           &binding_rec->header_.uuid);
>> +    } else if (!strcmp(binding_rec->type, "l2gateway")) {
>> +        const char *chassis_id = smap_get(&binding_rec->options,
>> "chassis");
>>
>
> Can we change this to "l2gateway-chassis" instead of just "chassis"?  That
> will make it consistent with the L3 gateway option name, which is
> "gateway-chassis", corresponding to the "gateway" port type.my ri
>

 Logical_Router.options is supporting "chassis"
​

​
in OVN NB DB
https://github.com/openvswitch/ovs/blob/master/ovn/ovn-nb.xml#L746
​ and Port_Binding.options is supporting "gateway-chassis" in OVN SB DB (
https://github.com/openvswitch/ovs/blob/master/ovn/ovn-sb.xml#L1600
​).

Shall I follow the same to be consistent ?
​


​

>
>
>> +        if (!chassis_id) {
>>
>
>
> I think one case is missing here.  If the chassis option is changed, the
> chassis column won't be cleared.
>
> In the normal case, it should work itself out, because another
> ovn-controller will change the chassis column.  However, if the option is
> set to a bad value, or for a host that is currently down, the port will
> stay bound to this chassis, when really it should be set to NULL.
>
> I think you can just change this to:
>
>
​Thanks for pointing this edge case.

    if (!chassis_id || strcmp(chassis_id chassis_rec->name)) {
>
> +            if ((binding_rec->chassis == chassis_rec) &&
>> ctx->ovnsb_idl_txn) {
>> +                VLOG_INFO("Releasing l2gateway port %s from this
>> chassis.",
>> +                          binding_rec->logical_port);
>> +                sbrec_port_binding_set_chassis(binding_rec, NULL);
>> +            }
>> +            return;
>> +        }
>> +
>> +        if (binding_rec->chassis == chassis_rec) {
>> +            return;
>> +        }
>> +
>> +        if (!strcmp(chassis_id, chassis_rec->name) &&
>> ctx->ovnsb_idl_txn) {
>> +            VLOG_INFO("Claiming l2gateway port %s for this chassis.",
>> +                      binding_rec->logical_port);
>> +            sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
>> +            sset_add(&all_lports, binding_rec->logical_port);
>> +            add_local_datapath(local_datapaths, binding_rec,
>> +                               &binding_rec->header_.uuid);
>> +        }
>
>      } else if (chassis_rec && binding_rec->chassis == chassis_rec
>>                 && strcmp(binding_rec->type, "gateway")) {
>>          if (ctx->ovnsb_idl_txn) {
>> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
>> index ff2e695..db56e5d 100644
>> --- a/ovn/ovn-nb.xml
>> +++ b/ovn/ovn-nb.xml
>> @@ -197,6 +197,13 @@
>>            uses its local configuration to determine exactly how to
>> connect to
>>            this network.
>>          </column>
>> +
>> +        <column name="options" key="chassis">
>>
>
> This will need to change to "l2gateway-chassis".
>
>
>> +          Required. The chassis on which the <code>l2gateway</code>
>> logical
>> +          port should be bound to. <code>ovn-controller</code> running
>> on the
>> +          defined chassis will connect this logical port to the physical
>> network.
>> +        </column>
>> +
>>        </group>
>>
>>        <group title="Options for vtep ports">
>> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
>> index 759513f..6bc7208 100644
>> --- a/ovn/ovn-sb.xml
>> +++ b/ovn/ovn-sb.xml
>> @@ -1657,6 +1657,12 @@ tcp.flags = RST;
>>          </p>
>>        </column>
>>
>> +      <column name="options" key="chassis">
>>
>
> "l2gateway-chassis".
>
>
>> +        Required. <code>ovn-controller</code> running on the defined
>> chassis
>> +        will bind the <code>l2gateway</code> logical port and connect it
>> to the
>> +        physical network.
>> +      </column>
>> +
>>        <column name="tag">
>>          If set, indicates that the gateway is connected to a specific
>>          VLAN on the physical network. The VLAN ID is used to match
>>
>
> The documentation for the "chassis" column of Port_Binding will need to be
> updated in this file, as well.  It still refers to the old behavior.
>
>
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index feb68d3..78311ed 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -1325,7 +1325,7 @@ ovn-nbctl lsp-set-addresses lp2 f0:00:00:00:00:02
>>
>>  ovn-nbctl lsp-add lsw0 lp-gw
>>  ovn-nbctl lsp-set-type lp-gw l2gateway
>> -ovn-nbctl lsp-set-options lp-gw network_name=physnet1
>> +ovn-nbctl lsp-set-options lp-gw network_name=physnet1 chassis=hv_gw
>>
>
> l2gateway-chassis after the option name change.
>
>
>>  ovn-nbctl lsp-set-addresses lp-gw unknown
>>
>>  net_add n1               # Network to connect hv1, hv2, and gw
>> @@ -1355,9 +1355,6 @@ ovs-vsctl add-br br-phys2
>>  net_attach n2 br-phys2
>>  ovs-vsctl set open . external_ids:ovn-bridge-mappings="physnet1:br-phys2"
>>
>> -# Bind our gateway port to the hv_gw chassis
>> -ovn-sbctl lport-bind lp-gw hv_gw
>> -
>>  # Add hv3 on the other side of the GW
>>  sim_add hv3
>>  as hv3
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>
>
> --
> Russell Bryant
>



More information about the dev mailing list