Bug 1460577 - regression: lvm2 pvresize command suddenly became interactive, breaking automated usage
regression: lvm2 pvresize command suddenly became interactive, breaking autom...
Status: NEW
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: lvm2 (Show other bugs)
7.4
Unspecified Unspecified
medium Severity medium
: rc
: ---
Assigned To: LVM and device-mapper development team
cluster-qe@redhat.com
:
Depends On:
Blocks: TRACKER-bugs-affecting-libguestfs 1460692
  Show dependency treegraph
 
Reported: 2017-06-12 01:37 EDT by Xianghua Chen
Modified: 2017-08-17 14:31 EDT (History)
12 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
: 1460692 (view as bug list)
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
log.pvresize (51.30 KB, text/plain)
2017-06-12 01:44 EDT, Xianghua Chen
no flags Details

  None (edit)
Description Xianghua Chen 2017-06-12 01:37:26 EDT
Description of problem:
pvresize-size failed to resize an LVM physical volume.
Please refer to the attachment for detailed log.


Version-Release number of selected component (if applicable):
libguestfs-1.36.3-4.el7.x86_64
kernel-3.10.0-679

How reproducible:
100%


Steps:

1. Create a image for test:
# qemu-img create -f raw lvm_partition.none.raw 10G

2.Create pv in the image:
# guestfish -a lvm_partition.none.raw run : pvcreate /dev/sda

3. Use pvresize-size to resize the pv:
# guestfish -a lvm_partition.none.raw run : pvresize-size /dev/sda 5368709120 -v -x
... ...
commandrvf: lvm pvresize --setphysicalvolumesize 5368709120b /dev/sda
/dev/sda: Requested size 5.00 GiB is less than real size 10.00 GiB. Proceed?  [y/n]: [n]
  Physical Volume /dev/sda not resized.
guestfsd: error: /dev/sda: /dev/sda: Requested size 5.00 GiB is less than real size 10.00 GiB. Proceed?  [y/n]: [n]
  Physical Volume /dev/sda not resized.
guestfsd: main_loop: proc 249 (pvresize_size) took 0.02 seconds
libguestfs: trace: pvresize_size = -1 (error)
libguestfs: error: pvresize_size: /dev/sda: /dev/sda: Requested size 5.00 GiB is less than real size 10.00 GiB. Proceed?  [y/n]: [n]
  Physical Volume /dev/sda not resized.
libguestfs: trace: close
... ...


Actual results:
The pv can not be resized successfully.

Expected results:
The pv should be resized successfully.

Additional info:
This command failed on x86_64/ppc64le/aarch64.
It can pass on kernel-3.10.0-514.el7.x86_64 with libguestfs-1.36.3-4.el7.x86_64.
Comment 1 Xianghua Chen 2017-06-12 01:44 EDT
Created attachment 1286926 [details]
log.pvresize
Comment 2 Richard W.M. Jones 2017-06-12 07:42:01 EDT
The reproducer is:

  $ guestfish -N disk:10G pvcreate /dev/sda : pvresize-size /dev/sda 5G
  libguestfs: error: pvresize_size: /dev/sda: /dev/sda: Requested size 5.00 GiB is less than real size 10.00 GiB. Proceed?  [y/n]: [n]
    Physical Volume /dev/sda not resized.

Add -v -x to the command line to see the lvm2 commands being performed.

Actually the problem is not the kernel, but the lvm2 package.

WORKING => lvm2-2.02.166-1.el7_3.4.x86_64
 BROKEN => lvm2-2.02.171-4.el7.x86_64
Comment 3 Richard W.M. Jones 2017-06-12 07:58:09 EDT
Reproducible with lvm2-2.02.171-2.fc27.x86_64 (from Fedora Rawhide)
so I guess this is an upstream change.
Comment 4 Richard W.M. Jones 2017-06-12 08:01:11 EDT
Seems the upstream commit which changed this was:

  commit cbc69f8c693edf0d1307c9447e2e66d07a04bfe9
  Author: Alasdair G Kergon <agk@redhat.com>
  Date:   Thu Apr 27 02:36:34 2017 +0100

    pvresize: Prompt when non-default size supplied.
    
    Seek confirmation before changing the PV size to one that differs
    from the underlying block device.

Be nice if we didn't push breaking changes from less than two
months ago straight into RHEL.

I will add a workaround to libguestfs, but unfortunately it's too
late to get this into RHEL 7.4 GA.
Comment 5 Marian Csontos 2017-06-12 08:08:09 EDT
See https://bugzilla.redhat.com/show_bug.cgi?id=1444130 for explanation. Especially Bug 1444130#c3.

I closed the original request as WONTFIX but was overridden - seems this is a frequently misused option, and ideally would not be used at all, except for testing.

But IMO as there is no risk if PV is smaller than physical device, creating smaller PV should not need a prompt.
Comment 6 Zdenek Kabelac 2017-06-12 08:18:53 EDT
lvm2 users should NOT use --setphysicalsize  in non-interactive scripts.

If user resizes underlying PV properly - then 'just' plain pvresize shall get the matching size right with auto-detection.

Anything that needs to override 'detected' defaults is going to need prompting, as we get far more problem from not prompting - compared with few brakes of basically broken scripts.

So my advice here is - to fix  libguestfs to NOT user --setphysicalsize - since this is purely admin command (just like you should mostly not use any --force inside scripts)
Comment 7 Pino Toscano 2017-06-12 08:38:40 EDT
(In reply to Zdenek Kabelac from comment #6)
> lvm2 users should NOT use --setphysicalsize  in non-interactive scripts.
> [...]
> So my advice here is - to fix  libguestfs to NOT user --setphysicalsize -
> since this is purely admin command (just like you should mostly not use any
> --force inside scripts)

What are the script-friendly alternatives, then?
Comment 8 Zdenek Kabelac 2017-06-12 08:52:41 EDT
What is wrong  with plain 'pvresize  /dev/sda'  ?

Why is lvm2 not detecting proper size  ?

Why is passed differently sized device to lvm2 command ?
Comment 9 Pino Toscano 2017-06-12 09:01:17 EDT
(In reply to Zdenek Kabelac from comment #8)
> What is wrong  with plain 'pvresize  /dev/sda'  ?
> 
> Why is lvm2 not detecting proper size  ?
> 
> Why is passed differently sized device to lvm2 command ?

The size can specified in case a particular size is wanted. And yes, there are uses cases for that.
Comment 10 Alasdair Kergon 2017-06-12 09:02:51 EDT
https://sourceware.org/git/?p=lvm2.git;a=commitdiff;h=cbc69f8c693edf0d1307c9447e2e66d07a04bfe9

So the request is to revert this part of the patch in RHEL7.4, but to leave it upstream:

else if (new_size < size)
        if (!yes && yes_no_prompt("%s: Requested size %s is less than real size %s. Proceed?  [y/n]: ", pv_name, display_size(cmd, (uint64_t) new_size), display_size(cmd, (uint64_t) size)) == 'n')
                goto_out;
Comment 11 Zdenek Kabelac 2017-06-12 09:07:22 EDT
(In reply to Pino Toscano from comment #9)
> (In reply to Zdenek Kabelac from comment #8)
> > What is wrong  with plain 'pvresize  /dev/sda'  ?
> > 
> > Why is lvm2 not detecting proper size  ?
> > 
> > Why is passed differently sized device to lvm2 command ?
> 
> The size can specified in case a particular size is wanted. And yes, there
> are uses cases for that.

Such 'extra' use case is still 'supported' - but needs prompt confirmation as in most cases it's usually user's fault to use wrongly sized device - so the prompt got preference here - so user may realize if he really wants to use differently sized PV then the one lvm2 is really seeing in your system.

Note - there could be numerous problems with the missing physical space on device where lvm2 was the taken 'responsible' for letting happen this problem.

Please reconsider your tool workflow if you really need to pass in 'wrongly' sized device to lvm2 command.
Comment 12 Pino Toscano 2017-06-12 09:14:36 EDT
(In reply to Zdenek Kabelac from comment #11)
> (In reply to Pino Toscano from comment #9)
> > (In reply to Zdenek Kabelac from comment #8)
> > > What is wrong  with plain 'pvresize  /dev/sda'  ?
> > > 
> > > Why is lvm2 not detecting proper size  ?
> > > 
> > > Why is passed differently sized device to lvm2 command ?
> > 
> > The size can specified in case a particular size is wanted. And yes, there
> > are uses cases for that.
> 
> Such 'extra' use case is still 'supported' - but needs prompt confirmation
> as in most cases it's usually user's fault to use wrongly sized device - so
> the prompt got preference here - so user may realize if he really wants to
> use differently sized PV then the one lvm2 is really seeing in your system.

Sure, hence my question above about the proper way to make *this* user case work in scripted scenarios, where this situation is known, and already taken care of.
Comment 13 Alasdair Kergon 2017-06-12 09:17:53 EDT
The assumption has been that people would always resize the device first and THEN run pvresize to pick up the new size.

But if you're not sure that the part of the device being removed is completely unused (not in complete control of allocation and worried about something allocating in parallel to your resizing operations) then you might make the case that it's safer to do a 3-step process: pvresize explicitly to a size known to be smaller than the final size you want, reduce the real device size, run pvresize again (without --setphysicalvolumesize) to adjust back up to the actual size.
Comment 14 Zdenek Kabelac 2017-06-12 09:19:31 EDT
We do support generic  '--yes'   to answer Y to all prompts.

But this is kind of  'blank check' for anything 'strange' and prompted during cmdline processing.


So I'd most probably highly recommend to pass correctly sized device in the first place - so it's validated during all steps properly and there is minimized chance of an unexpected error.


So please reconsider the model workflow if you can pass correctly sized device when you run pvresize.
Comment 15 Pino Toscano 2017-06-12 09:33:21 EDT
(In reply to Zdenek Kabelac from comment #14)
> We do support generic  '--yes'   to answer Y to all prompts.

And that's indeed what is going to be used soon.

> But this is kind of  'blank check' for anything 'strange' and prompted
> during cmdline processing.

Well, too bad (not for us, though).

> So please reconsider the model workflow if you can pass correctly sized
> device when you run pvresize.

There's a misunderstanding on your side about what we provide in libguestfs:
1) a 'pvresize' API, which just calls `pvresize` on the specified device
2) a 'pvresize_size' API, which allows to specify a size, and thus `pvresize --setphysicalvolumesize SIZE` is called

In both the cases, those are *user* APIs, i.e. the user invokes to do all the operation they need. Nothing directly in libguestfs itself uses it, and only "pvresize" is used in virt-resize to resize a PV.

As a consequence of the above: this is totally in control of the user, and thus the (broken record) suggestion "fix your workflow" does not make any sense, and would be nice if you could avoid mentioning it once again, thanks.
Comment 16 Zdenek Kabelac 2017-06-12 09:47:52 EDT
Providing  dangerous operation in API without any user confirmation is almost on pair  with running  i.e. 'mkfs' over existing formatted device and not asking for confirmation.


Clearly there are cases where a user may need to do it and pvresize the disk to a size mismatching the size reported by kernel.   But there is way way more users how actually do not want this to happen (often doing it in 'virtual' machines and passing wrongly sized device to pvresize and expecting things to work - since  'pvresize' has passed....)


So which API user does need to have it supported out-of-the box ?
The command really should require confirmation even from API user.

The intention lvm2 has is obviously totally defeated once user will convert every pvresize call to use  --yes    (it's even worse since more things will get 'green light' and some other suspicious actions  will go on without prompt)

Maybe something like:  --setphysicalsizedifferentfromkernel  might be the answer - so if user really really knows what is going on  - it might be some way.

Such option would likely need to check for different  size - and only in this case apply the size (otherwise again script writers would 'blindly'  convert existing option to more powerful one....
Comment 17 Richard W.M. Jones 2017-06-12 10:01:15 EDT
libguestfs provides APIs for creating and manipulating disk
images.  These APIs can be called from (eg) C programs, and many
other situations where there is no "user" to talk to.  And yes
we do very much provide a 'mkfs' operation which works without
prompting -- that is in fact the entire point.

Anyway, the issue here is that this change was made at the last
minute.  Upstream 6 weeks ago.  In RHEL 7.4, 5 weeks ago.

The change is still not in Fedora 26, but we're pushing it to
our customers anyway.

So please can you (in future) not make large changes like this
rebase long after the development phase of RHEL and before it has
had any significant testing anywhere.

I have (in bug 1460692) added the --yes flag to our invocation
of pvresize.
Comment 18 Zdenek Kabelac 2017-06-12 10:12:24 EDT
For 'scripts' running in  I-don't-care  mode  using  '--yes'  is clearly the way to go - as with ANY other lvm2 command.

Prompts are added regularly when some operation is seen for a regular user a dangerous one.

Although for normal 'scripts' - they should be living for default 'no' answer for non-interactive runs.

This brings however new 'light' -  lvm2 should consider storing such 'bypass' flags within it's physical on-disk metadata so it's known for analysis there has been such 'override' action provided.
Comment 19 Alasdair Kergon 2017-06-12 10:16:32 EDT
(In reply to Pino Toscano from comment #15)

> 2) a 'pvresize_size' API, which allows to specify a size, and thus `pvresize
> --setphysicalvolumesize SIZE` is called

So what guidance is provided to people using this part of the libguestfs API to make sure they don't make the same mistakes that command line users make?  We have deliberately made it harder to use to try to stop people who don't need it from using it without thinking properly about it.  If they look, even with --yes, presumably API users will see our new warning message appearing in their output when there's a size mismatch.

log_warn("WARNING: %s: Pretending size is %" PRIu64 " not %" PRIu64 " sectors.", pv_name, new_size, size);
Comment 20 Richard W.M. Jones 2017-06-12 10:22:33 EDT
(In reply to Alasdair Kergon from comment #19)
> (In reply to Pino Toscano from comment #15)
> 
> > 2) a 'pvresize_size' API, which allows to specify a size, and thus `pvresize
> > --setphysicalvolumesize SIZE` is called
> 
> So what guidance is provided to people using this part of the libguestfs API
> to make sure they don't make the same mistakes that command line users make?

To unask the question, libguestfs is mostly used in situations where
you're automatically creating disk images, and in that case the program
has calculated the right size before calling pvresize_size.  If the
program calculates the wrong size then it has a bug.

I was going to say this API is only used by virt-resize.  In fact that
isn't true: it's only used (by our code) in a test script used to test
virt-resize ...

Note You need to log in before you can comment on or make changes to this bug.