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

[Numan Siddique]

On Jul 8, 2016 8:04 PM, "Russell Bryant" <russell at ovn.org> wrote:
>
>
>
> On Thu, Jul 7, 2016 at 11:47 PM, Numan Siddique <nusiddiq at redhat.com>
wrote:
>>
>> 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 ?
>
>
> I would use "l2gateway-chassis" on both.  In the l2gateway case, we're
setting an option on the port, and all other Logical_Switch_Port options
are copied to Port_Binding directly.

Thanks. The v2 of the patch does the same.

Regards
Numan

>
> The "gateway-chassis" case is a little different since the original
option is set on Logical_Router.  I probably would have chosen to make it
"gateway-chassis" in both cases, but I guess it's not a big deal.
>
> --
> Russell Bryant