The final goal for the bug #869456 is to add GUI support for specifying thin pool profile, but I'd like you to have a chance to judge these building blocks first.
Vratislav Podzimek (1): Add support for thin pool profile specification in kickstart
pyanaconda/kickstart.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
Related: rhbz#869456 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pyanaconda/kickstart.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py index 07d08e9..f345ff1 100644 --- a/pyanaconda/kickstart.py +++ b/pyanaconda/kickstart.py @@ -21,7 +21,7 @@ from pyanaconda.errors import ScriptError, errorHandler from blivet.deviceaction import ActionCreateFormat, ActionDestroyFormat, ActionResizeDevice, ActionResizeFormat from blivet.devices import LUKSDevice -from blivet.devicelibs.lvm import getPossiblePhysicalExtents, LVM_PE_SIZE +from blivet.devicelibs.lvm import getPossiblePhysicalExtents, LVM_PE_SIZE, KNOWN_THPOOL_PROFILES from blivet.devicelibs import swap as swap_lib from blivet.formats import getFormat from blivet.partitioning import doPartitioning @@ -744,7 +744,7 @@ class Lang(commands.lang.F19_Lang): # no overrides needed here Eula = commands.eula.F20_Eula
-class LogVol(commands.logvol.F20_LogVol): +class LogVol(commands.logvol.RHEL7_LogVol): def execute(self, storage, ksdata, instClass): for l in self.lvList: l.execute(storage, ksdata, instClass) @@ -752,7 +752,7 @@ class LogVol(commands.logvol.F20_LogVol): if self.lvList: growLVM(storage)
-class LogVolData(commands.logvol.F20_LogVolData): +class LogVolData(commands.logvol.RHEL7_LogVolData): def execute(self, storage, ksdata, instClass): devicetree = storage.devicetree
@@ -890,11 +890,19 @@ class LogVolData(commands.logvol.F20_LogVolData): else: parents = [vg]
+ pool_args = {} if self.thin_pool: - pool_args = { "metadatasize": Size("%d MiB" % self.metadata_size), - "chunksize": Size("%d KiB" % self.chunk_size) } - else: - pool_args = {} + if self.profile: + matching = (p for p in KNOWN_THPOOL_PROFILES if p.name == self.profile) + profile = next(matching, None) + if profile: + pool_args["profile"] = profile + + if self.metadata_size: + pool_args["metadatasize"] = Size("%d MiB" % self.metadata_size) + + if self.chunk_size: + pool_args["chunksize"] = Size("%d KiB" % self.chunk_size)
if self.maxSizeMB: try:
Related: rhbz#869456 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pykickstart/commands/logvol.py | 30 ++++++++++++++++++++++++++++++ pykickstart/handlers/control.py | 4 ++-- tests/commands/logvol.py | 10 ++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/pykickstart/commands/logvol.py b/pykickstart/commands/logvol.py index 6122f42..eae1f79 100644 --- a/pykickstart/commands/logvol.py +++ b/pykickstart/commands/logvol.py @@ -261,6 +261,19 @@ class F20_LogVolData(F18_LogVolData):
return retval
+class RHEL7_LogVolData(F20_LogVolData): + def __init__(self, *args, **kwargs): + F20_LogVolData.__init__(self, *args, **kwargs) + self.profile = kwargs.get("profile", "") + + def _getArgsAsStr(self): + retval = F20_LogVolData._getArgsAsStr(self) + + if self.profile: + retval += " --profile=%s" % self.profile + + return retval + class FC3_LogVol(KickstartCommand): removedKeywords = KickstartCommand.removedKeywords removedAttrs = KickstartCommand.removedAttrs @@ -473,3 +486,20 @@ class F20_LogVol(F18_LogVol): raise KickstartParseError(formatErrorMsg(self.lineno, msg=errorMsg))
return retval + +class RHEL7_LogVol(F20_LogVol): + def _getParser(self): + op = F20_LogVol._getParser(self) + op.add_option("--profile") + + return op + + def parse(self, args): + retval = F20_LogVol.parse(self, args) + + if not retval.thin_pool and retval.profile: + err = formatErrorMsg(self.lineno, + msg=_("--profile can only be used together with --thinpool")) + raise KickstartParseError(err) + + return retval diff --git a/pykickstart/handlers/control.py b/pykickstart/handlers/control.py index 53e0378..2738aeb 100644 --- a/pykickstart/handlers/control.py +++ b/pykickstart/handlers/control.py @@ -1343,7 +1343,7 @@ commandMap = { "lang": lang.F19_Lang, "liveimg": liveimg.F19_Liveimg, "logging": logging.FC6_Logging, - "logvol": logvol.F20_LogVol, + "logvol": logvol.RHEL7_LogVol, "mediacheck": mediacheck.FC4_MediaCheck, "method": method.F19_Method, "multipath": multipath.FC6_MultiPath, @@ -1723,7 +1723,7 @@ dataMap = { "FcoeData": fcoe.F13_FcoeData, "GroupData": group.F12_GroupData, "IscsiData": iscsi.F17_IscsiData, - "LogVolData": logvol.F20_LogVolData, + "LogVolData": logvol.RHEL7_LogVolData, "MultiPathData": multipath.FC6_MultiPathData, "NetworkData": network.RHEL7_NetworkData, "PartData": partition.F18_PartData, diff --git a/tests/commands/logvol.py b/tests/commands/logvol.py index 989ef2c..a3a5ff0 100644 --- a/tests/commands/logvol.py +++ b/tests/commands/logvol.py @@ -276,6 +276,16 @@ class F20_TestCase(F18_TestCase): self.assert_parse_error("logvol none --name=pool1 --vgname=vg " "--chunksize=512")
+class RHEL7_TestCase(F20_TestCase): + def runTest(self): + F20_TestCase.runTest(self) + + self.assert_parse("logvol none --name=pool1 --vgname=vg --thinpool --profile=performance", + "logvol none --thinpool --profile=performance --name=pool1 --vgname=vg\n") + + # --profile can only be specified for the pool + self.assert_parse_error("logvol /home --name=home --vgname=vg " + "--thin --poolname=pool1 --profile=performance")
if __name__ == "__main__": unittest.main()
+class RHEL7_LogVolData(F20_LogVolData):
- def __init__(self, *args, **kwargs):
F20_LogVolData.__init__(self, *args, **kwargs)self.profile = kwargs.get("profile", "")- def _getArgsAsStr(self):
retval = F20_LogVolData._getArgsAsStr(self)if self.profile:retval += " --profile=%s" % self.profilereturn retval
Is it possible we will one day want to add something else to the logvol command that could be named "profile"? I feel the current option name is too generic and we'll regret it later.
Aside from that, the pykickstart portion looks fine.
- Chris
On Fri, 2014-09-19 at 10:37 -0400, Chris Lumens wrote:
+class RHEL7_LogVolData(F20_LogVolData):
- def __init__(self, *args, **kwargs):
F20_LogVolData.__init__(self, *args, **kwargs)self.profile = kwargs.get("profile", "")- def _getArgsAsStr(self):
retval = F20_LogVolData._getArgsAsStr(self)if self.profile:retval += " --profile=%s" % self.profilereturn retvalIs it possible we will one day want to add something else to the logvol command that could be named "profile"? I feel the current option name is too generic and we'll regret it later.
Not much possible, as the '--profile' option is a generic option for the 'lvm' command documented in lvm (8). Thus I think it won't be ever used for anything else.
Is it possible we will one day want to add something else to the logvol command that could be named "profile"? I feel the current option name is too generic and we'll regret it later.
Not much possible, as the '--profile' option is a generic option for the 'lvm' command documented in lvm (8). Thus I think it won't be ever used for anything else.
Definitely - profile is something used by LVM. However the man page doesn't say it is specific to thinp support. The pykickstart patch here makes it an error to use --profile without --thinpool. What I am wondering is if that's too strict. If the intention is to have a --profile that must be used with --thinpool, perhaps the option should be named differently. On the other hand, if we will later use --profile for some other LVM-related purpose, perhaps the check for --thinpool also being specified does not need to be there.
- Chris
On Mon, 2014-09-22 at 11:10 -0400, Chris Lumens wrote:
Is it possible we will one day want to add something else to the logvol command that could be named "profile"? I feel the current option name is too generic and we'll regret it later.
Not much possible, as the '--profile' option is a generic option for the 'lvm' command documented in lvm (8). Thus I think it won't be ever used for anything else.
Definitely - profile is something used by LVM. However the man page doesn't say it is specific to thinp support. The pykickstart patch here makes it an error to use --profile without --thinpool. What I am wondering is if that's too strict.
Now I get what you mean. Yes, you're right, it is too strict I think. AFAICT the --profile option is only used for thin pools, but lvm (8) doesn't specify that strictly so it may be used for other things as well in the future. I'm dropping that requirement from the patch.
Profile definitions are needed e.g. for creation of thin pools with specific profile.
Related: rhbz#869456 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- share/runtime-postinstall.tmpl | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/share/runtime-postinstall.tmpl b/share/runtime-postinstall.tmpl index fb13b8d..bdba76a 100644 --- a/share/runtime-postinstall.tmpl +++ b/share/runtime-postinstall.tmpl @@ -115,7 +115,12 @@ append etc/depmod.d/dd.conf "search updates built-in" append etc/multipath.conf "defaults {\n\tfind_multipaths yes\n\tuser_friendly_names yes\n}\n"
## make lvm auto-activate -remove etc/lvm/* +remove etc/lvm/archive/* +remove etc/lvm/archive +remove etc/lvm/backup/* +remove etc/lvm/backup +remove etc/lvm/cache/* +remove etc/lvm/cache append etc/lvm/lvm.conf "global {\n\tuse_lvmetad = 1\n}\n"
## TODO: we could run prelink here if we wanted?
Related: rhbz#869456 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- blivet/devicelibs/lvm.py | 12 ++++++++++-- blivet/devices.py | 10 ++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 426794f..e3b7ca9 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -22,6 +22,7 @@
import math from decimal import Decimal +from collections import namedtuple
import logging log = logging.getLogger("blivet") @@ -31,7 +32,7 @@ from ..size import Size from .. import util from .. import arch from ..errors import LVMError -from ..i18n import _ +from ..i18n import _, N_
MAX_LV_SLOTS = 256
@@ -47,6 +48,10 @@ LVM_THINP_MAX_CHUNK_SIZE = Size("1 GiB")
RAID_levels = raid.RAIDLevels(["raid0", "raid1", "linear"])
+ThPoolProfile = namedtuple("ThPoolProfile", ["name", "desc"]) +KNOWN_THPOOL_PROFILES = (ThPoolProfile("thin-generic", N_("Generic")), + ThPoolProfile("thin-performance", N_("Performance"))) + def has_lvm(): if util.find_program_in_path("lvm"): for line in open("/proc/devices").readlines(): @@ -559,7 +564,7 @@ def lvsnapshotcreate(vg_name, snap_name, size, origin_name): except LVMError as msg: raise LVMError("lvsnapshotcreate failed for %s/%s: %s" % (vg_name, snap_name, msg))
-def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None): +def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None, profile=None): args = ["lvcreate", "--thinpool", "%s/%s" % (vg_name, lv_name), "--size", "%dm" % size.convertTo(spec="mib")]
@@ -571,6 +576,9 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None): # default unit is KiB args += ["--chunksize", "%d" % chunksize.convertTo(spec="kib")]
+ if profile: + args += ["--profile=%s" % profile] + args += _getConfigArgs()
try: diff --git a/blivet/devices.py b/blivet/devices.py index 26dfff8..9cd6eb6 100644 --- a/blivet/devices.py +++ b/blivet/devices.py @@ -3441,7 +3441,7 @@ class LVMThinPoolDevice(LVMLogicalVolumeDevice): def __init__(self, name, parents=None, size=None, uuid=None, fmt=None, exists=False, sysfsPath='', grow=None, maxsize=None, percent=None, - metadatasize=None, chunksize=None, segType=None): + metadatasize=None, chunksize=None, segType=None, profile=None): """ :param name: the device name (generally a device node's basename) :type name: str @@ -3472,6 +3472,9 @@ class LVMThinPoolDevice(LVMLogicalVolumeDevice): :type metadatasize: :class:`~.size.Size` :keyword chunksize: chunk size for the pool :type chunksize: :class:`~.size.Size` + :keyword profile: (allocation) profile for the pool or None (unspecified) + :type profile: :class:`~.devicelibs.lvm.ThPoolProfile` or NoneType + """ if metadatasize is not None and \ not lvm.is_valid_thin_pool_metadata_size(metadatasize): @@ -3491,6 +3494,7 @@ class LVMThinPoolDevice(LVMLogicalVolumeDevice):
self.metaDataSize = metadatasize or 0 self.chunkSize = chunksize or 0 + self.profile = profile self._lvs = []
def _addLogVol(self, lv): @@ -3536,7 +3540,8 @@ class LVMThinPoolDevice(LVMLogicalVolumeDevice): # TODO: chunk size, data/metadata split --> profile lvm.thinpoolcreate(self.vg.name, self.lvname, self.size, metadatasize=self.metaDataSize, - chunksize=self.chunkSize) + chunksize=self.chunkSize, + profile=self.profile)
def dracutSetupArgs(self): return set() @@ -3552,6 +3557,7 @@ class LVMThinPoolDevice(LVMLogicalVolumeDevice): data.thin_pool = True data.metadata_size = self.metaDataSize data.chunk_size = self.chunkSize + data.profile = self.profile
class LVMThinLogicalVolumeDevice(LVMLogicalVolumeDevice): """ An LVM Thin Logical Volume """
On Fri, 2014-09-19 at 15:47 +0200, Vratislav Podzimek wrote:
Related: rhbz#869456 Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
blivet/devicelibs/lvm.py | 12 ++++++++++-- blivet/devices.py | 10 ++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/blivet/devicelibs/lvm.py b/blivet/devicelibs/lvm.py index 426794f..e3b7ca9 100644 --- a/blivet/devicelibs/lvm.py +++ b/blivet/devicelibs/lvm.py @@ -22,6 +22,7 @@
import math from decimal import Decimal +from collections import namedtuple
import logging log = logging.getLogger("blivet") @@ -31,7 +32,7 @@ from ..size import Size from .. import util from .. import arch from ..errors import LVMError -from ..i18n import _ +from ..i18n import _, N_
MAX_LV_SLOTS = 256
@@ -47,6 +48,10 @@ LVM_THINP_MAX_CHUNK_SIZE = Size("1 GiB")
RAID_levels = raid.RAIDLevels(["raid0", "raid1", "linear"])
+ThPoolProfile = namedtuple("ThPoolProfile", ["name", "desc"]) +KNOWN_THPOOL_PROFILES = (ThPoolProfile("thin-generic", N_("Generic")),
ThPoolProfile("thin-performance", N_("Performance")))def has_lvm(): if util.find_program_in_path("lvm"): for line in open("/proc/devices").readlines(): @@ -559,7 +564,7 @@ def lvsnapshotcreate(vg_name, snap_name, size, origin_name): except LVMError as msg: raise LVMError("lvsnapshotcreate failed for %s/%s: %s" % (vg_name, snap_name, msg))
-def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None): +def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None, profile=None): args = ["lvcreate", "--thinpool", "%s/%s" % (vg_name, lv_name), "--size", "%dm" % size.convertTo(spec="mib")]
@@ -571,6 +576,9 @@ def thinpoolcreate(vg_name, lv_name, size, metadatasize=None, chunksize=None): # default unit is KiB args += ["--chunksize", "%d" % chunksize.convertTo(spec="kib")]
if profile:
args += ["--profile=%s" % profile]args += _getConfigArgs()
try:
diff --git a/blivet/devices.py b/blivet/devices.py index 26dfff8..9cd6eb6 100644 --- a/blivet/devices.py +++ b/blivet/devices.py @@ -3441,7 +3441,7 @@ class LVMThinPoolDevice(LVMLogicalVolumeDevice): def __init__(self, name, parents=None, size=None, uuid=None, fmt=None, exists=False, sysfsPath='', grow=None, maxsize=None, percent=None,
metadatasize=None, chunksize=None, segType=None):
metadatasize=None, chunksize=None, segType=None, profile=None): """ :param name: the device name (generally a device node's basename) :type name: str@@ -3472,6 +3472,9 @@ class LVMThinPoolDevice(LVMLogicalVolumeDevice): :type metadatasize: :class:`~.size.Size` :keyword chunksize: chunk size for the pool :type chunksize: :class:`~.size.Size`
:keyword profile: (allocation) profile for the pool or None (unspecified):type profile: :class:`~.devicelibs.lvm.ThPoolProfile` or NoneType""" if metadatasize is not None and \ not lvm.is_valid_thin_pool_metadata_size(metadatasize):@@ -3491,6 +3494,7 @@ class LVMThinPoolDevice(LVMLogicalVolumeDevice):
self.metaDataSize = metadatasize or 0 self.chunkSize = chunksize or 0
self.profile = profile self._lvs = []def _addLogVol(self, lv):
@@ -3536,7 +3540,8 @@ class LVMThinPoolDevice(LVMLogicalVolumeDevice): # TODO: chunk size, data/metadata split --> profile lvm.thinpoolcreate(self.vg.name, self.lvname, self.size, metadatasize=self.metaDataSize,
chunksize=self.chunkSize)
chunksize=self.chunkSize,profile=self.profile)
This should be 'profile=self.profile.name'. Fixed locally.
On Fri, 2014-09-19 at 15:45 +0200, Vratislav Podzimek wrote:
The final goal for the bug #869456 is to add GUI support for specifying thin pool profile, but I'd like you to have a chance to judge these building blocks first.
Any other issues with this patch set? I'd like to push it if it looks okay with the less restrictive version of the pykickstart as suggested by clumens.
anaconda-patches@lists.fedorahosted.org
-
Chris Lumens -
Vratislav Podzimek