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.

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 would test only OBC deletin (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

rewantsoni · 2026-03-04T11:00:00Z

internal/controller/obc_controller.go

+	if err := r.Get(r.ctx, types.NamespacedName{Name: ownerRef.Name}, storageClient); err != nil {
+		return nil, fmt.Errorf("get StorageClient %q (owner of StorageClass %q): %w", ownerRef.Name, storageClassName, err)
+	}
+	if storageClient.Status.ConsumerID == "" || storageClient.Spec.StorageProviderEndpoint == "" {


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

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...

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

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?

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

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

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

internal/controller/obc_controller.go

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

rewantsoni · 2026-03-09T05:55:13Z

internal/controller/obc_controller.go

+	"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"


Please format the imports as

standard pkg others

Thanks, I'll look again.

rewantsoni · 2026-03-09T05:56:13Z

internal/controller/obc_controller.go

+	err := r.Get(r.ctx, req.NamespacedName, obc)
+	if err != nil {


Could you use inline errors?

Yes, thanks.

rewantsoni · 2026-03-09T05:59:18Z

internal/controller/obc_controller.go

+	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 {


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?

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).

rewantsoni · 2026-03-09T06:01:43Z

internal/controller/obc_controller.go

+	if err := r.Get(r.ctx, types.NamespacedName{Name: ownerRef.Name}, storageClient); err != nil {
+		return nil, fmt.Errorf("get StorageClient %q (owner of StorageClass %q): %w", ownerRef.Name, storageClassName, err)
+	}
+	if storageClient.Status.ConsumerID == "" || storageClient.Spec.StorageProviderEndpoint == "" {


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

…rEndpoint Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

like in storage client controller Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

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 <57721533+shirady@users.noreply.github.com>

CR changes - remove check of empty string name of storageclass

5295f7f

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

CR changes - loop with slices.IndexFunc OwnerReferences of storageclass

e2ebed2

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

CR changes - edit comment on the function releaseAndDeleteOB

4ba063b

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

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 <57721533+shirady@users.noreply.github.com>

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 <57721533+shirady@users.noreply.github.com>

remove resource deletion

831b83e

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

reconcile on spec updates

4b66106

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

RBAC update (no resource deletion)

4159530

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

shirady added 2 commits March 5, 2026 13:22

simplify the predicate function

8f8bd07

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

simplicy predicate to use GenerationChangedPredicate

1f5e9be

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

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

shirady added 6 commits March 9, 2026 08:16

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

bf2d809

…rEndpoint Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

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

2773353

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

CR changes - move the error to inline and use kerrors

74a1bab

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

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

6780e67

like in storage client controller Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

split lines

b11eb72

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

setFailedPhaseIfNotDeleting only on empty status phase

da8406a

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Milestone

Development

Uh oh!

3 participants

shirady commented Feb 25, 2026 •

edited

Loading