refactor: improve metadata fencing code organization (cloudnative-pg#…

…4152) Currently, the metadata fencing code is challenging for inexperienced developers to use, given that it is fragmented across the codebase without a clear pattern. This patch aggregates the logic and adds a fluent builder to execute the fencing metadata logic. It also improves handling certain operations like fencing or unfencing an already present or deleted instance. In those cases, the executor does not return an error anymore. Signed-off-by: Armando Ruocco <[email protected]> Signed-off-by: Leonardo Cecchi <[email protected]> Co-authored-by: Leonardo Cecchi <[email protected]>
EnterpriseDB · Apr 10, 2024 · 570787f · 570787f
1 parent 182157e
commit 570787f
Show file tree

Hide file tree

Showing 13 changed files with 197 additions and 197 deletions.
diff --git a/api/v1/cluster_types.go b/api/v1/cluster_types.go
@@ -2998,7 +2998,7 @@ func (cluster *Cluster) IsInstanceFenced(instance string) bool {
 		return false
 	}
 
-	if fencedInstances.Has(utils.FenceAllServers) {
+	if fencedInstances.Has(utils.FenceAllInstances) {
 		return true
 	}
 	return fencedInstances.Has(instance)

diff --git a/controllers/cluster_restore.go b/controllers/cluster_restore.go
@@ -104,22 +104,10 @@ func ensureClusterIsNotFenced(
 	cli client.Client,
 	cluster *apiv1.Cluster,
 ) error {
-	fencedInstances, err := utils.GetFencedInstances(cluster.Annotations)
-	if err != nil {
-		return err
-	}
-	if fencedInstances.Len() == 0 {
-		return nil
-	}
-
-	clusterOrig := cluster.DeepCopy()
-
-	// we remove the fenced instances this way to ensure that the patch method will work
-	if err := utils.RemoveFencedInstance(utils.FenceAllServers, &cluster.ObjectMeta); err != nil {
-		return err
-	}
-
-	return cli.Patch(ctx, cluster, client.MergeFrom(clusterOrig))
+	return utils.NewFencingMetadataExecutor(cli).
+		RemoveFencing().
+		ForAllInstances().
+		Execute(ctx, client.ObjectKeyFromObject(cluster), cluster)
 }
 
 // restoreClusterStatus bootstraps the status needed to make the restored cluster work

diff --git a/controllers/cluster_restore_test.go b/controllers/cluster_restore_test.go
@@ -81,8 +81,9 @@ var _ = Describe("ensureClusterIsNotFenced", func() {
 
 	Context("when fenced instances exist", func() {
 		BeforeEach(func() {
-			err := utils.AddFencedInstance(utils.FenceAllServers, &cluster.ObjectMeta)
+			modified, err := utils.AddFencedInstance(utils.FenceAllInstances, &cluster.ObjectMeta)
 			Expect(err).ToNot(HaveOccurred())
+			Expect(modified).To(BeTrue())
 			mockCli = fake.NewClientBuilder().
 				WithScheme(k8scheme.BuildWithAllKnownScheme()).
 				WithObjects(cluster).

diff --git a/internal/cmd/plugin/fence/fence.go b/internal/cmd/plugin/fence/fence.go
@@ -21,14 +21,22 @@ import (
 	"context"
 	"fmt"
 
+	"k8s.io/apimachinery/pkg/types"
+
+	apiv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1"
 	"github.com/cloudnative-pg/cloudnative-pg/internal/cmd/plugin"
-	"github.com/cloudnative-pg/cloudnative-pg/pkg/resources"
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/utils"
 )
 
 // fencingOn marks an instance in a cluster as fenced
 func fencingOn(ctx context.Context, clusterName string, serverName string) error {
-	err := resources.ApplyFenceFunc(ctx, plugin.Client, clusterName, plugin.Namespace, serverName, utils.AddFencedInstance)
+	err := utils.NewFencingMetadataExecutor(plugin.Client).
+		AddFencing().
+		ForInstance(serverName).
+		Execute(ctx,
+			types.NamespacedName{Name: clusterName, Namespace: plugin.Namespace},
+			&apiv1.Cluster{},
+		)
 	if err != nil {
 		return err
 	}
@@ -38,8 +46,13 @@ func fencingOn(ctx context.Context, clusterName string, serverName string) error
 
 // fencingOff marks an instance in a cluster as not fenced
 func fencingOff(ctx context.Context, clusterName string, serverName string) error {
-	err := resources.ApplyFenceFunc(ctx, plugin.Client, clusterName, plugin.Namespace,
-		serverName, utils.RemoveFencedInstance)
+	err := utils.NewFencingMetadataExecutor(plugin.Client).
+		RemoveFencing().
+		ForInstance(serverName).
+		Execute(ctx,
+			types.NamespacedName{Name: clusterName, Namespace: plugin.Namespace},
+			&apiv1.Cluster{},
+		)
 	if err != nil {
 		return err
 	}

diff --git a/internal/cmd/plugin/hibernate/on.go b/internal/cmd/plugin/hibernate/on.go
@@ -177,14 +177,14 @@ func (on *onCommand) fenceClusterStep() error {
 	contextLogger := log.FromContext(on.ctx)
 
 	contextLogger.Debug("applying the fencing annotation to the cluster manifest")
-	if err := resources.ApplyFenceFunc(
-		on.ctx,
-		plugin.Client,
-		on.cluster.Name,
-		plugin.Namespace,
-		utils.FenceAllServers,
-		utils.AddFencedInstance,
-	); err != nil {
+	if err := utils.NewFencingMetadataExecutor(plugin.Client).
+		AddFencing().
+		ForAllInstances().
+		Execute(
+			on.ctx,
+			types.NamespacedName{Name: on.cluster.Name, Namespace: plugin.Namespace},
+			&apiv1.Cluster{},
+		); err != nil {
 		return err
 	}
 	contextLogger.Debug("fencing annotation set on the cluster manifest")
@@ -202,15 +202,11 @@ func (on *onCommand) rollbackFenceClusterIfNeeded() {
 	contextLogger := log.FromContext(on.ctx)
 
 	fmt.Println("rolling back hibernation: removing the fencing annotation")
-	err := resources.ApplyFenceFunc(
-		on.ctx,
-		plugin.Client,
-		on.cluster.Name,
-		plugin.Namespace,
-		utils.FenceAllServers,
-		utils.RemoveFencedInstance,
-	)
-	if err != nil {
+	if err := utils.NewFencingMetadataExecutor(plugin.Client).
+		RemoveFencing().
+		ForAllInstances().
+		Execute(on.ctx,
+			types.NamespacedName{Name: on.cluster.Name, Namespace: plugin.Namespace}, &apiv1.Cluster{}); err != nil {
 		contextLogger.Error(err, "Rolling back from hibernation failed")
 	}
 }

diff --git a/internal/cmd/plugin/status/status.go b/internal/cmd/plugin/status/status.go
@@ -158,7 +158,7 @@ func ExtractPostgresqlStatus(ctx context.Context, clusterName string) (*Postgres
 }
 
 func listFencedInstances(fencedInstances *stringset.Data) string {
-	if fencedInstances.Has(utils.FenceAllServers) {
+	if fencedInstances.Has(utils.FenceAllInstances) {
 		return "All Instances"
 	}
 	return strings.Join(fencedInstances.ToList(), ", ")

diff --git a/pkg/reconciler/backup/volumesnapshot/offline.go b/pkg/reconciler/backup/volumesnapshot/offline.go
@@ -31,7 +31,6 @@ import (
 
 	apiv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1"
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/management/log"
-	"github.com/cloudnative-pg/cloudnative-pg/pkg/resources"
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/utils"
 )
 
@@ -124,19 +123,10 @@ func (o *offlineExecutor) ensurePodIsFenced(
 			"targetBackup", backup.Name, "targetPod", targetPodName,
 		)
 	}
-
-	err = resources.ApplyFenceFunc(
-		ctx,
-		o.cli,
-		cluster.Name,
-		cluster.Namespace,
-		targetPodName,
-		utils.AddFencedInstance,
-	)
-	if errors.Is(err, utils.ErrorServerAlreadyFenced) {
-		return nil
-	}
-	if err != nil {
+	if err = utils.NewFencingMetadataExecutor(o.cli).
+		AddFencing().
+		ForInstance(targetPodName).
+		Execute(ctx, client.ObjectKeyFromObject(cluster), cluster); err != nil {
 		return err
 	}
 

diff --git a/pkg/reconciler/backup/volumesnapshot/offline_test.go b/pkg/reconciler/backup/volumesnapshot/offline_test.go
@@ -115,8 +115,9 @@ var _ = Describe("offlineExecutor", func() {
 	})
 
 	It("finalize should remove the fencing annotation from the cluster", func(ctx SpecContext) {
-		err := utils.AddFencedInstance(pod.Name, &cluster.ObjectMeta)
+		modified, err := utils.AddFencedInstance(pod.Name, &cluster.ObjectMeta)
 		Expect(err).ToNot(HaveOccurred())
+		Expect(modified).To(BeTrue())
 
 		err = oe.cli.Update(ctx, cluster)
 		Expect(err).ToNot(HaveOccurred())

diff --git a/pkg/reconciler/backup/volumesnapshot/reconciler.go b/pkg/reconciler/backup/volumesnapshot/reconciler.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"strconv"
 	"time"
@@ -37,7 +36,6 @@ import (
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/management/log"
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/management/postgres"
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/reconciler/persistentvolumeclaim"
-	"github.com/cloudnative-pg/cloudnative-pg/pkg/resources"
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/resources/instance"
 	"github.com/cloudnative-pg/cloudnative-pg/pkg/utils"
 )
@@ -346,18 +344,10 @@ func EnsurePodIsUnfenced(
 ) error {
 	contextLogger := log.FromContext(ctx)
 
-	err := resources.ApplyFenceFunc(
-		ctx,
-		cli,
-		cluster.Name,
-		cluster.Namespace,
-		targetPod.Name,
-		utils.RemoveFencedInstance,
-	)
-	if errors.Is(err, utils.ErrorServerAlreadyUnfenced) {
-		return nil
-	}
-	if err != nil {
+	if err := utils.NewFencingMetadataExecutor(cli).
+		RemoveFencing().
+		ForInstance(targetPod.Name).
+		Execute(ctx, client.ObjectKeyFromObject(cluster), cluster); err != nil {
 		return err
 	}
 

diff --git a/pkg/resources/fencing.go b/pkg/resources/fencing.go