NE-2218: Add new HAProxy 2.8 and 3.2 image references#1499
Conversation
|
@jcmoraisjr: This pull request references NE-2218 which is a valid jira issue. 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
jira/valid-reference
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change adds a separate HAProxy image path from ingress-operator startup through deployment manifests into router deployment generation. The start command now accepts a HAProxy previous-image flag and stores it in operator configuration. The deployment manifests add matching flag and environment wiring, and the image references add a HAProxy 2.8 tag. The router deployment now uses the HAProxy-specific image for the init container. Related issues: Not specified. 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/ingress-operator/start.go (1)
137-153: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWire
ImageHAProxy28into a consumer or drop the required flag
cmd/ingress-operator/start.go:137-153
ImageHAProxy28is only logged here; nothing in this change uses it. Thread it through the operator config/controller that needs it, or remove the required flag until there is a consumer.🤖 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 `@cmd/ingress-operator/start.go` around lines 137 - 153, `ImageHAProxy28` is currently only being logged in `start.go` and never consumed, so either wire it into the code path that needs the HAProxy 2.8 image or stop requiring it for startup. Update `operatorConfig`/the relevant controller or config type to carry `opts.ImageHAProxy28` through to the consumer, and if no consumer exists yet, remove the required flag and related plumbing from `start.go` and `operatorconfig.Config`.
🤖 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 `@cmd/ingress-operator/start.go`:
- Around line 86-87: The local run helper is still using the old image flag
name, so update the ingress-operator startup path to use the renamed flag. In
the code that defines CLI flags in start.go, keep or map the new image-router
option consistently with the rest of the startup flow, and update
hack/run-local.sh to pass --image-router instead of --image so the command
matches the current flag names. If you need backward compatibility, add an alias
in the same flag setup around IngressControllerImage rather than leaving the
script on the deprecated name.
---
Outside diff comments:
In `@cmd/ingress-operator/start.go`:
- Around line 137-153: `ImageHAProxy28` is currently only being logged in
`start.go` and never consumed, so either wire it into the code path that needs
the HAProxy 2.8 image or stop requiring it for startup. Update
`operatorConfig`/the relevant controller or config type to carry
`opts.ImageHAProxy28` through to the consumer, and if no consumer exists yet,
remove the required flag and related plumbing from `start.go` and
`operatorconfig.Config`.
🪄 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: b3e3894f-1843-438f-9d94-a99292b449d4
📒 Files selected for processing (4)
cmd/ingress-operator/start.gomanifests/02-deployment-ibm-cloud-managed.yamlmanifests/02-deployment.yamlmanifests/image-references
jcmoraisjr
force-pushed
the
NE-2218-link-haproxy28
branch
4 times, most recently
from
95b68ae to
41a2cae
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/operator/controller/ingress/deployment.go (1)
302-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new HAProxy image wiring.
deployment_test.gowasn't included in this diff, and this change altersInitContainers[1].Imagebehavior (previously derived fromconfig.IngressControllerImage, nowconfig.HAProxyImage). A test assertingdesiredRouterDeploymentsets the HAProxy init container image fromconfig.HAProxyImage(and that the router container / init container 0 still useconfig.IngressControllerImage) would guard against regressions.Want me to draft this test case?
Also applies to: 833-835
🤖 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 `@pkg/operator/controller/ingress/deployment.go` at line 302, Add or update coverage in desiredRouterDeployment to assert the new HAProxy image wiring: verify InitContainers[1].Image comes from config.HAProxyImage, while the router container and InitContainers[0] still use config.IngressControllerImage. Put the assertions in deployment_test.go against desiredRouterDeployment so this behavior is locked in and regressions in the image source are caught.cmd/ingress-operator/start.go (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor naming inconsistency across layers.
StartOptions.ImageHAProxy28maps tooperatorconfig.Config.HAProxyImage, then toingresscontroller.Config.HAProxyImage. Consider aligning the field name (e.g.,HAProxyImage) with the downstream config for consistency, similar to howIngressControllerImagematches its counterpart throughout the chain.Also applies to: 146-146
🤖 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 `@cmd/ingress-operator/start.go` around lines 54 - 55, The HAProxy image field name is inconsistent across the config flow: StartOptions.ImageHAProxy28 maps to operatorconfig.Config.HAProxyImage and then to ingresscontroller.Config.HAProxyImage. Rename the StartOptions field to HAProxyImage and update the related mappings/usages in StartOptions, Config conversion, and any references in the start path so the naming matches the downstream configs, similar to IngressControllerImage.
🤖 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 `@cmd/ingress-operator/start.go`:
- Around line 54-55: The HAProxy image field name is inconsistent across the
config flow: StartOptions.ImageHAProxy28 maps to
operatorconfig.Config.HAProxyImage and then to
ingresscontroller.Config.HAProxyImage. Rename the StartOptions field to
HAProxyImage and update the related mappings/usages in StartOptions, Config
conversion, and any references in the start path so the naming matches the
downstream configs, similar to IngressControllerImage.
In `@pkg/operator/controller/ingress/deployment.go`:
- Line 302: Add or update coverage in desiredRouterDeployment to assert the new
HAProxy image wiring: verify InitContainers[1].Image comes from
config.HAProxyImage, while the router container and InitContainers[0] still use
config.IngressControllerImage. Put the assertions in deployment_test.go against
desiredRouterDeployment so this behavior is locked in and regressions in the
image source are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0fbf195b-f33d-49a4-9b7f-d2650b0e3e62
📒 Files selected for processing (8)
cmd/ingress-operator/start.gomanifests/02-deployment-ibm-cloud-managed.yamlmanifests/02-deployment.yamlmanifests/image-referencespkg/operator/config/config.gopkg/operator/controller/ingress/controller.gopkg/operator/controller/ingress/deployment.gopkg/operator/operator.go
🚧 Files skipped from review as they are similar to previous changes (1)
- manifests/image-references
|
/retest-required |
|
/test e2e-gcp-operator |
|
/test e2e-hypershift |
|
/test e2e-aws-operator-techpreview |
|
/test e2e-hypershift |
jcmoraisjr
force-pushed
the
NE-2218-link-haproxy28
branch
from
41a2cae to
7f15a6b
Compare
|
/payload-job periodic-ci-openshift-release-main-okd-scos-5.0-e2e-aws-ovn |
|
@gcs278: 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/b61d77e0-795d-11f1-84b9-f968a2a33dc6-0 |
|
@jcmoraisjr I'm like 90% sure if we override ci/prow/okd-scos-images, then we will break OKD 5.0 builds, but I triggered a OKD 5.0 payload job to test that theory above. |
|
/test e2e-aws-ovn-serial-2of2 |
|
It seems infra related: Let's try it again. /test e2e-hypershift |
|
/override ci/prow/okd-scos-images |
|
@Prashanth684: Overrode contexts on behalf of Prashanth684: ci/prow/okd-scos-images 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 kubernetes-sigs/prow repository. |
|
It seems Hypershift tests are back after a cleanup. Let's give it another chance. /test e2e-hypershift |
|
Waiting for OKD for now. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/assign @gcs278 |
|
/test ci/prow/okd-scos-images |
|
/test okd-scos-images |
Add minimum implementation linking the new HAProxy 2.8 image into image-references. https://redhat.atlassian.net/browse/NE-2218
jcmoraisjr
force-pushed
the
NE-2218-link-haproxy28
branch
from
7f15a6b to
b36d9f9
Compare
|
@jcmoraisjr generally LGTM - straightforward to include the images in the payload. I don't see any issues - except don't forget to update the title to also mention 3.2. This PR doesn't use the images directly, but adds them to the nightly payload so we can build clusterbots with them for testing. I don't think this PR is strictly necessary (or I could still be misunderstanding), but I'm okay with merging to unblock progress. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278 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
jcmoraisjr
changed the title
|
OKD failed due to the missing of /retest |
|
Checking availability of /test okd-scos-images |
|
Is this a thing? |
|
/test ? |
|
|
|
/test hypershift-e2e-aks |
|
/test e2e-aws-ovn-single-node-rhcos10-techpreview |
|
/test e2e-aws-ovn-upgrade-haproxy28-techpreview |
|
just a heads up, the haproxy28 tests won't really do anything useful here as this PR isn't vendoring the new API in the IngressController CRD (which the haproxy28 tests require). It's still a valid upgrade test though for the default haproxy. |
|
OKD update merged, let's give this one another attempt: /test okd-scos-images |
|
We're green on OKD! /hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
This PR is critical for implementing a release-blocking feature. /label acknowledge-critical-fixes-only
|
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
/test images |
|
/hold some discussion on whether openshift-eng/ocp-build-data#11725 needs to be merged first before this - feel free to remove when you get an answer @jcmoraisjr |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@jcmoraisjr: 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. |
6 participants
Add minimum implementation linking the new HAProxy 2.8 and 3.2 images into image-references.
https://redhat.atlassian.net/browse/NE-2218