8:15 a.m.
When domain is started with setting that cannot be done, i.e. those
that require cgroups, there is no error reported and it succeeds
without any message whatsoever.
When setting with API, virsh, an error is reported, but only due to
the fact that no cgroups are mounted (priv->cgroup == NULL).
Given the above it seems reasonable to reject such unsupported
settings.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1023366
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
Notes:
v2:
- Allow CPU pinning since setting affinity should still work.
- Remove bogus 'cfg = ...' in SetBlockIoTune.
src/qemu/qemu_command.c | 26 ++++++++++++++++++
src/qemu/qemu_driver.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 98 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 18d0a64..611d21d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7762,6 +7762,32 @@ qemuBuildCommandLine(virConnectPtr conn,
emulator = def->emulator;
+ if (!cfg->privileged) {
+ /* If we have no cgroups than we can have no tunings that
+ * require them */
+
+ if (def->mem.hard_limit || def->mem.soft_limit ||
+ def->mem.min_guarantee || def->mem.swap_hard_limit) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Memory tuning is not available in session
mode"));
+ goto error;
+ }
+
+ if (def->blkio.weight || def->blkio.ndevices) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto error;
+ }
+
+ if (def->cputune.shares || def->cputune.period ||
+ def->cputune.quota || def->cputune.emulator_period ||
+ def->cputune.emulator_quota) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("CPU tuning is not available in session mode"));
+ goto error;
+ }
+ }
+
for (i = 0; i < def->ngraphics; ++i) {
switch (def->graphics[i]->type) {
case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4fbcb27..1e50572 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7434,6 +7434,8 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
char *ret = NULL;
virDomainObjPtr vm = NULL;
qemuDomainObjPrivatePtr priv;
+ virQEMUDriverPtr driver = dom->conn->privateData;
+ virQEMUDriverConfigPtr cfg = NULL;
if (!(vm = qemuDomObjFromDomain(dom)))
goto cleanup;
@@ -7443,6 +7445,13 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
if (virDomainGetSchedulerTypeEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("CPU tuning is not available in session mode"));
+ goto cleanup;
+ }
+
/* Domain not running, thus no cgroups - return defaults */
if (!virDomainObjIsActive(vm)) {
if (nparams)
@@ -7469,6 +7478,7 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
cleanup:
if (vm)
virObjectUnlock(vm);
+ virObjectUnref(cfg);
return ret;
}
@@ -7683,6 +7693,12 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
if (virDomainSetBlkioParametersEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -7863,6 +7879,7 @@ qemuDomainGetBlkioParameters(virDomainPtr dom,
int ret = -1;
virCapsPtr caps = NULL;
qemuDomainObjPrivatePtr priv;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -7881,6 +7898,13 @@ qemuDomainGetBlkioParameters(virDomainPtr dom,
if (virDomainGetBlkioParametersEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8269,6 +8293,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
@@ -8316,6 +8341,12 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
if (virDomainSetMemoryParametersEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Memory tuning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8423,6 +8454,7 @@ qemuDomainGetMemoryParameters(virDomainPtr dom,
int ret = -1;
virCapsPtr caps = NULL;
qemuDomainObjPrivatePtr priv;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -8439,6 +8471,13 @@ qemuDomainGetMemoryParameters(virDomainPtr dom,
if (virDomainGetMemoryParametersEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Memory tuning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8550,6 +8589,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
@@ -8975,6 +9015,13 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
if (virDomainSetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def, flags)
< 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("CPU tuning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -9210,6 +9257,7 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom,
virDomainDefPtr persistentDef;
virCapsPtr caps = NULL;
qemuDomainObjPrivatePtr priv;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -9226,6 +9274,13 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom,
if (virDomainGetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("CPU tuning is not available in session mode"));
+ goto cleanup;
+ }
+
if (*nparams > 1)
cpu_bw_status = virCgroupSupportsCpuBW(priv->cgroup);
@@ -9320,6 +9375,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
@@ -15521,11 +15577,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
if (virDomainSetBlockIoTuneEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto cleanup;
+ }
+
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
priv = vm->privateData;
- cfg = virQEMUDriverGetConfig(driver);
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto endjob;
@@ -15668,6 +15730,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
int ret = -1;
size_t i;
virCapsPtr caps = NULL;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -15682,6 +15745,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
if (virDomainGetBlockIoTuneEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -15784,6 +15854,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
--
1.9.0
3:04 p.m.
On 03/04/2014 07:15 AM, Martin Kletzander wrote:
When domain is started with setting that cannot be done, i.e. those
that require cgroups, there is no error reported and it succeeds
without any message whatsoever.
When setting with API, virsh, an error is reported, but only due to
the fact that no cgroups are mounted (priv->cgroup == NULL).
Given the above it seems reasonable to reject such unsupported
settings.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1023366
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
@@ -7443,6 +7445,13 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
if (virDomainGetSchedulerTypeEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("CPU tuning is not available in session mode"));
+ goto cleanup;
+ }
+
/* Domain not running, thus no cgroups - return defaults */
if (!virDomainObjIsActive(vm)) {
I can understand failing the Set commands if we can't change things; but
since we have a fallback for the Get command even for an offline domain,
shouldn't we stick to returning the defaults rather than erroring out?
Weak ACK; I'd still wait for Dan to weigh in.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:37 a.m.
On Tue, Mar 04, 2014 at 02:04:05PM -0700, Eric Blake wrote:
On 03/04/2014 07:15 AM, Martin Kletzander wrote:
> When domain is started with setting that cannot be done, i.e. those
> that require cgroups, there is no error reported and it succeeds
> without any message whatsoever.
>
> When setting with API, virsh, an error is reported, but only due to
> the fact that no cgroups are mounted (priv->cgroup == NULL).
>
> Given the above it seems reasonable to reject such unsupported
> settings.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1023366
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
>
> @@ -7443,6 +7445,13 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
> if (virDomainGetSchedulerTypeEnsureACL(dom->conn, vm->def) < 0)
> goto cleanup;
>
> + cfg = virQEMUDriverGetConfig(driver);
> + if (!cfg->privileged) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("CPU tuning is not available in session
mode"));
> + goto cleanup;
> + }
> +
> /* Domain not running, thus no cgroups - return defaults */
> if (!virDomainObjIsActive(vm)) {
I can understand failing the Set commands if we can't change things; but
since we have a fallback for the Get command even for an offline domain,
shouldn't we stick to returning the defaults rather than erroring out?
Particularly for SchedulerType, we can allow the Get method, but the
reading will still fail as there are no cgroups set up. If we rework
our cgroups code for non-privileged daemons, we will have to check the
cgroup in which the domain belongs to, which will be the same as other
user processes, and doe to that fact the settings won't apply only to
that domain, but for everything in that cgroup; that will make the
values less useful (e.g. maximum used memory cannot be guaranteed
since the cgroup has more tasks inside than only qemu). Our cgroup
path assembly functions will then need to differ based on whether it
is non-privileged or not.
If that's really needed, I'll rework it that way (even though it'll
probably need to get changed back when user cgroups will be
user-editable), but until then, could we make the error reported now:
$ virsh -c qemu:///session schedinfo dummy
Scheduler : Unknown
error: Requested operation is not valid: cgroup CPU controller is not mounted
changed into:
$ virsh -c qemu:///session schedinfo dummy
Scheduler : Unknown
error: Operation not supported: CPU tuning is not available in session mode
???
// I should've added that info into the commit message, I just forgot.
Martin
Weak ACK; I'd still wait for Dan to weigh in.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:12 a.m.
On 03/05/2014 03:37 AM, Martin Kletzander wrote:
On Tue, Mar 04, 2014 at 02:04:05PM -0700, Eric Blake wrote:
> On 03/04/2014 07:15 AM, Martin Kletzander wrote:
>> When domain is started with setting that cannot be done, i.e. those
>> that require cgroups, there is no error reported and it succeeds
>> without any message whatsoever.
>>
>> When setting with API, virsh, an error is reported, but only due to
>> the fact that no cgroups are mounted (priv->cgroup == NULL).
>>
>> Given the above it seems reasonable to reject such unsupported
>> settings.
>>
>
> I can understand failing the Set commands if we can't change things; but
> since we have a fallback for the Get command even for an offline domain,
> shouldn't we stick to returning the defaults rather than erroring out?
>
If that's really needed, I'll rework it that way (even though it'll
probably need to get changed back when user cgroups will be
user-editable), but until then, could we make the error reported now:
$ virsh -c qemu:///session schedinfo dummy
Scheduler : Unknown
error: Requested operation is not valid: cgroup CPU controller is not mounted
changed into:
$ virsh -c qemu:///session schedinfo dummy
Scheduler : Unknown
error: Operation not supported: CPU tuning is not available in session mode
// I should've added that info into the commit message, I just
forgot.
Indeed, trading one error for another nicer one is always safe - and
mentioning it in the commit message highlights that it is an improvement
(we aren't causing an error on any situation where we used to pass).
ACK to this patch, then.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:40 a.m.
On Wed, Mar 05, 2014 at 06:12:41AM -0700, Eric Blake wrote:
On 03/05/2014 03:37 AM, Martin Kletzander wrote:
> On Tue, Mar 04, 2014 at 02:04:05PM -0700, Eric Blake wrote:
>> On 03/04/2014 07:15 AM, Martin Kletzander wrote:
>>> When domain is started with setting that cannot be done, i.e. those
>>> that require cgroups, there is no error reported and it succeeds
>>> without any message whatsoever.
>>>
>>> When setting with API, virsh, an error is reported, but only due to
>>> the fact that no cgroups are mounted (priv->cgroup == NULL).
>>>
>>> Given the above it seems reasonable to reject such unsupported
>>> settings.
>>>
>>
>> I can understand failing the Set commands if we can't change things; but
>> since we have a fallback for the Get command even for an offline domain,
>> shouldn't we stick to returning the defaults rather than erroring out?
>>
>
>
> If that's really needed, I'll rework it that way (even though it'll
> probably need to get changed back when user cgroups will be
> user-editable), but until then, could we make the error reported now:
>
> $ virsh -c qemu:///session schedinfo dummy
> Scheduler : Unknown
> error: Requested operation is not valid: cgroup CPU controller is not mounted
>
> changed into:
>
> $ virsh -c qemu:///session schedinfo dummy
> Scheduler : Unknown
> error: Operation not supported: CPU tuning is not available in session mode
> // I should've added that info into the commit message, I just forgot.
Indeed, trading one error for another nicer one is always safe - and
mentioning it in the commit message highlights that it is an improvement
(we aren't causing an error on any situation where we used to pass).
ACK to this patch, then.
I added the info into the commit message and pushed, thanks for the
review.
Martin
3914
days inactive
3916
days old
4 comments
2 participants
participants (2)
-
Eric Blake -
Martin Kletzander