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.

Sign up for GitHub

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

Jump to bottom

admin: stream more json results #16551

Merged
merged 3 commits into from
Feb 26, 2024
Merged

admin: stream more json results #16551

merged 3 commits into from
Feb 26, 2024

Conversation

rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Feb 8, 2024 •
edited

This patch set is the result of auditing the admin server looking for places that can cause oversized allocations.

Fixes: https://github.com/redpanda-data/core-internal/issues/1063

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

Bug Fixes

  • Fixed a few oversized allocations for some admin server endpoints.

@rockwotj
Copy link
Contributor Author

rockwotj commented Feb 8, 2024

Related is #16512 which changes the leader info to not be a std::vector which should help here as well, but is still technically prone to reactor stalls until we switch to a for_each interface or something.

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Feb 8, 2024 •
edited

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44909#018d8a68-fbbb-4ed3-8059-7bfeb407083f

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/44945#018d8eca-3c28-4245-9a10-de70c63d0c3f

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45305#018dd74f-5f07-4228-b2b1-f5bcb3df0885

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45305#018dd74f-5f04-4e0a-a6c0-3a9d34eb2880

@vbotbuildovich
Copy link
Collaborator

new failures in https://buildkite.com/redpanda/redpanda/builds/44909#018d8a68-fbc5-479e-ace6-656afd790e74:

"rptest.tests.cpu_profiler_admin_api_test.CPUProfilerAdminAPITest.test_get_cpu_profile_with_override"

@rockwotj rockwotj changed the title admin/debug/get_leaders_info: stream json results admin: stream more json results Feb 8, 2024
michael-redpanda
michael-redpanda previously approved these changes Feb 9, 2024
View reviewed changes
Copy link
Contributor

@michael-redpanda michael-redpanda left a comment

Choose a reason for hiding this comment

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

lgtm, only nits so nothing blocking merge imo

src/v/redpanda/admin/debug.cc
Comment on lines +235 to +247
// TODO(rockwood): get_leaders can lead to a reactor stall, fix to use
// an async version
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Did you file a core-internal ticket for this? I can if you like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Michal already has a PR that I can use to fix this, just need to wait for #16512 (and backport it)

src/v/redpanda/admin/partition.cc Outdated
#include "redpanda/admin/api-doc/debug.json.hh"
#include "redpanda/admin/api-doc/partition.json.hh"
#include "redpanda/admin/server.h"
#include "redpanda/admin/util.h"

#include <seastar/coroutine/maybe_yield.hh>
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Was this include intentionally added? I ask because I don't see a yield in this commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I had tried to add it, but realized it would not be valid. I will remove the include

@rockwotj
Copy link
Contributor Author

rockwotj commented Feb 9, 2024

Force push: remove unused include

BenPope
BenPope previously approved these changes Feb 10, 2024
View reviewed changes
src/v/redpanda/admin/debug.cc
: -1;
info.previous_leader = leader_info.previous_leader.has_value()
? leader_info.previous_leader.value()
: -1;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: It's not really part of the refactor, which is fine, but it would be nice to use .value_or(-1) in these two places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix in a followup, I am going to merge as the perf team was asking about the oversized allocs

michael-redpanda and others added 3 commits February 23, 2024 12:12
@michael-redpanda
admin/debug/get_leaders_info: stream json results
34d9ec9 
The current approach can lead to an oversized allocation. This is not a
full solution as get_leaders needs to not be prone to reactor stalls and
oversized allocs.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj @michael-redpanda
admin/partition: prevent oversized allocation
2818c02 
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj @michael-redpanda
admin/transaction: prevent oversized allocs
3b0ef74 
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@michael-redpanda michael-redpanda dismissed stale reviews from BenPope and themself via 3b0ef74 February 23, 2024 17:14
@michael-redpanda
Copy link
Contributor

Force push 3b0ef74:

  • Rebase off of dev to fix conflict

@michael-redpanda michael-redpanda merged commit e0466a4 into redpanda-data:dev Feb 26, 2024
16 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.2.x

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-16551-v23.2.x-5 remotes/upstream/v23.2.x
git cherry-pick -x 34d9ec9262ebadf7da887fcf37fce40afbce467c 2818c0265670367186f2919ed8698492f495a290 3b0ef74009e59aaf7e6e139ee59390b3b06bae7b

Workflow run logs.

@vbotbuildovich vbotbuildovich mentioned this pull request Feb 26, 2024
Closed
@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-16551-v23.3.x-61 remotes/upstream/v23.3.x
git cherry-pick -x 34d9ec9262ebadf7da887fcf37fce40afbce467c 2818c0265670367186f2919ed8698492f495a290 3b0ef74009e59aaf7e6e139ee59390b3b06bae7b

Workflow run logs.

@vbotbuildovich vbotbuildovich mentioned this pull request Feb 26, 2024
Closed
@michael-redpanda michael-redpanda mentioned this pull request Feb 27, 2024
Merged
@michael-redpanda
Copy link
Contributor

@piyushredpanda there were some significant refactoring of the admin API between v23.2 and v23.3 which makes resolving the backport issues a bit of a lift. Happy to do it if we want to prioritize it.

cc @aanthony-rp

@michael-redpanda michael-redpanda mentioned this pull request Feb 27, 2024
Merged
dotnwat
dotnwat reviewed Feb 28, 2024
View reviewed changes
src/v/redpanda/admin/debug.cc

return ss::make_ready_future<ss::json::json_return_type>(ans);
return ss::make_ready_future<ss::json::json_return_type>(
ss::json::stream_range_as_array(
Copy link
Member

Choose a reason for hiding this comment

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

woah 🔥 didn't know this existed

@rockwotj rockwotj deleted the admin branch March 28, 2024 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotnwat dotnwat dotnwat left review comments

@michael-redpanda michael-redpanda michael-redpanda approved these changes

@graphcareful graphcareful Awaiting requested review from graphcareful

@BenPope BenPope Awaiting requested review from BenPope

Assignees

@rockwotj rockwotj

Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rockwotj @vbotbuildovich @michael-redpanda @dotnwat @BenPope