Avoiding lockspace already exists error from virtlockd

[Jim Fehlig]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-locking-Add-flag-to-ingore-existing-lockspace-when-c.patch
Type: text/x-patch
Size: 7636 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210630/743bb12e/attachment-0001.bin>