virsh_find_storage_* : fix for Failed to connect socket to '/var/run/…#6864
virsh_find_storage_* : fix for Failed to connect socket to '/var/run/…#6864sneh-3 wants to merge 1 commit into
Conversation
…libvirt/virtstoraged-sock' Added virtstoraged restart i.e. Deamon restarts after polkit configuration Signed-off-by: Sneh Shikha Yadav <syadav@linux.ibm.com>
WalkthroughThis change enhances a virsh storage pool test file by adding polkit PolicyKit ACL configuration support. When the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
| # Explicitly restart virtstoraged for storage driver ACL tests | ||
| # The libvirtd.restart() in polkit.setup() doesn't always restart virtstoraged | ||
| from virttest.staging import service | ||
| try: | ||
| virtstoraged = service.Factory.create_service("virtstoraged") | ||
| logging.info("Restarting virtstoraged to apply polkit rules") | ||
| virtstoraged.restart() | ||
| logging.info("virtstoraged restarted successfully") | ||
| except Exception as svc_err: | ||
| logging.warning("Failed to restart virtstoraged: %s", svc_err) | ||
| logging.warning("This may cause ACL test failures for storage operations") |
There was a problem hiding this comment.
Fail fast when the required virtstoraged restart does not happen.
This restart is the core of the fix, but on split-daemon hosts a failure is only logged and the test continues with stale storage-daemon state. That turns a deterministic setup failure into a later ACL/socket failure with a less obvious cause. Gate this on utils_split_daemons.is_modular_daemon() and raise TestError if the restart fails there.
Suggested change
- from virttest.staging import service
- try:
- virtstoraged = service.Factory.create_service("virtstoraged")
- logging.info("Restarting virtstoraged to apply polkit rules")
- virtstoraged.restart()
- logging.info("virtstoraged restarted successfully")
- except Exception as svc_err:
- logging.warning("Failed to restart virtstoraged: %s", svc_err)
- logging.warning("This may cause ACL test failures for storage operations")
+ if utils_split_daemons.is_modular_daemon():
+ from virttest.staging import service
+ try:
+ virtstoraged = service.Factory.create_service("virtstoraged")
+ logging.info("Restarting virtstoraged to apply polkit rules")
+ virtstoraged.restart()
+ logging.info("virtstoraged restarted successfully")
+ except Exception as svc_err:
+ raise exceptions.TestError(
+ "Failed to restart virtstoraged after polkit setup: %s" % svc_err
+ ) from svc_err🧰 Tools
🪛 Ruff (0.15.12)
[warning] 82-82: 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 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.
| 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.
1 participant
…libvirt/virtstoraged-sock'
Added virtstoraged restart i.e. Deamon restarts after polkit configuration
Summary by CodeRabbit