[libvirt] [RFC PATCH 0/7] Adding 'config' driver
This patchset adds a driver named 'config' that allows access to configuration data, such as secret and storage definitions. This is a pre-requisite for my next patchset which resolves the race condition on libvirtd startup and the circular dependencies between QEMU and the storage driver. The basic rationale behind this idea is that there exist circumstances under which a driver may need to access things such as secrets during a time at which there is no active connection to a hypervisor. Without a connection, the data can't be accessed currently. I felt that this was a much simpler solution to the problem that building new APIs that do not require a connection to operate. This driver is technically what one may call a hypervisor driver, but it does not implement any domain operations. It simply exists to handle requests by drivers for access to informatino that would otherwise require a connection. The URI used for this driver is 'config:///' and has been tested working on 4 different machines of mine, running three different distributions of Linux (Archlinux, Gentoo, and CentOS). Being a very simple driver, I would expect it to work pretty much anywhere. I would love to hear any comments and suggestions you may have about this driver. At the very least this plus my next patchset resolves the startup race condition on my machine. If a more robust setup (likely a new internal API) is in the works, this driver could act as a band-aid to allow access to this type of data in the interim if a better resolution is a ways off. Adam Walters (7): config: Adding source for the config driver config: Adding header for the config driver virterror: Adding a new VIR_FROM_ define libvirtd: Add config driver hooks po: Add config_driver.c to POTFILES.in configure: Add config driver to configure script Makefile: Add config driver to src/Makefile.am configure.ac | 10 ++ daemon/libvirtd.c | 21 ++-- include/libvirt/virterror.h | 2 + po/POTFILES.in | 1 + src/Makefile.am | 25 +++++ src/config/config_driver.c | 237 ++++++++++++++++++++++++++++++++++++++++++++ src/config/config_driver.h | 44 ++++++++ src/util/virerror.c | 2 + 8 files changed, 336 insertions(+), 6 deletions(-) create mode 100644 src/config/config_driver.c create mode 100644 src/config/config_driver.h -- 1.8.5.2
This is the source code to the config driver. This driver is a hypervisor driver that does not support any domain operations. The sole purpose of this driver is to allow access to various bits of configuration information, such as secret or network definitions, from the initialization and auto-start routines of other drivers. This is the first step toward breaking the circular dependencies present in QEMU and the Storage driver, in addition to preventing future cases. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/config/config_driver.c | 237 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 237 insertions(+) create mode 100644 src/config/config_driver.c diff --git a/src/config/config_driver.c b/src/config/config_driver.c new file mode 100644 index 0000000..a057300 --- /dev/null +++ b/src/config/config_driver.c @@ -0,0 +1,237 @@ +/* + * config_driver.c: core driver methods for accessing configs + * + * Copyright (C) 2013 Adam Walters + * + * 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/>. + * + * Author: Adam Walters <adam@pandorasboxen.com> + */ +#include <config.h> +#include <string.h> + +#include "internal.h" +#include "datatypes.h" +#include "driver.h" +#include "virlog.h" +#include "viralloc.h" +#include "virerror.h" +#include "virstring.h" +#include "viraccessapicheck.h" +#include "nodeinfo.h" +#include "config_driver.h" + +#define VIR_FROM_THIS VIR_FROM_CONFIG + +virConfigDriverPtr config_driver = NULL; + +static int configStateCleanup(void) { + if (config_driver == NULL) + return -1; + + virObjectUnref(config_driver->closeCallbacks); + + virSysinfoDefFree(config_driver->hostsysinfo); + + virMutexDestroy(&config_driver->lock); + VIR_FREE(config_driver); + + return 0; +} + +static int configStateInitialize(bool privileged, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + if (!privileged) { + VIR_INFO("Not running privileged, disabling driver"); + return 0; + } + + if (VIR_ALLOC(config_driver) < 0) + return -1; + + if (virMutexInit(&config_driver->lock) < 0) { + VIR_ERROR(_("cannot initialize mutex")); + VIR_FREE(config_driver); + return -1; + } + + config_driver->hostsysinfo = virSysinfoRead(); + + if (!(config_driver->closeCallbacks = virCloseCallbacksNew())) + goto cleanup; + + return 0; + +cleanup: + configStateCleanup(); + return -1; +} + +static int configStateReload(void) { + return 0; +} + +static virDrvOpenStatus configConnectOpen(virConnectPtr conn, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + unsigned int flags) +{ + virDrvOpenStatus ret = VIR_DRV_OPEN_ERROR; + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + + if (conn->uri != NULL) { + /* If URI isn't 'config', decline the connection */ + if (conn->uri->scheme == NULL || + STRNEQ(conn->uri->scheme, "config")) { + ret = VIR_DRV_OPEN_DECLINED; + goto cleanup; + } + + /* Allow remote driver to deal with URIs with hostname set */ + if (conn->uri->server != NULL) { + ret = VIR_DRV_OPEN_DECLINED; + goto cleanup; + } + + if (config_driver == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("config state driver is not active")); + goto cleanup; + } + } else { + ret = VIR_DRV_OPEN_DECLINED; + goto cleanup; + } + + if (virConnectOpenEnsureACL(conn) < 0) + goto cleanup; + + conn->privateData = config_driver; + + ret = VIR_DRV_OPEN_SUCCESS; +cleanup: + return ret; +} + +static int configConnectClose(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* Cleanup callbacks on config_driver */ + /* Set conn->privateData to NULL */ + return 0; +} + +static int +configConnectSupportsFeature(virConnectPtr conn, int feature ATTRIBUTE_UNUSED) +{ + if (virConnectSupportsFeatureEnsureACL(conn) < 0) + return -1; + + return 0; +} + +static const char *configConnectGetType(virConnectPtr conn ATTRIBUTE_UNUSED) { + if (virConnectGetTypeEnsureACL(conn) < 0) + return NULL; + + return "config"; +} + +static int configConnectIsSecure(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 1; +} + +static int configConnectIsEncrypted(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 0; +} + +static char *configConnectGetHostname(virConnectPtr conn) +{ + if (virConnectGetHostnameEnsureACL(conn) < 0) + return NULL; + + return virGetHostname(); +} + +static char * +configConnectGetSysinfo(virConnectPtr conn, unsigned int flags) +{ + virConfigDriverPtr driver = conn->privateData; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virCheckFlags(0, NULL); + + if (virConnectGetSysinfoEnsureACL(conn) < 0) + return NULL; + + if (!driver->hostsysinfo) { + return NULL; + } + + if (virSysinfoFormat(&buf, driver->hostsysinfo) < 0) + return NULL; + + if (virBufferError(&buf)) { + virReportOOMError(); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +static int +configNodeGetInfo(virConnectPtr conn, + virNodeInfoPtr nodeinfo) +{ + if (virNodeGetInfoEnsureACL(conn) < 0) + return -1; + + return nodeGetInfo(nodeinfo); +} + +static int configConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 1; +} + +static virDriver configDriver = { + .name = "config", + .connectOpen = configConnectOpen, /* 0.2.0 */ + .connectClose = configConnectClose, /* 0.2.0 */ + .connectSupportsFeature = configConnectSupportsFeature, /* 0.5.0 */ + .connectGetType = configConnectGetType, /* 0.2.0 */ + .connectGetHostname = configConnectGetHostname, /* 0.3.3 */ + .connectGetSysinfo = configConnectGetSysinfo, /* 0.8.8 */ + .nodeGetInfo = configNodeGetInfo, /* 0.2.0 */ + .connectIsEncrypted = configConnectIsEncrypted, /* 0.7.3 */ + .connectIsSecure = configConnectIsSecure, /* 0.7.3 */ + .connectIsAlive = configConnectIsAlive, /* 0.9.8 */ +}; + + +static virStateDriver configStateDriver = { + .name = "config", + .stateInitialize = configStateInitialize, + .stateCleanup = configStateCleanup, + .stateReload = configStateReload, +}; + +int configRegister(void) { + virRegisterDriver(&configDriver); + virRegisterStateDriver(&configStateDriver); + return 0; +} -- 1.8.5.2
On 12/21/13 05:03, Adam Walters wrote:
This is the source code to the config driver. This driver is a hypervisor driver that does not support any domain operations. The sole purpose of this driver is to allow access to various bits of configuration information, such as secret or network definitions, from the initialization and auto-start routines of other drivers. This is the first step toward breaking the circular dependencies present in QEMU and the Storage driver, in addition to preventing future cases.
Again, please trim the commit message (see 2/3 in your other series for instructions).
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/config/config_driver.c | 237 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 237 insertions(+) create mode 100644 src/config/config_driver.c
diff --git a/src/config/config_driver.c b/src/config/config_driver.c new file mode 100644 index 0000000..a057300 --- /dev/null +++ b/src/config/config_driver.c @@ -0,0 +1,237 @@ +/* + * config_driver.c: core driver methods for accessing configs + * + * Copyright (C) 2013 Adam Walters + * + * 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/>. + * + * Author: Adam Walters <adam@pandorasboxen.com> + */ +#include <config.h> +#include <string.h> + +#include "internal.h" +#include "datatypes.h" +#include "driver.h" +#include "virlog.h" +#include "viralloc.h" +#include "virerror.h" +#include "virstring.h" +#include "viraccessapicheck.h" +#include "nodeinfo.h" +#include "config_driver.h" + +#define VIR_FROM_THIS VIR_FROM_CONFIG + +virConfigDriverPtr config_driver = NULL; + +static int configStateCleanup(void) { + if (config_driver == NULL) + return -1; + + virObjectUnref(config_driver->closeCallbacks); + + virSysinfoDefFree(config_driver->hostsysinfo); + + virMutexDestroy(&config_driver->lock); + VIR_FREE(config_driver); + + return 0; +} + +static int configStateInitialize(bool privileged, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + if (!privileged) { + VIR_INFO("Not running privileged, disabling driver"); + return 0; + }
Won't this driver be beneficial for session connections too? (they run unprivileged.
+ + if (VIR_ALLOC(config_driver) < 0) + return -1; + + if (virMutexInit(&config_driver->lock) < 0) { + VIR_ERROR(_("cannot initialize mutex")); + VIR_FREE(config_driver); + return -1; + } + + config_driver->hostsysinfo = virSysinfoRead(); + + if (!(config_driver->closeCallbacks = virCloseCallbacksNew())) + goto cleanup; + + return 0; + +cleanup: + configStateCleanup(); + return -1; +}
...
+ +static virDriver configDriver = { + .name = "config", + .connectOpen = configConnectOpen, /* 0.2.0 */ + .connectClose = configConnectClose, /* 0.2.0 */ + .connectSupportsFeature = configConnectSupportsFeature, /* 0.5.0 */ + .connectGetType = configConnectGetType, /* 0.2.0 */ + .connectGetHostname = configConnectGetHostname, /* 0.3.3 */ + .connectGetSysinfo = configConnectGetSysinfo, /* 0.8.8 */ + .nodeGetInfo = configNodeGetInfo, /* 0.2.0 */ + .connectIsEncrypted = configConnectIsEncrypted, /* 0.7.3 */ + .connectIsSecure = configConnectIsSecure, /* 0.7.3 */ + .connectIsAlive = configConnectIsAlive, /* 0.9.8 */
The comments here should state the release where the API was implemented for the driver, thus you need to change them to /* 1.2.2 */.
+}; + + +static virStateDriver configStateDriver = { + .name = "config", + .stateInitialize = configStateInitialize, + .stateCleanup = configStateCleanup, + .stateReload = configStateReload, +}; + +int configRegister(void) { + virRegisterDriver(&configDriver); + virRegisterStateDriver(&configStateDriver); + return 0; +}
Peter
Peter, Thank you for the feedback on my patches, and sorry for the delay. I was away all weekend, since it was a holiday weekend (definitely a plus, having my day job in education). I figured it would be a bit spammy to reply to each individual message, but I did read each of them, and will be working to implement the suggestions tonight and/or tomorrow night. On Mon, Jan 20, 2014 at 10:34 AM, Peter Krempa <pkrempa@redhat.com> wrote:
This is the source code to the config driver. This driver is a hypervisor driver that does not support any domain operations. The sole
On 12/21/13 05:03, Adam Walters wrote: purpose of this driver is to allow access to various bits of configuration information, such as secret or network definitions, from the initialization and auto-start routines of other drivers. This is the first step toward breaking the circular dependencies present in QEMU and the Storage driver, in addition to preventing future cases.
Again, please trim the commit message (see 2/3 in your other series for instructions).
I apologize for the long lines. I had forgotten about reading the log messages in a standard terminal, as I mostly read those in a web interface.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/config/config_driver.c | 237
+++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 237 insertions(+) create mode 100644 src/config/config_driver.c
diff --git a/src/config/config_driver.c b/src/config/config_driver.c new file mode 100644 index 0000000..a057300 --- /dev/null +++ b/src/config/config_driver.c @@ -0,0 +1,237 @@ +/* + * config_driver.c: core driver methods for accessing configs + * + * Copyright (C) 2013 Adam Walters + * + * 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/>. + * + * Author: Adam Walters <adam@pandorasboxen.com> + */ +#include <config.h> +#include <string.h> + +#include "internal.h" +#include "datatypes.h" +#include "driver.h" +#include "virlog.h" +#include "viralloc.h" +#include "virerror.h" +#include "virstring.h" +#include "viraccessapicheck.h" +#include "nodeinfo.h" +#include "config_driver.h" + +#define VIR_FROM_THIS VIR_FROM_CONFIG + +virConfigDriverPtr config_driver = NULL; + +static int configStateCleanup(void) { + if (config_driver == NULL) + return -1; + + virObjectUnref(config_driver->closeCallbacks); + + virSysinfoDefFree(config_driver->hostsysinfo); + + virMutexDestroy(&config_driver->lock); + VIR_FREE(config_driver); + + return 0; +} + +static int configStateInitialize(bool privileged, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + if (!privileged) { + VIR_INFO("Not running privileged, disabling driver"); + return 0; + }
Won't this driver be beneficial for session connections too? (they run unprivileged.
As for denying unprivileged connections, I'm honestly not sure if it would be useful. All this driver does is grant access to definitions of networks, storage pools, secrets, etc. I just ensured that this driver is loaded very early in the load order, and has zero external dependencies. I suppose that I made an assumption that the privileged variable indicated a privileged connection, but thinking about it more, I suppose it could also mean running privileged in the OS. I simply disabled the driver if unprivileged so that I would not leak any privileged information to an unprivileged connection by accident. If that isn't a concern (either I misunderstood the meaning of the privileged variable, or the code wouldn't leak information), I can certainly drop those lines to allow the driver when unprivileged. As I've mentioned before, I am not intimately familiar with the libvirt code, so I tend to err on the side of caution.
+ + if (VIR_ALLOC(config_driver) < 0) + return -1; + + if (virMutexInit(&config_driver->lock) < 0) { + VIR_ERROR(_("cannot initialize mutex")); + VIR_FREE(config_driver); + return -1; + } + + config_driver->hostsysinfo = virSysinfoRead(); + + if (!(config_driver->closeCallbacks = virCloseCallbacksNew())) + goto cleanup; + + return 0; + +cleanup: + configStateCleanup(); + return -1; +}
...
+ +static virDriver configDriver = { + .name = "config", + .connectOpen = configConnectOpen, /* 0.2.0 */ + .connectClose = configConnectClose, /* 0.2.0 */ + .connectSupportsFeature = configConnectSupportsFeature, /* 0.5.0 */ + .connectGetType = configConnectGetType, /* 0.2.0 */ + .connectGetHostname = configConnectGetHostname, /* 0.3.3 */ + .connectGetSysinfo = configConnectGetSysinfo, /* 0.8.8 */ + .nodeGetInfo = configNodeGetInfo, /* 0.2.0 */ + .connectIsEncrypted = configConnectIsEncrypted, /* 0.7.3 */ + .connectIsSecure = configConnectIsSecure, /* 0.7.3 */ + .connectIsAlive = configConnectIsAlive, /* 0.9.8 */
The comments here should state the release where the API was implemented for the driver, thus you need to change them to /* 1.2.2 */.
I hadn't forgotten about the API version comment, but when I wrote the patches, I simply had no idea when version would be next. Never know if there will be a minor version increment instead of the standard micro. Since it was more of an RFC-type patch, I didn't even have an idea of a timeframe for submission, since I thought there might be more discussion prior to looking at pulling a change of this type into the code.
+}; + + +static virStateDriver configStateDriver = { + .name = "config", + .stateInitialize = configStateInitialize, + .stateCleanup = configStateCleanup, + .stateReload = configStateReload, +}; + +int configRegister(void) { + virRegisterDriver(&configDriver); + virRegisterStateDriver(&configStateDriver); + return 0; +}
Peter
This is the header file for the config driver. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/config/config_driver.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 src/config/config_driver.h diff --git a/src/config/config_driver.h b/src/config/config_driver.h new file mode 100644 index 0000000..c74af71 --- /dev/null +++ b/src/config/config_driver.h @@ -0,0 +1,44 @@ +/* + * config_driver.h: core driver methods for accessing configs + * + * Copyright (C) 2013 Adam Walters + * + * 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/>. + * + * Author: Adam Walters <adam@pandorasboxen.com> + */ + +#ifndef __CONFIG_DRIVER_H__ +# define __CONFIG_DRIVER_H__ + +# include "virsysinfo.h" +# include "virclosecallbacks.h" + +typedef struct _virConfigDriver virConfigDriver; +typedef virConfigDriver *virConfigDriverPtr; + +struct _virConfigDriver { + virMutex lock; + + /* Immutable pointer, lockless APIs*/ + virSysinfoDefPtr hostsysinfo; + + /* Immutable pointer, self-locking APIs*/ + virCloseCallbacksPtr closeCallbacks; +}; + +int configRegister(void); + +#endif /* __CONFIG_DRIVER_H__ */ -- 1.8.5.2
On 12/21/13 05:03, Adam Walters wrote:
This is the header file for the config driver.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/config/config_driver.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 src/config/config_driver.h
Looks good, but should be squashed into 1/7. Peter
This patch adds VIR_FROM_CONFIG to the virErrorDomain enum. Both of these files must be patched in unison to prevent compilation failures. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- include/libvirt/virterror.h | 2 ++ src/util/virerror.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index e31e9c4..018c880 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -121,6 +121,8 @@ typedef enum { VIR_FROM_ACCESS = 55, /* Error from access control manager */ VIR_FROM_SYSTEMD = 56, /* Error from systemd code */ + VIR_FROM_CONFIG = 57, /* Error from config driver */ + # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST # endif diff --git a/src/util/virerror.c b/src/util/virerror.c index d9a9fc4..04a3acf 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -124,6 +124,8 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "Access Manager", /* 55 */ "Systemd", + + "Config Driver", /* 57 */ ) -- 1.8.5.2
On 12/21/13 05:03, Adam Walters wrote:
This patch adds VIR_FROM_CONFIG to the virErrorDomain enum. Both of these files must be patched in unison to prevent compilation failures.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- include/libvirt/virterror.h | 2 ++ src/util/virerror.c | 2 ++ 2 files changed, 4 insertions(+)
This patch should be squashed into 1/7. Looks good otherwise. Peter
This patch adds the config driver hooks and moves the secret driver hook definitions higher on the list. The secret driver move isn't strictly needed, but the comments state that these should be in preferred load order. Since other drivers might utilize the secret driver, it makes sense to have it loaded earlier in the startup sequence. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- daemon/libvirtd.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 49c42ad..251c2f4 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -95,6 +95,9 @@ # ifdef WITH_NWFILTER # include "nwfilter/nwfilter_driver.h" # endif +# ifdef WITH_CONFIG +# include "config/config_driver.h" +# endif #endif #include "configmake.h" @@ -369,18 +372,21 @@ static void daemonInitialize(void) * If they try to open a connection for a module that * is not loaded they'll get a suitable error at that point */ +# ifdef WITH_CONFIG + virDriverLoadModule("config"); +# endif # ifdef WITH_NETWORK virDriverLoadModule("network"); # endif +# ifdef WITH_SECRETS + virDriverLoadModule("secret"); +# endif # ifdef WITH_STORAGE virDriverLoadModule("storage"); # endif # ifdef WITH_NODE_DEVICES virDriverLoadModule("nodedev"); # endif -# ifdef WITH_SECRETS - virDriverLoadModule("secret"); -# endif # ifdef WITH_NWFILTER virDriverLoadModule("nwfilter"); # endif @@ -406,21 +412,24 @@ static void daemonInitialize(void) virDriverLoadModule("vbox"); # endif #else +# ifdef WITH_CONFIG + configRegister(); +# endif # ifdef WITH_NETWORK networkRegister(); # endif # ifdef WITH_INTERFACE interfaceRegister(); # endif +# ifdef WITH_SECRETS + secretRegister(); +# endif # ifdef WITH_STORAGE storageRegister(); # endif # ifdef WITH_NODE_DEVICES nodedevRegister(); # endif -# ifdef WITH_SECRETS - secretRegister(); -# endif # ifdef WITH_NWFILTER nwfilterRegister(); # endif -- 1.8.5.2
On 12/21/13 05:03, Adam Walters wrote:
This patch adds the config driver hooks and moves the secret driver hook definitions higher on the list. The secret driver move isn't strictly needed, but the comments state that these should be in preferred load order. Since other drivers might utilize the secret driver, it makes sense to have it loaded earlier in the startup sequence.
The change of module order looks okay to me but it should be split out into a separate patch. The loading of the config driver can then be squashed in the patch dealing with configure/makefile.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- daemon/libvirtd.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
Peter
Adding config_driver.c to POTFILES.in to fix a syntax-check error. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- po/POTFILES.in | 1 + 1 file changed, 1 insertion(+) diff --git a/po/POTFILES.in b/po/POTFILES.in index 49dfc9c..0e23610 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -27,6 +27,7 @@ src/conf/snapshot_conf.c src/conf/storage_conf.c src/conf/storage_encryption_conf.c src/conf/virchrdev.c +src/config/config_driver.c src/cpu/cpu.c src/cpu/cpu_generic.c src/cpu/cpu_map.c -- 1.8.5.2
On 12/21/13 05:03, Adam Walters wrote:
Adding config_driver.c to POTFILES.in to fix a syntax-check error.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- po/POTFILES.in | 1 + 1 file changed, 1 insertion(+)
diff --git a/po/POTFILES.in b/po/POTFILES.in index 49dfc9c..0e23610 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -27,6 +27,7 @@ src/conf/snapshot_conf.c src/conf/storage_conf.c src/conf/storage_encryption_conf.c src/conf/virchrdev.c +src/config/config_driver.c src/cpu/cpu.c src/cpu/cpu_generic.c src/cpu/cpu_map.c
This patch will need to be squashed into the patch that actually introduces the syntax-check error. Libvirt's policy is that after each patch the tree must be buildable and pass "make check" and " make syntax-check". Peter
This conditionally enables compilation of the config driver based on if we are building libvirtd or not. Since this is only needed for hypervisor modules during libvirtd startup, we don't need to bother compiling the config driver when only building the client. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- configure.ac | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/configure.ac b/configure.ac index ddbcc8e..4834c51 100644 --- a/configure.ac +++ b/configure.ac @@ -1606,6 +1606,16 @@ if test "$with_secrets" = "yes" ; then fi AM_CONDITIONAL([WITH_SECRETS], [test "$with_secrets" = "yes"]) +if test "$with_libvirtd" = "no" ; then + with_config=no +else + with_config=yes +fi +if test "$with_config" = "yes" ; then + AC_DEFINE_UNQUOTED([WITH_CONFIG], 1, [whether local config driver is enabled]) +fi +AM_CONDITIONAL([WITH_CONFIG], [test "$with_config" = "yes"]) + AC_ARG_WITH([storage-dir], [AS_HELP_STRING([--with-storage-dir], -- 1.8.5.2
On 12/21/13 05:03, Adam Walters wrote:
This conditionally enables compilation of the config driver based on if we are building libvirtd or not. Since this is only needed for hypervisor modules during libvirtd startup, we don't need to bother compiling the config driver when only building the client.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- configure.ac | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/configure.ac b/configure.ac index ddbcc8e..4834c51 100644 --- a/configure.ac +++ b/configure.ac @@ -1606,6 +1606,16 @@ if test "$with_secrets" = "yes" ; then fi AM_CONDITIONAL([WITH_SECRETS], [test "$with_secrets" = "yes"])
+if test "$with_libvirtd" = "no" ; then + with_config=no +else + with_config=yes +fi +if test "$with_config" = "yes" ; then + AC_DEFINE_UNQUOTED([WITH_CONFIG], 1, [whether local config driver is enabled]) +fi +AM_CONDITIONAL([WITH_CONFIG], [test "$with_config" = "yes"]) +
AC_ARG_WITH([storage-dir], [AS_HELP_STRING([--with-storage-dir],
This patch definitely needs to go after the makefile change, otherwise the tree won't be buildable after this patch. Also you may want to squash it with the makefile changes. Also the configure script contains a section outputting the config summary at the bottom of configure.ac where you need to add a corresponding entry for the new driver. Peter
This completes the addition of the config driver to libvirt. The final piece here is to add the config driver into the Makefile (via automake) so that the driver is actually compiled and linked. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/Makefile.am | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/Makefile.am b/src/Makefile.am index 57e163f..e0b3677 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -504,6 +504,7 @@ DRIVER_SOURCE_FILES = \ $(QEMU_DRIVER_SOURCES) \ $(REMOTE_DRIVER_SOURCES) \ $(SECRET_DRIVER_SOURCES) \ + $(CONFIG_DRIVER_SOURCES) \ $(STORAGE_DRIVER_SOURCES) \ $(TEST_DRIVER_SOURCES) \ $(UML_DRIVER_SOURCES) \ @@ -523,6 +524,7 @@ STATEFUL_DRIVER_SOURCE_FILES = \ $(NWFILTER_DRIVER_SOURCES) \ $(QEMU_DRIVER_SOURCES) \ $(SECRET_DRIVER_SOURCES) \ + $(CONFIG_DRIVER_SOURCES) \ $(STORAGE_DRIVER_SOURCES) \ $(UML_DRIVER_SOURCES) \ $(XEN_DRIVER_SOURCES) \ @@ -799,6 +801,9 @@ endif WITH_INTERFACE SECRET_DRIVER_SOURCES = \ secret/secret_driver.h secret/secret_driver.c +CONFIG_DRIVER_SOURCES = \ + config/config_driver.c config/config_driver.h + # Storage backend specific impls STORAGE_DRIVER_SOURCES = \ storage/storage_driver.h storage/storage_driver.c \ @@ -1386,6 +1391,25 @@ endif WITH_DRIVER_MODULES libvirt_driver_secret_la_SOURCES = $(SECRET_DRIVER_SOURCES) endif WITH_SECRETS +if WITH_CONFIG +if WITH_DRIVER_MODULES +mod_LTLIBRARIES += libvirt_driver_config.la +else ! WITH_DRIVER_MODULES +noinst_LTLIBRARIES += libvirt_driver_config.la +# Stateful, so linked to daemon instead +#libvirt_la_BUILT_LIBADD += libvirt_driver_config.la +endif ! WITH_DRIVER_MODULES +libvirt_driver_config_la_CFLAGS = \ + -I$(top_srcdir)/src/access \ + -I$(top_srcdir)/src/conf \ + $(AM_CFLAGS) +if WITH_DRIVER_MODULES +libvirt_driver_config_la_LIBADD = ../gnulib/lib/libgnu.la +libvirt_driver_config_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS) +endif WITH_DRIVER_MODULES +libvirt_driver_config_la_SOURCES = $(CONFIG_DRIVER_SOURCES) +endif WITH_CONFIG + # Needed to keep automake quiet about conditionals libvirt_driver_storage_impl_la_SOURCES = libvirt_driver_storage_impl_la_CFLAGS = \ @@ -1661,6 +1685,7 @@ EXTRA_DIST += \ $(SECURITY_DRIVER_SELINUX_SOURCES) \ $(SECURITY_DRIVER_APPARMOR_SOURCES) \ $(SECRET_DRIVER_SOURCES) \ + $(CONFIG_DRIVER_SOURCES) \ $(VBOX_DRIVER_EXTRA_DIST) \ $(VMWARE_DRIVER_SOURCES) \ $(XENXS_SOURCES) \ -- 1.8.5.2
On 12/21/13 05:04, Adam Walters wrote:
This completes the addition of the config driver to libvirt. The final piece here is to add the config driver into the Makefile (via automake) so that the driver is actually compiled and linked.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/Makefile.am | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
These changes look reasonable but they need to be before or merged with the previous patch. Peter
On 12/21/13 05:03, Adam Walters wrote:
This patchset adds a driver named 'config' that allows access to configuration data, such as secret and storage definitions. This is a pre-requisite for my next patchset which resolves the race condition on libvirtd startup and the circular dependencies between QEMU and the storage driver.
The basic rationale behind this idea is that there exist circumstances under which a driver may need to access things such as secrets during a time at which there is no active connection to a hypervisor. Without a connection, the data can't be accessed currently. I felt that this was a much simpler solution to the problem that building new APIs that do not require a connection to operate.
This driver is technically what one may call a hypervisor driver, but it does not implement any domain operations. It simply exists to handle requests by drivers for access to informatino that would otherwise require a connection. The URI used for this driver is 'config:///' and has been tested working on 4 different machines of mine, running three different distributions of Linux (Archlinux, Gentoo, and CentOS). Being a very simple driver, I would expect it to work pretty much anywhere.
I would love to hear any comments and suggestions you may have about this driver. At the very least this plus my next patchset resolves the startup race condition on my machine. If a more robust setup (likely a new internal API) is in the works, this driver could act as a band-aid to allow access to this type of data in the interim if a better resolution is a ways off.
Adam Walters (7): config: Adding source for the config driver config: Adding header for the config driver virterror: Adding a new VIR_FROM_ define libvirtd: Add config driver hooks po: Add config_driver.c to POTFILES.in configure: Add config driver to configure script Makefile: Add config driver to src/Makefile.am
configure.ac | 10 ++ daemon/libvirtd.c | 21 ++-- include/libvirt/virterror.h | 2 + po/POTFILES.in | 1 + src/Makefile.am | 25 +++++ src/config/config_driver.c | 237 ++++++++++++++++++++++++++++++++++++++++++++ src/config/config_driver.h | 44 ++++++++ src/util/virerror.c | 2 + 8 files changed, 336 insertions(+), 6 deletions(-) create mode 100644 src/config/config_driver.c create mode 100644 src/config/config_driver.h
In general the code looks sane. I'm looking forward to see a v2 of this series and I promise it won't take that long to review it as this time :). Peter
On Fri, Dec 20, 2013 at 11:03:53PM -0500, Adam Walters wrote:
This patchset adds a driver named 'config' that allows access to configuration data, such as secret and storage definitions. This is a pre-requisite for my next patchset which resolves the race condition on libvirtd startup and the circular dependencies between QEMU and the storage driver.
I vaguely recall something being mentioned in the past, but not the details. Can you explain the details of the circular dependancy problem that you're currently facing ?
The basic rationale behind this idea is that there exist circumstances under which a driver may need to access things such as secrets during a time at which there is no active connection to a hypervisor. Without a connection, the data can't be accessed currently. I felt that this was a much simpler solution to the problem that building new APIs that do not require a connection to operate.
We have a handful of places in our code where we call out to public APIs to implement some piece of functionality. I've never been all that happy about these scenarios. If we call the public API directly, they cause havoc with error reporting because we end up dispatching and resetting errors in the middle of an nested API call. Calling the driver methods directly by doing conn->driver->invokeMethod() is somewhat tedious & error prone because we're then skipping various sanity tests that the public APIs do. With ACL checking now implemented we also have the slightly odd situation that a public API check which is documented as requiring permission 'xxxx' may also require permissions 'yyyy' and 'zzzz' to deal with other public APIs we invoke secretly. I think there is a fairly strong argument that our internal implementations could be decoupled from the public APIs, so we can call methods internally without having to go via the public APIs at all. On the flip side though, there's also a long term desire to separate the different drivers into separate daemons, eg so the secrets driver might move into a libvirt-secretsd daemon, which might explicitly require use to go via the public APIs.
This driver is technically what one may call a hypervisor driver, but it does not implement any domain operations. It simply exists to handle requests by drivers for access to informatino that would otherwise require a connection. The URI used for this driver is 'config:///' and has been tested working on 4 different machines of mine, running three different distributions of Linux (Archlinux, Gentoo, and CentOS). Being a very simple driver, I would expect it to work pretty much anywhere.
I would love to hear any comments and suggestions you may have about this driver. At the very least this plus my next patchset resolves the startup race condition on my machine. If a more robust setup (likely a new internal API) is in the works, this driver could act as a band-aid to allow access to this type of data in the interim if a better resolution is a ways off.
One big concern I have about this approach is the fact that once we add this 'config:///' driver, it is very hard for us to ever remove it again, since this concept leaks out into the public API. So we're going to want to be very sure that this is something we wish to support long term, and I don't really have such confidence myself. I'd like to understand a bit more about the dependancy issue you're facing to see if there's something else we can do to address it. 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 :|
On 01/20/14 17:27, Daniel P. Berrange wrote:
On Fri, Dec 20, 2013 at 11:03:53PM -0500, Adam Walters wrote:
This patchset adds a driver named 'config' that allows access to configuration data, such as secret and storage definitions. This is a pre-requisite for my next patchset which resolves the race condition on libvirtd startup and the circular dependencies between QEMU and the storage driver.
I vaguely recall something being mentioned in the past, but not the details. Can you explain the details of the circular dependancy problem that you're currently facing ?
The problem is that when we are re-starting with running VMs that have storage placed on RBD volumes with secrets stored in the secret driver we encounter the following dependancy: 1) the qemu driver depends on the storage driver to be started so that disk type="volume" can be resolved to the actual volumes 2) the storage driver depends on requesting credentials from the secret driver and thus needs a connection to do so. For some strange reason the storage driver opens "qemu:///system" for this purpose: static void storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; virConnectPtr conn = NULL; /* XXX Remove hardcoding of QEMU URI */ if (driverState->privileged) conn = virConnectOpen("qemu:///system"); else conn = virConnectOpen("qemu:///session"); /* Ignoring NULL conn - let backends decide */ This works if the drivers are initialized, but breaks in the case we use RBD volumes with secrets that and <disk type=volume, where we need to initialize the storage driver before the qemu driver. Peter
On Mon, Jan 20, 2014 at 11:27 AM, Daniel P. Berrange <berrange@redhat.com>wrote:
This patchset adds a driver named 'config' that allows access to configuration data, such as secret and storage definitions. This is a pre-requisite for my next patchset which resolves the race condition on libvirtd startup and
On Fri, Dec 20, 2013 at 11:03:53PM -0500, Adam Walters wrote: the
circular dependencies between QEMU and the storage driver.
I vaguely recall something being mentioned in the past, but not the details. Can you explain the details of the circular dependancy problem that you're currently facing ?
Peter's description is nearly complete. I have actually reproduced the issue even with file-backed volumes. Only when using the disk type of volume, though. There had been a thread on the list late last year where I brought up the problem, but the discussion sort of died off. I then worked on the patches here to resolve the issue. This certainly isn't the only method of resolution, but merely the best one that I could think of and implement on my own. In more detail, there are actually two problems (and thus the two patchsets I submitted) that I discovered in libvirt. The first is the unpredictable driver load order. In this issue, all of the VMs I ran (all were QEMU-based, using either RBD- or file-backed storage pool volumes) were randomly restarted due to a race condition in libvirt. QEMU and the storage driver are both starting simultaneously. If QEMU manages to start before storage, the domains get restarted due to the storage pool being offline (they are all offline initially). The patchset that changes the libvirt driver loading code resolves this particular issue. Once I fixed this locally, I ran into the second problem below. The second issue is the circular dependency problem. This issue is only really visible once the driver load race condition is resolved. In this issue, the storage driver has a hard-coded connection to QEMU, as Peter mentioned in his reply. This is the only problem that the config driver actually resolves. Without this (or a similar patch to fix the inter-dependency between QEMU and storage), an errata would be needed to note that using secrets with storage pools is not to be used in production due to known issues. If I remember correctly, RBD-backed pools used to require authentication. If that has not been resolved (and I will be honest that I have not checked in a while since I use cephx authentication), then no RBD pools should be used for the time being, unless the user is okay with their VMs being restarted anytime libvirt restarts. It should be a rare event, but painful when not expected. My third patchset was merely a resubmit of my code to add support for RBD storage pools to the domain XML. Currently, you are able to define a RBD pool, but not actually use it. I didn't like having to duplicate my cephx username, key, and the ceph monitor hosts in all of my domain's XML, so I wrote the patch to fix it. I resubmitted it due to the length of time that had passed. The only change was that it had been rebased to the latest libvirt version.
The basic rationale behind this idea is that there exist circumstances under which a driver may need to access things such as secrets during a time at which there is no active connection to a hypervisor. Without a connection, the data can't be accessed currently. I felt that this was a much simpler solution to the problem that building new APIs that do not require a connection to operate.
We have a handful of places in our code where we call out to public APIs to implement some piece of functionality. I've never been all that happy about these scenarios. If we call the public API directly, they cause havoc with error reporting because we end up dispatching and resetting errors in the middle of an nested API call. Calling the driver methods directly by doing conn->driver->invokeMethod() is somewhat tedious & error prone because we're then skipping various sanity tests that the public APIs do. With ACL checking now implemented we also have the slightly odd situation that a public API check which is documented as requiring permission 'xxxx' may also require permissions 'yyyy' and 'zzzz' to deal with other public APIs we invoke secretly.
I think there is a fairly strong argument that our internal implementations could be decoupled from the public APIs, so we can call methods internally without having to go via the public APIs at all.
On the flip side though, there's also a long term desire to separate the different drivers into separate daemons, eg so the secrets driver might move into a libvirt-secretsd daemon, which might explicitly require use to go via the public APIs.
If there were an internal API that did not require a connection, that would probably be a better solution, at least as long as libvirt stays one monolithic process, anyway. In the specific problem I have run in to, the conn variable is NULL, due to no connection existing (or being able to exist). In this case, trying conn->driver->invokeMethod() would fail unless there is some magic in the code I'm not aware of.
This driver is technically what one may call a hypervisor driver, but it does not implement any domain operations. It simply exists to handle requests by drivers for access to informatino that would otherwise require a connection. The URI used for this driver is 'config:///' and has been tested working on 4 different machines of mine, running three different distributions of Linux (Archlinux, Gentoo, and CentOS). Being a very simple driver, I would expect it to work pretty much anywhere.
I would love to hear any comments and suggestions you may have about this driver. At the very least this plus my next patchset resolves the startup race condition on my machine. If a more robust setup (likely a new internal API) is in the works, this driver could act as a band-aid to allow access to this type of data in the interim if a better resolution is a ways off.
One big concern I have about this approach is the fact that once we add this 'config:///' driver, it is very hard for us to ever remove it again, since this concept leaks out into the public API. So we're going to want to be very sure that this is something we wish to support long term, and I don't really have such confidence myself.
I'd like to understand a bit more about the dependancy issue you're facing to see if there's something else we can do to address it.
I agree that adding a new connection driver would make it difficult to remove later. In fact, as I (think I) mentioned in my patch descriptions, adding the config driver is not required to resolve the race condition. It is (or some similar fix to the circular dependency between storage and qemu), however, needed in order to allow storage volumes (of any kind) to use secrets on running VMs during a libvirt restart. Without a fix for that issue, any VM using a volume with a secret will be killed and restarted upon a libvirt restart. In fact, if the driver load order is resolved, but the circular dependency is not, the restart no longer randomly reboots VMs on my system, it always does. Though, to be fair, in that case, it never reboots a VM with a file-backed volume once the race condition is removed. I've also never seen a VM get rebooted on a libvirt restart when not using storage pools, either. Unless I'm mistaken, though, it looks to me like storage pools are sort of the future in libvirt. It makes the configuration of a VM a lot simpler, since the custom stuff is held in the storage pool. Storage pools also enable management frontends to support new storage backends with little to no work, which is a big plus for any administrator. Currently, you're pretty much just stuck with a file-backed VM unless you want to hand-craft your XML.
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:|
On Mon, Jan 20, 2014 at 11:27 AM, Daniel P. Berrange <berrange@redhat.com>wrote:
This patchset adds a driver named 'config' that allows access to configuration data, such as secret and storage definitions. This is a pre-requisite for my next patchset which resolves the race condition on libvirtd startup and
On Fri, Dec 20, 2013 at 11:03:53PM -0500, Adam Walters wrote: the
circular dependencies between QEMU and the storage driver.
I vaguely recall something being mentioned in the past, but not the details. Can you explain the details of the circular dependancy problem that you're currently facing ?
The basic rationale behind this idea is that there exist circumstances under which a driver may need to access things such as secrets during a time at which there is no active connection to a hypervisor. Without a connection, the data can't be accessed currently. I felt that this was a much simpler solution to the problem that building new APIs that do not require a connection to operate.
We have a handful of places in our code where we call out to public APIs to implement some piece of functionality. I've never been all that happy about these scenarios. If we call the public API directly, they cause havoc with error reporting because we end up dispatching and resetting errors in the middle of an nested API call. Calling the driver methods directly by doing conn->driver->invokeMethod() is somewhat tedious & error prone because we're then skipping various sanity tests that the public APIs do. With ACL checking now implemented we also have the slightly odd situation that a public API check which is documented as requiring permission 'xxxx' may also require permissions 'yyyy' and 'zzzz' to deal with other public APIs we invoke secretly.
I think there is a fairly strong argument that our internal implementations could be decoupled from the public APIs, so we can call methods internally without having to go via the public APIs at all.
On the flip side though, there's also a long term desire to separate the different drivers into separate daemons, eg so the secrets driver might move into a libvirt-secretsd daemon, which might explicitly require use to go via the public APIs.
Mulling this over last night, I think there may be an alternative implementation that would be sustainable long-term. You mentioned a desire to split the drivers into separate daemons eventually... It might seem silly today, but what if I went ahead and implemented a connection URI for each of the existing drivers? This would result in a 'network:///', 'secrets:///', 'storage:///', etc. Once complete, the existing code base could slowly be updated to utilize connections to those new URIs in preparation for splitting the code out into daemons. This would end up with the current libvirtd process eventually becoming a connection broker, with a compatibility shim to allow access to the API as it is currently defined for backwards compatibility. In effect, today nothing would change, other than the storage driver would use 'secrets:///' to open a connection for its backends. Eventually, though, all drivers would use the new URIs (the actual APIs implemented would not need to change), allowing an easier split of the daemons. The compatibility shim I mentioned would just be some code to open a connection to the proper URI, make the request, close the connection, and return the data. The one thing that would be needed at some point (which I'm not sure how to really implement today), would be a restriction on which API functions could be called. Basically, if you opened a connection to 'secrets:///', you should only be able to access secrets. If you tried to access network information, an error would need to be thrown in order to prevent all of the drivers having a connection to all of the other drivers, burning up sockets on the host system. I'm guessing that this could be implemented using the ACL framework that is already in place, but I haven't dug through the code to verify this yet. In this long-term desire to split libvirt into multiple daemons, is there a plan to also create a libvirt-domaind? Currently, the various hypervisors don't actually know how to read the domain XML fully, so a daemon like that would probably be desired. As a bonus, it could also allow a single connection to view all running domains, regardless of which hypervisor driver actually created it, though that isn't strictly needed. The reason I ask is that currently, it is nigh impossible to determine driver inter-dependencies automatically. If strict controls were in place to prevent cross-URI calls (in effect making libvirt as a single process act as though it were already split), it should become easier to determine a driver's dependencies at compile (or possibly even run) time. Having that data would allow for a true dependency-based driver load order. While I think implementation of a dependency-aware driver loader is probably beyond my personal programming skills, it would be the best method in the long run. Though, in a multi-process model, it may be simpler to define a new connection function that waits (optionally until a timeout expires) for the connection to succeed, performing automatic retries in that time. This would allow the various daemons to start in any order, yet automatically initialize in the proper dependency order. Any circular dependency would, of course, cause a long hang during startup, but a timeout could detect that and fail the startup of libvirt as a whole. The same timeout-based connection function could also be used in the single-process model, of course, removing the need for driver load order to be changed at all if all existing code used the proposed new URIs. In effect, the driver load order change would be needed today, but once all existing code is updated, the change could be reverted to the current free-for-all model, as there would be implied control over that by virtue of the fact that the drivers would wait for their dependencies to finish connecting.
This driver is technically what one may call a hypervisor driver, but it does not implement any domain operations. It simply exists to handle requests by drivers for access to informatino that would otherwise require a connection. The URI used for this driver is 'config:///' and has been tested working on 4 different machines of mine, running three different distributions of Linux (Archlinux, Gentoo, and CentOS). Being a very simple driver, I would expect it to work pretty much anywhere.
I would love to hear any comments and suggestions you may have about this driver. At the very least this plus my next patchset resolves the startup race condition on my machine. If a more robust setup (likely a new internal API) is in the works, this driver could act as a band-aid to allow access to this type of data in the interim if a better resolution is a ways off.
One big concern I have about this approach is the fact that once we add this 'config:///' driver, it is very hard for us to ever remove it again, since this concept leaks out into the public API. So we're going to want to be very sure that this is something we wish to support long term, and I don't really have such confidence myself.
I'd like to understand a bit more about the dependancy issue you're facing to see if there's something else we can do to address it.
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:|
2014/1/22 Adam Walters <adam@pandorasboxen.com>:
On Mon, Jan 20, 2014 at 11:27 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Fri, Dec 20, 2013 at 11:03:53PM -0500, Adam Walters wrote:
This patchset adds a driver named 'config' that allows access to configuration data, such as secret and storage definitions. This is a pre-requisite for my next patchset which resolves the race condition on libvirtd startup and the circular dependencies between QEMU and the storage driver.
I vaguely recall something being mentioned in the past, but not the details. Can you explain the details of the circular dependancy problem that you're currently facing ?
The basic rationale behind this idea is that there exist circumstances under which a driver may need to access things such as secrets during a time at which there is no active connection to a hypervisor. Without a connection, the data can't be accessed currently. I felt that this was a much simpler solution to the problem that building new APIs that do not require a connection to operate.
We have a handful of places in our code where we call out to public APIs to implement some piece of functionality. I've never been all that happy about these scenarios. If we call the public API directly, they cause havoc with error reporting because we end up dispatching and resetting errors in the middle of an nested API call. Calling the driver methods directly by doing conn->driver->invokeMethod() is somewhat tedious & error prone because we're then skipping various sanity tests that the public APIs do. With ACL checking now implemented we also have the slightly odd situation that a public API check which is documented as requiring permission 'xxxx' may also require permissions 'yyyy' and 'zzzz' to deal with other public APIs we invoke secretly.
I think there is a fairly strong argument that our internal implementations could be decoupled from the public APIs, so we can call methods internally without having to go via the public APIs at all.
On the flip side though, there's also a long term desire to separate the different drivers into separate daemons, eg so the secrets driver might move into a libvirt-secretsd daemon, which might explicitly require use to go via the public APIs.
Mulling this over last night, I think there may be an alternative implementation that would be sustainable long-term. You mentioned a desire to split the drivers into separate daemons eventually... It might seem silly today, but what if I went ahead and implemented a connection URI for each of the existing drivers? This would result in a 'network:///', 'secrets:///', 'storage:///', etc. Once complete, the existing code base could slowly be updated to utilize connections to those new URIs in preparation for splitting the code out into daemons. This would end up with the current libvirtd process eventually becoming a connection broker, with a compatibility shim to allow access to the API as it is currently defined for backwards compatibility.
But keep in mind that there is not only one network, secrets or storage driver. For example, the ESX hypervisor driver comes with its own ESX specific storage and network subdriver. It's the same for the VirtualBox, HyperV ans Parallels hypervisor driver. Today those are bundled under the URI of their hypervisor drivers: esx://, vbox://, hyperv:// and parallels://. How does this workout with a URI per subdriver? -- Matthias Bolte http://photron.blogspot.com
On Wed, Jan 22, 2014 at 8:36 AM, Matthias Bolte < matthias.bolte@googlemail.com> wrote:
On Mon, Jan 20, 2014 at 11:27 AM, Daniel P. Berrange < berrange@redhat.com> wrote:
On Fri, Dec 20, 2013 at 11:03:53PM -0500, Adam Walters wrote:
This patchset adds a driver named 'config' that allows access to configuration data, such as secret and storage definitions. This is a pre-requisite for my next patchset which resolves the race condition on libvirtd startup
and
the circular dependencies between QEMU and the storage driver.
I vaguely recall something being mentioned in the past, but not the details. Can you explain the details of the circular dependancy problem that you're currently facing ?
The basic rationale behind this idea is that there exist circumstances under which a driver may need to access things such as secrets during a time at which there is no active connection to a hypervisor. Without a connection, the data can't be accessed currently. I felt that this was a much simpler solution to the problem that building new APIs that do not require a connection to operate.
We have a handful of places in our code where we call out to public APIs to implement some piece of functionality. I've never been all that happy about these scenarios. If we call the public API directly, they cause havoc with error reporting because we end up dispatching and resetting errors in the middle of an nested API call. Calling the driver methods directly by doing conn->driver->invokeMethod() is somewhat tedious & error prone because we're then skipping various sanity tests that the public APIs do. With ACL checking now implemented we also have the slightly odd situation that a public API check which is documented as requiring permission 'xxxx' may also require
'yyyy' and 'zzzz' to deal with other public APIs we invoke secretly.
I think there is a fairly strong argument that our internal implementations could be decoupled from the public APIs, so we can call methods internally without having to go via the public APIs at all.
On the flip side though, there's also a long term desire to separate the different drivers into separate daemons, eg so the secrets driver might move into a libvirt-secretsd daemon, which might explicitly require use to go via the public APIs.
Mulling this over last night, I think there may be an alternative implementation that would be sustainable long-term. You mentioned a desire to split the drivers into separate daemons eventually... It might seem silly today, but what if I went ahead and implemented a connection URI for each of the existing drivers? This would result in a 'network:///', 'secrets:///', 'storage:///', etc. Once complete, the existing code base could slowly be updated to utilize connections to
2014/1/22 Adam Walters <adam@pandorasboxen.com>: permissions those
new URIs in preparation for splitting the code out into daemons. This would end up with the current libvirtd process eventually becoming a connection broker, with a compatibility shim to allow access to the API as it is currently defined for backwards compatibility.
But keep in mind that there is not only one network, secrets or storage driver. For example, the ESX hypervisor driver comes with its own ESX specific storage and network subdriver. It's the same for the VirtualBox, HyperV ans Parallels hypervisor driver. Today those are bundled under the URI of their hypervisor drivers: esx://, vbox://, hyperv:// and parallels://. How does this workout with a URI per subdriver?
Honestly, I don't know how that would work out. To be perfectly honest, I have not played around with any of the hypervisor drivers other than the QEMU and LXC drivers, which both use the libvirt generic storage and network drivers. Mainly, my thoughts were to suggest a possible alternative to 'config:///' driver, which Daniel had pointed out may not be a good long-term solution to the problem. The generic 'config:///' driver fixes the immediate need, but would not really make sense if and when libvirt gets split into multiple daemons. At that point, there will likely be a need for some form (possibly internal-only) of connection URI for each daemon. Though, if the ESX driver, for example, uses its own network driver, can you list the ESX networks through the libvirt net-list command? If not, then a change like that shouldn't really affect them, at least in theory. If you can list them, then perhaps an architecture there you have a few more daemons may fix it. Something like 'libvirt-esx-bridged' would handle the libvirt<->esx bridge. Then you could move the code from the esx driver that handles the networking over to the network driver. Similar theory of operation for other drivers. The bridge daemon would not have any dependencies, and thus could start before anything else. A setup like this would cause libvirt to expand into a lot of different processes, though, so I don't know if that would work out well, either. Perhaps for drivers like this, the 'libvirt-esxd' process could run two threads. One is a communication bridge, while the other could be the hypervisor driver. Basically just thinking out loud here, but not coming up with a whole lot of options to fix the circular dependencies currently present while also providing an easy migration path going forward to allow splitting libvirt into multiple processes while simultaneously not muddying the public API in the short-term. What would make that easier would be a method to make the 'config:///' URI only usable from explicitly allowed locations (in this case only from within the storage driver). That would allow the code I wrote to solve the current problem without really muddying the public APIs, since there would be some measure of control over who could utilize the new driver. Any additional use would require a patch to libvirt to enable it, thus prompting discussion over why the access is needed at all.
-- Matthias Bolte http://photron.blogspot.com
participants (4)
-
Adam Walters -
Daniel P. Berrange -
Matthias Bolte -
Peter Krempa