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

On 02/26/2010 06:23 AM, Daniel P. Berrange wrote: > On Thu, Feb 25, 2010 at 10:01:09PM -0500, Dave Allan 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 agree, we should not push 0.7.7 with the code the way it is now, but >> what I'm describing below is also trivial, so it wouldn't push back the >> release. >> >>> 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. >> >> The problem is that what build means has never been defined, and while >> it may have been the intention to implement only destructive operations, >> the backends implement a variety of actions. For some backends build is >> is easy to reverse; others not. The only guidance virsh help gives is >> "build a pool" which doesn't indicate any danger at all. I would define >> build as "make the changes to the media necessary to start the pool" and >> split those changes into destructive and non-destructive actions with a >> flag. (see below) > > The distinction of destructive vs non-destructive does not make sense > at an API level. The build operation is intended to do whatever data > formatting changes are required in order to construct the pool. Whether > this is destructive or not is really an artifact of whether there is any > data already on the storage that you already care about. > > This is why I think the build() operation, with flags=0, should refuse > to run if it detects that the pool already has been constructed. This > will protect against data loss with virt-manager's current usage, and > also avoid changing the semantics of the current LVM/disk backends. > > Adding a OVERWRITE flag will then allow apps to explicitly ignore the > checks for existing data, allowing possibly destructive operation. > >> >>> 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 >> >> I propose we add a DESTRUCTIVE flag and require it for destructive >> operations on all the backends. The downside, obviously, is that it >> changes the behavior of the disk and LVM backends that currently don't >> require a flag for destructive operations. I'm not too worried about >> that behavioral change, though, because what's in the tree right now >> changes the behavior of the fs backend making a previously >> non-destructive operation into a destructive operation without any >> change on the users part and without warning. > > I don't want us to be changing the semantics of the existing LVM/disk > backends here. In terms of our release schedule, I think we should just > revert the current change to the FS backend completely. Then make the > release. Then re-apply the change, with some followup patches to add > the checks for existing formatted data in the next release. Ok, I reverted the original patch as we're all in agreement on that. The utility of the check is dependent on what's on the disk. If the user specifies the wrong partition, which is not hard to do, for example, a choosing a database partition instead of the intended filesystem in the virt-manager gui, the partition will still be overwritten without warning, whereas other overwrite operations are, as Cole says, loudly warned about. The only way to prevent that is to require the flag before any fs format. It's somewhat gross, I freely admit, but I feel fairly strongly that we need to err on the side of caution when dealing with formatting. I'm ok with leaving the other backends the way they are. Dave