OCPBUGS-97830: Add wait-for-etcd init container to oauth-apiserver - #8940
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-97830, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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 ignored due to path filters (5)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
|
/jira refresh |
openshift-ci
Bot
added
the
approved
openshift-ci-robot
added
jira/valid-bug
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-97830, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-97830, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/openshift-oauth-apiserver/deployment.yaml (2)
129-145: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMissing resource requests on the init container.
Every other container in this manifest declares
resources.requests(cpu/memory). Add equivalent requests forwait-for-etcdfor consistent scheduling/QoS behavior.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/assets/openshift-oauth-apiserver/deployment.yaml` around lines 129 - 145, The wait-for-etcd init container is missing resource requests while the rest of the manifest defines them, so update the initContainers entry in the openshift-oauth-apiserver deployment asset to include resources.requests for cpu and memory on wait-for-etcd. Use the existing container resource patterns in this deployment as the reference and add equivalent requests in the same init container block.Source: Path instructions
132-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShebang/interpreter mismatch.
Script declares
#!/bin/shbut is invoked viacommand: /bin/bash. Since it's run asbash -c "<script>", the shebang line is a no-op comment; harmless but confusing.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/assets/openshift-oauth-apiserver/deployment.yaml` around lines 132 - 136, The startup script in the deployment manifest has a shebang/interpreter mismatch: the inline script under the container command is executed by /bin/bash, so the leading #!/bin/sh line is just a misleading comment. Update the script in the openshift-oauth-apiserver deployment asset to use a consistent interpreter choice by either removing the shebang from the inline block or aligning the command to sh, and keep the command/script pair under the container spec consistent with the existing nslookup loop.control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.go (1)
29-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the duplicate
Unmanagedcondition checks.The same
cpContext.HCP.Spec.Etcd.ManagementType == hyperv1.Unmanagedcondition is evaluated twice in adjacent blocks (lines 30 and 36). Combine them into a single branch for clarity.♻️ Proposed refactor
var err error etcdHostname := "etcd-client" if cpContext.HCP.Spec.Etcd.ManagementType == hyperv1.Unmanaged { etcdHostname, err = netutil.HostFromURL(cpContext.HCP.Spec.Etcd.Unmanaged.Endpoint) if err != nil { return err } + podspec.RemoveInitContainer("wait-for-etcd", &deployment.Spec.Template.Spec) } - if cpContext.HCP.Spec.Etcd.ManagementType == hyperv1.Unmanaged { - podspec.RemoveInitContainer("wait-for-etcd", &deployment.Spec.Template.Spec) - }🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.go` around lines 29 - 38, The OAuth apiserver deployment logic evaluates cpContext.HCP.Spec.Etcd.ManagementType == hyperv1.Unmanaged twice in adjacent blocks, which should be merged for clarity. Update the deployment.go logic around the etcdHostname handling so the unmanaged branch both derives the host with netutil.HostFromURL and removes the "wait-for-etcd" init container in a single conditional, using the same cpContext.HCP.Spec.Etcd.ManagementType check and the related deployment.Spec.Template.Spec/podspec.RemoveInitContainer path.Source: Coding guidelines
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/openshift-oauth-apiserver/deployment.yaml`:
- Around line 129-145: The wait-for-etcd init container is missing resource
requests while the rest of the manifest defines them, so update the
initContainers entry in the openshift-oauth-apiserver deployment asset to
include resources.requests for cpu and memory on wait-for-etcd. Use the existing
container resource patterns in this deployment as the reference and add
equivalent requests in the same init container block.
- Around line 132-136: The startup script in the deployment manifest has a
shebang/interpreter mismatch: the inline script under the container command is
executed by /bin/bash, so the leading #!/bin/sh line is just a misleading
comment. Update the script in the openshift-oauth-apiserver deployment asset to
use a consistent interpreter choice by either removing the shebang from the
inline block or aligning the command to sh, and keep the command/script pair
under the container spec consistent with the existing nslookup loop.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.go`:
- Around line 29-38: The OAuth apiserver deployment logic evaluates
cpContext.HCP.Spec.Etcd.ManagementType == hyperv1.Unmanaged twice in adjacent
blocks, which should be merged for clarity. Update the deployment.go logic
around the etcdHostname handling so the unmanaged branch both derives the host
with netutil.HostFromURL and removes the "wait-for-etcd" init container in a
single conditional, using the same cpContext.HCP.Spec.Etcd.ManagementType check
and the related deployment.Spec.Template.Spec/podspec.RemoveInitContainer path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cad16ca2-4cc8-478e-a9bd-e3eabef1c676
⛔ Files ignored due to path filters (5)
control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/GCP/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/openshift-oauth-apiserver/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8940 +/- ##
==========================================
+ Coverage 43.37% 43.53% +0.15%
==========================================
Files 771 771
Lines 95718 95799 +81
==========================================
+ Hits 41520 41708 +188
+ Misses 51313 51192 -121
- Partials 2885 2899 +14
... and 6 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:
|
| - -c | ||
| - | | ||
| #!/bin/sh | ||
| while ! nslookup etcd-client.$(POD_NAMESPACE).svc; do sleep 1; done |
There was a problem hiding this comment.
Is the goal to check for DNS resolution or actual connectivity to the endpoint?
There was a problem hiding this comment.
DNS resolution, not connectivity. The failure mode is that oauth-apiserver tries to connect to etcd-client before the Service has a backing Endpoints object (DNS returns NXDOMAIN), then exits with "context deadline exceeded" after 20s. Once DNS resolves, kube-proxy rules handle L4 connectivity. This mirrors the existing kube-apiserver wait-for-etcd init container which uses the same nslookup check.
Also merged the duplicate Unmanaged condition checks in deployment.go per a coderabbit nit.
AI-assisted response via Claude Code
…iserver The oauth-apiserver pod frequently restarts once during HCP startup because it connects to etcd before the etcd-client service is routable, exiting with "context deadline exceeded" after 20s. This is the primary flake in the AKS e2e job (~50% failure rate). Mirror the kube-apiserver pattern: add a wait-for-etcd init container that DNS-polls etcd-client.$NAMESPACE.svc until it resolves. For unmanaged etcd, strip the init container at reconciliation time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Note on deployment_test.go:68 — The existing test case "When etcd is unmanaged, it should configure custom etcd endpoint" exercises the unmanaged etcd path but doesn't verify the new RemoveInitContainer behavior. Could you add an assertion here? Something like:
initContainer := podspec.FindContainer("wait-for-etcd", deployment.Spec.Template.Spec.InitContainers)
g.Expect(initContainer).To(BeNil(), "wait-for-etcd init container should be removed for unmanaged etcd")(I know KAS doesn't test this either, but since you're already touching this code it's a good opportunity to close the gap.)
| if err != nil { | ||
| return err | ||
| } | ||
| podspec.RemoveInitContainer("wait-for-etcd", &deployment.Spec.Template.Spec) |
There was a problem hiding this comment.
The placement works, but KAS keeps init container removal in its own self-documenting block with an explanatory comment (kas/deployment.go:105-110). Nesting it inside the hostname-resolution block makes it look like a side-effect of URL parsing rather than an intentional design decision.
Consider:
// With managed etcd, we should wait for the known etcd client service name to
// at least resolve before starting up to avoid futile connection attempts and
// pod crashing. For unmanaged, make no assumptions.
if cpContext.HCP.Spec.Etcd.ManagementType == hyperv1.Unmanaged {
podspec.RemoveInitContainer("wait-for-etcd", &deployment.Spec.Template.Spec)
}Nit, not blocking — just consistency with KAS.
|
Dropped some comments. Thanks! /lgtm |
|
Scheduling tests matching the |
|
The PR does NOT touch any cluster creation code. Now I have enough evidence for the report. Let me compile the final analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe root cause is a transient CI infrastructure issue — the build01.ci.openshift.org API server was unable to handle requests during the window when the The Supporting evidence that this is an infrastructure issue, not a code issue:
Recommendations
Evidence
|
|
/test e2e-azure-v2-self-managed |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
/verified by e2e The name: wait-for-etcd
ready: true
restartCount: 0
state:
terminated:
containerID: cri-o://16044e11be598d5fe419f74bc393860b4f1da4652d258c1237dd2c3b17d7d8b5
exitCode: 0
finishedAt: "2026-07-13T17:38:01Z"
reason: Completed
startedAt: "2026-07-13T17:38:01Z"The init container resolved etcd DNS and completed successfully with zero restarts before the oauth-apiserver main container started. |
openshift-ci-robot
added
the
verified
|
@bryan-cox: 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. |
|
@bryan-cox: 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. |
|
@bryan-cox: Jira Issue Verification Checks: Jira Issue OCPBUGS-97830 Jira Issue OCPBUGS-97830 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
ModernTLS fixtures from openshift#8871 landed stale due to Tide merge skew. GHA tested against a base that predated openshift#8772, openshift#8940, openshift#8971, openshift#8705 which changed oauth masterURL, wait-for-etcd init container, router ordering, and etcd job label regex respectively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
ModernTLS fixtures from openshift#8871 landed stale due to Tide merge skew. GHA tested against a base that predated openshift#8772, openshift#8940, openshift#8971, openshift#8705 which changed oauth masterURL, wait-for-etcd init container, router ordering, and etcd job label regex respectively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
ModernTLS fixtures from openshift#8871 landed stale due to Tide merge skew. GHA tested against a base that predated openshift#8772, openshift#8940, openshift#8971, openshift#8705 which changed oauth masterURL, wait-for-etcd init container, router ordering, and etcd job label regex respectively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
4 participants
Summary
wait-for-etcdinit container to the oauth-apiserver deployment, mirroring the existing kube-apiserver patternetcd-client.$NAMESPACE.svcuntil it resolves before the main container startsProblem
The
openshift-oauth-apiserverpod frequently restarts once during HCP startup because it attempts to connect to etcd before theetcd-clientservice is routable. The container exits with"error building REST storage: context deadline exceeded"after a 20-second connection timeout, gets restarted, and succeeds on the second attempt.This is the primary flake in the AKS e2e CI job (
pull-ci-openshift-hypershift-main-e2e-aks), causingEnsureNoCrashingPodsto fail when it seesrestartCount=1. The job has a ~50% failure rate due to this issue.Root Cause
The
kube-apiserverhas await-for-etcdinit container that prevents this exact race condition. Theopenshift-oauth-apiserverconnects to etcd the same way but lacked this init container. TheWithDependenciescontroller chain only gates Deployment creation at the controller level, not pod-level startup — so the etcd Service can exist but not yet be routable when the oauth-apiserver container starts.Log evidence
PR #8849 — pod
openshift-oauth-apiserver-66b8fdb877-vr549(artifacts):Previous container logs (ran exactly 20s before crashing):
PR #8924 — pod
openshift-oauth-apiserver-55785bf8cc-z2sxz(artifacts):Previous container logs (ran 22s before crashing):
Both crashes follow the same sequence: oauth-apiserver starts → gRPC connections to
etcd-client:2379fail (DNS lookup canceled or TCP dial timeout) → 20s etcd client context expires → fatal"error building REST storage: context deadline exceeded"→ exit code 1 → restart succeeds. Both PRs (#8849 is a dependabot update, #8924 is an unrelated nil-map guard fix) are completely unrelated to this failure.Test plan
make lint-fixpasses (0 issues)make verifypasses (pre-existingverify-crd-schemafailure unrelated to this change)EnsureNoCrashingPodsfor oauth-apiserver restarts/jira:OCPBUGS-97830
🤖 Generated with Claude Code
Summary by CodeRabbit