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 :|