[libvirt] [PATCH 2/2] qemu, lxc: Fail explicitly if setting QoS on unsupported vNIC types
Laine Stump
laine at laine.org
Tue Jan 13 16:26:57 UTC 2015
On 01/13/2015 10:58 AM, John Ferlan wrote:
>
> On 01/07/2015 12:31 PM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1165993
>>
>> So, there are still plenty of vNIC types that we don't know how to set
>> bandwidth on. Let's fail explicitly in case user has requested it
>> instead of pretending everything
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> src/lxc/lxc_driver.c | 20 +++++++++++++++-----
>> src/lxc/lxc_process.c | 20 +++++++++++++++-----
>> src/qemu/qemu_command.c | 21 +++++++++++++++------
>> src/qemu/qemu_hotplug.c | 20 +++++++++++++++-----
>> 4 files changed, 60 insertions(+), 21 deletions(-)
>>
> Guess your .0 expectations regarding heavy discussion haven't come to
> fruition...
Partly because it got lost in the post vacation spew :-)
I do understand Michal's concern. In particular, the network hook script
support was put in specifically to allow management apps to support
bandwidth management on interface types that libvirt couldn't/didn't
directly support. If I recall correctly, the idea is that, although
libvirt does nothing with it, the bandwidth info will be in the XML
that's passed to the hook script when the interface is brought up, and
the hook script can do whatever it likes to setup the bandwidth.
Or am I remembering incorrectly.
If that functionality is important, then this patch can't go in. If not,
then it's a reasonable thing to do.
> With these changes we'll no longer be lying about setting
> QoS at least at the peril of course of upsetting those that have set
> them inappropriately on an unsupported vNIC.
>
> These changes though would mean a domain that was at one time
> successfully running will no longer start without changing (removing)
> the QoS settings on the unsupported vNIC - I could see that upsetting
> the customer base more than say perhaps somehow messaging (VIR_INFO,
> VIR_WARNING?) that the vNIC doesn't support QoS settings and are being
> ignored. Is there a way to message that out to something more than the
> log file(s) while still continuing?
This may be a reasonable middle ground.
On the other hand, there are *many* other instances of unsupported
config where we log an error when we could have instead logged a warning
and assumed that the user may have a hook script that adds in the
support; it would be nice to either be consistent about this one way or
the other, or at least have a good story about why we do it differently
in this case.
>
> In reality the settings were being ignored anyway previously so a
> message is at least an upgrade, although perhaps not quite an optimal
> solution for something which may not "see" the message if it isn't
> displayed back to the stdout/err.
>
> Another option... message via VIR_WARNING/INFO *and* then just clear
> them and save the domain status/xml so that they are not seen again...
> Then of course soldier on through startup. Is that a viable option based
> on history of dealing with such things?
I don't like that option - rather than immediately telling you that
you're doing something wrong and forcing you to fix it, it would
semi-silently remove your hard work.
>
> I'm OK with the changes as is; however, I think getting another opinion
> or two would be useful...
>
> John
>
>> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
>> index 51c664f..10a4a7e 100644
>> --- a/src/lxc/lxc_driver.c
>> +++ b/src/lxc/lxc_driver.c
>> @@ -4139,6 +4139,7 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn,
>> virLXCDomainObjPrivatePtr priv = vm->privateData;
>> int ret = -1;
>> int actualType;
>> + virNetDevBandwidthPtr actualBandwidth;
>> char *veth = NULL;
>>
>> if (!priv->initpid) {
>> @@ -4186,11 +4187,20 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn,
>> _("Network device type is not supported"));
>> goto cleanup;
>> }
>> - /* set network bandwidth */
>> - if (virNetDevSupportBandwidth(actualType) &&
>> - virNetDevBandwidthSet(net->ifname,
>> - virDomainNetGetActualBandwidth(net), false) < 0)
>> - goto cleanup;
>> + /* Set bandwidth or fail if requested and not supported. */
>> + actualBandwidth = virDomainNetGetActualBandwidth(net);
>> + if (actualBandwidth) {
>> + if (!virNetDevSupportBandwidth(actualType)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("setting bandwidth on interfaces of "
>> + "type '%s' is not implemented yet"),
>> + virDomainNetTypeToString(actualType));
>> + goto cleanup;
>> + }
>> +
>> + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0)
>> + goto cleanup;
>> + }
>>
>> if (virNetDevSetNamespace(veth, priv->initpid) < 0) {
>> virDomainAuditNet(vm, NULL, net, "attach", false);
>> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>> index d7eb8bc..9fa900e 100644
>> --- a/src/lxc/lxc_process.c
>> +++ b/src/lxc/lxc_process.c
>> @@ -371,6 +371,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
>>
>> for (i = 0; i < def->nnets; i++) {
>> char *veth = NULL;
>> + virNetDevBandwidthPtr actualBandwidth;
>> /* If appropriate, grab a physical device from the configured
>> * network's pool of devices, or resolve bridge device name
>> * to the one defined in the network definition.
>> @@ -424,11 +425,20 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
>>
>> }
>>
>> - /* set network bandwidth */
>> - if (virNetDevSupportBandwidth(type) &&
>> - virNetDevBandwidthSet(net->ifname,
>> - virDomainNetGetActualBandwidth(net), false) < 0)
>> - goto cleanup;
>> + /* Set bandwidth or fail if requested and not supported. */
>> + actualBandwidth = virDomainNetGetActualBandwidth(net);
>> + if (actualBandwidth) {
>> + if (!virNetDevSupportBandwidth(type)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("setting bandwidth on interfaces of "
>> + "type '%s' is not implemented yet"),
>> + virDomainNetTypeToString(type));
>> + goto cleanup;
>> + }
>> +
>> + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0)
>> + goto cleanup;
>> + }
>>
>> (*veths)[(*nveths)-1] = veth;
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index d5679de..1729866 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7358,6 +7358,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>> char **tapfdName = NULL;
>> char **vhostfdName = NULL;
>> int actualType = virDomainNetGetActualType(net);
>> + virNetDevBandwidthPtr actualBandwidth;
>> size_t i;
>>
>> if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)
>> @@ -7408,12 +7409,20 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>> goto cleanup;
>> }
>>
>> - /* Set Bandwidth */
>> - if (virNetDevSupportBandwidth(actualType) &&
>> - virNetDevBandwidthSet(net->ifname,
>> - virDomainNetGetActualBandwidth(net),
>> - false) < 0)
>> - goto cleanup;
>> + /* Set bandwidth or fail if requested and not supported. */
>> + actualBandwidth = virDomainNetGetActualBandwidth(net);
>> + if (actualBandwidth) {
>> + if (!virNetDevSupportBandwidth(actualType)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("setting bandwidth on interfaces of "
>> + "type '%s' is not implemented yet"),
>> + virDomainNetTypeToString(actualType));
>> + goto cleanup;
>> + }
>> +
>> + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0)
>> + goto cleanup;
>> + }
>>
>> if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>> actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 1714341..5ea374b 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -838,6 +838,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>> bool releaseaddr = false;
>> bool iface_connected = false;
>> int actualType;
>> + virNetDevBandwidthPtr actualBandwidth;
>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> size_t i;
>>
>> @@ -926,11 +927,20 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>> if (qemuInterfaceStartDevice(net) < 0)
>> goto cleanup;
>>
>> - /* Set Bandwidth */
>> - if (virNetDevSupportBandwidth(actualType) &&
>> - virNetDevBandwidthSet(net->ifname,
>> - virDomainNetGetActualBandwidth(net), false) < 0)
>> - goto cleanup;
>> + /* Set bandwidth or fail if requested and not supported. */
>> + actualBandwidth = virDomainNetGetActualBandwidth(net);
>> + if (actualBandwidth) {
>> + if (!virNetDevSupportBandwidth(actualType)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("setting bandwidth on interfaces of "
>> + "type '%s' is not implemented yet"),
>> + virDomainNetTypeToString(actualType));
>> + goto cleanup;
>> + }
>> +
>> + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0)
>> + goto cleanup;
>> + }
>>
>> for (i = 0; i < tapfdSize; i++) {
>> if (virSecurityManagerSetTapFDLabel(driver->securityManager,
>>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list