New: [rhinstaller/blivet/pulls/241 rhel7-branch] suppress LVM autobackup during image installs (rhbz#1269144)
Somehow the patches that fixed [rhbz#1066004] did not make their way into the previous blivet master branch, which means they got lost when this branch got rebased.. or something like that.
These patches should fix [rhbz#1269144] (which is a clone of [rhbz#1066004]).
A little massaging was necessary to fix conflicts in the `devicelibs.lvm` refactor patch, but it *looks* correct, at least.
[rhbz#1066004]: https://bugzilla.redhat.com/show_bug.cgi?id=1066004 [rhbz#1269144]: https://bugzilla.redhat.com/show_bug.cgi?id=1269144
Added label: rhel7-branch.
From: Will Woods wwoods@redhat.com
args.extend(str) will treat the str as iterable and append each individual character to args; should be args.append(str).
(cherry picked from commit 0a3bf71fb5c5181e5dde0f03189a9015a9bfd41e)
Related: rhbz#1269144 --- blivet/devicelibs/lvm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 3d1bf71..00c5ec2 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -281,7 +281,7 @@ def pvmove(source, dest=None): """ args = ["pvmove"] + _getConfigArgs() + [source] if dest: - args.extend(dest) + args.append(dest)
try: lvm(args)
From: Will Woods wwoods@redhat.com
Factor out a LVMAsRootTestCaseBase so we can more easily add new functional test cases.
Also fix a buglet in tearDown where a failed pvremove() could make us exit the loop without finishing.
(cherry picked from commit aa0b60affda67bc1c45cf3c22229448156441f80)
Related: rhbz#1269144 --- tests/devicelibs_test/lvm_test.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/tests/devicelibs_test/lvm_test.py b/tests/devicelibs_test/lvm_test.py index 375bf32..9b79856 100755 --- a/tests/devicelibs_test/lvm_test.py +++ b/tests/devicelibs_test/lvm_test.py @@ -7,6 +7,8 @@
from tests import loopbackedtestcase
+# TODO: test cases for lvorigin, lvsnapshot*, thin* + class LVMTestCase(unittest.TestCase):
def testGetPossiblePhysicalExtents(self): @@ -30,11 +32,11 @@ def testClampSize(self): # call if the device is non-existant, and usually that exception is caught and # an LVMError is then raised, but not always.
-class LVMAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): +class LVMAsRootTestCaseBase(loopbackedtestcase.LoopBackedTestCase):
def __init__(self, methodName='runTest'): """Set up the structure of the volume group.""" - super(LVMAsRootTestCase, self).__init__(methodName=methodName) + super(LVMAsRootTestCaseBase, self).__init__(methodName=methodName) self._vg_name = "test-vg" self._lv_name = "test-lv"
@@ -57,14 +59,17 @@ def tearDown(self): except LVMError: pass
- try: - for dev in self.loopDevices: + for dev in self.loopDevices: + try: lvm.pvremove(dev) - except LVMError: - pass + except LVMError: + pass + + super(LVMAsRootTestCaseBase, self).tearDown() +
- super(LVMAsRootTestCase, self).tearDown()
+class LVMAsRootTestCase(LVMAsRootTestCaseBase): def testLVM(self): _LOOP_DEV0 = self.loopDevices[0] _LOOP_DEV1 = self.loopDevices[1]
From: Will Woods wwoods@redhat.com
Every place in devicelibs.lvm that executes lvm uses _getConfigArgs()[1] to help build the argument list. So it'd be cleaner and simpler if we made every call to the lvm binary go through lvm(), and then just called _getConfigArgs() from there.
This also means we can pass the caller-requested lvm command+arguments to _getConfigArgs(), so it can examine them and add appropriate config options for us.
For example: right now the caller has to explicitly request read_only_locking if it wants read-only locks. The current code does that for every call to the informational commands (lvs, pvs, vgs, etc.).
This change moves that logic into _getConfigArgs(), which now just automatically does read-only locking whenever we call one of the info commands. Much simpler!
Mechanically, this patch should only make the following changes:
* make _getConfigArgs() do read_only_locking automatically when needed * make lvm() call _getConfigArgs() for every lvm command * remove all other calls to _getConfigArgs() * remove all calls to the `lvm` binary that don't use lvm(): * add (capture=False, ignore_errors=False) kwargs to lvm * replace `util.capture_output(["lvm"] + args)` with `lvm(args, capture=True, ignore_errors=True)`
This should result in no actual behavioral changes.
[1] except thinsnapshotcreate(), but that was an oversight (which is therefore fixed by this commit)
(cherry picked from commit 6c24bb25fddd3333592ae37bc3379e25f08fc3f5)
Related: rhbz#1269144 --- blivet/devicelibs/lvm.py | 106 +++++++++++++++++++++-------------------------- 1 file changed, 48 insertions(+), 58 deletions(-)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 00c5ec2..4382993 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -74,11 +74,15 @@ def has_lvm(): config_args_data = { "filterRejects": [], # regular expressions to reject. "filterAccepts": [] } # regexp to accept
-def _getConfigArgs(**kwargs): +def _getConfigArgs(args): """lvm command accepts lvm.conf type arguments preceded by --config. """ - config_args = []
- read_only_locking = kwargs.get("read_only_locking", False) + # These commands are read-only, so we can run them with read-only locking. + # (not an exhaustive list, but these are the only ones used here) + READONLY_COMMANDS = ('lvs', 'pvs', 'vgs') + + cmd = args[0] + config_args = []
filter_string = "" rejects = config_args_data["filterRejects"] @@ -98,7 +102,7 @@ def _getConfigArgs(**kwargs): # "preferred_names", "filter", "cache_dir", "write_cache_state", # "types", "sysfs_scan", "md_component_detection". see man lvm.conf. config_string = " devices { %s } " % (devices_string) # strings can be added - if read_only_locking: + if cmd in READONLY_COMMANDS: config_string += "global {locking_type=4} " if config_string: config_args = ["--config", config_string] @@ -224,14 +228,24 @@ def strip_lvm_warnings(buf): """ return [l for l in buf.splitlines() if l and not l.lstrip().startswith("WARNING:")]
-def lvm(args): - ret = util.run_program(["lvm"] + args) - if ret: - raise LVMError("running lvm " + " ".join(args) + " failed") +def lvm(args, capture=False, ignore_errors=False): + """ Runs lvm with specified arguments. + + :param bool capture: if True, return the output of the command. + :param bool ignore_errors: if True, do not raise LVMError on failure + :returns: the output of the command or None + :rtype: str or NoneType + :raises: LVMError if command fails + """ + argv = ["lvm"] + args + _getConfigArgs(args) + (ret, out) = util.run_program_and_capture_output(argv) + if ret and not ignore_errors: + raise LVMError("running "+ " ".join(argv) + " failed") + if capture: + return out
def pvcreate(device, data_alignment=None): - args = ["pvcreate"] + \ - _getConfigArgs() + args = ["pvcreate"]
if data_alignment is not None: args.extend(["--dataalignment", "%dk" % data_alignment.convertTo(spec="KiB")]) @@ -243,10 +257,9 @@ def pvcreate(device, data_alignment=None): raise LVMError("pvcreate failed for %s: %s" % (device, msg))
def pvresize(device, size): - args = ["pvresize"] + \ - ["--setphysicalvolumesize", ("%dm" % size.convertTo(spec="mib"))] + \ - _getConfigArgs() + \ - [device] + args = ["pvresize", + "--setphysicalvolumesize", ("%dm" % size.convertTo(spec="mib")), + device]
try: lvm(args) @@ -254,9 +267,7 @@ def pvresize(device, size): raise LVMError("pvresize failed for %s: %s" % (device, msg))
def pvremove(device): - args = ["pvremove", "--force", "--force", "--yes"] + \ - _getConfigArgs() + \ - [device] + args = ["pvremove", "--force", "--force", "--yes", device]
try: lvm(args) @@ -264,9 +275,7 @@ def pvremove(device): raise LVMError("pvremove failed for %s: %s" % (device, msg))
def pvscan(device): - args = ["pvscan", "--cache",] + \ - _getConfigArgs() + \ - [device] + args = ["pvscan", "--cache", device]
try: lvm(args) @@ -279,7 +288,7 @@ def pvmove(source, dest=None): :param str source: pv device path to move extents off of :keyword str dest: pv device path to move the extents onto """ - args = ["pvmove"] + _getConfigArgs() + [source] + args = ["pvmove", source] if dest: args.append(dest)
@@ -324,13 +333,12 @@ def pvinfo(device=None): "--unit=k", "--nosuffix", "--nameprefixes", "--unquoted", "--noheadings", "-opv_name,pv_uuid,pe_start,vg_name,vg_uuid,vg_size,vg_free," - "vg_extent_size,vg_extent_count,vg_free_count,pv_count"] + \ - _getConfigArgs(read_only_locking=True) + "vg_extent_size,vg_extent_count,vg_free_count,pv_count"]
if device: args.append(device)
- buf = util.capture_output(["lvm"] + args) + buf = lvm(args, capture=True, ignore_errors=True) pvs = {} for line in buf.splitlines(): info = parse_lvm_vars(line) @@ -346,7 +354,6 @@ def vgcreate(vg_name, pv_list, pe_size): argv = ["vgcreate"] if pe_size: argv.extend(["-s", "%sk" % pe_size.convertTo(spec="KiB")]) - argv.extend(_getConfigArgs()) argv.append(vg_name) argv.extend(pv_list)
@@ -356,9 +363,7 @@ def vgcreate(vg_name, pv_list, pe_size): raise LVMError("vgcreate failed for %s: %s" % (vg_name, msg))
def vgremove(vg_name): - args = ["vgremove", "--force"] + \ - _getConfigArgs() +\ - [vg_name] + args = ["vgremove", "--force", vg_name]
try: lvm(args) @@ -366,9 +371,7 @@ def vgremove(vg_name): raise LVMError("vgremove failed for %s: %s" % (vg_name, msg))
def vgactivate(vg_name): - args = ["vgchange", "-a", "y"] + \ - _getConfigArgs() + \ - [vg_name] + args = ["vgchange", "-a", "y", vg_name]
try: lvm(args) @@ -376,9 +379,7 @@ def vgactivate(vg_name): raise LVMError("vgactivate failed for %s: %s" % (vg_name, msg))
def vgdeactivate(vg_name): - args = ["vgchange", "-a", "n"] + \ - _getConfigArgs() + \ - [vg_name] + args = ["vgchange", "-a", "n", vg_name]
try: lvm(args) @@ -400,7 +401,6 @@ def vgreduce(vg_name, pv, missing=False): it from the VG. You must do that first by calling :func:`.pvmove`. """ args = ["vgreduce"] - args.extend(_getConfigArgs()) if missing: args.extend(["--removemissing", "--force", vg_name]) else: @@ -417,7 +417,7 @@ def vgextend(vg_name, pv): :param str vg_name: the name of the VG :param str pv: device path of PV to add """ - args = ["vgextend"] + _getConfigArgs() + [vg_name, pv] + args = ["vgextend", vg_name, pv]
try: lvm(args) @@ -432,10 +432,10 @@ def vginfo(vg_name): """ args = ["vgs", "--noheadings", "--nosuffix", "--nameprefixes", "--unquoted", "--units", "m", - "-o", "uuid,size,free,extent_size,extent_count,free_count,pv_count"] + \ - _getConfigArgs(read_only_locking=True) + [vg_name] + "-o", "uuid,size,free,extent_size,extent_count,free_count,pv_count", + vg_name]
- buf = util.capture_output(["lvm"] + args) + buf = lvm(args, capture=True, ignore_errors=True) info = parse_lvm_vars(buf) if len(info.keys()) != 7: raise LVMError(_("vginfo failed for %s") % vg_name) @@ -454,12 +454,11 @@ def lvs(vg_name=None): args = ["lvs", "-a", "--unit", "k", "--nosuffix", "--nameprefixes", "--unquoted", "--noheadings", - "-ovg_name,lv_name,lv_uuid,lv_size,lv_attr,segtype"] + \ - _getConfigArgs(read_only_locking=True) + "-ovg_name,lv_name,lv_uuid,lv_size,lv_attr,segtype"] if vg_name: args.append(vg_name)
- buf = util.capture_output(["lvm"] + args) + buf = lvm(args, capture=True, ignore_errors=True) logvols = {} for line in buf.splitlines(): info = parse_lvm_vars(line) @@ -474,10 +473,9 @@ def lvs(vg_name=None):
def lvorigin(vg_name, lv_name): args = ["lvs", "--noheadings", "-o", "origin"] + \ - _getConfigArgs(read_only_locking=True) + \ ["%s/%s" % (vg_name, lv_name)]
- buf = util.capture_output(["lvm"] + args) + buf = lvm(args, capture=True, ignore_errors=True) lines = strip_lvm_warnings(buf) try: origin = lines[0].strip() @@ -493,7 +491,6 @@ def lvcreate(vg_name, lv_name, size, pvs=None): ["-L", "%dm" % size.convertTo(spec="mib")] + \ ["-n", lv_name] + \ ["-y"] + \ - _getConfigArgs() + \ [vg_name] + pvs
try: @@ -506,7 +503,7 @@ def lvremove(vg_name, lv_name, force=False): if force: args.extend(["--force", "--yes"])
- args += _getConfigArgs() + ["%s/%s" % (vg_name, lv_name)] + args += ["%s/%s" % (vg_name, lv_name)]
try: lvm(args) @@ -516,7 +513,6 @@ def lvremove(vg_name, lv_name, force=False): def lvresize(vg_name, lv_name, size): args = ["lvresize"] + \ ["--force", "-L", "%dm" % size.convertTo(spec="mib")] + \ - _getConfigArgs() + \ ["%s/%s" % (vg_name, lv_name)]
try: @@ -530,7 +526,7 @@ def lvactivate(vg_name, lv_name, ignore_skip=False): if ignore_skip: args.append("-K")
- args += _getConfigArgs() + ["%s/%s" % (vg_name, lv_name)] + args += ["%s/%s" % (vg_name, lv_name)]
try: lvm(args) @@ -539,7 +535,6 @@ def lvactivate(vg_name, lv_name, ignore_skip=False):
def lvdeactivate(vg_name, lv_name): args = ["lvchange", "-a", "n"] + \ - _getConfigArgs() + \ ["%s/%s" % (vg_name, lv_name)]
try: @@ -556,8 +551,7 @@ def lvsnapshotmerge(vg_name, lv_name): how merge is handled by lvm.
""" - args = ["lvconvert", "--merge", "%s/%s" % (vg_name, lv_name)] - args += _getConfigArgs() + [lv_name] + args = ["lvconvert", "--merge", "%s/%s" % (vg_name, lv_name), lv_name]
try: lvm(args) @@ -572,7 +566,7 @@ def lvsnapshotcreate(vg_name, snap_name, size, origin_name): :param str origin_name: the name of the origin logical volume """ args = ["lvcreate", "-s", "-L", "%dm" % size.convertTo(spec="MiB"), - "-n", snap_name] + _getConfigArgs() + ["%s/%s" % (vg_name, origin_name)] + "-n", snap_name, "%s/%s" % (vg_name, origin_name)]
try: lvm(args) @@ -594,8 +588,6 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None, pr if profile: args += ["--profile=%s" % profile]
- args += _getConfigArgs() - try: lvm(args) except LVMError as msg: @@ -604,8 +596,7 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None, pr def thinlvcreate(vg_name, pool_name, lv_name, size): args = ["lvcreate", "--thinpool", "%s/%s" % (vg_name, pool_name), "--virtualsize", "%dm" % size.convertTo(spec="MiB"), - "-n", lv_name] + \ - _getConfigArgs() + "-n", lv_name]
try: lvm(args) @@ -629,10 +620,9 @@ def thinsnapshotcreate(vg_name, snap_name, origin_name, pool_name=None):
def thinlvpoolname(vg_name, lv_name): args = ["lvs", "--noheadings", "-o", "pool_lv"] + \ - _getConfigArgs(read_only_locking=True) + \ ["%s/%s" % (vg_name, lv_name)]
- buf = util.capture_output(["lvm"] + args) + buf = lvm(args, capture=True, ignore_errors=True) lines = strip_lvm_warnings(buf) try: pool = lines[0].strip()
From: Will Woods wwoods@redhat.com
This adds a flag which - if set to False - will suppress the automatic backup of LVM metadata by adding the appropriate configuration options (backup.backup=0 and backup.archive=0) to all invocations of LVM.
It also adds a test case to ensure this actually works.
(cherry picked from commit 8cf6de18f1f332e5d3bd60bb0ca9fe8425172955)
Related: rhbz#1269144 --- blivet/devicelibs/lvm.py | 3 +++ blivet/flags.py | 4 ++++ tests/devicelibs_test/lvm_test.py | 27 +++++++++++++++++++++++++++ 3 files changed, 34 insertions(+)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 4382993..0df440a 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -35,6 +35,7 @@ from .. import arch from ..errors import LVMError from ..i18n import _, N_ +from ..flags import flags
MAX_LV_SLOTS = 256
@@ -104,6 +105,8 @@ def _getConfigArgs(args): config_string = " devices { %s } " % (devices_string) # strings can be added if cmd in READONLY_COMMANDS: config_string += "global {locking_type=4} " + if not flags.lvm_metadata_backup: + config_string += "backup {backup=0 archive=0} " if config_string: config_args = ["--config", config_string] return config_args diff --git a/blivet/flags.py b/blivet/flags.py index 459bd69..9236568 100644 --- a/blivet/flags.py +++ b/blivet/flags.py @@ -56,6 +56,10 @@ def __init__(self):
self.multipath_friendly_names = True
+ # set to False to suppress the default LVM behavior of saving + # backup metadata in /etc/lvm/{archive,backup} + self.lvm_metadata_backup = True + # whether to include nodev filesystems in the devicetree (only # meaningful when flags.installer_mode is False) self.include_nodev = False diff --git a/tests/devicelibs_test/lvm_test.py b/tests/devicelibs_test/lvm_test.py index 9b79856..f85e412 100755 --- a/tests/devicelibs_test/lvm_test.py +++ b/tests/devicelibs_test/lvm_test.py @@ -1,5 +1,6 @@ #!/usr/bin/python import unittest +import glob
import blivet.devicelibs.lvm as lvm from blivet.size import Size @@ -67,6 +68,32 @@ def tearDown(self):
super(LVMAsRootTestCaseBase, self).tearDown()
+class LVM_Metadata_Backup_TestCase(LVMAsRootTestCaseBase): + def _list_backups(self): + return set(glob.glob("/etc/lvm/archive/%s_*" % self._vg_name)) + + def setUp(self): + super(LVM_Metadata_Backup_TestCase, self).setUp() + self._old_backups = self._list_backups() + for dev in self.loopDevices: + lvm.pvcreate(dev) + + def test_backup_enabled(self): + lvm.flags.lvm_metadata_backup = True + lvm.vgcreate(self._vg_name, self.loopDevices, Size("4MiB")) + + current_backups = self._list_backups() + self.assertTrue(current_backups.issuperset(self._old_backups), + "old backups disappeared??") + self.assertTrue(current_backups.difference(self._old_backups), + "lvm_metadata_backup enabled but no backups created") + + def test_backup_disabled(self): + lvm.flags.lvm_metadata_backup = False + lvm.vgcreate(self._vg_name, self.loopDevices, Size("4MiB")) + + self.assertEqual(self._old_backups, self._list_backups(), + "lvm_metadata_backup disabled but backups created")
class LVMAsRootTestCase(LVMAsRootTestCaseBase):
From: Will Woods wwoods@redhat.com
If we're doing an image install, the host system doesn't really care about the metadata for these volume groups - and if you do a lot of image installs (say, on a build system) the archive/ directory is going to grow without bound.
So: suppress the autobackup behavior during image installs to prevent modifying things on the host.
(cherry picked from commit 817eeeb85b4a476e6f92f8646c027846f817904a)
Resolves: rhbz#1269144 --- blivet/flags.py | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/blivet/flags.py b/blivet/flags.py index 9236568..0ff1c96 100644 --- a/blivet/flags.py +++ b/blivet/flags.py @@ -110,4 +110,9 @@ def update_from_anaconda_flags(self, anaconda_flags): self.ibft = anaconda_flags.ibft self.dmraid = anaconda_flags.dmraid
+ # We don't want image installs writing backups of the *image* metadata + # into the *host's* /etc/lvm. This can get real messy on build systems. + if self.image_install: + self.lvm_metadata_backup = False + flags = Flags()
These all look good to me.
Added label: ACK.
Tested with image install and it works great, nothing written to /etc/
Closed.
anaconda-patches@lists.fedorahosted.org
-
dashea