Address review comments by @seshachalam-yv

gardener · Aug 21, 2024 · c696b9c · c696b9c
1 parent d8217e3
commit c696b9c
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 19 deletions.
diff --git a/go.mod b/go.mod
@@ -13,7 +13,7 @@ require (
 	github.com/onsi/gomega v1.29.0
 	github.com/prometheus/client_golang v1.16.0
 	github.com/rakyll/gotest v0.0.6
-	github.com/robfig/cron v1.2.0
+	github.com/robfig/cron/v3 v3.0.1
 	github.com/spf13/pflag v1.0.5
 	go.uber.org/mock v0.2.0
 	go.uber.org/zap v1.26.0
@@ -105,7 +105,6 @@ require (
 	github.com/prometheus/client_model v0.4.0 // indirect
 	github.com/prometheus/common v0.44.0 // indirect
 	github.com/prometheus/procfs v0.10.1 // indirect
-	github.com/robfig/cron/v3 v3.0.1 // indirect
 	github.com/russross/blackfriday/v2 v2.1.0 // indirect
 	github.com/sirupsen/logrus v1.9.0 // indirect
 	github.com/spf13/afero v1.9.5 // indirect

diff --git a/go.sum b/go.sum
@@ -438,8 +438,6 @@ github.com/prometheus/procfs v0.10.1 h1:kYK1Va/YMlutzCGazswoHKo//tZVlFpKYh+Pymzi
 github.com/prometheus/procfs v0.10.1/go.mod h1:nwNm2aOCAYw8uTR/9bWRREkZFxAUcWzPHWJq+XBB/FM=
 github.com/rakyll/gotest v0.0.6 h1:hBTqkO3jiuwYW/M9gL4bu0oTYcm8J6knQAAPUsJsz1I=
 github.com/rakyll/gotest v0.0.6/go.mod h1:SkoesdNCWmiD4R2dljIUcfSnNdVZ12y8qK4ojDkc2Sc=
-github.com/robfig/cron v1.2.0 h1:ZjScXvvxeQ63Dbyxy76Fj3AT3Ut0aKsyd2/tl3DTMuQ=
-github.com/robfig/cron v1.2.0/go.mod h1:JGuDeoQd7Z6yL4zQhZ3OPEVHB7fL6Ka6skscFHfmt2k=
 github.com/robfig/cron/v3 v3.0.1 h1:WdRxkvbJztn8LMz/QEvLN5sBU+xKpSqwwUO1Pjr4qDs=
 github.com/robfig/cron/v3 v3.0.1/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzGIFLtro=
 github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg=

diff --git a/internal/health/condition/check_backup_ready.go b/internal/health/condition/check_backup_ready.go
@@ -72,7 +72,6 @@ func BackupReadyCheck(results []Result) Result {
 		fullSnapshotBackupMissedSchedule  = FullSnapshotBackupReadyCheckResult.Status() == druidv1alpha1.ConditionFalse
 		fullSnapshotBackupSucceeded       = FullSnapshotBackupReadyCheckResult.Status() == druidv1alpha1.ConditionTrue
 		deltaSnapshotBackupMissedSchedule = DeltaSnapshotBackupReadyCheckResult.Status() == druidv1alpha1.ConditionFalse
-		deltaSnapshotBackupSucceeded      = DeltaSnapshotBackupReadyCheckResult.Status() == druidv1alpha1.ConditionTrue
 	)
 	// False case
 	if fullSnapshotBackupMissedSchedule || deltaSnapshotBackupMissedSchedule {
@@ -82,7 +81,7 @@ func BackupReadyCheck(results []Result) Result {
 		return result
 	}
 	// True case
-	if fullSnapshotBackupSucceeded && deltaSnapshotBackupSucceeded {
+	if fullSnapshotBackupSucceeded {
 		result.status = druidv1alpha1.ConditionTrue
 		result.reason = BackupSucceeded
 		result.message = "Snapshot backup succeeded"

diff --git a/internal/utils/miscellaneous.go b/internal/utils/miscellaneous.go
@@ -10,7 +10,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/robfig/cron"
+	cron "github.com/robfig/cron/v3"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 

diff --git a/test/e2e/etcd_backup_test.go b/test/e2e/etcd_backup_test.go
@@ -241,18 +241,7 @@ func checkEtcdReady(ctx context.Context, cl client.Client, logger logr.Logger, e
 		if len(etcd.Status.Conditions) == 0 {
 			return fmt.Errorf("etcd %s status conditions is empty", etcd.Name)
 		}
-
-		for _, c := range etcd.Status.Conditions {
-			// skip BackupReady, FullSnapshotBackupReady & DeltaSnapshotBackupReady status checks if etcd.Spec.Backup.Store is not configured.
-			if etcd.Spec.Backup.Store == nil && (c.Type == v1alpha1.ConditionTypeBackupReady || c.Type == v1alpha1.ConditionTypeFullSnapshotBackupReady || c.Type == v1alpha1.ConditionTypeDeltaSnapshotBackupReady) {
-				continue
-			}
-			if c.Status != v1alpha1.ConditionTrue {
-				return fmt.Errorf("etcd %q status %q condition %s is not True",
-					etcd.Name, c.Type, c.Status)
-			}
-		}
-		return nil
+		return checkEtcdConditions(etcd)
 	}, timeout, pollingInterval).Should(BeNil())
 	logger.Info("etcd is ready")
 
@@ -333,3 +322,26 @@ func purgeEtcdPVCs(ctx context.Context, cl client.Client, etcdName string) {
 		DeleteOptions: delOptions,
 	}))).ShouldNot(HaveOccurred())
 }
+
+func checkEtcdConditions(etcd *v1alpha1.Etcd) error {
+	backupEnabled := etcd.Spec.Backup.Store != nil
+	for _, condition := range etcd.Status.Conditions {
+		// determine if backup conditions need to be checked
+		// skip BackupReady, FullSnapshotBackupReady & DeltaSnapshotBackupReady status checks if etcd.Spec.Backup.Store is not configured.
+		if !backupEnabled && isBackupCondition(condition.Type) {
+			continue
+		}
+		// return an error if any condition is not true
+		if condition.Status != v1alpha1.ConditionTrue {
+			return fmt.Errorf("etcd %q has a %q condition that is not True: status %s", etcd.Name, condition.Type, condition.Status)
+		}
+	}
+	return nil
+}
+
+// isBackupCondition checks if the condition type refers to backups health
+func isBackupCondition(conditionType v1alpha1.ConditionType) bool {
+	return conditionType == v1alpha1.ConditionTypeBackupReady ||
+		conditionType == v1alpha1.ConditionTypeFullSnapshotBackupReady ||
+		conditionType == v1alpha1.ConditionTypeDeltaSnapshotBackupReady
+}