Skip to content

Commit 688b7f2

Browse files
naimadswdnDamian Seredyn
naimadswdn
and
Damian Seredyn
authored
fix: Operator crash when PDB defined only for Redis follower (#1563)
Fix bug related to PDB creation only for follower, not the leader. Make MinAvailable and MaxUnavailable mutually exclusive with priority order. Signed-off-by: Damian Seredyn <[email protected]> Co-authored-by: Damian Seredyn <[email protected]>
1 parent 36bdc48 commit 688b7f2

File tree

2 files changed

+106
-19
lines changed

2 files changed

+106
-19
lines changed

‎internal/k8sutils/poddisruption.go‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ func ReconcileRedisPodDisruptionBudget(ctx context.Context, cr *rcvb2.RedisClust
2424
labels := getRedisLabels(cr.Name, cluster, role, cr.GetLabels())
2525
annotations := generateStatefulSetsAnots(cr.ObjectMeta, cr.Spec.KubernetesConfig.IgnoreAnnotations)
2626
pdbMeta := generateObjectMetaInformation(pdbName, cr.Namespace, labels, annotations)
27-
pdbDef := generatePodDisruptionBudgetDef(ctx, cr, role, pdbMeta, cr.Spec.RedisLeader.PodDisruptionBudget)
27+
pdbDef := generatePodDisruptionBudgetDef(ctx, cr, role, pdbMeta, pdbParams)
2828
return CreateOrUpdatePodDisruptionBudget(ctx, pdbDef, cl)
2929
} else {
3030
// Check if one exists, and delete it.
@@ -97,14 +97,14 @@ func generatePodDisruptionBudgetDef(ctx context.Context, cr *rcvb2.RedisCluster,
9797
Selector: lblSelector,
9898
},
9999
}
100-
if pdbParams.MinAvailable != nil {
101-
pdbTemplate.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: (*pdbParams.MinAvailable)}
102-
}
100+
// PodDisruptionBudget spec allows either MinAvailable OR MaxUnavailable, but not both
101+
// Priority: MaxUnavailable > MinAvailable > default quorum
103102
if pdbParams.MaxUnavailable != nil {
104103
pdbTemplate.Spec.MaxUnavailable = &intstr.IntOrString{Type: intstr.Int, IntVal: *pdbParams.MaxUnavailable}
105-
}
106-
// If we don't have a value for either, assume quorum: (N/2)+1
107-
if pdbTemplate.Spec.MaxUnavailable == nil && pdbTemplate.Spec.MinAvailable == nil {
104+
} else if pdbParams.MinAvailable != nil {
105+
pdbTemplate.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: (*pdbParams.MinAvailable)}
106+
} else {
107+
// If we don't have a value for either, assume quorum: (N/2)+1
108108
pdbTemplate.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: (*cr.Spec.ClusterSize / 2) + 1}
109109
}
110110
AddOwnerRefToObject(pdbTemplate, redisClusterAsOwner(cr))
@@ -124,14 +124,14 @@ func generateReplicationPodDisruptionBudgetDef(ctx context.Context, cr *rrvb2.Re
124124
Selector: lblSelector,
125125
},
126126
}
127-
if pdbParams.MinAvailable != nil {
128-
pdbTemplate.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: *pdbParams.MinAvailable}
129-
}
127+
// PodDisruptionBudget spec allows either MinAvailable OR MaxUnavailable, but not both
128+
// Priority: MaxUnavailable > MinAvailable > default quorum
130129
if pdbParams.MaxUnavailable != nil {
131130
pdbTemplate.Spec.MaxUnavailable = &intstr.IntOrString{Type: intstr.Int, IntVal: *pdbParams.MaxUnavailable}
132-
}
133-
// If we don't have a value for either, assume quorum: (N/2)+1
134-
if pdbTemplate.Spec.MaxUnavailable == nil && pdbTemplate.Spec.MinAvailable == nil {
131+
} else if pdbParams.MinAvailable != nil {
132+
pdbTemplate.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: *pdbParams.MinAvailable}
133+
} else {
134+
// If we don't have a value for either, assume quorum: (N/2)+1
135135
pdbTemplate.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: (*cr.Spec.Size / 2) + 1}
136136
}
137137
AddOwnerRefToObject(pdbTemplate, redisReplicationAsOwner(cr))
@@ -151,14 +151,14 @@ func generateSentinelPodDisruptionBudgetDef(ctx context.Context, cr *rsvb2.Redis
151151
Selector: lblSelector,
152152
},
153153
}
154-
if pdbParams.MinAvailable != nil {
155-
pdbTemplate.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: *pdbParams.MinAvailable}
156-
}
154+
// PodDisruptionBudget spec allows either MinAvailable OR MaxUnavailable, but not both
155+
// Priority: MaxUnavailable > MinAvailable > default quorum
157156
if pdbParams.MaxUnavailable != nil {
158157
pdbTemplate.Spec.MaxUnavailable = &intstr.IntOrString{Type: intstr.Int, IntVal: *pdbParams.MaxUnavailable}
159-
}
160-
// If we don't have a value for either, assume quorum: (N/2)+1
161-
if pdbTemplate.Spec.MaxUnavailable == nil && pdbTemplate.Spec.MinAvailable == nil {
158+
} else if pdbParams.MinAvailable != nil {
159+
pdbTemplate.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: *pdbParams.MinAvailable}
160+
} else {
161+
// If we don't have a value for either, assume quorum: (N/2)+1
162162
pdbTemplate.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: (*cr.Spec.Size / 2) + 1}
163163
}
164164
AddOwnerRefToObject(pdbTemplate, redisSentinelAsOwner(cr))

‎internal/k8sutils/poddisruption_test.go‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
package k8sutils
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
"k8s.io/api/policy/v1"
8+
"k8s.io/apimachinery/pkg/util/intstr"
9+
)
10+
11+
type pdbParams struct {
12+
MinAvailable *int32
13+
MaxUnavailable *int32
14+
}
15+
16+
func TestGeneratePodDisruptionBudgetDef_PriorityLogic(t *testing.T) {
17+
cases := []struct {
18+
name string
19+
params pdbParams
20+
clusterSize int32
21+
expectMin *int32
22+
expectMax *int32
23+
}{
24+
{
25+
name: "only MinAvailable set",
26+
params: pdbParams{MinAvailable: int32Ptr(2)},
27+
clusterSize: 4,
28+
expectMin: int32Ptr(2),
29+
expectMax: nil,
30+
},
31+
{
32+
name: "only MaxUnavailable set",
33+
params: pdbParams{MaxUnavailable: int32Ptr(1)},
34+
clusterSize: 4,
35+
expectMin: nil,
36+
expectMax: int32Ptr(1),
37+
},
38+
{
39+
name: "both set, MaxUnavailable wins",
40+
params: pdbParams{MinAvailable: int32Ptr(2), MaxUnavailable: int32Ptr(1)},
41+
clusterSize: 4,
42+
expectMin: nil,
43+
expectMax: int32Ptr(1),
44+
},
45+
{
46+
name: "neither set, default quorum",
47+
params: pdbParams{},
48+
clusterSize: 4,
49+
expectMin: int32Ptr(3), // (4/2)+1
50+
expectMax: nil,
51+
},
52+
}
53+
54+
for _, tc := range cases {
55+
t.Run(tc.name, func(t *testing.T) {
56+
pdb := generateTestPDB(tc.params, tc.clusterSize)
57+
if tc.expectMin != nil {
58+
assert.NotNil(t, pdb.Spec.MinAvailable)
59+
assert.Equal(t, intstr.Int, pdb.Spec.MinAvailable.Type)
60+
assert.Equal(t, *tc.expectMin, pdb.Spec.MinAvailable.IntVal)
61+
} else {
62+
assert.Nil(t, pdb.Spec.MinAvailable)
63+
}
64+
if tc.expectMax != nil {
65+
assert.NotNil(t, pdb.Spec.MaxUnavailable)
66+
assert.Equal(t, intstr.Int, pdb.Spec.MaxUnavailable.Type)
67+
assert.Equal(t, *tc.expectMax, pdb.Spec.MaxUnavailable.IntVal)
68+
} else {
69+
assert.Nil(t, pdb.Spec.MaxUnavailable)
70+
}
71+
})
72+
}
73+
}
74+
75+
func int32Ptr(i int32) *int32 { return &i }
76+
77+
func generateTestPDB(params pdbParams, clusterSize int32) *v1.PodDisruptionBudget {
78+
pdb := &v1.PodDisruptionBudget{Spec: v1.PodDisruptionBudgetSpec{}}
79+
if params.MaxUnavailable != nil {
80+
pdb.Spec.MaxUnavailable = &intstr.IntOrString{Type: intstr.Int, IntVal: *params.MaxUnavailable}
81+
} else if params.MinAvailable != nil {
82+
pdb.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: *params.MinAvailable}
83+
} else {
84+
pdb.Spec.MinAvailable = &intstr.IntOrString{Type: intstr.Int, IntVal: (clusterSize / 2) + 1}
85+
}
86+
return pdb
87+
}

0 commit comments

Comments
 (0)