Skip to content

OCPBUGS-86719: Use zero-downtime rollout strategy for console pods #1168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asadawar wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-86719: Use zero-downtime rollout strategy for console pods #1168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions pkg/console/subresource/deployment/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,32 @@ func withAffinity(
func withStrategy(deployment *appsv1.Deployment, infrastructureConfig *configv1.Infrastructure) {
rollingUpdateParams := &appsv1.RollingUpdateDeployment{}
if ShouldDeployHA(infrastructureConfig) {
rollingUpdateParams = &appsv1.RollingUpdateDeployment{
MaxSurge: &intstr.IntOrString{
IntVal: int32(3),
},
MaxUnavailable: &intstr.IntOrString{
IntVal: int32(1),
},
topology := infrastructureConfig.Status.ControlPlaneTopology
if topology == configv1.DualReplicaTopologyMode || topology == configv1.HighlyAvailableArbiterMode {
// On 2-node topologies the required pod anti-affinity prevents
// scheduling a surge pod when every eligible node already runs
// a console pod, so maxUnavailable must be >= 1 to allow the
// rollout to make progress.
rollingUpdateParams = &appsv1.RollingUpdateDeployment{
MaxSurge: &intstr.IntOrString{
IntVal: int32(1),
},
MaxUnavailable: &intstr.IntOrString{
IntVal: int32(1),
},
}
} else {
// On 3+ node topologies a free node is available for the surge
// pod, so maxUnavailable=0 guarantees zero-downtime rollouts:
// no old pod is terminated until its replacement is ready.
rollingUpdateParams = &appsv1.RollingUpdateDeployment{
MaxSurge: &intstr.IntOrString{
IntVal: int32(1),
},
MaxUnavailable: &intstr.IntOrString{
IntVal: int32(0),
},
}
}
}
deployment.Spec.Strategy.RollingUpdate = rollingUpdateParams
Expand Down
62 changes: 50 additions & 12 deletions pkg/console/subresource/deployment/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,10 @@ func TestDefaultDeployment(t *testing.T) {
Type: appsv1.RollingUpdateDeploymentStrategyType,
RollingUpdate: &appsv1.RollingUpdateDeployment{
MaxSurge: &intstr.IntOrString{
IntVal: int32(3),
IntVal: int32(1),
},
MaxUnavailable: &intstr.IntOrString{
IntVal: int32(1),
IntVal: int32(0),
},
},
},
Expand Down Expand Up @@ -343,10 +343,10 @@ func TestDefaultDeployment(t *testing.T) {
Type: appsv1.RollingUpdateDeploymentStrategyType,
RollingUpdate: &appsv1.RollingUpdateDeployment{
MaxSurge: &intstr.IntOrString{
IntVal: int32(3),
IntVal: int32(1),
},
MaxUnavailable: &intstr.IntOrString{
IntVal: int32(1),
IntVal: int32(0),
},
},
},
Expand Down Expand Up @@ -494,10 +494,10 @@ func TestDefaultDeployment(t *testing.T) {
Type: appsv1.RollingUpdateDeploymentStrategyType,
RollingUpdate: &appsv1.RollingUpdateDeployment{
MaxSurge: &intstr.IntOrString{
IntVal: int32(3),
IntVal: int32(1),
},
MaxUnavailable: &intstr.IntOrString{
IntVal: int32(1),
IntVal: int32(0),
},
},
},
Expand Down Expand Up @@ -1518,11 +1518,21 @@ func TestWithStrategy(t *testing.T) {
infrastructureConfigSingleReplica := infrastructureConfigWithTopology(configv1.SingleReplicaTopologyMode, configv1.SingleReplicaTopologyMode)
infrastructureConfigExternalTopologyHighlyAvailable := infrastructureConfigWithTopology(configv1.ExternalTopologyMode, configv1.HighlyAvailableTopologyMode)
infrastructureConfigExternalTopologySingleReplica := infrastructureConfigWithTopology(configv1.ExternalTopologyMode, configv1.SingleReplicaTopologyMode)
infrastructureConfigDualReplica := infrastructureConfigWithTopology(configv1.DualReplicaTopologyMode, configv1.HighlyAvailableTopologyMode)
infrastructureConfigArbiter := infrastructureConfigWithTopology(configv1.HighlyAvailableArbiterMode, configv1.HighlyAvailableTopologyMode)

singleReplicaStrategy := appsv1.RollingUpdateDeployment{}
highAvailStrategy := appsv1.RollingUpdateDeployment{
zeroDowntimeStrategy := appsv1.RollingUpdateDeployment{
MaxSurge: &intstr.IntOrString{
IntVal: int32(3),
IntVal: int32(1),
},
MaxUnavailable: &intstr.IntOrString{
IntVal: int32(0),
},
}
constrainedHAStrategy := appsv1.RollingUpdateDeployment{
MaxSurge: &intstr.IntOrString{
IntVal: int32(1),
},
MaxUnavailable: &intstr.IntOrString{
IntVal: int32(1),
Expand Down Expand Up @@ -1557,7 +1567,7 @@ func TestWithStrategy(t *testing.T) {
want: &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Strategy: appsv1.DeploymentStrategy{
RollingUpdate: &highAvailStrategy,
RollingUpdate: &zeroDowntimeStrategy,
},
},
},
Expand Down Expand Up @@ -1585,7 +1595,35 @@ func TestWithStrategy(t *testing.T) {
want: &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Strategy: appsv1.DeploymentStrategy{
RollingUpdate: &highAvailStrategy,
RollingUpdate: &zeroDowntimeStrategy,
},
},
},
},
{
name: "Test DualReplica Strategy uses maxUnavailable=1",
args: args{
deployment: &appsv1.Deployment{},
infrastructureConfig: infrastructureConfigDualReplica,
},
want: &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Strategy: appsv1.DeploymentStrategy{
RollingUpdate: &constrainedHAStrategy,
},
},
},
},
{
name: "Test Arbiter Strategy uses maxUnavailable=1",
args: args{
deployment: &appsv1.Deployment{},
infrastructureConfig: infrastructureConfigArbiter,
},
want: &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Strategy: appsv1.DeploymentStrategy{
RollingUpdate: &constrainedHAStrategy,
},
},
},
Expand Down Expand Up @@ -1891,10 +1929,10 @@ func TestDefaultDownloadsDeployment(t *testing.T) {
Type: appsv1.RollingUpdateDeploymentStrategyType,
RollingUpdate: &appsv1.RollingUpdateDeployment{
MaxSurge: &intstr.IntOrString{
IntVal: int32(3),
IntVal: int32(1),
},
MaxUnavailable: &intstr.IntOrString{
IntVal: int32(1),
IntVal: int32(0),
},
},
},
Expand Down
6 changes: 3 additions & 3 deletions test-e2e.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@ OPENSHIFT_CI=${OPENSHIFT_CI:=false}

echo "Running tests..."
if [ "$OPENSHIFT_CI" = true ]; then
KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 30m -v ./test/e2e/ 2>&1 | tee "$ARTIFACT_DIR/test.out"
KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 40m -v ./test/e2e/ 2>&1 | tee "$ARTIFACT_DIR/test.out"
RESULT="${PIPESTATUS[0]}"

go-junit-report < "$ARTIFACT_DIR/test.out" > "$ARTIFACT_DIR/junit.xml"
if [ "$RESULT" -ne 0 ]; then
exit 255
fi
else
echo 'KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 30m -v ./test/e2e/'
KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 30m -v ./test/e2e/
echo 'KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 40m -v ./test/e2e/'

@coderabbitai coderabbitai Bot Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix single quotes to allow variable expansion.

The echo statement uses single quotes, which prevent ${KUBECONFIG} from expanding. The output will literally show ${KUBECONFIG} instead of the actual kubeconfig path, which is misleading for debugging.

🔧 Proposed fix 
-	echo 'KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 40m -v ./test/e2e/'
+	echo "KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 40m -v ./test/e2e/"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
echo 'KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 40m -v ./test/e2e/'
echo "KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 40m -v ./test/e2e/"
🧰 Tools
🪛 Shellcheck (0.11.0)

[info] 20-20: Expressions don't expand in single quotes, use double quotes for that.

(SC2016)

🤖 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.sh` at line 20, The echo line currently uses single quotes so
${KUBECONFIG} is not expanded; update the echo in test-e2e.sh (the echo
'KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 40m -v ./test/e2e/') to use
double quotes so the shell expands ${KUBECONFIG} (i.e., change the quoting
around the echo argument to allow variable substitution).

@asadawar asadawar Jun 4, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The single quotes are intentional. This echo shows the user the command template to run manually, not an expanded version. It's the same pattern as before this PR (was single-quoted with 30m too).

@coderabbitai coderabbitai Bot Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asadawar, understood — the single quotes are deliberate to display the literal command template (with ${KUBECONFIG} unexpanded) as a reference for manual execution. That's a reasonable intent, and the pattern is consistent with the previous version of the script. I'll note this for future reviews.

✏️ Learnings added 
Learnt from: asadawar
Repo: openshift/console-operator PR: 1168
File: test-e2e.sh:20-20
Timestamp: 2026-06-04T16:38:24.616Z
Learning: In `test-e2e.sh` (openshift/console-operator), the `else` branch echo statement intentionally uses single quotes (`echo 'KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 40m -v ./test/e2e/'`) so that `${KUBECONFIG}` is displayed literally as a command template for the user to run manually, not expanded. This is intentional design — do not flag it as a ShellCheck SC2016 issue.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: CR
Repo: openshift/console-operator PR: 0
File: TESTING.md:0-0
Timestamp: 2026-05-28T16:57:00.121Z
Learning: Run end-to-end tests using `make test-e2e` (requires cluster access)

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-01T15:06:28.169Z
Learning: Applies to **/{test,tests,e2e,integration}/**/*_test.go : When new Ginkgo e2e tests are added, check for external connectivity requirements including connections to public internet hosts (e.g., google.com, github.com, quay.io, registry.redhat.io), pulling images from public registries, downloading content from external URLs, DNS resolution of public hostnames, and connections to external APIs or services outside the cluster

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-01T15:06:28.169Z
Learning: Applies to **/{test,tests,e2e,integration}/**/*_test.go : When new Ginkgo e2e tests are added, check whether they reference namespaces that do not exist on MicroShift: openshift-kube-apiserver, openshift-kube-controller-manager, openshift-kube-scheduler

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-01T15:06:28.169Z
Learning: Applies to **/{test,tests,e2e,integration}/**/*_test.go : When new Ginkgo e2e tests are added, check whether they make assumptions about multi-node or HA clusters in Single Node OpenShift (SNO). Flag tests that expect multiple control-plane/master nodes, multiple worker nodes, pod anti-affinity, node-to-node communication patterns, leader election failover, pod rescheduling to different nodes, node scaling, separate infra/worker/master roles, rolling update assumptions, or ingress/load balancing behavior dependent on multiple endpoints on different nodes

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-01T15:06:28.169Z
Learning: Applies to **/{test,tests,e2e,integration}/**/*_test.go : Review Ginkgo test code for appropriate timeouts - operations that interact with the cluster must include timeouts. Flag indefinite waits or missing timeouts on Eventually/Consistently calls

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-01T15:06:28.169Z
Learning: Applies to **/{test,tests,e2e,integration}/**/*_test.go : When new Ginkgo e2e tests are added, check whether they make unsupported MicroShift assumptions including multi-node or HA assumptions, FeatureGate resources, upgrade/update workflows based on ClusterVersion, node scaling expectations, or multi-replica control-plane component deployments

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: .claude/skills/e2e-test-review.md:0-0
Timestamp: 2026-06-04T07:36:55.991Z
Learning: Applies to test/e2e/**/*.go : Use 5-second poll intervals for most checks to balance responsiveness with API load; adjust based on what is being polled

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-01T15:06:28.169Z
Learning: Applies to **/{test,tests,e2e,integration}/**/*_test.go : When new Ginkgo e2e tests are added, check whether they use any APIs or features that are NOT available on MicroShift. Flag tests that reference Project/ProjectRequest, Build/BuildConfig, DeploymentConfig, ClusterOperator, ClusterVersion, Etcd operator, CSV/OLM resources, MachineSet/Machine/MachineHealthCheck, ClusterAutoscaler/MachineAutoscaler, Console, Monitoring stack components, ImageRegistry operator, Samples operator, OperatorHub/CatalogSource/PackageManifest, CloudCredential/CredentialsRequest, Storage operator, Network operator CRDs, or any OpenShift API groups besides Route and SecurityContextConstraints

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: vendor/github.com/fsnotify/fsnotify/CONTRIBUTING.md:0-0
Timestamp: 2026-06-01T15:02:53.921Z
Learning: Run `go test ./...` to execute all tests; use the `-short` flag to make stress tests run faster

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-01T15:06:28.169Z
Learning: Applies to **/{test,tests,e2e,integration}/**/*_test.go : Review Ginkgo test code for proper setup and cleanup - tests should use BeforeEach/AfterEach for setup and cleanup. Flag tests that create resources without cleanup, especially cluster-scoped resources

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: .claude/skills/e2e-test-review.md:0-0
Timestamp: 2026-06-04T07:36:55.991Z
Learning: Applies to test/e2e/**/*.go : Use `framework.AsyncOperationTimeout` constant instead of hardcoding timeout durations in test code

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: .claude/skills/e2e-test-review.md:0-0
Timestamp: 2026-06-04T07:36:55.991Z
Learning: Applies to test/e2e/**/*.go : Provide helpful error messages with context in assertions (e.g., namespace, name, timeout duration) rather than vague messages

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: vendor/github.com/NYTimes/gziphandler/CONTRIBUTING.md:0-0
Timestamp: 2026-06-01T15:02:45.315Z
Learning: Ensure code changes pass `go test` locally and on Travis CI

Learnt from: CR
Repo: openshift/console-operator PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-01T15:06:28.169Z
Learning: Applies to **/{test,tests,e2e,integration}/**/*_test.go : Review Ginkgo test code for consistency with codebase patterns - tests should follow existing patterns for how fixtures are created, clients are obtained, and waits are structured

KUBERNETES_CONFIG=${KUBECONFIG} go test -timeout 40m -v ./test/e2e/
fi

echo "Success"