WIP: feat: implement context manager-based resource cleanup #246
Closed
krcmarik wants to merge 2 commits intoRedHatQE:mainfrom
Closed
WIP: feat: implement context manager-based resource cleanup #246krcmarik wants to merge 2 commits intoRedHatQE:mainfrom
krcmarik wants to merge 2 commits intoRedHatQE:mainfrom
Conversation
Contributor
WalkthroughThis PR refactors resource lifecycle management to use context managers throughout fixtures, utilities, and tests. Many resource-creation functions now return context-managed iterators (via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @conftest.py:
- Around line 739-740: Replace the LOGGER.error call in the VM cleanup except
block so that the full traceback is preserved: change the except block that
catches Exception (current code using LOGGER.error(f"Failed to delete VM
{vm['name']}: {exc}")) to call LOGGER.exception with %-style formatting (e.g.,
LOGGER.exception("Failed to delete VM %s", vm["name"])) and remove the unused
exc variable; this ensures the stack trace is logged and avoids f-string lint
warnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
conftest.pyutilities/pytest_utils.py
💤 Files with no reviewable changes (1)
- utilities/pytest_utils.py
🧰 Additional context used
🪛 Ruff (0.14.10)
conftest.py
737-737: Logging statement uses f-string
(G004)
739-739: Do not catch blind exception: Exception
(BLE001)
740-740: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
740-740: Logging statement uses f-string
(G004)
🔇 Additional comments (1)
conftest.py (1)
727-738: LGTM! Immediate VM cleanup aligns with PR objectivesThe cleanup logic correctly:
- Targets destination VMs in
target_namespace(not source VMs)- Uses
existscheck to avoidNotFoundErroron already-deleted VMs- Employs
wait=Truefor synchronous deletion, preventing resource contention between sequential tests- Handles the case where
vm["name"]was updated to the migrated name on line 711This directly addresses the resource exhaustion issue on smaller clusters.
redhat-qe-bot2
added
changes-requested-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @conftest.py:
- Around line 739-740: Remove the unused exception variable by changing the
except clause from "except Exception as exc:" to "except Exception:" around the
block that calls LOGGER.exception("Failed to delete VM %s", vm["name"]);
LOGGER.exception already logs the current exception context so drop the "as exc"
binding and leave the LOGGER.exception call unchanged.
- Line 737: Replace the eager f-string in the LOGGER.info call with lazy logging
interpolation: change the LOGGER.info invocation that currently uses f"Deleting
VM {vm_obj.name} after test" to use a format string and argument(s) so the
message is constructed only if the log level is enabled (i.e.,
LOGGER.info("Deleting VM %s after test", vm_obj.name)); update the specific
LOGGER.info call that references vm_obj.name accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
conftest.py
🧰 Additional context used
🪛 Ruff (0.14.10)
conftest.py
737-737: Logging statement uses f-string
(G004)
739-739: Local variable exc is assigned to but never used
Remove assignment to unused variable exc
(F841)
🔇 Additional comments (2)
conftest.py (2)
727-729: LGTM on the cleanup strategy.Moving VM cleanup from session-level to function-level addresses the resource exhaustion issue on smaller clusters. The comment about Pods being garbage collected via
ownerReferencesis accurate — Kubernetes will handle pod cleanup automatically.
730-740: No issues found. Thevm["name"]mutation at line 711 is intentional and correct. Thesource_provider.vm_dict()method returns the actual cloned/renamed VM name insource_vm_details["name"]. Updatingvm["name"]with this value ensures cleanup uses the correct actual VM name in the target namespace, which is the proper design. The updated name is consistently used throughout (line 717 waits for the updated name, line 733 cleans up the updated name).
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @conftest.py:
- Around line 727-740: The LOGGER.info call uses an f-string while other logs
use lazy % formatting; change the info call inside the VM cleanup loop to use
lazy formatting by replacing the f-string with LOGGER.info("Deleting VM %s after
test", vm_obj.name) so logging is consistent with LOGGER.exception("Failed to
delete VM %s", vm["name"]) and avoids eager interpolation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
conftest.pyutilities/pytest_utils.py
💤 Files with no reviewable changes (1)
- utilities/pytest_utils.py
🧰 Additional context used
🪛 Ruff (0.14.10)
conftest.py
737-737: Logging statement uses f-string
(G004)
⏰ 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). (6)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: tox
- GitHub Check: pre-commit
- GitHub Check: build-container
🔇 Additional comments (1)
conftest.py (1)
729-740: Immediate per-test VM cleanup is a solid improvement.This change directly addresses the resource exhaustion issue described in the PR objectives. Deleting VMs immediately after each test rather than deferring to session end prevents OCP clusters from running out of CPU/memory when running sequential warm migrations or migrations with target power state "on".
The assumption that Kubernetes will garbage-collect pods via
ownerReferencesis correct - VMs own their virt-launcher pods, so pod cleanup is automatic.
myakove
requested changes
Jan 12, 2026
| "module": pod.__module__, | ||
| }) | ||
| try: | ||
| vm_obj = VirtualMachine( |
Collaborator
There was a problem hiding this comment.
This change defeats the whole process of having a functional environment for debugging if someone needs it.
If the issue is resources then I prefer to stop the VMs and not delete them here.
And do not remove the exsiting code, we store them also in order to get leftovers for the current run
Implement automatic resource cleanup using Python context managers with immediate cleanup when contexts exit, eliminating resource accumulation during test sessions. Key changes: - Convert create_and_store_resource() to @contextmanager for automatic cleanup - Convert create_storagemap_and_networkmap() and helpers to context managers - Convert run_migration() to context manager that yields Plan - Update migrate_vms() to use context manager for Plan/Migration - Update all tests to use with blocks for explicit resource scope - Mark cleaned resources in tracking (cleaned_by_context) for session verification Resource cleanup order (outer to inner): 1. NetworkMap - deleted first (inner context) 2. StorageMap - deleted second 3. Migration - deleted third 4. Plan - deleted last (outer context) Implementation follows context manager pattern from plan: - Phase 1: Added --skip-teardown CLI option and fixture - Phase 2: Converted create_and_store_resource() to context manager - Phase 3: Converted migration helpers to context managers - Phase 4: Updated migrate_vms() and run_migration() - Phase 5: Updated all test files to use with blocks Critical fixes applied: - run_migration() yields Plan instead of returning (keeps context alive) - Resources marked as cleaned but kept in tracking for session teardown - Proper nested context cleanup order maintained - Fixed all mypy type annotation errors Tests now use explicit with blocks showing resource lifecycle: with create_storagemap_and_networkmap(...) as (sm, nm): with run_migration(...) as plan: # Use resources pass # Plan and Migration cleaned up here # Storage and Network maps cleaned up here
redhat-qe-bot
added
size/XXL
and removed
lgtm-coderabbitai[bot]
size/S
needs-rebase
labels
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 581-583: Replace the f-string log messages with lazy `%`-style
logging to match the codebase convention: change LOGGER.info(f"Created NAD:
{nad_name} in namespace {target_namespace}") to use LOGGER.info("Created NAD: %s
in namespace %s", nad_name, target_namespace) and change LOGGER.info(f"Created
{len(created_nads)} NADs for migration: {created_nads}") to LOGGER.info("Created
%d NADs for migration: %s", len(created_nads), created_nads); edit the logging
calls where LOGGER.info is used with variables nad_name, target_namespace,
created_nads to use the `%` formatting signature so interpolation is deferred.
In `@utilities/mtv_migration.py`:
- Line 202: Fix the typo in the error message logged by LOGGER.error in
utilities/mtv_migration.py: change the string "Destinaion provider:
{dest_provider.instance}" to "Destination provider: {dest_provider.instance}" so
the message reads correctly; locate the LOGGER.error call that references
dest_provider.instance and update the f-string accordingly.
- Around line 190-230: Replace bare LOGGER.error calls in the
TimeoutExpiredError handling with LOGGER.exception so the stack trace is
preserved (references: Plan.wait_for_condition, the TimeoutExpiredError except
block that logs plan and provider instances). In the finally block where
archive_plan(plan) is called, stop using LOGGER.error for the archive exception
and instead catch Exception as archive_exc and call LOGGER.exception("Failed to
archive Plan %s", plan.name) (referencing archive_plan and the archive_exc
variable) so the full traceback is kept; keep suppressing the archive exception
to avoid masking the original error but ensure the log contains the exception
details.
In `@utilities/resources.py`:
- Around line 99-106: The condition kwargs.get("teardown", True) is redundant
because the key is always set earlier (kwargs["teardown"] = not skip_teardown);
change the check to use the direct key (e.g., if kwargs["teardown"]: ) or read
into a local variable (teardown = kwargs["teardown"]; if teardown:) and keep the
existing removal/exception handling in the block that manipulates
fixture_store["teardown"][deployed.kind] and logs via LOGGER.debug.
In `@utilities/utils.py`:
- Around line 267-268: Replace the generic ValueError with a dedicated exception
type to allow programmatic handling: add a SecretCreationError (e.g., class
SecretCreationError(ValueError)) in your exceptions module and update the raise
site in utilities/utils.py to raise SecretCreationError("Failed to create source
provider secret") where source_provider_secret is checked; ensure any callers
that catch ValueError are updated if they should handle the new
SecretCreationError explicitly.
- Around line 327-351: The loop uses the create_and_store_resource context
manager with skip_teardown=True (resource class VirtualMachineFromInstanceType)
but immediately exits the with-block, which is semantically confusing; update
the code to make intent explicit by either adding a concise inline comment above
the with-call explaining that skip_teardown=True is intentionally used to only
register the VM in fixture_store while deferring cleanup to session teardown, or
replace the pattern with a small helper function (e.g.,
register_vm_without_teardown) that calls create_and_store_resource with
skip_teardown=True and returns the VM before appending to vms_to_create so the
purpose is clear; ensure references to create_and_store_resource, skip_teardown,
VirtualMachineFromInstanceType, and vms_to_create are updated accordingly.
♻️ Duplicate comments (1)
conftest.py (1)
751-756: MEDIUM: Use LOGGER.exception for full traceback in cleanup failures.Per static analysis (TRY400) and past review feedback,
LOGGER.exceptionautomatically captures the traceback, which is critical for diagnosing intermittent cleanup failures in CI.♻️ Proposed fix
try: LOGGER.info("Deleting VM %s in plan fixture teardown", vm_obj.name) vm_obj.clean_up(wait=True) LOGGER.info("Successfully deleted VM %s", vm_obj.name) - except Exception as exc: - LOGGER.error("Failed to delete VM %s: %s", vm_obj.name, exc) + except Exception: + LOGGER.exception("Failed to delete VM %s", vm_obj.name)
redhat-qe-bot1
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
- Simplify redundant condition check in utilities/resources.py - Add custom SecretCreationError exception for clearer error handling - Replace generic ValueError with SecretCreationError in utilities/utils.py
krcmarik
changed the title
redhat-qe-bot1
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
Contributor
Author
|
/wip |
redhat-qe-bot2
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement automatic resource cleanup using Python context managers with
immediate cleanup when contexts exit, eliminating resource accumulation
during test sessions.
Key changes:
Resource cleanup order (outer to inner):
Implementation follows context manager pattern from plan:
Critical fixes applied:
Tests now use explicit with blocks showing resource lifecycle:
with create_storagemap_and_networkmap(...) as (sm, nm):
with run_migration(...) as plan:
# Use resources
pass
# Plan and Migration cleaned up here
Storage and Network maps cleaned up here
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.