Fix crash on systems with broken stratis pools#1464
Fix crash on systems with broken stratis pools#1464vojtechtrefny merged 1 commit intostoraged-project:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded defensive checks for missing or empty Stratis pool names: stopped-pool name extraction now handles absent Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blivet/static_data/stratis_info.py`:
- Around line 214-224: The code currently logs unnamed stopped pools but still
appends a StratisStoppedPoolInfo with name=None to stopped_pools; change this so
when pools_info[pool_uuid] lacks "name" you log the warning and skip
creating/appending the StratisStoppedPoolInfo (i.e., do not construct
StratisStoppedPoolInfo or append to stopped_pools for that pool_uuid), leaving
only pools with a valid name in stopped_pools.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f403a0f-610a-4b93-8cc9-0dba45bc3c50
📒 Files selected for processing (2)
blivet/populator/helpers/stratis.pyblivet/static_data/stratis_info.py
tasleson
left a comment
tasleson
left a comment
There was a problem hiding this comment.
My initial reaction was LGTM, but after a more detailed review, I think there is some ideas that should be considered before committing which I've noted in the review.
Also, what is the state of the devices key? Does it have valid entries or empties when this occurs? Do we need code guards for this or is it just the name that is absent? Do we want to be able to handle total garbage in the future if stratis breaks in different ways?
I also think it would be helpful to reference the stratis bug issue in the commit message. So that someone visiting this commit in the future can see what happened with stratis. I'm assuming that there is a plan for fixing it.
| pool_name = pools_info[pool_uuid]["name"].get_string() | ||
| else: | ||
| log.warning("Stopped Stratis pool %s does not have name", pool_uuid) | ||
| pool_name = None |
There was a problem hiding this comment.
Previously, the namedtuple StratisStoppedPoolInfo.pool_name had a type of string. With this change it can now be None. This could possibly cause a user of blivet to run into an error if they assume it will always be a valid string. Thus, it's kind of a subtle public API change. One could argue that changing this to an empty string would be more appropriate as it preserves the data type. However, the counter argument could be made that None forces caller to specifically address when their code blows up instead of silently displaying an empty string.
There was a problem hiding this comment.
Fortunately the static_data module is not part of the public API.
| if not pool_info.name: | ||
| # no name -> probably broken or partially constructed pool | ||
| # we cannot add a device without name | ||
| log.warning("Ignoring stopped stratis pool without name") |
There was a problem hiding this comment.
Please add the pool uuid to the warning, so that someone debugging can know which pool this is, for correlation.
log.warning("Ignoring stopped stratis pool %s without name", pool_info.uuid))There was a problem hiding this comment.
Good point, fixed.
A broken or partially created stratis pool can be reported as stopped without a name. In this case we fail to gather the info about it and crash. We can't work with devices without a name so we need to completely ignore these pools.
vojtechtrefny
force-pushed
the
main_stratis-stopped-pool-fix
branch
from
d2126a8 to
ae92b5a
Compare
2 participants
A broken or partially created stratis pool can be reported as stopped without a name. In this case we fail to gather the info about it and crash. We can't work with devices without a name so we need to completely ignore these pools.
Related: stratis-storage/project#861
Summary by CodeRabbit