K8SPG-648 Fix patroni-version-check (#1071)

pooknull · jvpasinatto · commit 0381fdb9d6e4 · 2025-03-07T17:21:54.000-03:00
diff --git a/build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml b/build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml
@@ -17525,6 +17525,8 @@ spec:
                 type: object
               postgres:
                 properties:
+                  imageID:
+                    type: string
                   instances:
                     items:
                       properties:
diff --git a/config/crd/bases/pgv2.percona.com_perconapgclusters.yaml b/config/crd/bases/pgv2.percona.com_perconapgclusters.yaml
@@ -17930,6 +17930,8 @@ spec:
                 type: object
               postgres:
                 properties:
+                  imageID:
+                    type: string
                   instances:
                     items:
                       properties:
diff --git a/deploy/bundle.yaml b/deploy/bundle.yaml
@@ -18223,6 +18223,8 @@ spec:
                 type: object
               postgres:
                 properties:
+                  imageID:
+                    type: string
                   instances:
                     items:
                       properties:
diff --git a/deploy/crd.yaml b/deploy/crd.yaml
@@ -18223,6 +18223,8 @@ spec:
                 type: object
               postgres:
                 properties:
+                  imageID:
+                    type: string
                   instances:
                     items:
                       properties:
diff --git a/deploy/cw-bundle.yaml b/deploy/cw-bundle.yaml
@@ -18223,6 +18223,8 @@ spec:
                 type: object
               postgres:
                 properties:
+                  imageID:
+                    type: string
                   instances:
                     items:
                       properties:
diff --git a/percona/controller/pgcluster/controller.go b/percona/controller/pgcluster/controller.go
@@ -294,7 +294,7 @@ func (r *PGClusterReconciler) Reconcile(ctx context.Context, request reconcile.R
 	}
 
 	var opRes controllerutil.OperationResult
-	if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+	if err := retry.OnError(retry.DefaultRetry, func(err error) bool { return err != nil }, func() error {
 		var err error
 		opRes, err = controllerutil.CreateOrUpdate(ctx, r.Client, postgresCluster, func() error {
 			var err error
@@ -334,7 +334,44 @@ func (r *PGClusterReconciler) reconcilePatroniVersionCheck(ctx context.Context,
 		cr.Annotations = make(map[string]string)
 	}
 
-	if cr.Status.Postgres.Version == cr.Spec.PostgresVersion && cr.Status.PatroniVersion != "" {
+	// This annotation is used for unit-tests only. Allows to skip the patroni version check
+	if _, ok := cr.Annotations[pNaming.InternalAnnotationDisablePatroniVersionCheck]; ok {
+		cr.Annotations[pNaming.AnnotationPatroniVersion] = cr.Status.PatroniVersion
+		return nil
+	}
+
+	getImageIDFromPod := func(pod *corev1.Pod, containerName string) string {
+		idx := slices.IndexFunc(pod.Status.ContainerStatuses, func(s corev1.ContainerStatus) bool {
+			return s.Name == containerName
+		})
+		if idx == -1 {
+			return ""
+		}
+		return pod.Status.ContainerStatuses[idx].ImageID
+	}
+
+	pods := new(corev1.PodList)
+	instances, err := naming.AsSelector(naming.ClusterInstances(cr.Name))
+	if err != nil {
+		return err
+	}
+	if err = r.Client.List(ctx, pods, client.InNamespace(cr.Namespace), client.MatchingLabelsSelector{Selector: instances}); err != nil {
+		return err
+	}
+
+	// Collecting all image IDs from instance pods. Under normal conditions, this slice will contain a single image ID, as all pods typically use the same image.
+	// During an image update, it may contain multiple different image IDs as the update progresses.
+	imageIDs := []string{}
+	for _, pod := range pods.Items {
+		imageID := getImageIDFromPod(&pod, naming.ContainerDatabase)
+		if imageID != "" && !slices.Contains(imageIDs, imageID) {
+			imageIDs = append(imageIDs, imageID)
+		}
+	}
+
+	// If the imageIDs slice contains the imageID from the status, we skip checking the Patroni version.
+	// This ensures that the Patroni version is only checked after all pods have been updated.
+	if slices.Contains(imageIDs, cr.Status.Postgres.ImageID) && cr.Status.PatroniVersion != "" {
 		cr.Annotations[pNaming.AnnotationPatroniVersion] = cr.Status.PatroniVersion
 		return nil
 	}
@@ -348,7 +385,7 @@ func (r *PGClusterReconciler) reconcilePatroniVersionCheck(ctx context.Context,
 		ObjectMeta: meta,
 	}
 
-	err := r.Client.Get(ctx, client.ObjectKeyFromObject(p), p)
+	err = r.Client.Get(ctx, client.ObjectKeyFromObject(p), p)
 	if client.IgnoreNotFound(err) != nil {
 		return errors.Wrap(err, "failed to get patroni version check pod")
 	}
@@ -418,6 +455,7 @@ func (r *PGClusterReconciler) reconcilePatroniVersionCheck(ctx context.Context,
 
 	cr.Status.PatroniVersion = patroniVersion
 	cr.Status.Postgres.Version = cr.Spec.PostgresVersion
+	cr.Status.Postgres.ImageID = getImageIDFromPod(p, pNaming.ContainerPatroniVersionCheck)
 
 	if err := r.Client.Status().Patch(ctx, cr.DeepCopy(), client.MergeFrom(orig)); err != nil {
 		return errors.Wrap(err, "failed to patch patroni version")
diff --git a/percona/controller/pgcluster/controller_test.go b/percona/controller/pgcluster/controller_test.go
@@ -113,7 +113,6 @@ var _ = Describe("Annotations", Ordered, func() {
 	})
 
 	It("should create PerconaPGCluster with annotations", func() {
-		cr.Annotations = make(map[string]string)
 		cr.Annotations["pgv2.percona.com/trigger-switchover"] = "true"
 		cr.Annotations["egedemo.com/test"] = "true"
 
diff --git a/percona/controller/pgcluster/testutils_test.go b/percona/controller/pgcluster/testutils_test.go
@@ -21,6 +21,7 @@ import (
 	"github.com/percona/percona-postgresql-operator/internal/controller/postgrescluster"
 	"github.com/percona/percona-postgresql-operator/internal/naming"
 	"github.com/percona/percona-postgresql-operator/percona/controller/pgbackup"
+	pNaming "github.com/percona/percona-postgresql-operator/percona/naming"
 	"github.com/percona/percona-postgresql-operator/percona/utils/registry"
 	"github.com/percona/percona-postgresql-operator/percona/watcher"
 	v2 "github.com/percona/percona-postgresql-operator/pkg/apis/pgv2.percona.com/v2"
@@ -94,6 +95,10 @@ func readTestCR(name, namespace, testFile string) (*v2.PerconaPGCluster, error)
 	if cr.Spec.PostgresVersion == 0 {
 		return nil, errors.New("postgresVersion should be specified")
 	}
+	if cr.Annotations == nil {
+		cr.Annotations = make(map[string]string)
+	}
+	cr.Annotations[pNaming.InternalAnnotationDisablePatroniVersionCheck] = "true"
 	cr.Status.Postgres.Version = cr.Spec.PostgresVersion
 	cr.Status.PatroniVersion = "4.0.0"
 	return cr, nil
@@ -112,6 +117,10 @@ func readDefaultCR(name, namespace string) (*v2.PerconaPGCluster, error) {
 	}
 
 	cr.Name = name
+	if cr.Annotations == nil {
+		cr.Annotations = make(map[string]string)
+	}
+	cr.Annotations[pNaming.InternalAnnotationDisablePatroniVersionCheck] = "true"
 	cr.Namespace = namespace
 	cr.Status.Postgres.Version = cr.Spec.PostgresVersion
 	cr.Status.PatroniVersion = "4.0.0"
diff --git a/percona/naming/annotations.go b/percona/naming/annotations.go
@@ -39,4 +39,7 @@ const (
 	AnnotationClusterBootstrapRestore = PrefixPerconaPGV2 + "cluster-bootstrap-restore"
 
 	AnnotationPatroniVersion = PrefixPerconaPGV2 + "patroni-version"
+
+	// Should be used only for unit-testing
+	InternalAnnotationDisablePatroniVersionCheck = PrefixPerconaInternal + "patroni-version-check-disable"
 )
diff --git a/percona/watcher/wal.go b/percona/watcher/wal.go
@@ -61,7 +61,7 @@ func WatchCommitTimestamps(ctx context.Context, cli client.Client, eventChan cha
 				return
 			}
 
-			latestBackup, err := getLatestBackup(ctx, cli, cr)
+			latestBackup, err := getLatestBackup(ctx, cli, localCr)
 			if err != nil {
 				if !errors.Is(err, errRunningBackup) && !errors.Is(err, errNoBackups) {
 					log.Error(err, "get latest backup")
@@ -70,7 +70,7 @@ func WatchCommitTimestamps(ctx context.Context, cli client.Client, eventChan cha
 				continue
 			}
 
-			ts, err := GetLatestCommitTimestamp(ctx, cli, execCli, cr, latestBackup)
+			ts, err := GetLatestCommitTimestamp(ctx, cli, execCli, localCr, latestBackup)
 			if err != nil {
 				switch {
 				case errors.Is(err, PrimaryPodNotFound) && localCr.Status.State != pgv2.AppStateReady:
diff --git a/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go b/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go
@@ -401,6 +401,9 @@ type PostgresStatus struct {
 
 	// +optional
 	Version int `json:"version"`
+
+	// +optional
+	ImageID string `json:"imageID"`
 }
 
 type PGBouncerStatus struct {

Original file line number	Diff line number	Diff line change
`@@ -39,4 +39,7 @@ const (`
`39`	`39`	`AnnotationClusterBootstrapRestore = PrefixPerconaPGV2 + "cluster-bootstrap-restore"`
`40`	`40`
`41`	`41`	`AnnotationPatroniVersion = PrefixPerconaPGV2 + "patroni-version"`
	`42`	`+`
	`43`	`+ // Should be used only for unit-testing`
	`44`	`+ InternalAnnotationDisablePatroniVersionCheck = PrefixPerconaInternal + "patroni-version-check-disable"`
`42`	`45`	`)`
Original file line number	Diff line number	Diff line change
`@@ -401,6 +401,9 @@ type PostgresStatus struct {`
`401`	`401`
`402`	`402`	`// +optional`
`403`	`403`	Version int `json:"version"`
	`404`	`+`
	`405`	`+ // +optional`
	`406`	+ ImageID string `json:"imageID"`
`404`	`407`	`}`
`405`	`408`
`406`	`409`	`type PGBouncerStatus struct {`