Updating of additonal config field of OBC

[thotz]

I am not sure whether bug or current design limitation. In rook quota for OBC is supported with help of additionalConfig string Map. When I try to update the field it is not working via kubectl edit or kubectl apply the request is not handled properly.

I can see following logs from rook-operator

I1014 10:05:53.347471       6 controller.go:210]  "msg"="reconciling claim" "key"="default/ceph-delete-bucket" 
I1014 10:05:53.347535       6 helpers.go:80]  "msg"="getting claim for key" "key"="default/ceph-delete-bucket" 
I1014 10:05:53.349255       6 helpers.go:181]  "msg"="getting ObjectBucketClaim's StorageClass" "key"="default/ceph-delete-bucket" 
I1014 10:05:53.353931       6 helpers.go:186]  "msg"="got StorageClass" "key"="default/ceph-delete-bucket" "name"="rook-ceph-delete-bucket"
I1014 10:05:53.354343       6 helpers.go:63]  "msg"="checking OBC for OB name, this indicates provisioning is complete" "key"="default/ceph-delete-bucket" "ceph-delete-bucket"=null
I1014 10:05:53.354404       6 helpers.go:65]  "msg"="provisioning already completed" "key"="default/ceph-delete-bucket" "ObjectBucket"="obc-default-ceph-delete-bucket"
I1014 10:05:53.354424       6 controller.go:246]  "msg"="skipping provision" "key"="default/ceph-delete-bucket"

If I understand correctly current code follow in lib-bucket-provisioner the request reaches syncHandler.

   // ***********************
    // Delete or Revoke Bucket
    // ***********************
    if obc.ObjectMeta.DeletionTimestamp != nil {
        log.Info("OBC deleted, proceeding with cleanup")
        return c.handleDeleteClaim(key, obc)
    }

    // *******************************************************
    // Provision New Bucket or Grant Access to Existing Bucket
    // *******************************************************
    if !shouldProvision(obc) {
        log.Info("skipping provision")
        return nil
    }

    // update the OBC's status to pending before any provisioning related errors can occur
    obc, err = updateObjectBucketClaimPhase(
        c.libClientset,

Firsts it check for whether request delete/revokes, if not checks whether provisioning is needed via shouldProvision. The shouldProvision checks whether OB name is present on OBC CRD. If exists it returns true. In case of updating OBC field this always exist and skips further part of code. Is this intended behavior?

CCing @yuvalif

[thotz]

@jeffvance @guymguym @copejon any thoughts on above issue?

[jeffvance]

shouldProvision returns true when obc.bucketName is empty, meaning the OBC has not been bound to the OB. In the log we see that the OBC is already bound to the OB so no provisioning is performed. I don't see a connection between this and additionalConfig

[yuvalif]

@jeffvance the question is in the context bucket notification CRD. the plan is to referenced it from the OBC (via additionalconfig). we want to cover the case where bucket notification could be configured on a bucket after it was created. for that we need to be triggered when OBC is modified (new additionlconfig value is added). note that no bucket provisioning is required at this point, just triggering the notification provisioning hook.

[jeffvance]

@yuvalif thanks for clarification. In quickly looking at the code code I don't see that it handles update events. The update key is added to the work queue but is not processed in syncHandler.

[yuvalif]

I guess that all bucket provisioning related fields of the OBC are not supposed to change. do you think it makes sense to add logic that would check if the additionalConfiig map changed, and allow others (e.g bucket notification code in rook) to register to that event?

[jeffvance]

The code en-queues only the new obc. Maybe we can compare the obc retrieved from the GET call against the obc from the informer cache to determine which obc fields changed?

[thotz]

The code en-queues only the new obc. Maybe we can compare the obc retrieved from the GET call against the obc from the informer cache to determine which obc fields changed?

IMO values in AdditionalConfig need to compared between new and old OBC. Better to keep other fields like storageclass, bucketname immutable and throws an error if user tries to change it. But I am not sure how(or is right to) values inside AdditionalConfig can be compared since key-value pairs are opaque to lib-bucket-provisioner?

[yuvalif]

Better to keep other fields like storageclass, bucketname immutable and throws an error if user tries to change it

this is a nice enhancement regardless of bucket notifications. currently we just silently ignore updates to immutable fields, would be better to error on that.

[jeffvance]

currently we just silently ignore updates to immutable fields, would be better to error on that.

True. We do the same with the OB too (no OB watch even).

values inside AdditionalConfig can be compared since key-value pairs are opaque to lib-bucket-provisione

It is tricky to compare maps because you cannot expect the keys to be retrieved in the same order. Go's "reflect" pkg should work, eg: reflect.DeepEqual(a, b)

[thotz]

currently we just silently ignore updates to immutable fields, would be better to error on that.

True. We do the same with the OB too (no OB watch even).

values inside AdditionalConfig can be compared since key-value pairs are opaque to lib-bucket-provisione

It is tricky to compare maps because you cannot expect the keys to be retrieved in the same order. Go's "reflect" pkg should work, eg: reflect.DeepEqual(a, b)

@jeffvance Cool. How will we able to move forward here? Will you able to push a PR ?

[jeffvance]

Cool. How will we able to move forward here? Will you able to push a PR ?

Not likely. I do have some time to review lib PRs. Are you able to @thotz ?

[thotz]

Cool. How will we able to move forward here? Will you able to push a PR ?

Not likely. I do have some time to review lib PRs. Are you able to @thotz ?

Sure I will give a try then

[thotz]

@jeffvance : It looks a bit complex than I thought. If understand correctly in the library has Provisioner which internally creates an obcController. From Rook, a new provisioner is initialized and Started for handling OBC requests. Provisioner works on a queue filled by the obcController. This happens based on the events received in the controller and the following code handles it :+1:

   obcInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
        AddFunc: ctrl.enqueueOBC,
        UpdateFunc: func(old, new interface{}) {
            oldObc := old.(*v1alpha1.ObjectBucketClaim)
            newObc := new.(*v1alpha1.ObjectBucketClaim)
            if newObc.ResourceVersion == oldObc.ResourceVersion {
                // periodic re-sync can be ignored
                return
            }   
            // if old and new both have deletionTimestamps we can also ignore the
            // update since these events are occurring on an obc marked for deletion,
            // eg. extra finalizers being added and deleted.
            if newObc.ObjectMeta.DeletionTimestamp != nil && oldObc.ObjectMeta.DeletionTimestamp != nil {
                return
            }   
            // handle this update
            ctrl.enqueueOBC(new)
        },  
        DeleteFunc: func(obj interface{}) {
            // Since a finalizer is added to the obc and thus the obc will remain
            // visible, we do not need to handle delete events here. Instead, obc
            // deletes are indicated by the deletionTimestamp being non-nil.
            return
        },
    })

If I understand correctly UpdateFunc needs to modify for fixing this issue. But I am not sure how the queue will be effective in the scenario. Do we need to change the current design for Provisioner.Start() as well?

[jeffvance]

If I understand correctly UpdateFunc needs to modify for fixing this issue. But I am not sure how the queue will be effective in the scenario.

The code in your comment above added the "namespace/name" of the new object to the work queue for update events. But the syncHandler methods needs to be enhanced to process update events. Today it only handles delete and add events.

I think an approach would be to keep the delete logic where it is with no changes. If the timestamp is nil then we have either add or update. The shouldProvision func checks if the OBC is bound to the OB and if it returns false we know we have an update event. However, I think there are windows in this logic where the OBC is partially updated due to processing the new OBC and then an update event comes in. As an initial effort we can assume that if shouldProvision returns false we need to handle update, otherwise we have an add event. You'll need to write a new handleUpdateClaim method which will get the old OBC (not sure if better to use API Get or retrieve from cache) and then compare additionalConfig using reflect.DeepEqual.

What do you think?

[yuvalif]

please see discussion here: https://github.com/rook/rook/pull/6425#discussion_r539844190 if we are going to use labels there, than there is not need for this enhancement anymore. unless there is another motivation, IMO we can close this issue.