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