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