Bug 1353759
Summary: | lvconvert flags that no longer work with cache due to 'type' changes in .160-1 | ||
---|---|---|---|
Product: | Red Hat Enterprise Linux 7 | Reporter: | Corey Marthaler <cmarthal> |
Component: | lvm2 | Assignee: | David Teigland <teigland> |
lvm2 sub component: | Command-line tools | QA Contact: | cluster-qe <cluster-qe> |
Status: | CLOSED ERRATA | Docs Contact: | |
Severity: | medium | ||
Priority: | medium | CC: | agk, heinzm, jbrassow, msnitzer, prajnoha, prockai, rbednar, teigland, thornber, zkabelac |
Version: | 7.3 | ||
Target Milestone: | rc | ||
Target Release: | --- | ||
Hardware: | x86_64 | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | lvm2-2.02.161-3.el7 | Doc Type: | If docs needed, set a value |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2016-11-04 04:22:10 UTC | Type: | Bug |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Corey Marthaler
2016-07-07 23:02:08 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 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 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. 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. (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. 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. Upstream patch should restore the functionality: https://www.redhat.com/archives/lvm-devel/2016-July/msg00198.html 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? > # 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.
(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. 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 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 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 |