Skip to content

Commit 3f15a9d

Browse files
yoav-katzyoav-katz
committed
fix(dcs): clarify leader Service error and restore NodePort test coverage
1 parent d5b6db0 commit 3f15a9d

6 files changed

Lines changed: 64 additions & 42 deletions

File tree

‎internal/controller/postgrescluster/cluster_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -594,7 +594,7 @@ func TestGenerateClusterPrimaryService(t *testing.T) {
594594
leader.Spec.ClusterIP = "1.9.8.3"
595595

596596
_, _, err := reconciler.generateClusterPrimaryService(cluster, nil)
597-
assert.ErrorContains(t, err, "not implemented")
597+
assert.ErrorContains(t, err, "not available yet")
598598

599599
alwaysExpect := func(t testing.TB, service *corev1.Service, endpoints *corev1.Endpoints) {
600600
assert.Assert(t, cmp.MarshalMatches(service.TypeMeta, `
@@ -699,7 +699,7 @@ func TestReconcileClusterPrimaryService(t *testing.T) {
699699
assert.NilError(t, cc.Create(ctx, cluster))
700700

701701
_, err := reconciler.reconcileClusterPrimaryService(ctx, cluster, nil)
702-
assert.ErrorContains(t, err, "not implemented")
702+
assert.ErrorContains(t, err, "not available yet")
703703

704704
leader := &corev1.Service{}
705705
leader.Spec.ClusterIP = "192.0.2.10"

‎internal/controller/postgrescluster/instance.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1272,7 +1272,8 @@ func (r *Reconciler) reconcileInstance(
12721272
}
12731273

12741274
// K8SPG-708
1275-
initImage, err := k8s.InitImage(ctx, r.Client, cluster, spec)
1275+
var initImage string
1276+
initImage, err = k8s.InitImage(ctx, r.Client, cluster, spec)
12761277
if err != nil {
12771278
return errors.Wrap(err, "failed to determine initial init image")
12781279
}

‎internal/patroni/config.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -354,8 +354,6 @@ func instanceEnvironment(
354354
// - https://github.com/zalando/patroni/blob/v2.0.2/patroni/config.py#L247
355355
// - https://github.com/zalando/patroni/blob/v2.0.2/patroni/postgresql/postmaster.py#L215-L216
356356

357-
// Insert after PATRONI_NAME: appending would reorder existing StatefulSet
358-
// env and force a rolling restart.
359357
variables := []corev1.EnvVar{
360358
// Set "name" to the v1.Pod's name. Required for Patroni's node identity.
361359
// Patroni must be restarted when changing this value.
@@ -420,6 +418,8 @@ func instanceEnvironment(
420418
},
421419
}
422420

421+
// Insert after PATRONI_NAME: appending would reorder existing StatefulSet
422+
// env and force a rolling restart.
423423
return slices.Insert(variables, 1, dcsEnvVars...)
424424
}
425425

‎internal/patroni/config_test.go‎

Lines changed: 9 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"sigs.k8s.io/yaml"
2020

2121
"github.com/percona/percona-postgresql-operator/v2/internal/naming"
22+
"github.com/percona/percona-postgresql-operator/v2/internal/patroni/dcs"
2223
"github.com/percona/percona-postgresql-operator/v2/internal/postgres"
2324
"github.com/percona/percona-postgresql-operator/v2/internal/testing/cmp"
2425
"github.com/percona/percona-postgresql-operator/v2/internal/testing/require"
@@ -27,24 +28,6 @@ import (
2728
"github.com/percona/percona-postgresql-operator/v2/pkg/apis/upstream.pgv2.percona.com/v1beta1"
2829
)
2930

30-
// kubernetesClusterYAML stands in for dcs.For(cluster).ClusterYAML(cluster)
31-
// (see internal/patroni/dcs) without importing that package from here.
32-
func kubernetesClusterYAML(cluster *v1beta1.PostgresCluster) map[string]any {
33-
labels := map[string]string{naming.LabelCluster: cluster.Name}
34-
if cluster.CompareVersion("2.9.0") >= 0 {
35-
labels = naming.Merge(cluster.Spec.Metadata.GetLabelsOrNil(), labels)
36-
}
37-
return map[string]any{
38-
"kubernetes": map[string]any{
39-
"namespace": cluster.Namespace,
40-
"role_label": naming.LabelRole,
41-
"scope_label": naming.LabelPatroni,
42-
"use_endpoints": true,
43-
"labels": labels,
44-
},
45-
}
46-
}
47-
4831
func TestClusterYAML(t *testing.T) {
4932
t.Parallel()
5033

@@ -55,7 +38,7 @@ func TestClusterYAML(t *testing.T) {
5538
cluster.Namespace = "some-namespace"
5639
cluster.Name = "cluster-name"
5740

58-
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, kubernetesClusterYAML(cluster))
41+
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, dcs.For(cluster).ClusterYAML(cluster))
5942
assert.NilError(t, err)
6043
assert.Equal(t, data, strings.TrimSpace(`
6144
# Generated by postgres-operator. DO NOT EDIT UNLESS YOU KNOW WHAT YOU'RE DOING.
@@ -122,7 +105,7 @@ watchdog:
122105
},
123106
}
124107

125-
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, kubernetesClusterYAML(cluster))
108+
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, dcs.For(cluster).ClusterYAML(cluster))
126109
assert.NilError(t, err)
127110

128111
var parsed map[string]any
@@ -152,7 +135,7 @@ watchdog:
152135
}
153136
cluster.Spec.Patroni.Default()
154137

155-
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, kubernetesClusterYAML(cluster))
138+
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, dcs.For(cluster).ClusterYAML(cluster))
156139
assert.NilError(t, err)
157140

158141
var parsed map[string]any
@@ -174,7 +157,7 @@ watchdog:
174157
}
175158
cluster.Spec.Patroni.Default()
176159

177-
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, kubernetesClusterYAML(cluster))
160+
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, dcs.For(cluster).ClusterYAML(cluster))
178161
assert.NilError(t, err)
179162

180163
var parsed map[string]any
@@ -199,7 +182,7 @@ watchdog:
199182
}
200183
cluster.Spec.Patroni.Default()
201184

202-
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, kubernetesClusterYAML(cluster))
185+
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, dcs.For(cluster).ClusterYAML(cluster))
203186
assert.NilError(t, err)
204187

205188
var parsed map[string]any
@@ -221,7 +204,7 @@ watchdog:
221204
cluster.Spec.PostgresVersion = 17
222205
cluster.Spec.Extensions.PGTDE.Enabled = true
223206

224-
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, kubernetesClusterYAML(cluster))
207+
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, dcs.For(cluster).ClusterYAML(cluster))
225208
assert.NilError(t, err)
226209

227210
var parsed map[string]any
@@ -248,7 +231,7 @@ watchdog:
248231
cluster.Spec.Patroni = &v1beta1.PatroniSpec{}
249232
cluster.Spec.Patroni.Default()
250233

251-
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, kubernetesClusterYAML(cluster))
234+
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, dcs.For(cluster).ClusterYAML(cluster))
252235
assert.NilError(t, err)
253236

254237
var parsed map[string]any
@@ -268,7 +251,7 @@ watchdog:
268251
cluster.Name = "cluster-name"
269252
cluster.Spec.PostgresVersion = 14
270253

271-
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, kubernetesClusterYAML(cluster))
254+
data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, dcs.For(cluster).ClusterYAML(cluster))
272255
assert.NilError(t, err)
273256
assert.Equal(t, data, strings.TrimSpace(`
274257
# Generated by postgres-operator. DO NOT EDIT UNLESS YOU KNOW WHAT YOU'RE DOING.

‎internal/patroni/dcs/kubernetes_endpoints.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -251,8 +251,7 @@ func (kubernetesEndpointsBackend) PrimaryService(
251251
// ClusterIP of the Service created in Reconciler.reconcilePatroniLeaderLease
252252
// when Patroni is using Endpoints.
253253
if leader == nil {
254-
// TODO(cbandy): We need to build a different kind of Service here.
255-
return corev1.ServiceSpec{}, nil, errors.New("Patroni DCS other than Kubernetes Endpoints is not implemented")
254+
return corev1.ServiceSpec{}, nil, errors.New("Patroni leader Service is not available yet")
256255
}
257256

258257
// Allocate no IP address (headless) and manage the Endpoints ourselves.

‎internal/patroni/dcs/kubernetes_endpoints_test.go‎

Lines changed: 48 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -249,15 +249,54 @@ kind: Service
249249
})
250250
}
251251

252-
t.Run("NodePortWithClusterIP", func(t *testing.T) {
253-
cluster := cluster.DeepCopy()
254-
cluster.Spec.Service = &v1beta1.ServiceSpec{Type: "ClusterIP", NodePort: new(int32(32000))}
252+
typesAndPort := []struct {
253+
Description string
254+
Type string
255+
NodePort *int32
256+
Expect func(testing.TB, *corev1.Service, error)
257+
}{
258+
{Description: "ClusterIP with Port 32000", Type: "ClusterIP",
259+
NodePort: new(int32(32000)), Expect: func(t testing.TB, service *corev1.Service, err error) {
260+
assert.ErrorContains(t, err, `NodePort cannot be set with type ClusterIP on Service "pg2-ha"`)
261+
assert.Assert(t, service == nil)
262+
}},
263+
{Description: "NodePort with Port 32001", Type: "NodePort",
264+
NodePort: new(int32(32001)), Expect: func(t testing.TB, service *corev1.Service, err error) {
265+
assert.NilError(t, err)
266+
alwaysExpect(t, service)
267+
assert.Equal(t, service.Spec.Type, corev1.ServiceTypeNodePort)
268+
assert.Assert(t, cmp.MarshalMatches(service.Spec.Ports, `
269+
- name: postgres
270+
nodePort: 32001
271+
port: 9876
272+
protocol: TCP
273+
targetPort: postgres
274+
`))
275+
}},
276+
{Description: "LoadBalancer with Port 32002", Type: "LoadBalancer",
277+
NodePort: new(int32(32002)), Expect: func(t testing.TB, service *corev1.Service, err error) {
278+
assert.Equal(t, service.Spec.Type, corev1.ServiceTypeLoadBalancer)
279+
assert.NilError(t, err)
280+
alwaysExpect(t, service)
281+
assert.Assert(t, cmp.MarshalMatches(service.Spec.Ports, `
282+
- name: postgres
283+
nodePort: 32002
284+
port: 9876
285+
protocol: TCP
286+
targetPort: postgres
287+
`))
288+
}},
289+
}
255290

256-
recorder := new(record.FakeRecorder)
257-
service, err := (kubernetesEndpointsBackend{}).LeaderLeaseService(cluster, recorder)
258-
assert.ErrorContains(t, err, `NodePort cannot be set with type ClusterIP on Service "pg2-ha"`)
259-
assert.Assert(t, service == nil)
260-
})
291+
for _, test := range typesAndPort {
292+
t.Run(test.Description, func(t *testing.T) {
293+
cluster := cluster.DeepCopy()
294+
cluster.Spec.Service = &v1beta1.ServiceSpec{Type: test.Type, NodePort: test.NodePort}
295+
296+
service, err := (kubernetesEndpointsBackend{}).LeaderLeaseService(cluster, new(record.FakeRecorder))
297+
test.Expect(t, service, err)
298+
})
299+
}
261300
}
262301

263302
func TestKubernetesEndpointsPrimaryService(t *testing.T) {
@@ -266,7 +305,7 @@ func TestKubernetesEndpointsPrimaryService(t *testing.T) {
266305

267306
t.Run("NoLeader", func(t *testing.T) {
268307
spec, subset, err := (kubernetesEndpointsBackend{}).PrimaryService(cluster, nil)
269-
assert.ErrorContains(t, err, "not implemented")
308+
assert.ErrorContains(t, err, "not available yet")
270309
assert.DeepEqual(t, spec, corev1.ServiceSpec{})
271310
assert.Assert(t, subset == nil)
272311
})

0 commit comments

Comments
 (0)