OSAC-768: Add network attachment support for VMaaS tests - #45
Merged
Merged
Conversation
|
@ori-amizur: This pull request references OSAC-768 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds CLI support for per-instance network_attachments and fixtures to create a VirtualNetwork and Subnet for tests; updates vmaas tests to attach instances to that subnet, adds a session autouse OVN egress fixture, and bumps Containerfile OSAC_VERSION. Risk: Low — test/infrastructure changes only. ChangesNetwork Attachment Feature
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant GRPCClient
participant K8sClient
TestRunner->>GRPCClient: Create VirtualNetwork CR
GRPCClient->>K8sClient: Wait for VirtualNetwork CR name and Ready
TestRunner->>GRPCClient: Create Subnet CR linked to VN
GRPCClient->>K8sClient: Wait for Subnet CR name and Ready
TestRunner->>TestRunner: yield subnet_id and subnet_ref
TestRunner->>K8sClient: delete Subnet CR, wait for deletion
TestRunner->>K8sClient: delete VirtualNetwork CR, wait for deletion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 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: 5
🧹 Nitpick comments (2)
tests/core/helpers.py (1)
90-92: ⚡ Quick winDon’t suppress all exceptions in the job-state probe.
Catching
Exceptionand silently passing hides real regressions (auth, API, parsing) and removes debugging signal. Catch only expected failures and surface context.🤖 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 `@tests/core/helpers.py` around lines 90 - 92, The catch-all "except Exception:" in the job-state probe (the except Exception: block in tests/core/helpers.py) should not silently pass; replace it with explicit expected exceptions (e.g., network/parsing/auth exceptions your probe can raise such as requests.RequestException, ValueError, KeyError or whatever your probe uses) and handle them (log context or call pytest.fail) while letting unexpected exceptions bubble up (or re-raise them). In short: change "except Exception: pass" to "except (ExpectedExceptionA, ExpectedExceptionB) as e: <handle/log/fail with context>" and keep a final "except Exception: raise" only if you need to re-raise unrecognized errors so regressions aren’t suppressed.tests/vmaas/test_compute_instance_cli_fields.py (1)
26-29: ⚡ Quick winAssert the network attachment made it into the ComputeInstance spec.
This test now passes a subnet attachment, but it never verifies
spec.networkAttachments. Adding that check will make this test actually guard the new CLI behavior.Proposed assertion block
spec: dict[str, Any] = ci_spec["spec"] + attachments: list[dict[str, Any]] = spec.get("networkAttachments", []) + assert len(attachments) > 0, "networkAttachments should not be empty" + assert any( + a.get("subnet") == default_subnet or a.get("subnetRef") == default_subnet + for a in attachments + ), f"Expected subnet attachment for {default_subnet}, got {attachments}" + assert spec["cores"] == TEST_CORES, f"cores mismatch: {spec['cores']} != {TEST_CORES}"🤖 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 `@tests/vmaas/test_compute_instance_cli_fields.py` around lines 26 - 29, After creating the instance with cli.create_compute_instance (the call assigned to uuid), fetch the created ComputeInstance resource and assert that its spec.networkAttachments contains the passed attachment (e.g., contains an entry with subnet == default_subnet); specifically, locate the test's use of cli.create_compute_instance and add an assertion that the retrieved ComputeInstance.spec.networkAttachments includes the expected subnet attachment to ensure the CLI actually populated spec.networkAttachments.
🤖 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 `@tests/core/helpers.py`:
- Around line 64-65: The current fixed max_iterations = 60 (5s sleeps) caps
total wait to 300s, which makes the default max_stuck_time (600s) unreachable;
update the loop bounds to derive max_iterations from max_stuck_time and the
sleep interval (e.g., max_iterations = int(math.ceil(max_stuck_time /
sleep_seconds))) so the "stuck for max_stuck_time" branch can be reached; apply
the same change to the other loops that use the hardcoded 60 (the blocks
referenced around the other checks) and ensure the code uses the shared sleep
interval variable and max_stuck_time parameter rather than a hardcoded iteration
cap.
- Around line 84-86: The force-cleanup branch in tests/core/helpers.py calls
k8s.remove_finalizers(resource="computeinstance", name=name), sleeps 5s and
returns without verifying deletion, which can race with downstream checks;
replace the blind sleep+return with a polling loop that checks for the CR's
absence (e.g., repeatedly call k8s.get or a provided
k8s.exists/wait_for_deletion helper with a timeout and short sleep interval)
until the resource is gone or timeout; apply the same change to the other
identical block (lines referenced as 97-100) so both force-cleanup paths confirm
the CR is deleted before returning.
In `@tests/core/k8s_client.py`:
- Around line 44-45: The subprocess.run call that executes kubectl
(subprocess.run(args, input=manifest, capture_output=True, text=True,
check=True)) is missing a timeout and can hang CI; add a reasonable timeout
argument (e.g., timeout=30) to that subprocess.run invocation, and update error
handling to catch subprocess.TimeoutExpired in addition to
subprocess.CalledProcessError so the test fails fast and emits a clear error
(refer to subprocess.run, subprocess.CalledProcessError,
subprocess.TimeoutExpired, args, and manifest to locate the code).
In `@tests/core/osac_cli.py`:
- Around line 57-69: The loop handling network_attachments in
tests/core/osac_cli.py currently allows invalid entries like subnet=None; update
the validation in the for attachment in network_attachments block to (1) require
that attachment.get("subnet") returns a non-empty string (raise a ValueError
with a clear message referencing the attachment when not), (2) ensure
attachment.get("security_groups") if present is a list and every element is a
non-empty string (raise TypeError/ValueError if not), and (3) only build parts
and call args.extend(["--network-attachment", ",".join(parts)]) when validation
passes; include the variable names subnet and security_groups in the error
messages to aid debugging.
In `@tests/vmaas/conftest.py`:
- Around line 60-125: The fixture default_networking currently creates vn_id and
subnet_id before yield but only performs cleanup after yield; wrap the setup and
yield in a try/finally so any partial failures still trigger teardown: move the
yield inside a try block and place the existing cleanup logic in the finally
block, track whether vn_id/vn_cr_name and subnet_id/subnet_cr_name were set
(e.g., check for None) and only call grpc.delete_subnet,
grpc.delete_virtual_network, wait_for_*_deletion and
k8s_hub_client.remove_finalizers for resources that were actually created; keep
using the same helper names (create_virtual_network, create_subnet,
wait_for_virtual_network_cr, wait_for_subnet_cr, wait_for_virtual_network_ready,
wait_for_subnet_ready, wait_for_subnet_deletion,
wait_for_virtual_network_deletion) to locate where to insert the try/finally and
conditional cleanup checks.
---
Nitpick comments:
In `@tests/core/helpers.py`:
- Around line 90-92: The catch-all "except Exception:" in the job-state probe
(the except Exception: block in tests/core/helpers.py) should not silently pass;
replace it with explicit expected exceptions (e.g., network/parsing/auth
exceptions your probe can raise such as requests.RequestException, ValueError,
KeyError or whatever your probe uses) and handle them (log context or call
pytest.fail) while letting unexpected exceptions bubble up (or re-raise them).
In short: change "except Exception: pass" to "except (ExpectedExceptionA,
ExpectedExceptionB) as e: <handle/log/fail with context>" and keep a final
"except Exception: raise" only if you need to re-raise unrecognized errors so
regressions aren’t suppressed.
In `@tests/vmaas/test_compute_instance_cli_fields.py`:
- Around line 26-29: After creating the instance with
cli.create_compute_instance (the call assigned to uuid), fetch the created
ComputeInstance resource and assert that its spec.networkAttachments contains
the passed attachment (e.g., contains an entry with subnet == default_subnet);
specifically, locate the test's use of cli.create_compute_instance and add an
assertion that the retrieved ComputeInstance.spec.networkAttachments includes
the expected subnet attachment to ensure the CLI actually populated
spec.networkAttachments.
🪄 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: Enterprise
Run ID: 876a90d4-e9ec-498c-9599-56f8c78c3478
📒 Files selected for processing (12)
tests/conftest.pytests/core/helpers.pytests/core/k8s_client.pytests/core/osac_cli.pytests/core/runner.pytests/vmaas/conftest.pytests/vmaas/test_compute_instance_api_fields.pytests/vmaas/test_compute_instance_cli_fields.pytests/vmaas/test_compute_instance_creation.pytests/vmaas/test_compute_instance_delete_during_provision.pytests/vmaas/test_compute_instance_restart.pytests/vmaas/test_compute_instance_restart_negative.py
eranco74
reviewed
May 24, 2026
eranco74
left a comment
eranco74
left a comment
Contributor
There was a problem hiding this comment.
The networking part looks good.
I don't think we need the rest though
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 `@tests/core/runner.py`:
- Around line 13-23: The run() function currently only catches
subprocess.CalledProcessError; update it to also catch subprocess.TimeoutExpired
and re-raise a RuntimeError with the same enriched context (include the command
args, the timeout value, and any available stdout/stderr) so timeouts are
formatted consistently with CalledProcessError handling; reference the run()
function and handle subprocess.TimeoutExpired similarly to how
CalledProcessError (e) is processed, combining a clear message with
e.stdout/e.stderr and the timeout attribute before raising.
In `@tests/vmaas/conftest.py`:
- Around line 99-147: The finally block only runs robust teardown when both
vn_id/vn_cr_name and subnet_id/subnet_cr_name are present, causing leaked
resources on partial failures; modify the cleanup logic in the exception/finally
path to handle VN and Subnet independently: if vn_id and vn_cr_name exist run
the virtual network deletion sequence (grpc.delete_virtual_network,
wait_for_virtual_network_deletion, and fallback
k8s_hub_client.remove_finalizers), and if subnet_id and subnet_cr_name exist run
the subnet deletion sequence (grpc.delete_subnet, wait_for_subnet_deletion, and
fallback remove_finalizers); ensure these independent cleanup attempts occur
both in the except block for partial setup failures and in finally so either
resource gets the robust wait+finalizer removal logic even when the other
resource was not created.
🪄 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: Enterprise
Run ID: c167cdfc-c1c8-4701-9f87-a9f8fac4d25d
📒 Files selected for processing (12)
tests/conftest.pytests/core/helpers.pytests/core/k8s_client.pytests/core/osac_cli.pytests/core/runner.pytests/vmaas/conftest.pytests/vmaas/test_compute_instance_api_fields.pytests/vmaas/test_compute_instance_cli_fields.pytests/vmaas/test_compute_instance_creation.pytests/vmaas/test_compute_instance_delete_during_provision.pytests/vmaas/test_compute_instance_restart.pytests/vmaas/test_compute_instance_restart_negative.py
✅ Files skipped from review due to trivial changes (1)
- tests/core/helpers.py
Comment on lines
+13
to
+23
| try: | ||
| result = subprocess.run(args, capture_output=True, text=True, timeout=timeout, check=True) | ||
| return result.stdout.strip() | ||
| except subprocess.CalledProcessError as e: | ||
| # Re-raise with stderr included in the error message for better debugging | ||
| error_msg = f"Command {args} failed with exit code {e.returncode}" | ||
| if e.stderr: | ||
| error_msg += f"\n\nSTDERR:\n{e.stderr.strip()}" | ||
| if e.stdout: | ||
| error_msg += f"\n\nSTDOUT:\n{e.stdout.strip()}" | ||
| raise RuntimeError(error_msg) from e |
There was a problem hiding this comment.
Wrap subprocess.TimeoutExpired in tests/core/runner.py::run()
run() wraps subprocess.CalledProcessError into RuntimeError, but subprocess.run(..., timeout=timeout) can also raise subprocess.TimeoutExpired, which currently escapes unwrapped and loses the existing error formatting/context.
Proposed fix
def run(*args: str, timeout: int = 300) -> str:
try:
result = subprocess.run(args, capture_output=True, text=True, timeout=timeout, check=True)
return result.stdout.strip()
+ except subprocess.TimeoutExpired as e:
+ error_msg = f"Command {args} timed out after {timeout}s"
+ if e.stderr:
+ error_msg += f"\n\nSTDERR:\n{str(e.stderr).strip()}"
+ if e.stdout:
+ error_msg += f"\n\nSTDOUT:\n{str(e.stdout).strip()}"
+ raise RuntimeError(error_msg) from e
except subprocess.CalledProcessError as e:
# Re-raise with stderr included in the error message for better debugging
error_msg = f"Command {args} failed with exit code {e.returncode}"
if e.stderr:
error_msg += f"\n\nSTDERR:\n{e.stderr.strip()}"
if e.stdout:
error_msg += f"\n\nSTDOUT:\n{e.stdout.strip()}"🧰 Tools
🪛 Ruff (0.15.13)
[error] 14-14: subprocess call: check for execution of untrusted input
(S603)
🤖 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 `@tests/core/runner.py` around lines 13 - 23, The run() function currently only
catches subprocess.CalledProcessError; update it to also catch
subprocess.TimeoutExpired and re-raise a RuntimeError with the same enriched
context (include the command args, the timeout value, and any available
stdout/stderr) so timeouts are formatted consistently with CalledProcessError
handling; reference the run() function and handle subprocess.TimeoutExpired
similarly to how CalledProcessError (e) is processed, combining a clear message
with e.stdout/e.stderr and the timeout attribute before raising.
Contributor
|
/approve |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/vmaas/conftest.py (1)
99-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPartial setup failures still risk leaked networking resources (Severity: Major).
Line 116 gates robust teardown on all VN+Subnet identifiers being present, so a mid-setup failure can skip deletion-wait/finalizer fallback for whichever resource was created. Impact: leftover CRs can poison later test sessions and cause cascading flakes.
Suggested fix
- finally: - # Normal cleanup runs regardless of setup success/failure - # Only attempt cleanup if resources were successfully created - if vn_id and vn_cr_name and subnet_id and subnet_cr_name: + finally: + # Cleanup subnet independently if created + if subnet_id and subnet_cr_name: print(f"\nCleaning up test networking resources: {test_run_id}") - - # Delete subnet first try: print(f"Deleting Subnet {subnet_id}...") grpc.delete_subnet(subnet_id=subnet_id) wait_for_subnet_deletion(k8s=k8s_hub_client, name=subnet_cr_name) print(f"Subnet {subnet_id} deleted") except Exception as e: print(f"WARNING: Failed to delete subnet {subnet_id}: {e}") + try: + k8s_hub_client.remove_finalizers(resource="subnet", name=subnet_cr_name) + time.sleep(5) + except Exception as cleanup_error: + print(f"WARNING: Failed to force-cleanup subnet: {cleanup_error}") - # Delete virtual network + # Cleanup virtual network independently if created + if vn_id and vn_cr_name: try: print(f"Deleting VirtualNetwork {vn_id}...") grpc.delete_virtual_network(vn_id=vn_id) wait_for_virtual_network_deletion(k8s=k8s_hub_client, name=vn_cr_name) print(f"VirtualNetwork {vn_id} deleted") except Exception as e: print(f"WARNING: Failed to delete virtual network {vn_id}: {e}") + try: + k8s_hub_client.remove_finalizers(resource="virtualnetwork", name=vn_cr_name) + time.sleep(5) + except Exception as cleanup_error: + print(f"WARNING: Failed to force-cleanup virtualnetwork: {cleanup_error}")🤖 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 `@tests/vmaas/conftest.py` around lines 99 - 136, The finally block only runs deletion when all four identifiers (vn_id, vn_cr_name, subnet_id, subnet_cr_name) are truthy, which leaks resources created before a mid-setup failure; change the cleanup to attempt deletion of each resource independently: check subnet_id and subnet_cr_name and run grpc.delete_subnet + wait_for_subnet_deletion (handle exceptions and log warnings), and separately check vn_id and vn_cr_name and run grpc.delete_virtual_network + wait_for_virtual_network_deletion (handle exceptions and log warnings); if both exist prefer deleting subnet first as currently done, but do not gate one on the other so partial creations are always cleaned up (refer to the variables vn_id, vn_cr_name, subnet_id, subnet_cr_name and functions grpc.delete_subnet, grpc.delete_virtual_network, wait_for_subnet_deletion, wait_for_virtual_network_deletion).
🤖 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.
Duplicate comments:
In `@tests/vmaas/conftest.py`:
- Around line 99-136: The finally block only runs deletion when all four
identifiers (vn_id, vn_cr_name, subnet_id, subnet_cr_name) are truthy, which
leaks resources created before a mid-setup failure; change the cleanup to
attempt deletion of each resource independently: check subnet_id and
subnet_cr_name and run grpc.delete_subnet + wait_for_subnet_deletion (handle
exceptions and log warnings), and separately check vn_id and vn_cr_name and run
grpc.delete_virtual_network + wait_for_virtual_network_deletion (handle
exceptions and log warnings); if both exist prefer deleting subnet first as
currently done, but do not gate one on the other so partial creations are always
cleaned up (refer to the variables vn_id, vn_cr_name, subnet_id, subnet_cr_name
and functions grpc.delete_subnet, grpc.delete_virtual_network,
wait_for_subnet_deletion, wait_for_virtual_network_deletion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d75e7470-3a06-4bc6-93af-b0d6c4a15618
📒 Files selected for processing (9)
Containerfiletests/core/osac_cli.pytests/vmaas/conftest.pytests/vmaas/test_compute_instance_api_fields.pytests/vmaas/test_compute_instance_cli_fields.pytests/vmaas/test_compute_instance_creation.pytests/vmaas/test_compute_instance_delete_during_provision.pytests/vmaas/test_compute_instance_restart.pytests/vmaas/test_compute_instance_restart_negative.py
Contributor
Author
|
/retest |
Contributor
|
/lgtm |
Contributor
|
/retest |
Contributor
|
/retest |
1 similar comment
Contributor
|
/retest |
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Update all compute instance tests to use explicit network attachments, ensuring VMs are provisioned on OSAC-managed subnets instead of the default pod network. Changes: 1. Add network_attachments parameter to OsacCLI.create_compute_instance() - Supports subnet and security-groups configuration - Builds --network-attachment CLI flags - Validates attachment structure 2. Add networking fixtures in tests/vmaas/conftest.py: - test_run_id: Unique ID per test run to avoid resource conflicts - default_networking: Creates VirtualNetwork + Subnet with cleanup - default_subnet: Returns subnet ID for CLI/gRPC usage - default_subnet_ref: Returns subnet CR name for K8s API usage 3. Update all compute instance tests to use network attachments: - test_compute_instance_api_fields: Use default_subnet_ref - test_compute_instance_cli_fields: Use default_subnet - test_compute_instance_creation: Use default_subnet - test_compute_instance_delete_during_provision: Use default_subnet - test_compute_instance_restart: Use default_subnet - test_compute_instance_restart_negative: Use default_subnet This ensures VMs are created with proper tenant network isolation, IP management, and security group support rather than using the default Kubernetes pod network (masquerade). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
/retest |
Contributor
|
/lgtm |
Contributor
Author
|
/retest |
2 similar comments
Contributor
|
/retest |
Contributor
|
/retest |
ori-amizur
enabled auto-merge (squash)
June 2, 2026 10:52
Contributor
|
/lgtm |
Contributor
Author
|
/retest |
Contributor
Author
|
/test e2e-vmaas |
Contributor
|
/retest |
1 similar comment
Contributor
|
/retest |
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eranco74, omer-vishlitzky, ori-amizur The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
|
/retest ci/prow/images |
Contributor
|
/test images |
2 tasks
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
4 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.
Summary by CodeRabbit
Tests
Chores
Tests