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

[Daniel P. Berrange]

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. 

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

 - 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

 - Add a 'OVERWRITE' flag, to allow apps to forcably reformat,
    regardless of current state

This will let us keep consistent semantics for all pool types, while still
protecting against broken apps like virt-manager which are blindly calling
build when they shouldn't.

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|