Skip to content

OCPBUGS-57444: set appropriate rolling update settings #789

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

Merged
merged 1 commit into from
Sep 18, 2025
+124 −5
Merged

OCPBUGS-57444: set appropriate rolling update settings #789

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
3 changes: 2 additions & 1 deletion bindata/oauth-openshift/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
maxUnavailable: 1
# those are being adjusted for each control plane size by the deployment controller
maxUnavailable: 0
maxSurge: 0
selector:
matchLabels:
Expand Down
25 changes: 21 additions & 4 deletions pkg/controllers/deployment/deployment_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/client-go/informers"
coreinformers "k8s.io/client-go/informers/core/v1"
"k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -253,12 +254,17 @@ func (c *oauthServerDeploymentSyncer) Sync(ctx context.Context, syncContext fact
return nil, false, append(errs, fmt.Errorf("unable to ensure at most one pod per node: %v", err))
}

// Set the replica count to the number of master nodes.
masterNodeCount, err := c.countNodes(expectedDeployment.Spec.Template.Spec.NodeSelector)
// Set the replica count to the number of control plane nodes.
controlPlaneCount, err := c.countNodes(expectedDeployment.Spec.Template.Spec.NodeSelector)
if err != nil {
return nil, false, append(errs, fmt.Errorf("failed to determine number of master nodes: %v", err))
return nil, false, append(errs, fmt.Errorf("failed to determine number of control plane nodes: %v", err))
}
expectedDeployment.Spec.Replicas = masterNodeCount
if controlPlaneCount == nil {
return nil, false, append(errs, fmt.Errorf("found nil control plane nodes count"))
}

expectedDeployment.Spec.Replicas = controlPlaneCount
setRollingUpdateParameters(*controlPlaneCount, expectedDeployment)

Comment on lines +257 to 268
Copy link

@coderabbitai coderabbitai bot Sep 17, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

🧩 Analysis chain

Behavioral check: replicas tied to CP count — OK.

Replica assignment from countNodes and the nil-guard look correct. After fixing the helper, this should roll safely on N>=2 and degrade gracefully on N==1.

Run this to catch the undefined symbols before CI:

🏁 Script executed:

#!/bin/bash
# Verify no usage of intstr.FromInt32 and bare max(…) in Go sources
rg -nP --type=go '\bintstr\.FromInt32\s*\(' -C1
rg -nP --type=go '(?<![A-Za-z0-9_])max\s*\(' -C1

Length of output: 569

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "---- intstr.FromInt32 usages ----"
rg -n --type=go 'intstr\.FromInt32' -C1 || true

echo "---- occurrences of max(...) in Go files ----"
rg -n --type=go '\bmax\s*\(' -C3 || true

echo "---- definitions of func max(...) ----"
rg -n --type=go 'func\s+max\s*\(' -C3 || true

echo "---- show setRollingUpdateParameters context (lines ~310-340) ----"
sed -n '310,340p' pkg/controllers/deployment/deployment_controller.go || true

echo "---- show file imports (top 1-140 lines) ----"
sed -n '1,140p' pkg/controllers/deployment/deployment_controller.go || true

Length of output: 11363

Fix undefined 'max' in setRollingUpdateParameters

setRollingUpdateParameters calls max(controlPlaneCount-1, 1) but no max function is defined; add an int32 max helper (e.g. func max(a, b int32) int32 { if a > b { return a } return b }) or replace with an inline conditional.
Location: pkg/controllers/deployment/deployment_controller.go:325-329

🤖 Prompt for AI Agents 
In pkg/controllers/deployment/deployment_controller.go around lines 257 to 268,
setRollingUpdateParameters uses an undefined max(...) when computing
max(controlPlaneCount-1, 1); add a small int32 max helper (for example func
max(a, b int32) int32 { if a > b { return a } return b }) near the top of the
file or replace the call with an inline conditional (if controlPlaneCount-1 > 1
{ ... } else { ... }) so the expression compiles and correctly returns the
larger int32 value.

deployment, _, err := resourceapply.ApplyDeployment(ctx, c.deployments,
syncContext.Recorder(),
Expand Down Expand Up @@ -311,3 +317,14 @@ func (c *oauthServerDeploymentSyncer) getConfigResourceVersions() ([]string, err

return configRVs, nil
}

// Given the control plane sizes, we adjust the max unavailable and max surge values to mimic "MinAvailable".
// We always ensure it is controlPlaneCount - 1, as this allows us to keep have at least a single replica running.
// We also set MaxSurge to always be exactly the control plane count, as this allows us to more quickly replace failing
// deployments with a new replica set. This does not clash with the pod anti affinity set above.
func setRollingUpdateParameters(controlPlaneCount int32, deployment *appsv1.Deployment) {
maxUnavailable := intstr.FromInt32(max(controlPlaneCount-1, 1))
maxSurge := intstr.FromInt32(controlPlaneCount)
deployment.Spec.Strategy.RollingUpdate.MaxUnavailable = &maxUnavailable
deployment.Spec.Strategy.RollingUpdate.MaxSurge = &maxSurge
}
Comment on lines +321 to +330
Copy link

@coderabbitai coderabbitai bot Sep 17, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix compile-time errors and add defensive init.

max is undefined in this file, and intstr.FromInt32 does not exist (use FromInt). Also guard against a nil RollingUpdate to avoid panics. Clamp the 0-node case to 0 unavailability.

Apply this diff:

-// Given the control plane sizes, we adjust the max unavailable and max surge values to mimic "MinAvailable".
-// We always ensure it is controlPlaneCount - 1, as this allows us to keep have at least a single replica running.
-// We also set MaxSurge to always be exactly the control plane count, as this allows us to more quickly replace failing
-// deployments with a new replica set. This does not clash with the pod anti affinity set above.
+// Given the control plane size, adjust MaxUnavailable and MaxSurge to approximate a "MinAvailable"-like policy.
+// When controlPlaneCount > 1, we keep at least one replica running (MaxUnavailable = controlPlaneCount - 1).
+// On single-node control planes, anti-affinity may cause a brief gap during updates.
+// MaxSurge is set to controlPlaneCount to speed up replacement; anti-affinity prevents co-location.
 func setRollingUpdateParameters(controlPlaneCount int32, deployment *appsv1.Deployment) {
-	maxUnavailable := intstr.FromInt32(max(controlPlaneCount-1, 1))
-	maxSurge := intstr.FromInt32(controlPlaneCount)
-	deployment.Spec.Strategy.RollingUpdate.MaxUnavailable = &maxUnavailable
-	deployment.Spec.Strategy.RollingUpdate.MaxSurge = &maxSurge
+	if deployment.Spec.Strategy.RollingUpdate == nil {
+		deployment.Spec.Strategy.Type = appsv1.RollingUpdateDeploymentStrategyType
+		deployment.Spec.Strategy.RollingUpdate = &appsv1.RollingUpdateDeployment{}
+	}
+	var mu int32
+	switch {
+	case controlPlaneCount <= 0:
+		mu = 0
+	case controlPlaneCount == 1:
+		mu = 1
+	default:
+		mu = controlPlaneCount - 1
+	}
+	maxUnavailable := intstr.FromInt(int(mu))
+	maxSurge := intstr.FromInt(int(controlPlaneCount))
+	deployment.Spec.Strategy.RollingUpdate.MaxUnavailable = &maxUnavailable
+	deployment.Spec.Strategy.RollingUpdate.MaxSurge = &maxSurge
 }
🤖 Prompt for AI Agents 
In pkg/controllers/deployment/deployment_controller.go around lines 321-330,
replace the use of the undefined max and non-existent intstr.FromInt32 and add a
defensive init: ensure deployment.Spec.Strategy.RollingUpdate is non-nil
(allocate a new RollingUpdate if nil), compute maxUnavailableInt :=
int(controlPlaneCount-1) and if maxUnavailableInt < 0 set it to 0 (clamping the
0-node case), set maxSurgeInt := int(controlPlaneCount), then use
intstr.FromInt(maxUnavailableInt) and intstr.FromInt(maxSurgeInt) to create
values and assign them to deployment.Spec.Strategy.RollingUpdate.MaxUnavailable
and .MaxSurge respectively.

101 changes: 101 additions & 0 deletions pkg/controllers/deployment/deployment_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
package deployment

import (
"testing"

appsv1 "k8s.io/api/apps/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

func TestSetRollingUpdateParameters(t *testing.T) {
testCases := []struct {
name string
controlPlaneCount int32
expectedMaxUnavailable int32
expectedMaxSurge int32
}{
{
name: "single control plane node",
controlPlaneCount: 1,
expectedMaxUnavailable: 1, // max(1-1, 1) = max(0, 1) = 1
expectedMaxSurge: 1,
},
{
name: "two control plane nodes",
controlPlaneCount: 2,
expectedMaxUnavailable: 1, // max(2-1, 1) = max(1, 1) = 1
expectedMaxSurge: 2,
},
{
name: "three control plane nodes",
controlPlaneCount: 3,
expectedMaxUnavailable: 2, // max(3-1, 1) = max(2, 1) = 2
expectedMaxSurge: 3,
},
{
name: "four control plane nodes",
controlPlaneCount: 4,
expectedMaxUnavailable: 3, // max(4-1, 1) = max(3, 1) = 3
expectedMaxSurge: 4,
},
{
name: "five control plane nodes",
controlPlaneCount: 5,
expectedMaxUnavailable: 4, // max(5-1, 1) = max(4, 1) = 4
expectedMaxSurge: 5,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
// Create a test deployment with rolling update strategy
deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: "test-deployment",
Namespace: "test-namespace",
},
Spec: appsv1.DeploymentSpec{
Strategy: appsv1.DeploymentStrategy{
Type: appsv1.RollingUpdateDeploymentStrategyType,
RollingUpdate: &appsv1.RollingUpdateDeployment{
MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: 0},
MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: 0},
},
},
},
}

// Call the function under test
setRollingUpdateParameters(tc.controlPlaneCount, deployment)

// Verify MaxUnavailable is set correctly
if deployment.Spec.Strategy.RollingUpdate.MaxUnavailable == nil {
t.Errorf("MaxUnavailable should not be nil")
} else {
actualMaxUnavailable := deployment.Spec.Strategy.RollingUpdate.MaxUnavailable.IntVal
if actualMaxUnavailable != tc.expectedMaxUnavailable {
t.Errorf("Expected MaxUnavailable to be %d, got %d", tc.expectedMaxUnavailable, actualMaxUnavailable)
}
}

// Verify MaxSurge is set correctly
if deployment.Spec.Strategy.RollingUpdate.MaxSurge == nil {
t.Errorf("MaxSurge should not be nil")
} else {
actualMaxSurge := deployment.Spec.Strategy.RollingUpdate.MaxSurge.IntVal
if actualMaxSurge != tc.expectedMaxSurge {
t.Errorf("Expected MaxSurge to be %d, got %d", tc.expectedMaxSurge, actualMaxSurge)
}
}

// Verify the values are of type Int (not String)
if deployment.Spec.Strategy.RollingUpdate.MaxUnavailable.Type != intstr.Int {
t.Errorf("Expected MaxUnavailable to be of type Int, got %v", deployment.Spec.Strategy.RollingUpdate.MaxUnavailable.Type)
}
if deployment.Spec.Strategy.RollingUpdate.MaxSurge.Type != intstr.Int {
t.Errorf("Expected MaxSurge to be of type Int, got %v", deployment.Spec.Strategy.RollingUpdate.MaxSurge.Type)
}
})
}
}