Repository navigation
MGMT-22783: add pytest e2e test suite for vmaas - #18
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
@omer-vishlitzky: This pull request references MGMT-22783 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 "4.22.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. |
|
@omer-vishlitzky: This pull request references MGMT-22783 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 "4.22.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. |
b535310 to
fbbdafe
Compare
eranco74
left a comment
There was a problem hiding this comment.
Well-structured pytest port with solid architecture (runner/client abstraction layers, two-kubeconfig design). A few items to address:
Must fix
k8s_virt_client crashes with KeyError when OSAC_VM_KUBECONFIG is not set (conftest.py:51)
os.environ["OSAC_VM_KUBECONFIG"] raises a raw KeyError if the env var is missing, but the README says it defaults to the hub kubeconfig. Fix:
vm_kubeconfig = os.environ.get("OSAC_VM_KUBECONFIG")
return K8sClient(namespace=namespace, kubeconfig=vm_kubeconfig)Should fix
No cleanup on test failure
If a test fails after creating a compute instance but before the cleanup section, the resource leaks. Over time this causes resource exhaustion in CI. Use request.addfinalizer() or yield fixtures for cleanup.
cli fixture duplicates fulfillment_address derivation (conftest.py)
The cli fixture re-derives the fulfillment address instead of depending on the existing fulfillment_address fixture. Use the fixture to avoid duplication and potential inconsistency.
Worth noting
- Session-scoped token with
--duration 1hmay expire during long test runs (total possible poll time across all tests exceeds 1h) run_uncheckedmixes stdout/stderr into one string — works now but fragile for assertions that match on error messages- UUID parsing from CLI output (
re.search(r"'([^']+)'", stdout)) could silently grab wrong value if output format changes; a UUID regex would be safer - Inconsistent resource type references:
"computeinstance"vs"computeinstance.osac.openshift.io"— pick one convention - No lock file committed despite
uv syncin README — builds not reproducible
fbbdafe to
9666318
Compare
|
Self-contained image with all tools needed to run the e2e test suite on a bare metal machine via podman. Depends on osac-project#18 (pyproject.toml and uv.lock must exist).
fc33812 to
2f75918
Compare
|
/hold |
There was a problem hiding this comment.
what do you think about BDD? I think it would be great to have way to express test in this way, for example like in NetworkManager repo: https://gitlab.freedesktop.org/NetworkManager/NetworkManager-ci/-/merge_requests/1971/diffs
There was a problem hiding this comment.
can't we build a python fulfillment-service client from the proto files?
| self.kubeconfig: str | None = kubeconfig | ||
|
|
||
| def _base(self) -> list[str]: | ||
| args: list[str] = ["kubectl"] |
There was a problem hiding this comment.
why not use python k8s client?
| run_strategy, | ||
| ] | ||
| if user_data_secret_ref is not None: | ||
| args.extend(["--user-data-secret-ref", user_data_secret_ref]) |
There was a problem hiding this comment.
| args.extend(["--user-data-secret-ref", user_data_secret_ref]) | |
| args.extend(["--user-data", user_data_secret_ref]) |
it was renamed in osac-project/fulfillment-service#333
eranco74
left a comment
There was a problem hiding this comment.
Previous concerns addressed or reasonably justified:
- ✅
clifixture duplication — fixed, now usesfulfillment_addressfixture - ✅ Resource type consistency — standardized on
"computeinstance" - ✅ Lock file —
uv.lockcommitted ⚠️ k8s_virt_clientKeyError — intentional fail-fast, README updated to mark as required- ❌ Cleanup on failure — CI-only, machines returned to pool, acceptable
LGTM. The remaining items (token expiry, stdout/stderr mixing, UUID parsing) are minor and can be improved in follow-ups. adriengentil's architectural suggestions (native Python clients, BDD) are good long-term goals but out of scope here.
2f75918 to
5525d27
Compare
Port all ansible e2e tests to pytest with 1:1 parity. The hub creation test was removed — hub creation is implicitly verified by every compute instance test since they all go through the fulfillment api on the hub. Tests support a two-cluster topology where the hub cluster runs the osac operator and the remote cluster runs the kubevirt VMs. The k8s client takes a kubeconfig parameter, and tests that need to inspect VMs on the remote cluster use a separate OSAC_VM_KUBECONFIG env var. See MGMT-23623 for the remote cluster setup script. Tests: - compute instance lifecycle (create, wait for running, delete) - delete during provision - restart (trigger via grpc, verify new vmi creation timestamp) - restart negative (past timestamp ignored) - api fields (explicit cpu/memory/disk via grpc) - cli fields (explicit cpu/memory/disk via fulfillment-cli) Infrastructure: - k8s client with two-kubeconfig support (hub + remote) - grpc client wrapping grpcurl - fulfillment-cli wrapper with typed methods - poll_until helper matching ansible retry semantics - Makefile with MAKEFILE_TARGET dispatch for ci - pyproject.toml with ruff and basedpyright config
5525d27 to
19c02cf
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, eranco74, omer-vishlitzky 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 |
|
/unhold |
https://redhat.atlassian.net/browse/MGMT-22783
Port all ansible e2e tests to pytest with 1:1 parity. The hub creation test was removed — hub creation is implicitly verified by every compute instance test since they all go through the fulfillment api on the hub.
Tests support a two-cluster topology where the hub cluster runs the osac operator and the remote cluster runs the kubevirt vms. The k8s client takes a kubeconfig parameter, and tests that need to inspect vms on the remote cluster use a separate
OSAC_VM_KUBECONFIGenv var. See MGMT-23623 for the remote cluster setup script.Tests: