[libvirt] 'build' on FS pool now unconditionally formats?

[Cole Robinson]

On 02/25/2010 09:55 AM, Cole Robinson wrote:
> On 02/25/2010 08:25 AM, Daniel P. Berrange wrote:
>> On Thu, Feb 25, 2010 at 01:19:54PM +0100, Daniel Veillard wrote:
>>> On Wed, Feb 24, 2010 at 03:51:45PM -0500, Cole Robinson wrote:
>>>> Hi guys,
>>>>
>>>> Looking at the new FS pool build options and talking with Dave, I see that
>>>> calling PoolBuild on an FS pool now unconditionally calls mkfs. This is really
>>>> bad when mixed with virt-manager: previously, we assumed the FS build command
>>>> was always non destructive (at most it created a directory), so we called it
>>>> every time, and didn't even allow users to opt out, since there wasn't a use
>>>> case that called for it.
>>>>
>>>> This new formatting behavior really needs to be opt in, otherwise all
>>>> virt-manager versions creating an FS pool can destroy data.
>>>>
>>>> Just FYI, for disk pools (and certain LVM configurations) where this operation
>>>> has always been destructive, we default to build=off, and loudly warn the user
>>>> if they choose otherwise. We can do that with this new option as well, but the
>>>> previous behavior really needs to be reinstated IMO (and before the new release).
>>>>
>>>> I fully accept that this could be a bug in virt-manager's assumptions of the
>>>> build command, but even consider a virsh user: previously build just created a
>>>> directory, now it formats a partition, without any XML change.
>>>
>>> I was initially reluctant of changing the behaviour, and asked to use a
>>> flag to keep the original default semantic. I got convinced that noone
>>> could rely on it because the function was basically incomplete. But since
>>> virt-manager ships with an expectation on the previous behaviour, I
>>> revert my position, we need to add a _FORMAT = 4 flag for this call and
>>> only call mkfs if that flag is passed. Fix is trivial we should not
>>> push 0.7.7 without it,
>>
>> I really don't want to  add an extra flag, because it makes filesystem
>> pool a special case. The 'build' operation is intentionally destructive
>> by its very definition, and virt-mnager should never be expecting it to
>> be safe to call on specific pool types. 
>>
> 
> Where exactly is build listed as intentionally destructive? Prior to this
> change, build was destructive in 2 cases:
> 
> - Disk pools. This would re-label an existing disk. This was the only instance
> with a use case for build vs. not build, and we could even decide that for the
> user based on whether they manually specified a format or not.
> 
> - LVM volume groups when source devices were specified. In this case, source
> devices had no meaning or usefulness unless a 'build' was called. By
> specifying a source device, the user was already opting in to building.
> 
> In all other cases, build was unimplemented, or had no destructive effect
> whatsoever. All build did for the dir/fs/netfs case was create a directory. It
> would have been a disservice to virt-manager users to let them opt out, as it
> would only cause problems if they were pointing to a non existent target
> directory, and had _zero_ drawbacks.
> 
> I mean, the term 'build' doesn't even lend itself to being 'destructive', more
> like 'constructive'. The API docs don't point any of this out. I take
> responsibility for this failing as well, since I've spent time in the storage
> code and never thought to clarify this API point, but the intention of build
> to be always destructive was not obvious.
> 
>> IMHO, we should do two things to address this
>>
>>  - Fix virt-manager to not call build all the time for any pool
>>    type - it must only do it when expkicitly requested
>>
> 
> Agreed.
> 
>>  - Make the 'build' operation check to see if the pool is 
>>    already constructed (eg  LVM magic check for logical pools,
>>    FAT partition check for disk ools & filesystem magic check
>>    for the fs pool). Reject the build operation if any of these
>>    show that the pool exists / is alread ybuilt
>>
> 
> Why don't we copy the non-destructive build actions into pool-start: this
> basically means create directories at start time. Things like re-permissioning
> directories can still be done in 'build', along with all the new actions.
> 
> That way, build ONLY ever performs destructive actions, so API users
> (virt-manager) can provide a consistent interface for build. Otherwise,
> warning 'This may destroy something!' for building a directory pool is
> misleading (currently).
> 
>>  - Add a 'OVERWRITE' flag, to allow apps to forcably reformat,
>>     regardless of current state
>>
> 
> This can then be dropped.
> 

Actually, thinking some more, we still need this flag. Adding the detection
pieces requires a force flag.

But I don't think build should fail if the device is already 'built': what if
the user wants to change FS target permissions but not run mkfs? We don't want
build to fail. Another option is paying more attention to <format>: if user
specified format=auto, assume no mkfs. We could add an explicit auto format
for the disk backend as well.

- Cole