Conversation
…libvirt/virtstoraged-sock' Added virtstoraged restart i.e. Deamon restarts after polkit configuration Signed-off-by: Sneh Shikha Yadav <syadav@linux.ibm.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe test adds optional libvirt Polkit ACL setup and teardown around Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
before fix : after fix: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libvirt/tests/src/virsh_cmd/pool/virsh_find_storage_pool_sources.py`:
- Around line 156-161: polkit.cleanup() must be followed by a restart of the
storage daemon to avoid stale ACLs; update the cleanup block that calls
polkit.cleanup() to invoke virtstoraged.restart() (mirroring the setup path
where polkit.setup() and libvirtd.restart() were used), and ensure you catch/log
exceptions from both polkit.cleanup() and virtstoraged.restart() so failures are
reported.
- Around line 74-84: The restart failure for virtstoraged is only logged
currently, which hides deterministic setup failures on split-daemon hosts;
update the try/except around service.Factory.create_service("virtstoraged") /
virtstoraged.restart() to check utils_split_daemons.is_modular_daemon() and, if
that returns True and an exception occurs, re-raise a TestError (raise
TestError("Failed to restart virtstoraged: ...")) instead of merely logging;
otherwise keep the existing logging behavior. Ensure you reference the same
symbols: service.Factory.create_service, virtstoraged.restart,
utils_split_daemons.is_modular_daemon, and TestError when implementing the
change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e92805a8-ce4c-47fd-baa5-e11ea0460064
📒 Files selected for processing (1)
libvirt/tests/src/virsh_cmd/pool/virsh_find_storage_pool_sources.py
| if polkit: | ||
| logging.info("Cleaning up polkit configuration") | ||
| try: | ||
| polkit.cleanup() | ||
| except Exception as e: | ||
| logging.warning("Failed to cleanup polkit: %s", e) |
There was a problem hiding this comment.
Restart virtstoraged after removing the polkit rules too.
The setup path explicitly restarts virtstoraged because polkit.setup()/libvirtd.restart() does not reliably refresh the storage daemon. polkit.cleanup() changes the same ACL state, so skipping the matching restart here can leak stale authorization state into later storage tests.
Suggested change
if polkit:
logging.info("Cleaning up polkit configuration")
try:
polkit.cleanup()
+ if utils_split_daemons.is_modular_daemon():
+ from virttest.staging import service
+ virtstoraged = service.Factory.create_service("virtstoraged")
+ logging.info("Restarting virtstoraged after polkit cleanup")
+ virtstoraged.restart()
except Exception as e:
logging.warning("Failed to cleanup polkit: %s", e)🧰 Tools
🪛 Ruff (0.15.12)
[warning] 160-160: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/virsh_cmd/pool/virsh_find_storage_pool_sources.py` around
lines 156 - 161, polkit.cleanup() must be followed by a restart of the storage
daemon to avoid stale ACLs; update the cleanup block that calls polkit.cleanup()
to invoke virtstoraged.restart() (mirroring the setup path where polkit.setup()
and libvirtd.restart() were used), and ensure you catch/log exceptions from both
polkit.cleanup() and virtstoraged.restart() so failures are reported.
- Gate virtstoraged restart on is_modular_daemon() to avoid unnecessary restarts on monolithic daemon hosts - Raise TestError instead of logging warning when virtstoraged restart fails on modular daemon hosts, to fail fast on setup errors - Restart virtstoraged after polkit.cleanup() to flush stale ACLs, mirroring the setup path Signed-off-by: Sneh Shikha Yadav <syadav@linux.ibm.com>
|
--log after the recent fixes-- |
1 participant
…libvirt/virtstoraged-sock'
Added virtstoraged restart i.e. Deamon restarts after polkit configuration
Summary by CodeRabbit