[libvirt] [PATCH 1/3] Add APIs for talking to init via /dev/initctl
From: "Daniel P. Berrange" <berrange@redhat.com> To be able todo controlled shutdown/reboot of containers an API to talk to init via /dev/initctl is required. Fortunately this is quite straightforward to implement, and is supported by both sysvinit and systemd. Upstart support for /dev/initctl is unclear. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/libvirt/virterror.h | 1 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 4 ++ src/util/virinitctl.c | 161 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virinitctl.h | 41 +++++++++++ src/util/virterror.c | 1 + 7 files changed, 210 insertions(+) create mode 100644 src/util/virinitctl.c create mode 100644 src/util/virinitctl.h diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index a877683..4d79620 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -114,6 +114,7 @@ typedef enum { VIR_FROM_SSH = 50, /* Error from libssh2 connection transport */ VIR_FROM_LOCKSPACE = 51, /* Error from lockspace */ + VIR_FROM_INITCTL = 52, /* Error from initctl device communication */ # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST diff --git a/po/POTFILES.in b/po/POTFILES.in index ec59efb..77bc04b 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -153,6 +153,7 @@ src/util/virauthconfig.c src/util/virdbus.c src/util/virfile.c src/util/virhash.c +src/util/virinitctl.c src/util/virkeyfile.c src/util/virlockspace.c src/util/virnetdev.c diff --git a/src/Makefile.am b/src/Makefile.am index 627dbb5..6401dec 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -94,6 +94,7 @@ UTIL_SOURCES = \ util/virdbus.c util/virdbus.h \ util/virhash.c util/virhash.h \ util/virhashcode.c util/virhashcode.h \ + util/virinitctl.c util/virinitctl.h \ util/virkeycode.c util/virkeycode.h \ util/virkeyfile.c util/virkeyfile.h \ util/virkeymaps.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2573b8a..f308b55 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1356,6 +1356,10 @@ virFileTouch; virFileUpdatePerm; +# virinitctl.h +virInitctlSetRunLevel; + + # virkeycode.h virKeycodeSetTypeFromString; virKeycodeSetTypeToString; diff --git a/src/util/virinitctl.c b/src/util/virinitctl.c new file mode 100644 index 0000000..c70ea3a --- /dev/null +++ b/src/util/virinitctl.c @@ -0,0 +1,161 @@ +/* + * virinitctl.c: API for talking to init systems via initctl + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include <sys/param.h> +#include <fcntl.h> + +#include "internal.h" +#include "virinitctl.h" +#include "virterror_internal.h" +#include "util.h" +#include "memory.h" +#include "virfile.h" + +#define VIR_FROM_THIS VIR_FROM_INITCTL + +/* These constants & struct definitions are taken from + * systemd, under terms of LGPLv2+ + * + * initreq.h Interface to talk to init through /dev/initctl. + * + * Copyright (C) 1995-2004 Miquel van Smoorenburg + */ + +#if defined(__FreeBSD_kernel__) +# define VIR_INITCTL_FIFO "/etc/.initctl" +#else +# define VIR_INITCTL_FIFO "/dev/initctl" +#endif + +#define VIR_INITCTL_MAGIC 0x03091969 +#define VIR_INITCTL_CMD_START 0 +#define VIR_INITCTL_CMD_RUNLVL 1 +#define VIR_INITCTL_CMD_POWERFAIL 2 +#define VIR_INITCTL_CMD_POWERFAILNOW 3 +#define VIR_INITCTL_CMD_POWEROK 4 +#define VIR_INITCTL_CMD_BSD 5 +#define VIR_INITCTL_CMD_SETENV 6 +#define VIR_INITCTL_CMD_UNSETENV 7 + +#define VIR_INITCTL_CMD_CHANGECONS 12345 + +#ifdef MAXHOSTNAMELEN +# define VIR_INITCTL_RQ_HLEN MAXHOSTNAMELEN +#else +# define VIR_INITCTL_RQ_HLEN 64 +#endif + +/* +* This is what BSD 4.4 uses when talking to init. +* Linux doesn't use this right now. +*/ +struct virInitctlRequestBSD { + char gen_id[8]; /* Beats me.. telnetd uses "fe" */ + char tty_id[16]; /* Tty name minus /dev/tty */ + char host[VIR_INITCTL_RQ_HLEN]; /* Hostname */ + char term_type[16]; /* Terminal type */ + int signal; /* Signal to send */ + int pid_value; /* Process to send to */ + char exec_name[128]; /* Program to execute */ + char reserved[128]; /* For future expansion. */ +}; + + +/* + * Because of legacy interfaces, "runlevel" and "sleeptime" + * aren't in a separate struct in the union. + * + * The weird sizes are because init expects the whole + * struct to be 384 bytes. + */ +struct virInitctlRequest { + int magic; /* Magic number */ + int cmd; /* What kind of request */ + int runlevel; /* Runlevel to change to */ + int sleeptime; /* Time between TERM and KILL */ + union { + struct virInitctlRequestBSD bsd; + char data[368]; + } i; +}; + + +/* + * Send a message to init to change the runlevel + * + * Returns 1 on success, 0 if initctl does not exist, -1 on error + */ +int virInitctlSetRunLevel(virInitctlRunLevel level, + const char *vroot) +{ + struct virInitctlRequest req; + int fd = -1; + char *path = NULL; + int ret = -1; + + memset(&req, 0, sizeof(req)); + + req.magic = VIR_INITCTL_MAGIC; + req.sleeptime = 0; + req.cmd = VIR_INITCTL_CMD_RUNLVL; + req.runlevel = level; + + if (vroot) { + if (virAsprintf(&path, "%s/%s", vroot, VIR_INITCTL_FIFO) < 0) { + virReportOOMError(); + return -1; + } + } else { + if (!(path = strdup(VIR_INITCTL_FIFO))) { + virReportOOMError(); + return -1; + } + } + + if ((fd = open(path, O_WRONLY|O_NDELAY|O_CLOEXEC|O_NOCTTY)) < 0) { + if (errno == ENOENT) { + ret = 0; + goto cleanup; + } + virReportSystemError(errno, + _("Cannot open init control %s"), + path); + goto cleanup; + } + + if (safewrite(fd, &req, sizeof(req)) != sizeof(req)) { + virReportSystemError(errno, + _("Failed to send request to init control %s"), + path); + goto cleanup; + } + + ret = 1; + +cleanup: + VIR_FREE(path); + VIR_FORCE_CLOSE(fd); + return ret; +} diff --git a/src/util/virinitctl.h b/src/util/virinitctl.h new file mode 100644 index 0000000..09c078f --- /dev/null +++ b/src/util/virinitctl.h @@ -0,0 +1,41 @@ +/* + * virinitctl.h: API for talking to init systems via initctl + * + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __VIR_INITCTL_H__ +# define __VIR_INITCTL_H__ + +typedef enum virInitctlRunLevel virInitctlRunLevel; +enum virInitctlRunLevel { + VIR_INITCTL_RUNLEVEL_POWEROFF = 0, + VIR_INITCTL_RUNLEVEL_1 = 1, + VIR_INITCTL_RUNLEVEL_2 = 2, + VIR_INITCTL_RUNLEVEL_3 = 3, + VIR_INITCTL_RUNLEVEL_4 = 4, + VIR_INITCTL_RUNLEVEL_5 = 5, + VIR_INITCTL_RUNLEVEL_REBOOT = 6, +}; + +int virInitctlSetRunLevel(virInitctlRunLevel level, + const char *vroot); + +#endif diff --git a/src/util/virterror.c b/src/util/virterror.c index 213188e..1142c40 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -117,6 +117,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "SSH transport layer", /* 50 */ "Lock Space", + "Init control", ) -- 1.7.11.7
From: "Daniel P. Berrange" <berrange@redhat.com> The virDomainShutdownFlags and virDomainReboot APIs allow the caller to request the operation is implemented via either acpi button press or a guest agent. For containers, a couple of other methods make sense, a message to /dev/initctl, and direct kill(SIGTERM|HUP) of the container init process. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/libvirt/libvirt.h.in | 4 ++++ tools/virsh-domain.c | 18 ++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 84dcde1..1859165 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1385,6 +1385,8 @@ typedef enum { VIR_DOMAIN_SHUTDOWN_DEFAULT = 0, /* hypervisor choice */ VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ VIR_DOMAIN_SHUTDOWN_GUEST_AGENT = (1 << 1), /* Use guest agent */ + VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl */ + VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal */ } virDomainShutdownFlagValues; int virDomainShutdown (virDomainPtr domain); @@ -1395,6 +1397,8 @@ typedef enum { VIR_DOMAIN_REBOOT_DEFAULT = 0, /* hypervisor choice */ VIR_DOMAIN_REBOOT_ACPI_POWER_BTN = (1 << 0), /* Send ACPI event */ VIR_DOMAIN_REBOOT_GUEST_AGENT = (1 << 1), /* Use guest agent */ + VIR_DOMAIN_REBOOT_INITCTL = (1 << 2), /* Use initctl */ + VIR_DOMAIN_REBOOT_SIGNAL = (1 << 3), /* Send a signal */ } virDomainRebootFlagValues; int virDomainReboot (virDomainPtr domain, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cc47383..ff6a9a0 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4046,8 +4046,13 @@ cmdShutdown(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN; } else if (STREQ(mode, "agent")) { flags |= VIR_DOMAIN_SHUTDOWN_GUEST_AGENT; + } else if (STREQ(mode, "initctl")) { + flags |= VIR_DOMAIN_SHUTDOWN_INITCTL; + } else if (STREQ(mode, "signal")) { + flags |= VIR_DOMAIN_SHUTDOWN_SIGNAL; } else { - vshError(ctl, _("Unknown mode %s value, expecting 'acpi' or 'agent'"), mode); + vshError(ctl, _("Unknown mode %s value, expecting " + "'acpi', 'agent', 'initctl' or 'signal'"), mode); return false; } } @@ -4101,11 +4106,16 @@ cmdReboot(vshControl *ctl, const vshCmd *cmd) if (mode) { if (STREQ(mode, "acpi")) { - flags |= VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN; + flags |= VIR_DOMAIN_REBOOT_ACPI_POWER_BTN; } else if (STREQ(mode, "agent")) { - flags |= VIR_DOMAIN_SHUTDOWN_GUEST_AGENT; + flags |= VIR_DOMAIN_REBOOT_GUEST_AGENT; + } else if (STREQ(mode, "initctl")) { + flags |= VIR_DOMAIN_REBOOT_INITCTL; + } else if (STREQ(mode, "signal")) { + flags |= VIR_DOMAIN_REBOOT_SIGNAL; } else { - vshError(ctl, _("Unknown mode %s value, expecting 'acpi' or 'agent'"), mode); + vshError(ctl, _("Unknown mode %s value, expecting " + "'acpi', 'agent', 'initctl' or 'signal'"), mode); return false; } } -- 1.7.11.7
The virDomainShutdownFlags and virDomainReboot APIs allow the caller to request the operation is implemented via either acpi button press or a guest agent. For containers, a couple of other methods make sense, a message to /dev/initctl, and direct kill(SIGTERM|HUP) of the container init process.
Indeed - this is a nice way to tie in your earlier patches.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/libvirt/libvirt.h.in | 4 ++++ tools/virsh-domain.c | 18 ++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-)
Alas, you are missing documentation of the new flags in src/libvirt.c (not that the existing flags were documented there yet), as well as missing an update to this portion of libvirt.c:virDomainShutdownFlags() (and its reboot counterpart): /* At most one of these two flags should be set. */ if ((flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) && (flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) { virReportInvalidArg(flags, "%s", _("flags for acpi power button and guest agent are mutually exclusive")); goto error; } That should be updated to check that now at most one of the four flags are present. Everything else looks okay, but to ensure we get decent documentation, I'll defer ack until you post v2.
On Thu, Nov 29, 2012 at 01:27:26PM -0500, Eric Blake wrote:
The virDomainShutdownFlags and virDomainReboot APIs allow the caller to request the operation is implemented via either acpi button press or a guest agent. For containers, a couple of other methods make sense, a message to /dev/initctl, and direct kill(SIGTERM|HUP) of the container init process.
Indeed - this is a nice way to tie in your earlier patches.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/libvirt/libvirt.h.in | 4 ++++ tools/virsh-domain.c | 18 ++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-)
Alas, you are missing documentation of the new flags in src/libvirt.c (not that the existing flags were documented there yet),
Well the docs refer to the corresponding enum, whcih will be included in the docs with its descriptions. So isn't that better than duplicating the enum descriptions again.
as well as missing an update to this portion of libvirt.c:virDomainShutdownFlags() (and its reboot counterpart):
/* At most one of these two flags should be set. */ if ((flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) && (flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) { virReportInvalidArg(flags, "%s", _("flags for acpi power button and guest agent are mutually exclusive")); goto error; }
That should be updated to check that now at most one of the four flags are present.
IMHO that is a bogus check that should be deleted. Though the QEMU driver might not implement it, it seems perfectly valid to ask the driver to try multiple methods - after all it is able todo that by default. Sure you can't specify ordering, but that's fine. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Alas, you are missing documentation of the new flags in src/libvirt.c (not that the existing flags were documented there yet),
Well the docs refer to the corresponding enum, whcih will be included in the docs with its descriptions. So isn't that better than duplicating the enum descriptions again.
I suppose that's okay, if you think the enum itself has enough details about what each flag means. http://libvirt.org/html/libvirt-libvirt.html#virDomainShutdownFlagValues But there's still the question of semantics, on whether we have properly captured the fact that you can now specify more than one flag, and if so, it is hypervisor choice on which order to attempt the flags. At any rate, ACK to this patch, now that you've done a separate patch for moving the mutual exclusion of flags into the qemu driver.
From: "Daniel P. Berrange" <berrange@redhat.com> Add support for doing controlled shutdown / reboot in the LXC driver. The default behaviour is to try talking to /dev/initctl inside the container's virtual root (/proc/$INITPID/root). This works with sysvinit or systemd. If that file does not exist then send SIGTERM (for shutdown) or SIGHUP (for reboot). These signals are not any kind of particular standard for shutdown or reboot, just something apps can choose to handle. The new virDomainSendProcessSignal allows for sending custom signals. We might allow the choice of SIGTERM/HUP to be configured for LXC containers via the XML in the future. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_driver.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 991b593..210bc2d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -57,6 +57,7 @@ #include "domain_audit.h" #include "domain_nwfilter.h" #include "network/bridge_driver.h" +#include "virinitctl.h" #include "virnetdev.h" #include "virnetdevtap.h" #include "virnodesuspend.h" @@ -2683,6 +2684,179 @@ lxcListAllDomains(virConnectPtr conn, return ret; } +static int +lxcDomainShutdownFlags(virDomainPtr dom, + unsigned int flags) +{ + virLXCDriverPtr driver = dom->conn->privateData; + virLXCDomainObjPrivatePtr priv; + virDomainObjPtr vm; + char *vroot = NULL; + int ret = -1; + int rc; + + virCheckFlags(VIR_DOMAIN_SHUTDOWN_INITCTL | + VIR_DOMAIN_SHUTDOWN_SIGNAL, -1); + + lxcDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + lxcDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("No domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + priv = vm->privateData; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("Domain is not running")); + goto cleanup; + } + + if (priv->initpid == 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("Init process ID is not yet known")); + goto cleanup; + } + + if (virAsprintf(&vroot, "/proc/%llu/root", + (unsigned long long)priv->initpid) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (flags == 0 || + (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) { + if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, + vroot)) < 0) { + goto cleanup; + } + if (rc == 0 && flags != 0 && + ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Container does not provide an initctl pipe")); + goto cleanup; + } + } else { + rc = 0; + } + + if (rc == 0 && + (flags == 0 || + (flags & VIR_DOMAIN_SHUTDOWN_SIGNAL))) { + if (kill(priv->initpid, SIGTERM) < 0 && + errno != ESRCH) { + virReportSystemError(errno, + _("Unable to send SIGTERM to init pid %llu"), + (unsigned long long)priv->initpid); + goto cleanup; + } + } + + ret = 0; + +cleanup: + VIR_FREE(vroot); + if (vm) + virDomainObjUnlock(vm); + return ret; +} + +static int +lxcDomainShutdown(virDomainPtr dom) +{ + return lxcDomainShutdownFlags(dom, 0); +} + +static int +lxcDomainReboot(virDomainPtr dom, + unsigned int flags) +{ + virLXCDriverPtr driver = dom->conn->privateData; + virLXCDomainObjPrivatePtr priv; + virDomainObjPtr vm; + char *vroot = NULL; + int ret = -1; + int rc; + + virCheckFlags(VIR_DOMAIN_REBOOT_INITCTL | + VIR_DOMAIN_REBOOT_SIGNAL, -1); + + lxcDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + lxcDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("No domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + priv = vm->privateData; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("Domain is not running")); + goto cleanup; + } + + if (priv->initpid == 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("Init process ID is not yet known")); + goto cleanup; + } + + if (virAsprintf(&vroot, "/proc/%llu/root", + (unsigned long long)priv->initpid) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (flags == 0 || + (flags & VIR_DOMAIN_REBOOT_INITCTL)) { + if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, + vroot)) < 0) { + goto cleanup; + } + if (rc == 0 && flags != 0 && + ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Container does not provide an initctl pipe")); + goto cleanup; + } + } else { + rc = 0; + } + + if (rc == 0 && + (flags == 0 || + (flags & VIR_DOMAIN_REBOOT_SIGNAL))) { + if (kill(priv->initpid, SIGHUP) < 0 && + errno != ESRCH) { + virReportSystemError(errno, + _("Unable to send SIGTERM to init pid %llu"), + (unsigned long long)priv->initpid); + goto cleanup; + } + } + + ret = 0; + +cleanup: + VIR_FREE(vroot); + if (vm) + virDomainObjUnlock(vm); + return ret; +} + + /* Function Tables */ static virDriver lxcDriver = { .no = VIR_DRV_LXC, @@ -2751,6 +2925,9 @@ static virDriver lxcDriver = { .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */ .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */ + .domainShutdown = lxcDomainShutdown, /* 1.0.1 */ + .domainShutdownFlags = lxcDomainShutdownFlags, /* 1.0.1 */ + .domainReboot = lxcDomainReboot, /* 1.0.1 */ }; static virStateDriver lxcStateDriver = { -- 1.7.11.7
<...>
+static int +lxcDomainReboot(virDomainPtr dom, + unsigned int flags) +{ + virLXCDriverPtr driver = dom->conn->privateData; + virLXCDomainObjPrivatePtr priv; + virDomainObjPtr vm; + char *vroot = NULL; + int ret = -1; + int rc; + + virCheckFlags(VIR_DOMAIN_REBOOT_INITCTL | + VIR_DOMAIN_REBOOT_SIGNAL, -1); +
<...>
+ + if (flags == 0 || + (flags & VIR_DOMAIN_REBOOT_INITCTL)) { + if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, + vroot)) < 0) { + goto cleanup; + } + if (rc == 0 && flags != 0 && + ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) {
((flags & ~VIR_DOMAIN_REBOOT_INITCTL) == 0)) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Container does not provide an initctl pipe")); + goto cleanup; + } + } else { + rc = 0; + } +
<...>
Add support for doing controlled shutdown / reboot in the LXC driver. The default behaviour is to try talking to /dev/initctl inside the container's virtual root (/proc/$INITPID/root). This works with sysvinit or systemd. If that file does not exist then send SIGTERM (for shutdown) or SIGHUP (for reboot). These signals are not any kind of particular standard for shutdown or reboot, just something apps can choose to handle. The new virDomainSendProcessSignal allows for sending custom signals.
We might allow the choice of SIGTERM/HUP to be configured for LXC containers via the XML in the future.
Maybe even via new attributes or sub-elements of the existing <on_reboot> XML.
+ + if (flags == 0 || + (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) { + if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, + vroot)) < 0) { + goto cleanup; + } + if (rc == 0 && flags != 0 &&
Based on libvirt.c (well, if you follow my advice in 2/3 about enforcing the mutual exclusion between the four explicit shutdown methods), if 'flags != 0', then we are guaranteed that 'flags == VIR_DOMAIN_SHUTDOWN_INITCTL)' at this point...
+ ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) {
...which means this leg of the conditional is always true. Or is your intent to allow the user to specify multiple flags rather than enforcing mutual exclusion between the flags? And if so, you need to fix libvirt.c to drop the mutual exclusion, as well as to document that when multiple flags are specified, it is up to the hypervisor which method is attempted first.
+static int +lxcDomainReboot(virDomainPtr dom, + unsigned int flags) +{ + virLXCDriverPtr driver = dom->conn->privateData; + virLXCDomainObjPrivatePtr priv; + virDomainObjPtr vm; + char *vroot = NULL; + int ret = -1; + int rc; + + virCheckFlags(VIR_DOMAIN_REBOOT_INITCTL | + VIR_DOMAIN_REBOOT_SIGNAL, -1);
This code is very similar to the shutdown; is it worth factoring out a helper method that takes as additional arguments the choice of initctl message (_POWEROFF vs. _REBOOT) and signal (SIGKILL vs. SIGHUP) to use?
+ if (flags == 0 || + (flags & VIR_DOMAIN_REBOOT_INITCTL)) { + if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, + vroot)) < 0) { + goto cleanup; + } + if (rc == 0 && flags != 0 && + ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) {
Same review comments apply regarding whether we enforce mutual exclusion of flags at the API level.
On Thu, Nov 29, 2012 at 01:39:34PM -0500, Eric Blake wrote:
Add support for doing controlled shutdown / reboot in the LXC driver. The default behaviour is to try talking to /dev/initctl inside the container's virtual root (/proc/$INITPID/root). This works with sysvinit or systemd. If that file does not exist then send SIGTERM (for shutdown) or SIGHUP (for reboot). These signals are not any kind of particular standard for shutdown or reboot, just something apps can choose to handle. The new virDomainSendProcessSignal allows for sending custom signals.
We might allow the choice of SIGTERM/HUP to be configured for LXC containers via the XML in the future.
Maybe even via new attributes or sub-elements of the existing <on_reboot> XML.
+ + if (flags == 0 || + (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) { + if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, + vroot)) < 0) { + goto cleanup; + } + if (rc == 0 && flags != 0 &&
Based on libvirt.c (well, if you follow my advice in 2/3 about enforcing the mutual exclusion between the four explicit shutdown methods), if 'flags != 0', then we are guaranteed that 'flags == VIR_DOMAIN_SHUTDOWN_INITCTL)' at this point...
+ ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) {
...which means this leg of the conditional is always true.
Or is your intent to allow the user to specify multiple flags rather than enforcing mutual exclusion between the flags? And if so, you need to fix libvirt.c to drop the mutual exclusion, as well as to document that when multiple flags are specified, it is up to the hypervisor which method is attempted first.
Yep, I think allowing multiple flags is fine at the API level. I'd do that as a followup commit since it doesn't affect this, only the existing QEMU code. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Or is your intent to allow the user to specify multiple flags rather than enforcing mutual exclusion between the flags? And if so, you need to fix libvirt.c to drop the mutual exclusion, as well as to document that when multiple flags are specified, it is up to the hypervisor which method is attempted first.
Yep, I think allowing multiple flags is fine at the API level.
I'd do that as a followup commit since it doesn't affect this, only the existing QEMU code.
In which case, I think this patch is okay as-is (unless you wanted to follow my other suggestion of refactoring the common grunt code into a helper method, to reduce code duplication). ACK.
To be able todo controlled shutdown/reboot of containers an
s/todo/to do/
API to talk to init via /dev/initctl is required. Fortunately this is quite straightforward to implement, and is supported by both sysvinit and systemd. Upstart support for /dev/initctl is unclear.
+++ b/src/util/virinitctl.c @@ -0,0 +1,161 @@
+ +/* These constants & struct definitions are taken from + * systemd, under terms of LGPLv2+ + * + * initreq.h Interface to talk to init through /dev/initctl. + * + * Copyright (C) 1995-2004 Miquel van Smoorenburg + */
Thankfully, compatible with our usage.
+ +#if defined(__FreeBSD_kernel__) +# define VIR_INITCTL_FIFO "/etc/.initctl" +#else +# define VIR_INITCTL_FIFO "/dev/initctl" +#endif + +#define VIR_INITCTL_MAGIC 0x03091969
Wonder if the original author of this code picked a birthdate. Gotta love the Easter eggs present in open source software :)
+ +/* + * Because of legacy interfaces, "runlevel" and "sleeptime" + * aren't in a separate struct in the union. + * + * The weird sizes are because init expects the whole + * struct to be 384 bytes. + */ +struct virInitctlRequest { + int magic; /* Magic number */
'int' is not necessarily 4 bytes; I would feel slightly more comfortable had upstream used int32_t. I know you are just copying code from an existing header (so don't change the struct itself), but wonder if we should at least add our own sanity checking: verify(sizeof(virInitctlRequest)) == 384);
+ + if ((fd = open(path, O_WRONLY|O_NDELAY|O_CLOEXEC|O_NOCTTY)) < 0)
O_NDELAY is non-standard. I would spell it according to POSIX as O_NONBLOCK.
+typedef enum virInitctlRunLevel virInitctlRunLevel; +enum virInitctlRunLevel { + VIR_INITCTL_RUNLEVEL_POWEROFF = 0, + VIR_INITCTL_RUNLEVEL_1 = 1, + VIR_INITCTL_RUNLEVEL_2 = 2, + VIR_INITCTL_RUNLEVEL_3 = 3, + VIR_INITCTL_RUNLEVEL_4 = 4, + VIR_INITCTL_RUNLEVEL_5 = 5, + VIR_INITCTL_RUNLEVEL_REBOOT = 6,
Should you add VIR_INITCTL_RUNLEVEL_LAST, in case we ever expand this list? I've never used messages over the socket to initctl myself, but I'm assuming your code works. ACK once you fix the nits I've pointed out above.
On Thu, Nov 29, 2012 at 01:16:09PM -0500, Eric Blake wrote:
To be able todo controlled shutdown/reboot of containers an
s/todo/to do/
API to talk to init via /dev/initctl is required. Fortunately this is quite straightforward to implement, and is supported by both sysvinit and systemd. Upstart support for /dev/initctl is unclear.
+++ b/src/util/virinitctl.c @@ -0,0 +1,161 @@
+ +/* These constants & struct definitions are taken from + * systemd, under terms of LGPLv2+ + * + * initreq.h Interface to talk to init through /dev/initctl. + * + * Copyright (C) 1995-2004 Miquel van Smoorenburg + */
Thankfully, compatible with our usage.
+ +#if defined(__FreeBSD_kernel__) +# define VIR_INITCTL_FIFO "/etc/.initctl" +#else +# define VIR_INITCTL_FIFO "/dev/initctl" +#endif + +#define VIR_INITCTL_MAGIC 0x03091969
Wonder if the original author of this code picked a birthdate. Gotta love the Easter eggs present in open source software :)
+ +/* + * Because of legacy interfaces, "runlevel" and "sleeptime" + * aren't in a separate struct in the union. + * + * The weird sizes are because init expects the whole + * struct to be 384 bytes. + */ +struct virInitctlRequest { + int magic; /* Magic number */
'int' is not necessarily 4 bytes; I would feel slightly more comfortable had upstream used int32_t. I know you are just copying code from an existing header (so don't change the struct itself), but wonder if we should at least add our own sanity checking:
verify(sizeof(virInitctlRequest)) == 384);
I'm just adding the verify, since I think that's the more important thing todo.
+ + if ((fd = open(path, O_WRONLY|O_NDELAY|O_CLOEXEC|O_NOCTTY)) < 0)
O_NDELAY is non-standard. I would spell it according to POSIX as O_NONBLOCK.
Yep, good point.
+typedef enum virInitctlRunLevel virInitctlRunLevel; +enum virInitctlRunLevel { + VIR_INITCTL_RUNLEVEL_POWEROFF = 0, + VIR_INITCTL_RUNLEVEL_1 = 1, + VIR_INITCTL_RUNLEVEL_2 = 2, + VIR_INITCTL_RUNLEVEL_3 = 3, + VIR_INITCTL_RUNLEVEL_4 = 4, + VIR_INITCTL_RUNLEVEL_5 = 5, + VIR_INITCTL_RUNLEVEL_REBOOT = 6,
Should you add VIR_INITCTL_RUNLEVEL_LAST, in case we ever expand this list?
Unlikely, but doesn't hurt to have a sentinal Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Hu Tao