Bug 1353759 - lvconvert flags that no longer work with cache due to 'type' changes in .160-1
Summary: lvconvert flags that no longer work with cache due to 'type' changes in .160-1
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: lvm2
Version: 7.3
Hardware: x86_64
OS: Linux
medium
medium
Target Milestone: rc
: ---
Assignee: David Teigland
QA Contact: cluster-qe@redhat.com
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-07-07 23:02 UTC by Corey Marthaler
Modified: 2016-11-04 04:22 UTC (History)
10 users (show)

Fixed In Version: lvm2-2.02.161-3.el7
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-11-04 04:22:10 UTC
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Bugzilla 1344120 unspecified CLOSED LVM cache: 'lvconvert --cachepolicy' fails to change cache policy (or even give an error) 2020-10-14 00:28:05 UTC
Red Hat Product Errata RHBA-2016:1445 normal SHIPPED_LIVE lvm2 bug fix and enhancement update 2016-11-03 13:46:41 UTC

Internal Links: 1344120

Description Corey Marthaler 2016-07-07 23:02:08 UTC
Description of problem:
Not sure if we want to continue to support this lvconvert combination, but this was a case that passed before the .160-1 build.


2. Conversion to cache pool (w/o specified meta volume):
lvcreate  -n POOL1 -L 100M cache_sanity
lvcreate  -n POOL2 -L 100M cache_sanity
lvcreate  -n POOL3 -L 100M cache_sanity

[root@host-082 ~]# lvs -a -o +devices
  LV    VG            Attr       LSize   Pool Origin Data%  Meta%  Move Log Cpy%Sync Convert Devices
  POOL1 cache_sanity  -wi-a----- 100.00m                                                     /dev/sdf1(0)
  POOL2 cache_sanity  -wi-a----- 100.00m                                                     /dev/sdf1(25)
  POOL3 cache_sanity  -wi-a----- 100.00m                                                     /dev/sdf1(50)

lvconvert --cachepool cache_sanity/POOL1
  Operation not permitted on striped or linear LV cache_sanity/POOL1
  Operations permitted on a striped or linear LV are:
  --merge
  --type snapshot | --snapshot
  --type thin | --thin
  --type cache | --cache
  --type thin-pool
  --type cache-pool
  --type mirror | --mirrors
  --type raid* | --mirrors


Version-Release number of selected component (if applicable):
3.10.0-418.el7.x86_64

lvm2-2.02.160-1.el7    BUILT: Wed Jul  6 11:16:47 CDT 2016
lvm2-libs-2.02.160-1.el7    BUILT: Wed Jul  6 11:16:47 CDT 2016
lvm2-cluster-2.02.160-1.el7    BUILT: Wed Jul  6 11:16:47 CDT 2016
device-mapper-1.02.130-1.el7    BUILT: Wed Jul  6 11:16:47 CDT 2016
device-mapper-libs-1.02.130-1.el7    BUILT: Wed Jul  6 11:16:47 CDT 2016
device-mapper-event-1.02.130-1.el7    BUILT: Wed Jul  6 11:16:47 CDT 2016
device-mapper-event-libs-1.02.130-1.el7    BUILT: Wed Jul  6 11:16:47 CDT 2016
device-mapper-persistent-data-0.6.2-0.1.rc8.el7    BUILT: Wed May  4 02:56:34 CDT 2016
cmirror-2.02.160-1.el7    BUILT: Wed Jul  6 11:16:47 CDT 2016
sanlock-3.3.0-1.el7    BUILT: Wed Feb 24 09:52:30 CST 2016
sanlock-lib-3.3.0-1.el7    BUILT: Wed Feb 24 09:52:30 CST 2016
lvm2-lockd-2.02.160-1.el7    BUILT: Wed Jul  6 11:16:47 CDT 2016

Comment 2 Corey Marthaler 2016-07-08 16:38:40 UTC
Here are two other flags that are failing now. 

[root@host-083 ~]# lvs -a -o +devices
  LV              VG            Attr       LSize   Pool    Origin Data%  Meta%  Move Log Cpy%Sync Convert Devices         
  corigin         cache_sanity  Cwi-a-C---   4.00g [cpool]        0.00                   100.00           corigin_corig(0)
  [corigin_corig] cache_sanity  owi-aoC---   4.00g                                                        /dev/sdd1(0)
  [cpool]         cache_sanity  Cwi---C--- 500.00m                                                        cpool_cdata(0)
  [cpool_cdata]   cache_sanity  Cwi-ao---- 500.00m                                                        /dev/sdf1(4)
  [cpool_cmeta]   cache_sanity  ewi-ao----   8.00m                                                        /dev/sdf1(2)
  [lvol0_pmspare] cache_sanity  ewi-------   8.00m                                                        /dev/sdf1(0)

[root@host-083 ~]# lvconvert --cachepolicy smq cache_sanity/corigin
  Operation not permitted on cache LV cache_sanity/corigin
  Operations permitted on a cache LV are:
  --splitcache
  --uncache
  --splitmirrors (operates on mirror or raid sub LV)
  --type thin-pool

[root@host-083 ~]# lvconvert --cachemode writeback cache_sanity/corigin
  Operation not permitted on cache LV cache_sanity/corigin
  Operations permitted on a cache LV are:
  --splitcache
  --uncache
  --splitmirrors (operates on mirror or raid sub LV)
  --type thin-pool

Comment 3 David Teigland 2016-07-08 18:21:51 UTC
The old lvconvert was doing nothing with those options, so the new behavior is actually a bug fix when compared with the old.

Old behavior from lvconvert:

[root@null-02 ~]# lvs -o+cachemode bb/lv1
  LV   VG Attr       LSize Pool    Data%  Cpy%Sync CacheMode   
  lv1  bb Cwi-a-C--- 1.00g [cpool] 0.00   100.00   writethrough

[root@null-02 ~]# lvconvert --cachemode writeback bb/lv1

[root@null-02 ~]# lvs -o+cachemode bb/lv1
  LV   VG Attr       LSize Pool    Data%  Cpy%Sync CacheMode   
  lv1  bb Cwi-a-C--- 1.00g [cpool] 0.00   100.00   writethrough

[root@null-02 ~]# lvchange --cachemode writeback bb/lv1
  Logical volume bb/lv1 changed.

[root@null-02 ~]# lvs -o+cachemode bb/lv1
  LV   VG Attr       LSize Pool    Data%  Cpy%Sync CacheMode
  lv1  bb Cwi-a-C--- 1.00g [cpool] 0.08   0.00     writeback

Comment 4 David Teigland 2016-07-08 18:28:09 UTC
I can't find any place that indicates 'lvconvert --cachepool VG/LV' was a valid command, so for now I'm going to leave it out, rather than making it a new alias.

Comment 5 Zdenek Kabelac 2016-07-19 08:29:30 UTC
We have already bug  1344120   - basically requesting to add 'lvchange' logic to be usable through lvconvert as well.

There are few other 'examples' where there is not a 'clear' reasoning for a user to use  lvconvert/lvchange - and for lvm2 it's not a big issue to support both variant as long as the command line is unambiguous.

Although I see this as a low-prio issue...

Command definition is clear:

lvchange is changing LV property.
lvconvert is changing LV type.


It would possibly more important to capture & fix property changes made through lvconvert to be fully usable via lvchange.

Comment 7 David Teigland 2016-07-20 16:43:23 UTC
(In reply to Roman Bednář from comment #6)
> >I can't find any place that indicates 'lvconvert --cachepool VG/LV' was a valid command 
> 
> lvconvert manpages have a reference to this:
>  --cachepool CachePoolLogicalVolume{Name|Path}
>        This argument is necessary when converting a logical volume to a
> cache LV.

That's an interesting quote because it illustrates an easy confusion about this option.  The quote is about creating a cache LV, not about creating a cache pool LV.

> Anyway, this seems like an obvious regression. Since it worked up until now.
> Or is there any reason to remove --cachepool option? This basically just
> forces users to use lvconvert in a different way.

The standard, unambiguous method for converting an LV to a new type is:

lvconvert --type NewType VG/LV

The standard, direct method for converting an LV to a cache pool LV is:

1. lvconvert --type cache-pool VG/LV

The standard, direct method for converting an LV to a cache LV is:

2. lvconvert --type cache --cachepool vg/pool VG/LV

where the --cachepool arg specifies the cache pool to use when caching LV.

The non-standard, non-direct command which is the topic of this bz is:

3. lvconvert --cachepool VG/LV

This is ambiguous because it's not clear what is meant... it's similar to 2 where it's specifying a cache pool to use, but for what?  Command 3 doesn't indicate anything to actually convert, it's an incomplete command.  But, the code has previously taken this partial/ambiguous command 3 as an alias for command 1.
But there was never any need for command 1 to have an alias, an alias would just add confusion, especially since command 3 does not follow the standard command format.  It would be clearer for users if command 3 just reported an error and told users to run a standard, direct command which is not ambiguous (1 or 2).

Anyway, for the time being I've changed the code to once again allow command 3 to function as an alias for command 1, even though I never found any place that actually advertised this trick.

Comment 8 Zdenek Kabelac 2016-07-21 13:29:41 UTC
Let's recap functionality how it was meant to work - although it has not likely yet fully worked in past this way - since lvconvert was '2nd.' class citizen - so not fully finished.

lvm2 command has 2 levels of functionality -

Explicit mode (call it 'Expert-mode') - where the admin user has to specify all options explicitly  and if there is some mismatch error is reported as lvm2 has no 'freedoom' to estimate anything.  In this mode admin has to be really good expert in all levels and understand lvm2 internal internal design.
(this is typically used in scripts).


Implicit mode - where the 'actual' action is deduced by internal rules to follow 'common sense logic' aka what user would typically want to happen within given context - without taking much care about internal design and using short 'expression' (aka user has no idea what is segment type).
(Ignoring support of this mode leads to basically lots of various 'layering' tools trying to overtake this role - however they do it typically wrong...)
(typicaly used on command line).


So here we have simplified lvconvert synopsis:

lvconvert [OPTIONS] ConvertedLVName{path}  [PVs...]


And in case of cache-pool conversion we have these examples:

lvconvert --type cache-pool vg/lv

# Note: When vg/lv is not present (no free arg) - tool can still deduce it from
# 'the only' LV arg given for --cachepool.
# However in case PV would need to be specified - LV has to be repeated!
lvconvert --cachepool vg/lv   


# Buggy example (should be fixed) as free-arg  vg/lv cannot be used as PV.
lvconvert --cachepool vg/lv vg/lv

ATM it gives:
Physical Volume "vg/lv" not found in Volume Group "vg".

Internal lvconvert deducing logic missed to calculate cache-pool or cache type.


While in case of 'same' LV name and other none-conflicting options - it should use them 'equally'.


===== the 'most' used & simplest use-case =======

lvconvert -H --cachepool vg/lvol0 --poolmetadata vg/lvol1  vg/lvol2

Ideally '-H' would be also skipped as it could be deduced,
passed args are pretty clear and unambiguous.
Taking 3 volumes and building 1 cached LV.

Comment 9 Zdenek Kabelac 2016-07-28 12:46:33 UTC
Upstream patch should restore the functionality:

https://www.redhat.com/archives/lvm-devel/2016-July/msg00198.html

Comment 11 Roman Bednář 2016-08-05 14:08:05 UTC
lvconvert --cachepool is now reverted to previous behaviour:

# lvconvert --cachepool cache_sanity/POOL1
  WARNING: Converting logical volume cache_sanity/POOL1 to cache pool's data volume with metadata wiping.
  THIS WILL DESTROY CONTENT OF LOGICAL VOLUME (filesystem etc.)
Do you really want to convert cache_sanity/POOL1? [y/n]: y
  Converted cache_sanity/POOL1 to cache pool.
# lvs -a -o lv_name,vg_name,lv_attr,pool_lv
  LV              VG            Attr       Pool
  POOL1           cache_sanity  Cwi---C---     
  [POOL1_cdata]   cache_sanity  Cwi-------     
  [POOL1_cmeta]   cache_sanity  ewi-------     
  POOL2           cache_sanity  -wi-a-----     
  POOL3           cache_sanity  -wi-a-----     
  [lvol0_pmspare] cache_sanity  ewi-------     
  root            rhel_virt-036 -wi-ao----     
  swap            rhel_virt-036 -wi-ao----  

====================================================================
needinfo on lvconvert --cachpool with multiple args:

As Zdenek mentioned:
># Buggy example (should be fixed) as free-arg  vg/lv cannot be used as PV.
>lvconvert --cachepool vg/lv vg/lv

>ATM it gives:
>Physical Volume "vg/lv" not found in Volume Group "vg".

>Internal lvconvert deducing logic missed to calculate cache-pool or cache type.

So I'm guessing these two commands should end up by having a cached lv on vg/lv2? 

# lvconvert --cachepool vg/lv1
  WARNING: Converting logical volume vg/lv1 to cache pool's data volume with metadata wiping.
  THIS WILL DESTROY CONTENT OF LOGICAL VOLUME (filesystem etc.)
Do you really want to convert vg/lv1? [y/n]: y
  Converted vg/lv1 to cache pool.
# lvconvert --cachepool vg/lv1 vg/lv2
  Physical Volume "vg/lv2" not found in Volume Group "vg".


Is this going to be fixed too? Do we need another BZ to track this?

Comment 12 David Teigland 2016-08-05 14:25:08 UTC
> # lvconvert --cachepool vg/lv1
> # lvconvert --cachepool vg/lv1 vg/lv2

Neither of those are proper commands.  If you look in the most recent lvconvert man page you will find neither defined.  Any definition we might give to them would be inconsistent with some other properly defined command.

That said, if one of those commands actually did something in a previous release, then that may be a reason to retain the old behavior for historical reasons.  So, if you have a previous release where one of those commands did something, you can compare the current behavior to the old behavior and check they do the same thing.
But, apart from historical reasons, neither command is meaningful.

Comment 13 Zdenek Kabelac 2016-08-05 21:11:56 UTC
(In reply to Roman Bednář from comment #11)
> 
> So I'm guessing these two commands should end up by having a cached lv on
> vg/lv2? 
> 
> # lvconvert --cachepool vg/lv1
>   WARNING: Converting logical volume vg/lv1 to cache pool's data volume with
> metadata wiping.
>   THIS WILL DESTROY CONTENT OF LOGICAL VOLUME (filesystem etc.)
> Do you really want to convert vg/lv1? [y/n]: y
>   Converted vg/lv1 to cache pool.
> # lvconvert --cachepool vg/lv1 vg/lv2
>   Physical Volume "vg/lv2" not found in Volume Group "vg".
> 
> 
> Is this going to be fixed too? Do we need another BZ to track this?

These will get fixed.

'lvconvert' supports 'implict' deduction of type conversion (just like all other lvm2 commands and this support will not disappear.)

So this is just a clear bug in command line arg parser - which needs to 'take and compare' free-arg vg/lv  and avoid to take this arg as PV name.

But it's clearly not a regression - it's old existing bug - so please open new BZ for it.

Comment 14 Roman Bednář 2016-08-08 10:34:52 UTC
Marking as verified since the issue mentioned in Comment 1 has been solved. New BZ to track the mentioned issue with 'implicit' argument deduction has been created (BZ #1364994)



3.10.0-475.el7.x86_64

lvm2-2.02.161-2.el7    BUILT: Wed Jul 20 14:48:14 CEST 2016
lvm2-libs-2.02.161-2.el7    BUILT: Wed Jul 20 14:48:14 CEST 2016
lvm2-cluster-2.02.161-2.el7    BUILT: Wed Jul 20 14:48:14 CEST 2016
device-mapper-1.02.131-2.el7    BUILT: Wed Jul 20 14:48:14 CEST 2016
device-mapper-libs-1.02.131-2.el7    BUILT: Wed Jul 20 14:48:14 CEST 2016
device-mapper-event-1.02.131-2.el7    BUILT: Wed Jul 20 14:48:14 CEST 2016
device-mapper-event-libs-1.02.131-2.el7    BUILT: Wed Jul 20 14:48:14 CEST 2016
device-mapper-persistent-data-0.6.3-1.el7    BUILT: Fri Jul 22 12:29:13 CEST 2016
cmirror-2.02.161-2.el7    BUILT: Wed Jul 20 14:48:14 CEST 2016

Comment 15 Roman Bednář 2016-08-08 10:41:09 UTC
Pasted wrong package list, verified with:

3.10.0-481.el7.x86_64

lvm2-2.02.162-1.el7    BUILT: Fri Jul 29 02:26:36 CDT 2016
lvm2-libs-2.02.162-1.el7    BUILT: Fri Jul 29 02:26:36 CDT 2016
lvm2-cluster-2.02.162-1.el7    BUILT: Fri Jul 29 02:26:36 CDT 2016
device-mapper-1.02.132-1.el7    BUILT: Fri Jul 29 02:26:36 CDT 2016
device-mapper-libs-1.02.132-1.el7    BUILT: Fri Jul 29 02:26:36 CDT 2016
device-mapper-event-1.02.132-1.el7    BUILT: Fri Jul 29 02:26:36 CDT 2016
device-mapper-event-libs-1.02.132-1.el7    BUILT: Fri Jul 29 02:26:36 CDT 2016
device-mapper-persistent-data-0.6.3-1.el7    BUILT: Fri Jul 22 05:29:13 CDT 2016
cmirror-2.02.162-1.el7    BUILT: Fri Jul 29 02:26:36 CDT 2016

Comment 17 errata-xmlrpc 2016-11-04 04:22:10 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2016-1445.html


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