From c4b1d69ef9743cd51b27d6c06ff9d1e57a12f033 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Fri, 27 Nov 2020 17:43:04 +0100 Subject: [PATCH 01/18] add comments where inherited annotations could be added --- pkg/cluster/connection_pooler.go | 20 ++++++++++++-------- pkg/cluster/k8sres.go | 19 +++++++++++++------ 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index 10354ca32..cd25fe3cb 100644 --- a/pkg/cluster/connection_pooler.go +++ b/pkg/cluster/connection_pooler.go @@ -284,8 +284,9 @@ func (c *Cluster) generateConnectionPoolerPodTemplate(role PostgresRole) ( podTemplate := &v1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: c.connectionPoolerLabels(role, true).MatchLabels, - Namespace: c.Namespace, + Labels: c.connectionPoolerLabels(role, true).MatchLabels, + Namespace: c.Namespace, + // Annotations: c.annotationsSet(c.generatePodAnnotations(spec)), Annotations: c.generatePodAnnotations(spec), }, Spec: v1.PodSpec{ @@ -336,9 +337,10 @@ func (c *Cluster) generateConnectionPoolerDeployment(connectionPooler *Connectio deployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ - Name: connectionPooler.Name, - Namespace: connectionPooler.Namespace, - Labels: c.connectionPoolerLabels(connectionPooler.Role, true).MatchLabels, + Name: connectionPooler.Name, + Namespace: connectionPooler.Namespace, + Labels: c.connectionPoolerLabels(connectionPooler.Role, true).MatchLabels, + // Annotations: c.annotationsSet(map[string]string{}), Annotations: map[string]string{}, // make StatefulSet object its owner to represent the dependency. // By itself StatefulSet is being deleted with "Orphaned" @@ -387,9 +389,11 @@ func (c *Cluster) generateConnectionPoolerService(connectionPooler *ConnectionPo service := &v1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: connectionPooler.Name, - Namespace: connectionPooler.Namespace, - Labels: c.connectionPoolerLabels(connectionPooler.Role, false).MatchLabels, + Name: connectionPooler.Name, + Namespace: connectionPooler.Namespace, + Labels: c.connectionPoolerLabels(connectionPooler.Role, false).MatchLabels, + // TODO add generateServiceAnnotations? + // TODO c.annotationsSet(map[string]string{}) 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/k8sres.go b/pkg/cluster/k8sres.go index 798e163dd..f518f0eb9 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1180,6 +1180,7 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef podTemplate, err = c.generatePodTemplate( c.Namespace, c.labelsSet(true), + // TODO c.annotationsSet(annotations), annotations, spiloContainer, initContainers, @@ -1231,9 +1232,10 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef statefulSet := &appsv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ - Name: c.statefulSetName(), - Namespace: c.Namespace, - Labels: c.labelsSet(true), + Name: c.statefulSetName(), + Namespace: c.Namespace, + Labels: c.labelsSet(true), + // TODO Annotations: c.annotationsSet(c.AnnotationsToPropagate(annotations)), Annotations: c.AnnotationsToPropagate(annotations), }, Spec: appsv1.StatefulSetSpec{ @@ -1530,6 +1532,7 @@ func (c *Cluster) generateSingleUserSecret(namespace string, pgUser spec.PgUser) Name: c.credentialSecretName(username), Namespace: namespace, Labels: c.labelsSet(true), + // TODO Annotations: c.annotationsSet(map[string]string{}), }, Type: v1.SecretTypeOpaque, Data: map[string][]byte{ @@ -1600,9 +1603,10 @@ func (c *Cluster) generateService(role PostgresRole, spec *acidv1.PostgresSpec) service := &v1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: c.serviceName(role), - Namespace: c.Namespace, - Labels: c.roleLabelsSet(true, role), + Name: c.serviceName(role), + Namespace: c.Namespace, + Labels: c.roleLabelsSet(true, role), + // TODO Annotations: c.annotationsSet(c.generateServiceAnnotations(role, spec)), Annotations: c.generateServiceAnnotations(role, spec), }, Spec: serviceSpec, @@ -1653,6 +1657,7 @@ func (c *Cluster) generateEndpoint(role PostgresRole, subsets []v1.EndpointSubse Name: c.endpointName(role), Namespace: c.Namespace, Labels: c.roleLabelsSet(true, role), + // TODO Annotations: c.annotationsSet(map[string]string{}), }, } if len(subsets) > 0 { @@ -1809,6 +1814,7 @@ func (c *Cluster) generatePodDisruptionBudget() *policybeta1.PodDisruptionBudget Name: c.podDisruptionBudgetName(), Namespace: c.Namespace, Labels: c.labelsSet(true), + // TODO Annotations: c.annotationsSet(map[string]string{}), }, Spec: policybeta1.PodDisruptionBudgetSpec{ MinAvailable: &minAvailable, @@ -1931,6 +1937,7 @@ func (c *Cluster) generateLogicalBackupJob() (*batchv1beta1.CronJob, error) { Name: c.getLogicalBackupJobName(), Namespace: c.Namespace, Labels: c.labelsSet(true), + // TODO Annotations: c.annotationsSet(map[string]string{}), }, Spec: batchv1beta1.CronJobSpec{ Schedule: schedule, From 63e7304a0e4cd44358ac11daf4277f04e5d01252 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Fri, 4 Dec 2020 15:13:55 +0100 Subject: [PATCH 02/18] add inheritedAnnotations feature --- .../crds/operatorconfigurations.yaml | 4 ++ charts/postgres-operator/values-crd.yaml | 6 ++- charts/postgres-operator/values.yaml | 5 +- docs/reference/operator_parameters.md | 21 +++++--- e2e/tests/test_e2e.py | 3 ++ manifests/configmap.yaml | 1 + manifests/operatorconfiguration.crd.yaml | 4 ++ ...gresql-operator-default-configuration.yaml | 2 + pkg/apis/acid.zalan.do/v1/crds.go | 10 +++- .../v1/operator_configuration_type.go | 3 +- .../acid.zalan.do/v1/zz_generated.deepcopy.go | 15 ++++-- pkg/cluster/connection_pooler.go | 26 ++++----- pkg/cluster/k8sres.go | 53 +++++++++---------- pkg/cluster/util.go | 21 ++++++++ pkg/controller/operator_config.go | 1 + pkg/util/config/config.go | 1 + 16 files changed, 117 insertions(+), 59 deletions(-) diff --git a/charts/postgres-operator/crds/operatorconfigurations.yaml b/charts/postgres-operator/crds/operatorconfigurations.yaml index 4f85d1642..e784cd3ac 100644 --- a/charts/postgres-operator/crds/operatorconfigurations.yaml +++ b/charts/postgres-operator/crds/operatorconfigurations.yaml @@ -166,6 +166,10 @@ spec: type: string template: type: boolean + inherited_annotations: + type: array + items: + type: string inherited_labels: type: array items: diff --git a/charts/postgres-operator/values-crd.yaml b/charts/postgres-operator/values-crd.yaml index 71c2d5bb1..00d82e0ef 100644 --- a/charts/postgres-operator/values-crd.yaml +++ b/charts/postgres-operator/values-crd.yaml @@ -89,7 +89,11 @@ configKubernetes: # namespaced name of the secret containing infrastructure roles names and passwords # infrastructure_roles_secret_name: postgresql-infrastructure-roles - # list of labels that can be inherited from the cluster manifest + # list of annotation keys that can be inherited from the cluster manifest + # inherited_labels: + # - owned-by + + # list of label keys that can be inherited from the cluster manifest # inherited_labels: # - application # - environment diff --git a/charts/postgres-operator/values.yaml b/charts/postgres-operator/values.yaml index 95865503d..6ffd26b59 100644 --- a/charts/postgres-operator/values.yaml +++ b/charts/postgres-operator/values.yaml @@ -86,7 +86,10 @@ configKubernetes: # namespaced name of the secret containing infrastructure roles names and passwords # infrastructure_roles_secret_name: postgresql-infrastructure-roles - # list of labels that can be inherited from the cluster manifest + # list of annotation keys that can be inherited from the cluster manifest + # inherited_annotations: owned-by + + # list of label keys that can be inherited from the cluster manifest # inherited_labels: application,environment # timeout for successful migration of master pods from unschedulable node diff --git a/docs/reference/operator_parameters.md b/docs/reference/operator_parameters.md index bd8c80d9c..9d4e5fc58 100644 --- a/docs/reference/operator_parameters.md +++ b/docs/reference/operator_parameters.md @@ -271,6 +271,12 @@ configuration they are grouped under the `kubernetes` key. are extracted. For the ConfigMap this has to be a string which allows referencing only one infrastructure roles secret. The default is empty. +* **inherited_annotations** + list of annotation keys that can be inherited from the cluster manifest, and + added to each child objects (`Deployment`, `StatefulSet`, `Pod`, `PVCs`, + `PDB`, `Service`, `Endpoints` and `Secrets`) created by the operator. + The default is empty. + * **pod_role_label** name of the label assigned to the Postgres pods (and services/endpoints) by the operator. The default is `spilo-role`. @@ -280,15 +286,16 @@ configuration they are grouped under the `kubernetes` key. objects. The default is `application:spilo`. * **inherited_labels** - list of labels that can be inherited from the cluster manifest, and added to - each child objects (`StatefulSet`, `Pod`, `Service` and `Endpoints`) created - by the operator. Typical use case is to dynamically pass labels that are - specific to a given Postgres cluster, in order to implement `NetworkPolicy`. - The default is empty. + list of label keys that can be inherited from the cluster manifest, and + added to each child objects (`Deployment`, `StatefulSet`, `Pod`, `PVCs`, + `PDB`, `Service`, `Endpoints` and `Secrets`) created by the operator. + Typical use case is to dynamically pass labels that are specific to a + given Postgres cluster, in order to implement `NetworkPolicy`. The default + is empty. * **cluster_name_label** - name of the label assigned to Kubernetes objects created by the operator that - indicates which cluster a given object belongs to. The default is + name of the label assigned to Kubernetes objects created by the operator + that indicates which cluster a given object belongs to. The default is `cluster-name`. * **node_readiness_label** diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index aac056ed4..525d523a6 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -852,6 +852,7 @@ def test_statefulset_annotation_propagation(self): patch_sset_propagate_annotations = { "data": { "downscaler_annotations": "deployment-time,downscaler/*", + "inherited_annotations": "owned-by", } } k8s.update_config(patch_sset_propagate_annotations) @@ -861,6 +862,7 @@ def test_statefulset_annotation_propagation(self): "annotations": { "deployment-time": "2020-04-30 12:00:00", "downscaler/downtime_replicas": "0", + "owned-by": "acid" }, } } @@ -870,6 +872,7 @@ def test_statefulset_annotation_propagation(self): annotations = { "deployment-time": "2020-04-30 12:00:00", "downscaler/downtime_replicas": "0", + "owned-by": "acid", } self.eventuallyTrue(lambda: k8s.check_statefulset_annotations(cluster_label, annotations), "Annotations missing") diff --git a/manifests/configmap.yaml b/manifests/configmap.yaml index 59283fd6e..004e4ea46 100644 --- a/manifests/configmap.yaml +++ b/manifests/configmap.yaml @@ -54,6 +54,7 @@ data: # kubernetes_use_configmaps: "false" # infrastructure_roles_secret_name: "postgresql-infrastructure-roles" # infrastructure_roles_secrets: "secretname:monitoring-roles,userkey:user,passwordkey:password,rolekey:inrole" + # inherited_annotations: owned-by # inherited_labels: application,environment # kube_iam_role: "" # log_s3_bucket: "" diff --git a/manifests/operatorconfiguration.crd.yaml b/manifests/operatorconfiguration.crd.yaml index f529d3353..7348a24e0 100644 --- a/manifests/operatorconfiguration.crd.yaml +++ b/manifests/operatorconfiguration.crd.yaml @@ -162,6 +162,10 @@ spec: type: string template: type: boolean + inherited_annotations: + type: array + items: + type: string inherited_labels: type: array items: diff --git a/manifests/postgresql-operator-default-configuration.yaml b/manifests/postgresql-operator-default-configuration.yaml index 84537e06a..879992b4e 100644 --- a/manifests/postgresql-operator-default-configuration.yaml +++ b/manifests/postgresql-operator-default-configuration.yaml @@ -49,6 +49,8 @@ configuration: # - secretname: "other-infrastructure-role" # userkey: "other-user-key" # passwordkey: "other-password-key" + # inherited_annotations: + # - owned-by # inherited_labels: # - application # - environment diff --git a/pkg/apis/acid.zalan.do/v1/crds.go b/pkg/apis/acid.zalan.do/v1/crds.go index 63d486dad..c6e4657b4 100644 --- a/pkg/apis/acid.zalan.do/v1/crds.go +++ b/pkg/apis/acid.zalan.do/v1/crds.go @@ -963,6 +963,14 @@ var OperatorConfigCRDResourceValidation = apiextv1.CustomResourceValidation{ }, }, }, + "inherited_annotations": { + Type: "array", + Items: &apiextv1.JSONSchemaPropsOrArray{ + Schema: &apiextv1.JSONSchemaProps{ + Type: "string", + }, + }, + }, "inherited_labels": { Type: "array", Items: &apiextv1.JSONSchemaPropsOrArray{ @@ -1400,7 +1408,7 @@ func buildCRD(name, kind, plural, short string, columns []apiextv1.CustomResourc }, Scope: apiextv1.NamespaceScoped, Versions: []apiextv1.CustomResourceDefinitionVersion{ - apiextv1.CustomResourceDefinitionVersion{ + { Name: SchemeGroupVersion.Version, Served: true, Storage: true, diff --git a/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go b/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go index a9abcf0ee..adc3fba49 100644 --- a/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go +++ b/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go @@ -66,6 +66,7 @@ type KubernetesMetaConfiguration struct { PodRoleLabel string `json:"pod_role_label,omitempty"` ClusterLabels map[string]string `json:"cluster_labels,omitempty"` InheritedLabels []string `json:"inherited_labels,omitempty"` + InheritedAnnotations []string `json:"inherited_annotations,omitempty"` DownscalerAnnotations []string `json:"downscaler_annotations,omitempty"` ClusterNameLabel string `json:"cluster_name_label,omitempty"` DeleteAnnotationDateKey string `json:"delete_annotation_date_key,omitempty"` @@ -167,7 +168,7 @@ type ScalyrConfiguration struct { ScalyrMemoryLimit string `json:"scalyr_memory_limit,omitempty"` } -// Defines default configuration for connection pooler +// ConnectionPoolerConfiguration defines default configuration for connection pooler type ConnectionPoolerConfiguration struct { NumberOfInstances *int32 `json:"connection_pooler_number_of_instances,omitempty"` Schema string `json:"connection_pooler_schema,omitempty"` diff --git a/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go b/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go index de260dc53..c4d1fc9b7 100644 --- a/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go +++ b/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go @@ -202,6 +202,11 @@ func (in *KubernetesMetaConfiguration) DeepCopyInto(out *KubernetesMetaConfigura *out = make([]string, len(*in)) copy(*out, *in) } + if in.InheritedAnnotations != nil { + in, out := &in.InheritedAnnotations, &out.InheritedAnnotations + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.DownscalerAnnotations != nil { in, out := &in.DownscalerAnnotations, &out.DownscalerAnnotations *out = make([]string, len(*in)) @@ -623,6 +628,11 @@ func (in *PostgresSpec) DeepCopyInto(out *PostgresSpec) { (*out)[key] = *val.DeepCopy() } } + if in.SchedulerName != nil { + in, out := &in.SchedulerName, &out.SchedulerName + *out = new(string) + **out = **in + } if in.Tolerations != nil { in, out := &in.Tolerations, &out.Tolerations *out = make([]corev1.Toleration, len(*in)) @@ -687,11 +697,6 @@ func (in *PostgresSpec) DeepCopyInto(out *PostgresSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.SchedulerName != nil { - in, out := &in.SchedulerName, &out.SchedulerName - *out = new(string) - **out = **in - } return } diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index cd25fe3cb..7f75ccea1 100644 --- a/pkg/cluster/connection_pooler.go +++ b/pkg/cluster/connection_pooler.go @@ -284,10 +284,9 @@ func (c *Cluster) generateConnectionPoolerPodTemplate(role PostgresRole) ( podTemplate := &v1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: c.connectionPoolerLabels(role, true).MatchLabels, - Namespace: c.Namespace, - // Annotations: c.annotationsSet(c.generatePodAnnotations(spec)), - Annotations: c.generatePodAnnotations(spec), + Labels: c.connectionPoolerLabels(role, true).MatchLabels, + Namespace: c.Namespace, + Annotations: c.annotationsSet(c.generatePodAnnotations(spec)), }, Spec: v1.PodSpec{ ServiceAccountName: c.OpConfig.PodServiceAccountName, @@ -337,11 +336,10 @@ func (c *Cluster) generateConnectionPoolerDeployment(connectionPooler *Connectio deployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ - Name: connectionPooler.Name, - Namespace: connectionPooler.Namespace, - Labels: c.connectionPoolerLabels(connectionPooler.Role, true).MatchLabels, - // Annotations: c.annotationsSet(map[string]string{}), - Annotations: map[string]string{}, + Name: connectionPooler.Name, + Namespace: connectionPooler.Namespace, + Labels: c.connectionPoolerLabels(connectionPooler.Role, true).MatchLabels, + Annotations: c.annotationsSet(map[string]string{}), // make StatefulSet object its owner to represent the dependency. // By itself StatefulSet is being deleted with "Orphaned" // propagation policy, which means that it's deletion will not @@ -389,12 +387,10 @@ func (c *Cluster) generateConnectionPoolerService(connectionPooler *ConnectionPo service := &v1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: connectionPooler.Name, - Namespace: connectionPooler.Namespace, - Labels: c.connectionPoolerLabels(connectionPooler.Role, false).MatchLabels, - // TODO add generateServiceAnnotations? - // TODO c.annotationsSet(map[string]string{}) - Annotations: map[string]string{}, + Name: connectionPooler.Name, + Namespace: connectionPooler.Namespace, + Labels: c.connectionPoolerLabels(connectionPooler.Role, false).MatchLabels, + Annotations: c.annotationsSet(c.generateServiceAnnotations(connectionPooler.Role, spec)), // make StatefulSet object its owner to represent the dependency. // By itself StatefulSet is being deleted with "Orphaned" // propagation policy, which means that it's deletion will not diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index f518f0eb9..be3a38d03 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1180,8 +1180,7 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef podTemplate, err = c.generatePodTemplate( c.Namespace, c.labelsSet(true), - // TODO c.annotationsSet(annotations), - annotations, + c.annotationsSet(annotations), spiloContainer, initContainers, sidecarContainers, @@ -1232,11 +1231,10 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef statefulSet := &appsv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ - Name: c.statefulSetName(), - Namespace: c.Namespace, - Labels: c.labelsSet(true), - // TODO Annotations: c.annotationsSet(c.AnnotationsToPropagate(annotations)), - Annotations: c.AnnotationsToPropagate(annotations), + Name: c.statefulSetName(), + Namespace: c.Namespace, + Labels: c.labelsSet(true), + Annotations: c.annotationsSet(c.AnnotationsToPropagate(annotations)), }, Spec: appsv1.StatefulSetSpec{ Replicas: &numberOfInstances, @@ -1529,10 +1527,10 @@ func (c *Cluster) generateSingleUserSecret(namespace string, pgUser spec.PgUser) username := pgUser.Name secret := v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: c.credentialSecretName(username), - Namespace: namespace, - Labels: c.labelsSet(true), - // TODO Annotations: c.annotationsSet(map[string]string{}), + Name: c.credentialSecretName(username), + Namespace: namespace, + Labels: c.labelsSet(true), + Annotations: c.annotationsSet(map[string]string{}), }, Type: v1.SecretTypeOpaque, Data: map[string][]byte{ @@ -1603,11 +1601,10 @@ func (c *Cluster) generateService(role PostgresRole, spec *acidv1.PostgresSpec) service := &v1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: c.serviceName(role), - Namespace: c.Namespace, - Labels: c.roleLabelsSet(true, role), - // TODO Annotations: c.annotationsSet(c.generateServiceAnnotations(role, spec)), - Annotations: c.generateServiceAnnotations(role, spec), + Name: c.serviceName(role), + Namespace: c.Namespace, + Labels: c.roleLabelsSet(true, role), + Annotations: c.annotationsSet(c.generateServiceAnnotations(role, spec)), }, Spec: serviceSpec, } @@ -1654,10 +1651,10 @@ func (c *Cluster) generateServiceAnnotations(role PostgresRole, spec *acidv1.Pos func (c *Cluster) generateEndpoint(role PostgresRole, subsets []v1.EndpointSubset) *v1.Endpoints { endpoints := &v1.Endpoints{ ObjectMeta: metav1.ObjectMeta{ - Name: c.endpointName(role), - Namespace: c.Namespace, - Labels: c.roleLabelsSet(true, role), - // TODO Annotations: c.annotationsSet(map[string]string{}), + Name: c.endpointName(role), + Namespace: c.Namespace, + Labels: c.roleLabelsSet(true, role), + Annotations: c.annotationsSet(map[string]string{}), }, } if len(subsets) > 0 { @@ -1811,10 +1808,10 @@ func (c *Cluster) generatePodDisruptionBudget() *policybeta1.PodDisruptionBudget return &policybeta1.PodDisruptionBudget{ ObjectMeta: metav1.ObjectMeta{ - Name: c.podDisruptionBudgetName(), - Namespace: c.Namespace, - Labels: c.labelsSet(true), - // TODO Annotations: c.annotationsSet(map[string]string{}), + Name: c.podDisruptionBudgetName(), + Namespace: c.Namespace, + Labels: c.labelsSet(true), + Annotations: c.annotationsSet(map[string]string{}), }, Spec: policybeta1.PodDisruptionBudgetSpec{ MinAvailable: &minAvailable, @@ -1934,10 +1931,10 @@ func (c *Cluster) generateLogicalBackupJob() (*batchv1beta1.CronJob, error) { cronJob := &batchv1beta1.CronJob{ ObjectMeta: metav1.ObjectMeta{ - Name: c.getLogicalBackupJobName(), - Namespace: c.Namespace, - Labels: c.labelsSet(true), - // TODO Annotations: c.annotationsSet(map[string]string{}), + Name: c.getLogicalBackupJobName(), + Namespace: c.Namespace, + Labels: c.labelsSet(true), + Annotations: c.annotationsSet(map[string]string{}), }, Spec: batchv1beta1.CronJobSpec{ Schedule: schedule, diff --git a/pkg/cluster/util.go b/pkg/cluster/util.go index a2fdcb08e..0f17af88f 100644 --- a/pkg/cluster/util.go +++ b/pkg/cluster/util.go @@ -271,6 +271,27 @@ func (c *Cluster) getTeamMembers(teamID string) ([]string, error) { return members, nil } +// Returns annotations used to create or list k8s objects such as pods +// For backward compatibility, shouldAddExtraLabels must be false +// when listing k8s objects. See operator PR #252 +func (c *Cluster) annotationsSet(annotations map[string]string) map[string]string { + + // allow to inherit certain labels from the 'postgres' object + if spec, err := c.GetSpec(); err == nil { + for k, v := range spec.ObjectMeta.Annotations { + for _, match := range c.OpConfig.InheritedAnnotations { + if k == match { + annotations[k] = v + } + } + } + } else { + c.logger.Warningf("could not get the list of InheritedAnnoations for cluster %q: %v", c.Name, err) + } + + return annotations +} + func (c *Cluster) waitForPodLabel(podEvents chan PodEvent, stopChan chan struct{}, role *PostgresRole) (*v1.Pod, error) { timeout := time.After(c.OpConfig.PodLabelWaitTimeout) for { diff --git a/pkg/controller/operator_config.go b/pkg/controller/operator_config.go index 9b2713da8..002479f47 100644 --- a/pkg/controller/operator_config.go +++ b/pkg/controller/operator_config.go @@ -92,6 +92,7 @@ func (c *Controller) importConfigurationFromCRD(fromCRD *acidv1.OperatorConfigur result.PodRoleLabel = util.Coalesce(fromCRD.Kubernetes.PodRoleLabel, "spilo-role") result.ClusterLabels = util.CoalesceStrMap(fromCRD.Kubernetes.ClusterLabels, map[string]string{"application": "spilo"}) result.InheritedLabels = fromCRD.Kubernetes.InheritedLabels + result.InheritedAnnotations = fromCRD.Kubernetes.InheritedAnnotations result.DownscalerAnnotations = fromCRD.Kubernetes.DownscalerAnnotations result.ClusterNameLabel = util.Coalesce(fromCRD.Kubernetes.ClusterNameLabel, "cluster-name") result.DeleteAnnotationDateKey = fromCRD.Kubernetes.DeleteAnnotationDateKey diff --git a/pkg/util/config/config.go b/pkg/util/config/config.go index 47a120227..8ad835181 100644 --- a/pkg/util/config/config.go +++ b/pkg/util/config/config.go @@ -36,6 +36,7 @@ type Resources struct { SpiloPrivileged bool `name:"spilo_privileged" default:"false"` ClusterLabels map[string]string `name:"cluster_labels" default:"application:spilo"` InheritedLabels []string `name:"inherited_labels" default:""` + InheritedAnnotations []string `name:"inherited_annotations" default:""` DownscalerAnnotations []string `name:"downscaler_annotations"` ClusterNameLabel string `name:"cluster_name_label" default:"cluster-name"` DeleteAnnotationDateKey string `name:"delete_annotation_date_key"` From 79f00ea8f328db6a6f69f1e02fbfc81c557471d9 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Fri, 4 Dec 2020 15:29:51 +0100 Subject: [PATCH 03/18] return nil if no annotations are set --- pkg/cluster/util.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/util.go b/pkg/cluster/util.go index 0f17af88f..8004af175 100644 --- a/pkg/cluster/util.go +++ b/pkg/cluster/util.go @@ -276,6 +276,8 @@ func (c *Cluster) getTeamMembers(teamID string) ([]string, error) { // when listing k8s objects. See operator PR #252 func (c *Cluster) annotationsSet(annotations map[string]string) map[string]string { + var result map[string]string + // allow to inherit certain labels from the 'postgres' object if spec, err := c.GetSpec(); err == nil { for k, v := range spec.ObjectMeta.Annotations { @@ -289,7 +291,11 @@ func (c *Cluster) annotationsSet(annotations map[string]string) map[string]strin c.logger.Warningf("could not get the list of InheritedAnnoations for cluster %q: %v", c.Name, err) } - return annotations + if len(annotations) > 0 { + result = annotations + } + + return result } func (c *Cluster) waitForPodLabel(podEvents chan PodEvent, stopChan chan struct{}, role *PostgresRole) (*v1.Pod, error) { From 94eaef478df7ffd45756b7250fc858414d5f87a4 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Fri, 4 Dec 2020 16:09:52 +0100 Subject: [PATCH 04/18] minor changes --- charts/postgres-operator/values-crd.yaml | 2 +- pkg/cluster/util.go | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/charts/postgres-operator/values-crd.yaml b/charts/postgres-operator/values-crd.yaml index 00d82e0ef..211d46dbe 100644 --- a/charts/postgres-operator/values-crd.yaml +++ b/charts/postgres-operator/values-crd.yaml @@ -90,7 +90,7 @@ configKubernetes: # infrastructure_roles_secret_name: postgresql-infrastructure-roles # list of annotation keys that can be inherited from the cluster manifest - # inherited_labels: + # inherited_annotations: # - owned-by # list of label keys that can be inherited from the cluster manifest diff --git a/pkg/cluster/util.go b/pkg/cluster/util.go index 8004af175..670ffe290 100644 --- a/pkg/cluster/util.go +++ b/pkg/cluster/util.go @@ -271,13 +271,9 @@ func (c *Cluster) getTeamMembers(teamID string) ([]string, error) { return members, nil } -// Returns annotations used to create or list k8s objects such as pods -// For backward compatibility, shouldAddExtraLabels must be false -// when listing k8s objects. See operator PR #252 +// Returns annotations to be passed to child objects func (c *Cluster) annotationsSet(annotations map[string]string) map[string]string { - var result map[string]string - // allow to inherit certain labels from the 'postgres' object if spec, err := c.GetSpec(); err == nil { for k, v := range spec.ObjectMeta.Annotations { @@ -292,10 +288,10 @@ func (c *Cluster) annotationsSet(annotations map[string]string) map[string]strin } if len(annotations) > 0 { - result = annotations + return annotations } - return result + return nil } func (c *Cluster) waitForPodLabel(podEvents chan PodEvent, stopChan chan struct{}, role *PostgresRole) (*v1.Pod, error) { From 621c81fc52c33f09daaca3cadba170804e1c80c6 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Fri, 4 Dec 2020 18:47:32 +0100 Subject: [PATCH 05/18] first downscaler then inherited annotations --- pkg/cluster/k8sres.go | 10 +++++----- pkg/cluster/sync.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index be3a38d03..5d0e54ba9 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1174,13 +1174,13 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef tolerationSpec := tolerations(&spec.Tolerations, c.OpConfig.PodToleration) effectivePodPriorityClassName := util.Coalesce(spec.PodPriorityClassName, c.OpConfig.PodPriorityClassName) - annotations := c.generatePodAnnotations(spec) + podAnnotations := c.generatePodAnnotations(spec) // generate pod template for the statefulset, based on the spilo container and sidecars podTemplate, err = c.generatePodTemplate( c.Namespace, c.labelsSet(true), - c.annotationsSet(annotations), + c.annotationsSet(podAnnotations), spiloContainer, initContainers, sidecarContainers, @@ -1226,15 +1226,15 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef return nil, fmt.Errorf("could not set the pod management policy to the unknown value: %v", c.OpConfig.PodManagementPolicy) } - annotations = make(map[string]string) - annotations[rollingUpdateStatefulsetAnnotationKey] = strconv.FormatBool(false) + stsAnnotations := c.AnnotationsToPropagate(map[string]string{}) + stsAnnotations[rollingUpdateStatefulsetAnnotationKey] = strconv.FormatBool(false) statefulSet := &appsv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ Name: c.statefulSetName(), Namespace: c.Namespace, Labels: c.labelsSet(true), - Annotations: c.annotationsSet(c.AnnotationsToPropagate(annotations)), + Annotations: c.annotationsSet(stsAnnotations), }, Spec: appsv1.StatefulSetSpec{ Replicas: &numberOfInstances, diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index e91adf757..d6cba7be8 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -354,7 +354,7 @@ func (c *Cluster) syncStatefulSet() error { } } annotations := c.AnnotationsToPropagate(c.Statefulset.Annotations) - c.updateStatefulSetAnnotations(annotations) + c.updateStatefulSetAnnotations(c.annotationsSet(annotations)) if !podsRollingUpdateRequired && !c.OpConfig.EnableLazySpiloUpgrade { // even if desired and actual statefulsets match From 1ac2decdea2b4220ce28de304af27ecb0aa49586 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Mon, 7 Dec 2020 17:23:49 +0100 Subject: [PATCH 06/18] add unit test for inherited annotations --- e2e/tests/k8s_api.py | 4 +- e2e/tests/test_e2e.py | 2 - pkg/cluster/util_test.go | 132 ++++++++++++++++++++++++++++++++++++ pkg/cluster/volumes_test.go | 4 +- 4 files changed, 136 insertions(+), 6 deletions(-) create mode 100644 pkg/cluster/util_test.go diff --git a/e2e/tests/k8s_api.py b/e2e/tests/k8s_api.py index 30165e6a0..95e1dc9ad 100644 --- a/e2e/tests/k8s_api.py +++ b/e2e/tests/k8s_api.py @@ -117,7 +117,7 @@ def check_service_annotations(self, svc_labels, annotations, namespace='default' for svc in svcs: for key, value in annotations.items(): if not svc.metadata.annotations or key not in svc.metadata.annotations or svc.metadata.annotations[key] != value: - print("Expected key {} not found in annotations {}".format(key, svc.metadata.annotations)) + print("Expected key {} not found in service annotations {}".format(key, svc.metadata.annotations)) return False return True @@ -126,7 +126,7 @@ def check_statefulset_annotations(self, sset_labels, annotations, namespace='def for sset in ssets: for key, value in annotations.items(): if key not in sset.metadata.annotations or sset.metadata.annotations[key] != value: - print("Expected key {} not found in annotations {}".format(key, sset.metadata.annotations)) + print("Expected key {} not found in statefulset annotations {}".format(key, sset.metadata.annotations)) return False return True diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index 525d523a6..b8618a2dd 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -877,8 +877,6 @@ def test_statefulset_annotation_propagation(self): self.eventuallyTrue(lambda: k8s.check_statefulset_annotations(cluster_label, annotations), "Annotations missing") - self.eventuallyTrue(lambda: k8s.check_statefulset_annotations(cluster_label, annotations), "Annotations missing") - @timeout_decorator.timeout(TEST_TIMEOUT_SEC) @unittest.skip("Skipping this test until fixed") def test_zzz_taint_based_eviction(self): diff --git a/pkg/cluster/util_test.go b/pkg/cluster/util_test.go new file mode 100644 index 000000000..51d433ff6 --- /dev/null +++ b/pkg/cluster/util_test.go @@ -0,0 +1,132 @@ +package cluster + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1" + "github.com/zalando/postgres-operator/pkg/util" + "github.com/zalando/postgres-operator/pkg/util/config" + "github.com/zalando/postgres-operator/pkg/util/k8sutil" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" +) + +func newFakeK8sAnnotationsClient() (k8sutil.KubernetesClient, *fake.Clientset) { + clientSet := fake.NewSimpleClientset() + + return k8sutil.KubernetesClient{ + DeploymentsGetter: clientSet.AppsV1(), + EndpointsGetter: clientSet.CoreV1(), + PodsGetter: clientSet.CoreV1(), + PodDisruptionBudgetsGetter: clientSet.PolicyV1beta1(), + SecretsGetter: clientSet.CoreV1(), + ServicesGetter: clientSet.CoreV1(), + StatefulSetsGetter: clientSet.AppsV1(), + }, clientSet +} + +func TestInheritedAnnotations(t *testing.T) { + testName := "test inheriting annotations from manifest" + client, _ := newFakeK8sAnnotationsClient() + clusterName := "acid-test-cluster" + namespace := "default" + + annotationKey := "acid" + pg := acidv1.Postgresql{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Annotations: map[string]string{ + "owned-by": annotationKey, + }, + }, + Spec: acidv1.PostgresSpec{ + EnableReplicaConnectionPooler: boolToPointer(true), + }, + } + + var cluster = New( + Config{ + OpConfig: config.Config{ + Resources: config.Resources{ + ClusterLabels: map[string]string{"application": "spilo"}, + ClusterNameLabel: "cluster-name", + InheritedAnnotations: []string{"owned-by"}, + }, + }, + }, client, pg, logger, eventRecorder) + + cluster.Name = clusterName + cluster.Namespace = namespace + + // test annotationsSet function + inheritedAnnotations := cluster.annotationsSet(map[string]string{}) + + listOptions := metav1.ListOptions{ + LabelSelector: cluster.labelsSet(false).String(), + } + + // check pooler deployment annotations + deployList, err := cluster.KubeClient.Deployments(namespace).List(context.TODO(), listOptions) + assert.NoError(t, err) + for _, deploy := range deployList.Items { + if !(util.MapContains(deploy.ObjectMeta.Annotations, inheritedAnnotations)) { + t.Errorf("%s: Deployment %v not inherited annotations %#v, got %#v", testName, deploy.ObjectMeta.Name, inheritedAnnotations, deploy.ObjectMeta.Annotations) + } + } + + // check statefulset annotations + stsList, err := cluster.KubeClient.StatefulSets(namespace).List(context.TODO(), listOptions) + assert.NoError(t, err) + for _, sts := range stsList.Items { + if !(util.MapContains(sts.ObjectMeta.Annotations, inheritedAnnotations)) { + t.Errorf("%s: StatefulSet %v not inherited annotations %#v, got %#v", testName, sts.ObjectMeta.Name, inheritedAnnotations, sts.ObjectMeta.Annotations) + } + } + + // check pod annotations + podList, err := cluster.KubeClient.Pods(namespace).List(context.TODO(), listOptions) + assert.NoError(t, err) + for _, pod := range podList.Items { + if !(util.MapContains(pod.ObjectMeta.Annotations, inheritedAnnotations)) { + t.Errorf("%s: Pod %v not inherited annotations %#v, got %#v", testName, pod.ObjectMeta.Name, inheritedAnnotations, pod.ObjectMeta.Annotations) + } + } + + // check service annotations + svcList, err := cluster.KubeClient.Services(namespace).List(context.TODO(), listOptions) + assert.NoError(t, err) + for _, svc := range svcList.Items { + if !(util.MapContains(svc.ObjectMeta.Annotations, inheritedAnnotations)) { + t.Errorf("%s: Service %v not inherited annotations %#v, got %#v", testName, svc.ObjectMeta.Name, inheritedAnnotations, svc.ObjectMeta.Annotations) + } + } + + // check endpoint annotations + epList, err := cluster.KubeClient.Endpoints(namespace).List(context.TODO(), listOptions) + assert.NoError(t, err) + for _, ep := range epList.Items { + if !(util.MapContains(ep.ObjectMeta.Annotations, inheritedAnnotations)) { + t.Errorf("%s: Endpoint %v not inherited annotations %#v, got %#v", testName, ep.ObjectMeta.Name, inheritedAnnotations, ep.ObjectMeta.Annotations) + } + } + + // check pod disruption budget annotations + pdbList, err := cluster.KubeClient.PodDisruptionBudgets(namespace).List(context.TODO(), listOptions) + assert.NoError(t, err) + for _, pdb := range pdbList.Items { + if !(util.MapContains(pdb.ObjectMeta.Annotations, inheritedAnnotations)) { + t.Errorf("%s: Pod Disruption Budget %v not inherited annotations %#v, got %#v", testName, pdb.ObjectMeta.Name, inheritedAnnotations, pdb.ObjectMeta.Annotations) + } + } + + // check secret annotations + secretList, err := cluster.KubeClient.Secrets(namespace).List(context.TODO(), listOptions) + assert.NoError(t, err) + for _, secret := range secretList.Items { + if !(util.MapContains(secret.ObjectMeta.Annotations, inheritedAnnotations)) { + t.Errorf("%s: Secret %v not inherited annotations %#v, got %#v", testName, secret.ObjectMeta.Name, inheritedAnnotations, secret.ObjectMeta.Annotations) + } + } +} diff --git a/pkg/cluster/volumes_test.go b/pkg/cluster/volumes_test.go index 49fbbd228..2b9882d49 100644 --- a/pkg/cluster/volumes_test.go +++ b/pkg/cluster/volumes_test.go @@ -18,7 +18,7 @@ import ( "k8s.io/client-go/kubernetes/fake" ) -func NewFakeKubernetesClient() (k8sutil.KubernetesClient, *fake.Clientset) { +func newFakeK8sPVCclient() (k8sutil.KubernetesClient, *fake.Clientset) { clientSet := fake.NewSimpleClientset() return k8sutil.KubernetesClient{ @@ -28,7 +28,7 @@ func NewFakeKubernetesClient() (k8sutil.KubernetesClient, *fake.Clientset) { func TestResizeVolumeClaim(t *testing.T) { testName := "test resizing of persistent volume claims" - client, _ := NewFakeKubernetesClient() + client, _ := newFakeK8sPVCclient() clusterName := "acid-test-cluster" namespace := "default" newVolumeSize := "2Gi" From ce8a0d6f5bd805dcfe8845cd43e965e421087b69 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Mon, 7 Dec 2020 18:34:00 +0100 Subject: [PATCH 07/18] add pvc to test + minor changes --- pkg/cluster/connection_pooler.go | 3 ++- pkg/cluster/sync.go | 5 +++-- pkg/cluster/util_test.go | 28 +++++++++++++++++++--------- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index 7f75ccea1..1c1ffaff9 100644 --- a/pkg/cluster/connection_pooler.go +++ b/pkg/cluster/connection_pooler.go @@ -866,7 +866,8 @@ func (c *Cluster) syncConnectionPoolerWorker(oldSpec, newSpec *acidv1.Postgresql } } - newAnnotations := c.AnnotationsToPropagate(c.ConnectionPooler[role].Deployment.Annotations) + newAnnotations := c.annotationsSet(c.ConnectionPooler[role].Deployment.Annotations) + newAnnotations = c.AnnotationsToPropagate(newAnnotations) if newAnnotations != nil { deployment, err = updateConnectionPoolerAnnotations(c.KubeClient, c.ConnectionPooler[role].Deployment, newAnnotations) if err != nil { diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index d6cba7be8..6404f554b 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -353,8 +353,9 @@ func (c *Cluster) syncStatefulSet() error { } } } - annotations := c.AnnotationsToPropagate(c.Statefulset.Annotations) - c.updateStatefulSetAnnotations(c.annotationsSet(annotations)) + annotations := c.annotationsSet(c.Statefulset.Annotations) + annotations = c.AnnotationsToPropagate(annotations) + c.updateStatefulSetAnnotations(annotations) if !podsRollingUpdateRequired && !c.OpConfig.EnableLazySpiloUpgrade { // even if desired and actual statefulsets match diff --git a/pkg/cluster/util_test.go b/pkg/cluster/util_test.go index 51d433ff6..637a442ba 100644 --- a/pkg/cluster/util_test.go +++ b/pkg/cluster/util_test.go @@ -17,13 +17,14 @@ func newFakeK8sAnnotationsClient() (k8sutil.KubernetesClient, *fake.Clientset) { clientSet := fake.NewSimpleClientset() return k8sutil.KubernetesClient{ - DeploymentsGetter: clientSet.AppsV1(), - EndpointsGetter: clientSet.CoreV1(), - PodsGetter: clientSet.CoreV1(), - PodDisruptionBudgetsGetter: clientSet.PolicyV1beta1(), - SecretsGetter: clientSet.CoreV1(), - ServicesGetter: clientSet.CoreV1(), - StatefulSetsGetter: clientSet.AppsV1(), + DeploymentsGetter: clientSet.AppsV1(), + EndpointsGetter: clientSet.CoreV1(), + PersistentVolumeClaimsGetter: clientSet.CoreV1(), + PodsGetter: clientSet.CoreV1(), + PodDisruptionBudgetsGetter: clientSet.PolicyV1beta1(), + SecretsGetter: clientSet.CoreV1(), + ServicesGetter: clientSet.CoreV1(), + StatefulSetsGetter: clientSet.AppsV1(), }, clientSet } @@ -32,13 +33,13 @@ func TestInheritedAnnotations(t *testing.T) { client, _ := newFakeK8sAnnotationsClient() clusterName := "acid-test-cluster" namespace := "default" + annotationValue := "acid" - annotationKey := "acid" pg := acidv1.Postgresql{ ObjectMeta: metav1.ObjectMeta{ Name: clusterName, Annotations: map[string]string{ - "owned-by": annotationKey, + "owned-by": annotationValue, }, }, Spec: acidv1.PostgresSpec{ @@ -94,6 +95,15 @@ func TestInheritedAnnotations(t *testing.T) { } } + // check pvc annotations + pvcList, err := cluster.KubeClient.PersistentVolumeClaims(namespace).List(context.TODO(), listOptions) + assert.NoError(t, err) + for _, pvc := range pvcList.Items { + if !(util.MapContains(pvc.ObjectMeta.Annotations, inheritedAnnotations)) { + t.Errorf("%s: PVC %v not inherited annotations %#v, got %#v", testName, pvc.ObjectMeta.Name, inheritedAnnotations, pvc.ObjectMeta.Annotations) + } + } + // check service annotations svcList, err := cluster.KubeClient.Services(namespace).List(context.TODO(), listOptions) assert.NoError(t, err) From 48be164b05735d116fef37a01b2a9f6b97158b6c Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Mon, 7 Dec 2020 18:51:36 +0100 Subject: [PATCH 08/18] missing comma --- e2e/tests/test_e2e.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index b8618a2dd..b891b5c71 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -862,7 +862,7 @@ def test_statefulset_annotation_propagation(self): "annotations": { "deployment-time": "2020-04-30 12:00:00", "downscaler/downtime_replicas": "0", - "owned-by": "acid" + "owned-by": "acid", }, } } From 71124bbdc75d7339fbc92a07985bc202a6cf6d29 Mon Sep 17 00:00:00 2001 From: Rafia Sabih Date: Tue, 8 Dec 2020 11:23:50 +0100 Subject: [PATCH 09/18] fix nil map assignment --- e2e/tests/test_e2e.py | 2 +- pkg/cluster/util.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index b891b5c71..13e769377 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -874,7 +874,7 @@ def test_statefulset_annotation_propagation(self): "downscaler/downtime_replicas": "0", "owned-by": "acid", } - + self.eventuallyEqual(lambda: k8s.get_operator_state(), {"0": "idle"}, "Operator does not get in sync") self.eventuallyTrue(lambda: k8s.check_statefulset_annotations(cluster_label, annotations), "Annotations missing") @timeout_decorator.timeout(TEST_TIMEOUT_SEC) diff --git a/pkg/cluster/util.go b/pkg/cluster/util.go index 670ffe290..9f53fd177 100644 --- a/pkg/cluster/util.go +++ b/pkg/cluster/util.go @@ -274,6 +274,9 @@ func (c *Cluster) getTeamMembers(teamID string) ([]string, error) { // Returns annotations to be passed to child objects func (c *Cluster) annotationsSet(annotations map[string]string) map[string]string { + if annotations == nil { + annotations = make(map[string]string) + } // allow to inherit certain labels from the 'postgres' object if spec, err := c.GetSpec(); err == nil { for k, v := range spec.ObjectMeta.Annotations { From 641c2052f493bf29d3f83765d4595edbeb2a9b80 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 8 Dec 2020 15:19:51 +0100 Subject: [PATCH 10/18] set annotations in the same order it is done in other places --- pkg/cluster/k8sres.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index 5d0e54ba9..5b4315718 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1226,7 +1226,8 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef return nil, fmt.Errorf("could not set the pod management policy to the unknown value: %v", c.OpConfig.PodManagementPolicy) } - stsAnnotations := c.AnnotationsToPropagate(map[string]string{}) + stsAnnotations := c.annotationsSet(map[string]string{}) + stsAnnotations = c.AnnotationsToPropagate(stsAnnotations) stsAnnotations[rollingUpdateStatefulsetAnnotationKey] = strconv.FormatBool(false) statefulSet := &appsv1.StatefulSet{ @@ -1234,7 +1235,7 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef Name: c.statefulSetName(), Namespace: c.Namespace, Labels: c.labelsSet(true), - Annotations: c.annotationsSet(stsAnnotations), + Annotations: stsAnnotations, }, Spec: appsv1.StatefulSetSpec{ Replicas: &numberOfInstances, From 37bc33d176c55466e251bc3364d8e129c137d526 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 8 Dec 2020 15:20:43 +0100 Subject: [PATCH 11/18] replace acidClientSet with acid getters in K8s client --- pkg/cluster/util_test.go | 11 ++++++++--- pkg/util/k8sutil/k8sutil.go | 32 +++++++++++++++++++++----------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/pkg/cluster/util_test.go b/pkg/cluster/util_test.go index 637a442ba..4fac01483 100644 --- a/pkg/cluster/util_test.go +++ b/pkg/cluster/util_test.go @@ -6,15 +6,17 @@ import ( "github.com/stretchr/testify/assert" acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1" + fakeacidv1 "github.com/zalando/postgres-operator/pkg/generated/clientset/versioned/fake" "github.com/zalando/postgres-operator/pkg/util" "github.com/zalando/postgres-operator/pkg/util/config" "github.com/zalando/postgres-operator/pkg/util/k8sutil" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes/fake" + k8sFake "k8s.io/client-go/kubernetes/fake" ) -func newFakeK8sAnnotationsClient() (k8sutil.KubernetesClient, *fake.Clientset) { - clientSet := fake.NewSimpleClientset() +func newFakeK8sAnnotationsClient() (k8sutil.KubernetesClient, *k8sFake.Clientset) { + clientSet := k8sFake.NewSimpleClientset() + acidClientSet := fakeacidv1.NewSimpleClientset() return k8sutil.KubernetesClient{ DeploymentsGetter: clientSet.AppsV1(), @@ -25,6 +27,7 @@ func newFakeK8sAnnotationsClient() (k8sutil.KubernetesClient, *fake.Clientset) { SecretsGetter: clientSet.CoreV1(), ServicesGetter: clientSet.CoreV1(), StatefulSetsGetter: clientSet.AppsV1(), + PostgresqlsGetter: acidClientSet.AcidV1(), }, clientSet } @@ -54,12 +57,14 @@ func TestInheritedAnnotations(t *testing.T) { ClusterLabels: map[string]string{"application": "spilo"}, ClusterNameLabel: "cluster-name", InheritedAnnotations: []string{"owned-by"}, + PodRoleLabel: "spilo-role", }, }, }, client, pg, logger, eventRecorder) cluster.Name = clusterName cluster.Namespace = namespace + cluster.Create() // test annotationsSet function inheritedAnnotations := cluster.annotationsSet(map[string]string{}) diff --git a/pkg/util/k8sutil/k8sutil.go b/pkg/util/k8sutil/k8sutil.go index 19f95d9f1..006f52398 100644 --- a/pkg/util/k8sutil/k8sutil.go +++ b/pkg/util/k8sutil/k8sutil.go @@ -11,7 +11,9 @@ import ( batchv1beta1 "k8s.io/api/batch/v1beta1" clientbatchv1beta1 "k8s.io/client-go/kubernetes/typed/batch/v1beta1" - acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1" + apiacidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1" + acidv1client "github.com/zalando/postgres-operator/pkg/generated/clientset/versioned" + acidv1 "github.com/zalando/postgres-operator/pkg/generated/clientset/versioned/typed/acid.zalan.do/v1" "github.com/zalando/postgres-operator/pkg/spec" apiappsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" @@ -19,6 +21,7 @@ import ( apiextclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" apiextv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" appsv1 "k8s.io/client-go/kubernetes/typed/apps/v1" @@ -27,9 +30,6 @@ import ( rbacv1 "k8s.io/client-go/kubernetes/typed/rbac/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" - - acidv1client "github.com/zalando/postgres-operator/pkg/generated/clientset/versioned" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func Int32ToPointer(value int32) *int32 { @@ -55,9 +55,11 @@ type KubernetesClient struct { policyv1beta1.PodDisruptionBudgetsGetter apiextv1.CustomResourceDefinitionsGetter clientbatchv1beta1.CronJobsGetter + acidv1.OperatorConfigurationsGetter + acidv1.PostgresTeamsGetter + acidv1.PostgresqlsGetter - RESTClient rest.Interface - AcidV1ClientSet *acidv1client.Clientset + RESTClient rest.Interface } type mockSecret struct { @@ -154,15 +156,23 @@ func NewFromConfig(cfg *rest.Config) (KubernetesClient, error) { } kubeClient.CustomResourceDefinitionsGetter = apiextClient.ApiextensionsV1() - kubeClient.AcidV1ClientSet = acidv1client.NewForConfigOrDie(cfg) + + acidClient, err := acidv1client.NewForConfig(cfg) + if err != nil { + return kubeClient, fmt.Errorf("could not create acid.zalan.do clientset: %v", err) + } + + kubeClient.OperatorConfigurationsGetter = acidClient.AcidV1() + kubeClient.PostgresTeamsGetter = acidClient.AcidV1() + kubeClient.PostgresqlsGetter = acidClient.AcidV1() return kubeClient, nil } // SetPostgresCRDStatus of Postgres cluster -func (client *KubernetesClient) SetPostgresCRDStatus(clusterName spec.NamespacedName, status string) (*acidv1.Postgresql, error) { - var pg *acidv1.Postgresql - var pgStatus acidv1.PostgresStatus +func (client *KubernetesClient) SetPostgresCRDStatus(clusterName spec.NamespacedName, status string) (*apiacidv1.Postgresql, error) { + var pg *apiacidv1.Postgresql + var pgStatus apiacidv1.PostgresStatus pgStatus.PostgresClusterStatus = status patch, err := json.Marshal(struct { @@ -176,7 +186,7 @@ func (client *KubernetesClient) SetPostgresCRDStatus(clusterName spec.Namespaced // we cannot do a full scale update here without fetching the previous manifest (as the resourceVersion may differ), // however, we could do patch without it. In the future, once /status subresource is there (starting Kubernetes 1.11) // we should take advantage of it. - pg, err = client.AcidV1ClientSet.AcidV1().Postgresqls(clusterName.Namespace).Patch( + pg, err = client.PostgresqlsGetter.Postgresqls(clusterName.Namespace).Patch( context.TODO(), clusterName.Name, types.MergePatchType, patch, metav1.PatchOptions{}, "status") if err != nil { return pg, fmt.Errorf("could not update status: %v", err) From ec0d8f8aeccbddd7ca550794c2cfa8f98af34bb6 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 8 Dec 2020 15:42:49 +0100 Subject: [PATCH 12/18] more fixes on clientSet vs getters --- pkg/cluster/sync.go | 5 +++++ pkg/controller/operator_config.go | 2 +- pkg/controller/postgresql.go | 2 +- pkg/controller/util.go | 2 +- pkg/util/k8sutil/k8sutil.go | 11 ++++++----- 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 6404f554b..6b26603a1 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -398,6 +398,11 @@ func (c *Cluster) syncStatefulSet() error { // AnnotationsToPropagate get the annotations to update if required // based on the annotations in postgres CRD func (c *Cluster) AnnotationsToPropagate(annotations map[string]string) map[string]string { + + if annotations == nil { + annotations = make(map[string]string) + } + toPropagateAnnotations := c.OpConfig.DownscalerAnnotations pgCRDAnnotations := c.Postgresql.ObjectMeta.GetAnnotations() diff --git a/pkg/controller/operator_config.go b/pkg/controller/operator_config.go index 002479f47..7da384c07 100644 --- a/pkg/controller/operator_config.go +++ b/pkg/controller/operator_config.go @@ -15,7 +15,7 @@ import ( func (c *Controller) readOperatorConfigurationFromCRD(configObjectNamespace, configObjectName string) (*acidv1.OperatorConfiguration, error) { - config, err := c.KubeClient.AcidV1ClientSet.AcidV1().OperatorConfigurations(configObjectNamespace).Get( + config, err := c.KubeClient.OperatorConfigurationsGetter.OperatorConfigurations(configObjectNamespace).Get( context.TODO(), configObjectName, metav1.GetOptions{}) if err != nil { return nil, fmt.Errorf("could not get operator configuration object %q: %v", configObjectName, err) diff --git a/pkg/controller/postgresql.go b/pkg/controller/postgresql.go index 4b5d68fe5..0fe0c1120 100644 --- a/pkg/controller/postgresql.go +++ b/pkg/controller/postgresql.go @@ -46,7 +46,7 @@ func (c *Controller) listClusters(options metav1.ListOptions) (*acidv1.Postgresq var pgList acidv1.PostgresqlList // TODO: use the SharedInformer cache instead of quering Kubernetes API directly. - list, err := c.KubeClient.AcidV1ClientSet.AcidV1().Postgresqls(c.opConfig.WatchedNamespace).List(context.TODO(), options) + list, err := c.KubeClient.PostgresqlsGetter.Postgresqls(c.opConfig.WatchedNamespace).List(context.TODO(), options) if err != nil { c.logger.Errorf("could not list postgresql objects: %v", err) } diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 7f87de97d..815bc7b74 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -398,7 +398,7 @@ func (c *Controller) loadPostgresTeams() { // reset team map c.pgTeamMap = teams.PostgresTeamMap{} - pgTeams, err := c.KubeClient.AcidV1ClientSet.AcidV1().PostgresTeams(c.opConfig.WatchedNamespace).List(context.TODO(), metav1.ListOptions{}) + pgTeams, err := c.KubeClient.PostgresTeamsGetter.PostgresTeams(c.opConfig.WatchedNamespace).List(context.TODO(), metav1.ListOptions{}) if err != nil { c.logger.Errorf("could not list postgres team objects: %v", err) } diff --git a/pkg/util/k8sutil/k8sutil.go b/pkg/util/k8sutil/k8sutil.go index 006f52398..a23c1f842 100644 --- a/pkg/util/k8sutil/k8sutil.go +++ b/pkg/util/k8sutil/k8sutil.go @@ -59,7 +59,8 @@ type KubernetesClient struct { acidv1.PostgresTeamsGetter acidv1.PostgresqlsGetter - RESTClient rest.Interface + RESTClient rest.Interface + AcidV1ClientSet *acidv1client.Clientset } type mockSecret struct { @@ -157,14 +158,14 @@ func NewFromConfig(cfg *rest.Config) (KubernetesClient, error) { kubeClient.CustomResourceDefinitionsGetter = apiextClient.ApiextensionsV1() - acidClient, err := acidv1client.NewForConfig(cfg) + kubeClient.AcidV1ClientSet = acidv1client.NewForConfigOrDie(cfg) if err != nil { return kubeClient, fmt.Errorf("could not create acid.zalan.do clientset: %v", err) } - kubeClient.OperatorConfigurationsGetter = acidClient.AcidV1() - kubeClient.PostgresTeamsGetter = acidClient.AcidV1() - kubeClient.PostgresqlsGetter = acidClient.AcidV1() + kubeClient.OperatorConfigurationsGetter = kubeClient.AcidV1ClientSet.AcidV1() + kubeClient.PostgresTeamsGetter = kubeClient.AcidV1ClientSet.AcidV1() + kubeClient.PostgresqlsGetter = kubeClient.AcidV1ClientSet.AcidV1() return kubeClient, nil } From f6212da046051d6b257149e9d985c3bcaf6ebe17 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 8 Dec 2020 16:46:39 +0100 Subject: [PATCH 13/18] minor changes --- pkg/cluster/connection_pooler.go | 7 +++++-- pkg/cluster/k8sres.go | 15 +++++++-------- pkg/cluster/sync.go | 7 +++---- pkg/cluster/util.go | 9 +++++---- pkg/cluster/util_test.go | 2 +- 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index 1c1ffaff9..af4db7d19 100644 --- a/pkg/cluster/connection_pooler.go +++ b/pkg/cluster/connection_pooler.go @@ -334,12 +334,15 @@ func (c *Cluster) generateConnectionPoolerDeployment(connectionPooler *Connectio return nil, err } + annotations := c.annotationsSet(nil) + annotations = c.AnnotationsToPropagate(annotations) + deployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: connectionPooler.Name, Namespace: connectionPooler.Namespace, Labels: c.connectionPoolerLabels(connectionPooler.Role, true).MatchLabels, - Annotations: c.annotationsSet(map[string]string{}), + Annotations: annotations, // make StatefulSet object its owner to represent the dependency. // By itself StatefulSet is being deleted with "Orphaned" // propagation policy, which means that it's deletion will not @@ -868,7 +871,7 @@ func (c *Cluster) syncConnectionPoolerWorker(oldSpec, newSpec *acidv1.Postgresql newAnnotations := c.annotationsSet(c.ConnectionPooler[role].Deployment.Annotations) newAnnotations = c.AnnotationsToPropagate(newAnnotations) - if newAnnotations != nil { + if newAnnotations != nil && len(newAnnotations) > 0 { deployment, err = updateConnectionPoolerAnnotations(c.KubeClient, c.ConnectionPooler[role].Deployment, newAnnotations) if err != nil { return nil, err diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index 5b4315718..b5b842053 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1226,7 +1226,7 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef return nil, fmt.Errorf("could not set the pod management policy to the unknown value: %v", c.OpConfig.PodManagementPolicy) } - stsAnnotations := c.annotationsSet(map[string]string{}) + stsAnnotations := c.annotationsSet(nil) stsAnnotations = c.AnnotationsToPropagate(stsAnnotations) stsAnnotations[rollingUpdateStatefulsetAnnotationKey] = strconv.FormatBool(false) @@ -1531,7 +1531,7 @@ func (c *Cluster) generateSingleUserSecret(namespace string, pgUser spec.PgUser) Name: c.credentialSecretName(username), Namespace: namespace, Labels: c.labelsSet(true), - Annotations: c.annotationsSet(map[string]string{}), + Annotations: c.annotationsSet(nil), }, Type: v1.SecretTypeOpaque, Data: map[string][]byte{ @@ -1652,10 +1652,9 @@ func (c *Cluster) generateServiceAnnotations(role PostgresRole, spec *acidv1.Pos func (c *Cluster) generateEndpoint(role PostgresRole, subsets []v1.EndpointSubset) *v1.Endpoints { endpoints := &v1.Endpoints{ ObjectMeta: metav1.ObjectMeta{ - Name: c.endpointName(role), - Namespace: c.Namespace, - Labels: c.roleLabelsSet(true, role), - Annotations: c.annotationsSet(map[string]string{}), + Name: c.endpointName(role), + Namespace: c.Namespace, + Labels: c.roleLabelsSet(true, role), }, } if len(subsets) > 0 { @@ -1812,7 +1811,7 @@ func (c *Cluster) generatePodDisruptionBudget() *policybeta1.PodDisruptionBudget Name: c.podDisruptionBudgetName(), Namespace: c.Namespace, Labels: c.labelsSet(true), - Annotations: c.annotationsSet(map[string]string{}), + Annotations: c.annotationsSet(nil), }, Spec: policybeta1.PodDisruptionBudgetSpec{ MinAvailable: &minAvailable, @@ -1935,7 +1934,7 @@ func (c *Cluster) generateLogicalBackupJob() (*batchv1beta1.CronJob, error) { Name: c.getLogicalBackupJobName(), Namespace: c.Namespace, Labels: c.labelsSet(true), - Annotations: c.annotationsSet(map[string]string{}), + Annotations: c.annotationsSet(nil), }, Spec: batchv1beta1.CronJobSpec{ Schedule: schedule, diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 6b26603a1..997d6db19 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -403,11 +403,10 @@ func (c *Cluster) AnnotationsToPropagate(annotations map[string]string) map[stri annotations = make(map[string]string) } - toPropagateAnnotations := c.OpConfig.DownscalerAnnotations - pgCRDAnnotations := c.Postgresql.ObjectMeta.GetAnnotations() + pgCRDAnnotations := c.ObjectMeta.Annotations - if toPropagateAnnotations != nil && pgCRDAnnotations != nil { - for _, anno := range toPropagateAnnotations { + if pgCRDAnnotations != nil { + for _, anno := range c.OpConfig.DownscalerAnnotations { for k, v := range pgCRDAnnotations { matched, err := regexp.MatchString(anno, k) if err != nil { diff --git a/pkg/cluster/util.go b/pkg/cluster/util.go index 9f53fd177..d5e887656 100644 --- a/pkg/cluster/util.go +++ b/pkg/cluster/util.go @@ -277,17 +277,18 @@ func (c *Cluster) annotationsSet(annotations map[string]string) map[string]strin if annotations == nil { annotations = make(map[string]string) } + + pgCRDAnnotations := c.ObjectMeta.Annotations + // allow to inherit certain labels from the 'postgres' object - if spec, err := c.GetSpec(); err == nil { - for k, v := range spec.ObjectMeta.Annotations { + if pgCRDAnnotations != nil { + for k, v := range pgCRDAnnotations { for _, match := range c.OpConfig.InheritedAnnotations { if k == match { annotations[k] = v } } } - } else { - c.logger.Warningf("could not get the list of InheritedAnnoations for cluster %q: %v", c.Name, err) } if len(annotations) > 0 { diff --git a/pkg/cluster/util_test.go b/pkg/cluster/util_test.go index 4fac01483..19637ac36 100644 --- a/pkg/cluster/util_test.go +++ b/pkg/cluster/util_test.go @@ -67,7 +67,7 @@ func TestInheritedAnnotations(t *testing.T) { cluster.Create() // test annotationsSet function - inheritedAnnotations := cluster.annotationsSet(map[string]string{}) + inheritedAnnotations := cluster.annotationsSet(nil) listOptions := metav1.ListOptions{ LabelSelector: cluster.labelsSet(false).String(), From 322a8266aa31e786818b87376217441d5c01a12a Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 8 Dec 2020 16:48:51 +0100 Subject: [PATCH 14/18] remove endpoints from annotation test --- pkg/cluster/util_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/cluster/util_test.go b/pkg/cluster/util_test.go index 19637ac36..29fc63ff7 100644 --- a/pkg/cluster/util_test.go +++ b/pkg/cluster/util_test.go @@ -20,7 +20,6 @@ func newFakeK8sAnnotationsClient() (k8sutil.KubernetesClient, *k8sFake.Clientset return k8sutil.KubernetesClient{ DeploymentsGetter: clientSet.AppsV1(), - EndpointsGetter: clientSet.CoreV1(), PersistentVolumeClaimsGetter: clientSet.CoreV1(), PodsGetter: clientSet.CoreV1(), PodDisruptionBudgetsGetter: clientSet.PolicyV1beta1(), @@ -118,15 +117,6 @@ func TestInheritedAnnotations(t *testing.T) { } } - // check endpoint annotations - epList, err := cluster.KubeClient.Endpoints(namespace).List(context.TODO(), listOptions) - assert.NoError(t, err) - for _, ep := range epList.Items { - if !(util.MapContains(ep.ObjectMeta.Annotations, inheritedAnnotations)) { - t.Errorf("%s: Endpoint %v not inherited annotations %#v, got %#v", testName, ep.ObjectMeta.Name, inheritedAnnotations, ep.ObjectMeta.Annotations) - } - } - // check pod disruption budget annotations pdbList, err := cluster.KubeClient.PodDisruptionBudgets(namespace).List(context.TODO(), listOptions) assert.NoError(t, err) From 77540febac7526ff4d967166b9397b2b79999753 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 8 Dec 2020 18:32:40 +0100 Subject: [PATCH 15/18] refine unit test - but deployment and sts are still empty --- pkg/cluster/util_test.go | 51 ++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 34 deletions(-) diff --git a/pkg/cluster/util_test.go b/pkg/cluster/util_test.go index 29fc63ff7..47f26e362 100644 --- a/pkg/cluster/util_test.go +++ b/pkg/cluster/util_test.go @@ -11,6 +11,7 @@ import ( "github.com/zalando/postgres-operator/pkg/util/config" "github.com/zalando/postgres-operator/pkg/util/k8sutil" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" k8sFake "k8s.io/client-go/kubernetes/fake" ) @@ -21,9 +22,7 @@ func newFakeK8sAnnotationsClient() (k8sutil.KubernetesClient, *k8sFake.Clientset return k8sutil.KubernetesClient{ DeploymentsGetter: clientSet.AppsV1(), PersistentVolumeClaimsGetter: clientSet.CoreV1(), - PodsGetter: clientSet.CoreV1(), PodDisruptionBudgetsGetter: clientSet.PolicyV1beta1(), - SecretsGetter: clientSet.CoreV1(), ServicesGetter: clientSet.CoreV1(), StatefulSetsGetter: clientSet.AppsV1(), PostgresqlsGetter: acidClientSet.AcidV1(), @@ -63,17 +62,17 @@ func TestInheritedAnnotations(t *testing.T) { cluster.Name = clusterName cluster.Namespace = namespace - cluster.Create() // test annotationsSet function inheritedAnnotations := cluster.annotationsSet(nil) - listOptions := metav1.ListOptions{ - LabelSelector: cluster.labelsSet(false).String(), + poolerListOptions := metav1.ListOptions{ + LabelSelector: labels.Set(cluster.connectionPoolerLabels(Master, false).MatchLabels).String(), } // check pooler deployment annotations - deployList, err := cluster.KubeClient.Deployments(namespace).List(context.TODO(), listOptions) + cluster.createConnectionPooler(cluster.installLookupFunction) + deployList, err := client.Deployments(namespace).List(context.TODO(), poolerListOptions) assert.NoError(t, err) for _, deploy := range deployList.Items { if !(util.MapContains(deploy.ObjectMeta.Annotations, inheritedAnnotations)) { @@ -81,35 +80,27 @@ func TestInheritedAnnotations(t *testing.T) { } } + listOptions := metav1.ListOptions{ + LabelSelector: cluster.labelsSet(false).String(), + } + // check statefulset annotations - stsList, err := cluster.KubeClient.StatefulSets(namespace).List(context.TODO(), listOptions) + cluster.createStatefulSet() + stsList, err := client.StatefulSets(namespace).List(context.TODO(), listOptions) assert.NoError(t, err) for _, sts := range stsList.Items { if !(util.MapContains(sts.ObjectMeta.Annotations, inheritedAnnotations)) { t.Errorf("%s: StatefulSet %v not inherited annotations %#v, got %#v", testName, sts.ObjectMeta.Name, inheritedAnnotations, sts.ObjectMeta.Annotations) } - } - - // check pod annotations - podList, err := cluster.KubeClient.Pods(namespace).List(context.TODO(), listOptions) - assert.NoError(t, err) - for _, pod := range podList.Items { - if !(util.MapContains(pod.ObjectMeta.Annotations, inheritedAnnotations)) { - t.Errorf("%s: Pod %v not inherited annotations %#v, got %#v", testName, pod.ObjectMeta.Name, inheritedAnnotations, pod.ObjectMeta.Annotations) - } - } - // check pvc annotations - pvcList, err := cluster.KubeClient.PersistentVolumeClaims(namespace).List(context.TODO(), listOptions) - assert.NoError(t, err) - for _, pvc := range pvcList.Items { - if !(util.MapContains(pvc.ObjectMeta.Annotations, inheritedAnnotations)) { - t.Errorf("%s: PVC %v not inherited annotations %#v, got %#v", testName, pvc.ObjectMeta.Name, inheritedAnnotations, pvc.ObjectMeta.Annotations) + if !(util.MapContains(sts.Spec.Template.Annotations, inheritedAnnotations)) { + t.Errorf("%s: pod template %v not inherited annotations %#v, got %#v", testName, sts.ObjectMeta.Name, inheritedAnnotations, sts.ObjectMeta.Annotations) } } // check service annotations - svcList, err := cluster.KubeClient.Services(namespace).List(context.TODO(), listOptions) + cluster.createService(Master) + svcList, err := client.Services(namespace).List(context.TODO(), listOptions) assert.NoError(t, err) for _, svc := range svcList.Items { if !(util.MapContains(svc.ObjectMeta.Annotations, inheritedAnnotations)) { @@ -118,20 +109,12 @@ func TestInheritedAnnotations(t *testing.T) { } // check pod disruption budget annotations - pdbList, err := cluster.KubeClient.PodDisruptionBudgets(namespace).List(context.TODO(), listOptions) + cluster.createPodDisruptionBudget() + pdbList, err := client.PodDisruptionBudgets(namespace).List(context.TODO(), listOptions) assert.NoError(t, err) for _, pdb := range pdbList.Items { if !(util.MapContains(pdb.ObjectMeta.Annotations, inheritedAnnotations)) { t.Errorf("%s: Pod Disruption Budget %v not inherited annotations %#v, got %#v", testName, pdb.ObjectMeta.Name, inheritedAnnotations, pdb.ObjectMeta.Annotations) } } - - // check secret annotations - secretList, err := cluster.KubeClient.Secrets(namespace).List(context.TODO(), listOptions) - assert.NoError(t, err) - for _, secret := range secretList.Items { - if !(util.MapContains(secret.ObjectMeta.Annotations, inheritedAnnotations)) { - t.Errorf("%s: Secret %v not inherited annotations %#v, got %#v", testName, secret.ObjectMeta.Name, inheritedAnnotations, secret.ObjectMeta.Annotations) - } - } } From 5a0d97d3d9e89d2f615d11ee50cf1733a8d40c8f Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Wed, 9 Dec 2020 17:23:46 +0100 Subject: [PATCH 16/18] fix checkinng sts and deployment --- pkg/cluster/connection_pooler.go | 2 +- pkg/cluster/util_test.go | 69 +++++++++++++++++++++----------- 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index af4db7d19..53c036287 100644 --- a/pkg/cluster/connection_pooler.go +++ b/pkg/cluster/connection_pooler.go @@ -325,7 +325,7 @@ func (c *Cluster) generateConnectionPoolerDeployment(connectionPooler *Connectio if *numberOfInstances < constants.ConnectionPoolerMinInstances { msg := "Adjusted number of connection pooler instances from %d to %d" - c.logger.Warningf(msg, numberOfInstances, constants.ConnectionPoolerMinInstances) + c.logger.Warningf(msg, *numberOfInstances, constants.ConnectionPoolerMinInstances) *numberOfInstances = constants.ConnectionPoolerMinInstances } diff --git a/pkg/cluster/util_test.go b/pkg/cluster/util_test.go index 47f26e362..7afc59f28 100644 --- a/pkg/cluster/util_test.go +++ b/pkg/cluster/util_test.go @@ -11,7 +11,6 @@ import ( "github.com/zalando/postgres-operator/pkg/util/config" "github.com/zalando/postgres-operator/pkg/util/k8sutil" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" k8sFake "k8s.io/client-go/kubernetes/fake" ) @@ -20,12 +19,10 @@ func newFakeK8sAnnotationsClient() (k8sutil.KubernetesClient, *k8sFake.Clientset acidClientSet := fakeacidv1.NewSimpleClientset() return k8sutil.KubernetesClient{ - DeploymentsGetter: clientSet.AppsV1(), - PersistentVolumeClaimsGetter: clientSet.CoreV1(), - PodDisruptionBudgetsGetter: clientSet.PolicyV1beta1(), - ServicesGetter: clientSet.CoreV1(), - StatefulSetsGetter: clientSet.AppsV1(), - PostgresqlsGetter: acidClientSet.AcidV1(), + PodDisruptionBudgetsGetter: clientSet.PolicyV1beta1(), + ServicesGetter: clientSet.CoreV1(), + StatefulSetsGetter: clientSet.AppsV1(), + PostgresqlsGetter: acidClientSet.AcidV1(), }, clientSet } @@ -35,6 +32,7 @@ func TestInheritedAnnotations(t *testing.T) { clusterName := "acid-test-cluster" namespace := "default" annotationValue := "acid" + role := Master pg := acidv1.Postgresql{ ObjectMeta: metav1.ObjectMeta{ @@ -45,15 +43,30 @@ func TestInheritedAnnotations(t *testing.T) { }, Spec: acidv1.PostgresSpec{ EnableReplicaConnectionPooler: boolToPointer(true), + Volume: acidv1.Volume{ + Size: "1Gi", + }, }, } var cluster = New( Config{ OpConfig: config.Config{ + ConnectionPooler: config.ConnectionPooler{ + ConnectionPoolerDefaultCPURequest: "100m", + ConnectionPoolerDefaultCPULimit: "100m", + ConnectionPoolerDefaultMemoryRequest: "100Mi", + ConnectionPoolerDefaultMemoryLimit: "100Mi", + NumberOfInstances: int32ToPointer(1), + }, + PodManagementPolicy: "ordered_ready", Resources: config.Resources{ ClusterLabels: map[string]string{"application": "spilo"}, ClusterNameLabel: "cluster-name", + DefaultCPURequest: "300m", + DefaultCPULimit: "300m", + DefaultMemoryRequest: "300Mi", + DefaultMemoryLimit: "300Mi", InheritedAnnotations: []string{"owned-by"}, PodRoleLabel: "spilo-role", }, @@ -66,36 +79,28 @@ func TestInheritedAnnotations(t *testing.T) { // test annotationsSet function inheritedAnnotations := cluster.annotationsSet(nil) - poolerListOptions := metav1.ListOptions{ - LabelSelector: labels.Set(cluster.connectionPoolerLabels(Master, false).MatchLabels).String(), - } - - // check pooler deployment annotations - cluster.createConnectionPooler(cluster.installLookupFunction) - deployList, err := client.Deployments(namespace).List(context.TODO(), poolerListOptions) - assert.NoError(t, err) - for _, deploy := range deployList.Items { - if !(util.MapContains(deploy.ObjectMeta.Annotations, inheritedAnnotations)) { - t.Errorf("%s: Deployment %v not inherited annotations %#v, got %#v", testName, deploy.ObjectMeta.Name, inheritedAnnotations, deploy.ObjectMeta.Annotations) - } - } - listOptions := metav1.ListOptions{ LabelSelector: cluster.labelsSet(false).String(), } // check statefulset annotations - cluster.createStatefulSet() + _, err := cluster.createStatefulSet() + assert.NoError(t, err) + stsList, err := client.StatefulSets(namespace).List(context.TODO(), listOptions) assert.NoError(t, err) for _, sts := range stsList.Items { if !(util.MapContains(sts.ObjectMeta.Annotations, inheritedAnnotations)) { t.Errorf("%s: StatefulSet %v not inherited annotations %#v, got %#v", testName, sts.ObjectMeta.Name, inheritedAnnotations, sts.ObjectMeta.Annotations) } - - if !(util.MapContains(sts.Spec.Template.Annotations, inheritedAnnotations)) { + // pod template + if !(util.MapContains(sts.Spec.Template.ObjectMeta.Annotations, inheritedAnnotations)) { t.Errorf("%s: pod template %v not inherited annotations %#v, got %#v", testName, sts.ObjectMeta.Name, inheritedAnnotations, sts.ObjectMeta.Annotations) } + // pvc template + if util.MapContains(sts.Spec.VolumeClaimTemplates[0].Annotations, inheritedAnnotations) { + t.Errorf("%s: PVC template %v not expected to have inherited annotations %#v, got %#v", testName, sts.ObjectMeta.Name, inheritedAnnotations, sts.ObjectMeta.Annotations) + } } // check service annotations @@ -117,4 +122,20 @@ func TestInheritedAnnotations(t *testing.T) { t.Errorf("%s: Pod Disruption Budget %v not inherited annotations %#v, got %#v", testName, pdb.ObjectMeta.Name, inheritedAnnotations, pdb.ObjectMeta.Annotations) } } + + // check pooler deployment annotations + cluster.ConnectionPooler = map[PostgresRole]*ConnectionPoolerObjects{} + cluster.ConnectionPooler[role] = &ConnectionPoolerObjects{ + Name: cluster.connectionPoolerName(role), + ClusterName: cluster.ClusterName, + Namespace: cluster.Namespace, + Role: role, + } + deploy, err := cluster.generateConnectionPoolerDeployment(cluster.ConnectionPooler[role]) + assert.NoError(t, err) + + if !(util.MapContains(deploy.ObjectMeta.Annotations, inheritedAnnotations)) { + t.Errorf("%s: Deployment %v not inherited annotations %#v, got %#v", testName, deploy.ObjectMeta.Name, inheritedAnnotations, deploy.ObjectMeta.Annotations) + } + } From 4d18f788d2cf4f66b601dfe940e9ace3493a3936 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Thu, 10 Dec 2020 15:41:07 +0100 Subject: [PATCH 17/18] make annotations setter one liners --- docs/reference/operator_parameters.md | 6 +++--- pkg/cluster/connection_pooler.go | 8 ++------ pkg/cluster/k8sres.go | 4 ++-- pkg/cluster/sync.go | 11 +++++++---- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/docs/reference/operator_parameters.md b/docs/reference/operator_parameters.md index 9d4e5fc58..a812c5d9f 100644 --- a/docs/reference/operator_parameters.md +++ b/docs/reference/operator_parameters.md @@ -273,9 +273,9 @@ configuration they are grouped under the `kubernetes` key. * **inherited_annotations** list of annotation keys that can be inherited from the cluster manifest, and - added to each child objects (`Deployment`, `StatefulSet`, `Pod`, `PVCs`, - `PDB`, `Service`, `Endpoints` and `Secrets`) created by the operator. - The default is empty. + added to each child objects (`Deployment`, `StatefulSet`, `Pod`, `PDB` and + `Services`) created by the operator incl. the ones from the connection + pooler deployment. The default is empty. * **pod_role_label** name of the label assigned to the Postgres pods (and services/endpoints) by diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index 53c036287..d36aff7f1 100644 --- a/pkg/cluster/connection_pooler.go +++ b/pkg/cluster/connection_pooler.go @@ -334,15 +334,12 @@ func (c *Cluster) generateConnectionPoolerDeployment(connectionPooler *Connectio return nil, err } - annotations := c.annotationsSet(nil) - annotations = c.AnnotationsToPropagate(annotations) - deployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: connectionPooler.Name, Namespace: connectionPooler.Namespace, Labels: c.connectionPoolerLabels(connectionPooler.Role, true).MatchLabels, - Annotations: annotations, + Annotations: c.AnnotationsToPropagate(c.annotationsSet(nil)), // make StatefulSet object its owner to represent the dependency. // By itself StatefulSet is being deleted with "Orphaned" // propagation policy, which means that it's deletion will not @@ -869,8 +866,7 @@ func (c *Cluster) syncConnectionPoolerWorker(oldSpec, newSpec *acidv1.Postgresql } } - newAnnotations := c.annotationsSet(c.ConnectionPooler[role].Deployment.Annotations) - newAnnotations = c.AnnotationsToPropagate(newAnnotations) + newAnnotations := c.AnnotationsToPropagate(c.annotationsSet(c.ConnectionPooler[role].Deployment.Annotations)) if newAnnotations != nil && len(newAnnotations) > 0 { deployment, err = updateConnectionPoolerAnnotations(c.KubeClient, c.ConnectionPooler[role].Deployment, newAnnotations) if err != nil { diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index b5b842053..a585649db 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1226,9 +1226,9 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef return nil, fmt.Errorf("could not set the pod management policy to the unknown value: %v", c.OpConfig.PodManagementPolicy) } - stsAnnotations := c.annotationsSet(nil) - stsAnnotations = c.AnnotationsToPropagate(stsAnnotations) + stsAnnotations := make(map[string]string) stsAnnotations[rollingUpdateStatefulsetAnnotationKey] = strconv.FormatBool(false) + stsAnnotations = c.AnnotationsToPropagate(c.annotationsSet(nil)) statefulSet := &appsv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 997d6db19..211903689 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -353,9 +353,8 @@ func (c *Cluster) syncStatefulSet() error { } } } - annotations := c.annotationsSet(c.Statefulset.Annotations) - annotations = c.AnnotationsToPropagate(annotations) - c.updateStatefulSetAnnotations(annotations) + + c.updateStatefulSetAnnotations(c.AnnotationsToPropagate(c.annotationsSet(c.Statefulset.Annotations))) if !podsRollingUpdateRequired && !c.OpConfig.EnableLazySpiloUpgrade { // even if desired and actual statefulsets match @@ -420,7 +419,11 @@ func (c *Cluster) AnnotationsToPropagate(annotations map[string]string) map[stri } } - return annotations + if len(annotations) > 0 { + return annotations + } + + return nil } // checkAndSetGlobalPostgreSQLConfiguration checks whether cluster-wide API parameters From 6be1b0ce4375cac5dac73c2e31f93598b0c579de Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Thu, 10 Dec 2020 15:45:29 +0100 Subject: [PATCH 18/18] no need for len check anymore --- pkg/cluster/connection_pooler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index d36aff7f1..89330d373 100644 --- a/pkg/cluster/connection_pooler.go +++ b/pkg/cluster/connection_pooler.go @@ -867,7 +867,7 @@ func (c *Cluster) syncConnectionPoolerWorker(oldSpec, newSpec *acidv1.Postgresql } newAnnotations := c.AnnotationsToPropagate(c.annotationsSet(c.ConnectionPooler[role].Deployment.Annotations)) - if newAnnotations != nil && len(newAnnotations) > 0 { + if newAnnotations != nil { deployment, err = updateConnectionPoolerAnnotations(c.KubeClient, c.ConnectionPooler[role].Deployment, newAnnotations) if err != nil { return nil, err