Issue stratisd 3597

[mulkieran]

Summary by CodeRabbit

• New Features

  • Added pool encryption management: encrypt unencrypted pools, decrypt encrypted pools, and re-encrypt with a new master key
  • Pool information now displays the "Last Time Reencrypted" timestamp to track encryption operation history
  • Support for in-place encryption operations to enable long-running encryption transactions

• Improvements

  • Reduced D-Bus timeout from 120 to 60 seconds to improve responsiveness

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo dnf install -y 'dnf*-command(copr)'
• dnf copr enable packit/stratis-storage-stratis-cli-1257-copr_pull
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[mulkieran]

Right now, we do have the option of making the timeout shorter for our long-running operations than for our other operations. The best way to do this properly is to import only the methods we need in building the dbus-python-client-gen methods and do it exactly where the methods are used. We could still use the public make_class method, but modify the spec file passed to it.

[mulkieran]

Long running operation decorator needs to specify the function that is allowed to run for a long time. Otherwise, if it's GetManagedObjects that is the long-running function, "Operation initiated" may be erroneously returned. Should allow it to take a list argument, just to be on the safe side.

[jbaublitz]

Just one observation: I don't think GetManagedObjects every can be the long-running operation in the current state of stratisd but I'm aware that could change in the future with changes to our locking.

[mulkieran]

Just one observation: I don't think GetManagedObjects every can be the long-running operation in the current state of stratisd but I'm aware that could change in the future with changes to our locking.

In a bunch of method we call GetManagedObjects() first and then, e.g., EncryptPool. If GetManagedObjects is the method that times out, we need to treat that as an actual timeout..., with the non-specific decorator on the Python methods stratis-cli is reporting "Operation initiated" on a GetManagedObjects timeout, which is wrong. That's what needs to be fixed.

[mulkieran]

I do not know why building stratisd man pages was failing...but we do not need those pages built to test stratisd, so I'm just fixing that in the most basic way.

[mulkieran]

Ok. Redirects are working....

[mulkieran]

Works again...

[mulkieran]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

This PR introduces pool encryption lifecycle management for Stratis CLI, adding encryption subcommands (on/off/reencrypt) with in-place operation support, new D-Bus methods (EncryptPool, DecryptPool, ReencryptPool), supporting utilities for long-running operations, integration and unit tests, and updated documentation.

Changes

Cohort / File(s)	Summary
Configuration and Metadata .packit.yaml, setup.cfg	Removed manual trigger from copr_build job; bumped dbus-python-client-gen minimum to 0.8.4 and added python-dateutil and wcwidth dependencies.
D-Bus Interface Definitions src/stratis_cli/_actions/_introspect.py	Added three D-Bus methods (EncryptPool, DecryptPool, ReencryptPool) to pool.r9 interface with key_descs/clevis_infos inputs; modified Encrypted property and added LastReencryptedTimestamp property.
Encryption Action Implementation src/stratis_cli/_actions/_crypt.py	New CryptActions class with static methods (encrypt, unencrypt, reencrypt) to manage pool encryption lifecycle, including validation, D-Bus invocation, error handling, and timeout management.
Parser Integration src/stratis_cli/_parser/_encryption.py, src/stratis_cli/_parser/_pool.py, src/stratis_cli/_parser/_shared.py	Added new encryption subcommands (on, off, reencrypt) and shared argument groups (CLEVIS_AND_KERNEL, IN_PLACE) for CLI argument definition and reuse across pool creation and encryption operations.
Utilities and Error Handling src/stratis_cli/_actions/_utils.py, src/stratis_cli/_actions/_data.py, src/stratis_cli/_error_reporting.py, src/stratis_cli/_errors.py, src/stratis_cli/_actions/__init__.py	Added long_running_operation decorator and get_errors helper for async operation handling; moved get_errors to _actions module; reduced D-Bus timeout from 120 to 60 seconds; added StratisCliInPlaceNotSpecified exception; exported CryptActions and get_errors.
Display and Documentation src/stratis_cli/_actions/_list_pool.py, docs/stratis.txt	Added "Last Time Reencrypted" field to pool list detail view; documented new encryption subcommands, --in-place option behavior, and pool list field semantics.
Tests tests/integration/pool/test_encryption.py, tests/integration/test_parser.py, tests/unit/test_running.py	Added comprehensive test cases for encryption workflows (encrypt/decrypt/reencrypt with various pool states), in-place flag validation, Clevis option parsing, and long_running_operation error handling.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant CLI as CLI Parser
    participant CryptActions
    participant LongRunning as long_running_operation
    participant DBus as D-Bus (Stratisd)
    participant Handler as Error Handler

    User->>CLI: Run encryption command<br/>(pool encryption on --in-place)
    CLI->>CryptActions: encrypt(namespace)
    activate CryptActions
    CryptActions->>CryptActions: Validate in_place flag
    CryptActions->>CryptActions: Resolve pool via PoolId
    CryptActions->>LongRunning: Wrap EncryptPool call
    deactivate CryptActions
    
    activate LongRunning
    LongRunning->>DBus: Call EncryptPool method
    activate DBus
    DBus->>DBus: Process encryption
    DBus-->>LongRunning: Return results + return_code
    deactivate DBus
    
    alt Success (return_code == 0)
        LongRunning-->>CLI: Return success
    else NoReply/Timeout
        LongRunning->>LongRunning: Catch DPClientInvocationError
        LongRunning->>Handler: Print long-running message
        LongRunning-->>CLI: Return None
    else Other Error
        LongRunning->>Handler: Propagate exception
        Handler-->>CLI: Raise StratisCliEngineError
    end
    deactivate LongRunning
    
    CLI-->>User: Display result/error

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• Version 3.8.3 #1233: Directly overlaps with encryption parser and D-Bus introspection modifications, with potential removal or downgrade of the same encryption interfaces and commands introduced in this PR.

Suggested reviewers

• jbaublitz
• drckeefe

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The PR title 'Issue stratisd 3597' is vague and does not clearly describe the main changes in the changeset, using only a generic issue reference without conveying what was actually implemented or changed.	Consider revising the title to be more descriptive of the primary changes, such as 'Add pool encryption commands and long-running operation support' or similar that conveys the main feature additions.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

You can generate walkthrough in a markdown collapsible section to save space.

Enable the reviews.collapse_walkthrough setting to generate walkthrough in a markdown collapsible section.

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

src/stratis_cli/_actions/_utils.py (1)

337-343: Return the wrapped action's result explicitly.

wrapper() drops func(namespace)'s return value on the success path and falls through with an implicit None on the accepted-timeout path. That makes the decorator non-transparent and can silently change behavior for any action that ever returns a value or exit code.

Possible fix

         `@wraps`(func)
         def wrapper(namespace: Namespace):
             """
             Wrapper
             """
             try:
-                func(namespace)
+                return func(namespace)
             except DPClientInvocationError as err:
                 if not any(
                     isinstance(e, DBusException)
                     and e.get_dbus_name() == "org.freedesktop.DBus.Error.NoReply"
                     for e in get_errors(err)
                 ):
                     raise err

                 context = err.context
                 if (
                     isinstance(context, DPClientMethodCallContext)
                     and context.method_name in method_names
                 ):
                     print("Operation initiated", file=sys.stderr)
+                    return None

                 else:
                     raise err

Also applies to: 352-357

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/stratis_cli/_actions/_utils.py` around lines 337 - 343, The decorator's
inner function wrapper currently calls func(namespace) but does not return its
result (and similarly does not return the result on the accepted-timeout path),
making the decorator non-transparent; update wrapper to return the value from
func(namespace) on the normal path and return the result/value from the
accepted-timeout branch as well (i.e., ensure both code paths in wrapper return
the wrapped action's result), locating the logic inside the wrapper function
around the current func(namespace) call and the accepted-timeout handling (also
apply the same fix to the similar block at the other occurrence).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/stratis.txt`:
- Line 285: Replace the phrase "can not" with the standard single-word "cannot"
in the sentence that currently reads "for example, extending filesystems, can
not be taken on the pool." — locate that exact sentence (contains "extending
filesystems, can not be taken on the pool") and update it to use "cannot" for
correct and consistent wording.

In `@src/stratis_cli/_actions/_list_pool.py`:
- Around line 387-394: The "Last Time Reencrypted" output is printed for all
encrypted pools but must only appear for metadata V2 pools; move the call to
mopool.LastReencryptedTimestamp() and the print(f"    Last Time Reencrypted:
{reencrypted}") into the metadata V2 branch (the branch that handles V2 pool
output) so it is emitted only when the pool is V2, and remove or avoid invoking
LastReencryptedTimestamp() for non-V2 branches.

In `@src/stratis_cli/_parser/_shared.py`:
- Around line 278-299: The CLEVIS_AND_KERNEL argument bundle is missing
TRUST_URL_OR_THUMBPRINT which causes ClevisEncryptionOptions.__init__ to fail
when it unconditionally accesses namespace.thumbprint and namespace.trust_url;
update CLEVIS_AND_KERNEL to include the same entries used by
TRUST_URL_OR_THUMBPRINT (the arguments that supply namespace.trust_url and
namespace.thumbprint) so the bundle is self-contained and safe to reuse—ensure
the new entries use the same option names, help text and parsing behavior as
TRUST_URL_OR_THUMBPRINT so ClevisEncryptionOptions.__init__ can always read
namespace.trust_url and namespace.thumbprint without AttributeError.

In `@tests/unit/test_running.py`:
- Around line 56-58: The test docstring currently says "Should succeed because
it catches the distinguishing NoReply D-Bus error from the identified method"
but the test actually verifies the opposite (a method name mismatch leading to
exception propagation); update the docstring in tests/unit/test_running.py for
the affected test to clearly state that the call should raise/propagate the
NoReply D-Bus error when the method name does not match, referencing the test
function or assertion that checks exception propagation.

---

Nitpick comments:
In `@src/stratis_cli/_actions/_utils.py`:
- Around line 337-343: The decorator's inner function wrapper currently calls
func(namespace) but does not return its result (and similarly does not return
the result on the accepted-timeout path), making the decorator non-transparent;
update wrapper to return the value from func(namespace) on the normal path and
return the result/value from the accepted-timeout branch as well (i.e., ensure
both code paths in wrapper return the wrapped action's result), locating the
logic inside the wrapper function around the current func(namespace) call and
the accepted-timeout handling (also apply the same fix to the similar block at
the other occurrence).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e3dd3a8c-8dc7-4a52-af8f-f1e794e4d95e

📥 Commits

Reviewing files that changed from the base of the PR and between 84e9033 and 29959dd.

📒 Files selected for processing (17)

• .packit.yaml
• docs/stratis.txt
• setup.cfg
• src/stratis_cli/_actions/__init__.py
• src/stratis_cli/_actions/_crypt.py
• src/stratis_cli/_actions/_data.py
• src/stratis_cli/_actions/_introspect.py
• src/stratis_cli/_actions/_list_pool.py
• src/stratis_cli/_actions/_utils.py
• src/stratis_cli/_error_reporting.py
• src/stratis_cli/_errors.py
• src/stratis_cli/_parser/_encryption.py
• src/stratis_cli/_parser/_pool.py
• src/stratis_cli/_parser/_shared.py
• tests/integration/pool/test_encryption.py
• tests/integration/test_parser.py
• tests/unit/test_running.py

💤 Files with no reviewable changes (1)

• .packit.yaml