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...

[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

[fsuba]

/packit build

[mulkieran]

rebased, conflicts resolved.

[mulkieran]

Need to catch a few more exceptions.