tests/rgw/notifications: Add tests for RGWPSListTopicsOp::execute() #60788

oshrey16 · 2024-11-21T10:00:01Z

Test Additions:
Added new test cases for RGWPSListTopicsOp::execute() to cover the following scenarios:

Migration case: Verifying support_all_zones with v1 topics to ensure proper handling during migration.
v2 notification case: Ensuring correct behavior when v2 notifications are supported.
v1 notification case: Verifying fallback behavior when v2 notifications are unavailable.

Version Handling for delete_all_topics:
Updated the delete_all_topics function to include a version parameter, allowing for better control over topic deletions in v1 and v2 systems. The logic now correctly handles v1 responses, which include a result key in topics_json.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

yuvalif · 2024-11-25T17:12:08Z

src/test/rgw/bucket_notification/test_bn.py

@@ -4359,6 +4360,243 @@ def test_ps_s3_multiple_topics_notification():
    http_server.close()


+@attr('basic_test')


Suggested change

@attr('basic_test')

@attr('data_path_v2_test')

yuvalif · 2024-11-25T17:16:47Z

src/test/rgw/bucket_notification/test_bn.py

+
+
+@attr('basic_test')
+def test_ps_s3_multiple_topics_v2():


Suggested change

def test_ps_s3_multiple_topics_v2():

def test_ps_s3_list_topics():

this is not version specific. juts testing topic listing via REST and not via admin command (the main test that was missing)

yuvalif · 2024-11-25T17:17:02Z

src/test/rgw/bucket_notification/test_bn.py

+        tenant_topic_conf.del_config(tenant_topic_arn2)
+
+@attr('basic_test')
+def test_ps_s3_multiple_topics_v1():


Suggested change

def test_ps_s3_multiple_topics_v1():

def test_ps_s3_list_topics_v1():

oshrey16 · 2024-11-30T09:35:48Z

src/test/rgw/bucket_notification/test_bn.py

+        tenant_topic_conf.del_config(tenant_topic_arn1)
+        tenant_topic_conf.del_config(tenant_topic_arn2)
+
+@attr('data_path_v1_test')


I'm uncertain whether this test should be tagged as basic_test. Is it appropriate to add @attr('data_path_v1_test') here? It seems that there aren't currently any tag for v1 tests, while attributes like data_path_v2_test are present.

the main reason for the v2 tag is so that the automatic test system would run this test in an environment where there is a realm configured.
this is why this test should eb marked as "v2" even though it tests the v1 case

oshrey16 · 2024-12-07T10:54:45Z

jenkins test make check arm64

yuvalif · 2024-12-08T19:40:06Z

src/test/rgw/bucket_notification/test_bn.py

+        ListTopicsResult = ListTopicsResponse.get('ListTopicsResult', {})
+        Topics = ListTopicsResult.get('Topics', {})
+        member = Topics['member'] if Topics else []
+        assert_equal(len(member), 4)


member is holding 6 topics, both the v2 and the v1 topics

yuvalif · 2024-12-08T19:40:23Z

src/test/rgw/bucket_notification/test_bn.py

+        ListTopicsResult = ListTopicsResponse.get('ListTopicsResult', {})
+        Topics = ListTopicsResult.get('Topics', {})
+        member = Topics['member'] if Topics else []
+        assert_equal(len(member), 2)


member is holding 3 tenant topics, both the v2 and the v1 topics

yuvalif · 2024-12-08T19:48:07Z

src/test/rgw/bucket_notification/api.py

    """ delete all topics """
    if tenant == '':
        topics_result = admin(['topic', 'list'], cluster)
        topics_json = json.loads(topics_result[0])
+        if(version == 'v1'):


this may break, depending with the order opf execution of the test.
what if you try to delete v1 topics that were created intentionally in v1, whiule the conf says v2?

would recommend a more robust way, do a try-catch on Key error, and select the option based on that

yuvalif · 2024-12-16T12:43:51Z

src/test/rgw/bucket_notification/api.py

@@ -247,13 +247,15 @@ def delete_all_topics(conn, tenant, cluster):
    if tenant == '':
        topics_result = admin(['topic', 'list'], cluster)
        topics_json = json.loads(topics_result[0])
-        for topic in topics_json['topics']:
+        topics = topics_json.get('topics', [])


but in this case we do not clear v1 topics.
maybe better to check if topics is empty, and try to: topics_json.get('result, []) ?
if the value here is not empty, get the topics.

in this case, it might be simpler to use the KeyError exception

yuvalif · 2024-12-18T11:41:53Z

src/test/rgw/bucket_notification/api.py

+            for topic in topics_json['topics']:
+                rm_result = admin(['topic', 'rm', '--topic', topic['name']], cluster)
+                print(rm_result)
+        except KeyError as e:


please do not catch the exception here.
if the results dont have ['topics'] and dont have ['result']['topics'] it is better to let the KeyError not be caught and fail the test

Thank you for the feedback! I’ve removed the try-except block. If the keys topics or result are missing, the KeyError will now surface naturally, the test will fail appropriately.

yuvalif · 2024-12-18T11:42:03Z

src/test/rgw/bucket_notification/api.py

+            for topic in topics_json['topics']:
+                rm_result = admin(['topic', 'rm', '--tenant', tenant, '--topic', topic['name']], cluster)
+                print(rm_result)
+        except KeyError as e:


same comment

Tests: Add comprehensive test cases to verify the behavior of `RGWPSListTopicsOp::execute()` under various scenarios: Migration case: Validate correct handling when `support_all_zones` is enabled, with v1 in a new state after migration and v2 topics present. v2 notification case: Ensure proper retrieval when v2 notifications are supported. v1 notification case: Verify fallback behavior when v2 notifications are unavailable. Enhancements: Update `delete_all_topics` to handle v1 responses with the `result` key. fixes: https://tracker.ceph.com/issues/68756 Signed-off-by: Oshrey Avraham <[email protected]>

yuvalif · 2024-12-18T16:31:55Z

teuthology passing: https://pulpito.ceph.com/yuvalif-2024-12-18_15:19:36-rgw:notifications-wip-yuval-kafka_multiple_brokers-distro-default-smithi/

yuvalif · 2024-12-18T17:54:47Z

jenkins test api

oshrey16 requested a review from a team as a code owner November 21, 2024 10:00

github-actions bot added rgw tests labels Nov 21, 2024

yuvalif reviewed Nov 25, 2024

View reviewed changes

oshrey16 force-pushed the tests-rgw-ps-list-topics-op branch from 8800baa to cadd9ef Compare November 29, 2024 21:53

oshrey16 commented Nov 30, 2024

View reviewed changes

oshrey16 force-pushed the tests-rgw-ps-list-topics-op branch from cadd9ef to 6d25188 Compare December 6, 2024 15:05

yuvalif reviewed Dec 8, 2024

View reviewed changes

oshrey16 force-pushed the tests-rgw-ps-list-topics-op branch from 6d25188 to 0dad861 Compare December 16, 2024 10:28

yuvalif reviewed Dec 16, 2024

View reviewed changes

oshrey16 force-pushed the tests-rgw-ps-list-topics-op branch from 0dad861 to 9171f06 Compare December 18, 2024 10:15

yuvalif reviewed Dec 18, 2024

View reviewed changes

oshrey16 force-pushed the tests-rgw-ps-list-topics-op branch from 9171f06 to 65614c4 Compare December 18, 2024 14:23

yuvalif self-requested a review December 18, 2024 14:36

yuvalif approved these changes Dec 18, 2024

View reviewed changes

yuvalif merged commit 9349823 into ceph:main Dec 19, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests/rgw/notifications: Add tests for RGWPSListTopicsOp::execute() #60788

tests/rgw/notifications: Add tests for RGWPSListTopicsOp::execute() #60788

oshrey16 commented Nov 21, 2024

yuvalif Nov 25, 2024

yuvalif Nov 25, 2024

yuvalif Nov 25, 2024

oshrey16 Nov 30, 2024 •

edited

Loading

yuvalif Dec 3, 2024

oshrey16 commented Dec 7, 2024

yuvalif Dec 8, 2024

yuvalif Dec 8, 2024

yuvalif Dec 8, 2024

yuvalif Dec 16, 2024

yuvalif Dec 18, 2024

oshrey16 Dec 18, 2024

yuvalif Dec 18, 2024

yuvalif commented Dec 18, 2024

yuvalif commented Dec 18, 2024

		@@ -4359,6 +4360,243 @@ def test_ps_s3_multiple_topics_notification():
		http_server.close()


		@attr('basic_test')

	def test_ps_s3_multiple_topics_v2():
	def test_ps_s3_list_topics():

	def test_ps_s3_multiple_topics_v1():
	def test_ps_s3_list_topics_v1():

tests/rgw/notifications: Add tests for RGWPSListTopicsOp::execute() #60788

tests/rgw/notifications: Add tests for RGWPSListTopicsOp::execute() #60788

Conversation

oshrey16 commented Nov 21, 2024

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshrey16 Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshrey16 commented Dec 7, 2024

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

yuvalif commented Dec 18, 2024

yuvalif commented Dec 18, 2024

oshrey16 Nov 30, 2024 •

edited

Loading