Replace the backup disk with a soft_delete flag on object storage disks - #44
Open
joelynch wants to merge 1 commit into
Open
Replace the backup disk with a soft_delete flag on object storage disks#44joelynch wants to merge 1 commit into
backup disk with a soft_delete flag on object storage disks#44joelynch wants to merge 1 commit into
Conversation
| /// The wrapping must happen here, before the object storage reaches the router, the metadata storage, | ||
| /// the blob killer and the blob copier, so that none of them can hold the undecorated storage and | ||
| /// delete a blob behind the marker's back. | ||
| ObjectStoragePtr wrapIfSoftDelete( |
There was a problem hiding this comment.
soft_delete is silent in both directions
- nothing in the log when it engages
- no error when it is set where nothing reads it
After create_directories we log when it engages
LOG_INFO(
getLogger("registerDiskObjectStorage"),
"Disk `{}`: soft delete enabled, deletion markers under {}",
disk_name, markers_path);
This makes the invariant auditable in an incident: "was this disk soft-deleting?" becomes one grep instead of an inference from the stack shape.
Reject it where it is ignored
/// `soft_delete` is honoured only by the object storage disk that owns the blobs (see
/// `wrapIfSoftDelete`). A disk layer above it cannot implement soft delete: it would
/// delegate the removal downwards and the blob would be physically unlinked. Reject the
/// flag rather than accept a config that silently does nothing.
void rejectSoftDeleteFlag(
const Poco::Util::AbstractConfiguration & config,
const String & config_prefix,
const String & name,
const String & disk_type)
{
if (!config.getBool(config_prefix + ".soft_delete", false))
return;
throw Exception(
ErrorCodes::BAD_ARGUMENTS,
"Disk `{}` of type `{}` does not support `soft_delete`. Set it on the object storage disk "
"that holds the blobs, not on a layer above it",
name, disk_type);
}
… disks
Aiven's external GC requires that ClickHouse never physically deletes remote
blobs: removals must become marker files that an out-of-band process reconciles.
This was implemented as a `backup` disk type that wrapped an already-constructed
disk, plus machinery to suppress the physical deletions the layers underneath it
would otherwise still perform.
That approach could not be made correct. By the time the `backup` disk wrapped
another disk, the raw object storage had already been handed out to several
owners the wrapper had no way to reach:
* the inner disk itself, which stays live in the global `DisksMap`;
* the inner disk's `BlobCopierThread`, which was never disabled;
* the inner disk's `BlobKillerThread` one level deeper than the wrapped disk -
`wrapWithBackup` only disabled the disk it directly wrapped, so in the
production `cache -> object_storage` stack the base killer kept draining its
own removal queue through the raw object storage;
* `plain` and `plain_rewritable` metadata storages, which capture the object
storage by value in `MetadataStorageFactory` and call `removeObjectsIfExist`
directly, bypassing the removal queue entirely.
The last one has no possible fix at the disk level. It happens not to fire today
only because the deployment uses `metadata_type = local`, whose
`MetadataStorageFromDisk` holds no object storage pointer at all - an accident of
configuration, not a property of the design.
Wrap at construction instead. `RegisterDiskObjectStorage` now applies
`SoftDeleteObjectStorage` to the object storage as it is created, before it is
placed in the router. Every downstream consumer - the router, metadata storage
factory, blob killer, blob copier, transactions - receives the decorated storage
and no component can hold an undecorated one. The invariant becomes structural
rather than something maintained by disabling things after the fact.
Configuration moves onto the object storage disk itself:
<disk_name>
<type>object_storage</type>
<object_storage_type>s3</object_storage_type>
<soft_delete>1</soft_delete>
<soft_delete_markers_path>...</soft_delete_markers_path>
</disk_name>
`soft_delete` is rejected on multi-location disks, where a removal is only
complete once every location has dropped the blob and a single marker cannot
express that.
Because nothing can bypass the soft-delete layer any more, all of the
compensating machinery is removed rather than ported:
* the `backup` disk type and `registerDiskBackup`;
* `DiskObjectStorage::wrapWithBackup` and `stopRecordingRemovals`;
* `BlobKillerThread::detachWrapped`, `disable` and the sticky `force_disabled`
flag that had to survive `SYSTEM RELOAD CONFIG`;
* `setRecordRemovals` / `record_removals` in `MetadataStorageFromDisk` and
`MetadataStorageFromCacheObjectStorage` - removals are enqueued
unconditionally again.
`BackupObjectStorage` is renamed to `SoftDeleteObjectStorage`, which describes
what it does rather than what it was for; the disk is no more a backup than any
other, it just defers deletion.
Tests: `test_aiven_backup_disk` becomes `test_aiven_soft_delete` and
`test_aiven_backup_disk_cache_layer` becomes `test_aiven_soft_delete_cache_layer`,
both configuring the flag inline on the object storage disk. The cache-layer test
is the interesting one - it pins the shape that used to be unsound, where a
killer below the wrapper unlinked blobs the layer above had only marked.
`test_aiven_backup_disk_cache_layer_reload` is deleted outright: it existed only
to prove the sticky disable flag survived a config reload, and there is no longer
a disable flag to make sticky.
Supersedes patch-fix (026).
joelynch
force-pushed
the
joelynch/fix-backup-disk
branch
from
August 20, 2026 12:34
98fbd2a to
3517bf9
Compare
joelynch
marked this pull request as ready for review
August 21, 2026 08:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.