Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
19 changes: 18 additions & 1 deletion bundle/manifests/ocs-client-operator.clusterserviceversion.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ metadata:
categories: Storage
console.openshift.io/plugins: '["odf-client-console"]'
containerImage: quay.io/ocs-dev/ocs-client-operator:latest
createdAt: "2026-01-22T06:52:52Z"
createdAt: "2026-03-05T11:10:00Z"
description: OpenShift Data Foundation client operator enables consumption of
storage services from a remote centralized OpenShift Data Foundation provider
cluster.
Expand Down Expand Up @@ -282,6 +282,23 @@ spec:
- list
- update
- watch
- apiGroups:
- objectbucket.io
resources:
- objectbucketclaims
verbs:
- get
- list
- update
- watch
- apiGroups:
- objectbucket.io
resources:
- objectbucketclaims/status
verbs:
- get
- patch
- update
- apiGroups:
- ocs.openshift.io
resources:
Expand Down
32 changes: 32 additions & 0 deletions cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
groupsnapapi "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumegroupsnapshot/v1beta1"
snapapi "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
nbapis "github.com/noobaa/noobaa-operator/v5/pkg/apis"
nbv1 "github.com/noobaa/noobaa-operator/v5/pkg/apis/noobaa/v1alpha1"
configv1 "github.com/openshift/api/config/v1"
consolev1 "github.com/openshift/api/console/v1"
quotav1 "github.com/openshift/api/quota/v1"
Expand All @@ -49,10 +50,12 @@ import (
odfgsapiv1b1 "github.com/red-hat-storage/external-snapshotter/client/v8/apis/volumegroupsnapshot/v1beta1"
admrv1 "k8s.io/api/admissionregistration/v1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
_ "k8s.io/client-go/plugin/pkg/client/auth"
Expand Down Expand Up @@ -93,6 +96,16 @@ func init() {
utilruntime.Must(groupsnapapi.AddToScheme(scheme))
utilruntime.Must(odfgsapiv1b1.AddToScheme(scheme))
utilruntime.Must(csiaddonsv1alpha1.AddToScheme(scheme))
// ObjectBucketClaim/ObjectBucket (objectbucket.io); nbapis.AddToScheme does not register these types
// this part was added to avoid direct import of lib-bucket-provisioner
objectBucketGV := schema.GroupVersion{Group: "objectbucket.io", Version: "v1alpha1"}
scheme.AddKnownTypes(objectBucketGV,
&nbv1.ObjectBucketClaim{},
&nbv1.ObjectBucketClaimList{},
&nbv1.ObjectBucket{},
&nbv1.ObjectBucketList{},
)
metav1.AddToGroupVersion(scheme, objectBucketGV)
//+kubebuilder:scaffold:scheme
}

Expand Down Expand Up @@ -217,6 +230,17 @@ func main() {
// only cache our validation webhook
Field: subscriptionwebhookSelector,
},
// Watch ObjectBucketClaim and OBC-related resources in all namespaces so OBC controller reconciles regardless of WATCH_NAMESPACE.
// Empty ByObject would be defaulted to DefaultNamespaces; explicitly set NamespaceAll to avoid that.
&nbv1.ObjectBucketClaim{}: {
Namespaces: map[string]cache.Config{corev1.NamespaceAll: {}},
},
Comment on lines +235 to +237
Copy link
Contributor

Choose a reason for hiding this comment

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

even the returned OB/Secret/Configmap should be watched in other namespaces, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The secret and config map should be watched in other namespaces as well, yes -I added the change.
OB is cluster-scoped, so I didn't change.

&corev1.ConfigMap{}: {
Namespaces: map[string]cache.Config{corev1.NamespaceAll: {}},
},
&corev1.Secret{}: {
Namespaces: map[string]cache.Config{corev1.NamespaceAll: {}},
},
},
DefaultNamespaces: defaultNamespaces,
},
Expand Down Expand Up @@ -324,6 +348,14 @@ func main() {
}
}

if err = (&controller.OBCReconciler{
Client: mgr.GetClient(),
Scheme: mgr.GetScheme(),
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "ObjectBucketClaim")
os.Exit(1)
}

setupLog.Info("starting manager")
if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
setupLog.Error(err, "problem running manager")
Expand Down
17 changes: 17 additions & 0 deletions config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,23 @@ rules:
- list
- update
- watch
- apiGroups:
- objectbucket.io
resources:
- objectbucketclaims
verbs:
- get
- list
- update
- watch
- apiGroups:
- objectbucket.io
resources:
- objectbucketclaims/status
verbs:
- get
- patch
- update
- apiGroups:
- ocs.openshift.io
resources:
Expand Down
13 changes: 2 additions & 11 deletions internal/controller/maintenancemode_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
ramenv1alpha1 "github.com/ramendr/ramen/api/v1alpha1"
"github.com/red-hat-storage/ocs-client-operator/api/v1alpha1"
"github.com/red-hat-storage/ocs-client-operator/pkg/utils"
providerclient "github.com/red-hat-storage/ocs-operator/services/provider/api/v4/client"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -124,17 +123,9 @@ func (r *MaintenanceModeReconciler) Reconcile(ctx context.Context, _ ctrl.Reques
}

func (r *MaintenanceModeReconciler) toggleMaintenanceModeForClient(storageClient *v1alpha1.StorageClient, enable bool) error {
providerClient, err := providerclient.NewProviderClient(
r.ctx,
storageClient.Spec.StorageProviderEndpoint,
utils.OcsClientTimeout,
)
providerClient, err := utils.NewProviderClientForStorageClient(r.ctx, storageClient.Spec.StorageProviderEndpoint)
if err != nil {
return fmt.Errorf(
"failed to create provider client with endpoint %v: %v",
storageClient.Spec.StorageProviderEndpoint,
err,
)
return err
}
// Close client-side connections.
defer providerClient.Close()
Expand Down
187 changes: 187 additions & 0 deletions internal/controller/obc_controller.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
package controller

import (
"context"
"fmt"
"slices"

"github.com/go-logr/logr"
nbv1 "github.com/noobaa/noobaa-operator/v5/pkg/apis/noobaa/v1alpha1"
"github.com/red-hat-storage/ocs-client-operator/api/v1alpha1"
"github.com/red-hat-storage/ocs-client-operator/pkg/utils"
storagev1 "k8s.io/api/storage/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
Copy link
Member

Choose a reason for hiding this comment

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

Please format the imports as

standard
pkg
others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll look again.

)

const (
ObcFinalizer = nbv1.ObjectBucketFinalizer
ObjectBucketClaimStatusPhaseFailed = "Failed"
)

// OBCReconciler reconciles a ObjectBucketClaim object
type OBCReconciler struct {
client.Client
Scheme *runtime.Scheme
log logr.Logger
ctx context.Context
}

// SetupWithManager sets up the controller with the Manager
func (r *OBCReconciler) SetupWithManager(mgr ctrl.Manager) error {
// Reconcile on Create, Delete, and Update when the object is being deleted or when the spec (generation) changes.
obcPredicate := predicate.Or(
predicate.GenerationChangedPredicate{},
predicate.NewPredicateFuncs(func(obj client.Object) bool {
return !obj.GetDeletionTimestamp().IsZero()
}),
)

return ctrl.NewControllerManagedBy(mgr).
Named("ObjectBucketClaim").
For(
&nbv1.ObjectBucketClaim{},
builder.WithPredicates(obcPredicate),
).
Complete(r)
}

//+kubebuilder:rbac:groups=objectbucket.io,resources=objectbucketclaims,verbs=get;list;watch;update
//+kubebuilder:rbac:groups=objectbucket.io,resources=objectbucketclaims/status,verbs=get;update;patch
//+kubebuilder:rbac:groups=ocs.openshift.io,resources=storageclients,verbs=get
//+kubebuilder:rbac:groups=storage.k8s.io,resources=storageclasses,verbs=get

func (r *OBCReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
r.ctx = ctx
r.log = ctrl.LoggerFrom(r.ctx).WithName("OBC")

r.log.Info("Starting reconcile iteration for OBC", "req", req)

obc := &nbv1.ObjectBucketClaim{}
err := r.Get(r.ctx, req.NamespacedName, obc)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Could you use inline errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks.

if errors.IsNotFound(err) {
r.log.Info("OBC resource not found. Ignoring since object must be deleted.")
return reconcile.Result{}, nil
}
r.log.Error(err, "failed to get OBC")
return reconcile.Result{}, fmt.Errorf("failed to get OBC: %v", err)
}

if !obc.GetDeletionTimestamp().IsZero() {
r.log.Info("OBC deleted", "namespaced/name", client.ObjectKeyFromObject(obc))
storageClient, err := r.getStorageClientFromStorageClass(obc.Spec.StorageClassName)
if err != nil {
r.log.Error(err, "failed to get StorageClient for OBC delete")
return reconcile.Result{}, fmt.Errorf("failed to get StorageClient for OBC delete: %v", err)
}
if err := r.notifyObcDeleted(storageClient, types.NamespacedName{Namespace: obc.Namespace, Name: obc.Name}); err != nil {
r.log.Error(err, "failed to notify provider of OBC deletion", "namespaced/name", client.ObjectKeyFromObject(obc))
return reconcile.Result{}, fmt.Errorf("failed to in Notify gRPC call of OBC deleted: %v", err)
}
if controllerutil.RemoveFinalizer(obc, ObcFinalizer) {
r.log.Info("removing finalizer from OBC", "namespaced/name", client.ObjectKeyFromObject(obc))
if err := r.Update(r.ctx, obc); err != nil {
r.log.Info("Failed to remove finalizer from OBC", "namespaced/name", client.ObjectKeyFromObject(obc))
return reconcile.Result{}, fmt.Errorf("failed to remove finalizer from OBC: %v", err)
}
}
return reconcile.Result{}, nil
}

r.log.Info("OBC created", "namespace", obc.Namespace, "name", obc.Name)
if controllerutil.AddFinalizer(obc, ObcFinalizer) {
r.log.Info("Finalizer not found for OBC. Adding finalizer", "namespaced/name", client.ObjectKeyFromObject(obc))
if err := r.Update(r.ctx, obc); err != nil {
r.log.Info("Failed to add finalizer to OBC", "namespaced/name", client.ObjectKeyFromObject(obc))
return reconcile.Result{}, fmt.Errorf("failed to add finalizer to OBC: %v", err)
}
}
storageClient, err := r.getStorageClientFromStorageClass(obc.Spec.StorageClassName)
if err != nil {
r.log.Error(err, "failed to get StorageClient for OBC create")
obc.Status.Phase = ObjectBucketClaimStatusPhaseFailed
if statusErr := r.Client.Status().Update(r.ctx, obc); statusErr != nil {
Copy link
Member

Choose a reason for hiding this comment

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

It would be good if have status updates from a single place. Can you split reconcile into reconcile and reconcilePhases, and update the status towards the end of the reconcile similar to what we are doing for storageclient controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to refactor it so we will have the reconcile function, reconcilePhases function (and more functions called inside it), and update the status toward the end of the reconcile.

My only concern with this change is a race condition between the status update and the "Bound" that is patched from outside.
Therefore, I tried to status was saved, and the update will be called only if it was changed (and it changes to Failed only in the creation/update flow).

r.log.Error(statusErr, "Failed to update OBC status")
}
return reconcile.Result{}, fmt.Errorf("failed to get StorageClient for OBC created: %v", err)
}
if err := r.notifyObcCreated(storageClient, obc); err != nil {
r.log.Error(err, "failed to notify provider of OBC creation", "namespaced/name", client.ObjectKeyFromObject(obc))
obc.Status.Phase = ObjectBucketClaimStatusPhaseFailed
if statusErr := r.Client.Status().Update(r.ctx, obc); statusErr != nil {
r.log.Error(statusErr, "Failed to update OBC status")
}
return reconcile.Result{}, fmt.Errorf("failed to in Notify gRPC call of OBC creation: %v", err)

}
// Clear Failed status when a retry succeeds
if obc.Status.Phase == ObjectBucketClaimStatusPhaseFailed {
obc.Status.Phase = ""
if statusErr := r.Client.Status().Update(r.ctx, obc); statusErr != nil {
r.log.Error(statusErr, "Failed to update OBC status after success")
}
}
return reconcile.Result{}, nil
}

// getStorageClientFromStorageClass returns the StorageClient that owns the given StorageClass (via ownerReference).
func (r *OBCReconciler) getStorageClientFromStorageClass(storageClassName string) (*v1alpha1.StorageClient, error) {
sc := &storagev1.StorageClass{}
if err := r.Get(r.ctx, types.NamespacedName{Name: storageClassName}, sc); err != nil {
return nil, fmt.Errorf("get StorageClass %q: %w", storageClassName, err)
}
ownerStorageClientIndex := slices.IndexFunc(sc.OwnerReferences, func(owner metav1.OwnerReference) bool {
return owner.Kind == "StorageClient"
})
if ownerStorageClientIndex == -1 {
return nil, fmt.Errorf("StorageClass %q has no StorageClient ownerReference", storageClassName)
}
storageClient := &v1alpha1.StorageClient{}
storageClientName := sc.OwnerReferences[ownerStorageClientIndex].Name
if err := r.Get(r.ctx, types.NamespacedName{Name: storageClientName}, storageClient); err != nil {
return nil, fmt.Errorf("get StorageClient %q (owner of StorageClass %q): %w", storageClientName, storageClassName, err)
}
if storageClient.Status.ConsumerID == "" || storageClient.Spec.StorageProviderEndpoint == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe check if storageClient is onboarded or not. If onboarding succeeded, we should have the consumerUID and the storageProviderEndpoint as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to check if the phase is StorageClientConnected?
But it is not the same as checking those fields, from what I see.
For example, when there was an issue in the cluster:

  • phase: Initializing
  • oc get storageclient storage-client-2202 -o json | jq .spec.storageProviderEndpoint (output with value)
  • oc get storageclient storage-client-2202 -o json | jq .status.consumerID null

The storage client was successfully onboarded in the past.
If I check the phase, it will not be able to create OBCs...

Copy link
Member

Choose a reason for hiding this comment

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

If the storageClient is not Onboarding, we shouldn't be performing any calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The storageClient is onboarded.
In this example, an unrelated issue caused the phase to change to Initializing.
@rewantsoni Would you like me to remove this condition, or do you have another suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

And I don't think we should be having this check here; this should only return the Storage Client

Copy link
Member

Choose a reason for hiding this comment

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

I am trying to think of a case where the Client will be Connected, but the consumer UID will be empty 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably would not happen, it is set as the last step in onboarding.

return nil, fmt.Errorf("StorageClient %q has no ConsumerID or StorageProviderEndpoint", storageClient.Name)
}
return storageClient, nil
}

// notifyObcCreated notifies the provider of the creation of an OBC.
func (r *OBCReconciler) notifyObcCreated(storageClient *v1alpha1.StorageClient, obc *nbv1.ObjectBucketClaim) error {
pc, err := utils.NewProviderClientForStorageClient(r.ctx, storageClient.Spec.StorageProviderEndpoint)
if err != nil {
return err
}
defer pc.Close()
_, err = pc.NotifyObcCreated(r.ctx, storageClient.Status.ConsumerID, obc)
if err != nil {
return fmt.Errorf("NotifyObcCreated: %w", err)
}
r.log.Info("Notify of OBC created completed", "namespace", obc.Namespace, "name", obc.Name)
return nil
}

// notifyObcDeleted notifies the provider of the deletion of an OBC.
func (r *OBCReconciler) notifyObcDeleted(storageClient *v1alpha1.StorageClient, obcDetails types.NamespacedName) error {
pc, err := utils.NewProviderClientForStorageClient(r.ctx, storageClient.Spec.StorageProviderEndpoint)
if err != nil {
return err
}
defer pc.Close()
_, err = pc.NotifyObcDeleted(r.ctx, storageClient.Status.ConsumerID, obcDetails)
if err != nil {
return fmt.Errorf("NotifyObcDeleted: %w", err)
}
r.log.Info("Notify of OBC deleted completed", "namespace", obcDetails.Namespace, "name", obcDetails.Name)
return nil
}
5 changes: 2 additions & 3 deletions internal/controller/storageclient_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -577,10 +577,9 @@ func (r *storageClientReconcile) deletionPhase(externalClusterClient *providerCl
// newExternalClusterClient returns the *providerClient.OCSProviderClient
func (r *storageClientReconcile) newExternalClusterClient() (*providerClient.OCSProviderClient, error) {

ocsProviderClient, err := providerClient.NewProviderClient(
r.ctx, r.storageClient.Spec.StorageProviderEndpoint, utils.OcsClientTimeout)
ocsProviderClient, err := utils.NewProviderClientForStorageClient(r.ctx, r.storageClient.Spec.StorageProviderEndpoint)
if err != nil {
return nil, fmt.Errorf("failed to create a new provider client with endpoint %v: %v", r.storageClient.Spec.StorageProviderEndpoint, err)
return nil, err
}

return ocsProviderClient, nil
Expand Down
17 changes: 17 additions & 0 deletions pkg/utils/provider.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package utils

import (
"context"
"fmt"

providerClient "github.com/red-hat-storage/ocs-operator/services/provider/api/v4/client"
)

// NewProviderClientForStorageClient creates an OCS provider gRPC client for the given StorageClient.
func NewProviderClientForStorageClient(ctx context.Context, storageProviderEndpoint string) (*providerClient.OCSProviderClient, error) {
pc, err := providerClient.NewProviderClient(ctx, storageProviderEndpoint, OcsClientTimeout)
if err != nil {
return nil, fmt.Errorf("failed to create provider client with endpoint %v: %w", storageProviderEndpoint, err)
}
return pc, nil
}
6 changes: 1 addition & 5 deletions service/status-report/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,7 @@ func main() {
os.Exit(0)
}

providerClient, err := providerclient.NewProviderClient(
ctx,
storageClient.Spec.StorageProviderEndpoint,
utils.OcsClientTimeout,
)
providerClient, err := utils.NewProviderClientForStorageClient(ctx, storageClient.Spec.StorageProviderEndpoint)
if err != nil {
klog.Exitf("Failed to create grpc client with endpoint %v: %v", storageClient.Spec.StorageProviderEndpoint, err)
}
Expand Down
Loading