From 4e37193bdef3377c96c8acea50454b83f9497152 Mon Sep 17 00:00:00 2001 From: mulhern Date: Wed, 27 Nov 2024 10:32:22 -0500 Subject: [PATCH] Add options for specifying integrity metadata pre-allocations Signed-off-by: mulhern --- docs/stratis.txt | 4 +- src/stratis_cli/_actions/_introspect.py | 3 + src/stratis_cli/_actions/_pool.py | 17 ++- src/stratis_cli/_constants.py | 25 +++ src/stratis_cli/_parser/_pool.py | 142 +++++++++++++++++- src/stratis_cli/_parser/_range.py | 10 ++ .../whitebox/integration/pool/test_create.py | 20 +++ tests/whitebox/integration/test_parser.py | 48 +++++- 8 files changed, 261 insertions(+), 8 deletions(-) diff --git a/docs/stratis.txt b/docs/stratis.txt index 1160c4723..f273055e2 100644 --- a/docs/stratis.txt +++ b/docs/stratis.txt @@ -36,8 +36,10 @@ GLOBAL OPTIONS COMMANDS -------- -pool create [--key-desc ] [--clevis <(nbde|tang|tpm2)> [--tang-url ] [<(--thumbprint | --trust-url)>] [--no-overprovision] [..]:: +pool create [--key-desc ] [--clevis <(nbde|tang|tpm2)> [--tang-url ] [<(--thumbprint | --trust-url)>] [--no-overprovision] [--integrity <(no,pre-allocate)>] [--journal-size ] [--tag-spec ] [..]:: Create a pool from one or more block devices, with the given pool name. + The --tag-spec and --journal-size options are used to configure the amount + of space to reserve for integrity metadata. pool stop <(--uuid |--name )>:: Stop a pool, specifying the pool by its UUID or by its name. Tear down the storage stack but leave all metadata intact. diff --git a/src/stratis_cli/_actions/_introspect.py b/src/stratis_cli/_actions/_introspect.py index 0f520d465..9920f2e29 100644 --- a/src/stratis_cli/_actions/_introspect.py +++ b/src/stratis_cli/_actions/_introspect.py @@ -13,6 +13,9 @@ + + + diff --git a/src/stratis_cli/_actions/_pool.py b/src/stratis_cli/_actions/_pool.py index 8e1a27ca4..89dec7f81 100644 --- a/src/stratis_cli/_actions/_pool.py +++ b/src/stratis_cli/_actions/_pool.py @@ -25,7 +25,7 @@ # isort: THIRDPARTY from justbytes import Range -from .._constants import Id, IdType, UnlockMethod +from .._constants import Id, IdType, IntegrityOption, IntegrityTagSpec, UnlockMethod from .._error_codes import PoolErrorCode from .._errors import ( StratisCliEngineError, @@ -180,6 +180,16 @@ def create_pool(namespace): # pylint: disable=too-many-locals None if namespace.clevis is None else ClevisInfo.get_info(namespace.clevis) ) + (journal_size, tag_spec, allocate_superblock) = ( + ((True, 0), (True, IntegrityTagSpec.B0), (True, False)) + if namespace.integrity.integrity is IntegrityOption.NO + else ( + (True, namespace.integrity.journal_size.magnitude.numerator), + (True, namespace.integrity.tag_spec), + (True, True), + ) + ) + ( (changed, (pool_object_path, _)), return_code, @@ -199,10 +209,13 @@ def create_pool(namespace): # pylint: disable=too-many-locals if clevis_info is None else (True, (clevis_info.pin, json.dumps(clevis_info.config))) ), + "journal_size": journal_size, + "tag_spec": tag_spec, + "allocate_superblock": allocate_superblock, }, ) - if return_code != StratisdErrors.OK: # pragma: no cover + if return_code != StratisdErrors.OK: raise StratisCliEngineError(return_code, message) if not changed: # pragma: no cover diff --git a/src/stratis_cli/_constants.py b/src/stratis_cli/_constants.py index b8f80e0a8..419992ab5 100644 --- a/src/stratis_cli/_constants.py +++ b/src/stratis_cli/_constants.py @@ -116,3 +116,28 @@ class Clevis(Enum): def __str__(self): return self.value + + +class IntegrityTagSpec(Enum): + """ + Options for specifying integrity tag size. + """ + + B0 = "0b" + B32 = "32b" + B512 = "512b" + + def __str__(self): + return self.value + + +class IntegrityOption(Enum): + """ + Options for specifying integrity choices on create. + """ + + NO = "no" + PRE_ALLOCATE = "pre-allocate" + + def __str__(self): + return self.value diff --git a/src/stratis_cli/_parser/_pool.py b/src/stratis_cli/_parser/_pool.py index 27a829783..ce0d45a9e 100644 --- a/src/stratis_cli/_parser/_pool.py +++ b/src/stratis_cli/_parser/_pool.py @@ -20,12 +20,22 @@ from argparse import SUPPRESS, ArgumentTypeError from uuid import UUID +# isort: THIRDPARTY +from justbytes import MiB, Range + from .._actions import BindActions, PoolActions -from .._constants import Clevis, EncryptionMethod, UnlockMethod, YesOrNo +from .._constants import ( + Clevis, + EncryptionMethod, + IntegrityOption, + IntegrityTagSpec, + UnlockMethod, + YesOrNo, +) from .._error_codes import PoolErrorCode from ._bind import BIND_SUBCMDS, REBIND_SUBCMDS from ._debug import POOL_DEBUG_SUBCMDS -from ._range import RejectAction +from ._range import DefaultAction, RejectAction, parse_range class ClevisEncryptionOptions: # pylint: disable=too-few-public-methods @@ -81,6 +91,62 @@ def verify(self, namespace, parser): namespace.clevis = None if self.clevis is None else self +class IntegrityOptions: # pylint: disable=too-few-public-methods + """ + Gathers and verifies integrity options. + """ + + def __init__(self, namespace): + self.integrity = copy.copy(namespace.integrity) + del namespace.integrity + + self.journal_size = copy.copy(namespace.journal_size) + del namespace.journal_size + + self.journal_size_default = getattr(namespace, "journal_size_default", True) + + self.tag_spec = copy.copy(namespace.tag_spec) + del namespace.tag_spec + + self.tag_spec_default = getattr(namespace, "tag_spec_default", True) + + def verify(self, namespace, parser): + """ + Do supplementary parsing of conditional arguments. + """ + + if self.integrity is IntegrityOption.NO and not self.journal_size_default: + parser.error( + f'--integrity value "{IntegrityOption.NO}" and --journal-size ' + "option can not be specified together" + ) + + if self.integrity is IntegrityOption.NO and not self.tag_spec_default: + parser.error( + f'--integrity value "{IntegrityOption.NO}" and --tag-spec ' + "option can not be specified together" + ) + + namespace.integrity = self + + +class CreateOptions: # pylint: disable=too-few-public-methods + """ + Gathers and verifies options specified on pool create. + """ + + def __init__(self, namespace): + self.clevis_encryption_options = ClevisEncryptionOptions(namespace) + self.integrity_options = IntegrityOptions(namespace) + + def verify(self, namespace, parser): + """ + Verify that create command-line is formed correctly. + """ + self.clevis_encryption_options.verify(namespace, parser) + self.integrity_options.verify(namespace, parser) + + def _ensure_nat(arg): """ Raise error if argument is not an natural number. @@ -110,7 +176,7 @@ def _ensure_nat(arg): "--post-parser", { "action": RejectAction, - "default": ClevisEncryptionOptions, + "default": CreateOptions, "help": SUPPRESS, "nargs": "?", }, @@ -163,7 +229,75 @@ def _ensure_nat(arg): ) ], }, - ) + ), + ( + "integrity", + { + "description": ( + "Optional parameters for configuring integrity " + "metadata pre-allocation" + ), + "args": [ + ( + "--integrity", + { + "help": ( + "Integrity options for this pool. If " + f'"{IntegrityOption.NO}" no space will ' + "be allocated for integrity metadata " + "and it will never be possible to turn " + "on integrity functionality for this " + "pool. " + f'If "{IntegrityOption.PRE_ALLOCATE}" ' + "then space will be allocated for " + "integrity metadata and it will be " + "possible to switch on integrity " + "functionality in future. The default " + 'is "%(default)s".' + ), + "choices": list(IntegrityOption), + "default": IntegrityOption.PRE_ALLOCATE, + "type": IntegrityOption, + }, + ), + ( + "--journal-size", + { + "help": ( + "Size of integrity device's journal. " + "Each block is written to this journal " + "before being written to its address. " + "The default is %(default)s." + ), + "action": DefaultAction, + "default": Range(128, MiB), + "type": parse_range, + }, + ), + ( + "--tag-spec", + { + "help": ( + "Integrity tag specification defining " + "the size of the tag used to store a " + "checksum or other value for each " + "block on a device. All size " + "specifications are in bits. The " + "default is %(default)s." + ), + "action": DefaultAction, + "default": IntegrityTagSpec.B512, + "choices": [ + x + for x in list(IntegrityTagSpec) + if x is not IntegrityTagSpec.B0 + ], + "type": IntegrityTagSpec, + }, + ), + ], + }, + ), ], "args": [ ("pool_name", {"help": "Name of new pool"}), diff --git a/src/stratis_cli/_parser/_range.py b/src/stratis_cli/_parser/_range.py index c70d8c2d7..c41a5100c 100644 --- a/src/stratis_cli/_parser/_range.py +++ b/src/stratis_cli/_parser/_range.py @@ -88,3 +88,13 @@ def __call__(self, parser, namespace, values, option_string=None): raise argparse.ArgumentError( self, f"Option {option_string} can not be assigned to or set." ) + + +class DefaultAction(argparse.Action): + """ + Detect if the default value was set. + """ + + def __call__(self, parser, namespace, values, option_string=None): + setattr(namespace, self.dest, values) + setattr(namespace, self.dest + "_default", False) diff --git a/tests/whitebox/integration/pool/test_create.py b/tests/whitebox/integration/pool/test_create.py index 36e5b3002..99806de43 100644 --- a/tests/whitebox/integration/pool/test_create.py +++ b/tests/whitebox/integration/pool/test_create.py @@ -18,6 +18,7 @@ # isort: LOCAL from stratis_cli import StratisCliErrorCodes from stratis_cli._errors import ( + StratisCliEngineError, StratisCliInUseSameTierError, StratisCliNameConflictError, ) @@ -165,3 +166,22 @@ def test_no_overprovision(self): self._MENU + [self._POOLNAME] + self._DEVICES + ["--no-overprovision"] ) TEST_RUNNER(command_line) + + +class Create7TestCase(SimTestCase): + """ + Test create with integrity options. + """ + + _MENU = ["--propagate", "pool", "create"] + _DEVICES = _DEVICE_STRATEGY() + _POOLNAME = "thispool" + + def test_invalid_journal_size(self): + """ + Test creating with an invalid journal size. + """ + command_line = ( + self._MENU + [self._POOLNAME] + self._DEVICES + ["--journal-size=131079KiB"] + ) + self.check_error(StratisCliEngineError, command_line, _ERROR) diff --git a/tests/whitebox/integration/test_parser.py b/tests/whitebox/integration/test_parser.py index 141d7f39e..7076a5a49 100644 --- a/tests/whitebox/integration/test_parser.py +++ b/tests/whitebox/integration/test_parser.py @@ -23,7 +23,7 @@ _PARSE_ERROR = StratisCliErrorCodes.PARSE_ERROR -class ParserTestCase(RunTestCase): +class ParserTestCase(RunTestCase): # pylint: disable=too-many-public-methods """ Test parser behavior. The behavior should be identical, regardless of whether the "--propagate" flag is set. That is, stratis should never produce @@ -221,6 +221,52 @@ def test_create_with_post_parser_set(self): for prefix in [[], ["--propagate"]]: self.check_system_exit(prefix + command_line, _PARSE_ERROR) + def test_create_with_bad_tag_value(self): + """ + Verify that an unrecognized tag value causes an error. + """ + command_line = [ + "pool", + "create", + "pn", + "/dev/n", + "--tag-spec=512", + ] + for prefix in [[], ["--propagate"]]: + self.check_system_exit(prefix + command_line, _PARSE_ERROR) + + def test_create_with_integrity_no_journal_size(self): + """ + Verify that creating with integrity = no plus good journal-size + results in a parse error. + """ + command_line = [ + "pool", + "create", + "pn", + "/dev/n", + "--integrity=no", + "--journal-size=128MiB", + ] + for prefix in [[], ["--propagate"]]: + self.check_system_exit(prefix + command_line, _PARSE_ERROR) + + def test_create_with_integrity_no_tag_spec(self): + """ + Verify that creating with integrity = no plus good tag-size + results in a parse error. + """ + command_line = [ + "pool", + "create", + "pn", + "/dev/n", + "--integrity=no", + "--tag-spec=32b", + ] + for prefix in [[], ["--propagate"]]: + self.check_system_exit(prefix + command_line, _PARSE_ERROR) + def test_stratis_list_filesystem_with_name_no_pool(self): """ We want to get a parse error if filesystem UUID is specified but no