[WIP] CNTRLPLANE-3487: Multi-hop upgrade tests#8752
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@mgencur: This pull request references CNTRLPLANE-3487 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 story 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgencur The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
area/platform/azure
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThe PR extends the E2E lifecycle test infrastructure to support multi-hop cluster upgrades. The Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8752 +/- ##
==========================================
+ Coverage 41.75% 41.79% +0.04%
==========================================
Files 758 759 +1
Lines 93981 94037 +56
==========================================
+ Hits 39240 39304 +64
+ Misses 51988 51983 -5
+ Partials 2753 2750 -3 see 5 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@test/e2e/v2/lifecycle/azure.go`:
- Around line 346-399: The Azure test configuration has commented out major test
groups (private, oauth-lb, autoscaling, external-oidc, upgrade-and-chaos) in the
Parallel and Sequential slices, but the corresponding cluster variants are still
being created in the ClusterSpecs initialization, causing unnecessary resource
overhead without test coverage. Either re-enable these test groups in the
Parallel and Sequential field definitions by uncommenting them, or if they are
intentionally disabled, remove the matching cluster variant definitions from the
ClusterSpecs slice to avoid creating unused clusters. Ensure alignment between
the clusters created and the tests that validate them.
In `@test/e2e/v2/tests/multi_hop_upgrade_test.go`:
- Line 38: The test suite is missing required annotations and labels according
to coding guidelines. Add the `[sig-hypershift][Jira:Hypershift][Feature:XYZ]`
annotation to the Describe block that contains the multi-hop upgrade test. For
the It case at line 38 that starts with "should upgrade control plane and
NodePool through multiple minor versions", add a Label() call with appropriate
test labels for filtering. Apply the same Label() fix to the other It case
mentioned at line 162. Ensure all Describe blocks have the required Sippy/CR
annotations and all It blocks have appropriate Label() calls as per the test
coding guidelines.
- Line 47: The Skip message references incorrect environment variable names.
Update the Skip() call to reference the actual environment variables used by
buildReleaseImageChain function, which are E2E_N3_RELEASE_IMAGE,
E2E_N2_RELEASE_IMAGE, E2E_N1_RELEASE_IMAGE, and E2E_LATEST_RELEASE_IMAGE,
instead of the currently mentioned E2E_N2 through E2E_N4. This will ensure the
error message accurately guides users to set the correct environment variables
for the multi-hop upgrade test.
- Around line 131-132: The GinkgoWriter.Printf statement on line 132 directly
accesses hc.Status.Version.History[0] without verifying that the History slice
is not empty, which will cause a panic if the slice is empty and mask the actual
upgrade failure. Guard this array access by checking that
len(hc.Status.Version.History) is greater than zero before dereferencing the
first element. If the history is empty, log a diagnostic message that includes
the namespace and name information from the hc object to help with debugging,
otherwise proceed with logging the final version from History[0].Version.
🪄 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: Enterprise
Run ID: 9d47daa1-077d-46af-b089-c99e77e69595
📒 Files selected for processing (7)
test/e2e/v2/cmd/create-guests/main.gotest/e2e/v2/cmd/destroy-guests/main.gotest/e2e/v2/cmd/dump-guests/main.gotest/e2e/v2/internal/env_vars.gotest/e2e/v2/lifecycle/azure.gotest/e2e/v2/lifecycle/platform.gotest/e2e/v2/tests/multi_hop_upgrade_test.go
| // and NodePool through multiple minor versions from the oldest available | ||
| // release image to the latest. | ||
| func MultiHopUpgradeTest(getTestCtx internal.TestContextGetter) { | ||
| It("should upgrade control plane and NodePool through multiple minor versions", func() { |
There was a problem hiding this comment.
Add required Sippy/CR annotations and an It label in this suite.
This suite is missing the required [sig-hypershift][Jira:Hypershift][Feature:XYZ] annotation on Describe, and the It case currently has no Label().
Suggested patch
-var _ = Describe("Multi-Hop Upgrade", Label("lifecycle", "multi-hop-upgrade"), func() {
+var _ = Describe("[sig-hypershift][Jira:Hypershift][Feature:MultiHopUpgrade] Multi-Hop Upgrade", Label("lifecycle", "multi-hop-upgrade"), func() {
@@
- It("should upgrade control plane and NodePool through multiple minor versions", func() {
+ It("should upgrade control plane and NodePool through multiple minor versions", Label("multi-hop-upgrade"), func() {As per coding guidelines, “Top-level Describe blocks must use [sig-hypershift][Jira:Hypershift] annotation … Include [Feature:XYZ] …” and “Use Label() on Describe and It blocks for test filtering.”
Also applies to: 162-162
🤖 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 `@test/e2e/v2/tests/multi_hop_upgrade_test.go` at line 38, The test suite is
missing required annotations and labels according to coding guidelines. Add the
`[sig-hypershift][Jira:Hypershift][Feature:XYZ]` annotation to the Describe
block that contains the multi-hop upgrade test. For the It case at line 38 that
starts with "should upgrade control plane and NodePool through multiple minor
versions", add a Label() call with appropriate test labels for filtering. Apply
the same Label() fix to the other It case mentioned at line 162. Ensure all
Describe blocks have the required Sippy/CR annotations and all It blocks have
appropriate Label() calls as per the test coding guidelines.
Source: Coding guidelines
There was a problem hiding this comment.
Finding is not valid — skipping. No v2 test file uses [sig-hypershift], [Jira:Hypershift],
or [Feature:XYZ] annotations. These are conventions from openshift-tests-private / the v1
e2e framework, not from the v2 framework. The v2 tests use Ginkgo Label() for filtering
(e.g., Label("lifecycle", "control-plane-upgrade")), which this test already has at line
187.
|
/test ci/prow/e2e-azure-v2-self-managed |
|
/test e2e-azure-v2-self-managed |
…grades Add an E2E test that upgrades both control plane and NodePool through multiple minor versions (N-3 → N-2 → N-1 → latest), validating cluster health at each hop. This satisfies the 4.22 Integration Test Matrix requirement for multi-hop upgrade coverage. - Register E2E_N3_RELEASE_IMAGE env var for oldest release image - Extend ClusterSpecs/TestMatrix interfaces with n3Image parameter - Add multi-hop-upgrade cluster variant and sequential test group - Map CI OCP_IMAGE_N* vars to E2E_N*_RELEASE_IMAGE in run-tests - New test: multi_hop_upgrade_test.go with inter-hop health checks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/v2/lifecycle/azure.go (1)
398-403: 💤 Low valueMinor: Inconsistent indentation in ExtraEnv slice.
The
ExtraEnvslice elements have an extra level of indentation compared to the surrounding code structure.🧹 Suggested formatting fix
ExtraEnv: []string{ - fmt.Sprintf("E2E_LATEST_RELEASE_IMAGE=%s", releaseImage), - fmt.Sprintf("E2E_N1_RELEASE_IMAGE=%s", n1Image), - fmt.Sprintf("E2E_N2_RELEASE_IMAGE=%s", n2Image), - fmt.Sprintf("E2E_N3_RELEASE_IMAGE=%s", n3Image), - }, + fmt.Sprintf("E2E_LATEST_RELEASE_IMAGE=%s", releaseImage), + fmt.Sprintf("E2E_N1_RELEASE_IMAGE=%s", n1Image), + fmt.Sprintf("E2E_N2_RELEASE_IMAGE=%s", n2Image), + fmt.Sprintf("E2E_N3_RELEASE_IMAGE=%s", n3Image), + },🤖 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 `@test/e2e/v2/lifecycle/azure.go` around lines 398 - 403, The ExtraEnv slice initialization has inconsistent indentation where the fmt.Sprintf elements and the closing brace have an extra level of indentation compared to the field declaration. Remove one level of indentation from each of the four fmt.Sprintf statements and the closing brace so they align properly with the standard code structure at that scope level.
🤖 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.
Nitpick comments:
In `@test/e2e/v2/lifecycle/azure.go`:
- Around line 398-403: The ExtraEnv slice initialization has inconsistent
indentation where the fmt.Sprintf elements and the closing brace have an extra
level of indentation compared to the field declaration. Remove one level of
indentation from each of the four fmt.Sprintf statements and the closing brace
so they align properly with the standard code structure at that scope level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 48fd4fcc-6c5b-4a02-b6f1-a74d39f93912
📒 Files selected for processing (3)
test/e2e/v2/cmd/run-tests/main.gotest/e2e/v2/lifecycle/azure.gotest/e2e/v2/lifecycle/platform.go
mgencur
force-pushed
the
create_multihop_upgrades
branch
from
7a0cf48 to
f697560
Compare
|
/test e2e-azure-v2-self-managed |
|
I have all the evidence needed. Here is the complete analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe gitlint check failed on commit Root CauseThe PR contains two commits. The second commit
This is a developer-side commit message formatting issue — the commit appears to be a temporary/WIP commit that was not squashed or reformatted before pushing, consistent with the PR's Recommendations
Evidence
|
|
@mgencur: all tests passed! 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. |
2 participants
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3487
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Tests
E2E_N3_RELEASE_IMAGE(along with N1/N2 and latest) to build the upgrade chain.Chores