On 6/25/21 4:53 AM, Michal Prívozník wrote:
On 5/28/21 8:30 PM, Jim Fehlig wrote:
Hi All!
I received a bug report about virtlockd emitting an error whenever
libvirtd is (re)started
May 25 15:44:31 virt81 virtlockd[7723]: Requested operation is not
valid: Lockspace for path /data/libvirtd/lockspace already exists
The problem is easily reproducible with git master by enabling lockd in
qemu.conf, setting file_lockspace_dir in qemu-lockd.conf, then
restarting libvirtd.
If I understand the code correctly, when the qemu driver loads, it calls
virLockManagerPluginNew, which dlopens lockd.so and calls drvInit, aka
virLockManagerLockDaemonInit. Here the driver object is created, config
loaded, and virLockManagerLockDaemonSetupLockspace is called.
virLockManagerLockDaemonSetupLockspace sends
virLockSpaceProtocolCreateLockSpaceArgs rpc to virtlockd, where it is
dispatched to virLockSpaceProtocolDispatchCreateLockSpace. Alas we
encounter the error when virLockDaemonFindLockSpace finds the existing
lockspace.
I'm not really sure how to go about fixing it and fishing for opinions.
virLockManagerLockDaemonSetupLockspace already has some code to handle
the error
https://gitlab.com/libvirt/libvirt/-/blob/master/src/locking/lock_driver_lockd.c#L286
Since libvirtd ignores VIR_ERR_OPERATION_INVALID, should virtlockd be
changed to not return error in that case? It would be better if libvirtd
knew it already told virtlockd to configure the lockspace and avoid
needlessly doing it again.
Yeah, that would be the best. But I'm not sure we have a reliable way to
store that info. I mean, what if I'd shut down libvirtd, then virtlockd
and then 'rm -rf $lockDir'.
Fortunately, the RPC between virtlockd and libvirtd is considered
internal enough that we can change it. We have guaranteed by the
specfile that the virtlockd and libvirtd will be the same version.
Therefore, I think we can add a new flag "hey, it's okay if the
lockspace exists, no need to report error".
I explored this option (see attached patch) but when testing realized whatever
is specified in file_lockspace_dir must be created by the user anyhow. If not,
VMs fail to start
# virsh start test
error: Failed to start domain 'test'
error: Unable to open/create resource
/data/libvirtd/lockspace/de22c4bf931e7c48b49e8ca64b477d44e78a51543e534df488b05ccd08ec5caa:
No such file or directory
Since the user must create the lockspace, it will always exist :-).
Maybe we can change virtlockd to not report error if the lockspace
exists even without the flag.
Callers are not interested in the error, so I took this approach
https://listman.redhat.com/archives/libvir-list/2021-June/msg00841.html
Regards,
Jim
>From 12d2631f96340ebc677d6e93b076fd9be8b00aa7 Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig@xxxxxxxx>
Date: Wed, 30 Jun 2021 15:48:42 -0600
Subject: [PATCH] locking: Add flag to ingore existing lockspace when creating
lock manager
Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
---
src/libxl/libxl_driver.c | 2 +-
src/lock_protocol-structs | 1 +
src/locking/lock_daemon_dispatch.c | 17 +++++++++++++----
src/locking/lock_driver.h | 4 +++-
src/locking/lock_driver_lockd.c | 13 ++++++++-----
src/locking/lock_driver_lockd.h | 4 ++++
src/locking/lock_protocol.x | 1 +
src/qemu/qemu_driver.c | 2 +-
8 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index c97b2fb485..af0daa0361 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -759,7 +759,7 @@ libxlStateInitialize(bool privileged,
cfg->lockManagerName : "nop",
"libxl",
cfg->configBaseDir,
- 0)))
+ VIR_LOCK_MANAGER_IGNORE_EXISTING_LOCKSPACE_ERROR)))
goto error;
/* read the host sysinfo */
diff --git a/src/lock_protocol-structs b/src/lock_protocol-structs
index 41be9ce347..bca752a258 100644
--- a/src/lock_protocol-structs
+++ b/src/lock_protocol-structs
@@ -38,6 +38,7 @@ struct virLockSpaceProtocolReleaseResourceArgs {
};
struct virLockSpaceProtocolCreateLockSpaceArgs {
virLockSpaceProtocolNonNullString path;
+ u_int flags;
};
enum virLockSpaceProtocolProcedure {
VIR_LOCK_SPACE_PROTOCOL_PROC_REGISTER = 1,
diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
index e8b9832453..97ae088f61 100644
--- a/src/locking/lock_daemon_dispatch.c
+++ b/src/locking/lock_daemon_dispatch.c
@@ -392,12 +392,16 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServer *server G_GNUC_UNUSED,
virLockSpaceProtocolCreateLockSpaceArgs *args)
{
int rv = -1;
+ unsigned int flags = args->flags;
virLockDaemonClient *priv =
virNetServerClientGetPrivateData(client);
virLockSpace *lockspace;
g_mutex_lock(&priv->lock);
+ virCheckFlagsGoto(VIR_LOCK_SPACE_PROTOCOL_CREATE_LOCKSPACE_IGNORE_EXISTING_RESOURCE,
+ cleanup);
+
if (priv->restricted) {
virReportError(VIR_ERR_OPERATION_DENIED, "%s",
_("lock manager connection has been restricted"));
@@ -405,10 +409,15 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServer *server G_GNUC_UNUSED,
}
if (virLockDaemonFindLockSpace(lockDaemon, args->path) != NULL) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("Lockspace for path %s already exists"),
- args->path);
- goto cleanup;
+ if (flags & VIR_LOCK_SPACE_PROTOCOL_CREATE_LOCKSPACE_IGNORE_EXISTING_RESOURCE) {
+ rv = 0;
+ goto cleanup;
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("Lockspace for path %s already exists"),
+ args->path);
+ goto cleanup;
+ }
}
if (!(lockspace = virLockSpaceNew(args->path)))
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index 087a918882..37266c8251 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -32,7 +32,9 @@ typedef struct _virLockManagerParam virLockManagerParam;
typedef enum {
/* State passing is used to re-acquire existing leases */
- VIR_LOCK_MANAGER_USES_STATE = (1 << 0)
+ VIR_LOCK_MANAGER_USES_STATE = (1 << 0),
+ /* Ignore existing lockspace errors on daemon init */
+ VIR_LOCK_MANAGER_IGNORE_EXISTING_LOCKSPACE_ERROR = (1 << 1),
} virLockManagerFlags;
typedef enum {
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 3a7386af30..115dfb2044 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -261,7 +261,8 @@ virLockManagerLockDaemonConnect(virLockManager *lock,
}
-static int virLockManagerLockDaemonSetupLockspace(const char *path)
+static int virLockManagerLockDaemonSetupLockspace(const char *path,
+ unsigned int flags)
{
virNetClient *client;
virNetClientProgram *program = NULL;
@@ -271,6 +272,8 @@ static int virLockManagerLockDaemonSetupLockspace(const char *path)
memset(&args, 0, sizeof(args));
args.path = (char*)path;
+ if (flags & VIR_LOCK_MANAGER_IGNORE_EXISTING_LOCKSPACE_ERROR)
+ args.flags = VIR_LOCK_SPACE_PROTOCOL_CREATE_LOCKSPACE_IGNORE_EXISTING_RESOURCE;
if (!(client = virLockManagerLockDaemonConnectionNew(geteuid() == 0, &program)))
return -1;
@@ -309,7 +312,7 @@ static int virLockManagerLockDaemonInit(unsigned int version,
{
VIR_DEBUG("version=%u configFile=%s flags=0x%x", version, NULLSTR(configFile), flags);
- virCheckFlags(0, -1);
+ virCheckFlags(VIR_LOCK_MANAGER_IGNORE_EXISTING_LOCKSPACE_ERROR, -1);
if (driver)
return 0;
@@ -324,15 +327,15 @@ static int virLockManagerLockDaemonInit(unsigned int version,
if (driver->autoDiskLease) {
if (driver->fileLockSpaceDir &&
- virLockManagerLockDaemonSetupLockspace(driver->fileLockSpaceDir) < 0)
+ virLockManagerLockDaemonSetupLockspace(driver->fileLockSpaceDir, flags) < 0)
goto error;
if (driver->lvmLockSpaceDir &&
- virLockManagerLockDaemonSetupLockspace(driver->lvmLockSpaceDir) < 0)
+ virLockManagerLockDaemonSetupLockspace(driver->lvmLockSpaceDir, flags) < 0)
goto error;
if (driver->scsiLockSpaceDir &&
- virLockManagerLockDaemonSetupLockspace(driver->scsiLockSpaceDir) < 0)
+ virLockManagerLockDaemonSetupLockspace(driver->scsiLockSpaceDir, flags) < 0)
goto error;
}
diff --git a/src/locking/lock_driver_lockd.h b/src/locking/lock_driver_lockd.h
index b9eaab4831..9457b95efc 100644
--- a/src/locking/lock_driver_lockd.h
+++ b/src/locking/lock_driver_lockd.h
@@ -25,3 +25,7 @@ enum virLockSpaceProtocolAcquireResourceFlags {
VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED = (1 << 0),
VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = (1 << 1),
};
+
+enum virLockSpaceProtocolCreateLockSpaceFlags {
+ VIR_LOCK_SPACE_PROTOCOL_CREATE_LOCKSPACE_IGNORE_EXISTING_RESOURCE = (1 << 0),
+};
diff --git a/src/locking/lock_protocol.x b/src/locking/lock_protocol.x
index 6d4cec39e2..a49080de98 100644
--- a/src/locking/lock_protocol.x
+++ b/src/locking/lock_protocol.x
@@ -65,6 +65,7 @@ struct virLockSpaceProtocolReleaseResourceArgs {
struct virLockSpaceProtocolCreateLockSpaceArgs {
virLockSpaceProtocolNonNullString path;
+ unsigned int flags;
};
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 235f575901..2ced052abd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -711,7 +711,7 @@ qemuStateInitialize(bool privileged,
cfg->lockManagerName : "nop",
"qemu",
cfg->configBaseDir,
- 0)))
+ VIR_LOCK_MANAGER_IGNORE_EXISTING_LOCKSPACE_ERROR)))
goto error;
if (cfg->macFilter) {
--
2.31.1
