test: add more test cases for bucket notfication #9169

thotz · 2021-11-15T12:55:58Z

Description of your changes:
Added following test cases for bucket notification integration test
suite:

different order: OBC - Topic - Notification
different order: OBC - Notification - Topic
adding a label to an existing OBC
deleting a label from an existing OBC

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

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

tests/integration/ceph_bucket_notification_test.go

tests/framework/clients/bucket.go

tests/integration/ceph_bucket_notification_test.go

thotz · 2021-11-17T05:24:13Z

tests/framework/utils/k8s_helper.go

@@ -56,6 +57,7 @@ type K8sHelper struct {
 	remoteExecutor   *exec.RemotePodCommandExecutor
 	Clientset        *kubernetes.Clientset
 	RookClientset    *rookclient.Clientset
+	BucketClientset  *bktclient.Clientset


if it is not okay to add bucketclient, I will remove it here and define in the test file

Yeah, I think it might be best to add this to the TestClient the same as the BucketClient. Not here. https://github.com/rook/rook/blob/master/tests/framework/clients/test_client.go

IMO it make more sense to add here than in TestClient{}, bucketclient is similar to rookclient to execute apis directly on OBC and it requires rest.Config

K8SHelper has all very low-level clients. What is appropriate for the K8SHelper, based on current context might be a low-level OBCClientset, but the overall of the BucketClientset I think should be in TestClient because it is much more than just a clientset. It has many more helper methods. I think adding the bucket clientset here would be good, and adding the core of BucketClientset with its helper methods to TestClient is the best thing.

There is already bucket client as part of TestClient, here it more like low-level k8s operations like updating labels which can be easily with bucket client apis

Unless this changed recently, I may have been misinterpreting something. This looks good here as BucketClientset *bktclient.Clientset with other stuff in TestClient.

tests/integration/ceph_bucket_notification_test.go

BlaineEXE · 2021-11-18T23:26:36Z

tests/integration/ceph_bucket_notification_test.go

+		created = utils.Retry(12, 2*time.Second, "notification is created", func() bool {
+			return helper.NotificationClient.CheckNotification(reverseNotificationName)
 		})


This is verifying the notification is created, but does it expect the CephBucketNotification resource to process and report a status? Is this checking the Ceph RGW's notification, or the CephBucketNotification resource?

I think my confusion here is a case of not having very good naming. What can we rename CheckNotification to in order to show it's purpose more clearly? What other methods can we create to reuse in these tests that can help others read them more easily without having to understand the deep inner workings?

Not really, the backend notification will be created both obc and topics are ready. I will rename the CheckNotification into CheckNotificationCR and CephBucketNotification into CephBucketNotificationSetonRGW

BlaineEXE

I have identified several places in the integration test where what is being tested isn't a valid check. At minimum, these aren't fully verifying the functionality we intend. Additionally, they may contribute to tests being flaky if the behavior in Rook is bugged.

The general trend I can identify is that for the tests that want to ensure something is not changing after some kind of update, the tests are not ensuring that Rook has finished reconciling the update before verifying the end state.

One unrelated question: are we testing that we can create multiple notifications for a single topic?

Another questions I have, possibly needing input from @yuvalif : are we testing the functional end-to-end to verify that notifications from buckets actually get sent to topics?

BlaineEXE · 2021-11-23T17:17:41Z

tests/integration/ceph_bucket_notification_test.go

+		})
+		t.Run("check CephBucketNotification reverse-notification created for bucket after creating topic", func(t *testing.T) {
+			// check whether bucket notification added, should fail since topic is not created
+			notificationPresent := helper.BucketClient.CheckBucketNotificationSetonRGW(namespace, storeName, reverseOBCName, reverseBucketName, reverseNotificationName, s3endpoint)


This is not waiting in a loop, so I don't see how this check is valid. This check would likely always return false, even if Rook eventually reconciled the notification successfully because the test isn't waiting on reconciling to be complete before checking that the reconcile doesn't provision the notification. In order for this check to be valid, we first have to be sure that Rook has finished reconciling the CephBucketNotification resource, then verify that the notification hasn't been provisioned in the RGW.

I need to check the false after reconciling is completed from rook side. From earlier your comments I guess the right way to it with help of status field in bucket notification CR. I will add TODO. so for the time being. Do I need to keep this check or not?

BlaineEXE · 2021-11-23T17:19:51Z

tests/framework/clients/notification.go

@@ -46,7 +45,7 @@ func (n *NotificationOperation) UpdateNotification(notificationName string, topi
 }

 // CheckNotification if notification was set
-func (t *NotificationOperation) CheckNotification(notificationName string) bool {
+func (t *NotificationOperation) CheckNotificationCR(notificationName string) bool {


This whole test isn't really useful. This merely verifies that kubernetes has received the request to create the cephbucketnotification resource and committed it to etcd. Things that are more critical to verify are (1) has Rook finished reconciling the notification, and (2) what is the status of the cephbucketnotification resource. I would suggest separate checks for each.

Yeah, I agree. It will become sense if the notification status is present in the CR. will add as todo list and modify it after it is added

this is planned to be added in phase1 (once the refactoring+UT PR is merged). see: https://gist.github.com/yuvalif/c4c7aed3dbc956a65399865a77c6a929#enhanced-cephbucketnotification-status

tests/integration/ceph_bucket_notification_test.go

BlaineEXE · 2021-11-23T17:56:32Z

tests/integration/ceph_bucket_notification_test.go

+		t.Run("delete CephBucketTopic: new-topic", func(t *testing.T) {
+			err = helper.TopicClient.DeleteTopic(newTopicName, storeName, httpEndpointService)
+			assert.Nil(t, err)
+		})


Should we validate after this that the new notification is removed from the RGW?

I am not quite sure @yuvalif do the backend notifications are removed when bucket notifications is CR removed. Or this will be removed only actual bucket is remvoed

this specific line is for topic CR deletion, not notification CR deletion

cascade deletion of notification when the notification CR is deleted is not implemented yet. it is planned to be at phase2

when a bucket is removed, all associated notifications are removed (this is done inside the RGW)

Let's add a t.Run() placeholder with a TODO comment and t.Skip() to know to come back later and verify then.

@BlaineEXE : Sorry I am not clear here, from @yuvalif comment topic CR deletion is not result in deleting backend notifications or the notification CR. The issue is even if we delete notification CR with current code the backend notification still exists. This is removed only when bucket is removed. I will add t.Run() and TODO with t.Skipped() as part of notification CR deletion tests cases. But not here. Am I missing something?

tests/integration/ceph_bucket_notification_test.go

BlaineEXE · 2021-11-23T18:04:50Z

tests/framework/utils/k8s_helper.go

@@ -56,6 +57,7 @@ type K8sHelper struct {
 	remoteExecutor   *exec.RemotePodCommandExecutor
 	Clientset        *kubernetes.Clientset
 	RookClientset    *rookclient.Clientset
+	BucketClientset  *bktclient.Clientset


K8SHelper has all very low-level clients. What is appropriate for the K8SHelper, based on current context might be a low-level OBCClientset, but the overall of the BucketClientset I think should be in TestClient because it is much more than just a clientset. It has many more helper methods. I think adding the bucket clientset here would be good, and adding the core of BucketClientset with its helper methods to TestClient is the best thing.

yuvalif · 2021-11-23T19:04:51Z

Another questions I have, possibly needing input from @yuvalif : are we testing the functional end-to-end to verify that notifications from buckets actually get sent to topics?

it is not planned to add that as part of this PR.
overall plan for bucket notifications is here: https://gist.github.com/yuvalif/c4c7aed3dbc956a65399865a77c6a929
would appreciate feedback o the priorities.

BlaineEXE · 2021-11-23T20:00:50Z

Another questions I have, possibly needing input from @yuvalif : are we testing the functional end-to-end to verify that notifications from buckets actually get sent to topics?

it is not planned to add that as part of this PR. overall plan for bucket notifications is here: https://gist.github.com/yuvalif/c4c7aed3dbc956a65399865a77c6a929 would appreciate feedback o the priorities.

TBH, I think verifying the functionality end-to-end is a pretty high priority item. The risk of not testing end-to-end here would be that the feature doesn't work for users and that the resources created don't actually enable anything. It's more acceptable in my mind if resources can't be deleted or can't be updated after they're created than if the feature doesn't work at all.

On that note, if this feature isn't ready for GA in the Rook 1.8 release, we have options. We could disable it by removing the CRDs from what we generate, but this is probably too everything-or-nothing for our desires right now. We could also update the documentation to warn that the feature is currently still under development and that users should use it with caution and please submit problems as bug reports to us.

My estimation is that we will still be actively polishing and finishing this feature for a couple months after we release Rook v1.8.0, meaning the first release where this is actually polished is somewhere in the range of v1.8.4-v1.8.8. Personally, I think we should wait to claim this feature is GA upstream until v1.9 (which doesn't necessarily reflect what we will do downstream). It might be good to have @travisn and @leseb weigh in here as well.

yuvalif · 2021-11-24T10:20:35Z

TBH, I think verifying the functionality end-to-end is a pretty high priority item

for sure this is high priority to verify that. but the question is whether this is high priority to automate in the test suite?
most of the changes we make to the code will not impact the actual sending of notifications from the RGW.
for example, this was verified in this demo

It's more acceptable in my mind if resources can't be deleted or can't be updated after they're created than if the feature doesn't work at all.

agree. these functionalities are in phase2 (post Rook1.8). for the upcoming release the 2 functional changes we plan are:

not delete all notifications on any update - this is critical to avoid service disruption on updates
add OBC list to the notification CR status - this is critical to be able to properly debug the system

we will try to add end2end testing as well in the Rook1.8 timeframe after these

leseb · 2021-11-25T14:04:54Z

tests/framework/clients/bucket.go

+	s3client, err := rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, "", true, nil)
+
+	if err != nil {
+		logger.Errorf("S3 client creation failed with error %v", err)


Let's return an error instead of logging and returning false

CheckBucketNotificationSetonRGW() is used with along with utils.Retry() and it expects the function to have only boolean, I can change the signature to include the error value as well, but it won't check properly in utils.Retry(). IMO code is more neat in this way

tests/framework/clients/bucket.go

BlaineEXE · 2021-12-01T16:02:23Z

I think that for any of these tests that are invalid, I would rather see them use t.Skipped() with a comment explaining what feature is holding the test/subtest back rather than implementing a test that isn't valid. It is very easy to see skipped tests and comments but a lot of work to analyze tests for whether they are effective or not.

For this PR, let's leave out implementations for tests where things are deleted or don't change with comments to come back later after specific features. Or whatever else might make the test ineffective.

thotz · 2021-12-02T06:27:15Z

I think that for any of these tests that are invalid, I would rather see them use t.Skipped() with a comment explaining what feature is holding the test/subtest back rather than implementing a test that isn't valid. It is very easy to see skipped tests and comments but a lot of work to analyze tests for whether they are effective or not.

For this PR, let's leave out implementations for tests where things are deleted or don't change with comments to come back later after specific features. Or whatever else might make the test ineffective.

@BlaineEXE PTAL

BlaineEXE

A few nits, and I think it will be better to not implement tests that aren't valid or for features that aren't yet implemented. I'd rather see TODO items with t.Skip() that are easy to see rather than forgetting to fix partially-implemented tests that are harder to notice.

tests/framework/clients/bucket.go

tests/framework/clients/notification.go

BlaineEXE · 2021-12-02T16:48:34Z

tests/framework/utils/k8s_helper.go

@@ -56,6 +57,7 @@ type K8sHelper struct {
 	remoteExecutor   *exec.RemotePodCommandExecutor
 	Clientset        *kubernetes.Clientset
 	RookClientset    *rookclient.Clientset
+	BucketClientset  *bktclient.Clientset


Unless this changed recently, I may have been misinterpreting something. This looks good here as BucketClientset *bktclient.Clientset with other stuff in TestClient.

tests/framework/utils/k8s_helper.go

tests/integration/ceph_bucket_notification_test.go

BlaineEXE · 2021-12-02T16:52:31Z

tests/integration/ceph_bucket_notification_test.go

+		t.Run("delete CephBucketTopic: new-topic", func(t *testing.T) {
+			err = helper.TopicClient.DeleteTopic(newTopicName, storeName, httpEndpointService)
+			assert.Nil(t, err)
+		})


Let's add a t.Run() placeholder with a TODO comment and t.Skip() to know to come back later and verify then.

Added following test cases for bucket notification integration test suite: * different order: OBC - Topic - Notification * different order: OBC - Notification - Topic * adding a label to an existing OBC * deleting a label from an existing OBC Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>

BlaineEXE · 2021-12-09T15:54:37Z

Thanks for the hard work and patience.

test: add more test cases for bucket notfication (backport #9169)

thotz requested a review from yuvalif November 15, 2021 12:56

thotz commented Nov 15, 2021

View reviewed changes

tests/integration/ceph_bucket_notification_test.go Show resolved Hide resolved

yuvalif reviewed Nov 15, 2021

View reviewed changes

tests/integration/ceph_bucket_notification_test.go Show resolved Hide resolved

yuvalif reviewed Nov 15, 2021

View reviewed changes

tests/integration/ceph_bucket_notification_test.go Show resolved Hide resolved

yuvalif reviewed Nov 15, 2021

View reviewed changes

tests/integration/ceph_bucket_notification_test.go Outdated Show resolved Hide resolved

yuvalif reviewed Nov 15, 2021

View reviewed changes

tests/framework/clients/bucket.go Outdated Show resolved Hide resolved

BlaineEXE reviewed Nov 15, 2021

View reviewed changes

tests/integration/ceph_bucket_notification_test.go Outdated Show resolved Hide resolved

thotz force-pushed the addtestcasesbucketnotification branch from 61e7c88 to 4040d21 Compare November 17, 2021 05:21

thotz requested review from yuvalif and BlaineEXE November 17, 2021 05:22

thotz commented Nov 17, 2021

View reviewed changes

thotz force-pushed the addtestcasesbucketnotification branch 3 times, most recently from 02461f4 to 618bdd4 Compare November 17, 2021 08:58

yuvalif approved these changes Nov 17, 2021

View reviewed changes

yuvalif mentioned this pull request Nov 18, 2021

ceph: bucket notifications refactoring work #9150

Merged

7 tasks

BlaineEXE requested changes Nov 18, 2021

View reviewed changes

thotz force-pushed the addtestcasesbucketnotification branch from 618bdd4 to db9a13c Compare November 19, 2021 14:19

thotz requested a review from BlaineEXE November 19, 2021 14:20

BlaineEXE requested changes Nov 23, 2021

View reviewed changes

leseb reviewed Nov 25, 2021

View reviewed changes

thotz force-pushed the addtestcasesbucketnotification branch from db9a13c to 5d21d0a Compare November 30, 2021 10:07

thotz requested review from leseb and BlaineEXE November 30, 2021 10:09

thotz force-pushed the addtestcasesbucketnotification branch from 5d21d0a to db06c5a Compare December 2, 2021 06:22

BlaineEXE requested changes Dec 2, 2021

View reviewed changes

thotz force-pushed the addtestcasesbucketnotification branch from db06c5a to 7a9a712 Compare December 2, 2021 17:33

thotz requested a review from BlaineEXE December 2, 2021 17:33

thotz added the backport-release-1.8 label Dec 9, 2021

BlaineEXE approved these changes Dec 9, 2021

View reviewed changes

BlaineEXE merged commit 2467daa into rook:master Dec 9, 2021

mergify bot mentioned this pull request Dec 9, 2021

test: add more test cases for bucket notfication (backport #9169) #9371

Merged

mergify bot added a commit that referenced this pull request Dec 9, 2021

Merge pull request #9371 from rook/mergify/bp/release-1.8/pr-9169

934a575

test: add more test cases for bucket notfication (backport #9169)

thotz deleted the addtestcasesbucketnotification branch December 9, 2021 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add more test cases for bucket notfication #9169

test: add more test cases for bucket notfication #9169

thotz commented Nov 15, 2021

thotz Nov 17, 2021

BlaineEXE Nov 18, 2021 •

edited

thotz Nov 19, 2021

BlaineEXE Nov 23, 2021

thotz Nov 30, 2021

BlaineEXE Dec 2, 2021

BlaineEXE Nov 18, 2021

thotz Nov 19, 2021

BlaineEXE left a comment

BlaineEXE Nov 23, 2021

thotz Nov 30, 2021 •

edited

BlaineEXE Nov 23, 2021

thotz Nov 30, 2021

yuvalif Nov 30, 2021

BlaineEXE Nov 23, 2021

thotz Nov 30, 2021

yuvalif Nov 30, 2021

BlaineEXE Dec 2, 2021

thotz Dec 2, 2021 •

edited

BlaineEXE Nov 23, 2021

yuvalif commented Nov 23, 2021

BlaineEXE commented Nov 23, 2021 •

edited

yuvalif commented Nov 24, 2021 •

edited

leseb Nov 25, 2021

thotz Nov 30, 2021

BlaineEXE commented Dec 1, 2021 •

edited

thotz commented Dec 2, 2021

BlaineEXE left a comment

BlaineEXE Dec 2, 2021

BlaineEXE Dec 2, 2021

BlaineEXE commented Dec 9, 2021

test: add more test cases for bucket notfication #9169

test: add more test cases for bucket notfication #9169

Conversation

thotz commented Nov 15, 2021

Choose a reason for hiding this comment

BlaineEXE Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thotz Nov 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thotz Dec 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvalif commented Nov 23, 2021

BlaineEXE commented Nov 23, 2021 • edited

yuvalif commented Nov 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE commented Dec 1, 2021 • edited

thotz commented Dec 2, 2021

BlaineEXE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE commented Dec 9, 2021

BlaineEXE Nov 18, 2021 •

edited

thotz Nov 30, 2021 •

edited

thotz Dec 2, 2021 •

edited

BlaineEXE commented Nov 23, 2021 •

edited

yuvalif commented Nov 24, 2021 •

edited

BlaineEXE commented Dec 1, 2021 •

edited