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 ?
>
>
>> + 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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
