| Message ID | a026f582-c75d-a2f4-d770-b8e3c0aff578@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
Thanks for working on this! A few comments ... On Mon, Jul 4, 2016 at 7:10 AM, Numan Siddique <nusiddiq@redhat.com> wrote: > ovn-controller will now bind the l2gateway logical ports. > > Signed-Off-by: Numan Siddique <nusiddiq@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 > + 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: 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@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
Thanks for the review Russel. Please see comments inline. On Fri, Jul 8, 2016 at 1:19 AM, Russell Bryant <russell@ovn.org> wrote: > Thanks for working on this! A few comments ... > > On Mon, Jul 4, 2016 at 7:10 AM, Numan Siddique <nusiddiq@redhat.com> > wrote: > >> ovn-controller will now bind the l2gateway logical ports. >> >> Signed-Off-by: Numan Siddique <nusiddiq@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@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >> > > > > -- > Russell Bryant >
On Thu, Jul 7, 2016 at 11:47 PM, Numan Siddique <nusiddiq@redhat.com> wrote: > Thanks for the review Russel. > Please see comments inline. > > On Fri, Jul 8, 2016 at 1:19 AM, Russell Bryant <russell@ovn.org> wrote: > >> Thanks for working on this! A few comments ... >> >> On Mon, Jul 4, 2016 at 7:10 AM, Numan Siddique <nusiddiq@redhat.com> >> wrote: >> >>> ovn-controller will now bind the l2gateway logical ports. >>> >>> Signed-Off-by: Numan Siddique <nusiddiq@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.
On Jul 8, 2016 8:04 PM, "Russell Bryant" <russell@ovn.org> wrote: > > > > On Thu, Jul 7, 2016 at 11:47 PM, Numan Siddique <nusiddiq@redhat.com> wrote: >> >> Thanks for the review Russel. >> Please see comments inline. >> >> On Fri, Jul 8, 2016 at 1:19 AM, Russell Bryant <russell@ovn.org> wrote: >>> >>> Thanks for working on this! A few comments ... >>> >>> On Mon, Jul 4, 2016 at 7:10 AM, Numan Siddique <nusiddiq@redhat.com> wrote: >>>> >>>> ovn-controller will now bind the l2gateway logical ports. >>>> >>>> Signed-Off-by: Numan Siddique <nusiddiq@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
On Fri, Jul 8, 2016 at 9:36 AM, Numan Siddique <nusiddiq@redhat.com> wrote: > On Jul 8, 2016 8:04 PM, "Russell Bryant" <russell@ovn.org> wrote: > > > > > > > > On Thu, Jul 7, 2016 at 11:47 PM, Numan Siddique <nusiddiq@redhat.com> > wrote: > >> > >> Thanks for the review Russel. > >> Please see comments inline. > >> > >> On Fri, Jul 8, 2016 at 1:19 AM, Russell Bryant <russell@ovn.org> wrote: > >>> > >>> Thanks for working on this! A few comments ... > >>> > >>> On Mon, Jul 4, 2016 at 7:10 AM, Numan Siddique <nusiddiq@redhat.com> > wrote: > >>>> > >>>> ovn-controller will now bind the l2gateway logical ports. > >>>> > >>>> Signed-Off-by: Numan Siddique <nusiddiq@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.
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 - -** 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"); + if (!chassis_id) { + 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"> + 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"> + 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 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 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
ovn-controller will now bind the l2gateway logical ports. Signed-Off-by: Numan Siddique <nusiddiq@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(-)