[ovs-dev] [PATCH ovn] Support additional 'vif-id' options in OVS inteface for claiming an lport.
Mark Gray
mark.d.gray at redhat.com
Fri Aug 20 16:40:46 UTC 2021
On 18/08/2021 22:35, numans at ovn.org wrote:
> From: Numan Siddique <numans at ovn.org>
>
> In order for the ovn-controller to claim a logical port, it maps the
> OVS interface external_ids:iface-id to the port binding's logical_port
> column. This patch adds support for another option - 'vif-id'.
> CMS needs to set the same key-value in Logical_Switch_Port.options
> column.
>
> ovn-controller will claim the OVS interface only if
> external_ids:iface-id matches with the Port_Binding.logical_port
> and external_ids:vif-id matches with the Port_Binding.options:vif-id.
> This is not mandatory. If Port_binding.options:vif-id is not
> set, then OVS Interface.external_ids:vif-id if set is ignored.
>
Some small changes below otherwise:
Acked-by: Mark D. Gray <mark.d.gray at redhat.com>
It seems like the CMS could encode this information in the name but
perhaps there are good reasons why it cannot. This doesn't seem like a
very intrusive change.
I think the name is a little confusing as it suggests that vif-id and
iface-id are two different things whereas they are closely related.
Could we call it something like "iface-id-version"? In this way, I think
it more closely describes what is going on?
> This support is added so that CMS can uniquely identify the OVS
> interface to the corresponding logical port.
>
> The referenced bugzilla and this ovn-k8s commit [1] has additional details.
>
> [1] - https://github.com/ovn-org/ovn-kubernetes/commit/22bed6a10c6
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1995333
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
> controller/binding.c | 47 ++++++++++++++-
> ovn-nb.xml | 8 +++
> ovn-sb.xml | 8 +++
> tests/ovn.at | 141 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 201 insertions(+), 3 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 52eb47b79..743809a21 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -600,6 +600,9 @@ static struct binding_lport *binding_lport_check_and_cleanup(
> struct binding_lport *, struct shash *b_lports);
>
> static char *get_lport_type_str(enum en_lport_type lport_type);
> +static bool is_ovs_iface_matches_lport_vif_id(
> + const struct ovsrec_interface *,
> + const struct sbrec_port_binding *);
nit: I think this could be renamed to ovs_iface_matches....
>
> void
> related_lports_init(struct related_lports *rp)
> @@ -1165,9 +1168,30 @@ consider_vif_lport(const struct sbrec_port_binding *pb,
>
> struct binding_lport *b_lport = NULL;
> if (lbinding) {
> - struct shash *binding_lports =
> - &b_ctx_out->lbinding_data->lports;
> - b_lport = local_binding_add_lport(binding_lports, lbinding, pb, LP_VIF);
> + bool add_b_lport = true;
Is this flag needed? Could it not be written with if/else?
> +
> + /* Make sure that the pb's vif-id if set matches with the
> + * lbinding's vif-id. */
> + if (lbinding->iface &&
> + !is_ovs_iface_matches_lport_vif_id(lbinding->iface, pb)) {
> + /* We can't associate the b_lport for this local_binding
> + * because the vif-id doesn't match. */
> + add_b_lport = false;
> +
> + b_lport = local_binding_get_primary_lport(lbinding);
> + if (b_lport) {
> + binding_lport_delete(&b_ctx_out->lbinding_data->lports,
> + b_lport);
> + b_lport = NULL;
> + }
> + }
> +
> + if (add_b_lport) {
> + struct shash *binding_lports =
> + &b_ctx_out->lbinding_data->lports;
> + b_lport = local_binding_add_lport(binding_lports, lbinding, pb,
> + LP_VIF);
> + }
> }
>
> return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in,
> @@ -2850,3 +2874,20 @@ cleanup:
>
> return b_lport;
> }
> +
> +
> +static bool
> +is_ovs_iface_matches_lport_vif_id(const struct ovsrec_interface *iface,
> + const struct sbrec_port_binding *pb)
> +{
> + const char *pb_vif_id = smap_get(&pb->options, "vif-id");
> + const char *vif_id = smap_get(&iface->external_ids, "vif-id");
> +
> + if (pb_vif_id) {
> + if (!vif_id || strcmp(pb_vif_id, vif_id)) {
> + return false;
> + }
> + }
> +
> + return true;
> +}
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 8a942b54c..4848dbfe0 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1002,6 +1002,14 @@
> one chassis.
> </column>
>
> + <column name="options" key="vif-id">
> + If set, this port will be bound by <code>ovn-controller</code>
> + only if this same key and value is configured in the
> + <ref table="Interface" column="external_ids" db="Open_vSwitch"/>
> + column in the Open_vSwitch database's
> + <ref table="Interface" db="Open_vSwitch"/> table.
> + </column>
> +
> <column name="options" key="qos_max_rate">
> If set, indicates the maximum rate for data sent from this interface,
> in bit/s. The traffic will be shaped according to this limit.
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 687555c47..2163162b8 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -3149,6 +3149,14 @@ tcp.flags = RST;
> one chassis.
> </column>
>
> + <column name="options" key="vif-id">
> + If set, this port will be bound by <code>ovn-controller</code>
> + only if this same key and value is configured in the
> + <ref table="Interface" column="external_ids" db="Open_vSwitch"/>
> + column in the Open_vSwitch database's
> + <ref table="Interface" db="Open_vSwitch"/> table.
> + </column>
> +
> <column name="options" key="qos_max_rate">
> If set, indicates the maximum rate for data sent from this interface,
> in bit/s. The traffic will be shaped according to this limit.
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f2882d1ad..17ac2b78a 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -27900,3 +27900,144 @@ AT_CHECK([as hv2 ovs-ofctl dump-flows br-int table=24 | grep 10.0.1.2], [0], [ig
> OVN_CLEANUP([hv1],[hv2])
> AT_CLEANUP
> ])
> +
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller port binding with vif-id])
Thanks for the comprehensive tests! I did some local testing and it
worked for me.
> +ovn_start
> +
> +net_add n1
> +
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl -- add-port br-int hv1-vm1
> +
> +ovn-nbctl ls-add sw0
> +ovn-nbctl lsp-add sw0 sw0-port1
> +ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 10.0.0.3"
> +
> +ovn-nbctl lsp-set-options sw0-port1 vif-id=foo
> +
> +ovn-nbctl lsp-add sw0 sw0-port2
> +ovn-nbctl --wait=hv lsp-set-addresses sw0-port2 "50:54:00:00:00:02 10.0.0.4"
> +
> +hv1_uuid=$(fetch_column Chassis _uuid name=hv1)
> +hv2_uuid=$(fetch_column Chassis _uuid name=hv2)
> +
> +check as hv1 ovs-vsctl add-port br-int vif11 \
> + -- set interface vif11 external_ids:iface-id=sw0-port1
> +
> +# sw0-port1 should not be claimed.
> +ovn-nbctl --wait=hv sync
> +check_column "" Port_Binding chassis logical_port=sw0-port1
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0-port1]], OVS interface name : [[vif11]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> +# Set vif-id on vif11. hv1 ovn-controller should claim it now.
> +check as hv1 ovs-vsctl set interface vif11 external_ids:vif-id=foo
> +
> +wait_for_ports_up sw0-port1
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0-port1]], OVS interface name : [[vif11]], num binding lports : [[1]]
> +primary lport : [[sw0-port1]]
> +----------------------------------------
> +])
> +
> +# Clear the vif-id from vif11 and hv1 ovn-controller should release it.
> +check as hv1 ovs-vsctl remove interface vif11 external_ids vif-id
> +ovn-nbctl --wait=hv sync
> +check_column "" Port_Binding chassis logical_port=sw0-port1
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0-port1]], OVS interface name : [[vif11]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> +# Clear the sw0-port1 vif-id options and sw0-port1 should be claimed.
> +check ovn-nbctl clear logical_switch_port sw0-port1 options
> +
> +wait_for_ports_up sw0-port1
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0-port1]], OVS interface name : [[vif11]], num binding lports : [[1]]
> +primary lport : [[sw0-port1]]
> +----------------------------------------
> +])
> +
> +# Set the options:vif-id to sw0-port1 with different value.
> +check ovn-nbctl lsp-set-options sw0-port1 vif-id=bar
> +
> +ovn-nbctl --wait=hv sync
> +check_column "" Port_Binding chassis logical_port=sw0-port1
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0-port1]], OVS interface name : [[vif11]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> +# Set vif-id on vif11. hv1 ovn-controller should claim it now.
> +check as hv1 ovs-vsctl set interface vif11 external_ids:vif-id=bar
> +wait_for_ports_up sw0-port1
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0-port1]], OVS interface name : [[vif11]], num binding lports : [[1]]
> +primary lport : [[sw0-port1]]
> +----------------------------------------
> +])
> +
> +# Set a different vif-id on vif11.
> +check as hv1 ovs-vsctl set interface vif11 external_ids:vif-id=bar2
> +
> +ovn-nbctl --wait=hv sync
> +check_column "" Port_Binding chassis logical_port=sw0-port1
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0-port1]], OVS interface name : [[vif11]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> +# Set the type of sw0-port1 to localport. vif-id is ignored for localports.
Why is this? It should be specified in the man page.
> +# So sw0-port1 should be internally claimed without setting sw0-port1 to up.
> +check ovn-nbctl lsp-set-type sw0-port1 localport
> +
> +ovn-nbctl --wait=hv sync
> +check_column "" Port_Binding chassis logical_port=sw0-port1
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0-port1]], OVS interface name : [[vif11]], num binding lports : [[1]]
> +localport lport : [[sw0-port1]]
> +----------------------------------------
> +])
> +
> +check as hv1 ovs-vsctl add-port br-int vif12 \
> + -- set interface vif12 external_ids:iface-id=sw0-port2
> +
> +wait_for_ports_up sw0-port2
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0-port1]], OVS interface name : [[vif11]], num binding lports : [[1]]
> +localport lport : [[sw0-port1]]
> +----------------------------------------
> +name: [[sw0-port2]], OVS interface name : [[vif12]], num binding lports : [[1]]
> +primary lport : [[sw0-port2]]
> +----------------------------------------
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
>
More information about the dev
mailing list