[ovs-dev] [PATCH ovn] Support additional 'vif-id' options in OVS inteface for claiming an lport.
Mark Gray
mark.d.gray at redhat.com
Sat Aug 21 07:37:09 UTC 2021
On 21/08/2021 00:13, Numan Siddique wrote:
> 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.
Oh yeah, makes sense. No problem with not making this change. Thanks
>
> 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