Skip to content

Replace the backup disk with a soft_delete flag on object storage disks - #44

Open
joelynch wants to merge 1 commit into
v26.3.15.4-lts-aivenfrom
joelynch/fix-backup-disk
Open

Replace the backup disk with a soft_delete flag on object storage disks#44
joelynch wants to merge 1 commit into
v26.3.15.4-lts-aivenfrom
joelynch/fix-backup-disk

Conversation

@joelynch

Copy link
Copy Markdown

No description provided.

/// 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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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
joelynch force-pushed the joelynch/fix-backup-disk branch from 98fbd2a to 3517bf9 Compare August 20, 2026 12:34
@joelynch
joelynch marked this pull request as ready for review August 21, 2026 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants