[libvirt] virsh blockresize syntax is inconsistent with vol-resize and somewhat dangerous

[Cole Robinson]

On 11/18/19 3:16 PM, Cole Robinson wrote:
> On 10/25/19 4:28 AM, Patrik Martinsson wrote:
>> Hi Tim,
>>
>> I recently stumbled on the same thing, accidentally shrinking a blockdevice.
>>
>> I have written a patch for virsh that will force the user to append a
>> '--force' flag if shrinking is desired.
>>
>> The behavior is somewhat still inconsistent with the vol-resize
>> command, however a bigger rewrite is needed to make both commands
>> operate exactly the same, which I don't know if actually needed.
>>
>> Previous discussion can be found below,
>> - https://www.redhat.com/archives/libvir-list/2019-October/msg00258.html
>> - https://www.redhat.com/archives/libvir-list/2019-October/msg01437.html
>>
>> Best regards,
>> Patrik
>>
>>
>> On Thu, Oct 24, 2019 at 6:04 PM Tim Small <tim at seoss.co.uk> wrote:
>>>
>>> Hello,
>>>
>>> virsh has two commands which can be used to resize block devices -
>>> "blockresize" for volumes in use by and active guest, and "vol-resize"
>>> for volumes which are not in use.
>>>
>>> The vol-resize syntax allows to specify the size as a delta (increase or
>>> decrease vs. the current size), and also refuses to shrink a volume
>>> unless the "--shrink" argument is also passed.
>>>
>>> Most other tools which can be used for block device resizing (outside of
>>> libvirt) also have similar "--shrink" argument requirements when
>>> reducing the size of an existing block device.  e.g. ceph requires
>>> "--allow-shrink" when using the "rbd resize" command.
>>>
>>> The lack of such a safety device makes "blockresize" a foot-gun (which I
>>> recently found to great effect when I typoed the domain name to another
>>> valid domain).
>>>
>>> It seems I am not alone in making this error e.g.
>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=902171
>>>
>>> One possible solution would be to make a new command e.g. "domblkresize"
>>> or perhaps "live-resize", which implement the "--shrink" and "--delta"
>>> behaviour to make it consistent with "vol-resize" syntax, and mark the
>>> "blockresize" command as deprecated in the documentation and help (so
>>> that existing automation which depends on the current behaviour doesn't
>>> break).
>>>
>>> Any thoughts?  Should I open this as an RFE?
>>>
> 
> Considering there's been multiple people hitting it, I think it's
> something we should fix in libvirt. Just need buy in from other devs. To
> summarize:
> 
> 'virsh blockresize' will online resize an image path for a running VM.
> It does this with the qemu block_resize monitor command via the
> virDomainBlockResize API. The API doesn't provide any protection against
> shrinking the disk image though, which I presume is both the less common
> intention of the operation, and much less often safe to do for a running
> VM. And a user typo can mean data loss
> 
> virsh vol-resize, which is storage API virStorageVolResize, is for
> offline image resizing, mostly using qemu-img. It has had a SHRINK API
> flag from the outset, rejecting requests to reduce the image size unless
> the flag is passed. Seems like a safe pattern to follow.
> 
> Can we change existing blockresize behavior? I think it's reasonable;
> we've added flags to other APIs that are required to restore old
> behavior, UNDEFINE_NVRAM for one example.
> 

I brought this question up in this thread:
https://www.redhat.com/archives/libvir-list/2019-December/msg00817.html

danpb suggested making this a protection that lives in virsh only. So,
change blockresize to reject shrinking, but add a --shrink option to
override that behavior, and all the code lives in tools/ so the old API
behavior is preserved. You can CC me on a patch and I'll review it (but
I'll be offline until January)

Thanks,
Cole