[CORE-1198] cluster: cloud storage self test #17586

WillemKauf · 2024-04-03T16:24:12Z

Adds a cloud storage check to the cluster self test, as requested in #9225.
Depending on read/write permissions, the cloud storage test uses the cloud_storage::remote object configured at the application layer to:

Upload an object to S3.
List objects in a bucket.
Download an object from the bucket.
Delete the original object, if it was uploaded.

Backports Required

Release Notes

Features

A cloud storage check as part of the cluster's self test.

To run, use rpk cluster self-test start. The following flags have also been added to rpk for use with the cloud storage test:

--cloud-backoff-ms uint, the backoff in milliseconds for a cloud storage request.
--cloud-timeout-ms uint, the timeout in milliseconds for a cloud storage request.
--only-cloud-test, in order to run only the cloud storage test.

dotnwat

still need to read through it more detail, but its looking good. it will need to have tests added.

src/go/rpk/pkg/adminapi/api_debug.go

src/v/cluster/self_test/metrics.h

dotnwat · 2024-04-03T22:38:15Z

src/v/cluster/self_test/cloudcheck.cc

+  const cloud_storage_clients::bucket_name& bucket,
+  const cloud_storage_clients::object_key& key) {


general best practice unless performance sensitive is to have coroutines take parameters by value.

dotnwat · 2024-04-03T22:39:53Z

src/v/cluster/self_test_rpc_types.h

+    // Amount of fibers to run per shard
+    uint16_t parallelism{10};


this seems to be unused, did i miss it? separately, is it is desired for the cloud self-test to perform a benchmark vs only verify that connectivity and basic operations are working? i'm not sure what the right answer is.

Yes, currently this is unused- I also had the open question of whether more rigorous benchmarking across multiple shards was desired, or if just verifying connectivity with cloud storage was ideal.

andrwng

Great work! Looking pretty solid

src/v/cluster/topic_table.h

src/v/cluster/self_test/cloudcheck.cc

src/go/rpk/pkg/adminapi/api_debug.go

src/v/cluster/self_test_rpc_types.h

dotnwat · 2024-04-06T20:10:58Z

src/v/redpanda/admin/server.cc

@@ -2740,17 +2740,20 @@ admin_server::self_test_start_handler(std::unique_ptr<ss::http::request> req) {
                    r.dtos.push_back(cluster::diskcheck_opts::from_json(obj));
                } else if (test_type == "network") {
                    r.ntos.push_back(cluster::netcheck_opts::from_json(obj));
+                } else if (test_type == "cloud") {


i'd recommend splitting out the changes to the admin server into a commit the comes right before the rpk changes that use those new http interfaces. makes it much easier to review.

src/v/cluster/self_test_backend.cc

src/v/cluster/self_test_frontend.cc

dotnwat · 2024-04-06T20:17:35Z

src/v/cluster/self_test_rpc_types.cc

+    case self_test_stage::cloud:
+        return "cloud";
+    default:
+        __builtin_unreachable();


you can probably drop builtin_unreachable if the cases are a covering set for the enum. if they aren't a covering set and you want to fail on default, then use vassert. if they are a covering set but you get a compiler warning, move builtin_unreachable outside the switch statement.

WillemKauf · 2024-04-10T17:39:58Z

still need to read through it more detail, but its looking good. it will need to have tests added.

The cloud storage self test will be ran in the existing self_test_test.py ducktape test. I have added a commit that modifies it slightly in order to assert the correct number of self test reports.

Let me know what you think!

vbotbuildovich · 2024-04-16T18:10:14Z

new failures in https://buildkite.com/redpanda/redpanda/builds/47869#018ee7eb-f4d9-40b0-a136-68e114f85b8e:

"rptest.tests.self_test_test.SelfTestTest.test_self_test"

new failures in https://buildkite.com/redpanda/redpanda/builds/47869#018ee7eb-f4d4-4c5e-a17a-ecc57728cc49:

"rptest.tests.self_test_test.SelfTestTest.test_self_test_node_crash"

new failures in https://buildkite.com/redpanda/redpanda/builds/47869#018ee7eb-f4dc-4598-99e9-0d0a7e4bbec5:

"rptest.tests.self_test_test.SelfTestTest.test_self_test_cancellable"

new failures in https://buildkite.com/redpanda/redpanda/builds/47885#018ee86a-e740-4d78-9d33-f46f75ac8b0f:

"rptest.tests.self_test_test.SelfTestTest.test_self_test"

new failures in https://buildkite.com/redpanda/redpanda/builds/47885#018ee86a-e73a-429f-9e6c-08f703304971:

"rptest.tests.self_test_test.SelfTestTest.test_self_test_node_crash"

new failures in https://buildkite.com/redpanda/redpanda/builds/47895#018ee8e3-a7b3-4196-b91b-35895ff4e5e5:

"rptest.tests.self_test_test.SelfTestTest.test_self_test"

new failures in https://buildkite.com/redpanda/redpanda/builds/47895#018ee8e3-a7ae-40be-9fc1-6f5a5dba3224:

"rptest.tests.self_test_test.SelfTestTest.test_self_test_node_crash"

new failures in https://buildkite.com/redpanda/redpanda/builds/47895#018ee8eb-3320-407d-8c7a-de7d02439a7d:

"rptest.tests.self_test_test.SelfTestTest.test_self_test"

new failures in https://buildkite.com/redpanda/redpanda/builds/47895#018ee8eb-331b-4625-887a-0e177fa527eb:

"rptest.tests.self_test_test.SelfTestTest.test_self_test_node_crash"

andrwng · 2024-04-29T21:58:39Z

tests/rptest/tests/self_test_test.py

+            assert report['error'] == error_msg
+


nit: can we also add an assertion that cloud_storage is among the reports we've received?

Added assert statement to confirm number of received cloud storage reports.

andrwng · 2024-04-29T22:03:34Z

tests/rptest/tests/self_test_test.py

+                            assert_fail(
+                                report,
+                                'Remote read is not enabled for this cluster.')
+                    elif report['info'] in ['upload', 'delete']:


nit: maybe make this an else and then assert report['info'] in ['upload', 'delete']? Just so any future cloud_storage report types that get added necessarily have to be considered here

andrwng · 2024-04-29T22:09:10Z

tests/rptest/tests/self_test_test.py

+        remote_read = False
+        remote_write = False
+        if hasattr(ctx, 'injected_args') \
+        and ctx.injected_args is not None:
+            if 'cloud_storage_enable_remote_read' in ctx.injected_args:
+                remote_read = ctx.injected_args[
+                    'cloud_storage_enable_remote_read']
+            if 'cloud_storage_enable_remote_write' in ctx.injected_args:
+                remote_write = ctx.injected_args[
+                    'cloud_storage_enable_remote_write']
+
+        super(SelfTestTest, self).__init__(
+            test_context=ctx,
+            si_settings=SISettings(
+                test_context=ctx,
+                cloud_storage_enable_remote_read=remote_read,
+                cloud_storage_enable_remote_write=remote_write))


Just FYI, there are other test base classes that allow you to start Redpanda in the test body. EndToEndTest is one example.

Feel free to keep this as is, but in case you want to avoid depending on low level things like injected_args, you could switch SelfTestTest over to using it and calling start_redpanda() in the test body

Thanks for letting me know about this! It's much nicer than dealing with injected_args.

Fixed.

dotnwat

yeh this looks awesome. i think if we squash the fixes back into previous commits this is looking like its pretty much ready.

src/v/cluster/self_test/cloudcheck.cc

dotnwat · 2024-04-30T04:41:52Z

src/go/rpk/pkg/cli/cluster/selftest/start.go

+* Cloud tests:
+  * Latency test: 1024-bit object.
+    * Depending on read/write permissions, a series of cloud storage operations are performed.
+


@r-vasquez IIUC the way this will work is that if the cluster is already set up with TS then these tests will use those configurations.

src/v/cluster/self_test_backend.h

src/v/cluster/self_test_rpc_types.cc

src/v/cluster/self_test/cloudcheck.cc

tests/rptest/tests/self_test_test.py

The cloud storage check performs the following operations: - Upload an object to the configured S3 bucket - List from the bucket - Download from the bucket - Delete from the bucket The cluster self-test contains disk and network tests to validate and benchmark those subsystems. A test for cloud storage helps to ensure credentials and permissions have been correctly configured in redpanda.

Adds the `self_test::cloudcheck` object to `cluster::self_test_backend` so that it can be invoked alongside the existing self-test routines.

To allow for customization of cloud storage self-test options, `admin_server::self_test_start_handler()` will now be able to read options set in the JSON request created on the `rpk` side.

Adds cloud storage self-test bindings to `rpk`, allowing users to invoke the self-test. Also adds the following flags for use with the cloud storage test: - `--cloud-timeout-ms`, the timeout in ms for a cloud storage request - `--cloud-backoff-ms`, the backoff in ms for a cloud storage request - `--only-cloud-test`, in order to run only the cloud storage test

Add 'self_test_stage' as an indicator to user of which self-test routine is currently running. This commit appends the `self_test_stage` to the `self_test_backend` and `self_test_frontend`. Currently, a user can request the status of self-test in 'rpk' using 'cluster self-test status'. However, this only indicates whether a test is running on a node or if the tests are finished, but not which test is being ran. After adding bindings on the `rpk` side, this will enable users to see which self-test is currently running.

`admin_server` will now set the self test stage in its status reports presented to user.

`rpk` will now generate a report of running nodes with node ID, as well as the current stage of the self test on those running nodes. This results in more detailed status updates presented to the user when `cluster self-test status` is ran in `rpk`. User will now see which test is currently being run on which node, if a test is running.

Adds cloud storage self test result parsing to `SelfTestTest`. Also adds configuration for the cloud storage self-test to `clients/rpk.py`.

Feediver1 · 2024-04-30T16:15:20Z

src/go/rpk/pkg/cli/cluster/selftest/start.go

@@ -48,6 +51,15 @@ of the cluster. Available tests to run:
    * Unique pairs of Redpanda nodes each act as a client and a server.
    * The test pushes as much data over the wire, within the test parameters.

+* Cloud tests:
+  * Latency test: 1024-bit object.
+    * Depending on cluster read/write permissions (cloud_storage_enable_remote_read, cloud_storage_enable_remote_write), a series of cloud storage operations are performed.


Suggested change

* Depending on cluster read/write permissions (cloud_storage_enable_remote_read, cloud_storage_enable_remote_write), a series of cloud storage operations are performed.

* Depending on cluster read/write permissions (cloud_storage_enable_remote_read, cloud_storage_enable_remote_write), a series of cloud storage operations are performed:

Feediver1

Left minor punctuation update.

Extended the self test ducktape test coverage for all combinations of `cloud_storage_enable_remote_read` and `cloud_storage_enable_remote_write` permissions.

dotnwat

everything looks good. just wondering about the necessity of using the e2e test fixture in ducktape.

tests/rptest/tests/self_test_test.py

github-actions bot added area/rpk area/redpanda labels Apr 3, 2024

WillemKauf changed the title ~~Cluster cloud storage validation testing~~ Cluster cloud storage self test Apr 3, 2024

WillemKauf requested a review from andrwng April 3, 2024 16:24

WillemKauf force-pushed the cluster_cloud_storage_validation_testing branch 3 times, most recently from 6adb3db to f867c05 Compare April 3, 2024 18:11

WillemKauf changed the title ~~Cluster cloud storage self test~~ cluster: cloud storage self test Apr 3, 2024

dotnwat requested review from dotnwat and graphcareful April 3, 2024 22:27

dotnwat reviewed Apr 3, 2024

View reviewed changes

WillemKauf force-pushed the cluster_cloud_storage_validation_testing branch 2 times, most recently from 45be6ca to 2ebaec8 Compare April 4, 2024 15:23

andrwng reviewed Apr 5, 2024

View reviewed changes

WillemKauf force-pushed the cluster_cloud_storage_validation_testing branch 3 times, most recently from 0015d8e to bb13248 Compare April 5, 2024 14:47

dotnwat changed the title ~~cluster: cloud storage self test~~ CORE-1198: cluster: cloud storage self test Apr 5, 2024

dotnwat reviewed Apr 6, 2024

View reviewed changes

WillemKauf force-pushed the cluster_cloud_storage_validation_testing branch 3 times, most recently from 4459db0 to c399af3 Compare April 10, 2024 17:38

WillemKauf mentioned this pull request Apr 11, 2024

CORE-2157: cloud_storage_clients: add support for path-style addressing #17806

Merged

6 tasks

WillemKauf marked this pull request as ready for review April 16, 2024 15:45

WillemKauf requested review from twmb, r-vasquez and gene-redpanda as code owners April 16, 2024 15:45

WillemKauf force-pushed the cluster_cloud_storage_validation_testing branch from c399af3 to 79983dc Compare April 16, 2024 18:32

andrwng previously approved these changes Apr 29, 2024

View reviewed changes

WillemKauf dismissed andrwng’s stale review via 38631cc April 29, 2024 23:17

WillemKauf force-pushed the cluster_cloud_storage_validation_testing branch from 006c70b to 38631cc Compare April 29, 2024 23:17

WillemKauf requested a review from Deflaimun as a code owner April 29, 2024 23:17

WillemKauf force-pushed the cluster_cloud_storage_validation_testing branch from 38631cc to 4d1376e Compare April 29, 2024 23:34

dotnwat reviewed Apr 30, 2024

View reviewed changes

WillemKauf added 10 commits April 30, 2024 08:49

cloud_storage: add max_keys parameter to remote::list_objects

a062640

cluster: add cloud storage test to self-test frontend/backend

7fb22c8

Adds the `self_test::cloudcheck` object to `cluster::self_test_backend` so that it can be invoked alongside the existing self-test routines.

redpanda/admin: add cluster::cloudcheck_opts to server handler

1170951

To allow for customization of cloud storage self-test options, `admin_server::self_test_start_handler()` will now be able to read options set in the JSON request created on the `rpk` side.

redpanda/admin: add self_test_stage to results handler

732915e

`admin_server` will now set the self test stage in its status reports presented to user.

tests: add cloud storage test results to self_test_test

294e210

Adds cloud storage self test result parsing to `SelfTestTest`. Also adds configuration for the cloud storage self-test to `clients/rpk.py`.

cluster: prefer static_cast<> to C-style cast in self-test metrics

697707b

WillemKauf force-pushed the cluster_cloud_storage_validation_testing branch from 4d1376e to 3ab7798 Compare April 30, 2024 12:52

WillemKauf requested a review from dotnwat April 30, 2024 12:58

dotnwat requested a review from andrwng April 30, 2024 16:04

Feediver1 reviewed Apr 30, 2024

View reviewed changes

Feediver1 previously approved these changes Apr 30, 2024

View reviewed changes

WillemKauf added 2 commits April 30, 2024 12:18

rpk: more verbose self-test documentation

eded0fc

rptest: parameterized self-test test

62dbcd7

Extended the self test ducktape test coverage for all combinations of `cloud_storage_enable_remote_read` and `cloud_storage_enable_remote_write` permissions.

WillemKauf dismissed Feediver1’s stale review via 62dbcd7 April 30, 2024 16:19

WillemKauf force-pushed the cluster_cloud_storage_validation_testing branch from 3ab7798 to 62dbcd7 Compare April 30, 2024 16:19

dotnwat reviewed Apr 30, 2024

View reviewed changes

tests/rptest/tests/self_test_test.py Show resolved Hide resolved

dotnwat approved these changes Apr 30, 2024

View reviewed changes

andrwng approved these changes Apr 30, 2024

View reviewed changes

tests/rptest/tests/self_test_test.py Show resolved Hide resolved

dotnwat merged commit ab18cc6 into redpanda-data:dev Apr 30, 2024
25 checks passed

WillemKauf deleted the cluster_cloud_storage_validation_testing branch May 2, 2024 01:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-1198] cluster: cloud storage self test #17586

[CORE-1198] cluster: cloud storage self test #17586

WillemKauf commented Apr 3, 2024 •

edited

Loading

dotnwat left a comment

dotnwat Apr 3, 2024

dotnwat Apr 3, 2024

WillemKauf Apr 4, 2024 •

edited

Loading

andrwng left a comment

dotnwat Apr 6, 2024

dotnwat Apr 6, 2024

WillemKauf commented Apr 10, 2024

vbotbuildovich commented Apr 16, 2024 •

edited

Loading

andrwng Apr 29, 2024

WillemKauf Apr 29, 2024

andrwng Apr 29, 2024

WillemKauf Apr 29, 2024

andrwng Apr 29, 2024

WillemKauf Apr 29, 2024

dotnwat left a comment

dotnwat Apr 30, 2024

Feediver1 Apr 30, 2024

Feediver1 left a comment

dotnwat left a comment

		const cloud_storage_clients::bucket_name& bucket,
		const cloud_storage_clients::object_key& key) {

		// Amount of fibers to run per shard
		uint16_t parallelism{10};

	* Depending on cluster read/write permissions (cloud_storage_enable_remote_read, cloud_storage_enable_remote_write), a series of cloud storage operations are performed.
	* Depending on cluster read/write permissions (cloud_storage_enable_remote_read, cloud_storage_enable_remote_write), a series of cloud storage operations are performed:

[CORE-1198] cluster: cloud storage self test #17586

[CORE-1198] cluster: cloud storage self test #17586

Conversation

WillemKauf commented Apr 3, 2024 • edited Loading

Backports Required

Release Notes

Features

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillemKauf Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

andrwng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillemKauf commented Apr 10, 2024

vbotbuildovich commented Apr 16, 2024 • edited Loading

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

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feediver1 left a comment

Choose a reason for hiding this comment

dotnwat left a comment

Choose a reason for hiding this comment

WillemKauf commented Apr 3, 2024 •

edited

Loading

WillemKauf Apr 4, 2024 •

edited

Loading

vbotbuildovich commented Apr 16, 2024 •

edited

Loading