Address review

Author: hughcapet

docs/administrator.md

@@ -620,22 +620,34 @@ By default the topology key for the pod anti affinity is set to
 `kubernetes.io/hostname`, you can set another topology key e.g.
 `failure-domain.beta.kubernetes.io/zone`. See [built-in node labels](https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#interlude-built-in-node-labels) for available topology keys.
 
-## Pod Disruption Budget
+## Pod Disruption Budgets
 
-By default the operator uses a PodDisruptionBudget (PDB) to protect the cluster
-from voluntarily disruptions and hence unwanted DB downtime. The `MinAvailable`
-parameter of the PDB is set to `1` which prevents killing masters in single-node
-clusters and/or the last remaining running instance in a multi-node cluster.
+By default the operator creates two PodDisruptionBudgets (PDB) to protect the cluster
+from voluntarily disruptions and hence unwanted DB downtime: so-called primary PDB and
+and PDB for critical operations.
+
+### Primary PDB
+The `MinAvailable` parameter of this PDB is set to `1` and, if `pdb_master_label_selector`
+is enabled, label selector includes `spilo-role=master` condition, which prevents killing
+masters in single-node clusters and/or the last remaining running instance in a multi-node
+cluster.
+
+## PDB for critical operations
+The `MinAvailable` parameter of this PDB is equal to the `numberOfInstances` set in the
+cluster manifest, while label selector includes `critical-operation=true` condition. This
+allows to protect all pods of a cluster, given they are labeled accordingly.
+For example, Operator labels all Spilo pods with `critical-operation=true` during the major
+version upgrade run. You may want to protect cluster pods during other critical operations
+by assigning the label to pods yourself or using other means of automation.
 
 The PDB is only relaxed in two scenarios:
 
 * If a cluster is scaled down to `0` instances (e.g. for draining nodes)
 * If the PDB is disabled in the configuration (`enable_pod_disruption_budget`)
 
-The PDB is still in place having `MinAvailable` set to `0`. If enabled it will
-be automatically set to `1` on scale up. Disabling PDBs helps avoiding blocking
-Kubernetes upgrades in managed K8s environments at the cost of prolonged DB
-downtime. See PR [#384](https://github.com/zalando/postgres-operator/pull/384)
+The PDBs are still in place having `MinAvailable` set to `0`. Disabling PDBs
+helps avoiding blocking Kubernetes upgrades in managed K8s environments at the
+cost of prolonged DB downtime. See PR [#384](https://github.com/zalando/postgres-operator/pull/384)
 for the use case.
 
 ## Add cluster-specific labels

docs/quickstart.md

@@ -230,7 +230,7 @@ kubectl delete postgresql acid-minimal-cluster
 ```
 
 This should remove the associated StatefulSet, database Pods, Services and
-Endpoints. The PersistentVolumes are released and the PodDisruptionBudget is
+Endpoints. The PersistentVolumes are released and the PodDisruptionBudgets are
 deleted. Secrets however are not deleted and backups will remain in place.
 
 When deleting a cluster while it is still starting up or got stuck during that

docs/reference/operator_parameters.md

@@ -334,13 +334,13 @@ configuration they are grouped under the `kubernetes` key.
   pod namespace).
 
 * **pdb_name_format**
-  defines the template for PDB (Pod Disruption Budget) names created by the
+  defines the template for primary PDB (Pod Disruption Budget) name created by the
   operator. The default is `postgres-{cluster}-pdb`, where `{cluster}` is
   replaced by the cluster name. Only the `{cluster}` placeholders is allowed in
   the template.
 
 * **pdb_master_label_selector**
-  By default the PDB will match the master role hence preventing nodes to be
+  By default the primary PDB will match the master role hence preventing nodes to be
   drained if the node_readiness_label is not used. If this option if set to
   `false` the `spilo-role=master` selector will not be added to the PDB.
 

e2e/tests/test_e2e.py

@@ -2547,9 +2547,9 @@ def check_cluster_child_resources_owner_references(self, cluster_name, cluster_n
         self.assertTrue(self.has_postgresql_owner_reference(config_ep.metadata.owner_references, inverse), "config endpoint owner reference check failed")
 
         pdb = k8s.api.policy_v1.read_namespaced_pod_disruption_budget("postgres-{}-pdb".format(cluster_name), cluster_namespace)
-        self.assertTrue(self.has_postgresql_owner_reference(pdb.metadata.owner_references, inverse), "pod disruption budget owner reference check failed")
+        self.assertTrue(self.has_postgresql_owner_reference(pdb.metadata.owner_references, inverse), "primary pod disruption budget owner reference check failed")
 
-        pdb = k8s.api.policy_v1.read_namespaced_pod_disruption_budget("postgres-{}-critical-operation-pdb".format(cluster_name), cluster_namespace)
+        pdb = k8s.api.policy_v1.read_namespaced_pod_disruption_budget("postgres-{}-critical-op-pdb".format(cluster_name), cluster_namespace)
         self.assertTrue(self.has_postgresql_owner_reference(pdb.metadata.owner_references, inverse), "pod disruption budget for critical operations owner reference check failed")
 
         pg_secret = k8s.api.core_v1.read_namespaced_secret("postgres.{}.credentials.postgresql.acid.zalan.do".format(cluster_name), cluster_namespace)

pkg/cluster/cluster.go

@@ -66,7 +66,7 @@ type kubeResources struct {
 	Secrets                       map[types.UID]*v1.Secret
 	Statefulset                   *appsv1.StatefulSet
 	VolumeClaims                  map[types.UID]*v1.PersistentVolumeClaim
-	GeneralPodDisruptionBudget    *policyv1.PodDisruptionBudget
+	PrimaryPodDisruptionBudget    *policyv1.PodDisruptionBudget
 	CriticalOpPodDisruptionBudget *policyv1.PodDisruptionBudget
 	LogicalBackupJob              *batchv1.CronJob
 	Streams                       map[string]*zalandov1.FabricEventStream
@@ -1735,7 +1735,7 @@ func (c *Cluster) GetStatus() *ClusterStatus {
 		MasterService:                 c.GetServiceMaster(),
 		ReplicaService:                c.GetServiceReplica(),
 		StatefulSet:                   c.GetStatefulSet(),
-		GeneralPodDisruptionBudget:    c.GetGeneralPodDisruptionBudget(),
+		PrimaryPodDisruptionBudget:    c.GetPrimaryPodDisruptionBudget(),
 		CriticalOpPodDisruptionBudget: c.GetCriticalOpPodDisruptionBudget(),
 		CurrentProcess:                c.GetCurrentProcess(),
 

pkg/cluster/k8sres.go

@@ -109,12 +109,12 @@ func (c *Cluster) servicePort(role PostgresRole) int32 {
 	return pgPort
 }
 
-func (c *Cluster) generalPodDisruptionBudgetName() string {
+func (c *Cluster) PrimaryPodDisruptionBudgetName() string {
 	return c.OpConfig.PDBNameFormat.Format("cluster", c.Name)
 }
 
 func (c *Cluster) criticalOpPodDisruptionBudgetName() string {
-	pdbTemplate := config.StringTemplate("postgres-{cluster}-critical-operation-pdb")
+	pdbTemplate := config.StringTemplate("postgres-{cluster}-critical-op-pdb")
 	return pdbTemplate.Format("cluster", c.Name)
 }
 
@@ -2212,7 +2212,7 @@ func (c *Cluster) generateStandbyEnvironment(description *acidv1.StandbyDescript
 	return result
 }
 
-func (c *Cluster) generateGeneralPodDisruptionBudget() *policyv1.PodDisruptionBudget {
+func (c *Cluster) generatePrimaryPodDisruptionBudget() *policyv1.PodDisruptionBudget {
 	minAvailable := intstr.FromInt(1)
 	pdbEnabled := c.OpConfig.EnablePodDisruptionBudget
 	pdbMasterLabelSelector := c.OpConfig.PDBMasterLabelSelector
@@ -2230,7 +2230,7 @@ func (c *Cluster) generateGeneralPodDisruptionBudget() *policyv1.PodDisruptionBu
 
 	return &policyv1.PodDisruptionBudget{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:            c.generalPodDisruptionBudgetName(),
+			Name:            c.PrimaryPodDisruptionBudgetName(),
 			Namespace:       c.Namespace,
 			Labels:          c.labelsSet(true),
 			Annotations:     c.annotationsSet(nil),
@@ -2255,7 +2255,7 @@ func (c *Cluster) generateCriticalOpPodDisruptionBudget() *policyv1.PodDisruptio
 	}
 
 	labels := c.labelsSet(false)
-	labels["critical-operaton"] = "true"
+	labels["critical-operation"] = "true"
 
 	return &policyv1.PodDisruptionBudget{
 		ObjectMeta: metav1.ObjectMeta{

pkg/cluster/k8sres_test.go

@@ -2349,7 +2349,7 @@ func TestGeneratePodDisruptionBudget(t *testing.T) {
 		}
 	}
 
-	testLabelsAndSelectors := func(isGeneral bool) func(cluster *Cluster, podDisruptionBudget *policyv1.PodDisruptionBudget) error {
+	testLabelsAndSelectors := func(isPrimary bool) func(cluster *Cluster, podDisruptionBudget *policyv1.PodDisruptionBudget) error {
 		return func(cluster *Cluster, podDisruptionBudget *policyv1.PodDisruptionBudget) error {
 			masterLabelSelectorDisabled := cluster.OpConfig.PDBMasterLabelSelector != nil && !*cluster.OpConfig.PDBMasterLabelSelector
 			if podDisruptionBudget.ObjectMeta.Namespace != "myapp" {
@@ -2360,15 +2360,15 @@ func TestGeneratePodDisruptionBudget(t *testing.T) {
 				return fmt.Errorf("Labels incorrect, got %#v, expected %#v", podDisruptionBudget.Labels, expectedLabels)
 			}
 			if !masterLabelSelectorDisabled {
-				if isGeneral {
+				if isPrimary {
 					expectedLabels := &metav1.LabelSelector{
 						MatchLabels: map[string]string{"spilo-role": "master", "cluster-name": "myapp-database"}}
 					if !reflect.DeepEqual(podDisruptionBudget.Spec.Selector, expectedLabels) {
 						return fmt.Errorf("MatchLabels incorrect, got %#v, expected %#v", podDisruptionBudget.Spec.Selector, expectedLabels)
 					}
 				} else {
 					expectedLabels := &metav1.LabelSelector{
-						MatchLabels: map[string]string{"cluster-name": "myapp-database", "critical-operaton": "true"}}
+						MatchLabels: map[string]string{"cluster-name": "myapp-database", "critical-operation": "true"}}
 					if !reflect.DeepEqual(podDisruptionBudget.Spec.Selector, expectedLabels) {
 						return fmt.Errorf("MatchLabels incorrect, got %#v, expected %#v", podDisruptionBudget.Spec.Selector, expectedLabels)
 					}
@@ -2503,7 +2503,7 @@ func TestGeneratePodDisruptionBudget(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		result := tt.spec.generateGeneralPodDisruptionBudget()
+		result := tt.spec.generatePrimaryPodDisruptionBudget()
 		for _, check := range tt.check {
 			err := check(tt.spec, result)
 			if err != nil {
@@ -2530,7 +2530,7 @@ func TestGeneratePodDisruptionBudget(t *testing.T) {
 				eventRecorder),
 			check: []func(cluster *Cluster, podDisruptionBudget *policyv1.PodDisruptionBudget) error{
 				testPodDisruptionBudgetOwnerReference,
-				hasName("postgres-myapp-database-critical-operation-pdb"),
+				hasName("postgres-myapp-database-critical-op-pdb"),
 				hasMinAvailable(3),
 				testLabelsAndSelectors(false),
 			},
@@ -2547,7 +2547,7 @@ func TestGeneratePodDisruptionBudget(t *testing.T) {
 				eventRecorder),
 			check: []func(cluster *Cluster, podDisruptionBudget *policyv1.PodDisruptionBudget) error{
 				testPodDisruptionBudgetOwnerReference,
-				hasName("postgres-myapp-database-critical-operation-pdb"),
+				hasName("postgres-myapp-database-critical-op-pdb"),
 				hasMinAvailable(0),
 				testLabelsAndSelectors(false),
 			},
@@ -2564,7 +2564,7 @@ func TestGeneratePodDisruptionBudget(t *testing.T) {
 				eventRecorder),
 			check: []func(cluster *Cluster, podDisruptionBudget *policyv1.PodDisruptionBudget) error{
 				testPodDisruptionBudgetOwnerReference,
-				hasName("postgres-myapp-database-critical-operation-pdb"),
+				hasName("postgres-myapp-database-critical-op-pdb"),
 				hasMinAvailable(0),
 				testLabelsAndSelectors(false),
 			},
@@ -2581,7 +2581,7 @@ func TestGeneratePodDisruptionBudget(t *testing.T) {
 				eventRecorder),
 			check: []func(cluster *Cluster, podDisruptionBudget *policyv1.PodDisruptionBudget) error{
 				testPodDisruptionBudgetOwnerReference,
-				hasName("postgres-myapp-database-critical-operation-pdb"),
+				hasName("postgres-myapp-database-critical-op-pdb"),
 				hasMinAvailable(3),
 				testLabelsAndSelectors(false),
 			},

pkg/cluster/resources.go

@@ -23,8 +23,13 @@ const (
 )
 
 func (c *Cluster) listResources() error {
-	if c.GeneralPodDisruptionBudget != nil {
-		c.logger.Infof("found general pod disruption budget: %q (uid: %q)", util.NameFromMeta(c.GeneralPodDisruptionBudget.ObjectMeta), c.GeneralPodDisruptionBudget.UID)
+	if c.PrimaryPodDisruptionBudget != nil {
+		c.logger.Infof("found primary pod disruption budget: %q (uid: %q)", util.NameFromMeta(c.PrimaryPodDisruptionBudget.ObjectMeta), c.PrimaryPodDisruptionBudget.UID)
+	}
+
+	if c.CriticalOpPodDisruptionBudget != nil {
+		c.logger.Infof("found pod disruption budget for critical operations: %q (uid: %q)", util.NameFromMeta(c.CriticalOpPodDisruptionBudget.ObjectMeta), c.CriticalOpPodDisruptionBudget.UID)
+
 	}
 
 	if c.Statefulset != nil {
@@ -417,30 +422,30 @@ func (c *Cluster) generateEndpointSubsets(role PostgresRole) []v1.EndpointSubset
 	return result
 }
 
-func (c *Cluster) createGeneralPodDisruptionBudget() error {
-	c.logger.Debug("creating general pod disruption budget")
-	if c.GeneralPodDisruptionBudget != nil {
-		return fmt.Errorf("general pod disruption budget already exists in the cluster")
+func (c *Cluster) createPrimaryPodDisruptionBudget() error {
+	c.logger.Debug("creating primary pod disruption budget")
+	if c.PrimaryPodDisruptionBudget != nil {
+		return fmt.Errorf("primary pod disruption budget already exists in the cluster")
 	}
 
-	podDisruptionBudgetSpec := c.generateGeneralPodDisruptionBudget()
+	podDisruptionBudgetSpec := c.generatePrimaryPodDisruptionBudget()
 	podDisruptionBudget, err := c.KubeClient.
 		PodDisruptionBudgets(podDisruptionBudgetSpec.Namespace).
 		Create(context.TODO(), podDisruptionBudgetSpec, metav1.CreateOptions{})
 
 	if err != nil {
 		return err
 	}
-	c.logger.Infof("pod disruption budget %q has been successfully created", util.NameFromMeta(podDisruptionBudget.ObjectMeta))
-	c.GeneralPodDisruptionBudget = podDisruptionBudget
+	c.logger.Infof("primary pod disruption budget %q has been successfully created", util.NameFromMeta(podDisruptionBudget.ObjectMeta))
+	c.PrimaryPodDisruptionBudget = podDisruptionBudget
 
 	return nil
 }
 
 func (c *Cluster) createCriticalOpPodDisruptionBudget() error {
 	c.logger.Debug("creating pod disruption budget for critical operations")
 	if c.CriticalOpPodDisruptionBudget != nil {
-		return fmt.Errorf("pod disruption budget  for critical operations already exists in the cluster")
+		return fmt.Errorf("pod disruption budget for critical operations already exists in the cluster")
 	}
 
 	podDisruptionBudgetSpec := c.generateCriticalOpPodDisruptionBudget()
@@ -451,7 +456,7 @@ func (c *Cluster) createCriticalOpPodDisruptionBudget() error {
 	if err != nil {
 		return err
 	}
-	c.logger.Infof("pod disruption budget %q has been successfully created", util.NameFromMeta(podDisruptionBudget.ObjectMeta))
+	c.logger.Infof("pod disruption budget for critical operations %q has been successfully created", util.NameFromMeta(podDisruptionBudget.ObjectMeta))
 	c.CriticalOpPodDisruptionBudget = podDisruptionBudget
 
 	return nil
@@ -460,9 +465,9 @@ func (c *Cluster) createCriticalOpPodDisruptionBudget() error {
 func (c *Cluster) createPodDisruptionBudgets() error {
 	errors := make([]string, 0)
 
-	err := c.createGeneralPodDisruptionBudget()
+	err := c.createPrimaryPodDisruptionBudget()
 	if err != nil {
-		errors = append(errors, fmt.Sprintf("could not create general pod disruption budget: %v", err))
+		errors = append(errors, fmt.Sprintf("could not create primary pod disruption budget: %v", err))
 	}
 
 	err = c.createCriticalOpPodDisruptionBudget()
@@ -476,23 +481,23 @@ func (c *Cluster) createPodDisruptionBudgets() error {
 	return nil
 }
 
-func (c *Cluster) updateGeneralPodDisruptionBudget(pdb *policyv1.PodDisruptionBudget) error {
-	c.logger.Debug("updating general pod disruption budget")
-	if c.GeneralPodDisruptionBudget == nil {
-		return fmt.Errorf("there is no general pod disruption budget in the cluster")
+func (c *Cluster) updatePrimaryPodDisruptionBudget(pdb *policyv1.PodDisruptionBudget) error {
+	c.logger.Debug("updating primary pod disruption budget")
+	if c.PrimaryPodDisruptionBudget == nil {
+		return fmt.Errorf("there is no primary pod disruption budget in the cluster")
 	}
 
-	if err := c.deleteGeneralPodDisruptionBudget(); err != nil {
-		return fmt.Errorf("could not delete general pod disruption budget: %v", err)
+	if err := c.deletePrimaryPodDisruptionBudget(); err != nil {
+		return fmt.Errorf("could not delete primary pod disruption budget: %v", err)
 	}
 
 	newPdb, err := c.KubeClient.
 		PodDisruptionBudgets(pdb.Namespace).
 		Create(context.TODO(), pdb, metav1.CreateOptions{})
 	if err != nil {
-		return fmt.Errorf("could not create general pod disruption budget: %v", err)
+		return fmt.Errorf("could not create primary pod disruption budget: %v", err)
 	}
-	c.GeneralPodDisruptionBudget = newPdb
+	c.PrimaryPodDisruptionBudget = newPdb
 
 	return nil
 }
@@ -518,25 +523,25 @@ func (c *Cluster) updateCriticalOpPodDisruptionBudget(pdb *policyv1.PodDisruptio
 	return nil
 }
 
-func (c *Cluster) deleteGeneralPodDisruptionBudget() error {
-	c.logger.Debug("deleting general pod disruption budget")
-	if c.GeneralPodDisruptionBudget == nil {
-		c.logger.Debug("there is no general pod disruption budget in the cluster")
+func (c *Cluster) deletePrimaryPodDisruptionBudget() error {
+	c.logger.Debug("deleting primary pod disruption budget")
+	if c.PrimaryPodDisruptionBudget == nil {
+		c.logger.Debug("there is no primary pod disruption budget in the cluster")
 		return nil
 	}
 
-	pdbName := util.NameFromMeta(c.GeneralPodDisruptionBudget.ObjectMeta)
+	pdbName := util.NameFromMeta(c.PrimaryPodDisruptionBudget.ObjectMeta)
 	err := c.KubeClient.
-		PodDisruptionBudgets(c.GeneralPodDisruptionBudget.Namespace).
-		Delete(context.TODO(), c.GeneralPodDisruptionBudget.Name, c.deleteOptions)
+		PodDisruptionBudgets(c.PrimaryPodDisruptionBudget.Namespace).
+		Delete(context.TODO(), c.PrimaryPodDisruptionBudget.Name, c.deleteOptions)
 	if k8sutil.ResourceNotFound(err) {
-		c.logger.Debugf("PodDisruptionBudget %q has already been deleted", util.NameFromMeta(c.GeneralPodDisruptionBudget.ObjectMeta))
+		c.logger.Debugf("PodDisruptionBudget %q has already been deleted", util.NameFromMeta(c.PrimaryPodDisruptionBudget.ObjectMeta))
 	} else if err != nil {
-		return fmt.Errorf("could not delete general pod disruption budget: %v", err)
+		return fmt.Errorf("could not delete primary pod disruption budget: %v", err)
 	}
 
-	c.logger.Infof("pod disruption budget %q has been deleted", util.NameFromMeta(c.GeneralPodDisruptionBudget.ObjectMeta))
-	c.GeneralPodDisruptionBudget = nil
+	c.logger.Infof("pod disruption budget %q has been deleted", util.NameFromMeta(c.PrimaryPodDisruptionBudget.ObjectMeta))
+	c.PrimaryPodDisruptionBudget = nil
 
 	err = retryutil.Retry(c.OpConfig.ResourceCheckInterval, c.OpConfig.ResourceCheckTimeout,
 		func() (bool, error) {
@@ -550,7 +555,7 @@ func (c *Cluster) deleteGeneralPodDisruptionBudget() error {
 			return false, err2
 		})
 	if err != nil {
-		return fmt.Errorf("could not delete general pod disruption budget: %v", err)
+		return fmt.Errorf("could not delete primary pod disruption budget: %v", err)
 	}
 
 	return nil
@@ -597,7 +602,7 @@ func (c *Cluster) deleteCriticalOpPodDisruptionBudget() error {
 func (c *Cluster) deletePodDisruptionBudgets() error {
 	errors := make([]string, 0)
 
-	if err := c.deleteGeneralPodDisruptionBudget(); err != nil {
+	if err := c.deletePrimaryPodDisruptionBudget(); err != nil {
 		errors = append(errors, fmt.Sprintf("%v", err))
 	}
 
@@ -827,12 +832,12 @@ func (c *Cluster) GetStatefulSet() *appsv1.StatefulSet {
 	return c.Statefulset
 }
 
-// GetPodDisruptionBudget returns cluster's general kubernetes PodDisruptionBudget
-func (c *Cluster) GetGeneralPodDisruptionBudget() *policyv1.PodDisruptionBudget {
-	return c.GeneralPodDisruptionBudget
+// GetPrimaryPodDisruptionBudget returns cluster's primary kubernetes PodDisruptionBudget
+func (c *Cluster) GetPrimaryPodDisruptionBudget() *policyv1.PodDisruptionBudget {
+	return c.PrimaryPodDisruptionBudget
 }
 
-// GetPodDisruptionBudget returns cluster's kubernetes PodDisruptionBudget for critical operations
+// GetCriticalOpPodDisruptionBudget returns cluster's kubernetes PodDisruptionBudget for critical operations
 func (c *Cluster) GetCriticalOpPodDisruptionBudget() *policyv1.PodDisruptionBudget {
 	return c.CriticalOpPodDisruptionBudget
 }