On Fri, Jul 8, 2016 at 9:36 AM, Numan Siddique <nusid...@redhat.com> wrote:
> 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.
>
Great, I will review today.
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
