[libvirt] [PATCH] sanlock: Don't spam logs with "target pid not found"
Commit v1.2.4-52-gda879e5 fixed issues with domains started before sanlock driver was enabled by checking whether a running domain is registered with sanlock and if it's not, sanlock driver is basically ignored for the domain. However, it was checking this even for domain which has just been started and no sanlock_* API was called for them yet. This results in cmd 9 target pid 2135544 not found error messages to appear in sanlock.log whenever we start a new domain. This patch avoids this useless check for freshly started domains. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/locking/domain_lock.c | 22 ++++++++++++---------- src/locking/lock_driver.h | 12 +++++++++++- src/locking/lock_driver_lockd.c | 2 +- src/locking/lock_driver_sanlock.c | 9 ++++++--- src/locking/lock_manager.c | 2 +- 5 files changed, 31 insertions(+), 16 deletions(-) diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index d7b681e..705b132 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -104,7 +104,8 @@ static int virDomainLockManagerAddImage(virLockManagerPtr lock, static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, const char *uri, virDomainObjPtr dom, - bool withResources) + bool withResources, + unsigned int flags) { virLockManagerPtr lock; size_t i; @@ -138,7 +139,7 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, ARRAY_CARDINALITY(params), params, - 0))) + flags))) goto error; if (withResources) { @@ -177,7 +178,8 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p paused=%d fd=%p", plugin, dom, paused, fd); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, + VIR_LOCK_MANAGER_NEW_STARTED))) return -1; if (paused) @@ -201,7 +203,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, 0))) return -1; ret = virLockManagerRelease(lock, state, 0); @@ -221,7 +223,7 @@ int virDomainLockProcessResume(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%s", plugin, dom, NULLSTR(state)); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, 0))) return -1; ret = virLockManagerAcquire(lock, state, 0, dom->def->onLockFailure, NULL); @@ -240,7 +242,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, 0))) return -1; ret = virLockManagerInquire(lock, state, 0); @@ -260,7 +262,7 @@ int virDomainLockImageAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, 0))) return -1; if (virDomainLockManagerAddImage(lock, src) < 0) @@ -297,7 +299,7 @@ int virDomainLockImageDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, 0))) return -1; if (virDomainLockManagerAddImage(lock, src) < 0) @@ -334,7 +336,7 @@ int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, 0))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) @@ -362,7 +364,7 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, 0))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index a7d9782..f8fd38e 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -65,6 +65,11 @@ typedef enum { VIR_LOCK_MANAGER_ACQUIRE_RESTRICT = (1 << 1), } virLockManagerAcquireFlags; +typedef enum { + /* virLockManagerNew called for a freshly started domain */ + VIR_LOCK_MANAGER_NEW_STARTED = (1 << 0), +} virLockManagerNewFlags; + enum { VIR_LOCK_MANAGER_PARAM_TYPE_STRING, VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, @@ -142,13 +147,18 @@ typedef int (*virLockDriverDeinit)(void); * @type: the type of process to be supervised * @nparams: number of metadata parameters * @params: extra metadata parameters - * @flags: optional flags, currently unused + * @flags: bitwise-OR of virLockManagerNewFlags * * Initialize a new context to supervise a process, usually * a virtual machine. The lock driver implementation can use * the <code>privateData</code> field of <code>man</code> * to store a pointer to any driver specific state. * + * If @flags contains VIR_LOCK_MANAGER_NEW_STARTED, this API is called for + * a domain that has just been started and may therefore skip some actions. + * Specifically, checking whether the domain is registered with a lock + * daemon is useless in this case. + * * A process of VIR_LOCK_MANAGER_START_DOMAIN will be * given the following parameters * diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 8d184fe..72a4a0c 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -439,7 +439,7 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, virLockManagerLockDaemonPrivatePtr priv; size_t i; - virCheckFlags(VIR_LOCK_MANAGER_USES_STATE, -1); + virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1); if (VIR_ALLOC(priv) < 0) return -1; diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 60f305c..dbe79bc 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -455,7 +455,7 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, size_t i; int resCount = 0; - virCheckFlags(0, -1); + virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1); if (!driver) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -497,8 +497,11 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, * if it returns any other error (rv < 0), then we cannot fail due * to back-compat. So this whole call is non-fatal, because it's * called from all over the place (it will usually fail). It merely - * updates privateData. */ - if (sanlock_inquire(-1, priv->vm_pid, 0, &resCount, NULL) >= 0) + * updates privateData. + * If the process has just been started, we are pretty sure it is not + * registered. */ + if (!(flags & VIR_LOCK_MANAGER_NEW_STARTED) && + sanlock_inquire(-1, priv->vm_pid, 0, &resCount, NULL) >= 0) priv->registered = true; lock->privateData = priv; diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index f277f22..a002ea8 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -287,7 +287,7 @@ virLockDriverPtr virLockManagerPluginGetDriver(virLockManagerPluginPtr plugin) * virLockManagerNew: * @driver: the lock manager implementation to use * @type: the type of process to be supervised - * @flags: optional flags, currently unused + * @flags: bitwise-OR of virLockManagerNewFlags * * Create a new context to supervise a process, usually * a virtual machine. -- 2.3.1
On Fri, Mar 06, 2015 at 16:58:22 +0100, Jiri Denemark wrote:
Commit v1.2.4-52-gda879e5 fixed issues with domains started before sanlock driver was enabled by checking whether a running domain is registered with sanlock and if it's not, sanlock driver is basically ignored for the domain.
However, it was checking this even for domain which has just been started and no sanlock_* API was called for them yet. This results in
cmd 9 target pid 2135544 not found
error messages to appear in sanlock.log whenever we start a new domain.
This patch avoids this useless check for freshly started domains.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/locking/domain_lock.c | 22 ++++++++++++---------- src/locking/lock_driver.h | 12 +++++++++++- src/locking/lock_driver_lockd.c | 2 +- src/locking/lock_driver_sanlock.c | 9 ++++++--- src/locking/lock_manager.c | 2 +- 5 files changed, 31 insertions(+), 16 deletions(-) ... diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 8d184fe..72a4a0c 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -439,7 +439,7 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, virLockManagerLockDaemonPrivatePtr priv; size_t i;
- virCheckFlags(VIR_LOCK_MANAGER_USES_STATE, -1); + virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1);
if (VIR_ALLOC(priv) < 0) return -1;
Confused with this weird change? So am I :-) The VIR_LOCK_MANAGER_USES_STATE should really have been 0 (as in the sanlock driver) since virLockManagerNew is never called with such a flag. However, should VIR_LOCK_MANAGER_USES_STATE be set to virLockDriverImpl.flags in src/locking/lock_driver_lockd.c, which should be the only usage of this flag in any lock driver? Sanlock sets the flag but should lock set it too or not? Jirka
On Fri, Mar 06, 2015 at 05:03:16PM +0100, Jiri Denemark wrote:
On Fri, Mar 06, 2015 at 16:58:22 +0100, Jiri Denemark wrote:
Commit v1.2.4-52-gda879e5 fixed issues with domains started before sanlock driver was enabled by checking whether a running domain is registered with sanlock and if it's not, sanlock driver is basically ignored for the domain.
However, it was checking this even for domain which has just been started and no sanlock_* API was called for them yet. This results in
cmd 9 target pid 2135544 not found
error messages to appear in sanlock.log whenever we start a new domain.
This patch avoids this useless check for freshly started domains.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/locking/domain_lock.c | 22 ++++++++++++---------- src/locking/lock_driver.h | 12 +++++++++++- src/locking/lock_driver_lockd.c | 2 +- src/locking/lock_driver_sanlock.c | 9 ++++++--- src/locking/lock_manager.c | 2 +- 5 files changed, 31 insertions(+), 16 deletions(-) ... diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 8d184fe..72a4a0c 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -439,7 +439,7 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, virLockManagerLockDaemonPrivatePtr priv; size_t i;
- virCheckFlags(VIR_LOCK_MANAGER_USES_STATE, -1); + virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1);
if (VIR_ALLOC(priv) < 0) return -1;
Confused with this weird change? So am I :-) The VIR_LOCK_MANAGER_USES_STATE should really have been 0 (as in the sanlock driver) since virLockManagerNew is never called with such a flag.
However, should VIR_LOCK_MANAGER_USES_STATE be set to virLockDriverImpl.flags in src/locking/lock_driver_lockd.c, which should be the only usage of this flag in any lock driver? Sanlock sets the flag but should lock set it too or not?
The "state" here is information about the locks held on the source host which may need to be transferred to the target host during migration. Sanlock uses this facility to transfer lease information that needs to be maintained for its disk paxos algorithm. The lockd impl has no state information that needs to be transferred, so it should never references the VIR_LOCK_MANAGER_USES_STATE flag at all. IOW your change here is fine - if I wanted to nitpick though I'd say you should remove VIR_LOCK_MANAGER_USES_STATE from this method in a separate patch to this, just to avoid confusing future reviewers. Regards, 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 Fri, Mar 06, 2015 at 04:58:22PM +0100, Jiri Denemark wrote:
Commit v1.2.4-52-gda879e5 fixed issues with domains started before sanlock driver was enabled by checking whether a running domain is registered with sanlock and if it's not, sanlock driver is basically ignored for the domain.
However, it was checking this even for domain which has just been started and no sanlock_* API was called for them yet. This results in
cmd 9 target pid 2135544 not found
error messages to appear in sanlock.log whenever we start a new domain.
This patch avoids this useless check for freshly started domains.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/locking/domain_lock.c | 22 ++++++++++++---------- src/locking/lock_driver.h | 12 +++++++++++- src/locking/lock_driver_lockd.c | 2 +- src/locking/lock_driver_sanlock.c | 9 ++++++--- src/locking/lock_manager.c | 2 +- 5 files changed, 31 insertions(+), 16 deletions(-)
ACK, this makes sense. Regards, 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, Mar 09, 2015 at 12:31:02 +0000, Daniel P. Berrange wrote:
On Fri, Mar 06, 2015 at 04:58:22PM +0100, Jiri Denemark wrote:
Commit v1.2.4-52-gda879e5 fixed issues with domains started before sanlock driver was enabled by checking whether a running domain is registered with sanlock and if it's not, sanlock driver is basically ignored for the domain.
However, it was checking this even for domain which has just been started and no sanlock_* API was called for them yet. This results in
cmd 9 target pid 2135544 not found
error messages to appear in sanlock.log whenever we start a new domain.
This patch avoids this useless check for freshly started domains.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/locking/domain_lock.c | 22 ++++++++++++---------- src/locking/lock_driver.h | 12 +++++++++++- src/locking/lock_driver_lockd.c | 2 +- src/locking/lock_driver_sanlock.c | 9 ++++++--- src/locking/lock_manager.c | 2 +- 5 files changed, 31 insertions(+), 16 deletions(-)
ACK, this makes sense.
Thanks, I separated the change in flags into a new patch and pushed both. Jirka
participants (2)
-
Daniel P. Berrange -
Jiri Denemark