OBC Controller and Reconcile by shirady · Pull Request #520 · red-hat-storage/ocs-client-operator

shirady · 2026-02-25T13:58:50Z

Describe the problem

Part of RHSTOR-6230
Continue of PRs red-hat-storage/ocs-operator#3692 by calling the Notify when an OBC is created and deleted.

Explain the changes

Register the OBC and OBC types.
Note: to avoid direct import of lib-bucket-provision, I had to add them manually.
In case we decide to import it directly, it can be in the same style as others.
OBC can be created in all namespaces, so add the needed changes for it.
Create the OBC controller, which is running on the client cluster (that doesn't have NooBaa installed and the OBC controller from lib-bucket-provisionser), the controller with predicate so the reconcile loop will be triggered on specific changes only: creation, deletion, and update only by setting the deletion timestamp.
In creation - add a finalizer and call Notify.
Notes:

On failures, change the status to Failed.
Decided at this point not to change to Pending.
To identify the storage client (as there can be a couple of storage clients in the client cluster), we use the ownerReferences from the storageclass.
To be able to list the OBCs by storage client - added a label with that information on the OBC (plan to use it as part of preventing deletion of storage client in case it has OBCs).

In deletion call Notify and remove finalizer.
Adding permission to the controller for the above actions.
Changes to RBAC after running make manifests and make bundle.

Testing Instructions:

Manual Test:

Set 2 clusters - provider and client, after onboarding. The client cluster with this PR code changes, and the provider cluster with Implement Notify - OBC Created and Deleted ocs-operator#3692 code changes (Notify implementation).
On the client cluster, install the OBC CRD manually from noobaa-operator repo: kubectl apply -f ./noobaa-operator/deploy/obc/objectbucket.io_objectbucketclaims_crd.yaml
Set the permission manually:

kubectl create clusterrole ocs-client-operator-obc-role \
  --verb=get,list,watch,patch,update \
  --resource=objectbucketclaims.objectbucket.io

# Add the status subresource rule
kubectl patch clusterrole ocs-client-operator-obc-role --type='json' -p='[
  {"op": "add", "path": "/rules/-", "value": {
    "apiGroups": ["objectbucket.io"],
    "resources": ["objectbucketclaims/status"],
    "verbs": ["get", "patch", "update"]
  }}
]'

kubectl create clusterrolebinding ocs-client-operator-obc-rolebinding \
  --clusterrole=ocs-client-operator-obc-role \
  --serviceaccount=openshift-storage-client:ocs-client-operator-controller-manager

Add a storage cluster with ownerReferences of the storage client (you can copy from another storage class on the cluster) manually with a temp finalizer (else it is deleted).
Example of storage class:

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: openshift-storage.noobaa.io
  finalizers:
  - ocs-client-operator.ocs.openshift.io/temp
  ownerReferences:
  - apiVersion: ocs.openshift.io/v1alpha1
    blockOwnerDeletion: true
    controller: true
    kind: StorageClient
    name: storage-client-2202
    uid: 841f7ac8-17cb-4d34-8ee2-7c6ebaf82103
parameters:
  bucketclass: noobaa-default-bucket-class
provisioner: openshift-storage.noobaa.io/obc
reclaimPolicy: Delete
volumeBindingMode: Immediate

Creation:

Client cluster: create OBC on client cluster - should be created without status change.
Example of OBC:

apiVersion: objectbucket.io/v1alpha1
kind: ObjectBucketClaim
metadata:
  name: shira-obc-2302
  namespace: openshift-storage-client
spec:
  storageClassName: openshift-storage.noobaa.io
  generateBucketName: shira-obc-2302
  additionalConfig: {}

Note: OBC can be created in any namespace. If you want to test on another namespace - create the namespace (kubectl create ns <namespacename>) and change the namespace value in the yaml.

Provider cluster: check that the OBC on the provider cluster is created and in Bound phase.

Deletion:
Since we do not have the copy mechanism implemented yet, we will test only the OBC deleted (without additional resources):

Manual steps:
a. Install the OB CRD (kubectl apply -f noobaa-operator/deploy/obc/objectbucket.io_objectbuckets_crd.yaml)
b. Change the clusterRole (use Ai generated file from the kubebuilder marks).
Client cluster: delete OBC on client cluster - should be deleted.
Provider cluster: check that the OBC was deleted (with all related resources).

openshift-ci · 2026-02-25T13:58:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shirady
Once this PR has been reviewed and has the lgtm label, please assign nb-ohad for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

leelavg · 2026-02-26T07:23:51Z

install the OBC CRD manually from noobaa-operator repo

unless we finalize on how we are bringing these CRDs we can't have this PR in current format, can there be a dynamic caching mechanism for this controller, like, when the PR is merged and client-op is deployed standalone even without OBC CRD client-op will not error out and serve existing cases?

manually with a temp finalizer (else it is deleted).

why? it gets deleted if the ownerref is wrong or the server doesn't send this resource? I suppose it's the latter and may need a fix, good find.

leelavg

didn't review the controller yet as pr is in draft.

leelavg · 2026-02-26T07:26:07Z

cmd/main.go

+				&nbv1.ObjectBucketClaim{}: {
+					Namespaces: map[string]cache.Config{corev1.NamespaceAll: {}},
+				},


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

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.

shirady · 2026-02-26T12:04:47Z

install the OBC CRD manually from noobaa-operator repo

unless we finalize on how we are bringing these CRDs we can't have this PR in current format, can there be a dynamic caching mechanism for this controller, like, when the PR is merged and client-op is deployed standalone even without OBC CRD client-op will not error out and serve existing cases?

From what I understand from @nb-ohad for testing we can install the CRD that we need.
If you think that we need to add something specific for the general case, we can add.
The qoute was taken from the manual testing that was done.

manually with a temp finalizer (else it is deleted).

why? it gets deleted if the ownerref is wrong or the server doesn't send this resource? I suppose it's the latter and may need a fix, good find.

Yes, you are correct. We would need to make changes. As we didn't want to be blocked at this point, it was suggested that we add the finalizer for testing purposes.

shirady · 2026-03-04T10:37:21Z

@leelavg @rewantsoni, Please take a look

internal/controller/obc_controller.go

Signed-off-by: shirady <[email protected]>

internal/controller/obc_controller.go

…rEndpoint Signed-off-by: shirady <[email protected]>

Signed-off-by: shirady <[email protected]>

like in storage client controller Signed-off-by: shirady <[email protected]>

Signed-off-by: shirady <[email protected]>

…erClient Signed-off-by: shirady <[email protected]>

will be used in deletion phase of the storage client before calling offboarding Signed-off-by: shirady <[email protected]>

internal/controller/obc_controller.go

Signed-off-by: shirady <[email protected]>

rewantsoni

The PR looks good expect one comment regarding adding the storageClient label to the OBC

rewantsoni · 2026-03-12T12:56:11Z

internal/controller/obc_controller.go

+	ObcFinalizer                       = nbv1.ObjectBucketFinalizer
+	ObjectBucketClaimStatusPhaseFailed = "Failed"


Instead of creating a copy of the Phase and Finalizer, can we not use it from the pkg directly?

I will use it directly, thanks.
For the status, currently I don't have it, but I raised a PR to have it -
noobaa/noobaa-operator#1823

rewantsoni · 2026-03-12T13:00:40Z

internal/controller/obc_controller.go

+		if r.obc.GetDeletionTimestamp().IsZero() && r.obc.Status.Phase == "" {
+			r.obc.Status.Phase = ObjectBucketClaimStatusPhaseFailed
+		}


Suggested change

if r.obc.GetDeletionTimestamp().IsZero() && r.obc.Status.Phase == "" {

r.obc.Status.Phase = ObjectBucketClaimStatusPhaseFailed

}

if r.obc.Status.Phase == "" {

r.obc.Status.Phase = ObjectBucketClaimStatusPhaseFailed

}

We have a check for deletionTimestamp above

Sure, thanks

rewantsoni · 2026-03-12T13:04:01Z

internal/controller/obc_controller.go

+	if r.obc.Status.Phase == ObjectBucketClaimStatusPhaseFailed {
+		r.obc.Status.Phase = ""
+	}


Can this cause a deadlock? I am thinking of a scenario, if for some reason the OBC on the provider reached a failed state, we update the Status of OBC from the storage_client controller to a failed state, and then the OBC controller updates the Phase to "".

From what I checked in the code, there no place where the status phase is Failed.
I added the status change since it was raised as a comment in the design to reflect it.
To be on the safe side I will not change the phase, and the way to debug it would be to check the logs and see the error.

and remove redandunt check of deletion timestamp Signed-off-by: shirady <[email protected]>

Signed-off-by: shirady <[email protected]>

shirady · 2026-03-15T06:26:33Z

@nb-ohad , please take a look

nb-ohad · 2026-03-15T10:41:19Z

pkg/utils/provider.go

+func NewProviderClientForStorageClient(
+	ctx context.Context,
+	storageProviderEndpoint string,
+) (*providerClient.OCSProviderClient, error) {
+	ocsProviderClient, 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 ocsProviderClient, nil
+}


This is a meaningless util, it does nothing but call another function. And it does not even allow us to configure the error message in accordance with the given context of the call site which means that the call site now needs to wrap the underlaying error with another error. I don't get why this is being introduced.

This function also limits all call site to share the same timeout value, which seems wrong. The next developer who will need a different value for timeout will now add the timeout as an argument (which I don't blame them), but then you cannot even use the timeout as a reason for the existence of this function.

In addition, now a reader need to have a context switch when reading the call site code with no apparent value.

It seems like this function was only introduced to "skip" typing the timeout constant in multiple places (which is kind of the wrong reason for refactoring). We should not create code that limits the usability of our libraries. If we wanted a fixed timeout, or a fixed error message, we could have just hard coded it into the NewProviderClient function itself, we chose not to do so and provide the caller the flexibility. This function seems to go directly against that design decision.

@shirady Could you please make a case for this function, or just revert the change.

CC: @leelavg

nb-ohad · 2026-03-15T10:50:33Z

internal/controller/obc_controller.go

+		predicate.NewPredicateFuncs(func(obj client.Object) bool {
+			return !obj.GetDeletionTimestamp().IsZero()
+		}),


This predicate means that we will not handle "deleting". In that case who should be handling the deleting phase? shouldn't this controller be owning that code path?

nb-ohad · 2026-03-15T10:52:42Z

internal/controller/obc_controller.go

+	r.obc.Name = req.Name
+	r.obc.Namespace = req.Namespace
+
+	r.log.Info("Starting reconcile iteration for OBC", "req", req)


I would suggest adding the NamespaceName from the request (the only thing that is actually on the request) as a WithValues on the logger, to make sure that every log message will have the context of the obc that it refers to. This also means there will be no need to add the req value in this log statement.

nb-ohad · 2026-03-15T10:59:29Z

internal/controller/obc_controller.go

+	if r.obc.Status.Phase != initialPhase {
+		statusErr = r.Client.Status().Update(r.ctx, &r.obc)
+		if statusErr != nil {
+			r.log.Error(statusErr, "Failed to update OBC status.")
+		}
+	}


This is wrong! By design of the controller pattern, each reconcile start with a conceptual empty status, because each reconcile runs all of the steps from step 0 every time. This also mean it is a bug/mistake to take the output of a previous reconcile and use it as input for the current reconcile.

In addition, the fact that the phase field on the status didn't change, don't not mean that other fields didn't change. This mean that the phase field cannot be a good indication for an skipping the update operation. Even if one makes a case that there are no other fields, I would argue that the next developer might add additional fields then we have a subtle bug here that will be hard to detect.

If you really want to run the update command only when the status change you need to condition on a deep equal operation, but I think this is an over kill as this controller should only run when there is a change on the obc so I don't think there is any harm in running the update operation on each reconcile.

Suggested change

if r.obc.Status.Phase != initialPhase {

statusErr = r.Client.Status().Update(r.ctx, &r.obc)

if statusErr != nil {

r.log.Error(statusErr, "Failed to update OBC status.")

}

}

if statusErr := r.Client.Status().Update(r.ctx, &r.obc); statusErr != nil {

r.log.Error(statusErr, "Failed to update OBC status.")

}

CC: @leelavg

nb-ohad · 2026-03-15T11:01:12Z

internal/controller/obc_controller.go

+	if !r.obc.GetDeletionTimestamp().IsZero() {
+		return r.deletionPhase(ocsProviderClient, storageClient)
+	}


This goes against the predicate that was set on the setupWithManager. This is one example of why that predicate need to get removed

nb-ohad · 2026-03-15T11:05:04Z

internal/controller/obc_controller.go

+	if !r.obc.GetDeletionTimestamp().IsZero() {
+		return r.deletionPhase(ocsProviderClient, storageClient)
+	}


If we have a branch based on GetDeletionTimestamp I would have structure the code to be more readable and apparent. this means an if ... else ... structure. And if you already decided that deletion phase gets its own function (instead of implemented here) then it make more sense to give the same treatment to the second code flow. I would also prefer that we handle the common case first (creation/update) and the uncommon case later (as a user reads code from top to bottom).

With all of these changes the code should look something like

if r.obc.GetDeletionTimeStamp.IsZero() { r.handlerObcCreationOrUpdate(...) } else { r.handleObcDeletion(...) }

nb-ohad · 2026-03-15T11:06:10Z

internal/controller/obc_controller.go

+	}
+
+	if _, err := ocsProviderClient.NotifyObcCreated(r.ctx, storageClient.Status.ConsumerID, &r.obc); err != nil {
+		r.log.Error(err, "failed to notify provider of OBC created/updated", "namespaced/name", client.ObjectKeyFromObject(&r.obc))


The obc namespace name / object key, should already be on the logger

Suggested change

r.log.Error(err, "failed to notify provider of OBC created/updated", "namespaced/name", client.ObjectKeyFromObject(&r.obc))

r.log.Error(err, "failed to notify provider of OBC created/updated")

@shirady Please apply that to all of the log messages

nb-ohad · 2026-03-15T11:07:50Z

internal/controller/obc_controller.go

+) (ctrl.Result, error) {
+	r.log.Info("OBC deleted", "namespaced/name", client.ObjectKeyFromObject(&r.obc))
+
+	obcNamespacedName := types.NamespacedName{Namespace: r.obc.Namespace, Name: r.obc.Name}


The Namespaced name already exist on the req, I am not sure why we need to get it from the obc.
And even if we do, we can get it using client.ObjectKeyFromObject(&r.obc)

nb-ohad · 2026-03-15T11:10:50Z

internal/controller/obc_controller.go

+// getStorageClientFromStorageClass returns the StorageClient that owns the given StorageClass (via ownerReference).
+func (r *obcReconcile) getStorageClientFromStorageClass(storageClassName string) (*v1alpha1.StorageClient, error) {
+	sc := &storagev1.StorageClass{}
+	if err := r.Get(r.ctx, types.NamespacedName{Name: storageClassName}, sc); err != nil {


Please prefer ObjectKeyFromObject to manually creating a NamespcedName / ObjectKey

nb-ohad · 2026-03-15T11:12:14Z

internal/controller/obc_controller.go

+}
+
+// getStorageClientFromStorageClass returns the StorageClient that owns the given StorageClass (via ownerReference).
+func (r *obcReconcile) getStorageClientFromStorageClass(storageClassName string) (*v1alpha1.StorageClient, error) {


Why aren't we just creating a client index for SC by client? If we do that this entire function becomes redundant and we only need to have a simple list operation (with a limit of 1) to get the client that is associated with a given sc.

shirady marked this pull request as draft February 25, 2026 13:58

openshift-ci bot added the do-not-merge/work-in-progress label Feb 25, 2026

leelavg reviewed Feb 26, 2026

View reviewed changes

shirady force-pushed the obc-controller-implementation branch from a8fa7f2 to d7c518a Compare February 26, 2026 11:59

This was referenced Mar 2, 2026

Update provider api module for notify rpc calls #525

Merged

Add ObjectBucketClaimStatus and the different statuses in obc_types.go noobaa/noobaa-operator#1823

Open

shirady force-pushed the obc-controller-implementation branch from 159612a to 622dfff Compare March 4, 2026 08:25

pull-request-size bot added the size/L label Mar 4, 2026

shirady force-pushed the obc-controller-implementation branch 3 times, most recently from e1d0de3 to 2f407e5 Compare March 4, 2026 09:05

shirady marked this pull request as ready for review March 4, 2026 10:37

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 4, 2026

rewantsoni suggested changes Mar 4, 2026

View reviewed changes

openshift-ci bot assigned rewantsoni Mar 4, 2026

shirady added 4 commits March 4, 2026 17:19

controllers: OBC controller with Notify gRPC calls

d199af5

Signed-off-by: shirady <[email protected]>

CR changes - remove check of empty string name of storageclass

5295f7f

Signed-off-by: shirady <[email protected]>

CR changes - loop with slices.IndexFunc OwnerReferences of storageclass

e2ebed2

Signed-off-by: shirady <[email protected]>

CR changes - edit comment on the function releaseAndDeleteOB

4ba063b

Signed-off-by: shirady <[email protected]>

shirady force-pushed the obc-controller-implementation branch from 2f407e5 to d45478a Compare March 4, 2026 16:06

CR changes - move providerClient.NewProviderClient to util package

5d62d88

Signed-off-by: shirady <[email protected]>

shirady force-pushed the obc-controller-implementation branch from d45478a to 5d62d88 Compare March 5, 2026 07:31

shirady added 4 commits March 5, 2026 10:31

CR changes - 1 deletion function (and get)

f56369b

Signed-off-by: shirady <[email protected]>

remove resource deletion

831b83e

Signed-off-by: shirady <[email protected]>

reconcile on spec updates

4b66106

Signed-off-by: shirady <[email protected]>

RBAC update (no resource deletion)

4159530

Signed-off-by: shirady <[email protected]>

shirady added 2 commits March 5, 2026 13:22

simplify the predicate function

8f8bd07

Signed-off-by: shirady <[email protected]>

simplicy predicate to use GenerationChangedPredicate

1f5e9be

Signed-off-by: shirady <[email protected]>

shirady changed the title ~~RHSTOR-6230 | OBC Controller and Reconcile~~ OBC Controller and Reconcile Mar 8, 2026

shirady mentioned this pull request Mar 8, 2026

Add noobaa storage class to the list that can be disribute red-hat-storage/ocs-operator#3729

Open

rewantsoni suggested changes Mar 9, 2026

View reviewed changes

internal/controller/obc_controller.go Show resolved Hide resolved

internal/controller/obc_controller.go Outdated Show resolved Hide resolved

internal/controller/obc_controller.go Outdated Show resolved Hide resolved

internal/controller/obc_controller.go Outdated Show resolved Hide resolved

shirady added 8 commits March 9, 2026 08:16

CR changes - remove codtion to check consumer UUID and StorageProvide…

bf2d809

…rEndpoint Signed-off-by: shirady <[email protected]>

import list - move the ocs-client-operator to pkg part

2773353

Signed-off-by: shirady <[email protected]>

CR changes - move the error to inline and use kerrors

74a1bab

Signed-off-by: shirady <[email protected]>

CR chnages - separate to reconcilePhases and update status in the end

6780e67

like in storage client controller Signed-off-by: shirady <[email protected]>

split lines

b11eb72

Signed-off-by: shirady <[email protected]>

setFailedPhaseIfNotDeleting only on empty status phase

da8406a

Signed-off-by: shirady <[email protected]>

split function signature in provider utils and rename pc to ocsProvid…

413e284

…erClient Signed-off-by: shirady <[email protected]>

add label of storageClient on the OBC

cec15a5

will be used in deletion phase of the storage client before calling offboarding Signed-off-by: shirady <[email protected]>

This was referenced Mar 10, 2026

Prevent offboarding when OBC exists #531

Open

Block Offboarding of Storage Consumer with OBCs [hold] red-hat-storage/ocs-operator#3696

Open

rewantsoni suggested changes Mar 11, 2026

View reviewed changes

internal/controller/obc_controller.go Outdated Show resolved Hide resolved

internal/controller/obc_controller.go Show resolved Hide resolved

internal/controller/obc_controller.go Outdated Show resolved Hide resolved

internal/controller/obc_controller.go Outdated Show resolved Hide resolved

shirady added 3 commits March 11, 2026 12:59

remove helper function setFailedPhaseIfNotDeleting

8925e1a

Signed-off-by: shirady <[email protected]>

remove createOrUpdatePhase function

bb90203

Signed-off-by: shirady <[email protected]>

remove the added label with the storageclient

caec16f

Signed-off-by: shirady <[email protected]>

rewantsoni suggested changes Mar 12, 2026

View reviewed changes

shirady added 5 commits March 12, 2026 15:45

CR changes - use the obc finalizer directly

888c620

and remove redandunt check of deletion timestamp Signed-off-by: shirady <[email protected]>

do not change the phase to Failed

c2e1212

Signed-off-by: shirady <[email protected]>

remove permission on OBC status

8f5226f

Signed-off-by: shirady <[email protected]>

after make manifests

78c3ff0

Signed-off-by: shirady <[email protected]>

fter make bundle

626e7f6

Signed-off-by: shirady <[email protected]>

nb-ohad requested changes Mar 15, 2026

View reviewed changes

openshift-ci bot assigned nb-ohad Mar 15, 2026

		ObcFinalizer = nbv1.ObjectBucketFinalizer
		ObjectBucketClaimStatusPhaseFailed = "Failed"

	r.log.Error(err, "failed to notify provider of OBC created/updated", "namespaced/name", client.ObjectKeyFromObject(&r.obc))
	r.log.Error(err, "failed to notify provider of OBC created/updated")

Conversation

shirady commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the problem

Explain the changes

Testing Instructions:

Manual Test:

Uh oh!

openshift-ci bot commented Feb 25, 2026

Uh oh!

leelavg commented Feb 26, 2026

Uh oh!

leelavg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shirady commented Feb 26, 2026

Uh oh!

shirady commented Mar 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rewantsoni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shirady commented Mar 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

shirady commented Feb 25, 2026 •

edited

Loading