ceph: add support for update() from lib-bucket-provisioner

[thotz]

Description of your changes:
Recently lib-bucket-provisioner add support for update() API.
Include that on the obc implementation since it can be used to
update quota for OBC.

Which issue is resolved by this Pull Request:
Resolves #7146

Signed-off-by: Jiffin Tony Thottan thottanjiffin@gmail.com
Checklist:

• Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
• Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
• Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
• Reviewed the developer guide on Submitting a Pull Request
• Documentation has been updated, if necessary.
• Unit tests have been added, if necessary.
• Integration tests have been added, if necessary.
• Pending release notes updated with breaking and/or notable changes, if necessary.
• Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
• Code generation (make codegen) has been run to update object specifications, if necessary.

[travisn]

Please rebase on the latest master to see if the other CI fixes will help, or if there is still a CI issue to investigate. The object tests had been intermittently failing until yesterday, they should be more reliable in master now.

[travisn]

It seems the upgrade and smoke suites are failing consistently during test cleanup. The last thing in the test is:

2021-08-12 08:04:22.099486 I | testutil: Running kubectl [delete -f -]
objectbucketclaim.objectbucket.io "smoke-delete-bucket" deleted
panic: test timed out after 30m0s

The logs weren't collected unfortunately. It may be easiest to force push again and then connect to the tmate session to troubleshoot. The operator logs likely show some issue with the deletion. If it's your first time with the tmate session please do ping me or @subhamkrai.

[thotz]

It seems the upgrade and smoke suites are failing consistently during test cleanup. The last thing in the test is:

2021-08-12 08:04:22.099486 I | testutil: Running kubectl [delete -f -]
objectbucketclaim.objectbucket.io "smoke-delete-bucket" deleted
panic: test timed out after 30m0s

The logs weren't collected unfortunately. It may be easiest to force push again and then connect to the tmate session to troubleshoot. The operator logs likely show some issue with the deletion. If it's your first time with the tmate session please do ping me or @subhamkrai.

@travisn : apparently with the latest libbucket provisioner(which have update API) delete is getting stuck every time(even without operation performing any update). I cannot see the delete request is reaching the Rook/bucketprovisoner code path. When I restart the rook-operator deletion is getting succeed . Not sure whether I am missing anything key for update to work.
kubectl apply gives below warning :

 kubectl apply -f cluster/examples/kubernetes/ceph/object-bucket-claim-delete-sam.yaml 
Warning: resource objectbucketclaims/ceph-delete-bucket is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.

Any clues ??

[travisn]

You can ignore the warning from kubectl apply, the command still succeeded.
Not sure why the latest lib-bucket-provisioner request would not be reaching the operator until the restart. @BlaineEXE Any ideas?

[BlaineEXE]

You can ignore the warning from kubectl apply, the command still succeeded.
Not sure why the latest lib-bucket-provisioner request would not be reaching the operator until the restart. @BlaineEXE Any ideas?

From the code, the delete operation is ignored because of the finalizer. I think this means the delete operations are handled via upates. With the new updateSupported() method coming before ctrl.enqueueOBC(), I suspect the OBC is not getting enqueued when the deletion timestamp changes.

A question I have is what other things might be getting skipped because of this? I think the updateSupported() method only applies when there are spec changes. I think if other things besides spec change, the update should be supported still.

Specifically, I think this (from the recent commit in lib-bucket-provisioner) is not correct:

if reflect.DeepEqual(new.Spec, old.Spec) {
	return false
}

If the new spec is the same as the old spec, then something else has changed, like metadata, and the update should still be supported. We really only want to stop the update from happening if the spec is modified AND unsupported parts of the spec are modified.

When you were developing the lib-bucket-provisioner changes, were you testing them in Rook? You really should be doing that if not so we don't get broken commits to lib-bucket-provisioner.

[thotz]

You can ignore the warning from kubectl apply, the command still succeeded.
Not sure why the latest lib-bucket-provisioner request would not be reaching the operator until the restart. @BlaineEXE Any ideas?

From the code, the delete operation is ignored because of the finalizer. I think this means the delete operations are handled via upates. With the new updateSupported() method coming before ctrl.enqueueOBC(), I suspect the OBC is not getting enqueued when the deletion timestamp changes.

A question I have is what other things might be getting skipped because of this? I think the updateSupported() method only applies when there are spec changes. I think if other things besides spec change, the update should be supported still.

Specifically, I think this (from the recent commit in lib-bucket-provisioner) is not correct:

if reflect.DeepEqual(new.Spec, old.Spec) {
	return false
}

If the new spec is the same as the old spec, then something else has changed, like metadata, and the update should still be supported. We really only want to stop the update from happening if the spec is modified AND unsupported parts of the spec are modified.

When you were developing the lib-bucket-provisioner changes, were you testing them in Rook? You really should be doing that if not so we don't get broken commits to lib-bucket-provisioner.

@BlaineEXE : Thanks for debugging this issue, I added check to avoid update call if deletion stamp is set in NewController() via kube-object-storage/lib-bucket-provisioner#216, it works fine. Let me know your thoughts on the same

[BlaineEXE]

Please update this PR to reference your thotz:fixobcdeletestuck branch so that the Rook CI will validate it also.

[thotz]

Please update this PR to reference your thotz:fixobcdeletestuck branch so that the Rook CI will validate it also.

Sure

[BlaineEXE]

@thotz thanks. Merged the lib-bucket-provisioner fix. You should be good to go here.

[thotz]

@thotz thanks. Merged the lib-bucket-provisioner fix. You should be good to go here.

Done

[BlaineEXE]

Just nit comments. This looks good overall. Thanks @thotz

[thotz]

Just nit comments. This looks good overall. Thanks @thotz

@BlaineEXE : Done

[leseb]

nit feel free to address or not

[leseb]

Suggested change

	var maxObjectsInt64 int64
	var maxObjectsInt64, maxSizeInt64 int64