Re: [PATCH v2] vz: handle gracefully races on undefining domain

[Maxim Nestratov <mnestratov@xxxxxxxxxxxxx>]

21-Jun-16 11:35, Nikolay Shirokovskiy пишет:

>    This patch is not critical but nice to have. The original motivation
> was error message in logs on undefining domain thru vz driver.
> Undefine procedure drops domain lock while waiting for detaching
> disks vz sdk call. Meanwhile vz sdk event domain-config-changed
> arrives, its handler finds domain and is blocked waiting for job
> condition. After undefine API call finishes event processing procedes
> and tries to refreshes domain config thru existing vz sdk domain handle.
> Domain does not exists anymore and event processing fails. Everything
> is fine we just don't want to see error message in log for this
> particular case.
>
>    Fortunately domain has flag that domain is removed from list. This
> also imply that vz sdk domain is also undefined. Thus if we check
> for this flag right after domain is locked again on accuiring
> job condition we gracefully handle this situation.
>
>    Actually the race can happen in other situations too. Any
> time we wait for job condition in mutualy exclusive job in
> time when we acquire it vz sdk domain can cease to exist.
> So instead of general internal error we can return domain
> not found which is easier to handle. We don't need to patch
> other places in mutually exclusive jobs where domain lock
> is dropped as if job is started domain can't be undefine
> by mutually exclusive undefine job.
>
>    The code of this patch is quite similar to qemu driver checks
> for is domain is active after acquiring a job. The difference
> only while qemu domain is operational while process is active
> vz domain is operational while domain exists.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
> ---
>
> Changes from v1:
> 1. drop changes for API calls that do not accquire job condition.
> 2. fix prlsdkDomainChangeState to report error properly.
>
>   src/vz/vz_driver.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>   src/vz/vz_sdk.c    | 13 +++++++++++++
>   2 files changed, 59 insertions(+)
>
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index 1569278..711a354 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -716,6 +716,22 @@ vzDomainGetAutostart(virDomainPtr domain, int *autostart)
>       return 0;
>   }
>   
> +static bool
> +vzDomainObjIsExist(virDomainObjPtr dom)
> +{
> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> +    if (!dom->removing)
> +        return true;
> +
> +    virUUIDFormat(dom->def->uuid, uuidstr);
> +    virReportError(VIR_ERR_NO_DOMAIN,
> +                   _("no domain with matching uuid '%s' (%s)"),
> +                   uuidstr, dom->def->name);
> +
> +    return false;
> +}
> +

I think this function is misleading as it sets error while a caller 
could only ask if there is a domain.
Thus the usage of the functions implies that it should be called only 
when a caller doesn't expect a domain to be absent. Maybe we then should 
rename it to vzEnsureDomainExists ?

>   static virDomainPtr
>   vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
>   {
> @@ -782,6 +798,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags)
>                   goto cleanup;
>               job = true;
>   
> +            if (!vzDomainObjIsExist(dom))
> +                goto cleanup;
> +
>               if (prlsdkApplyConfig(driver, dom, def))
>                   goto cleanup;
>   
> @@ -1012,6 +1031,9 @@ vzDomainUndefineFlags(virDomainPtr domain,
>           goto cleanup;
>       job = true;
>   
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
>       ret = prlsdkUnregisterDomain(privconn->driver, dom, flags);
>   
>    cleanup:
> @@ -1070,6 +1092,9 @@ vzDomainManagedSave(virDomainPtr domain, unsigned int flags)
>           goto cleanup;
>       job = true;
>   
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
>       state = virDomainObjGetState(dom, &reason);
>   
>       if (state == VIR_DOMAIN_RUNNING && (flags & VIR_DOMAIN_SAVE_PAUSED)) {
> @@ -1160,6 +1185,9 @@ static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml,
>           goto cleanup;
>       job = true;
>   
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
>       switch (dev->type) {
>       case VIR_DOMAIN_DEVICE_DISK:
>           ret = prlsdkAttachVolume(privconn->driver, dom, dev->data.disk);
> @@ -1228,6 +1256,9 @@ static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml,
>           goto cleanup;
>       job = true;
>   
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
>       switch (dev->type) {
>       case VIR_DOMAIN_DEVICE_DISK:
>           ret = prlsdkDetachVolume(dom, dev->data.disk);
> @@ -1285,6 +1316,9 @@ vzDomainSetUserPassword(virDomainPtr domain,
>           goto cleanup;
>       job = true;
>   
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
>       ret = prlsdkDomainSetUserPassword(dom, user, password);
>   
>    cleanup:
> @@ -1618,6 +1652,9 @@ static int vzDomainSetMemoryFlagsImpl(virDomainPtr domain, unsigned long memory,
>           goto cleanup;
>       job = true;
>   
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
>       ret = prlsdkSetMemsize(dom, memory >> 10);
>   
>    cleanup:
> @@ -2102,6 +2139,9 @@ vzDomainSnapshotCreateXML(virDomainPtr domain,
>           goto cleanup;
>       job = true;
>   
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
>       /* snaphot name is ignored, it will be set to auto generated by sdk uuid */
>       if (prlsdkCreateSnapshot(dom, def->description) < 0)
>           goto cleanup;
> @@ -2163,6 +2203,9 @@ vzDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags)
>           goto cleanup;
>       job = true;
>   
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
>       ret = prlsdkSwitchToSnapshot(dom, snapshot->name,
>                                    flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED);
>    cleanup:
> @@ -2533,6 +2576,9 @@ vzDomainMigratePerformStep(virDomainPtr domain,
>           goto cleanup;
>       job = true;
>   
> +    if (!vzDomainObjIsExist(dom))
> +        goto cleanup;
> +
>       if (!(vzuri = vzParseVzURI(miguri)))
>           goto cleanup;
>   
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 551df10..ba3c28e 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -1812,6 +1812,9 @@ prlsdkHandleVmConfigEvent(vzDriverPtr driver,
>           goto cleanup;
>       job = true;
>   
> +    if (dom->removing)
> +        goto cleanup;
> +
>       if (prlsdkUpdateDomain(driver, dom) < 0)
>           goto cleanup;
>   
> @@ -2078,6 +2081,16 @@ prlsdkDomainChangeState(virDomainPtr domain,
>           goto cleanup;
>       job = true;
>   
> +    if (dom->removing) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> +        virUUIDFormat(dom->def->uuid, uuidstr);
> +        virReportError(VIR_ERR_NO_DOMAIN,
> +                       _("no domain with matching uuid '%s' (%s)"),
> +                       uuidstr, dom->def->name);
> +        goto cleanup;
> +    }
> +
>       ret = prlsdkDomainChangeStateLocked(privconn->driver, dom, chstate);
>   
>    cleanup:

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list