Hey all,
This patchset should make blivet (and therefore anaconda) suppress the creation of LVM metadata backup/archive files during image installs.
The first patch refactors devicelibs.lvm so that every call to the LVM binary goes through _getConfigArgs().
The second patch adds an 'autobackup' attribute, which disables metadata backup by having _getConfigArgs() add 'backup {backup=0 archive=0}' to the LVM config (via --config) if 'autobackup' is False. (It also adds a test for this to the existing LVM unit tests.)
The last patch makes Platform.update_from_flags() set autobackup=False if flags.image_install is set. We *could* set autobackup=False from inside anaconda, but update_from_flags() gets called from storageInitialize(), which is called by any anaconda process that uses blivet, so I'm pretty sure that will correctly disable autobackups during anaconda image installs.
The total diffstat is as follows:
blivet/devicelibs/lvm.py | 98 +++++++++++++++++---------------------- blivet/platform.py | 3 ++ tests/devicelibs_test/lvm_test.py | 21 +++++++++ 3 files changed, 66 insertions(+), 56 deletions(-)
'make test' shows no new failures after adding this code.
Thanks for your time and consideration,
-w
Every place in devicelibs.lvm that executes lvm uses _getConfigArgs()[1] to help build the argument list. For the sake of consistency and simplicity, we could just call _getConfigArgs() from inside lvm().
(If we add a lvm_output() function to do the same for informational commands, then every call to the lvm binary uses _getConfigArgs().)
This also means we can pass the caller-requested arguments to _getConfigArgs(), and then it can examine them and automatically add appropriate config options.
For example: right now, the caller has to explicitly request read_only_locking whenever we're doing a call to one of the informational commands (lvs, pvs, vgs). This change lets _getConfigArgs() figure that out and add the arguments for read-only locking whenever it's appropriate.
[1] except thinsnapshotcreate(), but that was an oversight (which is therefore fixed by this commit) --- blivet/devicelibs/lvm.py | 92 +++++++++++++++++++----------------------------- 1 file changed, 36 insertions(+), 56 deletions(-)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 87d2639..949532c 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -47,6 +47,9 @@ LVM_THINP_MAX_CHUNK_SIZE = Size("1 GiB")
RAID_levels = raid.RAIDLevels(["raid0", "raid1", "linear"])
+# These LVM commands are read-only, so we can run them with read-only locking +READONLY_COMMANDS = ('lvs', 'pvs', 'vgs') + def has_lvm(): if util.find_program_in_path("lvm"): for line in open("/proc/devices").readlines(): @@ -63,12 +66,11 @@ 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. """ + cmd = args[0] config_args = []
- read_only_locking = kwargs.get("read_only_locking", False) - filter_string = "" rejects = config_args_data["filterRejects"] for reject in rejects: @@ -87,7 +89,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] @@ -205,17 +207,19 @@ def is_valid_thin_pool_chunk_size(size, discard=False): return (size % LVM_THINP_MIN_CHUNK_SIZE == 0)
def lvm(args): + args += _getConfigArgs(args) ret = util.run_program(["lvm"] + args) if ret: raise LVMError("running lvm " + " ".join(args) + " failed")
+def lvm_output(args): + args += _getConfigArgs(args) + return util.capture_output(["lvm"] + args) + def pvcreate(device): # we force dataalignment=1024k since we cannot get lvm to tell us what # the pe_start will be in advance - args = ["pvcreate"] + \ - _getConfigArgs() + \ - ["--dataalignment", "1024k"] + \ - [device] + args = ["pvcreate", "--dataalignment", "1024k", device]
try: lvm(args) @@ -223,10 +227,9 @@ def pvcreate(device): 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) @@ -234,9 +237,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) @@ -244,9 +245,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) @@ -259,7 +258,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)
@@ -304,13 +303,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_output(args) pvs = {} for line in buf.splitlines(): info = parse_lvm_vars(line) @@ -326,7 +324,6 @@ def vgcreate(vg_name, pv_list, pe_size): argv = ["vgcreate"] if pe_size: argv.extend(["-s", "%dm" % pe_size.convertTo(spec="mib")]) - argv.extend(_getConfigArgs()) argv.append(vg_name) argv.extend(pv_list)
@@ -336,9 +333,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) @@ -346,9 +341,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) @@ -356,9 +349,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) @@ -380,7 +371,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: @@ -397,7 +387,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) @@ -412,10 +402,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_output(args) info = parse_lvm_vars(buf) if len(info.keys()) != 7: raise LVMError(_("vginfo failed for %s") % vg_name) @@ -434,12 +424,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_output(args) logvols = {} for line in buf.splitlines(): info = parse_lvm_vars(line) @@ -454,10 +443,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_output(args)
try: origin = buf.splitlines()[0].strip() @@ -473,7 +461,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: @@ -486,7 +473,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) @@ -496,7 +483,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: @@ -510,7 +496,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) @@ -519,7 +505,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: @@ -536,8 +521,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) @@ -552,7 +536,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) @@ -571,8 +555,6 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None): # default unit is KiB args += ["--chunksize", "%d" % chunksize.convertTo(spec="kib")]
- args += _getConfigArgs() - try: lvm(args) except LVMError as msg: @@ -581,8 +563,7 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None): 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) @@ -606,10 +587,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_output(args)
try: pool = buf.splitlines()[0].strip()
----- Original Message -----
From: "Will Woods" wwoods@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Friday, September 19, 2014 6:18:36 PM Subject: [PATCH blivet/master 1/3] devicelibs.lvm: refactor _getConfigArgs()/lvm()
Every place in devicelibs.lvm that executes lvm uses _getConfigArgs()[1] to help build the argument list. For the sake of consistency and simplicity, we could just call _getConfigArgs() from inside lvm().
(If we add a lvm_output() function to do the same for informational commands, then every call to the lvm binary uses _getConfigArgs().)
This also means we can pass the caller-requested arguments to _getConfigArgs(), and then it can examine them and automatically add appropriate config options.
For example: right now, the caller has to explicitly request read_only_locking whenever we're doing a call to one of the informational commands (lvs, pvs, vgs). This change lets _getConfigArgs() figure that out and add the arguments for read-only locking whenever it's appropriate.
[1] except thinsnapshotcreate(), but that was an oversight (which is therefore fixed by this commit)
blivet/devicelibs/lvm.py | 92 +++++++++++++++++++----------------------------- 1 file changed, 36 insertions(+), 56 deletions(-)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 87d2639..949532c 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -47,6 +47,9 @@ LVM_THINP_MAX_CHUNK_SIZE = Size("1 GiB")
RAID_levels = raid.RAIDLevels(["raid0", "raid1", "linear"])
+# These LVM commands are read-only, so we can run them with read-only locking +READONLY_COMMANDS = ('lvs', 'pvs', 'vgs')
def has_lvm(): if util.find_program_in_path("lvm"): for line in open("/proc/devices").readlines(): @@ -63,12 +66,11 @@ 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. """
- cmd = args[0] config_args = []
- read_only_locking = kwargs.get("read_only_locking", False)
- filter_string = "" rejects = config_args_data["filterRejects"] for reject in rejects:
@@ -87,7 +89,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]
@@ -205,17 +207,19 @@ def is_valid_thin_pool_chunk_size(size, discard=False): return (size % LVM_THINP_MIN_CHUNK_SIZE == 0)
def lvm(args):
- args += _getConfigArgs(args) ret = util.run_program(["lvm"] + args) if ret: raise LVMError("running lvm " + " ".join(args) + " failed")
+def lvm_output(args):
- args += _getConfigArgs(args)
- return util.capture_output(["lvm"] + args)
def pvcreate(device): # we force dataalignment=1024k since we cannot get lvm to tell us what # the pe_start will be in advance
- args = ["pvcreate"] + \
_getConfigArgs() + \
["--dataalignment", "1024k"] + \
[device]
args = ["pvcreate", "--dataalignment", "1024k", device]
try: lvm(args)
@@ -223,10 +227,9 @@ def pvcreate(device): 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)
@@ -234,9 +237,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)
@@ -244,9 +245,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)
@@ -259,7 +258,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)
@@ -304,13 +303,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_output(args) pvs = {} for line in buf.splitlines(): info = parse_lvm_vars(line)
@@ -326,7 +324,6 @@ def vgcreate(vg_name, pv_list, pe_size): argv = ["vgcreate"] if pe_size: argv.extend(["-s", "%dm" % pe_size.convertTo(spec="mib")])
- argv.extend(_getConfigArgs()) argv.append(vg_name) argv.extend(pv_list)
@@ -336,9 +333,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)
@@ -346,9 +341,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)
@@ -356,9 +349,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)
@@ -380,7 +371,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:
@@ -397,7 +387,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)
@@ -412,10 +402,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_output(args) info = parse_lvm_vars(buf) if len(info.keys()) != 7: raise LVMError(_("vginfo failed for %s") % vg_name)
@@ -434,12 +424,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)
if vg_name: args.append(vg_name)"-ovg_name,lv_name,lv_uuid,lv_size,lv_attr,segtype"]
- buf = util.capture_output(["lvm"] + args)
- buf = lvm_output(args) logvols = {} for line in buf.splitlines(): info = parse_lvm_vars(line)
@@ -454,10 +443,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_output(args)
try: origin = buf.splitlines()[0].strip()
@@ -473,7 +461,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:
@@ -486,7 +473,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)
@@ -496,7 +483,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:
@@ -510,7 +496,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)
@@ -519,7 +505,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:
@@ -536,8 +521,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)
@@ -552,7 +536,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)
@@ -571,8 +555,6 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None): # default unit is KiB args += ["--chunksize", "%d" % chunksize.convertTo(spec="kib")]
- args += _getConfigArgs()
- try: lvm(args) except LVMError as msg:
@@ -581,8 +563,7 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None): 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)
@@ -606,10 +587,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_output(args)
try: pool = buf.splitlines()[0].strip()
-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Moving _getConfigArgs() into lvm seems fine with me.
But, better to extend lvm() in a principled manner than to invent new method lvm_output. Seems more consistent w/ idea of moving _getConfigArgs() call into lvm(), anyway. See example in mdraid.py.
Fine w/ READONLY_COMMANDS change, generally, but think variable READONLY_COMMANDS should be local to _getConfigArgs(), not file global.
Please make sure to verify relevant tests succeed, you need to be root to run the majority.
- mulhern
On Mon, 2014-09-22 at 08:14 -0400, Anne Mulhern wrote:
Moving _getConfigArgs() into lvm seems fine with me.
But, better to extend lvm() in a principled manner than to invent new method lvm_output. Seems more consistent w/ idea of moving _getConfigArgs() call into lvm(), anyway. See example in mdraid.py.
I'd been thinking of them as analogues to subprocess.{call,check_output} but yeah, the mdraid() approach makes sense.
Fine w/ READONLY_COMMANDS change, generally, but think variable READONLY_COMMANDS should be local to _getConfigArgs(), not file global.
I... generally define those outside the scope of the function as a (possibly trivial) optimization, i.e. initialize the list once rather than on every function call? I wonder if the python interpreter optimizes that out?
Please make sure to verify relevant tests succeed, you need to be root to run the majority.
Yup - I mentioned that in the initial email:
'make test' shows no new failures after adding this code.
-w
----- Original Message -----
From: "Will Woods" wwoods@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Tuesday, September 23, 2014 12:25:38 PM Subject: Re: [PATCH blivet/master 1/3] devicelibs.lvm: refactor _getConfigArgs()/lvm()
On Mon, 2014-09-22 at 08:14 -0400, Anne Mulhern wrote:
Moving _getConfigArgs() into lvm seems fine with me.
But, better to extend lvm() in a principled manner than to invent new method lvm_output. Seems more consistent w/ idea of moving _getConfigArgs() call into lvm(), anyway. See example in mdraid.py.
I'd been thinking of them as analogues to subprocess.{call,check_output} but yeah, the mdraid() approach makes sense.
Fine w/ READONLY_COMMANDS change, generally, but think variable READONLY_COMMANDS should be local to _getConfigArgs(), not file global.
I... generally define those outside the scope of the function as a (possibly trivial) optimization, i.e. initialize the list once rather than on every function call? I wonder if the python interpreter optimizes that out?
The example below indicates no:
Source:
GLOBAL_CONSTANT = ('zippy', 'zoopy')
def junk():
CONSTANT = ('zippy', 'zoopy')
return CONSTANT[1]
def junk1():
return GLOBAL_CONSTANT[1]
Assembly:
dis(junk)
Disassembly of junk: 5 0 LOAD_CONST 4 (('zippy', 'zoopy')) 3 STORE_FAST 0 (CONSTANT)
7 6 LOAD_FAST 0 (CONSTANT) 9 LOAD_CONST 3 (1) 12 BINARY_SUBSCR 13 RETURN_VALUE
Disassembly of junk1: 11 0 LOAD_GLOBAL 0 (GLOBAL_CONSTANT) 3 LOAD_CONST 1 (1) 6 BINARY_SUBSCR 7 RETURN_VALUE
But in the context of every crazy expensive operation that blivet performs over and over again, trivial -> nugatory. You definitely should not be making that decision for performance reasons.
Using a tuple, instead of a list, though, turns out to be useful, because the list must be built item by item, but the tuple seems to come preassembled in the source.
So, defining the tuple globally could actually be a deoptimization, as loading a global might be more expensive than messing with a locally defined constant. Who knows, really?
On the doctrine of the least strength and smallest scope that gets what you need done (to which I subscribe, clearly), READONLY_COMMANDS defined as a tuple in the method wins.
Please make sure to verify relevant tests succeed, you need to be root to run the majority.
Yup - I mentioned that in the initial email:
'make test' shows no new failures after adding this code.
Yeah, thanks for doing that.
-w
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern
This adds the 'autobackup' attribute, 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).
Also, extend the test case so it tests this functionality. --- blivet/devicelibs/lvm.py | 6 ++++++ tests/devicelibs_test/lvm_test.py | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 949532c..df4746a 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -50,6 +50,10 @@ RAID_levels = raid.RAIDLevels(["raid0", "raid1", "linear"]) # These LVM commands are read-only, so we can run them with read-only locking READONLY_COMMANDS = ('lvs', 'pvs', 'vgs')
+# This defines whether we want automatic metadata backup or not. +# You probably want this on unless you're doing image builds. +autobackup = True + def has_lvm(): if util.find_program_in_path("lvm"): for line in open("/proc/devices").readlines(): @@ -91,6 +95,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 autobackup: + config_string += "backup {backup=0 archive=0} " if config_string: config_args = ["--config", config_string] return config_args diff --git a/tests/devicelibs_test/lvm_test.py b/tests/devicelibs_test/lvm_test.py index b7000ea..33f0185 100755 --- a/tests/devicelibs_test/lvm_test.py +++ b/tests/devicelibs_test/lvm_test.py @@ -7,6 +7,8 @@ from blivet.errors import LVMError
from tests import loopbackedtestcase
+import glob + class LVMTestCase(unittest.TestCase):
def testGetPossiblePhysicalExtents(self): @@ -38,6 +40,9 @@ class LVMAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): self._vg_name = "test-vg" self._lv_name = "test-lv"
+ def _list_backups(self): + return set(glob.glob("/etc/lvm/archive/%s_*" % self._vg_name)) + def tearDown(self): """Destroy volume group.""" try: @@ -68,6 +73,7 @@ class LVMAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): def testLVM(self): _LOOP_DEV0 = self.loopDevices[0] _LOOP_DEV1 = self.loopDevices[1] + _old_backups = self._list_backups()
## ## pvcreate @@ -109,6 +115,16 @@ class LVMAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): lvm.vgcreate("another-vg", [_LOOP_DEV0], Size("5MiB"))
## + ## check for autobackup - should be some new stuff here + ## FIXME: skip if backups have been disabled by host + ## + _current_backups = self._list_backups() + self.assertNotEqual(_old_backups, _current_backups, + "lvm.autobackup enabled but no backups created") + _old_backups = _current_backups + lvm.autobackup = False + + ## ## vgdeactivate ## # pass @@ -279,5 +295,10 @@ class LVMAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): # pv already removed self.assertEqual(lvm.pvremove(_LOOP_DEV0), None)
+ ## + ## check if autobackup=False worked + ## + self.assertEqual(_old_backups, self._list_backups()) + if __name__ == "__main__": unittest.main()
----- Original Message -----
From: "Will Woods" wwoods@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Friday, September 19, 2014 6:18:37 PM Subject: [PATCH blivet/master 2/3] devicelibs.lvm: Add 'autobackup' attribute
This adds the 'autobackup' attribute, 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).
Also, extend the test case so it tests this functionality.
blivet/devicelibs/lvm.py | 6 ++++++ tests/devicelibs_test/lvm_test.py | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 949532c..df4746a 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -50,6 +50,10 @@ RAID_levels = raid.RAIDLevels(["raid0", "raid1", "linear"]) # These LVM commands are read-only, so we can run them with read-only locking READONLY_COMMANDS = ('lvs', 'pvs', 'vgs')
+# This defines whether we want automatic metadata backup or not. +# You probably want this on unless you're doing image builds. +autobackup = True
Probably belongs in flags.py.
def has_lvm(): if util.find_program_in_path("lvm"): for line in open("/proc/devices").readlines(): @@ -91,6 +95,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 autobackup:
if config_string: config_args = ["--config", config_string] return config_argsconfig_string += "backup {backup=0 archive=0} "
diff --git a/tests/devicelibs_test/lvm_test.py b/tests/devicelibs_test/lvm_test.py index b7000ea..33f0185 100755 --- a/tests/devicelibs_test/lvm_test.py +++ b/tests/devicelibs_test/lvm_test.py @@ -7,6 +7,8 @@ from blivet.errors import LVMError
from tests import loopbackedtestcase
+import glob
Would prefer this import with other Python imports, i.e., unittest import.
class LVMTestCase(unittest.TestCase):
def testGetPossiblePhysicalExtents(self):
@@ -38,6 +40,9 @@ class LVMAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): self._vg_name = "test-vg" self._lv_name = "test-lv"
- def _list_backups(self):
return set(glob.glob("/etc/lvm/archive/%s_*" % self._vg_name))
- def tearDown(self): """Destroy volume group.""" try:
@@ -68,6 +73,7 @@ class LVMAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): def testLVM(self): _LOOP_DEV0 = self.loopDevices[0] _LOOP_DEV1 = self.loopDevices[1]
_old_backups = self._list_backups() ## ## pvcreate
@@ -109,6 +115,16 @@ class LVMAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): lvm.vgcreate("another-vg", [_LOOP_DEV0], Size("5MiB"))
##
## check for autobackup - should be some new stuff here
## FIXME: skip if backups have been disabled by host
##
_current_backups = self._list_backups()
self.assertNotEqual(_old_backups, _current_backups,
"lvm.autobackup enabled but no backups created")
_old_backups = _current_backups
lvm.autobackup = False
## ## vgdeactivate ## # pass
@@ -279,5 +295,10 @@ class LVMAsRootTestCase(loopbackedtestcase.LoopBackedTestCase): # pv already removed self.assertEqual(lvm.pvremove(_LOOP_DEV0), None)
##
## check if autobackup=False worked
##
self.assertEqual(_old_backups, self._list_backups())
This test should be in a separate method. The existing method is already 200 lines long and you're testing something quite specific here.
if __name__ == "__main__": unittest.main() -- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern
On Mon, 2014-09-22 at 08:43 -0400, Anne Mulhern wrote:
+# This defines whether we want automatic metadata backup or not. +# You probably want this on unless you're doing image builds. +autobackup = True
Probably belongs in flags.py.
Reasonable.
+import glob
Would prefer this import with other Python imports, i.e., unittest import.
Also reasonable.
##
## check if autobackup=False worked
##
self.assertEqual(_old_backups, self._list_backups())
This test should be in a separate method. The existing method is already 200 lines long and you're testing something quite specific here.
Ugh. Yes, but that entire method should be like 17 separate tests. So should I refactor the whole thing, or just factor out my own test? Because to do that I'm gonna end up writing extra setup and teardown functions, and then I might as well just do the rest...
-w
----- Original Message -----
From: "Will Woods" wwoods@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Tuesday, September 23, 2014 12:37:22 PM Subject: Re: [PATCH blivet/master 2/3] devicelibs.lvm: Add 'autobackup' attribute
On Mon, 2014-09-22 at 08:43 -0400, Anne Mulhern wrote:
+# This defines whether we want automatic metadata backup or not. +# You probably want this on unless you're doing image builds. +autobackup = True
Probably belongs in flags.py.
Reasonable.
+import glob
Would prefer this import with other Python imports, i.e., unittest import.
Also reasonable.
##
## check if autobackup=False worked
##
self.assertEqual(_old_backups, self._list_backups())
This test should be in a separate method. The existing method is already 200 lines long and you're testing something quite specific here.
Ugh. Yes, but that entire method should be like 17 separate tests. So should I refactor the whole thing, or just factor out my own test? Because to do that I'm gonna end up writing extra setup and teardown functions, and then I might as well just do the rest...
Yes, that method is way too long.
If there's a bunch of extra setUp and teardown stuff that is necessary for what you're testing to even work that's stuff that is useful to make explicit in the tests.
-w
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern
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. --- blivet/platform.py | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/blivet/platform.py b/blivet/platform.py index 1ab09e4..5261bfa 100644 --- a/blivet/platform.py +++ b/blivet/platform.py @@ -71,6 +71,9 @@ class Platform(object): log.warn("GPT is not a supported disklabel on this platform. Using default " "disklabel %s instead.", self.defaultDiskLabelType)
+ if flags.image_install: + devicelibs.lvm.autobackup = False + def __call__(self): return self
----- Original Message -----
From: "Will Woods" wwoods@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Friday, September 19, 2014 6:18:38 PM Subject: [PATCH blivet/master 3/3] Disable LVM autobackup when doing image installs (#1066004)
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.
blivet/platform.py | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/blivet/platform.py b/blivet/platform.py index 1ab09e4..5261bfa 100644 --- a/blivet/platform.py +++ b/blivet/platform.py @@ -71,6 +71,9 @@ class Platform(object): log.warn("GPT is not a supported disklabel on this platform. Using default " "disklabel %s instead.", self.defaultDiskLabelType)
An explanatory comment right here, containing much the same info as the commit message, would be great.
if flags.image_install:
devicelibs.lvm.autobackup = False
- def __call__(self): return self
-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern
----- Original Message -----
From: "Anne Mulhern" amulhern@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Monday, September 22, 2014 8:19:29 AM Subject: Re: [PATCH blivet/master 3/3] Disable LVM autobackup when doing image installs (#1066004)
----- Original Message -----
From: "Will Woods" wwoods@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Friday, September 19, 2014 6:18:38 PM Subject: [PATCH blivet/master 3/3] Disable LVM autobackup when doing image installs (#1066004)
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.
blivet/platform.py | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/blivet/platform.py b/blivet/platform.py index 1ab09e4..5261bfa 100644 --- a/blivet/platform.py +++ b/blivet/platform.py @@ -71,6 +71,9 @@ class Platform(object): log.warn("GPT is not a supported disklabel on this platform. Using default " "disklabel %s instead.", self.defaultDiskLabelType)
An explanatory comment right here, containing much the same info as the commit message, would be great.
if flags.image_install:
devicelibs.lvm.autobackup = False
- def __call__(self): return self
-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Also, blivet has its own flags class, it might be better to make a flag in that class and use it in devicelibs/lvm.py than to make it there. There's already an allow_degraded_mdraid flag in flags.py, so there is already precedent for putting device type specific flags in flags.py.
- mulhern
anaconda-patches@lists.fedorahosted.org