[CORE-16225] 3/3: Cloud I/O Scheduler: Cluster config and metrics#30596
Conversation
ce7b2b7 to
dcd4e1c
Compare
|
/ci-repeat 1 |
Retry command for Build#84915please wait until all jobs are finished before running the slash command |
dcd4e1c to
32dbce5
Compare
|
/ci-repeat 1 |
dc3a181 to
0e790ed
Compare
|
/ci-repeat 1 |
0e790ed to
8ec10ac
Compare
|
/ci-repeat 1 |
Retry command for Build#84936please wait until all jobs are finished before running the slash command |
8ec10ac to
cb416c2
Compare
|
/ci-repeat 1 |
|
/ci-repeat 1 |
There was a problem hiding this comment.
Pull request overview
This PR completes the Cloud I/O Scheduler rollout by adding cluster-level configuration and wiring the scheduler into the cloud client pool, along with metrics and unit tests. It introduces a cloud_io::scheduler admission gate (policy-driven, currently reservation-based) and tags cloud I/O operations with a cloud_io::group_id to enforce per-group concurrency behavior.
Changes:
- Add cluster properties for selecting the scheduler policy and configuring per-group reservation targets; plumb this config into
cloud_storage::configurationand down intocloud_storage_clients::client_pool. - Gate client-pool leases through the per-shard scheduler, and propagate
group_idthrough cloud I/O APIs/callers to classify operations (producer uploads vs consumer fetch vs default). - Add scheduler/reservation-policy unit tests and a design note doc; update fixtures and mocks for the new API surfaces.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rptest/services/redpanda.py | Adjusts test cluster config generation to avoid reservation defaults conflicting with small connection pools. |
| src/v/redpanda/tests/fixture.cc | Forces empty reservation config in fixtures to keep policy active without reserved lanes under small capacities. |
| src/v/redpanda/application_services.cc | Passes scheduler configuration into cloud storage client pool construction. |
| src/v/config/rjson_serialization.h | Adds JSON serialization support for cloud_io::policy_type. |
| src/v/config/rjson_serialization.cc | Implements JSON serialization for cloud_io::policy_type. |
| src/v/config/property.h | Registers cloud_io::policy_type as a string-typed config property. |
| src/v/config/convert.h | Adds YAML conversion for cloud_io::policy_type. |
| src/v/config/configuration.h | Declares new cluster properties for scheduler policy and reservation specs. |
| src/v/config/configuration.cc | Defines the new cluster properties, defaults, and validation for reservation spec shape. |
| src/v/config/BUILD | Adds dependency on cloud_io:scheduler_types for config library. |
| src/v/cluster/archival/tests/service_fixture.cc | Updates archival test fixture to configure the scheduler for small connection limits. |
| src/v/cloud_topics/level_zero/tests/l0_object_size_distribution_test.cc | Updates remote API mocks/calls for new group_id parameter. |
| src/v/cloud_topics/level_zero/reader/tests/mocks.h | Updates remote mock interface to accept group_id (with default in override helper). |
| src/v/cloud_topics/level_zero/reader/materialized_extent.cc | Tags L0 downloads as consumer_fetch. |
| src/v/cloud_topics/level_zero/batcher/tests/remote_mock.h | Updates batcher remote mocks/expectations for new group_id parameter. |
| src/v/cloud_topics/level_zero/batcher/batcher.cc | Tags L0 uploads as producer_upload. |
| src/v/cloud_topics/level_one/metastore/tests/garbage_collector_test.cc | Updates io::read_object overrides to accept group_id. |
| src/v/cloud_topics/level_one/frontend_reader/level_one_reader.cc | Tags L1 reads as consumer_fetch. |
| src/v/cloud_topics/level_one/common/file_io.h | Extends io implementation interface to accept group_id. |
| src/v/cloud_topics/level_one/common/file_io.cc | Plumbs group_id through to cloud download calls. |
| src/v/cloud_topics/level_one/common/fake_io.h | Updates fake IO interface to accept group_id. |
| src/v/cloud_topics/level_one/common/fake_io.cc | Implements updated fake IO signature (ignores group_id). |
| src/v/cloud_topics/level_one/common/BUILD | Adds dependency on cloud_io:scheduler_types. |
| src/v/cloud_topics/level_one/common/abstract_io.h | Extends abstract IO API (read_object, read_object_as_iobuf) with group_id. |
| src/v/cloud_topics/level_one/common/abstract_io.cc | Plumbs group_id through read_object_as_iobuf to read_object. |
| src/v/cloud_storage/configuration.h | Adds cloud_io::scheduler_config to cloud storage runtime configuration. |
| src/v/cloud_storage/configuration.cc | Builds scheduler config from cluster properties and guards against reservation sums exceeding pool capacity. |
| src/v/cloud_storage/BUILD | Adds dependency on cloud_io:scheduler_types. |
| src/v/cloud_storage_clients/tests/client_pool_builder.h | Adds ability to inject a scheduler config into test client pool builds. |
| src/v/cloud_storage_clients/client_pool.h | Adds scheduler integration and new acquire overloads that take a group_id. |
| src/v/cloud_storage_clients/client_pool.cc | Implements scheduler-gated lease acquisition, release, and borrow-path integration. |
| src/v/cloud_storage_clients/BUILD | Adds dependencies on cloud_io:scheduler and cloud_io:scheduler_types. |
| src/v/cloud_io/tests/scheduler_test.cc | Adds unit tests for scheduler wrapper behavior (passthrough + reservation integration). |
| src/v/cloud_io/tests/reservation_policy_test.cc | Adds detailed unit tests for reservation policy behavior and edge cases. |
| src/v/cloud_io/tests/BUILD | Registers new cloud_io gtests in Bazel build. |
| src/v/cloud_io/scheduler.h | Introduces scheduler wrapper API (admit/try_admit/release + observability). |
| src/v/cloud_io/scheduler.cc | Implements scheduler wrapper and passthrough policy + factory wiring. |
| src/v/cloud_io/scheduler-design.md | Adds design notes for the scheduler/reservation policy and config surface. |
| src/v/cloud_io/scheduler_types.h | Adds shared types: policy_type, group_id, parsing helpers, and scheduler config structs. |
| src/v/cloud_io/scheduler_policy.h | Adds scheduler policy abstract base class. |
| src/v/cloud_io/reservation_policy.h | Introduces reservation-based scheduler policy interface and metrics hooks. |
| src/v/cloud_io/reservation_policy.cc | Implements reservation policy admission/release logic and metrics registration. |
| src/v/cloud_io/reservation_policy_types.h | Adds internal structs for per-group state, waiters, and dwell/refill mechanics. |
| src/v/cloud_io/remote.h | Extends remote API methods to accept group_id for classification. |
| src/v/cloud_io/remote.cc | Plumbs group_id into client pool acquisition for download/upload paths. |
| src/v/cloud_io/remote_api.h | Extends remote_api virtual interface to accept group_id with defaults. |
| src/v/cloud_io/BUILD | Adds new cloud_io libraries for scheduler/policies/types and wires deps. |
Review — Cloud I/O Scheduler: cluster config and metricsNice, well-documented final step in the series. The config plumbing (enum_property + convert + rjson_serialize + property_type_name) is complete and consistent, the metrics wiring looks correct, and the graceful fallback in Notable behavior change (intended, but flagging)
Findings (left inline)
Minor / non-blocking
Overall looks solid — the inline items are small. 👍 |
8ff728c to
beaaf86
Compare
beaaf86 to
f364a76
Compare
|
/cdt |
f364a76 to
6a83dc1
Compare
Add the metrics surface for reservation_policy. Internal metrics: - aggregate: available_slots, total_capacity, total_waiters, total_waiters_canceled - per-group: in_flight, waiters, admit_total, admit_immediate_total, current_reserved, canceled_total Public metrics (shard-aggregated): - aggregate: available_slots, total_capacity - per-group: in_flight, waiters Also adds the required counters and accessors for tracking canceled waiters. Lumped in with the metrics because it's trivial and related. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
6a83dc1 to
7d0eaa9
Compare
|
force push CR changes force push rebase dev for merge conflict |
Add two cluster properties: - cloud_io_scheduler_policy: selector for cloud_io::scheduler's admission policy. needs_restart=true. - cloud_io_scheduler_reservation: list of "group_name:slots" entries that supply per-group reservation targets to reservation_policy. Defaults to 2 slots each for producer_upload, consumer_fetch, and default_group as a starvation-prevention floor. Only consulted when cloud_io_scheduler_policy=reservation. cloud_storage::configuration::get_config reads both properties at startup and stages them into the scheduler_config field that flows down to client_pool and scheduler. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Replace the placeholder reservation case in scheduler::make_policy with construction of reservation_policy from the reservation_policy_config threaded through scheduler_config. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Three integration tests that construct a real reservation_policy through the scheduler shell: - ReservationHasWaitersReflectsQueueState: has_waiters() flips with queue state across admit/release. - ReservationStopDrainsAndRejectsAdmits: stop() resolves queued waiters with abort and subsequent admit/try_admit reject. - ReservationReservationsRespectConfiguredTargets: the per-group reservation passed in via scheduler_config is observable in admission behavior. Policy internals are exercised by reservation_policy_test.cc. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
This is needed to support higher throughput usage for Cloud Topics in particular. The no-op passthrough policy is kept around as an escape hatch. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
7d0eaa9 to
19f7cf9
Compare
Retry command for Build#85726please wait until all jobs are finished before running the slash command |
|
/ci-repeat 1 |
|
custom partitioning test failed unrelated based on claude investigation but let's run it a few more times. |
Lazin
left a comment
There was a problem hiding this comment.
The code looks good.
What's the point of having passthrough mode? Is it just an escape hatch or do we need to test with both reservation and passthrough?
@Lazin I'd say a bit of both. Primarily an escape hatch though. |
Sequence:
Backports Required
Release Notes