Re: [libvirt PATCH v3 1/1] Ignore EPERM on attempts to clear VF VLAN ID
Hi Laine,
This is less effective than mocking netlink messages, however, even
with netlink messages responses would depend on providing the right
argument to trigger the right mock.
Even this testing was useful because I managed to make a mistake while
reworking one of the if statements and those tests at least trigger
the conditionals that check error codes.
As far as ideas go:
1. I could suggest returning the requestError coming from
virNetDevSendVfSetLinkRequest - so if there is a mistake in the mock
argument, the resulting error will be different from the one expected
in a test since there won't be a netdev on a system with a test name
and the real implementation of virNetDevSendVfSetLinkRequest will be
called.
2. Utilize netdevsim
https://github.com/torvalds/linux/blob/v5.15/drivers/net/netdevsim/netdev.c#L82-L109
Downsides:
* having to load the right kernel module (testing in docker containers
makes it difficult - I don't see any infrastructure in the current
test framework to do that);
* cannot test the EPERM case since nsim_set_vf_vlan does not return it
(it actually allows setting a VLAN for a simulated VF).
On the plus side, this doesn't require mocking netlink messages and
utilizes the netlink message sending/receiving infrastructure in
Libvirt in full.
3. Introduce netlink message mocking.
There will be some boilerplate to generate kernel-like responses or at
least partial messages that contain an error or lack thereof
(NLMSG_DONE, NLMSG_ERROR).
I'll start with (1) for now but I am open to discussing other
approaches further. I am certainly interested in having this tested on
every build so that it doesn't break for everybody else down the road.
Best Regards,
Dmitrii Shcherbakov
LP/oftc: dmitriis
On Wed, Nov 17, 2021 at 3:43 AM Laine Stump wrote:
>
> On 11/15/21 1:59 PM, Dmitrii Shcherbakov wrote:
> > [...]
> > diff --git a/tests/virnetdevtest.c b/tests/virnetdevtest.c
> > index aadbeb1ef4..bdaa94e83c 100644
> > --- a/tests/virnetdevtest.c
> > +++ b/tests/virnetdevtest.c
>
>
> Maybe I'm just looking at it too superficially, but it seems like these
> test cases are testing the test jig itself more than any of the library
> code (the one exception is that testVirNetDevSetVfConfig() checks that
> the real virNetDevSetVfConfig() returns success when setting the vlan
> failed after setting the MAC succeeded). I'm not saying that anything
> better could be accomplished without mocking netlink itself, just wonder
> if it's worth all this extra bulk for the small amount of testing of
> actual library code that's accomplished.
>
>
> > @@ -18,11 +18,17 @@
> >
> > #include
> >
> > +#include "internal.h"
> > #include "testutils.h"
> >
> > +#include "virnetlink.h"
> > +
> > +#define LIBVIRT_VIRNETDEVPRIV_H_ALLOW
> > +
> > #ifdef __linux__
> >
> > -# include "virnetdev.h"
> > +# include "virmock.h"
> > +# include "virnetdevpriv.h"
> >
> > # define VIR_FROM_THIS VIR_FROM_NONE
> >
> > @@ -59,6 +65,211 @@ testVirNetDevGetLinkInfo(const void *opaque)
> > return 0;
> > }
> >
> > +int
> > +(*real_virNetDevSendVfSetLinkRequest)(const char *ifname, int vfInfoType,
> > + const void *payload, const size_t
> > payloadLen);
> > +
> > +int
> > +(*real_virNetDevSetVfMac)(const char *ifname, int vf, const virMacAddr
> > *macaddr, bool *allowRetry);
> > +
> > +int
> > +(*real_virNetDevSetVfVlan)(const char *ifname, int vf, int vlanid);
> > +
> > +static void
> > +init_syms(void)
> > +{
> > +VIR_MOCK_REAL_INIT(virNetDevSendVfSetLinkRequest);
> > +VIR_MOCK_REAL_INIT(virNetDevSetVfMac);
> > +VIR_MOCK_REAL_INIT(virNetDevSetVfVlan);
> > +}
> > +
> > +int
> > +virNetDevSetVfMac(const char *ifname, int vf,
> > + const virMacAddr *macaddr,
> > + bool *allowRetry)
> > +{
> > +init_syms();
> > +
> > +if (STREQ_NULLABLE(ifname, "fakeiface-macerror")) {
> > +return -1;
> > +} else if (STREQ_NULLABLE(ifname, "fakeiface-altmacerror")) {
> > +return -2;
> > +} else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) {
> > +return -1;
> > +} else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) {
> > +return -1;
> > +} else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror"))
> > {
> > +return 0;
> > +}
> > +return real_virNetDevSetVfMac(ifname, vf, macaddr, allowRetry);
> > +}
> > +
> > +int
> > +virNetDevSetVfVlan(const char *ifname, int vf, int vlanid)
> > +{
> > +init_syms();
> > +
> > +if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) {
> > +return -1;
> > +} else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) {
> > +return 0;
> > +} else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror"))
> > {
> > +return 0;
> > +}
> > +return real_virNetDevSetVfVlan(ifname, vf, vlanid);
> > +}
> > +
> > +int
> > +virNetDevSendVfSetLinkRequest(const
Re: [libvirt PATCH v3 1/1] Ignore EPERM on attempts to clear VF VLAN ID
On 11/15/21 1:59 PM, Dmitrii Shcherbakov wrote:
[...]
diff --git a/tests/virnetdevtest.c b/tests/virnetdevtest.c
index aadbeb1ef4..bdaa94e83c 100644
--- a/tests/virnetdevtest.c
+++ b/tests/virnetdevtest.c
Maybe I'm just looking at it too superficially, but it seems like these
test cases are testing the test jig itself more than any of the library
code (the one exception is that testVirNetDevSetVfConfig() checks that
the real virNetDevSetVfConfig() returns success when setting the vlan
failed after setting the MAC succeeded). I'm not saying that anything
better could be accomplished without mocking netlink itself, just wonder
if it's worth all this extra bulk for the small amount of testing of
actual library code that's accomplished.
@@ -18,11 +18,17 @@
#include
+#include "internal.h"
#include "testutils.h"
+#include "virnetlink.h"
+
+#define LIBVIRT_VIRNETDEVPRIV_H_ALLOW
+
#ifdef __linux__
-# include "virnetdev.h"
+# include "virmock.h"
+# include "virnetdevpriv.h"
# define VIR_FROM_THIS VIR_FROM_NONE
@@ -59,6 +65,211 @@ testVirNetDevGetLinkInfo(const void *opaque)
return 0;
}
+int
+(*real_virNetDevSendVfSetLinkRequest)(const char *ifname, int vfInfoType,
+ const void *payload, const size_t
payloadLen);
+
+int
+(*real_virNetDevSetVfMac)(const char *ifname, int vf, const virMacAddr
*macaddr, bool *allowRetry);
+
+int
+(*real_virNetDevSetVfVlan)(const char *ifname, int vf, int vlanid);
+
+static void
+init_syms(void)
+{
+VIR_MOCK_REAL_INIT(virNetDevSendVfSetLinkRequest);
+VIR_MOCK_REAL_INIT(virNetDevSetVfMac);
+VIR_MOCK_REAL_INIT(virNetDevSetVfVlan);
+}
+
+int
+virNetDevSetVfMac(const char *ifname, int vf,
+ const virMacAddr *macaddr,
+ bool *allowRetry)
+{
+init_syms();
+
+if (STREQ_NULLABLE(ifname, "fakeiface-macerror")) {
+return -1;
+} else if (STREQ_NULLABLE(ifname, "fakeiface-altmacerror")) {
+return -2;
+} else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) {
+return -1;
+} else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) {
+return -1;
+} else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror")) {
+return 0;
+}
+return real_virNetDevSetVfMac(ifname, vf, macaddr, allowRetry);
+}
+
+int
+virNetDevSetVfVlan(const char *ifname, int vf, int vlanid)
+{
+init_syms();
+
+if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) {
+return -1;
+} else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) {
+return 0;
+} else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror")) {
+return 0;
+}
+return real_virNetDevSetVfVlan(ifname, vf, vlanid);
+}
+
+int
+virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
+ const void *payload, const size_t payloadLen)
+{
+init_syms();
+
+if (STREQ_NULLABLE(ifname, "fakeiface-eperm")) {
+return -EPERM;
+} else if (STREQ_NULLABLE(ifname, "fakeiface-eagain")) {
+return -EAGAIN;
+} else if (STREQ_NULLABLE(ifname, "fakeiface-einval")) {
+return -EINVAL;
+} else if (STREQ_NULLABLE(ifname, "fakeiface-ok")) {
+return 0;
+}
+return real_virNetDevSendVfSetLinkRequest(ifname, vfInfoType, payload,
payloadLen);
+}
+
+static int
+testVirNetDevSetVfMac(const void *opaque G_GNUC_UNUSED)
+{
+struct testCase {
+const char *ifname;
+const int vf_num;
+const virMacAddr macaddr;
+bool allow_retry;
+const int rc;
+};
+size_t i = 0;
+int rc = 0;
+struct testCase testCases[] = {
+{ .ifname = "fakeiface-ok", .vf_num = 1,
+ .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false,
.rc = 0 },
+{ .ifname = "fakeiface-ok", .vf_num = 2,
+ .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = false,
.rc = 0 },
+{ .ifname = "fakeiface-ok", .vf_num = 3,
+ .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = true,
.rc = 0 },
+{ .ifname = "fakeiface-ok", .vf_num = 4,
+ .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = true,
.rc = 0 },
+{ .ifname = "fakeiface-eperm", .vf_num = 5,
+ .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false,
.rc = -1 },
+{ .ifname = "fakeiface-einval", .vf_num = 6,
+ .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false,
.rc = -1 },
+{ .ifname = "fakeiface-einval", .vf_num = 7,
+ .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = true,
.rc = -2 },
+{ .ifname = "fakeiface-einval", .vf_num = 8,
+ .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = false,
.rc = -1 },
+{ .ifname = "fakeiface-einval", .vf_num = 9,
+ .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry =
Re: [libvirt PATCH v3 1/1] Ignore EPERM on attempts to clear VF VLAN ID
Hi Daniel,
Thanks a lot for the review, I'll send a v4 with the requested changes included.
On Tue, Nov 16, 2021 at 9:55 PM Daniel P. Berrangé wrote:
> > +int
> > +virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
> > + const void *payload, const size_t payloadLen)
>
> I think it would be desirable for this method to be introduced
> in a patch on its own, separate from the patch that then splits
> virNetDevSetVfConfig into 2 parts, and separate from one that
> adds EPERM handling.
>
> Our general guideline is that refactoring should never be mixed
> with functional behaviour changes, as this has often resulted
> in surprise regressions that were diguised by the mixing.
Ack, agreed. Apologies for not doing it right away - there is
definitely merit in doing it as you describe.
> > +
> > +requestError = virNetDevSendVfSetLinkRequest(ifname, IFLA_VF_VLAN,
> > + _vf_vlan,
> > sizeof(ifla_vf_vlan));
> > +
> > +/* If vlanid is 0 - we are attempting to clear an existing VLAN id.
> > + * An EPERM received at this stage is an indicator that the embedded
> > + * switch is not exposed to this host and the network driver is not
> > + * able to set a VLAN for a VF. */
> > +if (requestError == -EPERM && vlanid == 0) {
>
> This metod is taking a plain "int vlanid", but the eventual
> caller of this method is taking "virNetDevVlan *vlan".
>
> IOW, the caller can distinguish between two scenarios
>
> - vlan is NULL => set vlanid to 0
> - vlan is non-NULL and user specified vlanid of 0
>
> I think it is reasonable to ignore EPERM in the first
> scenario for the reasons you describe wrt hardware
> driver restrictions.
>
> I'm less sure it is a good idea to ignore EPERM when
> it wasn an explicit user configuration request to set
> vlanid to 0. It feels like we should be continuing to
> report an error if we can't honour an explicit user
> request - its a sign the user shouldn't have been
> settng the vlan in the first place.
Agreed, I can convert virNetDevSetVfConfig and virNetDevSetVfVlan to
accept a pointer to a vlan tag and ignore EPERM in virNetDevSetVfVlan
only when the VLAN pointer is NULL.
For the use-case this patch is introduced, the higher-level software
(Nova) does not specify a VLAN when formatting a device XML provided
to Libvirt.
https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/vif.py#L479-L485
designer.set_vif_host_backend_hw_veb(
conf, 'hostdev', vif.dev_address, None)
https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/designer.py#L97-L105
And the resulting device XML looks like this:
As you say, with that we can preserve the VLAN clearing behavior when
the VLAN pointer is NULL if it succeeds and ignore EPERM if it
doesn't. And if clearing is explicit fail on EPERM and other errors.
Best Regards,
Dmitrii Shcherbakov
LP/oftc: dmitriis
Re: [libvirt PATCH v3 1/1] Ignore EPERM on attempts to clear VF VLAN ID
On Mon, Nov 15, 2021 at 09:59:23PM +0300, Dmitrii Shcherbakov wrote:
> SmartNIC DPUs may not expose some privileged eswitch operations
> to the hypervisor hosts. For example, this happens with Bluefield
> devices running in the ECPF (default) mode for security reasons. While
> VF MAC address programming is possible via an RTM_SETLINK operation,
> trying to set a VLAN ID in the same operation will fail with EPERM.
>
> The equivalent ip link commands below provide an illustration:
>
> 1. This works:
>
> sudo ip link set enp130s0f0 vf 2 mac de:ad:be:ef:ca:fe
>
> 2. Setting (or clearing) a VLAN fails with EPERM:
>
> sudo ip link set enp130s0f0 vf 2 vlan 0
> RTNETLINK answers: Operation not permitted
>
> 3. This is what Libvirt attempts to do today (when trying to clear a
>VF VLAN at the same time as programming a VF MAC).
>
> sudo ip link set enp130s0f0 vf 2 vlan 0 mac de:ad:be:ef:ca:fe
> RTNETLINK answers: Operation not permitted
>
> If setting an explicit VLAN ID results in an EPERM, clearing a VLAN
> (setting a VLAN ID to 0) can be handled gracefully by ignoring the
> EPERM error with the rationale being that if we cannot set this state
> in the first place, we cannot clear it either.
>
> Thus, virNetDevSetVfConfig is split into two distinct functions. If
> clearing a VLAN ID fails with EPERM, the error is simply ignored.
>
> Both new functions rely virNetDevSendVfSetLinkRequest that implements
> common functionality related to formatting a request, sending it and
> handling error conditions and returns 0 or an error since in both cases
> the payload is either NLMSG_DONE (no error) or NLMSG_ERROR where an
> error message is needed by the caller to handle known cases
> appropriately. This function allows the conditional code to be unit tested.
>
> An alternative to this could be providing a higher level control plane
> mechanism that would provide metadata about a device being remotely
> managed in which case Libvirt would avoid trying to set or clear a
> VLAN ID. This would be more complicated since other software (like Nova
> in the OpenStack case) would have to annotate every guest device with an
> attribute indicating whether a device is remotely managed or not based
> on operator provided configuration so that Libvirt can act on this and
> avoid VLAN programming.
>
> Signed-off-by: Dmitrii Shcherbakov
> ---
> NEWS.rst | 9 ++
> src/libvirt_private.syms | 7 ++
> src/util/virnetdev.c | 196 +-
> src/util/virnetdevpriv.h | 44
> tests/virnetdevtest.c| 222 ++-
> 5 files changed, 405 insertions(+), 73 deletions(-)
> create mode 100644 src/util/virnetdevpriv.h
>
> diff --git a/NEWS.rst b/NEWS.rst
> index a71b84c363..d1702d95d5 100644
> --- a/NEWS.rst
> +++ b/NEWS.rst
> @@ -30,6 +30,15 @@ v7.10.0 (unreleased)
> Libvirt is now able to report interface information from the guest's
> perspective (using guest agent).
>
> + * virnetdev: Ignore EPERM on attempts to clear VF VLAN ID
> +
> +Libvirt will now ignore EPERM errors on attempts to clear a VLAN ID as
> +SmartNIC DPUs do not expose VLAN programming capabilities to the
> +hypervisor host. Libvirt tries to clear a VLAN in the same operation
> +as setting a MAC address for VIR_DOMAIN_NET_TYPE_HOSTDEV devices which
> +is now split into two distinct operations. EPERM errors received while
> +trying to program a non-zero VLAN ID will still cause errors.
> +
> * **Bug fixes**
>
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9ee8fda25f..4b8e3e2b15 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2851,6 +2851,13 @@ virNetDevOpenvswitchSetTimeout;
> virNetDevOpenvswitchUpdateVlan;
>
>
> +#util/virnetdevpriv.h
> +virNetDevSendVfSetLinkRequest;
> +virNetDevSetVfConfig;
> +virNetDevSetVfMac;
> +virNetDevSetVfVlan;
> +
> +
> # util/virnetdevtap.h
> virNetDevTapAttachBridge;
> virNetDevTapCreate;
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 58f7360a0f..5daec532b4 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -19,7 +19,9 @@
> #include
> #include
>
> -#include "virnetdev.h"
> +#define LIBVIRT_VIRNETDEVPRIV_H_ALLOW
> +
> +#include "virnetdevpriv.h"
> #include "viralloc.h"
> #include "virnetlink.h"
> #include "virmacaddr.h"
> @@ -1527,16 +1529,12 @@ static struct nla_policy
> ifla_vfstats_policy[IFLA_VF_STATS_MAX+1] = {
> [IFLA_VF_STATS_MULTICAST] = { .type = NLA_U64 },
> };
>
> -
> -static int
> -virNetDevSetVfConfig(const char *ifname, int vf,
> - const virMacAddr *macaddr, int vlanid,
> - bool *allowRetry)
> +int
> +virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
> + const void *payload, const size_t payloadLen)
I think it would be desirable for this method to be introduced
in a patch on
