tests: make cleanup.sh more robust for Stratis pools#388
Conversation
WalkthroughCapture and print stdout/stderr when stratis filesystem or pool destroy fails; stop unconditional pool destroy tied to /dev presence and instead check Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Script as tests/bin/cleanup.sh
participant Stratis as stratis CLI
Tester->>Script: run cleanup.sh
loop each filesystem in pool1
Script->>Stratis: filesystem destroy pool1 <FS>
alt failure (captured FAIL)
Stratis-->>Script: stderr/stdout
Script-->>Tester: "Failed to clean up pool1 <FS>: <FAIL>"
else success
Stratis-->>Script: success
end
end
Script->>Stratis: pool list
alt pool1 present
Script->>Stratis: pool destroy pool1
alt failure (captured FAIL)
Stratis-->>Script: stderr/stdout
Script-->>Tester: "Failed to clean up pool1: <FAIL>"
else success
Stratis-->>Script: success
end
else pool1 absent
Script-->>Tester: skip pool destroy
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/bin/cleanup.sh (2)
26-26: Quote variables and use printf for robust, readable error reporting.Safer when filesystem names or messages contain spaces/newlines; avoids word-splitting and globbing in echo. Prefer a clear if-not pattern.
- FAIL=$(stratis filesystem destroy pool1 $FS 2>&1) || echo Failed to clean up pool1 $FS: $FAIL + if ! FAIL=$(stratis filesystem destroy pool1 "$FS" 2>&1); then + printf 'Failed to clean up pool1 %s: %s\n' "$FS" "$FAIL" + fiNote: The if var=$(cmd) idiom tests the command’s exit status (not the assignment) per POSIX; this change is semantically equivalent but clearer. (unix.stackexchange.com)
30-31: Exact-match the pool name and harden error reporting.Avoid substring matches (e.g., pool12) and keep the clearer if-not pattern with quotes/printf.
-if stratis pool | grep pool1 > /dev/null; then - FAIL=$(stratis pool destroy pool1 2>&1) || echo Failed to clean up pool1: $FAIL +if stratis pool list | grep -qE '^pool1([[:space:]]|$)'; then + if ! FAIL=$(stratis pool destroy pool1 2>&1); then + printf 'Failed to clean up pool1: %s\n' "$FAIL" + fiThis uses an anchored, word-safe match and prevents echo/globbing pitfalls by quoting and using printf.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/bin/cleanup.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: python-basic-tests
🔇 Additional comments (1)
tests/bin/cleanup.sh (1)
30-31: Nice separation of filesystem vs. pool cleanup.Splitting pool destruction out of the /dev/stratis existence check directly addresses #387’s edge case and improves robustness.
|
N.B.: |
bmr-cymru
force-pushed
the
bmr-stratis-cleanup-harder
branch
from
a3ee69c to
e2b1556
Compare
If a stratis cleanup command fails, report the stdout/stderr, and separate pool cleanup from file system cleanup (if all file systems have been destroyed, /dev/stratis/$pool no longer exists, but the pool is still tracked by Stratis and needs to be destroyed separately). Resolves: #387
bmr-cymru
force-pushed
the
bmr-stratis-cleanup-harder
branch
from
e2b1556 to
e75db25
Compare
1 participant
If a stratis cleanup command fails, report the stdout/stderr, and separate pool cleanup from file system cleanup (if all file systems have been destroyed, /dev/stratis/$pool no longer exists, but the pool is still tracked by Stratis and needs to be destroyed separately).
Resolves: #387
Summary by CodeRabbit