Include unhealthy when retrieving all backends and remove "active" status

internal/backendstore/backend.go

@@ -13,15 +13,13 @@ import (
 type backend struct {
 	s3Client  S3Client
 	stsClient STSClient
-	active    bool
 	health    v1alpha1.HealthStatus
 }
 
-func newBackend(s3Client S3Client, stsClient STSClient, active bool, health v1alpha1.HealthStatus) *backend {
+func newBackend(s3Client S3Client, stsClient STSClient, health v1alpha1.HealthStatus) *backend {
 	return &backend{
 		s3Client:  s3Client,
 		stsClient: stsClient,
-		active:    active,
 		health:    health,
 	}
 }

internal/backendstore/backendstore.go

@@ -9,7 +9,7 @@ import (
 // s3Backends is a map of S3 backend name (eg ceph cluster name) to backend.
 type s3Backends map[string]*backend
 
-// BackendStore stores the active s3 backends.
+// BackendStore stores the s3 backends.
 type BackendStore struct {
 	s3Backends s3Backends
 	mu         sync.RWMutex
@@ -77,24 +77,13 @@ func (b *BackendStore) GetBackendS3Clients(beNames []string) map[string]S3Client
 	return clients
 }
 
-func (b *BackendStore) IsBackendActive(backendName string) bool {
+func (b *BackendStore) BackendExists(backendName string) bool {
 	b.mu.RLock()
 	defer b.mu.RUnlock()
 
-	if _, ok := b.s3Backends[backendName]; ok {
-		return b.s3Backends[backendName].active
-	}
-
-	return false
-}
-
-func (b *BackendStore) ToggleBackendActiveStatus(backendName string, active bool) {
-	b.mu.Lock()
-	defer b.mu.Unlock()
+	_, exists := b.s3Backends[backendName]
 
-	if _, ok := b.s3Backends[backendName]; ok {
-		b.s3Backends[backendName].active = active
-	}
+	return exists
 }
 
 func (b *BackendStore) GetBackendHealthStatus(backendName string) v1alpha1.HealthStatus {
@@ -124,11 +113,11 @@ func (b *BackendStore) DeleteBackend(backendName string) {
 	delete(b.s3Backends, backendName)
 }
 
-func (b *BackendStore) AddOrUpdateBackend(backendName string, s3C S3Client, stsC STSClient, active bool, health v1alpha1.HealthStatus) {
+func (b *BackendStore) AddOrUpdateBackend(backendName string, s3C S3Client, stsC STSClient, health v1alpha1.HealthStatus) {
 	b.mu.Lock()
 	defer b.mu.Unlock()
 
-	b.s3Backends[backendName] = newBackend(s3C, stsC, active, health)
+	b.s3Backends[backendName] = newBackend(s3C, stsC, health)
 }
 
 func (b *BackendStore) GetBackend(backendName string) *backend {
@@ -155,7 +144,7 @@ func (b *BackendStore) GetAllBackends() s3Backends {
 	return backends
 }
 
-func (b *BackendStore) GetActiveBackends(beNames []string) s3Backends {
+func (b *BackendStore) GetBackends(beNames []string) s3Backends {
 	requestedBackends := map[string]bool{}
 	for p := range beNames {
 		requestedBackends[beNames[p]] = true
@@ -167,7 +156,7 @@ func (b *BackendStore) GetActiveBackends(beNames []string) s3Backends {
 	// Create a new s3Backends to hold a copy of the backends
 	backends := make(s3Backends, 0)
 	for k, v := range b.s3Backends {
-		if _, ok := requestedBackends[k]; !ok || !v.active || v.health == v1alpha1.HealthStatusUnhealthy {
+		if _, ok := requestedBackends[k]; !ok {
 			continue
 		}
 
@@ -177,16 +166,12 @@ func (b *BackendStore) GetActiveBackends(beNames []string) s3Backends {
 	return backends
 }
 
-func (b *BackendStore) GetAllBackendNames(activeBackendsOnly bool) []string {
+func (b *BackendStore) GetAllBackendNames() []string {
 	b.mu.RLock()
 	defer b.mu.RUnlock()
 
 	backends := make([]string, 0)
-	for k, v := range b.s3Backends {
-		if activeBackendsOnly && !v.active || v.health == v1alpha1.HealthStatusUnhealthy {
-			continue
-		}
-
+	for k := range b.s3Backends {
 		backends = append(backends, k)
 	}
 

internal/controller/bucket/acl_test.go

@@ -62,7 +62,7 @@ func TestACLObserveBackend(t *testing.T) {
 					fake := backendstorefakes.FakeS3Client{}
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusUnhealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, apisv1alpha1.HealthStatusUnhealthy)
 
 					return bs
 				}(),
@@ -85,7 +85,7 @@ func TestACLObserveBackend(t *testing.T) {
 					fake := backendstorefakes.FakeS3Client{}
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, apisv1alpha1.HealthStatusHealthy)
 
 					return bs
 				}(),
@@ -113,7 +113,7 @@ func TestACLObserveBackend(t *testing.T) {
 					fake := backendstorefakes.FakeS3Client{}
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, apisv1alpha1.HealthStatusHealthy)
 
 					return bs
 				}(),
@@ -139,7 +139,7 @@ func TestACLObserveBackend(t *testing.T) {
 					fake := backendstorefakes.FakeS3Client{}
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, apisv1alpha1.HealthStatusHealthy)
 
 					return bs
 				}(),
@@ -167,7 +167,7 @@ func TestACLObserveBackend(t *testing.T) {
 					fake := backendstorefakes.FakeS3Client{}
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, apisv1alpha1.HealthStatusHealthy)
 
 					return bs
 				}(),
@@ -204,7 +204,7 @@ func TestACLObserveBackend(t *testing.T) {
 					fake := backendstorefakes.FakeS3Client{}
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, apisv1alpha1.HealthStatusHealthy)
 
 					return bs
 				}(),

internal/controller/bucket/bucket_validation_webhook.go

@@ -73,7 +73,7 @@ func (b *BucketValidator) ValidateDelete(ctx context.Context, obj runtime.Object
 
 func (b *BucketValidator) validateCreateOrUpdate(ctx context.Context, bucket *v1alpha1.Bucket) error {
 	if len(bucket.Spec.Providers) != 0 {
-		missingProviders := utils.MissingStrings(bucket.Spec.Providers, b.backendStore.GetAllBackendNames(false))
+		missingProviders := utils.MissingStrings(bucket.Spec.Providers, b.backendStore.GetAllBackendNames())
 		if len(missingProviders) != 0 {
 			return errors.New(fmt.Sprintf("providers %v listed in bucket.Spec.Providers cannot be found", missingProviders))
 		}
@@ -89,7 +89,7 @@ func (b *BucketValidator) validateCreateOrUpdate(ctx context.Context, bucket *v1
 }
 
 func (b *BucketValidator) validateLifecycleConfiguration(ctx context.Context, bucket *v1alpha1.Bucket) error {
-	s3Client := b.backendStore.GetActiveBackends(b.backendStore.GetAllBackendNames(true)).GetFirst()
+	s3Client := b.backendStore.GetAllBackends().GetFirst()
 	if s3Client == nil {
 		return errors.New(errNoS3BackendsStored)
 	}

internal/controller/bucket/consts.go

@@ -12,9 +12,8 @@ const (
 	errUpdateBucketCR = "failed to update Bucket CR"
 
 	// Backend store error messages.
-	errNoS3BackendsStored = "no s3 backends stored in backendstore"
-	errNoActiveS3Backends = "no active s3 backends in backendstore"
-	errMissingS3Backend   = "one or more desired providers are inactive or unhealthy"
+	errNoS3BackendsStored    = "no s3 backends stored in backendstore"
+	errAllS3BackendsDisabled = "all s3 backends have been disabled for this bucket - please check cr labels"
 
 	// Subresource error messages.
 	errObserveSubresource = "failed to observe bucket subresource"

internal/controller/bucket/create.go

@@ -97,47 +97,40 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext
 		return managed.ExternalCreation{}, err
 	}
 
-	// allBackendNames is a list of the names of all backends from backend store which
-	// are Healthy. These backends can be active or inactive. A backend is marked
-	// as inactive in the backend store when its ProviderConfig object has been deleted.
-	// Inactive backends are included in this list so that we can attempt to recreate
-	// this bucket on those backends should they become active again.
-	allBackendNames := c.backendStore.GetAllBackendNames(false)
-
-	// allBackendsToCreateOn is a list of names of all backends on which this S3 bucket
+	// allBackendNames is a list of the names of all backends in the backend store.
+	allBackendNames := c.backendStore.GetAllBackendNames()
+
+	// backendsToCreateOnNames is a list of names of all backends on which this S3 bucket
 	// is to be created. This will either be:
 	// 1. The list of bucket.Spec.Providers, if specified.
 	// 2. Otherwise, the allBackendNames list.
 	// In either case, the list will exclude any backends which have been specified as
 	// disabled on the Bucket CR. A backend is specified as disabled for a given bucket
 	// if it has been given the backend label (eg 'provider-ceph.backends.<backend-name>: "false"').
 	// This means that Provider Ceph will NOT create the bucket on this backend.
-	allBackendsToCreateOn := getBucketProvidersFilterDisabledLabel(bucket, allBackendNames)
-
-	// If none of the backends on which we wish to create the bucket are active then we
-	// return an error in order to requeue until backends become active.
-	// Otherwise we do a quick sanity check to see if there are backends
-	// that the bucket will not be created on and log these backends.
-	activeBackendsToCreateOn := c.backendStore.GetActiveBackends(allBackendsToCreateOn)
-	if len(activeBackendsToCreateOn) == 0 {
-		err := errors.New(errNoActiveS3Backends)
+	backendsToCreateOnNames := getBucketProvidersFilterDisabledLabel(bucket, allBackendNames)
+	if len(backendsToCreateOnNames) == 0 {
+		err := errors.New(errAllS3BackendsDisabled)
 		traces.SetAndRecordError(span, err)
 
 		return managed.ExternalCreation{}, err
-	} else if len(activeBackendsToCreateOn) != len(allBackendsToCreateOn) {
-		c.log.Info("Bucket will not be created on the following S3 backends", consts.KeyBucketName, bucket.Name, "backends", utils.MissingStrings(allBackendsToCreateOn, allBackendNames))
-		traces.SetAndRecordError(span, errors.New(errMissingS3Backend))
+	}
+
+	// Quick sanity check to see if there are backends that the bucket will not be created
+	// on and log these backends.
+	if len(allBackendNames) != len(backendsToCreateOnNames) {
+		c.log.Info("Bucket will not be created on the following S3 backends", consts.KeyBucketName, bucket.Name, "backends", utils.MissingStrings(allBackendNames, backendsToCreateOnNames))
 	}
 
 	// This value shows a bucket on the given backend is already created.
 	// It is used to prevent go routines from sending duplicated messages to `readyChan`.
 	bucketAlreadyCreated := atomic.Bool{}
 	backendCount := 0
-	errChan := make(chan error, len(activeBackendsToCreateOn))
+	errChan := make(chan error, len(backendsToCreateOnNames))
 	readyChan := make(chan string)
 
-	// Now we're ready to start creating S3 buckets on our desired active backends.
-	for beName := range activeBackendsToCreateOn {
+	// Now we're ready to start creating S3 buckets on our desired backends.
+	for _, beName := range backendsToCreateOnNames {
 		originalBucket := bucket.DeepCopy()
 
 		// Attempt to get an S3 client for the backend. This will either be the default
@@ -199,10 +192,10 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext
 			// Although no backends were found for the bucket, we still apply the backend
 			// label to the Bucket CR for each backend that the bucket was intended to be
 			// created on. This is to ensure the bucket will eventually be created on these
-			// backends whenever they become active again.
-			setAllBackendLabels(bucketLatest, allBackendsToCreateOn)
+			// backends.
+			setAllBackendLabels(bucketLatest, backendsToCreateOnNames)
 			// Pause the Bucket CR because there is no backend for it to be created on.
-			// If a backend for which it was intended becomes active, the health-check
+			// If a backend for which it was intended becomes healthy, the health-check
 			// controller will un-pause the Bucket CR (identifying it by its backend label)
 			// and it will be re-reconciled.
 			bucketLatest.Labels[meta.AnnotationKeyReconciliationPaused] = True
@@ -219,10 +212,10 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext
 		return managed.ExternalCreation{}, nil
 	}
 
-	return c.waitForCreationAndUpdateBucketCR(ctx, bucket, allBackendsToCreateOn, readyChan, errChan, backendCount)
+	return c.waitForCreationAndUpdateBucketCR(ctx, bucket, backendsToCreateOnNames, readyChan, errChan, backendCount)
 }
 
-func (c *external) waitForCreationAndUpdateBucketCR(ctx context.Context, bucket *v1alpha1.Bucket, allBackendsToCreateOn []string, readyChan <-chan string, errChan <-chan error, backendCount int) (managed.ExternalCreation, error) {
+func (c *external) waitForCreationAndUpdateBucketCR(ctx context.Context, bucket *v1alpha1.Bucket, backendsToCreateOnNames []string, readyChan <-chan string, errChan <-chan error, backendCount int) (managed.ExternalCreation, error) {
 	ctx, span := otel.Tracer("").Start(ctx, "waitForCreationAndUpdateBucketCR")
 	defer span.End()
 
@@ -244,7 +237,7 @@ func (c *external) waitForCreationAndUpdateBucketCR(ctx context.Context, bucket
 			// 3. The Bucket CR Status Backends with a Ready condition for the backend the bucket
 			// was created on.
 			err := c.updateBucketCR(ctx, bucket, func(bucketLatest *v1alpha1.Bucket) UpdateRequired {
-				setAllBackendLabels(bucketLatest, allBackendsToCreateOn)
+				setAllBackendLabels(bucketLatest, backendsToCreateOnNames)
 
 				return NeedsObjectUpdate
 			}, func(bucketLatest *v1alpha1.Bucket) UpdateRequired {

internal/controller/bucket/create_test.go

@@ -64,7 +64,6 @@ func TestCreateBasicErrors(t *testing.T) {
 			fields: fields{
 				backendStore: func() *backendstore.BackendStore {
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-0", nil, nil, false, apisv1alpha1.HealthStatusUnknown)
 
 					return bs
 				}(),
@@ -80,7 +79,7 @@ func TestCreateBasicErrors(t *testing.T) {
 				},
 			},
 			want: want{
-				err: errors.New(errNoActiveS3Backends),
+				err: errors.New(errNoS3BackendsStored),
 			},
 		},
 		"S3 backend not referenced and none exist": {
@@ -98,6 +97,27 @@ func TestCreateBasicErrors(t *testing.T) {
 				err: errors.New(errNoS3BackendsStored),
 			},
 		},
+		"S3 backend exists but is disabled for bucket": {
+			fields: fields{
+				backendStore: func() *backendstore.BackendStore {
+					bs := backendstore.NewBackendStore()
+					bs.AddOrUpdateBackend("s3-backend-1", nil, nil, apisv1alpha1.HealthStatusHealthy)
+
+					return bs
+				}(),
+			},
+			args: args{
+				mg: &v1alpha1.Bucket{
+					ObjectMeta: metav1.ObjectMeta{
+						Labels: map[string]string{v1alpha1.BackendLabelPrefix + "s3-backend-1": "false"},
+						Name:   "test-bucket",
+					},
+				},
+			},
+			want: want{
+				err: errors.New(errAllS3BackendsDisabled),
+			},
+		},
 	}
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
@@ -168,7 +188,7 @@ func TestCreate(t *testing.T) {
 					)
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", &fake, nil, apisv1alpha1.HealthStatusHealthy)
 
 					return bs
 				}(),
@@ -206,8 +226,8 @@ func TestCreate(t *testing.T) {
 					}
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", nil, &fake, true, apisv1alpha1.HealthStatusHealthy)
-					bs.AddOrUpdateBackend("s3-backend-2", nil, &fake, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", nil, &fake, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-2", nil, &fake, apisv1alpha1.HealthStatusHealthy)
 
 					return bs
 				}(),
@@ -273,9 +293,9 @@ func TestCreate(t *testing.T) {
 					)
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", &fakeClientError, nil, true, apisv1alpha1.HealthStatusHealthy)
-					bs.AddOrUpdateBackend("s3-backend-2", &fakeClientError, nil, true, apisv1alpha1.HealthStatusHealthy)
-					bs.AddOrUpdateBackend("s3-backend-3", &fakeClientOK, nil, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", &fakeClientError, nil, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-2", &fakeClientError, nil, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-3", &fakeClientOK, nil, apisv1alpha1.HealthStatusHealthy)
 
 					return bs
 				}(),
@@ -333,9 +353,9 @@ func TestCreate(t *testing.T) {
 					)
 
 					bs := backendstore.NewBackendStore()
-					bs.AddOrUpdateBackend("s3-backend-1", &fakeClientError, nil, true, apisv1alpha1.HealthStatusHealthy)
-					bs.AddOrUpdateBackend("s3-backend-2", &fakeClientError, nil, true, apisv1alpha1.HealthStatusHealthy)
-					bs.AddOrUpdateBackend("s3-backend-3", &fakeClientError, nil, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-1", &fakeClientError, nil, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-2", &fakeClientError, nil, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend("s3-backend-3", &fakeClientError, nil, apisv1alpha1.HealthStatusHealthy)
 
 					return bs
 				}(),