CNTRLPLANE-3863: add e2e v2 test flow document - #9151
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
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:
📝 WalkthroughWalkthroughAdds comprehensive documentation for the HyperShift v2 E2E test flow, covering Ginkgo lifecycle and labels, process and cluster isolation, CI orchestration, ordered test groups, inter-process communication, teardown, exit-code handling, JUnit reporting, and links from related CI documentation. Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ 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 |
openshift-ci
Bot
added
area/documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/content/how-to/ci/v2-testing/test-flow.md`:
- Line 324: Update the process table entry for create-guests to hyphenate “pre
step” as “pre-step,” leaving the rest of the documentation unchanged.
- Around line 68-87: Update the fenced diagram block in the test-flow
documentation to declare the text language using a `text` fence, while
preserving all existing diagram content unchanged.
- Line 129: Rename the Mermaid participant alias from TE to TOIDC at
docs/content/how-to/ci/v2-testing/test-flow.md lines 129-129, then update all
corresponding dispatch and activation references at lines 209-210 and return and
deactivation references at lines 236-237; do not use THE, and run make
verify-codespell.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e41559a-9682-4d68-a30c-bacfa02df7ca
📒 Files selected for processing (2)
docs/content/how-to/ci/v2-testing/test-flow.mddocs/mkdocs.yml
8f0f415 to
12aead2
Compare
ironcladlou
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ironcladlou: This pull request references CNTRLPLANE-3863 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 epic 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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
12aead2 to
8dda0fb
Compare
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Do we want to add any information to this from our lessons learned about OTE, and what we can and cannot do to make the new v2 tests OTE compatible?
| In practice, since `run-tests` does not pass `--procs` to Ginkgo, all specs within | ||
| a process already run sequentially — but `Serial` makes the constraint explicit and | ||
| future-proof. |
There was a problem hiding this comment.
OpenShift deliberately run their tests with a high level of parallelism (for parallel capable tests), almost as a stress test for the cluster. Is there any reason we aren't doing the same in HyperShift?
There was a problem hiding this comment.
I don't know why Ginkgo procs was left at 1 during the design of the v2 testing framework (maybe @csrwng or @bryan-cox knows?), but the fact that it has been serial since the beginning means I won't be surprised if it exposes issues with test isolation if we turn it on. I haven't tested it yet. Might just be another source of bug fixes that need made in anticipation of the OTE process isolation model that can be done in-place within the current framework
There was a problem hiding this comment.
Just some background on parallelism and stress testing: The parallelism we typically run at (30 tests concurrently) normally, a decision made as far as I can tell by gut feel many years ago, actually poses a lot of problems for us as we've (1) gotten better at detecting when tests are failing concurrently with a node being CPU pegged, and (2) ever increasing testing of more and more complex cluster wide features. I think this was a big part of the reason behind the historical belief that openshift tests are flaky (which they were) and the free re-try mechanism we used to give all e2e's.
In recent releases we have an ever growing list of blocks of tests being broken out to run separately at lower parallelization, down to 20 or even 10. It's looking more and more like a never ending and very vague trial and error process. As a stress test it can be frustrating as you can tell is regressions are real, if the product is slower, or if the tests run are different in semi-random buckets, and which of hundreds of concurrent tests might be causing the problem. (no one has been able to crack that yet)
I've been leaning more on our perfscale team which run stable and intense workloads and do deeper analysis, and trying to steer away from endless or borderline impossible debugging efforts when we see regressed tests correlated with high CPU load. I'd encourage parallelization for speed when the cluster can handle the resources, but it gets into the dark arts if you're going to try to use it for scale testing / regression analysis.
There was a problem hiding this comment.
Very interesting notes, thanks
| | Label | Effect | | ||
| |-------|--------| | ||
| | **`lifecycle`** | Marks tests that mutate cluster state (upgrades, nodepool scaling, etcd chaos, global pull secret, OS image stream, autoscaling, platform-specific lifecycle). The simple [`hypershift-e2e-v2` CI chain][e2e-v2-chain] filters these out with `--ginkgo.label-filter='!lifecycle'` so that read-only compliance runs don't trigger mutations. The `run-tests` orchestrator runs lifecycle tests on dedicated clusters via specific label filters. | | ||
| | **`Informing`** | The custom [`InformingAwareFailHandler`][fail-handler] converts failures on specs with this label into skips. The test appears as "skipped" in JUnit XML rather than "failed", so it doesn't block the CI job. Used for tests validating optional or in-progress features (e.g., metrics forwarding, custom labels/tolerations). | |
There was a problem hiding this comment.
Have you looked at how the openshift-tests(-extension) implements informing tests? I think rather than skips, it adds both the failed JUnit and a passing JUnit, which IIRC makes it so that Prow picks up the test as a flake (it looks like the test was retried and passed on the second attempt).
That might be better than skipped as I imagine there are genuine skips mixed in and we won't easily tell the difference?
There was a problem hiding this comment.
Yeah, I already made a targeted (end to end tested) targeted fix for this issue we could merge right now, I closed the previous PR since we didn't have a good sense of how viable OTE port would be in the short term. Now I think I should re-open a PR and get this data flowing from our existing tests.
Here's the fix:
main...ironcladlou:hypershift:ws/junit-informing
There was a problem hiding this comment.
Just for reference, my proposal to close the informing failures gap in the interim is here #9168
|
|
||
| ### Labels | ||
|
|
||
| | Label | Effect | |
There was a problem hiding this comment.
Any semantics for platform choice or feature gating yet? Or defer until OTE provides that?
There was a problem hiding this comment.
If I understand the question correctly, I don't think that exists yet in the current form... platform gates and capability checks are handled at the spec level (i.e. within each It()) and the labels seem primarily used to enable the static "test group to cluster mapping" for a given platform (and the informing label).
I thought about it but for this document I decided to make it self contained and purely descriptive of the architecture that exists in the context of its original design intent to avoid conflating/confusing content with speculative refactors to new architectures. My hope is that by having an authoritative description of the current design at the appropriate level of detail it'll be something reliable to feed into analysis designed to synthesize the current design with speculative designs. In hindsight I think I should have produced this document first, and a similar one for OTE itself to drive the gap analysis |
| with each other: the [**`run-tests` orchestrator**][run-tests] isolates test groups | ||
| into separate OS processes targeting different clusters, and **within each process**, | ||
| Ginkgo decorators and hooks manage execution order, state mutation, cleanup, and | ||
| reporting semantics. |
There was a problem hiding this comment.
Is this randomly assigned to diff clusters, or based on logic about the tests themselves, i.e. we can't combine some because they'll conflict?
There was a problem hiding this comment.
Answering my own Q, it does not look random, it's via labels and characteristics of the cluster. (private/public etc)
There was a problem hiding this comment.
The decision making around how things are scheduled seems somewhat arbitrary to me (and like, wouldn't we want to run the upgrade (and all other platform & capability agnostic tests) against all cluster variants? The current strategy is to ultimately run each spec exactly once "somewhere" amongst the test clusters, and how those are scheduled is by hand and according to some I think under-specified rules. Maybe someone can correct me if I'm wrong (and again my intent here is to describe what actually exists today)
|
|
||
| | Decorator | Purpose | Used by | | ||
| |-----------|---------|---------| | ||
| | **`Ordered`** | Specs in the container run in declaration order. If one fails, subsequent specs in the same container are skipped. Prevents dependent steps from running against corrupted state. | [BackupRestore, EtcdSnapshot][backup-restore-test], [EtcdChaos][etcd-chaos-test], [AzurePrivateLink, AzureEndpointAccess][azure-test], [PKI operator TLS modification][pki-test], [AdmissionPolicies][security-test], [ImageRegistryCapability][image-registry-test], [ExternalOIDCKeycloakAuth][external-oidc-test] | |
There was a problem hiding this comment.
The bail out on failure could be confusing at times in our signal, it can manifest as a test that just stopped running, but that's almost invisible to those of us watching CI signal. I'd be curious how often this happens, is there any data on skip rates due to prior failures? I'm not sure sippy would even represent those skips in it's database, it's likely just manifesting as test runs that either pass, or don't appear at all.
There was a problem hiding this comment.
Good q, worth adding to our notes of potential gaps to follow up on assuming I've got the current behavior described accurately
| | Decorator | Purpose | Used by | | ||
| |-----------|---------|---------| | ||
| | **`Ordered`** | Specs in the container run in declaration order. If one fails, subsequent specs in the same container are skipped. Prevents dependent steps from running against corrupted state. | [BackupRestore, EtcdSnapshot][backup-restore-test], [EtcdChaos][etcd-chaos-test], [AzurePrivateLink, AzureEndpointAccess][azure-test], [PKI operator TLS modification][pki-test], [AdmissionPolicies][security-test], [ImageRegistryCapability][image-registry-test], [ExternalOIDCKeycloakAuth][external-oidc-test] | | ||
| | **`Serial`** | Specs never run concurrently with other specs, even if Ginkgo parallel mode were enabled. Applied alongside `Ordered` when a test mutates shared cluster state that could interfere with other specs. | [BackupRestore, EtcdSnapshot][backup-restore-test] (separate binary), [PKI operator TLS modification][pki-test] | |
There was a problem hiding this comment.
Are serial tests all combined in the same prow job runs as ordered/unordered tests? Are they run on specific clusters together?
The implications of how this was implemented are interesting for ocp as a whole, one job, multiple clusters, tests scheduled against each. This is not something I've seen elsewhere in our main signal jobs.
There was a problem hiding this comment.
I think the answer would be in the sequence diagram and the serial/parallel handling diagram, a group of specs is dispatched to a cluster, with no Ginkgo parallelism enabled, and the usual in-process Ginkgo ordering semantics apply
| In practice, since `run-tests` does not pass `--procs` to Ginkgo, all specs within | ||
| a process already run sequentially — but `Serial` makes the constraint explicit and | ||
| future-proof. |
There was a problem hiding this comment.
Just some background on parallelism and stress testing: The parallelism we typically run at (30 tests concurrently) normally, a decision made as far as I can tell by gut feel many years ago, actually poses a lot of problems for us as we've (1) gotten better at detecting when tests are failing concurrently with a node being CPU pegged, and (2) ever increasing testing of more and more complex cluster wide features. I think this was a big part of the reason behind the historical belief that openshift tests are flaky (which they were) and the free re-try mechanism we used to give all e2e's.
In recent releases we have an ever growing list of blocks of tests being broken out to run separately at lower parallelization, down to 20 or even 10. It's looking more and more like a never ending and very vague trial and error process. As a stress test it can be frustrating as you can tell is regressions are real, if the product is slower, or if the tests run are different in semi-random buckets, and which of hundreds of concurrent tests might be causing the problem. (no one has been able to crack that yet)
I've been leaning more on our perfscale team which run stable and intense workloads and do deeper analysis, and trying to steer away from endless or borderline impossible debugging efforts when we see regressed tests correlated with high CPU load. I'd encourage parallelization for speed when the cluster can handle the resources, but it gets into the dark arts if you're going to try to use it for scale testing / regression analysis.
| |-------|--------| | ||
| | **`lifecycle`** | Marks tests that mutate cluster state (upgrades, nodepool scaling, etcd chaos, global pull secret, OS image stream, autoscaling, platform-specific lifecycle). The simple [`hypershift-e2e-v2` CI chain][e2e-v2-chain] filters these out with `--ginkgo.label-filter='!lifecycle'` so that read-only compliance runs don't trigger mutations. The `run-tests` orchestrator runs lifecycle tests on dedicated clusters via specific label filters. | | ||
| | **`Informing`** | The custom [`InformingAwareFailHandler`][fail-handler] converts failures on specs with this label into skips. The test appears as "skipped" in JUnit XML rather than "failed", so it doesn't block the CI job. Used for tests validating optional or in-progress features (e.g., metrics forwarding, custom labels/tolerations). | | ||
| | **Feature/platform labels** (e.g., `self-managed-azure-public`, `nodepool-autoscaling`, `control-plane-upgrade`) | Control which specs run in which `test-e2e-v2` process. The [`run-tests` orchestrator][run-tests] passes `--ginkgo.label-filter` with non-overlapping label sets so each process only runs specs relevant to its assigned cluster variant. The label-to-cluster mapping is defined by [`TestMatrix`][azure-platform] in the platform config. | |
There was a problem hiding this comment.
we have the convention of [FeatureGate:XYZ] in the test name in OCP used for feature gate promotion. Our tooling does not yet have knowledge of ginko labels, perhaps someday in the future it will though. Curious if hypershift has examples of FeatureGate in their test names or if these labels are all they're using.
|
@ironcladlou Thanks for this. |
|
/retest |
|
Hmm, the "Build Docs" should be passing now. It was fixed on main. Probably just needs a rebase. |
|
/rebase @mgencur maybe this will fix it 🤞🏻 |
|
🤖 Rebasing PR onto main: workflow run |
8dda0fb to
4ea652c
Compare
openshift-ci
Bot
added
the
needs-rebase
|
I pushed my own rebase the moment you tried to automate it |
openshift-ci
Bot
added
area/ci-tooling
4ea652c to
544622b
Compare
openshift-ci
Bot
removed
the
needs-rebase
544622b to
9b53d7a
Compare
@ironcladlou sorry 🤦🏻 ! I don't see the cancel button in the job that is running now to rebase. |
9b53d7a to
b4161cd
Compare
|
/retest |
b4161cd to
3c2ccb9
Compare
Describes the end-to-end flow of the HyperShift v2 e2e test framework, covering process boundaries, inter-process communication, Ginkgo lifecycle, and sequencing of mutually exclusive tests.
3c2ccb9 to
f015a06
Compare
|
@ironcladlou: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: devguyio, ironcladlou 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 |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
/retest |
|
/verified by docs preview |
|
@ironcladlou: This PR has been marked as verified by 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. |
openshift-ci-robot
added
the
verified
7 participants
Add a new e2e v2 test flow document which covers the system in detail at a multi-system level to facilitate architectural understanding.
Summary by CodeRabbit