[libvirt] [PATCH 3/8] storage: Prior to creating a volume, refresh the pool

[John Ferlan]

On 10/07/2015 10:27 AM, Ján Tomko wrote:
> On Mon, Oct 05, 2015 at 07:27:34PM -0400, John Ferlan wrote:
>>
>>
>> On 10/05/2015 07:28 AM, Michal Privoznik wrote:
>>> On 02.10.2015 15:41, John Ferlan wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1233003
>>>>
>>>> Although perhaps bordering on a don't do that type scenario, if
>>>> someone creates a volume in a pool outside of libvirt, then uses that
>>>> same name to create a volume in the pool via libvirt, then the creation
>>>> will fail and in some cases cause the same name volume to be deleted.
>>>>
>>>> This patch will refresh the pool just prior to checking whether the
>>>> named volume exists prior to creating the volume in the pool. While
>>>> it's still possible to have a timing window to create a file after the
>>>> check - at least we tried.  At that point, someone is being malicious.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>>> ---
>>>>  src/storage/storage_driver.c | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>>>> index 7aaa060..ddf4405 100644
>>>> --- a/src/storage/storage_driver.c
>>>> +++ b/src/storage/storage_driver.c
>>>> @@ -1796,6 +1796,15 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>>>      if (virStorageVolCreateXMLEnsureACL(obj->conn, pool->def, voldef) < 0)
>>>>          goto cleanup;
>>>>  
>>>> +    /* While not perfect, refresh the list of volumes in the pool and
>>>> +     * then check that the incoming name isn't already in the pool.
>>>> +     */
>>>> +    if (backend->refreshPool) {
>>>> +        virStoragePoolObjClearVols(pool);
>>>> +        if (backend->refreshPool(obj->conn, pool) < 0)
>>>> +            goto cleanup;
>>>> +    }
>>>> +
>>>>      if (virStorageVolDefFindByName(pool, voldef->name)) {
>>>>          virReportError(VIR_ERR_STORAGE_VOL_EXIST,
>>>>                         _("'%s'"), voldef->name);
>>>>
>>>
> 
> This is a workaround for the linked bug, not a fix. Refreshing the pool
> seems excessive to me.
> 

Initially I felt the same way, but it's not beyond the realm of
possibility that someone created a volume/file of the same name outside
of libvirt and the only way we'd know is when/if create fails later on.

The other advantage is refreshing will get the "sizes" a well and now
I'm wondering if this should be done perhaps earlier.  What would happen
if you had a 10G pool with 5G available.  Someone outside of libvirt
creates a 5G file (or other size)... Then we try to create another file
in the pool of say 1G.  After creation, wouldn't our pool technically
have negative capacity since there's a refreshVol done which adjusts the
pool alloc/capacity. Eventually a refreshPool may discover that - I
haven't tested that theory yet though.

>>> Okay, this makes sense. Not only from the POV you are presenting, but I'd expect my pool to be refreshed after I create new volume in it.
>>>
> 
> Other volume APIs don't refresh the pool, I think we should be
> consistent.
> 

The API's are createVol, buildVol, buildVolFrom, deleteVol, resizeVol,
uploadVol, downloadVol, and wipeVol.

Other than perhaps resizeVol, it doesn't seem upload, download, or wipe
would need to update the list of volumes in the pool. Although, sure one
could make the argument that I could create volume "foo" outside libvirt
and then use one of those API's and they'd fail until someone refreshed.

I think create & build are a bit "special" - I'd rather see us check if
they exist in the pool before trying to add them, but I also understand
the logic that says - just try the create and handle the failure.

>>> And I think we have a way to eliminate even the little window - just track if we successfully built the volume or not. Something among these lines:
>>>
> 
> After properly propagating the error value if the volume exists, we
> don't need this patch refreshing the pool to work around the original
> bug.
> 

We may not, unless there's a need due to the pool capacity calculation.

John