OSAC-3593: Sync test changes from osac-test-infra PRs #367, #410, #417 - #645
Merged
osac-ci-bot merged 3 commits intoAug 31, 2026
Merged
Conversation
…oject#367) Implements E2E test infrastructure and test cases for per-disk storage tier selection in ComputeInstances: - **Test infrastructure**: Session-scoped StorageBackend + StorageTier fixtures with autouse CLI integration - **CLI wrapper enhancements**: `additional_disks` parameter support (size_gib + storage_tier per disk) - **13 test cases**: Core tier selection, validation, immutability, and default resolution chain (CatalogItem FieldDefinition → Template SpecDefaults → session default) All tests validate both CR spec fields (via K8s API) and gRPC API responses. Two tests skipped pending mandatory validation in a follow-up osac PR. https://issues.redhat.com/browse/OSAC-3633 - [x] All existing tests pass (vmaas suite) - [x] New storage tier tests pass locally - [x] CLI wrapper validates storage_tier presence in additional_disks - [x] Lint checks pass (ruff not available locally, CI will validate) - [x] No secrets or credentials in changes - [x] Pre-flight security review: PASS (advisory findings only, untracked files) - [x] Pre-flight performance review: PASS (advisory findings only, untracked files) --- _This PR description was drafted with AI assistance ([create-pr](https://github.com/osac-project/osac-workspace/tree/main/skills/create-pr) v0.1.3). Review for accuracy_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Added private-mode authentication. - Added default disk image and storage-tier settings for compute instances. - Added separate storage tiers for boot and additional disks. - Added GPU options for instance types and optional cluster versions. - Added bare-metal instance description support. - **Bug Fixes** - Improved validation for missing or invalid storage tiers. - Corrected catalog defaults for disk images, instance types, and versions. - **Tests** - Expanded coverage for storage-tier persistence, overrides, defaults, and immutability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
…ct#410) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ori Amizur <oamizur@redhat.com>
…ent (osac-project#417) ## Summary - Move `protocol` from each `BackendAssociation` up to `StorageTierSpec` in `create_storage_tier`, matching the [OSAC-3014](https://redhat.atlassian.net/browse/OSAC-3014) storage tier API. - Fixes VMaaS e2e (`test_compute_instance_storage_tier`) which fails with `BackendAssociation has no known field named protocol` against the new API. - Depends on osac-project#292; this is a hard cutover (no payload satisfies both old and new APIs), so merge only once that API change is deployed to the e2e target. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved storage-tier creation requests by using a single protocol setting and simplifying backend configuration. * Standardized storage-tier retrieval request formatting and filter handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
@amej: This pull request references OSAC-3593 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.1.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. |
|
Important Review skippedToo many files! This PR contains 310 files, which is 10 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. ⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (151)
📒 Files selected for processing (310)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
🤖 Finished Review · ✅ Success · Started 12:58 PM UTC · Completed 1:17 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.39 |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
amej
marked this pull request as ready for review
August 31, 2026 13:21
osac-ci-bot
dismissed
fullsend-ai-review[bot]’s stale review
August 31, 2026 13:21
Auto-dismissed: only Prow labels gate merging
osac-ci-bot
enabled auto-merge
August 31, 2026 13:22
|
🤖 Review · ❌ Terminated · Started 1:23 PM UTC · Ended 1:36 PM UTC Commit: |
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amej, minmzzhang 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 |
E2E on
|
| { | ||
| "path": "disk_image", | ||
| "display_name": "Disk Image", | ||
| "editable": True, |
There was a problem hiding this comment.
[medium] nil-deref
Six test functions call wait_for_deletion(k8s=k8s_hub_client, name=ci_name) without checking if ci_name is None in their inner finally blocks. If wait_for_cr fails, ci_name remains None and the teardown throws an unrelated TypeError masking the original failure. Affected lines: 806, 928, 1042, 1148, 1257, 1364. Other tests in the same file (e.g. line 484) correctly guard with if ci_name is not None.
Suggested fix: Add if ci_name is not None: before each wait_for_deletion call, matching the pattern used in test_compute_instance_explicit_boot_disk_tier and test_compute_instance_multiple_disks_different_tiers.
| @@ -101,6 +105,34 @@ def create_compute_instance( | |||
| else: | |||
There was a problem hiding this comment.
[medium] api-contract
OsacCLI.create_compute_instance now unconditionally raises ValueError when neither boot_disk_storage_tier nor default_storage_tier is set. This makes storage_tier a hard requirement for ALL CLI-based compute instance creation. The _set_cli_default_storage_tier autouse fixture only applies to tests under tests/vmaas/. Any future test outside that directory calling create_compute_instance via OsacCLI would fail. Currently no such callers exist, but the risk is latent.
Suggested fix: Either make boot_disk_storage_tier optional (only pass --boot-disk-storage-tier when a value is available), or move the default_storage_tier fixture to the top-level tests/conftest.py.
| grpc.delete_compute_instance(ci_id=uuid) | ||
| wait_for_deletion(k8s=k8s_hub_client, name=ci_name) | ||
| finally: | ||
| private_grpc.delete_compute_instance_catalog_item(catalog_item_id=catalog_item_id) |
There was a problem hiding this comment.
[low] logic-error
verify_datavolume_storage_classes retrieves vmi_ns via get_compute_instance_vm_namespace but never uses it. The DataVolume lookup calls use k8s_virt_client.get_json which scopes to self.namespace (osac-devel), not the VMI namespace. Currently commented out at all call sites (no runtime impact) with a note referencing PR #257.
Suggested fix: When uncommenting after PR #257, pass vmi_ns to get_datavolume_storage_class or construct a properly-scoped client.
| "spec": { | ||
| "catalog_item": {"id": catalog_item_id}, | ||
| "instance_type": {"name": default_instance_type}, | ||
| "boot_disk": {"size_gib": 20}, # Uses CatalogItem default tier |
There was a problem hiding this comment.
[low] code-organization
import json inside test_compute_instance_tier_immutability is a function-body import. The codebase convention is module-level imports.
Suggested fix: Move import json to the top of the file alongside the other imports.
|
🤖 Finished Review · ✅ Success · Started 1:23 PM UTC · Completed 1:36 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.72 |
Merged
via the queue into
osac-project:main
with commit Aug 31, 2026
4148211
146 of 153 checks passed
Closed
6 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
7 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.
At time of publishing this PR ( Aug 31 2026 )
Whats present in the osac-test-infra but missing from the osac mono repo are