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