[libvirt] [PATCH 2/2] lxc: Fail connection attempt if cgroups not mounted

Currently a user can connect to lxc:/// if cgroups aren't mounted, but they can't do a whole lot: starting and even stopping guests doesn't work (the latter only if cgroups were unmounted behind libvirts back). To make matters worse, even after mounting cgroups, libvirt must be restarted to actually notice. This is frustrating for users who may make it all the way to the end of the virt-manager 'New VM' wizard only to receive an error that requires a libvirtd restart. Fix this by checking for cgroup mounts at lxc:/// connect time, and if none are found, fail the connection. I'm not sure if there are any negative consequences to putting this logic in lxcOpen... --- src/lxc/lxc_driver.c | 33 ++++++++++++++++++++++++++------- 1 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index d0f7158..770bdf0 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -107,6 +107,22 @@ static void lxcDomainEventFlush(int timer, void *opaque); static void lxcDomainEventQueue(lxc_driver_t *driver, virDomainEventPtr event); +static int lxcGetCgroup(lxc_driver_t *driver) +{ + int privileged = 1; + + if (driver->cgroup) + return 0; + + int rc = virCgroupForDriver("lxc", &driver->cgroup, privileged, 1); + if (rc < 0) { + char buf[1024]; + VIR_DEBUG("Unable to create cgroup for LXC driver: %s", + virStrerror(-rc, buf, sizeof(buf))); + } + + return rc; +} static virDrvOpenStatus lxcOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, @@ -146,6 +162,15 @@ static virDrvOpenStatus lxcOpen(virConnectPtr conn, "%s", _("lxc state driver is not active")); return VIR_DRV_OPEN_ERROR; } + + /* Driver loaded, but no cgroup stuff mounted. Refresh the data + * but error if nothing available */ + if (lxcGetCgroup(lxc_driver) < 0) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("The 'cpuacct', 'devices' & 'memory' cgroups " + "controllers must be mounted")); + return VIR_DRV_OPEN_ERROR; + } } conn->privateData = lxc_driver; @@ -2063,11 +2088,9 @@ cleanup: virDomainObjUnlock(vm); } - static int lxcStartup(int privileged) { char *ld; - int rc; /* Valgrind gets very annoyed when we clone containers, so * disable LXC when under valgrind @@ -2113,11 +2136,7 @@ static int lxcStartup(int privileged) lxc_driver->log_libvirtd = 0; /* by default log to container logfile */ lxc_driver->have_netns = lxcCheckNetNsSupport(); - rc = virCgroupForDriver("lxc", &lxc_driver->cgroup, privileged, 1); - if (rc < 0) { - char buf[1024]; - VIR_DEBUG("Unable to create cgroup for LXC driver: %s", - virStrerror(-rc, buf, sizeof(buf))); + if (lxcGetCgroup(lxc_driver) < 0) { /* Don't abort startup. We will explicitly report to * the user when they try to start a VM */ -- 1.7.4.4

On 06/21/2011 10:47 AM, Cole Robinson wrote:
Currently a user can connect to lxc:/// if cgroups aren't mounted, but they can't do a whole lot: starting and even stopping guests doesn't work (the latter only if cgroups were unmounted behind libvirts back). To make
s/libvirts/libvirt's/
matters worse, even after mounting cgroups, libvirt must be restarted to actually notice.
This is frustrating for users who may make it all the way to the end of the virt-manager 'New VM' wizard only to receive an error that requires a libvirtd restart.
Fix this by checking for cgroup mounts at lxc:/// connect time, and if none are found, fail the connection.
I'm not sure if there are any negative consequences to putting this logic in lxcOpen...
I'm not thinking of any; at any rate, this seems like a reasonable change. But there are some issues that probably require a v2:
+++ b/src/lxc/lxc_driver.c @@ -107,6 +107,22 @@ static void lxcDomainEventFlush(int timer, void *opaque); static void lxcDomainEventQueue(lxc_driver_t *driver, virDomainEventPtr event);
+static int lxcGetCgroup(lxc_driver_t *driver) +{ + int privileged = 1;
Why create this variable, if it never changes from the constant of 1? Is it even possible to have a non-privileged lxc driver instance, and if so, shouldn't we be getting this value from driver?
+ + if (driver->cgroup) + return 0; + + int rc = virCgroupForDriver("lxc", &driver->cgroup, privileged, 1); + if (rc < 0) { + char buf[1024]; + VIR_DEBUG("Unable to create cgroup for LXC driver: %s", + virStrerror(-rc, buf, sizeof(buf)));
Unrelated to your patch, but I can't help but wonder if we should change virStrerror into taking one argument and always returning a thread-local string, rather than making callers pass a stack-allocated buffer.
@@ -2113,11 +2136,7 @@ static int lxcStartup(int privileged) lxc_driver->log_libvirtd = 0; /* by default log to container logfile */ lxc_driver->have_netns = lxcCheckNetNsSupport();
- rc = virCgroupForDriver("lxc", &lxc_driver->cgroup, privileged, 1); - if (rc < 0) { - char buf[1024]; - VIR_DEBUG("Unable to create cgroup for LXC driver: %s", - virStrerror(-rc, buf, sizeof(buf))); + if (lxcGetCgroup(lxc_driver) < 0) {
Hmm, this makes it look like lxcGetCgroup should take a second parameter, privileged. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 06/21/2011 04:49 PM, Eric Blake wrote:
On 06/21/2011 10:47 AM, Cole Robinson wrote:
Currently a user can connect to lxc:/// if cgroups aren't mounted, but they can't do a whole lot: starting and even stopping guests doesn't work (the latter only if cgroups were unmounted behind libvirts back). To make
s/libvirts/libvirt's/
matters worse, even after mounting cgroups, libvirt must be restarted to actually notice.
This is frustrating for users who may make it all the way to the end of the virt-manager 'New VM' wizard only to receive an error that requires a libvirtd restart.
Fix this by checking for cgroup mounts at lxc:/// connect time, and if none are found, fail the connection.
I'm not sure if there are any negative consequences to putting this logic in lxcOpen...
I'm not thinking of any; at any rate, this seems like a reasonable change. But there are some issues that probably require a v2:
+++ b/src/lxc/lxc_driver.c @@ -107,6 +107,22 @@ static void lxcDomainEventFlush(int timer, void *opaque); static void lxcDomainEventQueue(lxc_driver_t *driver, virDomainEventPtr event);
+static int lxcGetCgroup(lxc_driver_t *driver) +{ + int privileged = 1;
Why create this variable, if it never changes from the constant of 1? Is it even possible to have a non-privileged lxc driver instance, and if so, shouldn't we be getting this value from driver?
Documentation purposes. Like you say, LXC can't even be run non-privileged. However for the next version of the patch I just chose to track the 'privileged' value in lxc_driver_t, similar to how is done with qemu, even though it's not useful yet for LXC. That way we can pass it around like usual, and if we ever support lxc:///session less sites will need to be changed. v2 coming. Thanks, Cole
+ + if (driver->cgroup) + return 0; + + int rc = virCgroupForDriver("lxc", &driver->cgroup, privileged, 1); + if (rc < 0) { + char buf[1024]; + VIR_DEBUG("Unable to create cgroup for LXC driver: %s", + virStrerror(-rc, buf, sizeof(buf)));
Unrelated to your patch, but I can't help but wonder if we should change virStrerror into taking one argument and always returning a thread-local string, rather than making callers pass a stack-allocated buffer.
@@ -2113,11 +2136,7 @@ static int lxcStartup(int privileged) lxc_driver->log_libvirtd = 0; /* by default log to container logfile */ lxc_driver->have_netns = lxcCheckNetNsSupport();
- rc = virCgroupForDriver("lxc", &lxc_driver->cgroup, privileged, 1); - if (rc < 0) { - char buf[1024]; - VIR_DEBUG("Unable to create cgroup for LXC driver: %s", - virStrerror(-rc, buf, sizeof(buf))); + if (lxcGetCgroup(lxc_driver) < 0) {
Hmm, this makes it look like lxcGetCgroup should take a second parameter, privileged.
participants (2)
-
Cole Robinson
-
Eric Blake