Skip to content

storage: admit tiered_cloud partitions to space management #30993

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

Open
oleiman wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

storage: admit tiered_cloud partitions to space management #30993

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
153100e
storage: include tiered_cloud partitions in disk space management
oleiman Jul 1, 2026
cdca56e
storage: skip local housekeeping and gc for cloud topics
oleiman Jul 3, 2026
a5d1f76
cloud_topics/stm: bound the prefix-truncate wait for tiered_cloud
oleiman Jul 2, 2026
717967d
tests: cover tiered_cloud capacity and idle-partition reclaim
oleiman Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/v/resource_mgmt/storage.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,9 @@ eviction_policy::collect_reclaimable_offsets() {
*/
chunked_vector<ss::lw_shared_ptr<cluster::partition>> partitions;
for (const auto& p : _pm->local().partitions()) {
if (!p.second->remote_partition()) {
if (
!p.second->remote_partition()
&& !p.second->log()->config().is_tiered_cloud()) {
continue;
}
Comment thread
oleiman marked this conversation as resolved.
partitions.push_back(p.second);
Expand Down
46 changes: 32 additions & 14 deletions src/v/storage/disk_log_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1228,9 +1228,9 @@ disk_log_impl::maybe_apply_local_storage_overrides(gc_config cfg) const {

// cloud_retention is disabled, do not override. Cloud-topic partitions
// (storage.mode in {cloud, tiered_cloud}) bypass this gate:
// is_cloud_retention_active() is false for them, but ctp_stm still needs
// is_archival_active() is false for them, but ctp_stm still needs
// the local-target override to engage under retention_local_strict.
if (!is_cloud_retention_active() && !config().cloud_topic_enabled()) {
if (!is_archival_active() && !config().cloud_topic_enabled()) {
return cfg;
}

Expand Down Expand Up @@ -1310,11 +1310,15 @@ gc_config disk_log_impl::apply_local_storage_overrides(gc_config cfg) const {
return cfg;
}

bool disk_log_impl::is_cloud_retention_active() const {
bool disk_log_impl::is_archival_active() const {
return config::shard_local_cfg().cloud_storage_enabled()
&& (config().is_archival_enabled());
}

bool disk_log_impl::is_cloud_gc_active() const {
return is_archival_active() || config().is_tiered_cloud();
}

@Lazin Lazin Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The funny thing is that I had this in my feature branch - https://github.com/redpanda-data/redpanda/pull/30512/changes#diff-8caa0c9719de25bd209fa674959d9327cd55237de2fb4683761acf9bcc6de217R1314
and it was lost in the process (PR stacking / review). Damn.

/*
* applies overrides for non-cloud storage settings
*/
Expand Down Expand Up @@ -1358,6 +1362,11 @@ gc_config disk_log_impl::apply_overrides(gc_config defaults) const {
}

ss::future<> disk_log_impl::housekeeping(housekeeping_config cfg) {
// Cloud topics do local retention/compaction elsewhere (ctp_stm trims the
// local log; compaction is at L1), so this loop does nothing for them.
if (config().cloud_topic_enabled()) {

@Lazin Lazin Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we need an additional check here?

co_return;
}
ss::gate::holder holder{_compaction_housekeeping_gate};
vlog(
gclog.trace,
Expand Down Expand Up @@ -1763,12 +1772,21 @@ ss::future<> disk_log_impl::rewrite_segment_with_offset_map(
}

ss::future<> disk_log_impl::gc(gc_config cfg) {
// Cloud topics do local retention/compaction elsewhere (ctp_stm trims the
// local log; compaction is at L1), so disk_log_impl gc does not apply.
if (config().cloud_topic_enabled()) {

@Lazin Lazin Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto, it should be bypassed on a different level

@Lazin Lazin Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The do_gc uses is_localy_collectible to gate the garbage collection. Maybe it makes sense to just not start the housekeeping loop in the log_manager::start?

co_return;
}
ss::gate::holder holder{_compaction_housekeeping_gate};
co_await do_gc(cfg);
}

ss::future<std::optional<model::offset>> disk_log_impl::do_gc(gc_config cfg) {
vassert(!_closed, "gc on closed log - {}", *this);
vassert(
!config().cloud_topic_enabled(),
"[{}] gc on cloud topic partition",
config().ntp());
Comment thread
oleiman marked this conversation as resolved.

cfg = apply_overrides(cfg);

Expand All @@ -1783,7 +1801,7 @@ ss::future<std::optional<model::offset>> disk_log_impl::do_gc(gc_config cfg) {
const auto offset = _cloud_gc_offset.value();
_cloud_gc_offset.reset();

if (!is_cloud_retention_active()) {
if (!is_archival_active()) {
vlog(
gclog.warn,
"[{}] expected remote retention to be active",
Expand Down Expand Up @@ -1916,12 +1934,12 @@ disk_log_impl::compute_gc_offset(gc_config cfg) {
// ctp_stm for cloud-topic partitions. The offset is retention-driven
// unless space management has pinned _cloud_gc_offset, which then takes
// precedence. maybe_apply_local_storage_overrides
// bypasses the is_cloud_retention_active() gate for cloud-topic partitions
// bypasses the is_archival_active() gate for cloud-topic partitions
// (the gate is false for storage.mode in {cloud, tiered_cloud}), so the
// local-target override engages there under retention_local_strict.
cfg = apply_kafka_retention_overrides(cfg);
if (_cloud_gc_offset.has_value()) {
co_return _cloud_gc_offset;
co_return std::exchange(_cloud_gc_offset, std::nullopt);
Comment thread
oleiman marked this conversation as resolved.
}
co_await maybe_adjust_retention_timestamps();
cfg = maybe_apply_local_storage_overrides(cfg);
Expand Down Expand Up @@ -3921,7 +3939,7 @@ disk_log_impl::disk_usage_and_reclaimable_space(gc_config input_cfg) {
&& seg->offsets().get_dirty_offset() <= retention_offset.value()) {
retention_segments.push_back(seg);
} else if (
is_cloud_retention_active()
is_cloud_gc_active()
&& seg->offsets().get_dirty_offset() <= max_removable) {
available_segments.push_back(seg);
} else {
Expand All @@ -3937,8 +3955,8 @@ disk_log_impl::disk_usage_and_reclaimable_space(gc_config input_cfg) {
* get_reclaimable_offsets is going to be merged together.
*/
if (
!config().is_read_replica_mode_enabled()
&& is_cloud_retention_active() && seg != _segs.back()
!config().is_read_replica_mode_enabled() && is_cloud_gc_active()
&& seg != _segs.back()
&& seg->offsets().get_dirty_offset() <= max_removable
&& local_retention_offset.has_value()
&& seg->offsets().get_dirty_offset()
Expand Down Expand Up @@ -4279,8 +4297,8 @@ ss::future<usage_report> disk_log_impl::disk_usage(gc_config cfg) {
chunked_vector<ss::lw_shared_ptr<segment>>
disk_log_impl::cloud_gc_eligible_segments() {
vassert(
is_cloud_retention_active(),
"Expected {} to have cloud retention enabled",
is_cloud_gc_active(),
"Expected cloud GC to be active for {}",
config().ntp());
Comment thread
oleiman marked this conversation as resolved.

constexpr size_t keep_segs = 1;
Expand Down Expand Up @@ -4313,7 +4331,7 @@ disk_log_impl::cloud_gc_eligible_segments() {
}

void disk_log_impl::set_cloud_gc_offset(model::offset offset) {
if (!is_cloud_retention_active()) {
if (!is_cloud_gc_active()) {
vlog(
stlog.debug,
"Ignoring request to set GC offset on non-cloud enabled partition "
Expand All @@ -4338,7 +4356,7 @@ disk_log_impl::get_reclaimable_offsets(gc_config cfg) {

reclaimable_offsets res;

if (!is_cloud_retention_active()) {
if (!is_cloud_gc_active()) {
vlog(
stlog.debug,
"Reporting no reclaimable offsets for non-cloud partition {}",
Expand Down Expand Up @@ -4540,7 +4558,7 @@ size_t disk_log_impl::reclaimable_size_bytes() const {
* local retention size may change. catch these before reporting potentially
* stale information.
*/
if (!is_cloud_retention_active()) {
if (!is_cloud_gc_active()) {
return 0;
}
if (config().is_read_replica_mode_enabled()) {
Expand Down
12 changes: 8 additions & 4 deletions src/v/storage/disk_log_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,9 @@ class disk_log_impl final : public log {
/// Applies retention overrides (callers need not pre-apply them) and
/// adjusts bogus (future) retention timestamps, mutating segment indexes
/// when an entire segment is bogus, then returns the offset GC would
/// evict to -- usually derived from local retention, but when space
/// management has set _cloud_gc_offset that offset is returned instead
/// (without resetting it -- that is do_gc's responsibility).
/// evict to. Usually derived from local retention; when space management
/// has set _cloud_gc_offset, that offset is returned and consumed (reset)
/// here, so the caller is responsible for acting on it.
ss::future<std::optional<model::offset>>
compute_gc_offset(gc_config cfg) final;

Expand Down Expand Up @@ -421,7 +421,11 @@ class disk_log_impl final : public log {
gc_config maybe_apply_local_storage_overrides(gc_config) const;
gc_config apply_local_storage_overrides(gc_config) const;

bool is_cloud_retention_active() const;
bool is_archival_active() const;
// True when local segments are a reclaimable cache of cloud-resident data
// (legacy tiered storage or tiered_cloud); broader than
// is_archival_active(), which is archival-only.
bool is_cloud_gc_active() const;
Comment on lines +425 to +428

@andrwng andrwng Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: IMO the naming of these don't make it clear that there's any difference between the two. Would it make sense to make is_cloud_retention_active refer to tiered storage specifically or something? Or if the idea is that the behavior changes because of local housekeeping, is there be a is_local_housekeeping_active() that can use to compose our conditions? Like AFAICT the only time we want to use is_cloud_retention_active() is determine if the housekeeping loop is running and we have cloud data

@oleiman oleiman Jul 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah fair

is_local_housekeeping_active

like replace cloud_retention_active with that, yeah? and leave cloud_gc_active alone? i don't mind that. ctp housekeeping loop is also cleaning up local data, but local_housekeeping as the condition governing the disk log impl code reads a bit better IMO.

@oleiman oleiman Jul 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

renamed is_cloud_retention_active to is_archival_active, which is at least a bit more accurate to its semantics. left is_cloud_gc_active alone. idk. this is all sort of circuitous but I can't think of a better factoring right now.


// returns retention_offset(cfg) but may also first apply adjustments to
// future timestamps if this option is turned on in configuration.
Expand Down