Fix connection pooler deployment selectors (#1213)

Stick with the existing pooler deployment selector labels to make it compatible with existing deployments.

Make the use of additional labels clear and avoid where not needed.

Deployment Selector and Service Selector now do not use extra labels, pod spec does.

e2e/scripts/watch_objects.sh

@@ -16,7 +16,8 @@ echo 'Statefulsets'
 kubectl get statefulsets --all-namespaces
 echo
 echo 'Deployments'
-kubectl get deployments --all-namespaces -l application=db-connection-pooler -l name=postgres-operator
+kubectl get deployments --all-namespaces -l application=db-connection-pooler
+kubectl get deployments --all-namespaces -l application=postgres-operator
 echo
 echo
 echo 'Step from operator deployment'

e2e/tests/test_e2e.py

@@ -152,6 +152,40 @@ class EndToEndTestCase(unittest.TestCase):
             print('Operator log: {}'.format(k8s.get_operator_log()))
             raise
 
+    @timeout_decorator.timeout(TEST_TIMEOUT_SEC)
+    def test_overwrite_pooler_deployment(self):
+        self.k8s.create_with_kubectl("manifests/minimal-fake-pooler-deployment.yaml")
+        self.eventuallyEqual(lambda: self.k8s.get_operator_state(), {"0": "idle"}, "Operator does not get in sync")
+        self.eventuallyEqual(lambda: self.k8s.get_deployment_replica_count(name="acid-minimal-cluster-pooler"), 1,
+                             "Initial broken deplyment not rolled out")
+
+        self.k8s.api.custom_objects_api.patch_namespaced_custom_object(
+        'acid.zalan.do', 'v1', 'default',
+        'postgresqls', 'acid-minimal-cluster',
+        {
+            'spec': {
+                'enableConnectionPooler': True
+            }
+        })
+
+        self.eventuallyEqual(lambda: self.k8s.get_operator_state(), {"0": "idle"}, "Operator does not get in sync")
+        self.eventuallyEqual(lambda: self.k8s.get_deployment_replica_count(name="acid-minimal-cluster-pooler"), 2,
+                             "Operator did not succeed in overwriting labels")
+
+        self.k8s.api.custom_objects_api.patch_namespaced_custom_object(
+        'acid.zalan.do', 'v1', 'default',
+        'postgresqls', 'acid-minimal-cluster',
+        {
+            'spec': {
+                'enableConnectionPooler': False
+            }
+        })
+
+        self.eventuallyEqual(lambda: self.k8s.get_operator_state(), {"0": "idle"}, "Operator does not get in sync")
+        self.eventuallyEqual(lambda: self.k8s.count_running_pods("connection-pooler=acid-minimal-cluster-pooler"),
+                             0, "Pooler pods not scaled down")
+
+
     @timeout_decorator.timeout(TEST_TIMEOUT_SEC)
     def test_enable_disable_connection_pooler(self):
         '''

manifests/minimal-fake-pooler-deployment.yaml

@@ -0,0 +1,35 @@
+# will not run but is good enough for tests to fail
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: acid-minimal-cluster-pooler
+  labels:
+    application: db-connection-pooler
+    connection-pooler: acid-minimal-cluster-pooler
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      application: db-connection-pooler
+      connection-pooler: acid-minimal-cluster-pooler
+      cluster-name: acid-minimal-cluster
+  template:
+    metadata:
+      labels:
+        application: db-connection-pooler
+        connection-pooler: acid-minimal-cluster-pooler
+        cluster-name: acid-minimal-cluster
+    spec:
+      serviceAccountName: postgres-operator
+      containers:
+      - name: postgres-operator
+        image: registry.opensource.zalan.do/acid/pgbouncer:master-11
+        imagePullPolicy: IfNotPresent
+        resources:
+          requests:
+            cpu: 100m
+            memory: 250Mi
+          limits:
+            cpu: 500m
+            memory: 500Mi
+        env: []

manifests/postgres-operator.yaml

@@ -2,6 +2,8 @@ apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: postgres-operator
+  labels:
+    application: postgres-operator
 spec:
   replicas: 1
   strategy:

pkg/cluster/connection_pooler.go

@@ -78,22 +78,22 @@ func needReplicaConnectionPoolerWorker(spec *acidv1.PostgresSpec) bool {
 // have e.g. different `application` label, so that recreatePod operation will
 // not interfere with it (it lists all the pods via labels, and if there would
 // be no difference, it will recreate also pooler pods).
-func (c *Cluster) connectionPoolerLabelsSelector(role PostgresRole) *metav1.LabelSelector {
-	connectionPoolerLabels := labels.Set(map[string]string{})
+func (c *Cluster) connectionPoolerLabels(role PostgresRole, addExtraLabels bool) *metav1.LabelSelector {
+	poolerLabels := c.labelsSet(addExtraLabels)
 
-	extraLabels := labels.Set(map[string]string{
-		"connection-pooler": c.connectionPoolerName(role),
-		"application":       "db-connection-pooler",
-		"spilo-role":        string(role),
-		"cluster-name":      c.Name,
-		"Namespace":         c.Namespace,
-	})
+	// TODO should be config values
+	poolerLabels["application"] = "db-connection-pooler"
+	poolerLabels["connection-pooler"] = c.connectionPoolerName(role)
 
-	connectionPoolerLabels = labels.Merge(connectionPoolerLabels, c.labelsSet(false))
-	connectionPoolerLabels = labels.Merge(connectionPoolerLabels, extraLabels)
+	if addExtraLabels {
+		extraLabels := map[string]string{}
+		extraLabels["spilo-role"] = string(role)
+
+		poolerLabels = labels.Merge(poolerLabels, extraLabels)
+	}
 
 	return &metav1.LabelSelector{
-		MatchLabels:      connectionPoolerLabels,
+		MatchLabels:      poolerLabels,
 		MatchExpressions: nil,
 	}
 }
@@ -284,7 +284,7 @@ func (c *Cluster) generateConnectionPoolerPodTemplate(role PostgresRole) (
 
 	podTemplate := &v1.PodTemplateSpec{
 		ObjectMeta: metav1.ObjectMeta{
-			Labels:      c.connectionPoolerLabelsSelector(role).MatchLabels,
+			Labels:      c.connectionPoolerLabels(role, true).MatchLabels,
 			Namespace:   c.Namespace,
 			Annotations: c.generatePodAnnotations(spec),
 		},
@@ -338,7 +338,7 @@ func (c *Cluster) generateConnectionPoolerDeployment(connectionPooler *Connectio
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        connectionPooler.Name,
 			Namespace:   connectionPooler.Namespace,
-			Labels:      c.connectionPoolerLabelsSelector(connectionPooler.Role).MatchLabels,
+			Labels:      c.connectionPoolerLabels(connectionPooler.Role, true).MatchLabels,
 			Annotations: map[string]string{},
 			// make StatefulSet object its owner to represent the dependency.
 			// By itself StatefulSet is being deleted with "Orphaned"
@@ -350,7 +350,7 @@ func (c *Cluster) generateConnectionPoolerDeployment(connectionPooler *Connectio
 		},
 		Spec: appsv1.DeploymentSpec{
 			Replicas: numberOfInstances,
-			Selector: c.connectionPoolerLabelsSelector(connectionPooler.Role),
+			Selector: c.connectionPoolerLabels(connectionPooler.Role, false),
 			Template: *podTemplate,
 		},
 	}
@@ -389,7 +389,7 @@ func (c *Cluster) generateConnectionPoolerService(connectionPooler *ConnectionPo
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        connectionPooler.Name,
 			Namespace:   connectionPooler.Namespace,
-			Labels:      c.connectionPoolerLabelsSelector(connectionPooler.Role).MatchLabels,
+			Labels:      c.connectionPoolerLabels(connectionPooler.Role, false).MatchLabels,
 			Annotations: map[string]string{},
 			// make StatefulSet object its owner to represent the dependency.
 			// By itself StatefulSet is being deleted with "Orphaned"

pkg/cluster/connection_pooler_test.go

@@ -838,9 +838,9 @@ func testResources(cluster *Cluster, podSpec *v1.PodTemplateSpec, role PostgresR
 func testLabels(cluster *Cluster, podSpec *v1.PodTemplateSpec, role PostgresRole) error {
 	poolerLabels := podSpec.ObjectMeta.Labels["connection-pooler"]
 
-	if poolerLabels != cluster.connectionPoolerLabelsSelector(role).MatchLabels["connection-pooler"] {
+	if poolerLabels != cluster.connectionPoolerLabels(role, true).MatchLabels["connection-pooler"] {
 		return fmt.Errorf("Pod labels do not match, got %+v, expected %+v",
-			podSpec.ObjectMeta.Labels, cluster.connectionPoolerLabelsSelector(role).MatchLabels)
+			podSpec.ObjectMeta.Labels, cluster.connectionPoolerLabels(role, true).MatchLabels)
 	}
 
 	return nil
@@ -848,7 +848,7 @@ func testLabels(cluster *Cluster, podSpec *v1.PodTemplateSpec, role PostgresRole
 
 func testSelector(cluster *Cluster, deployment *appsv1.Deployment) error {
 	labels := deployment.Spec.Selector.MatchLabels
-	expected := cluster.connectionPoolerLabelsSelector(Master).MatchLabels
+	expected := cluster.connectionPoolerLabels(Master, true).MatchLabels
 
 	if labels["connection-pooler"] != expected["connection-pooler"] {
 		return fmt.Errorf("Labels are incorrect, got %+v, expected %+v",