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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list