Uh oh!
There was an error while loading. Please reload this page.
🐛 Fix catalogd ha readiness - #2674
Merged
openshift-merge-bot[bot] merged 2 commits intoApr 27, 2026
Merged
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes catalogd HA behavior and adds an experimental e2e scenario to exercise multi-replica failover, aiming to prevent catalog serving stalls/unreadiness during upgrades.
Changes:
- Introduces HA-gated e2e steps + a new
@CatalogdHAfeature scenario that deletes the catalogd leader pod and waits for leader re-election. - Updates experimental manifests/helm values to run
catalogdandoperator-controllerwith 2 replicas. - Reworks catalogd HTTP server integration to use a custom runnable + readiness check.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/catalogd/serverutil/serverutil.go | Replaces controller-runtime manager.Server usage with a custom runnable and a readyz check. |
test/e2e/steps/steps.go | Registers new Godog steps for HA leader failover scenario. |
test/e2e/steps/hooks.go | Adds a CatalogdHA feature gate and enables it based on node count. |
test/e2e/steps/ha_steps.go | Implements steps to force-delete the leader pod and detect a newly elected leader. |
test/e2e/features/ha.feature | Adds @CatalogdHA scenario validating catalog continues serving after leader disruption. |
manifests/experimental.yaml | Sets catalogd and operator-controller replicas to 2 for experimental installs. |
manifests/experimental-e2e.yaml | Sets catalogd and operator-controller replicas to 2 for experimental-e2e installs. |
helm/experimental.yaml | Sets Helm experimental values to deploy both components with 2 replicas. |
Makefile | Increases default e2e timeout and bumps experimental-e2e timeout to 25m. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
tmshortforce-pushed
the
fix-catalogd-ha-readiness
branch
from
April 24, 2026 20:03
8c2f948 to
99db2dcCompareCodecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #2674 +/- ##
==========================================
- Coverage 68.03% 67.96% -0.07%
==========================================
Files 144 144 Lines 10573 10595 +22 ==========================================
+ Hits 7193 7201 +8 - Misses 2863 2871 +8 - Partials 517 523 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dtfranz
commented
Apr 27, 2026
Contributor
/lgtm |
camilamacedo86
approved these changes
Apr 27, 2026
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, rashmigottipati 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 |
camilamacedo86
commented
Apr 27, 2026
Contributor
@tmshort we will need to rebase |
ContributorAuthor
/hold |
The catalog HTTP server has OnlyServeWhenLeader: true, so only the leader pod should serve catalog content. Previously, net.Listen was called eagerly at startup for all pods: the listen socket was bound on non-leaders even though http.Serve was never called, causing TCP connections to queue without being served. With replicas > 1 this made ~50% of catalog content requests fail silently. Replace manager.Server with a custom Runnable (catalogServerRunnable) in serverutil that: - Binds the catalog port lazily inside Start(), which is only called on the leader by controller-runtime's leader election machinery. - Closes a ready channel once the listener is established, and registers a channel-select readiness check via AddReadyzCheck so non-leader pods fail the /readyz probe and are excluded from Service endpoints. This keeps cmd/catalogd/main.go health/readiness setup identical to cmd/operator-controller/main.go (healthz.Ping for both liveness and readiness); the catalog-server readiness check is an implementation detail of serverutil.AddCatalogServerToManager. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
tmshortforce-pushed
the
fix-catalogd-ha-readiness
branch
from
April 27, 2026 18:12
99db2dc to
3bebd05CompareNew changes are detected. LGTM label has been removed. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+68
| // catalogServerRunnable is a leader-only Runnable that binds the catalog HTTP port | ||
| // lazily inside Start(), so non-leader pods never hold the listen socket. |
There was a problem hiding this comment.
The struct comment says catalogServerRunnable is "leader-only" and that non-leader pods never hold the listen socket, but the implementation sets NeedLeaderElection() to false and listens/serves on every replica. Please update the comments to reflect the actual behavior (or adjust the behavior to match the comment) to avoid misleading future maintainers.
Suggested change
| // catalogServerRunnable is a leader-only Runnable that binds the catalog HTTP port | |
| // lazily inside Start(), so non-leader pods never hold the listen socket. | |
| // catalogServerRunnable is a Runnable that starts on every pod because | |
| // NeedLeaderElection returns false. It binds the catalog HTTP port inside | |
| // Start(), so every replica holds the listen socket and may serve requests | |
| // before it becomes leader. |
| }() | ||
| if err := r.server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) { | ||
| return err |
There was a problem hiding this comment.
If http.Server.Serve fails, the returned error isn't wrapped, which makes it harder to diagnose which server failed in aggregated manager errors/logs. Consider wrapping the error with context (e.g., include the catalog listen address) before returning.
Suggested change
| returnerr | |
| returnfmt.Errorf("catalog server serve failed on %q: %w", r.cfg.CatalogAddr, err) |
The experimental e2e suite uses a 2-node kind cluster, making it a natural fit to validate HA behaviour. Set replicas=2 for both components in helm/experimental.yaml so the experimental and experimental-e2e manifests exercise the multi-replica path end-to-end. This is safe for operator-controller (no leader-only HTTP servers) and for catalogd now that the catalog server starts on all pods via NeedLeaderElection=false, preventing the rolling-update deadlock that would arise if the server were leader-only. Also adds a @CatalogdHA experimental e2e scenario that force-deletes the catalogd leader pod and verifies that a new leader is elected and the catalog resumes serving. The scenario is gated on a 2-node cluster (detected in BeforeSuite and reflected in the featureGates map), so it is automatically skipped in the standard 1-node e2e suite. The experimental e2e timeout is bumped from 20m to 25m to accommodate leader re-election time (~163s worst case). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
tmshortforce-pushed
the
fix-catalogd-ha-readiness
branch
from
April 27, 2026 18:43
3bebd05 to
c74adffCompareopenshift-merge-botBot
merged commit Apr 27, 2026
e12d90b
into
operator-framework:main
26 checks passed
Uh oh!
There was an error while loading. Please reload this page.
4 tasks
tmshort added a commit
to tmshort/operator-controller
that referenced
this pull request
Apr 28, 2026
Two fixes on top of the catalogd HA work (operator-framework#2674): 1. serverutil: wrap http.Server.Serve error with the catalog listen address so the failure message is self-diagnosing in aggregated manager logs. 2. e2e: fix CatalogdHA gate to override node-count check with actual catalogd replica count. The node-count heuristic fires on any multi-node cluster (e.g. OpenShift standard e2e on AWS), causing the @CatalogdHA scenario to run even when catalogd has only 1 replica, which makes the step "catalogd is ready to reconcile resources" fail with exit status 1. After olmNamespace is populated, query the catalogd deployment's spec.replicas and set the gate unconditionally to (replicas >= 2), overriding the earlier node-count result. For upgrade scenarios the detectOLMDeployment early-return keeps the node-count value intact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
tmshort added a commit
to tmshort/operator-controller
that referenced
this pull request
Apr 28, 2026
Two fixes on top of the catalogd HA work (operator-framework#2674): 1. serverutil: wrap http.Server.Serve error with the catalog listen address so the failure message is self-diagnosing in aggregated manager logs. 2. e2e: fix CatalogdHA gate to override node-count check with actual catalogd replica count. The node-count heuristic fires on any multi-node cluster (e.g. OpenShift standard e2e on AWS), causing the @CatalogdHA scenario to run even when catalogd has only 1 replica, which makes the step "catalogd is ready to reconcile resources" fail with exit status 1. After olmNamespace is populated, query the catalogd deployment's spec.replicas and set the gate unconditionally to (replicas >= 2), overriding the earlier node-count result. For upgrade scenarios the detectOLMDeployment early-return keeps the node-count value intact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
openshift-merge-botBot
pushed a commit
that referenced
this pull request
Apr 29, 2026
Two fixes on top of the catalogd HA work (#2674): 1. serverutil: wrap http.Server.Serve error with the catalog listen address so the failure message is self-diagnosing in aggregated manager logs. 2. e2e: fix CatalogdHA gate to override node-count check with actual catalogd replica count. The node-count heuristic fires on any multi-node cluster (e.g. OpenShift standard e2e on AWS), causing the @CatalogdHA scenario to run even when catalogd has only 1 replica, which makes the step "catalogd is ready to reconcile resources" fail with exit status 1. After olmNamespace is populated, query the catalogd deployment's spec.replicas and set the gate unconditionally to (replicas >= 2), overriding the earlier node-count result. For upgrade scenarios the detectOLMDeployment early-return keeps the node-count value intact. Signed-off-by: Todd Short <tshort@redhat.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
tmshort added a commit
to tmshort/operator-framework-operator-controller
that referenced
this pull request
Jul 23, 2026
Backport of upstream #2674. In HA topology (replicas=2), the old catalogd serverutil used OnlyServeWhenLeader=true: the non-leader pod bound the TCP port via net.Listen so the OS accepted connections, but no HTTP server was running. Operator-controller hit the non-leader ~50% of the time and the connection hung silently. With the HTTP client timeout raised to 5m (OCPBUGS-92037), this hang lasted the full 5 minutes instead of 10s, causing catalog caches to never be populated within test windows. Fix: - catalogd serverutil: switch to NeedLeaderElection=false so all pods serve HTTP; non-leaders return 404 (empty local cache) instead of hanging. Register a readiness check so the pod is excluded from endpoints until the server is ready. - client.go: do not cache non-200 responses; return the error directly so the next reconcile retries a fresh HTTP request and eventually hits the leader. Upstream: operator-framework/operator-controller#2674 Signed-off-by: Todd Short <tshort@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes catalogd to permit multiple replicas. Adds tests to the experimental-e2e where there are multiple nodes, and subsequently, multiple replicas. This is part of a fix to avoid OLMv1 becoming unready when a cluster is upgraded.
Related to: OCPBUGS-62517
catalogd's catalog HTTP server previously called net.Listen eagerly at startup on every pod, even non-leaders that never called http.Serve. With replicas > 1 this caused ~50% of catalog requests to queue indefinitely in the kernel accept backlog.
Fix: replace manager.Server with a custom catalogServerRunnable that binds the port lazily inside Start() (only called on the leader) and closes a channel to signal readiness. A /readyz check selects on that channel, so non-leader pods fail the probe and are excluded from Service endpoints. cmd/catalogd/main.go health/readiness setup is now identical to cmd/operator-controller/main.go.
With that fix in place, helm/experimental.yaml is updated to set replicas: 2 for both components so the experimental (2-node kind) e2e suite exercises the multi-replica path. A new @CatalogdHA scenario force-deletes the catalogd leader pod and asserts that a new leader is elected and the catalog resumes serving. The scenario is automatically skipped in the standard 1-node suite (gated via BeforeSuite node-count detection in featureGates). The experimental e2e timeout is bumped from 20m to 25m to accommodate worst-case leader re-election (~163s).
Description
Reviewer Checklist