K8SPSMDB-1493: fix stuck backups when smart update is in progress (#2143)

* fix backup/smart-upgrde deadlock

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

* add comments

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

* fix unit test

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

* unit test

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

* unit tests for conditions

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

* linting

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

* remove debug statements

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

* linting

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

* Update pkg/controller/perconaservermongodb/status_test.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update pkg/apis/psmdb/v1/psmdb_types.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix test semantics

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

* wrap error

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

---------

Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Ege Güneş <ege.gunes@percona.com>

Author: 3 people

pkg/apis/psmdb/v1/condition_test.go

@@ -0,0 +1,48 @@
+package v1
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestConditions(t *testing.T) {
+	status := &PerconaServerMongoDBStatus{}
+
+	cond := status.FindCondition(AppStateReady)
+	assert.Nil(t, cond)
+
+	status.AddCondition(ClusterCondition{
+		Type:    AppStateReady,
+		Status:  ConditionTrue,
+		Reason:  "ClusterReady",
+		Message: "Cluster is ready",
+	})
+	cond = status.FindCondition(AppStateReady)
+	assert.NotNil(t, cond)
+	assert.Equal(t, ConditionTrue, cond.Status)
+	assert.Equal(t, "ClusterReady", cond.Reason)
+	assert.Equal(t, "Cluster is ready", cond.Message)
+	lastTransitionTime := cond.LastTransitionTime
+	assert.NotNil(t, lastTransitionTime)
+	assert.True(t, status.IsStatusConditionTrue(AppStateReady))
+
+	status.AddCondition(ClusterCondition{
+		Type:    AppStateReady,
+		Status:  ConditionFalse,
+		Reason:  "ClusterNotReady",
+		Message: "Cluster is not ready",
+	})
+	cond = status.FindCondition(AppStateReady)
+	assert.NotNil(t, cond)
+	assert.Equal(t, ConditionFalse, cond.Status)
+	assert.Equal(t, "ClusterNotReady", cond.Reason)
+	assert.Equal(t, "Cluster is not ready", cond.Message)
+	assert.NotNil(t, cond.LastTransitionTime)
+	assert.True(t, cond.LastTransitionTime.After(lastTransitionTime.Time))
+	assert.False(t, status.IsStatusConditionTrue(AppStateReady))
+
+	status.RemoveCondition(AppStateReady)
+	assert.False(t, status.IsStatusConditionTrue(AppStateReady))
+	assert.Nil(t, status.FindCondition(AppStateReady))
+}

pkg/apis/psmdb/v1/psmdb_types.go

@@ -339,6 +339,10 @@ const (
 	ConditionUnknown ConditionStatus = "Unknown"
 )
 
+// ConditionTypePendingSmartUpdate is a condition type set on PSMDBCluster when a smart update is required
+// but has not yet started. For e.g., if a backup/restore is running at the same time as a smart update is triggered.
+const ConditionTypePendingSmartUpdate AppState = "pendingSmartUpdate"
+
 type ClusterCondition struct {
 	Status             ConditionStatus `json:"status"`
 	Type               AppState        `json:"type"`
@@ -357,6 +361,14 @@ func (s *PerconaServerMongoDBStatus) FindCondition(conditionType AppState) *Clus
 	return nil
 }
 
+func (s *PerconaServerMongoDBStatus) IsStatusConditionTrue(conditionType AppState) bool {
+	cond := s.FindCondition(conditionType)
+	if cond == nil {
+		return false
+	}
+	return cond.Status == ConditionTrue
+}
+
 type PMMSpec struct {
 	Enabled                  bool                    `json:"enabled,omitempty"`
 	ServerHost               string                  `json:"serverHost,omitempty"`
@@ -1386,6 +1398,10 @@ func (cr *PerconaServerMongoDB) CanBackup(ctx context.Context) error {
 		return nil
 	}
 
+	if cr.Status.State == AppStateInit && cr.Status.IsStatusConditionTrue(ConditionTypePendingSmartUpdate) {
+		return nil
+	}
+
 	if cr.CompareVersion("1.15.0") <= 0 && !cr.Spec.UnsafeConf {
 		return errors.Errorf("allowUnsafeConfigurations must be true to run backup on cluster with status %s", cr.Status.State)
 	}
@@ -1414,6 +1430,15 @@ func (cr *PerconaServerMongoDB) CanRestore(ctx context.Context) error {
 	return nil
 }
 
+func (s *PerconaServerMongoDBStatus) RemoveCondition(conditionType AppState) {
+	for i, c := range s.Conditions {
+		if c.Type == conditionType {
+			s.Conditions = append(s.Conditions[:i], s.Conditions[i+1:]...)
+			return
+		}
+	}
+}
+
 func (s *PerconaServerMongoDBStatus) AddCondition(c ClusterCondition) {
 	existingCondition := s.FindCondition(c.Type)
 	if existingCondition == nil {

pkg/controller/perconaservermongodb/connections_test.go

@@ -355,6 +355,9 @@ func fakeStatefulset(cr *api.PerconaServerMongoDB, rs *api.ReplsetSpec, size int
 		},
 		Spec: appsv1.StatefulSetSpec{
 			Replicas: &size,
+			Selector: &metav1.LabelSelector{
+				MatchLabels: ls,
+			},
 			Template: corev1.PodTemplateSpec{
 				Spec: corev1.PodSpec{
 					Containers: []corev1.Container{

pkg/controller/perconaservermongodb/status.go

@@ -233,11 +233,61 @@ func (r *ReconcilePerconaServerMongoDB) updateStatus(ctx context.Context, cr *ap
 	clusterCondition.Type = cr.Status.State
 	cr.Status.AddCondition(clusterCondition)
 
+	cr.Status.RemoveCondition(api.ConditionTypePendingSmartUpdate)
+	if awaiting, err := r.isAwaitingSmartUpdate(ctx, cr); err != nil {
+		return errors.Wrap(err, "cannot check if awaiting smart update")
+	} else if cr.Status.Ready == cr.Status.Size && awaiting {
+		cr.Status.AddCondition(api.ClusterCondition{
+			Type:    api.ConditionTypePendingSmartUpdate,
+			Status:  api.ConditionTrue,
+			Message: "Smart update is pending but has not yet started",
+		})
+	}
+
 	cr.Status.ObservedGeneration = cr.ObjectMeta.Generation
 
 	return r.writeStatus(ctx, cr)
 }
 
+// isAwaitingSmartUpdate returns true if the cluster smart update is pending but has not yet started.
+func (r *ReconcilePerconaServerMongoDB) isAwaitingSmartUpdate(ctx context.Context, cr *api.PerconaServerMongoDB) (bool, error) {
+	if cr.Spec.UpdateStrategy != api.SmartUpdateStatefulSetStrategyType {
+		return false, nil
+	}
+
+	statefulSets := make([]string, 0, len(cr.Spec.Replsets)+2)
+	for _, rs := range cr.Spec.Replsets {
+		statefulSets = append(statefulSets, cr.StatefulsetNamespacedName(rs.Name).Name)
+	}
+	if cr.Spec.Sharding.Enabled {
+		statefulSets = append(statefulSets, cr.StatefulsetNamespacedName(api.ConfigReplSetName).Name)
+		statefulSets = append(statefulSets, cr.MongosNamespacedName().Name)
+	}
+
+	// count the number of updated pods from statefulsets that have changed
+	var updated int32
+	stsChanged := false // true if any statefulset has changed
+	for _, name := range statefulSets {
+		sts := &appsv1.StatefulSet{}
+		if err := r.client.Get(ctx, types.NamespacedName{Name: name, Namespace: cr.Namespace}, sts); err != nil {
+			return false, client.IgnoreNotFound(err)
+		}
+
+		pods := corev1.PodList{}
+		if err := r.client.List(ctx, &pods, &client.ListOptions{
+			Namespace:     cr.Namespace,
+			LabelSelector: labels.SelectorFromSet(sts.Spec.Selector.MatchLabels),
+		}); err != nil {
+			return false, errors.Wrap(err, "get pod list")
+		}
+		if isSfsChanged(sts, &pods) {
+			updated += sts.Status.UpdatedReplicas
+			stsChanged = true
+		}
+	}
+	return stsChanged && updated == 0, nil
+}
+
 func (r *ReconcilePerconaServerMongoDB) upgradeInProgress(ctx context.Context, cr *api.PerconaServerMongoDB, rsName string) (bool, error) {
 	sfsObj := &appsv1.StatefulSet{}
 	err := r.client.Get(ctx, types.NamespacedName{Name: cr.Name + "-" + rsName, Namespace: cr.Namespace}, sfsObj)

pkg/controller/perconaservermongodb/status_test.go

@@ -17,6 +17,7 @@ import (
 	fakeBackup "github.com/percona/percona-server-mongodb-operator/pkg/psmdb/backup/fake"
 	faketls "github.com/percona/percona-server-mongodb-operator/pkg/psmdb/tls/fake"
 	"github.com/percona/percona-server-mongodb-operator/pkg/version"
+	"github.com/stretchr/testify/assert"
 )
 
 // creates a fake client to mock API calls with the mock objects
@@ -323,3 +324,127 @@ func fakeSvc(name, namespace string, svcType corev1.ServiceType, ip, hostname st
 		},
 	}
 }
+
+func TestIsAwaitingSmartUpdate(t *testing.T) {
+	ctx := t.Context()
+	cr := &api.PerconaServerMongoDB{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "psmdb-mock",
+			Namespace:  "psmdb",
+			Generation: 1,
+		},
+		Spec: api.PerconaServerMongoDBSpec{
+			Backup: api.BackupSpec{
+				Enabled: false,
+			},
+			CRVersion: version.Version(),
+			Image:     "percona/percona-server-mongodb:latest",
+			Replsets: []*api.ReplsetSpec{
+				{
+					Name:       "rs0",
+					Size:       3,
+					VolumeSpec: fakeVolumeSpec(t),
+				},
+			},
+			UpdateStrategy: api.SmartUpdateStatefulSetStrategyType,
+			UpgradeOptions: api.UpgradeOptions{
+				SetFCV: true,
+			},
+			Sharding: api.Sharding{Enabled: false},
+		},
+		Status: api.PerconaServerMongoDBStatus{
+			MongoVersion:       "4.2",
+			ObservedGeneration: 1,
+			State:              api.AppStateReady,
+			MongoImage:         "percona/percona-server-mongodb:4.0",
+		},
+	}
+	sts := fakeStatefulset(cr, cr.Spec.Replsets[0], cr.Spec.Replsets[0].Size, "some-revision", "mongod")
+	pods := fakePodsForRS(cr, cr.Spec.Replsets[0])
+
+	testCases := []struct {
+		desc     string
+		expected bool
+		mock     func(cl client.Client) error
+		cluster  *api.PerconaServerMongoDB
+	}{
+		{
+			desc:     "smart update is disabled",
+			expected: false,
+			cluster: updateResource(cr.DeepCopy(), func(cr *api.PerconaServerMongoDB) {
+				cr.Spec.UpdateStrategy = ""
+			}),
+		},
+		{
+			desc:     "statefulset has not changed",
+			expected: false,
+			mock: func(cl client.Client) error {
+				for _, pod := range pods {
+					labels := pod.GetLabels()
+					labels["controller-revision-hash"] = "some-revision"
+					pod.SetLabels(labels)
+					if err := cl.Update(ctx, pod); err != nil {
+						return err
+					}
+				}
+				return nil
+			},
+			cluster: cr.DeepCopy(),
+		},
+		{
+			desc:     "statefulset has changed, no pods updated",
+			expected: true,
+			mock: func(cl client.Client) error {
+				for _, pod := range pods {
+					labels := pod.GetLabels()
+					labels["controller-revision-hash"] = "previous-revision"
+					pod.SetLabels(labels)
+					if err := cl.Update(ctx, pod); err != nil {
+						return err
+					}
+				}
+				fakeSts := sts.DeepCopyObject().(*appsv1.StatefulSet)
+				fakeSts.Status.UpdatedReplicas = 0
+				return cl.Status().Update(ctx, fakeSts)
+			},
+			cluster: cr.DeepCopy(),
+		},
+		{
+			desc:     "statefulset has changed, some pods updated",
+			expected: false,
+			mock: func(cl client.Client) error {
+				outdatedPod := pods[2]
+
+				labels := outdatedPod.GetLabels()
+				labels["controller-revision-hash"] = "previous-revision"
+				outdatedPod.SetLabels(labels)
+				if err := cl.Update(ctx, outdatedPod); err != nil {
+					return err
+				}
+				fakeSts := sts.DeepCopyObject().(*appsv1.StatefulSet)
+				fakeSts.Status.UpdatedReplicas = 2
+				return cl.Status().Update(ctx, fakeSts)
+			},
+			cluster: cr.DeepCopy(),
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			// Setup mocks
+			objs := []client.Object{}
+			objs = append(objs, tc.cluster, sts)
+			objs = append(objs, pods...)
+			r := buildFakeClient(objs...)
+			if tc.mock != nil {
+				err := tc.mock(r.client)
+				assert.NoError(t, err)
+			}
+
+			actual, err := r.isAwaitingSmartUpdate(ctx, tc.cluster)
+			assert.NoError(t, err)
+			assert.Equal(t, tc.expected, actual)
+		})
+
+	}
+}