Simplfy min replicas (#272)

* Simplify min-replicas:
- Make min replicas responsible for Ready status not Synced
- Remove min-replicas from autopause check
- Only pause when Ready and Synced

* Delete bucket to fix flaky test (unrelated to PR changes)

cmd/provider/main.go

@@ -100,7 +100,7 @@ func main() {
 		enableManagementPolicies   = app.Flag("enable-management-policies", "Enable support for Management Policies.").Default("false").Envar("ENABLE_MANAGEMENT_POLICIES").Bool()
 
 		autoPauseBucket       = app.Flag("auto-pause-bucket", "Enable auto pause of reconciliation of ready buckets").Default("false").Envar("AUTO_PAUSE_BUCKET").Bool()
-		minReplicas           = app.Flag("minimum-replicas", "Minimum number of replicas of a bucket before it is considered synced").Default("2").Envar("MINIMUM_REPLICAS").Uint()
+		minReplicas           = app.Flag("minimum-replicas", "Minimum number of replicas of a bucket before it is considered Ready").Default("1").Envar("MINIMUM_REPLICAS").Uint()
 		recreateMissingBucket = app.Flag("recreate-missing-bucket", "Recreates existing bucket if missing").Default("true").Envar("RECREATE_MISSING_BUCKET").Bool()
 
 		assumeRoleArn = app.Flag("assume-role-arn", "Assume role ARN to be used for STS authentication").Default("").Envar("ASSUME_ROLE_ARN").String()

e2e/tests/stable/chainsaw-test.yaml

@@ -568,6 +568,25 @@ spec:
         - local-dev-control-plane:32568
         entrypoint: ../../../hack/expect_bucket.sh
 
+  - name: Delete localstack-b-bucket.
+    try:
+    - command:
+      # We need to "unpause" auto-pause-bucket to allow deletion.
+        args:
+        - patch
+        - --type=merge
+        - buckets
+        - localstack-b-bucket
+        - -p
+        - '{"metadata":{"labels":{"crossplane.io/paused":"false"}}}'
+        entrypoint: kubectl
+    - command:
+        args:
+        - delete
+        - bucket
+        - localstack-b-bucket
+        entrypoint: kubectl
+
   - name: Make localstack-a unreachable and therefore Unhealthy.
     try:
     - command:
@@ -666,7 +685,8 @@ spec:
             autoPause: true
             forProvider: {}
     # Assert auto-pause-bucket is only created on localstack-b
-    # and localstack-c, as localstack-a is currently unhealthy.
+    # and localstack-c, as localstack-a is currently unhealthy
+    # and that it is not paused or Synced.
     - assert:
         resource:
           apiVersion: provider-ceph.ceph.crossplane.io/v1alpha1
@@ -676,7 +696,6 @@ spec:
             finalizers:
             - "finalizer.managedresource.crossplane.io"
             labels:
-              crossplane.io/paused: "true"
               provider-ceph.backends.localstack-a: "true"
               provider-ceph.backends.localstack-b: "true"
               provider-ceph.backends.localstack-c: "true"
@@ -697,8 +716,8 @@ spec:
             - reason: Available
               status: "True"
               type: Ready
-            - reason: ReconcileSuccess
-              status: "True"
+            - reason: ReconcileError
+              status: "False"
               type: Synced
     # Assert avoid-localstack-c-bucket is only created on localstack-b,
     # as localstack-a is currently unhealthy and this bucket is not intended
@@ -752,7 +771,7 @@ spec:
             - reason: HealthCheckSuccess
               status: "True"
               type: Ready
-
+    # Assert auto-pause-bucket is created on localstack-a and that it is Synced and paused.
   - name: Assert auto-pause-bucket and avoid-localstack-c-bucket are created on localstack-a now that it is Healthy.
     try:
     - assert:

internal/controller/bucket/delete.go

@@ -2,7 +2,6 @@ package bucket
 
 import (
 	"context"
-	"math"
 
 	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/attribute"
@@ -106,8 +105,7 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error {
 	// CR spec. If the deletion is successful or unsuccessful, the bucket CR status must be
 	// updated.
 	if err := c.updateBucketCR(ctx, bucket, func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired {
-		// Bucket status is unavailable at this point. Use math.MaxUint as minReplicas is irrelevant in this scenario.
-		setBucketStatus(bucketLatest, bucketBackends, providerNames, math.MaxUint)
+		setBucketStatus(bucketLatest, bucketBackends, providerNames, c.minReplicas)
 
 		return NeedsStatusUpdate
 	}); err != nil {

internal/controller/bucket/helpers.go

@@ -3,7 +3,6 @@ package bucket
 import (
 	"context"
 	"fmt"
-	"math"
 	"slices"
 	"strings"
 
@@ -36,9 +35,15 @@ func isBucketPaused(bucket *v1alpha1.Bucket) bool {
 // isPauseRequired determines if the Bucket should be paused.
 //
 //nolint:gocyclo,cyclop // Function requires numerous checks.
-func isPauseRequired(bucket *v1alpha1.Bucket, providerNames []string, minReplicas uint, c map[string]backendstore.S3Client, bb *bucketBackends, autopauseEnabled bool) bool {
-	// If the number of backends on which the bucket is available is less than the number of providerNames or minReplicas, then the bucket must not be paused.
-	if float64(bb.countBucketsAvailableOnBackends(bucket.Name, providerNames, c)) < math.Min(float64(len(providerNames)), float64(minReplicas)) {
+func isPauseRequired(bucket *v1alpha1.Bucket, providerNames []string, c map[string]backendstore.S3Client, bb *bucketBackends, autopauseEnabled bool) bool {
+	// Avoid pausing if the Bucket CR is not Ready and Synced.
+	if !(bucket.Status.GetCondition(xpv1.TypeReady).Equal(xpv1.Available()) &&
+		bucket.Status.GetCondition(xpv1.TypeSynced).Equal(xpv1.ReconcileSuccess())) {
+		return false
+	}
+
+	// Avoid pausing if the number of backends on which the bucket is available is less than the number of providerNames.
+	if float64(bb.countBucketsAvailableOnBackends(bucket.Name, providerNames, c)) < float64(len(providerNames)) {
 		return false
 	}
 
@@ -170,14 +175,15 @@ func setBucketStatus(bucket *v1alpha1.Bucket, bucketBackends *bucketBackends, pr
 		}
 		unavailableBackends = append(unavailableBackends, backendName)
 	}
-	// The Bucket CR is considered Available if the bucket is available on any backend.
-	if ok > 0 {
+	// The Bucket CR is considered Available if the bucket is available on "minReplicas"
+	// number of backends (default = 1).
+	if ok >= int(minReplicas) {
 		bucket.Status.SetConditions(xpv1.Available())
 	}
 	// The Bucket CR is considered Synced (ReconcileSuccess) once the bucket is available
-	// on the lesser of all backends or minimum replicas. We also ensure that the overall
-	// Bucket CR is available (in a Ready state) - this should already be the case.
-	if float64(ok) >= math.Min(float64(len(providerNames)), float64(minReplicas)) &&
+	// on all backends. We also ensure that the overall Bucket CR is available (in a Ready
+	// state) - this should already be the case.
+	if ok >= len(providerNames) &&
 		bucket.Status.GetCondition(xpv1.TypeReady).Equal(xpv1.Available()) {
 		bucket.Status.SetConditions(xpv1.ReconcileSuccess())