Skip to content
This repository was archived by the owner on Sep 9, 2026. It is now read-only.

OSAC-3011: remove defaultStorageClassSentinel fallback #397

Closed
zszabo-rh wants to merge 6 commits into from
Closed

OSAC-3011: remove defaultStorageClassSentinel fallback #397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5eb5428
OSAC-3011: remove defaultStorageClassSentinel fallback
zszabo-rh Jul 27, 2026
92e166b
OSAC-3011: remove unused allTenantReconcileRequests function
zszabo-rh Jul 28, 2026
dda2a1c
OSAC-3011: fix remaining defaultStorageClassSentinel references in tests
zszabo-rh Jul 29, 2026
a125adb
OSAC-3011: remove label-storageclass Helm hook
zszabo-rh Jul 29, 2026
a0f30d0
OSAC-3011: go fmt storage_controller.go
zszabo-rh Jul 30, 2026
cc7e777
OSAC-3011: fix stale comment in no-AAP storage class path
zszabo-rh Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 0 additions & 99 deletions charts/operator/templates/hooks/label-storageclass.yaml
View file
Open in desktop

This file was deleted.

73 changes: 11 additions & 62 deletions internal/controller/storage_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,39 +352,13 @@ func (r *StorageReconciler) handleUpdate(ctx context.Context, instance *v1alpha1
return ctrl.Result{}, nil
}

// No tenant-specific SCs. Check for shared Default SCs so VMs
// can provision while the AAP job creates the real one.
defaultFallback, err := getTenantStorageClasses(ctx, targetClient, tenantName)
if err != nil {
return ctrl.Result{}, err
}

for _, msg := range defaultFallback.duplicateMessages {
r.Recorder.Eventf(instance, nil, corev1.EventTypeWarning, eventReasonDuplicateStorageClass, eventActionDetectDuplicate, "%s", msg)
}

if len(defaultFallback.resolved) > 0 {
condMsg := r.appendMissingTierWarnings(instance, tierDefinitions, defaultFallback.resolved, defaultFallback.ambiguousTiers,
defaultFallback.conditionMessage()+"; tenant-specific provisioning pending")
instance.SetStatusCondition(v1alpha1.TenantConditionClusterStorageReady,
metav1.ConditionTrue,
v1alpha1.TenantReasonFound,
condMsg)
instance.Status.StorageClasses = defaultFallback.resolved
instance.Status.ClusterStorage = []v1alpha1.ClusterStorageStatus{
{ClusterName: clusterName, Ready: true, Reason: v1alpha1.TenantReasonFound},
}
} else {
condMsg := r.appendMissingTierWarnings(instance, tierDefinitions, nil, defaultFallback.ambiguousTiers,
fmt.Sprintf("no StorageClass found for tenant %q", tenantName))
instance.SetStatusCondition(v1alpha1.TenantConditionClusterStorageReady,
metav1.ConditionFalse,
v1alpha1.TenantReasonNotFound,
condMsg)
instance.Status.StorageClasses = nil
instance.Status.ClusterStorage = []v1alpha1.ClusterStorageStatus{
{ClusterName: clusterName, Ready: false, Reason: v1alpha1.TenantReasonNotFound},
}
instance.SetStatusCondition(v1alpha1.TenantConditionClusterStorageReady,
metav1.ConditionFalse,
v1alpha1.TenantReasonNotFound,
fmt.Sprintf("no StorageClass found for tenant %q", tenantName))
instance.Status.StorageClasses = nil
instance.Status.ClusterStorage = []v1alpha1.ClusterStorageStatus{
{ClusterName: clusterName, Ready: false, Reason: v1alpha1.TenantReasonNotFound},
}

return r.handleClusterStorageProvisioning(ctx, instance, hubSecretReady)
Expand All @@ -405,11 +379,10 @@ func (r *StorageReconciler) handleUpdate(ctx context.Context, instance *v1alpha1
}
} else {
// When no provisioning provider is configured, resolve StorageClasses
// using the full tier resolution logic: tenant-specific SCs take
// priority, with shared default SCs (labeled tenant=Default) as
// fallback. This serves environments running OSAC without AAP/VAST
// where an admin or prepare-tenant.sh has labeled existing
// StorageClasses manually.
// labeled osac.openshift.io/tenant=<tenantName>. This serves
// environments running OSAC without AAP where an admin has
// pre-provisioned tenant-specific StorageClasses manually.
// Note: the shared tenant=Default fallback was removed in OSAC-3011.
result, err := getTenantStorageClasses(ctx, targetClient, tenantName)
if err != nil {
return ctrl.Result{}, err
Expand Down Expand Up @@ -979,12 +952,6 @@ func (r *StorageReconciler) mapStorageClassToTenant(ctx context.Context, obj cli
return nil
}

if tenantName == defaultStorageClassSentinel {
log.Info("shared Default StorageClass changed, reconciling all tenants",
"storageClass", obj.GetName())
return r.allTenantReconcileRequests(ctx)
}

tenant := &v1alpha1.Tenant{}
if err := r.Get(ctx, client.ObjectKey{Namespace: r.tenantNamespace, Name: tenantName}, tenant); err != nil {
if client.IgnoreNotFound(err) != nil {
Expand All @@ -1011,24 +978,6 @@ func (r *StorageReconciler) mapSecretToTenant(ctx context.Context, obj client.Ob
return []reconcile.Request{{NamespacedName: client.ObjectKeyFromObject(tenant)}}
}

func (r *StorageReconciler) allTenantReconcileRequests(ctx context.Context) []reconcile.Request {
log := ctrllog.FromContext(ctx)

tenantList := &v1alpha1.TenantList{}
if err := r.List(ctx, tenantList, client.InNamespace(r.tenantNamespace)); err != nil {
log.Error(err, "unable to list Tenants for Default SC reconciliation")
return nil
}

requests := make([]reconcile.Request, 0, len(tenantList.Items))
for i := range tenantList.Items {
requests = append(requests, reconcile.Request{
NamespacedName: client.ObjectKeyFromObject(&tenantList.Items[i]),
})
}
return requests
}

// tenantSpecificStorageClasses is the result of resolveTenantSpecificStorageClasses:
// resolved StorageClasses per tier, plus the duplicate-SC messages and ambiguous
// tier names for tiers excluded from resolved because multiple StorageClasses
Expand Down
65 changes: 23 additions & 42 deletions internal/controller/storage_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,10 +304,9 @@ var _ = Describe("Storage Controller", func() {
Expect(cond.Reason).To(Equal(v1alpha1.TenantReasonFound))
})

It("should set StorageBackendReady=False with NoProvider and continue to Stage 2 when no provider configured", func() {
It("should set StorageBackendReady=False with NoProvider and ClusterStorageReady=False when no provider configured", func() {
name := "storage-test-no-provider"
createReadyTenantForStorage(ctx, name, testNamespace)
createLabeledStorageClass(ctx, "default-sc-"+name, defaultStorageClassSentinel, "default")

r := NewStorageReconciler(
testMcManager, testNamespace, mcmanager.LocalCluster,
Expand All @@ -329,10 +328,10 @@ var _ = Describe("Storage Controller", func() {

clusterCond := tenant.GetStatusCondition(v1alpha1.TenantConditionClusterStorageReady)
Expect(clusterCond).NotTo(BeNil())
Expect(clusterCond.Status).To(Equal(metav1.ConditionTrue))
Expect(clusterCond.Status).To(Equal(metav1.ConditionFalse))
Expect(clusterCond.Reason).To(Equal(v1alpha1.TenantReasonNotFound))

@rgolangh rgolangh Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the reason be that the provider not found, instead of tenant not found?

@rgolangh rgolangh Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore, the comment is wrong


Expect(tenant.Status.StorageClasses).To(HaveLen(1))
Expect(tenant.Status.StorageClasses[0].Name).To(Equal("default-sc-" + name))
Expect(tenant.Status.StorageClasses).To(BeNil())
})

It("should set ClusterStorageReady=False when no provider and no labeled SCs", func() {
Expand Down Expand Up @@ -367,7 +366,6 @@ var _ = Describe("Storage Controller", func() {
It("should preserve tenant controller fields when patching storage status", func() {
name := "storage-test-patch-preserves-phase"
createReadyTenantForStorage(ctx, name, testNamespace)
createLabeledStorageClass(ctx, "default-sc-"+name, defaultStorageClassSentinel, "default")

r := NewStorageReconciler(
testMcManager, testNamespace, mcmanager.LocalCluster,
Expand All @@ -384,9 +382,7 @@ var _ = Describe("Storage Controller", func() {

Expect(tenant.Status.Phase).To(Equal(v1alpha1.TenantPhaseReady))
Expect(tenant.Status.Namespace).To(Equal(name))

Expect(tenant.Status.StorageClasses).To(HaveLen(1))
Expect(tenant.Status.StorageClasses[0].Name).To(Equal("default-sc-" + name))
Expect(tenant.Status.StorageClasses).To(BeNil())
})

It("should propagate trigger error without creating fake job", func() {
Expand Down Expand Up @@ -473,11 +469,10 @@ var _ = Describe("Storage Controller", func() {
Expect(tenant.Status.StorageClasses[0].Tier).To(Equal("default"))
})

It("should use Default SC fallback and trigger provisioning when only default SC exists and provider is configured", func() {
It("should set ClusterStorageReady=False and trigger provisioning when no tenant SCs and provider is configured", func() {
name := "storage-test-default-only-with-provider"
createReadyTenantForStorage(ctx, name, testNamespace)
createHubSecret(ctx, name, secretsNamespace)
createLabeledStorageClass(ctx, "shared-default-sc-"+name, defaultStorageClassSentinel, "default")

clusterProvider := &mockProvisioningProvider{name: "cluster-storage-mock"}
r := NewStorageReconciler(
Expand All @@ -495,22 +490,20 @@ var _ = Describe("Storage Controller", func() {

clusterCond := tenant.GetStatusCondition(v1alpha1.TenantConditionClusterStorageReady)
Expect(clusterCond).NotTo(BeNil())
Expect(clusterCond.Status).To(Equal(metav1.ConditionTrue))
Expect(clusterCond.Reason).To(Equal(v1alpha1.TenantReasonFound))
Expect(clusterCond.Message).To(ContainSubstring("tenant-specific provisioning pending"))
Expect(clusterCond.Status).To(Equal(metav1.ConditionFalse))
Expect(clusterCond.Reason).To(Equal(v1alpha1.TenantReasonNotFound))

Expect(tenant.Status.StorageClasses).To(HaveLen(1))
Expect(tenant.Status.StorageClasses[0].Name).To(Equal("shared-default-sc-" + name))
Expect(tenant.Status.StorageClasses).To(BeNil())
Expect(tenant.Status.ClusterStorageJobs).To(HaveLen(1))
})

It("should detect duplicate Default SCs in AAP fallback and set ClusterStorageReady=False", func() {
name := "storage-test-dup-default-aap"
It("should detect duplicate tenant SCs and set ClusterStorageReady=False without triggering provisioning", func() {
name := "storage-test-dup-tenant-aap"
createReadyTenantForStorage(ctx, name, testNamespace)
createHubSecret(ctx, name, secretsNamespace)
// Two Default SCs for the same tier: triggers duplicate detection
createLabeledStorageClass(ctx, "shared-default-dup1-"+name, defaultStorageClassSentinel, "default")
createLabeledStorageClass(ctx, "shared-default-dup2-"+name, defaultStorageClassSentinel, "default")
// Two tenant-specific SCs for the same tier: triggers duplicate detection
createLabeledStorageClass(ctx, "tenant-dup1-"+name, name, "default")
createLabeledStorageClass(ctx, "tenant-dup2-"+name, name, "default")

clusterProvider := &mockProvisioningProvider{name: "cluster-storage-mock"}
r := NewStorageReconciler(
Expand All @@ -526,18 +519,12 @@ var _ = Describe("Storage Controller", func() {
tenant := &v1alpha1.Tenant{}
Expect(k8sClient.Get(ctx, nn, tenant)).To(Succeed())

// Default fallback found duplicates, so no SCs are resolved.
// The condition should reflect NotFound (no usable SCs) and
// provisioning should still be triggered to create the real one.
clusterCond := tenant.GetStatusCondition(v1alpha1.TenantConditionClusterStorageReady)
Expect(clusterCond).NotTo(BeNil())
Expect(clusterCond.Status).To(Equal(metav1.ConditionFalse))
Expect(clusterCond.Reason).To(Equal(v1alpha1.TenantReasonNotFound))
Expect(clusterCond.Reason).To(Equal(v1alpha1.TenantReasonMultipleFound))

Expect(tenant.Status.StorageClasses).To(BeNil())
// Cluster storage provisioning should still run to create a
// tenant-specific SC that supersedes the ambiguous defaults
Expect(tenant.Status.ClusterStorageJobs).To(HaveLen(1))
})

It("should set ClusterStorageReady=False and trigger provisioning when no SCs at all and provider is configured", func() {
Expand Down Expand Up @@ -633,11 +620,10 @@ var _ = Describe("Storage Controller", func() {
})

Context("Tier resolution", func() {
It("should fall back to Default StorageClass when no tenant-specific SC", func() {
name := "storage-test-default-fallback"
It("should return empty StorageClasses when no tenant-specific SC exists", func() {
name := "storage-test-no-tenant-sc"
createReadyTenantForStorage(ctx, name, testNamespace)
createHubSecret(ctx, name, secretsNamespace)
createLabeledStorageClass(ctx, "shared-default-sc-"+name, defaultStorageClassSentinel, "default")

r := NewStorageReconciler(
testMcManager, testNamespace, mcmanager.LocalCluster,
Expand All @@ -652,15 +638,13 @@ var _ = Describe("Storage Controller", func() {
tenant := &v1alpha1.Tenant{}
Expect(k8sClient.Get(ctx, nn, tenant)).To(Succeed())

Expect(tenant.Status.StorageClasses).To(HaveLen(1))
Expect(tenant.Status.StorageClasses[0].Name).To(Equal("shared-default-sc-" + name))
Expect(tenant.Status.StorageClasses).To(BeNil())
})

It("should prefer tenant-specific SC over Default", func() {
It("should resolve tenant-specific SC by label", func() {
name := "storage-test-tenant-priority"
createReadyTenantForStorage(ctx, name, testNamespace)
createHubSecret(ctx, name, secretsNamespace)
createLabeledStorageClass(ctx, "shared-sc-"+name, defaultStorageClassSentinel, "default")
createLabeledStorageClass(ctx, name+"-tenant-sc", name, "default")

r := NewStorageReconciler(
Expand Down Expand Up @@ -1765,10 +1749,9 @@ var _ = Describe("Storage Controller", func() {
})

Context("Backend API routing (OSAC-1957)", func() {
It("should fall through to SC resolution when BackendsClient is nil", func() {
It("should set StorageBackendReady=False with NoProvider and no StorageClasses when BackendsClient is nil", func() {
name := "storage-test-nil-client"
createReadyTenantForStorage(ctx, name, testNamespace)
createLabeledStorageClass(ctx, "default-sc-"+name, defaultStorageClassSentinel, "default")

// BackendProvider set but BackendsClient nil: should behave as no backend registered
r := NewStorageReconciler(
Expand All @@ -1789,15 +1772,13 @@ var _ = Describe("Storage Controller", func() {
Expect(backendCond.Status).To(Equal(metav1.ConditionFalse))
Expect(backendCond.Reason).To(Equal(v1alpha1.TenantReasonNoProvider))
Expect(backendCond.Message).To(ContainSubstring("No fulfillment service connection configured"))
// Falls through to Stage 2: default SC resolved, no AAP job triggered
Expect(tenant.Status.StorageClasses).NotTo(BeEmpty())
Expect(tenant.Status.StorageClasses).To(BeNil())
Expect(tenant.Status.StorageBackendJobs).To(BeEmpty())
})

It("should fall through to SC resolution when BackendsClient reports no backends (total=0)", func() {
It("should set StorageBackendReady=False with NoProvider and no StorageClasses when BackendsClient reports no backends (total=0)", func() {
name := "storage-test-zero-backends"
createReadyTenantForStorage(ctx, name, testNamespace)
createLabeledStorageClass(ctx, "default-sc-"+name, defaultStorageClassSentinel, "default")

r := NewStorageReconciler(
testMcManager, testNamespace, mcmanager.LocalCluster,
Expand All @@ -1818,7 +1799,7 @@ var _ = Describe("Storage Controller", func() {
Expect(backendCond.Status).To(Equal(metav1.ConditionFalse))
Expect(backendCond.Reason).To(Equal(v1alpha1.TenantReasonNoProvider))
Expect(backendCond.Message).To(ContainSubstring("No storage backend registered"))
Expect(tenant.Status.StorageClasses).NotTo(BeEmpty())
Expect(tenant.Status.StorageClasses).To(BeNil())
Expect(tenant.Status.StorageBackendJobs).To(BeEmpty())
})

Expand Down
Loading
Loading