NE-2839: Add HAProxy version upgrade tests - #31495
Conversation
The IngressControllerMultipleHAProxyVersions feature allows selecting HAProxy versions per IngressController. During y-stream upgrades (e.g. 4.22 to 5.0), the default HAProxy version may change (2.8 to 3.2). Two upgrade scenarios need validation: - Pinned version: an IngressController with an explicitly set HAProxyVersion must retain that version across any upgrade, regardless of default version changes. - Unset version: an IngressController with no HAProxyVersion set must follow the new release default after upgrade. Both tests create a custom IngressController before upgrade, wait for the upgrade to complete, then verify the expected HAProxy version via both the IngressController status (EffectiveHAProxyVersion) and the HAProxy runtime socket. Also refactors multi-haproxy.go to extract shared helpers (apiHasHAProxyVersionField, deleteAll) used by both the day-2 tests and the new upgrade tests. https://redhat.atlassian.net/browse/NE-2839 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jcmoraisjr: This pull request references NE-2839 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. |
WalkthroughThe router upgrade suite adds pinned and unpinned HAProxy version tests. It detects API support, validates status and runtime versions after upgrade, derives the default version, and uses shared CRD detection and cleanup helpers. ChangesHAProxy version upgrade testing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HAProxyVersionUpgradeTest
participant UpgradeFramework
participant IngressController
participant HAProxyRuntimeAPI
HAProxyVersionUpgradeTest->>UpgradeFramework: wait for upgrade completion
HAProxyVersionUpgradeTest->>IngressController: read effective HAProxy version
HAProxyVersionUpgradeTest->>HAProxyRuntimeAPI: verify runtime version
HAProxyVersionUpgradeTest->>IngressController: delete test resource
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jcmoraisjr 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/extended/router/haproxyversion_upgrade.go`:
- Around line 143-151: The Teardown cleanup in
test/extended/router/haproxyversion_upgrade.go:143-151 and the corresponding
cleanup in test/extended/router/multi-haproxy.go:55-58 must use an uncancelable
base context with a bounded timeout when calling controllers.deleteAll, rather
than the canceled test context; preserve the existing resource checks and error
logging.
🪄 Autofix
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: 36092e16-5729-4422-815b-dad8eebbcd3a
📒 Files selected for processing (3)
test/e2e/upgrade/upgrade.gotest/extended/router/haproxyversion_upgrade.gotest/extended/router/multi-haproxy.go
| func (h *HAProxyVersionUpgradeTest) Teardown(ctx context.Context, f *framework.Framework) { | ||
| if h.operatorClient == nil { | ||
| framework.Logf("Skipping cleanup because setup did not initialize test resources") | ||
| return | ||
| } | ||
| if err := h.controllers.deleteAll(ctx, h.operatorClient); err != nil { | ||
| framework.Logf("error deleting IngressController resource: %s", err.Error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that the declared toolchain supports context.WithoutCancel.
rg -n '^(go|toolchain) ' go.mod
# Inspect both cleanup call sites.
rg -n -C 5 'deleteAll\(' test/extended/router/multi-haproxy.go test/extended/router/haproxyversion_upgrade.goRepository: openshift/origin
Length of output: 2816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect imports and surrounding cleanup context usage in both test files.
for f in test/extended/router/multi-haproxy.go test/extended/router/haproxyversion_upgrade.go; do
echo "===== $f ====="
sed -n '1,90p' "$f"
echo
done
# Search for IngressController deletion helpers and context use around cleanup.
rg -n -C 4 'deleteAll|i\.Delete\(|OperatorV1\(\)\.IngressControllers|context\.With|context\.Background|context\.Timeout|context\.WithoutCancel' test/extended/router test -g '*.go' | head -n 200Repository: openshift/origin
Length of output: 21383
Use bounded cleanup contexts.
A canceled test context can make deleteAll fail before it sends deletion requests. Use an uncancelable cleanup base context with a timeout before calling deleteAll in both cleanup paths.
📍 Affects 2 files
test/extended/router/haproxyversion_upgrade.go#L143-L151(this comment)test/extended/router/multi-haproxy.go#L55-L58
🤖 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/extended/router/haproxyversion_upgrade.go` around lines 143 - 151, The
Teardown cleanup in test/extended/router/haproxyversion_upgrade.go:143-151 and
the corresponding cleanup in test/extended/router/multi-haproxy.go:55-58 must
use an uncancelable base context with a bounded timeout when calling
controllers.deleteAll, rather than the canceled test context; preserve the
existing resource checks and error logging.
Sources: Path instructions, Learnings
|
Scheduling required tests: Scheduling tests matching the |
|
/payload-job-with-prs ? |
|
@jcmoraisjr: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
|
/payload-job-with-prs |
|
@jcmoraisjr: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-aws-ovn-upgrade openshift/api#2971 openshift/cluster-ingress-operator#1545 |
|
@jcmoraisjr: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c80a2da0-92a7-11f1-9749-1b41a869f83d-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-upgrade openshift/api#2971 openshift/cluster-ingress-operator#1545 |
|
@jcmoraisjr: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/03588550-92a8-11f1-8016-b7673b47292e-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-upgrade openshift/api#2971 openshift/cluster-ingress-operator#1545 |
|
@jcmoraisjr: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/204e9d20-92a8-11f1-96af-9e21d2a7ed7f-0 |
|
@jcmoraisjr: 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 60d4a8f
|
|
/assign @gcs278 |
|
/assign @rhamini3 |
gcs278
left a comment
gcs278
left a comment
There was a problem hiding this comment.
Very nice upgrade tests! just a couple of thoughts on this - nothing super critical.
| err := wait.PollUntilContextTimeout(ctx, time.Second, rollingOutTimeout, true, func(ctx context.Context) (ready bool, err error) { | ||
| ic, err := h.operatorClient.OperatorV1().IngressControllers(h.ic.Namespace).Get(ctx, h.ic.Name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| framework.Logf("error getting IngressController resource: %s", err.Error()) | ||
| return false, nil | ||
| } | ||
| if ic.Status.EffectiveHAProxyVersion != expectedVersion { | ||
| framework.Logf("HAProxy version from IngressResource status %q does not match expected value %q", ic.Status.EffectiveHAProxyVersion, expectedVersion) | ||
| return false, nil | ||
| } | ||
| return true, nil | ||
| }) |
There was a problem hiding this comment.
nit we basically have repeated logic of this here: https://github.com/openshift/origin/blob/main/test/extended/router/multi-haproxy.go#L149
A helper to keep our code line count down would be nice.
| err := operatorClient.OperatorV1().IngressControllers(ic.controller.Namespace).Delete(ctx, ic.controller.Name, *metav1.NewDeleteOptions(1)) | ||
| errs = append(errs, client.IgnoreNotFound(err)) | ||
| } | ||
| return errors.Join(errs...) |
There was a problem hiding this comment.
this just fires a delete and moves on. I think it would be more thorough to poll that each ingresscontroller is cleaned up before moving on.
For example, what if we introduced a deletion deadlock while using spec.haproxyVersion? A check would be catch that.
| customIngress = func(ic *operatorv1.IngressController) { | ||
| ic.Spec.HAProxyVersion = versions.defaultVersion | ||
| } | ||
| h.pinnedVersion = versions.defaultVersion |
There was a problem hiding this comment.
ah this tripped me up. I wanted to ask "why not pin the non-default version"? but then I realized, we plan to always drop the non-default version in y-stream updates.
However, this test runs on z-stream as well. Right now, you only do binary pinned. Adding a test case for non-default pinning for z-streams (which gets skipped on y-streams because it's the --depracted-haproxy-version) feels also valuable too.
You'd need more than just a binary parameter for the test, something like:
&router.HAProxyVersionUpgradeTest{Mode: router.HAProxyUpgradeModeUnset},
&router.HAProxyVersionUpgradeTest{Mode: router.HAProxyUpgradeModeDefault},
&router.HAProxyVersionUpgradeTest{Mode: router.HAProxyUpgradeModeNonDefault},
And you'd need to discover the haproxy available versions + deprecated. I think it might be worth it, what do you think?
Here's a claude generated chart to help visualize (we have 3 types of y-stream upgrades: drop version , add version, same versions):
| Upgrade type | Example | Unset | Pinned default | Pinned non-default |
|---|---|---|---|---|
| y-stream (drop 2.8) | 5.0→5.1 | ✓ | ✓ | skip (2.8 deprecated) |
| y-stream (add 3.4) | 5.1→5.2 | ✓ (default floats 3.2→3.4) | ✓ (pin 3.2 holds) | skip (single version on 5.1) |
| y-stream (drop 3.2) | 5.2→5.3 | ✓ | ✓ | skip (3.2 deprecated) |
| y-stream (same) | 5.3→5.4 | ✓ | ✓ | skip (single version) |
| z-stream | 5.0.z→5.0.z | ✓ | ✓ | ✓ (pin 2.8 holds) |
| Namespace: ic.Namespace, | ||
| Name: ic.Name, | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we get a little more verbose on the logging? Depends on my comment above about discovering the available versions.
| framework.Logf("HAProxy version config: default=%s, pinned=%v", versions.defaultVersion, h.Pinned) | |
| framework.Logf("Created IngressController %s with spec.haproxyVersion=%q", h.ic.Name, h.pinnedVersion) |
| o.Expect(err).NotTo(o.HaveOccurred(), "error getting HAProxy versions") | ||
| expectedVersion = versions.defaultVersion | ||
| } | ||
|
|
There was a problem hiding this comment.
More logging suggestions
| framework.Logf("Post-upgrade HAProxy version check: expected=%s, pinned=%v", expectedVersion, h.Pinned) | |
| framework.Logf("HAProxy version config: available=%v, default=%s", versions.availableVersions, versions.defaultVersion) |
| alternateHAProxyVersion = operatorv1.HAProxyVersion32 | ||
| } else { | ||
| alternateHAProxyVersion = operatorv1.HAProxyVersion28 | ||
| } |
There was a problem hiding this comment.
nit If you (and only if) do update getHAProxyVersionParams to get the available haproxy versions, can you update this test to not hardcode 28 or 32 anywhere?
It's a tradeoff - hardcoding sets very explicit expectations, but also is tedious to maintain and will break when we asynchronous bump CIO to support/drop a new haproxy version. I'd rather just have it dynamically discover the available versions and build the tests from there.
| func (h *HAProxyVersionUpgradeTest) Test(ctx context.Context, f *framework.Framework, done <-chan struct{}, upgrade upgrades.UpgradeType) { | ||
| g.By("Waiting for upgrade to complete") | ||
| <-done | ||
|
|
There was a problem hiding this comment.
You should verify that the ingresscontroller is still avaiable and not gone degraded. Even though the version may return a value we expect, it doesn't guarantee that something else didn't break:
Somethign
| ingressControllerReady := []operatorv1.OperatorCondition{ | |
| {Type: operatorv1.IngressControllerAvailableConditionType, Status: operatorv1.ConditionTrue}, | |
| {Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionFalse}, | |
| {Type: operatorv1.DNSManagedIngressConditionType, Status: operatorv1.ConditionFalse}, | |
| {Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionFalse}, | |
| } | |
| // wait for the controller to be available | |
| err = shard.WaitForIngressControllerCondition(oc, readyTimeout, controller, ingressControllerReady...) | |
| if err != nil { | |
| return nil, err | |
| } |
But consider making this a helper - I just pulled this from multi-haproxy.go's createIngressController function.
Side note: I missed suggesting this for the existing tests in multi-haproxy.go - the tests that change the haproxyVersion should confirm that all of the status conditions are as expected after changing.
4 participants
The IngressControllerMultipleHAProxyVersions feature allows selecting HAProxy versions per IngressController. During y-stream upgrades (e.g. 4.22 to 5.0), the default HAProxy version may change (2.8 to 3.2). Two upgrade scenarios need validation:
Both tests create a custom IngressController before upgrade, wait for the upgrade to complete, then verify the expected HAProxy version via both the IngressController status (EffectiveHAProxyVersion) and the HAProxy runtime socket.
Also refactors multi-haproxy.go to extract shared helpers (apiHasHAProxyVersionField, deleteAll) used by both the day-2 tests and the new upgrade tests.
https://redhat.atlassian.net/browse/NE-2839
Summary by CodeRabbit