Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

object: check obc provisioner for bucket notification #11975

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

thotz
Copy link
Contributor

@thotz thotz commented Mar 24, 2023

Description of your changes:
obc-label-controller needs to check only the obc provisioned by the Rook operator.
Check other OBCs result in errors from the controller and keep on reconcilling

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@thotz thotz requested a review from yuvalif March 24, 2023 10:12
Copy link
Member

@galexrt galexrt left a comment

Choose a reason for hiding this comment

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

LGTM

@thotz thotz force-pushed the bucket-notification-use-obc-from-rook branch 3 times, most recently from dc3b9e9 to 408feb4 Compare March 24, 2023 12:58
@yuvalif
Copy link
Contributor

yuvalif commented Mar 25, 2023

@thotz maybe a simpler way is to use the error returned from the name parsing function as an indication that the bucket was provisioned by an external provisioner, and let the bucket notification controller ignore it without an error?

@thotz thotz force-pushed the bucket-notification-use-obc-from-rook branch from 408feb4 to 8765e37 Compare March 27, 2023 11:46
@travisn
Copy link
Member

travisn commented Mar 27, 2023

@thotz The object integration tests are failing with this error:

=== RUN   TestCephObjectSuite/TestWithTLS/perform_s3_operations_and_check_for_notifications/check_for_put_bucket_notification
2023-03-27 12:10:33.431665 D | exec: Running command: kubectl logs --selector=app=sample-http-server --tail 5
    ceph_bucket_notification_test.go:142: 
        	Error Trace:	/home/runner/work/rook/rook/tests/integration/ceph_bucket_notification_test.go:142
        	Error:      	Should be true
        	Test:       	TestCephObjectSuite/TestWithTLS/perform_s3_operations_and_check_for_notifications/check_for_put_bucket_notification

@BlaineEXE
Copy link
Member

Thanks for the good test updates in the PR. I think it looks good other than the small integration test bug Travis pointed out

@thotz
Copy link
Contributor Author

thotz commented Mar 28, 2023

@thotz The object integration tests are failing with this error:

=== RUN   TestCephObjectSuite/TestWithTLS/perform_s3_operations_and_check_for_notifications/check_for_put_bucket_notification
2023-03-27 12:10:33.431665 D | exec: Running command: kubectl logs --selector=app=sample-http-server --tail 5
    ceph_bucket_notification_test.go:142: 
        	Error Trace:	/home/runner/work/rook/rook/tests/integration/ceph_bucket_notification_test.go:142
        	Error:      	Should be true
        	Test:       	TestCephObjectSuite/TestWithTLS/perform_s3_operations_and_check_for_notifications/check_for_put_bucket_notification

I thought the provisioner's name was the combination of Rook Operator Namespace and "ceph.rook.io/bucket". But the current code uses the ceph cluster namespace. I am confused about how to get that here. In the Bucketprovisioner the are finding the ceph cluster details using the following,

cephCluster := &cephv1.CephCluster{}
	err := r.client.Get(r.opManagerContext, request.NamespacedName, cephCluster)
	if err != nil {
		if kerrors.IsNotFound(err) {
			logger.Infof("no ceph cluster found in %+v. not deploying the bucket provisioner", request.NamespacedName)
			return reconcile.Result{}, nil
		}
		// Error reading the object - requeue the request.
		return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed to get the ceph cluster")
	}

	if !cephCluster.DeletionTimestamp.IsZero() {
		logger.Debug("ceph cluster is being deleted, no need to reconcile the bucket provisioner")
		return reconcile.Result{}, nil
	}

	if !cephCluster.Spec.External.Enable && cephCluster.Spec.CleanupPolicy.HasDataDirCleanPolicy() {
		logger.Debug("ceph cluster has cleanup policy, the cluster will soon go away, no need to reconcile the bucket provisioner")
		return reconcile.Result{}, nil
	}

The request.NamespacedName should be the namespace of OBC right? AFAIK obc can exist in any namespace, not bind to the namespace of ceph cluster or object store

@travisn
Copy link
Member

travisn commented Mar 28, 2023

The namespace of the cephcluster is the same as the namespace of the object store. Would this method help? getNSNameFromAdditionalState()?

@thotz
Copy link
Contributor Author

thotz commented Mar 29, 2023

The namespace of the cephcluster is the same as the namespace of the object store. Would this method help? getNSNameFromAdditionalState()?

That function was erroring out while handling OBC which were not provisioned by Rook. So I thought handling this check before fetching details about object store in the OBC

@thotz thotz force-pushed the bucket-notification-use-obc-from-rook branch 2 times, most recently from 08b9e75 to 76dd62d Compare March 30, 2023 14:47
Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

Just adding a comment that lots of CI are failing.

obc-label-controller needs to check only obc provisioned by Rook operator.

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
@thotz thotz force-pushed the bucket-notification-use-obc-from-rook branch from 76dd62d to 01a3dd0 Compare March 31, 2023 09:26
@travisn travisn merged commit e85ef37 into rook:master Mar 31, 2023
46 of 50 checks passed
@thotz thotz deleted the bucket-notification-use-obc-from-rook branch April 1, 2023 08:19
mergify bot added a commit that referenced this pull request Apr 3, 2023
object: check obc provisioner for bucket notification (backport #11975)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants