[ovs-dev] [PATCH ovn] Support additional 'vif-id' options in OVS inteface for claiming an lport.
Numan Siddique
numans at ovn.org
Fri Aug 20 23:13:52 UTC 2021
On Fri, Aug 20, 2021 at 12:41 PM Mark Gray <mark.d.gray at redhat.com> wrote:
>
> 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>
Thanks for the review.
I incorporated all your suggestions, except for one (please see below)
and applied the patch to the main branch.
>
> 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....
Ack. Done
> >
> > 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?
Ack. Done.
>
> > +
> > + /* 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.
If you see the man page, The newly added config option is defined for
the VIF ports (i.e lports of type "")
and hence this would not apply to localports. So I have ignored your
comment. Please let me know
if you think there is still a need to add more documentation. I'll do
a follow up patch.
Also ovn-controllers don't claim the local ports.
Thanks
Numan
>
> > +# 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
> > +])
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list