On Jul 8, 2016 8:04 PM, "Russell Bryant" <russ...@ovn.org> wrote:
>
>
>
> On Thu, Jul 7, 2016 at 11:47 PM, Numan Siddique <nusid...@redhat.com>
wrote:
>>
>> Thanks for the review Russel.
>> Please see comments inline.
>>
>> On Fri, Jul 8, 2016 at 1:19 AM, Russell Bryant <russ...@ovn.org> wrote:
>>>
>>> Thanks for working on this! A few comments ...
>>>
>>> On Mon, Jul 4, 2016 at 7:10 AM, Numan Siddique <nusid...@redhat.com>
wrote:
>>>>
>>>> ovn-controller will now bind the l2gateway logical ports.
>>>>
>>>> Signed-Off-by: Numan Siddique <nusid...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
