From c4ae11629b2f84aba8cabd62afa28b09af1c6241 Mon Sep 17 00:00:00 2001 From: Jan Mussler Date: Mon, 23 Nov 2020 17:18:18 +0100 Subject: [PATCH] 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 | 3 +- e2e/tests/test_e2e.py | 34 ++++++++++++++++++ manifests/minimal-fake-pooler-deployment.yaml | 35 +++++++++++++++++++ manifests/postgres-operator.yaml | 2 ++ pkg/cluster/connection_pooler.go | 32 ++++++++--------- pkg/cluster/connection_pooler_test.go | 6 ++-- 6 files changed, 92 insertions(+), 20 deletions(-) create mode 100644 manifests/minimal-fake-pooler-deployment.yaml diff --git a/e2e/scripts/watch_objects.sh b/e2e/scripts/watch_objects.sh index 52364f247..4c9b82404 100755 --- a/e2e/scripts/watch_objects.sh +++ b/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' diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index f863123bd..95c7748cb 100644 --- a/e2e/tests/test_e2e.py +++ b/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): ''' diff --git a/manifests/minimal-fake-pooler-deployment.yaml b/manifests/minimal-fake-pooler-deployment.yaml new file mode 100644 index 000000000..0406b195a --- /dev/null +++ b/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: [] diff --git a/manifests/postgres-operator.yaml b/manifests/postgres-operator.yaml index 16719a5d9..ac319631b 100644 --- a/manifests/postgres-operator.yaml +++ b/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: diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index 0d9171b87..10354ca32 100644 --- a/pkg/cluster/connection_pooler.go +++ b/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" diff --git a/pkg/cluster/connection_pooler_test.go b/pkg/cluster/connection_pooler_test.go index b795fe14f..2528460f5 100644 --- a/pkg/cluster/connection_pooler_test.go +++ b/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",