CORE-7718 metadata #23508

michael-redpanda · 2024-09-26T12:49:15Z

permit setting the directory to use to output the debug bundle
Adds a metadata object to the debug bundle service
Upon completion of an debug bundle run (regardless of success), will enqueue that into the kvstore
When service restarts, will check kvstore to see if metadata exists and if it does will 'reload' the state

Backports Required

Release Notes

None

michael-redpanda · 2024-09-26T12:55:19Z

Force push fee45fc:

Cleaned up a couple commits

vbotbuildovich · 2024-09-26T15:58:55Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55205#01922eaa-a60b-4646-9e5a-f5d9ade2c89b

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55205#01922eaa-a60e-43d8-a530-eeba765811c2

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55254#01923047-8e82-4028-a01d-98427cb5f0f0

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55254#01923047-8e7a-4b6d-bf0f-4f030444378e

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55403#01924068-0272-407f-a7b0-944f81f080f0

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55453#019243ca-9db5-4aac-87f2-931eb80e32f5

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55553#019248db-a030-4473-b2ae-d6c335d4e7cc

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55553#019248f5-a158-4dc3-9234-4b7befbe21dc

michael-redpanda · 2024-09-26T17:51:03Z

Force push 7ea5560:

Rebased off of dev

BenPope

Not finished my review, but it's moving underneath me.

src/v/debug_bundle/debug_bundle_service.cc

src/v/bytes/bytes.h

BenPope · 2024-09-26T17:16:51Z

src/v/debug_bundle/debug_bundle_service.cc

@@ -250,7 +292,7 @@ ss::future<result<void>> service::initiate_rpk_debug_bundle_collection(
          job_id,
          co_await external_process::external_process::create_external_process(
            std::move(args)),
-          form_debug_bundle_file_path(_debug_bundle_dir, job_id));
+          form_debug_bundle_file_path(output_dir, job_id));


note: I see this gets changed to debug_bundle_file_path later, but this seems like an opportune moment.

src/v/debug_bundle/tests/debug_bundle_service_test.cc

src/v/debug_bundle/BUILD

src/v/debug_bundle/metadata.cc

src/v/debug_bundle/metadata.h

src/v/debug_bundle/debug_bundle_service.cc

src/v/debug_bundle/metadata.h

src/v/debug_bundle/debug_bundle_service.cc

src/v/debug_bundle/tests/debug_bundle_service_test.cc

src/v/debug_bundle/debug_bundle_service.cc

src/v/debug_bundle/tests/debug_bundle_service_test.cc

src/v/debug_bundle/debug_bundle_service.cc

michael-redpanda · 2024-09-26T20:08:35Z

Force push 4cda960:

Fixes from PR comments
Addressed cmake build issue
Addressed bazel build issue

michael-redpanda · 2024-09-26T20:27:58Z

Force push 21db2f8:

Fixed clang format issue

BenPope

There's a commit that has a cherry-pick message with a sha that won't be useful.

dotnwat

looks good but want to resolve the kvstore question before merging.

dotnwat · 2024-09-27T19:38:03Z

src/v/serde/rw/vector.h

@@ -35,6 +35,7 @@ concept Vector = requires(T t) {
    t.begin();
    t.end();
    { t.size() } -> std::convertible_to<std::size_t>;
+    t.shrink_to_fit();


the commit message doesn't seem to indicate that this was a problem prior to the commit, so i'm wondering: is this related to the previous commit that added shrink_to_fit to the bytes type? if so, we may have an issue.

Yes it's related. I'm storing the sha256 checksum as bytes into the kvstore. I can switch to use fragmented_vector if that's an issue.

Yes it's related. I'm storing the sha256 checksum as bytes into the kvstore. I can switch to use fragmented_vector if that's an issue.

but bytes should not be being handled by the vector serde encoder. i think the issue is that the serde encoder is working with "things that look like vectors". i think can drop the shrink-to-fit and the rw/vector.h and include rw/bytes.h?

separately ill take a todo item to make this stricter, or maybe they should be combined if their on-wire formats are identical.

include rw/bytes.h?

oh my bad, yeah good call

I will say that the change in rw/vector.h I think is still good (though probably no longer relevant to this PR), since the tag_invoke(tag_t<read_tag>) function calls shrink_to_fit()

dotnwat · 2024-09-27T19:44:33Z

src/v/debug_bundle/debug_bundle_service.cc

+    ssx::spawn_with_gate(_gate, [this, job_id]() {
+        return _rpk_process->wait()
+          .then([this, job_id](auto) { return handle_wait_result(job_id); })
+          .handle_exception_type([](const std::exception& e) {


did you mean handle_exception?

dotnwat · 2024-09-27T19:46:39Z

src/v/debug_bundle/debug_bundle_service.cc

+      _rpk_process->cout().copy(),
+      _rpk_process->cerr().copy(),
+      job_id,
+      debug_bundle_file,
+      std::move(sha256_checksum),
+      _rpk_process->get_wait_result());
+
+    iobuf buf;
+    serde::write(buf, std::move(md));


kvstore isn't really designed to handle large data. so storing stdout/stderr is a bit concerning here.

how big is too big? I want to say this is a few hundred bytes in the common case, but I'm not too sure.

what if go crashed and produced a large backtrace? or whatever there is a bug in the collector and it dumped a bunch of stuff out.

how big is too big?

yeh good question--it is stored in ram for the lifetime of the key, and there is no option not to store it in ram.

could store it on disk as a file with the same key

So I was planning on running rpk debug bundle with the --verbose flag which would probably increase the amount of data in stdout/stderr, which I guess could produce a bit of information.

Why keep around stdout/stderr?

In case a previous run of rpk debug bundle failed and the broker reset. Maybe we'd like to know why it failed. Or on success, if there was anything interesting in the verbose output that may help with debugging a problem.

Why kvstore?

Originally I was going to output a json metadata file with all this information in it, but as reminded about the existence of the kvstore, so switched to doing that.

So to Oren's question, how big is too big?

So to Oren's question, how big is too big?

we don't have any formal guidelines, but the intention of the kvstore in its current state is to store small metadata.

seems you could keep the metadata in the kvstore, and the stdout/stderr on disk with the same key name?

Happy to move it out of the kvstore and onto disk. If doing so I wonder if it should be JSON or it's fine to remain in a serde format.

seems you could keep the metadata in the kvstore, and the stdout/stderr on disk with the same key name?

Yeah that's probably fine something like <uuid>.stdout and <uuid>.stderr.

If doing so I wonder if it should be JSON or it's fine to remain in a serde format.

binary is probably fine. its presumably ascii or utf8?

oleiman

lgtm, few random comments but nothing pressing

src/v/debug_bundle/debug_bundle_service.cc

src/v/debug_bundle/tests/debug_bundle_service_test.cc

src/v/debug_bundle/metadata.h

src/v/debug_bundle/debug_bundle_service.h

michael-redpanda · 2024-10-01T11:51:42Z

Force push 44857e2:

Fixed configuration description per docs suggestion

michael-redpanda · 2024-10-01T11:53:49Z

Force push 53d077b:

Rebased off of dev to fix merge conflict

michael-redpanda · 2024-10-01T12:25:44Z

Force push 1464c52:

Fixed issue during rebase

Removed friendship between service and process and added accessors and helper methods. Signed-off-by: Michael Boquard <[email protected]>

Signed-off-by: Michael Boquard <[email protected]> (cherry picked from commit c483162)

Signed-off-by: Michael Boquard <[email protected]> (cherry picked from commit 55b414b)

Signed-off-by: Michael Boquard <[email protected]>

These structures are used to store metadata and process output to the kvstore and disk in order to maintain the state of the service between application restarts. Signed-off-by: Michael Boquard <[email protected]>

Now when a new process has been kicked off, the previous run will be cleaned up. Added tests to verify new functionality. Signed-off-by: Michael Boquard <[email protected]>

Signed-off-by: Michael Boquard <[email protected]>

Generating and storing metadata into kvstore and added tests to validate it. Signed-off-by: Michael Boquard <[email protected]>

Added functionality that will reload metadata from the kvstore after the service restarts. Signed-off-by: Michael Boquard <[email protected]>

I didn't touch it so no idea how this snuck in. Signed-off-by: Michael Boquard <[email protected]>

michael-redpanda · 2024-10-01T15:05:04Z

Force push d20e80c:

Fixed conflict with storage/BUILD

oleiman

lgtm. largely taking your word for it on the changes to unit tests

oleiman · 2024-10-01T19:19:17Z

src/v/debug_bundle/debug_bundle_service.cc

+    ssx::spawn_with_gate(_gate, [this, job_id]() {
+        return _rpk_process->wait()
+          .then([this, job_id](auto) {
+              auto hold = _gate.hold();


I'm not totally sure I get why you need to hold the gate here. is it because the background task completes when you return the _process_control_mutex.get... future below?

I don't write enough continuation style code 😅

Thanks to some merge ordering schenangians, these tests started failing when redpanda-data#23557 merged after redpanda-data#23508. This change addresses the test bug by properly obtaining the configuration property and handling a situation when the debug bundle directory configuration is empty. Signed-off-by: Michael Boquard <[email protected]>

michael-redpanda requested a review from a team September 26, 2024 12:49

michael-redpanda self-assigned this Sep 26, 2024

michael-redpanda requested a review from a team as a code owner September 26, 2024 12:49

michael-redpanda requested review from oleiman and removed request for a team September 26, 2024 12:49

github-actions bot added area/build area/redpanda labels Sep 26, 2024

michael-redpanda requested a review from BenPope September 26, 2024 12:49

michael-redpanda changed the title ~~Core 7718 metadata~~ CORE-7718 metadata Sep 26, 2024

michael-redpanda force-pushed the CORE-7718-metadata branch from 61550be to fee45fc Compare September 26, 2024 12:55

michael-redpanda force-pushed the CORE-7718-metadata branch from fee45fc to 7ea5560 Compare September 26, 2024 17:50

BenPope reviewed Sep 26, 2024

View reviewed changes

BenPope self-requested a review September 26, 2024 18:02

dotnwat reviewed Sep 26, 2024

View reviewed changes

src/v/debug_bundle/debug_bundle_service.cc Outdated Show resolved Hide resolved

dotnwat reviewed Sep 26, 2024

View reviewed changes

src/v/debug_bundle/metadata.h Outdated Show resolved Hide resolved

src/v/debug_bundle/metadata.h Outdated Show resolved Hide resolved

BenPope reviewed Sep 26, 2024

View reviewed changes

michael-redpanda force-pushed the CORE-7718-metadata branch from 7ea5560 to 4cda960 Compare September 26, 2024 20:08

michael-redpanda requested review from BenPope and dotnwat September 26, 2024 20:08

michael-redpanda force-pushed the CORE-7718-metadata branch from 4cda960 to 21db2f8 Compare September 26, 2024 20:27

BenPope previously approved these changes Sep 27, 2024

View reviewed changes

dotnwat previously requested changes Sep 27, 2024

View reviewed changes

oleiman previously approved these changes Sep 27, 2024

View reviewed changes

michael-redpanda dismissed stale reviews from oleiman and BenPope via 3b0aaa6 September 29, 2024 23:31

michael-redpanda force-pushed the CORE-7718-metadata branch from 5b048ec to 44857e2 Compare October 1, 2024 11:50

michael-redpanda force-pushed the CORE-7718-metadata branch from 44857e2 to 53d077b Compare October 1, 2024 11:53

michael-redpanda requested a review from micheleRP October 1, 2024 11:54

michael-redpanda requested a review from a team as a code owner October 1, 2024 12:09

michael-redpanda requested review from jackietung-redpanda and removed request for a team October 1, 2024 12:09

michael-redpanda force-pushed the CORE-7718-metadata branch from 798fc07 to 1464c52 Compare October 1, 2024 12:24

michael-redpanda force-pushed the CORE-7718-metadata branch 2 times, most recently from d129734 to ad7106c Compare October 1, 2024 13:59

michael-redpanda added 11 commits October 1, 2024 11:02

debug_bundle: Refactored debug_bundle_process

212a7a8

Removed friendship between service and process and added accessors and helper methods. Signed-off-by: Michael Boquard <[email protected]>

config: Introduced debug_bundle_storage_dir

66ab84e

Signed-off-by: Michael Boquard <[email protected]> (cherry picked from commit c483162)

debug_bundle: Wired up debug bundle directory config

7d4c0e5

Signed-off-by: Michael Boquard <[email protected]> (cherry picked from commit 55b414b)

debug_bundle/tests: Added some helper functions

bc67bac

Signed-off-by: Michael Boquard <[email protected]>

debug_bundle: Split out types and error library

066c85a

Signed-off-by: Michael Boquard <[email protected]>

debug_bundle: Introduced serde structures

bc1a03b

These structures are used to store metadata and process output to the kvstore and disk in order to maintain the state of the service between application restarts. Signed-off-by: Michael Boquard <[email protected]>

debug_bundle: Cleanup previous run

94416d2

Now when a new process has been kicked off, the previous run will be cleaned up. Added tests to verify new functionality. Signed-off-by: Michael Boquard <[email protected]>

debug_bundle: Wire in kvstore into debug bundle service

eee77ce

Signed-off-by: Michael Boquard <[email protected]>

debug_bundle: Generating and storing metadata

31fdcc5

Generating and storing metadata into kvstore and added tests to validate it. Signed-off-by: Michael Boquard <[email protected]>

debug_bundle: Reloading metadata on restart

55385b0

Added functionality that will reload metadata from the kvstore after the service restarts. Signed-off-by: Michael Boquard <[email protected]>

dt: Fixed lint with polaris

d20e80c

I didn't touch it so no idea how this snuck in. Signed-off-by: Michael Boquard <[email protected]>

michael-redpanda force-pushed the CORE-7718-metadata branch from ad7106c to d20e80c Compare October 1, 2024 15:04

BenPope approved these changes Oct 1, 2024

View reviewed changes

oleiman approved these changes Oct 1, 2024

View reviewed changes

michael-redpanda merged commit 4327ada into redpanda-data:dev Oct 1, 2024
18 checks passed

michael-redpanda mentioned this pull request Oct 2, 2024

dt/debug-bundle: Fixed tests #23586

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-7718 metadata #23508

CORE-7718 metadata #23508

michael-redpanda commented Sep 26, 2024 •

edited

Loading

michael-redpanda commented Sep 26, 2024

vbotbuildovich commented Sep 26, 2024 •

edited

Loading

michael-redpanda commented Sep 26, 2024

BenPope left a comment

BenPope Sep 26, 2024

michael-redpanda commented Sep 26, 2024

michael-redpanda commented Sep 26, 2024

BenPope left a comment

dotnwat left a comment

dotnwat Sep 27, 2024

michael-redpanda Sep 27, 2024

dotnwat Sep 27, 2024

michael-redpanda Sep 27, 2024

michael-redpanda Sep 27, 2024

dotnwat Sep 27, 2024

michael-redpanda Sep 27, 2024

dotnwat Sep 27, 2024

oleiman Sep 27, 2024

dotnwat Sep 27, 2024

dotnwat Sep 27, 2024

michael-redpanda Sep 27, 2024

dotnwat Sep 27, 2024

michael-redpanda Sep 27, 2024

michael-redpanda Sep 27, 2024

dotnwat Sep 27, 2024

oleiman left a comment

michael-redpanda commented Oct 1, 2024

michael-redpanda commented Oct 1, 2024

michael-redpanda commented Oct 1, 2024

michael-redpanda commented Oct 1, 2024

oleiman left a comment

oleiman Oct 1, 2024

CORE-7718 metadata #23508

CORE-7718 metadata #23508

Conversation

michael-redpanda commented Sep 26, 2024 • edited Loading

Backports Required

Release Notes

michael-redpanda commented Sep 26, 2024

vbotbuildovich commented Sep 26, 2024 • edited Loading

michael-redpanda commented Sep 26, 2024

BenPope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Sep 26, 2024

michael-redpanda commented Sep 26, 2024

BenPope left a 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

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

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

oleiman left a comment

Choose a reason for hiding this comment

michael-redpanda commented Oct 1, 2024

michael-redpanda commented Oct 1, 2024

michael-redpanda commented Oct 1, 2024

michael-redpanda commented Oct 1, 2024

oleiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Sep 26, 2024 •

edited

Loading

vbotbuildovich commented Sep 26, 2024 •

edited

Loading