From 3517bf94347c084f9e112208a33d731733050534 Mon Sep 17 00:00:00 2001 From: Joe Lynch Date: Wed, 19 Aug 2026 16:23:34 +0200 Subject: [PATCH] Replace the `backup` disk with a `soft_delete` flag on object storage 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: object_storage s3 1 ... `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). --- src/CMakeLists.txt | 2 +- src/Disks/DiskFactory.cpp | 20 ++ src/Disks/DiskFactory.h | 6 + .../DiskObjectStorage/DiskObjectStorage.cpp | 94 -------- .../DiskObjectStorage/DiskObjectStorage.h | 5 - .../MetadataStorageFromCacheObjectStorage.cpp | 5 +- .../MetadataStorageFromCacheObjectStorage.h | 9 - .../Local/MetadataStorageFromDisk.cpp | 4 - .../Local/MetadataStorageFromDisk.h | 9 - .../Backup/registerDiskBackup.cpp | 67 ------ .../SoftDeleteObjectStorage.cpp} | 28 +-- .../SoftDeleteObjectStorage.h} | 11 +- .../RegisterDiskObjectStorage.cpp | 78 +++++-- .../Replication/BlobKillerThread.cpp | 36 +-- .../Replication/BlobKillerThread.h | 19 +- src/Disks/registerDisks.cpp | 4 - src/Disks/tests/gtest_metadata_local_disk.cpp | 41 +--- .../__init__.py | 0 .../config.d/reload_storage.xml | 36 --- .../test.py | 164 ------------- .../__init__.py | 0 .../test.py | 215 ++++++++---------- .../__init__.py | 0 .../test.py | 85 +++---- 24 files changed, 247 insertions(+), 691 deletions(-) delete mode 100644 src/Disks/DiskObjectStorage/ObjectStorages/Backup/registerDiskBackup.cpp rename src/Disks/DiskObjectStorage/ObjectStorages/{Backup/BackupObjectStorage.cpp => SoftDelete/SoftDeleteObjectStorage.cpp} (70%) rename src/Disks/DiskObjectStorage/ObjectStorages/{Backup/BackupObjectStorage.h => SoftDelete/SoftDeleteObjectStorage.h} (93%) delete mode 100644 tests/integration/test_aiven_backup_disk_cache_layer_reload/__init__.py delete mode 100644 tests/integration/test_aiven_backup_disk_cache_layer_reload/config.d/reload_storage.xml delete mode 100644 tests/integration/test_aiven_backup_disk_cache_layer_reload/test.py rename tests/integration/{test_aiven_backup_disk => test_aiven_soft_delete}/__init__.py (100%) rename tests/integration/{test_aiven_backup_disk => test_aiven_soft_delete}/test.py (65%) rename tests/integration/{test_aiven_backup_disk_cache_layer => test_aiven_soft_delete_cache_layer}/__init__.py (100%) rename tests/integration/{test_aiven_backup_disk_cache_layer => test_aiven_soft_delete_cache_layer}/test.py (80%) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 375055bc361e..84c0b901f898 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -181,7 +181,7 @@ endif() add_headers_and_sources(dbms Databases/DataLake) -add_headers_and_sources(dbms Disks/DiskObjectStorage/ObjectStorages/Backup) +add_headers_and_sources(dbms Disks/DiskObjectStorage/ObjectStorages/SoftDelete) add_headers_and_sources(dbms Disks/DiskObjectStorage/ObjectStorages/Cached) add_headers_and_sources(dbms Disks/DiskObjectStorage/ObjectStorages/Local) add_headers_and_sources(dbms Disks/DiskObjectStorage/ObjectStorages/Web) diff --git a/src/Disks/DiskFactory.cpp b/src/Disks/DiskFactory.cpp index 00ebd4aac84b..d1691113d505 100644 --- a/src/Disks/DiskFactory.cpp +++ b/src/Disks/DiskFactory.cpp @@ -4,6 +4,7 @@ namespace DB { namespace ErrorCodes { + extern const int BAD_ARGUMENTS; extern const int LOGICAL_ERROR; extern const int UNKNOWN_ELEMENT_IN_CONFIG; } @@ -20,6 +21,11 @@ void DiskFactory::registerDiskType(const String & disk_type, Creator creator) throw Exception(ErrorCodes::LOGICAL_ERROR, "DiskFactory: the disk type '{}' is not unique", disk_type); } +void DiskFactory::markSoftDeleteCapable(const String & disk_type) +{ + soft_delete_capable_types.insert(disk_type); +} + DiskPtr DiskFactory::create( const String & name, const Poco::Util::AbstractConfiguration & config, @@ -44,6 +50,19 @@ DiskPtr DiskFactory::create( return nullptr; } + /// `soft_delete` is honoured only by the disk that owns the blobs. A layer above it delegates the + /// removal downwards, so the blob would be physically unlinked despite the flag; reject such a + /// config rather than let it silently do nothing. The check lives here, at the single point every + /// disk is created, so it cannot fall out of step as disk types are added. + if (config.getBool(config_prefix + ".soft_delete", false) && !soft_delete_capable_types.contains(disk_type)) + { + 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); + } + const auto & disk_creator = found->second; return disk_creator(name, config, config_prefix, context, map, attach, custom_disk); } @@ -51,5 +70,6 @@ DiskPtr DiskFactory::create( void DiskFactory::clearRegistry() { registry.clear(); + soft_delete_capable_types.clear(); } } diff --git a/src/Disks/DiskFactory.h b/src/Disks/DiskFactory.h index 4a4ffa616ff8..121f0589bfeb 100644 --- a/src/Disks/DiskFactory.h +++ b/src/Disks/DiskFactory.h @@ -10,6 +10,7 @@ #include #include #include +#include namespace DB @@ -35,6 +36,10 @@ class DiskFactory final : private boost::noncopyable void registerDiskType(const String & disk_type, Creator creator); + /// Declares that this disk type's creator honours `soft_delete`; every other type is rejected + /// when the flag is set. Call it wherever the type is registered, so the two cannot drift apart. + void markSoftDeleteCapable(const String & disk_type); + DiskPtr create( const String & name, const Poco::Util::AbstractConfiguration & config, @@ -50,6 +55,7 @@ class DiskFactory final : private boost::noncopyable private: using DiskTypeRegistry = std::unordered_map; DiskTypeRegistry registry; + std::unordered_set soft_delete_capable_types; }; } diff --git a/src/Disks/DiskObjectStorage/DiskObjectStorage.cpp b/src/Disks/DiskObjectStorage/DiskObjectStorage.cpp index 104bd15bdda5..4afdc245699f 100644 --- a/src/Disks/DiskObjectStorage/DiskObjectStorage.cpp +++ b/src/Disks/DiskObjectStorage/DiskObjectStorage.cpp @@ -16,9 +16,6 @@ #include #include #include -#include -#include -#include #include #include #include @@ -47,25 +44,6 @@ namespace ErrorCodes { extern const int INCORRECT_DISK_INDEX; extern const int CANNOT_RMDIR; - extern const int BAD_ARGUMENTS; -} - -namespace -{ - /// Turn off the in-memory removal queue of a metadata storage below a backup layer (see - /// `wrapWithBackup`). Reached by concrete type rather than a virtual on `IMetadataStorage` on - /// purpose: adding a virtual would shift every subclass's vtable slots and force a full rebuild; - /// only these two concrete storages own a removal queue. An unrecognized type simply keeps - /// recording (correct, just not optimized) — assert in debug so a new queue-holder is not missed. - void stopRecordingRemovals(IMetadataStorage & metadata_storage) - { - if (auto * from_disk = dynamic_cast(&metadata_storage)) - from_disk->setRecordRemovals(false); - else if (auto * from_cache = dynamic_cast(&metadata_storage)) - from_cache->setRecordRemovals(false); - else - chassert(false && "backup layer wraps a metadata storage with an undrained removal queue"); - } } DiskTransactionPtr DiskObjectStorage::createTransaction() @@ -80,78 +58,6 @@ ObjectStoragePtr DiskObjectStorage::getObjectStorage() return object_storages->takePointingTo(cluster->getLocalLocation()); } -DiskObjectStoragePtr DiskObjectStorage::wrapWithBackup(const String & layer_name, const String & backup_base_path) const -{ - /// The backup layer soft-deletes objects only at the local location's object storage. On a - /// multi-location (replicated) disk the background blob replication/GC would not observe those - /// soft-deletes, so the external-GC contract would be unsound. We only support single-location - /// object storage disks (tiered storage); reject the unsupported multi-location case explicitly. - if (cluster->getConfiguration().size() > 1) - throw Exception( - ErrorCodes::BAD_ARGUMENTS, - "Cannot wrap disk with backup layer `{}`: backup is only supported on single-location " - "object storage disks, but the wrapped disk has {} locations", - layer_name, cluster->getConfiguration().size()); - - auto registry = object_storages->getRegistry(); - auto local_location = cluster->getLocalLocation(); - registry[local_location] = std::make_shared(registry[local_location], backup_base_path, layer_name); - - /// Backup only intercepts object deletions, not metadata, so metadata_storage is passed through unchanged. - auto backup_disk = std::make_shared( - layer_name, - std::make_shared(layer_name, cluster->getConfiguration()), - metadata_storage, - std::make_shared(std::move(registry)), - std::dynamic_pointer_cast(shared_from_this()), - Context::getGlobalContextInstance()->getConfigRef(), - "storage_configuration.disks." + layer_name, - use_fake_transaction); - - /// Deferred-delete correctness. - /// - /// On 26.3 blob deletion is deferred: a DROP/merge enqueues orphaned blobs into an in-memory - /// removal queue owned by each disk's `metadata_storage`, and that disk's background - /// `BlobKillerThread` later drains it via - /// `object_storages->takePointingTo(location)->removeObjectsIfExist(...)`. In a stack - /// (`cache -> object_storage`) EACH layer owns its own queue: a commit fills the base queue and, - /// as it unwinds, each wrapper copies the transaction-local removal list into its own queue. - /// The backup layer reuses THIS disk's `metadata_storage` (only the local-location object - /// storage is swapped for `BackupObjectStorage`), so the backup killer drains THIS disk's queue - /// and writes deletion markers (soft delete) instead of unlinking. If the wrapped killers stayed - /// live they would drain THEIR queues through the RAW object storage and physically unlink the - /// blob — defeating the markers and the external-GC contract. - /// - /// Fix, two complementary halves: - /// - /// 1. Only the backup disk's killer may run on the queue it drains. The backup disk reuses - /// THIS disk's `metadata_storage`, so the backup killer drains THIS disk's removal queue and - /// writes soft-delete markers. Disable this disk's own killer (it would drain the same queue - /// through the RAW object storage and physically unlink the marked blobs) and detach the - /// wrapped chain from the backup killer so `triggerAndWait` won't fire it. `disable` is - /// sticky across `SYSTEM RELOAD CONFIG` (see `BlobKillerThread::disable`). - /// - /// 2. Disks BELOW this one (e.g. `cache -> object_storage`) each own a SEPARATE removal queue - /// filled at commit, but the backup killer only drains THIS disk's queue — nobody drains - /// theirs. Disable their killers too (defense-in-depth against a physical delete) AND stop - /// them recording removals at the source (`stopRecordingRemovals`): otherwise their queues - /// grow unbounded for the process lifetime (memory leak / eventual OOM). Suppressing at the - /// source keeps the transaction-local removal list intact, so this disk's queue is still - /// populated and the backup killer still soft-deletes every removed blob. - /// - /// This relies on the wrapped stack being private to the backup wrapper, which holds for our - /// single-location tiered-storage deployment. - backup_disk->blob_killer->detachWrapped(); - blob_killer->disable(); - for (DiskObjectStorageConstPtr layer = wrapped_disk; layer; layer = layer->wrapped_disk) - { - layer->blob_killer->disable(); - stopRecordingRemovals(*layer->metadata_storage); - } - - return backup_disk; -} - DiskTransactionPtr DiskObjectStorage::createObjectStorageTransaction() { return std::make_shared(cluster, metadata_storage, object_storages, blob_killer, wait_blob_removal, getReadResourceName(), getWriteResourceName()); diff --git a/src/Disks/DiskObjectStorage/DiskObjectStorage.h b/src/Disks/DiskObjectStorage/DiskObjectStorage.h index 90e525f9c524..2426aa987ef4 100644 --- a/src/Disks/DiskObjectStorage/DiskObjectStorage.h +++ b/src/Disks/DiskObjectStorage/DiskObjectStorage.h @@ -221,11 +221,6 @@ friend class DiskObjectStorageReservation; /// DiskObjectStorage(CachedObjectStorage(...CacheObjectStorage(S3ObjectStorage)...)) DiskObjectStoragePtr wrapWithCache(FileCachePtr cache, const FileCacheSettings & cache_settings, const String & layer_name) const; - /// Add a backup layer that turns object deletions into local deletion-marker files. - /// Like wrapWithCache, this returns a new DiskObjectStorage whose local object storage - /// is wrapped in a BackupObjectStorage and which links back to this disk via wrapped_disk. - DiskObjectStoragePtr wrapWithBackup(const String & layer_name, const String & backup_base_path) const; - bool supportsLayers() const override { return true; } /// Get names of all cache layers. Name is how cache is defined in configuration file. diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/Cache/MetadataStorageFromCacheObjectStorage.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/Cache/MetadataStorageFromCacheObjectStorage.cpp index 55ebd69e9cf2..f1d1af2ae2c2 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/Cache/MetadataStorageFromCacheObjectStorage.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/Cache/MetadataStorageFromCacheObjectStorage.cpp @@ -265,9 +265,6 @@ void MetadataStorageFromCacheObjectStorageTransaction::commit(const TransactionC { underlying->commit(options); - /// See `setRecordRemovals`: skip enqueuing when a backup layer below owns deletion for this - /// cache. The transaction-local list still flows up via `getSubmittedForRemovalBlobs`. - if (metadata_storage.record_removals.load(std::memory_order_relaxed)) { std::lock_guard guard(metadata_storage.removed_objects_mutex); metadata_storage.objects_to_remove.submitForRemoval(underlying->getSubmittedForRemovalBlobs()); @@ -278,7 +275,7 @@ TransactionCommitOutcomeVariant MetadataStorageFromCacheObjectStorageTransaction { auto result = underlying->tryCommit(options); - if (isSuccessfulOutcome(result) && metadata_storage.record_removals.load(std::memory_order_relaxed)) + if (isSuccessfulOutcome(result)) { std::lock_guard guard(metadata_storage.removed_objects_mutex); metadata_storage.objects_to_remove.submitForRemoval(underlying->getSubmittedForRemovalBlobs()); diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/Cache/MetadataStorageFromCacheObjectStorage.h b/src/Disks/DiskObjectStorage/MetadataStorages/Cache/MetadataStorageFromCacheObjectStorage.h index 5fa30e85021c..5cc03281bcba 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/Cache/MetadataStorageFromCacheObjectStorage.h +++ b/src/Disks/DiskObjectStorage/MetadataStorages/Cache/MetadataStorageFromCacheObjectStorage.h @@ -72,10 +72,6 @@ class MetadataStorageFromCacheObjectStorage : public IMetadataStorage int64_t recordAsRemoved(const StoredObjects & blobs) override; bool hasPendingRemovalBlobs(const StoredObjects & blobs) const override; - /// Non-virtual (reached via `dynamic_cast` from `DiskObjectStorage::wrapWithBackup`), see that - /// call site and `record_removals`. Adds no slot to the `IMetadataStorage` vtable. - void setRecordRemovals(bool value) { record_removals.store(value, std::memory_order_relaxed); } - BlobsToReplicate getBlobsToReplicate(const ClusterConfigurationPtr & cluster, int64_t max_count) override; int64_t recordAsReplicated(const BlobsToReplicate & blobs) override; bool hasUnreplicatedBlobs(const Location & location_to_check) override; @@ -92,11 +88,6 @@ class MetadataStorageFromCacheObjectStorage : public IMetadataStorage mutable std::mutex removed_objects_mutex; InMemoryRemovalQueue objects_to_remove TSA_GUARDED_BY(removed_objects_mutex); - - /// When false, commits skip enqueuing orphaned blobs into `objects_to_remove` (see - /// `setRecordRemovals`). Set by a backup layer only for a cache that sits BELOW the wrapped - /// disk; the directly-wrapped disk keeps recording (its queue is drained by the backup killer). - std::atomic_bool record_removals{true}; }; class MetadataStorageFromCacheObjectStorageTransaction : public IMetadataTransaction diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/Local/MetadataStorageFromDisk.cpp b/src/Disks/DiskObjectStorage/MetadataStorages/Local/MetadataStorageFromDisk.cpp index 5ed6ec6d936c..b2289e19b405 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/Local/MetadataStorageFromDisk.cpp +++ b/src/Disks/DiskObjectStorage/MetadataStorages/Local/MetadataStorageFromDisk.cpp @@ -185,10 +185,6 @@ void MetadataStorageFromDiskTransaction::commit(const TransactionCommitOptionsVa operations.finalize(); - /// Skip enqueuing when a backup layer owns deletion for this disk (see `setRecordRemovals`): - /// the queue would have no drainer and grow unbounded. `objects_to_remove` (transaction-local) - /// is still returned by `getSubmittedForRemovalBlobs`, so the layer above still soft-deletes. - if (metadata_storage.record_removals.load(std::memory_order_relaxed)) { std::lock_guard guard(metadata_storage.removed_objects_mutex); metadata_storage.objects_to_remove.submitForRemoval(objects_to_remove); diff --git a/src/Disks/DiskObjectStorage/MetadataStorages/Local/MetadataStorageFromDisk.h b/src/Disks/DiskObjectStorage/MetadataStorages/Local/MetadataStorageFromDisk.h index 05223643f1c9..af562ac5e672 100644 --- a/src/Disks/DiskObjectStorage/MetadataStorages/Local/MetadataStorageFromDisk.h +++ b/src/Disks/DiskObjectStorage/MetadataStorages/Local/MetadataStorageFromDisk.h @@ -33,10 +33,6 @@ class MetadataStorageFromDisk final : public IMetadataStorage mutable std::mutex removed_objects_mutex; InMemoryRemovalQueue objects_to_remove TSA_GUARDED_BY(removed_objects_mutex); - /// When false, transaction commits skip enqueuing orphaned blobs into `objects_to_remove` - /// (see `setRecordRemovals`). Set once by a backup layer at wrap time; never re-enabled. - std::atomic_bool record_removals{true}; - public: MetadataStorageFromDisk(DiskPtr disk_, String compatible_key_prefix_, ObjectStorageKeyGeneratorPtr key_generator_); @@ -94,11 +90,6 @@ class MetadataStorageFromDisk final : public IMetadataStorage BlobsToRemove getBlobsToRemove(const ClusterConfigurationPtr & cluster, int64_t max_count) override; int64_t recordAsRemoved(const StoredObjects & blobs) override; bool hasPendingRemovalBlobs(const StoredObjects & blobs) const override; - - /// Stop enqueuing orphaned blobs into `objects_to_remove` at commit time. Non-virtual on purpose - /// (reached via `dynamic_cast` from `DiskObjectStorage::wrapWithBackup`) so it adds no slot to - /// the `IMetadataStorage` vtable. See that call site and `record_removals` for the rationale. - void setRecordRemovals(bool value) { record_removals.store(value, std::memory_order_relaxed); } }; class MetadataStorageFromDiskTransaction final : public IMetadataTransaction diff --git a/src/Disks/DiskObjectStorage/ObjectStorages/Backup/registerDiskBackup.cpp b/src/Disks/DiskObjectStorage/ObjectStorages/Backup/registerDiskBackup.cpp deleted file mode 100644 index 37ec56c49a71..000000000000 --- a/src/Disks/DiskObjectStorage/ObjectStorages/Backup/registerDiskBackup.cpp +++ /dev/null @@ -1,67 +0,0 @@ -#include -#include -#include -#include -#include -#include -#include - -namespace DB -{ - -namespace ErrorCodes -{ - extern const int BAD_ARGUMENTS; -} - -void registerDiskBackup(DiskFactory & factory, bool global_skip_access_check) -{ - auto creator = [global_skip_access_check](const String & name, - const Poco::Util::AbstractConfiguration & config, - const String & config_prefix, - ContextPtr context, - const DisksMap & map, - bool /* attach */, - bool /* custom_disk */) -> DiskPtr - { - const bool skip_access_check = global_skip_access_check || config.getBool(config_prefix + ".skip_access_check", false); - - auto disk_name = config.getString(config_prefix + ".disk", ""); - if (disk_name.empty()) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Disk Backup requires `disk` field in config"); - - auto disk_it = map.find(disk_name); - if (disk_it == map.end()) - { - throw Exception( - ErrorCodes::BAD_ARGUMENTS, - "Cannot wrap disk `{}` with backup layer `{}`: there is no such disk (it should be initialized before backup disk)", - disk_name, name); - } - - auto disk = disk_it->second; - if (!dynamic_cast(disk.get())) - throw Exception( - ErrorCodes::BAD_ARGUMENTS, - "Cannot wrap disk `{}` with backup layer `{}`: backup disk is allowed only on top of object storage", - disk_name, name); - - auto backup_base_path = config.getString(config_prefix + ".path", fs::path(context->getPath()) / "disks" / name / "backup/"); - if (!fs::exists(backup_base_path)) - fs::create_directories(backup_base_path); - - auto backup_disk_object_storage = std::dynamic_pointer_cast(disk)->wrapWithBackup(name, backup_base_path); - backup_disk_object_storage->startup(skip_access_check); - - LOG_INFO( - getLogger("DiskBackup"), - "Registered backup disk (`{}`) with structure: {}", - name, assert_cast(backup_disk_object_storage.get())->getStructure()); - - return backup_disk_object_storage; - }; - - factory.registerDiskType("backup", creator); -} - -} diff --git a/src/Disks/DiskObjectStorage/ObjectStorages/Backup/BackupObjectStorage.cpp b/src/Disks/DiskObjectStorage/ObjectStorages/SoftDelete/SoftDeleteObjectStorage.cpp similarity index 70% rename from src/Disks/DiskObjectStorage/ObjectStorages/Backup/BackupObjectStorage.cpp rename to src/Disks/DiskObjectStorage/ObjectStorages/SoftDelete/SoftDeleteObjectStorage.cpp index 83c789c0ea5a..d3f30180cc7d 100644 --- a/src/Disks/DiskObjectStorage/ObjectStorages/Backup/BackupObjectStorage.cpp +++ b/src/Disks/DiskObjectStorage/ObjectStorages/SoftDelete/SoftDeleteObjectStorage.cpp @@ -1,4 +1,4 @@ -#include "BackupObjectStorage.h" +#include "SoftDeleteObjectStorage.h" #include #include @@ -18,22 +18,22 @@ namespace ErrorCodes extern const int CANNOT_CREATE_FILE; } -BackupObjectStorage::BackupObjectStorage( - const ObjectStoragePtr & object_storage_, const std::string & backup_base_path_, const std::string & backup_config_name_) +SoftDeleteObjectStorage::SoftDeleteObjectStorage( + const ObjectStoragePtr & object_storage_, const std::string & markers_path_, const std::string & disk_name_) : object_storage(object_storage_) - , backup_base_path(backup_base_path_) - , backup_config_name(backup_config_name_) + , markers_path(markers_path_) + , disk_name(disk_name_) , log(getLogger(getName())) { } -void BackupObjectStorage::removeObjectIfExists(const StoredObject & object) +void SoftDeleteObjectStorage::removeObjectIfExists(const StoredObject & object) { LOG_DEBUG(log, "removeObjectIfExists: {} -> {}", object.remote_path, object.local_path); removeObjectImpl(object.remote_path); } -void BackupObjectStorage::removeObjectsIfExist(const StoredObjects & objects) +void SoftDeleteObjectStorage::removeObjectsIfExist(const StoredObjects & objects) { for (const auto & object : objects) { @@ -42,12 +42,12 @@ void BackupObjectStorage::removeObjectsIfExist(const StoredObjects & objects) } } -bool BackupObjectStorage::exists(const StoredObject & object) const +bool SoftDeleteObjectStorage::exists(const StoredObject & object) const { return !isSoftDeleted(object.remote_path) && object_storage->exists(object); } -void BackupObjectStorage::listObjects(const std::string & path, RelativePathsWithMetadata & children, size_t max_keys) const +void SoftDeleteObjectStorage::listObjects(const std::string & path, RelativePathsWithMetadata & children, size_t max_keys) const { RelativePathsWithMetadata all_children; object_storage->listObjects(path, all_children, max_keys); @@ -60,7 +60,7 @@ void BackupObjectStorage::listObjects(const std::string & path, RelativePathsWit } } -ObjectStorageIteratorPtr BackupObjectStorage::iterate( +ObjectStorageIteratorPtr SoftDeleteObjectStorage::iterate( const std::string & path_prefix, size_t max_keys, bool /* with_tags */, @@ -72,17 +72,17 @@ ObjectStorageIteratorPtr BackupObjectStorage::iterate( return std::make_shared(std::move(children)); } -bool BackupObjectStorage::isSoftDeleted(const std::string & object_path) const +bool SoftDeleteObjectStorage::isSoftDeleted(const std::string & object_path) const { return FS::exists(getRemovedMarkerPath(object_path)); } -std::string BackupObjectStorage::getRemovedMarkerPath(const std::string & object_path) const +std::string SoftDeleteObjectStorage::getRemovedMarkerPath(const std::string & object_path) const { - return fs::path(backup_base_path) / escapeForFileName(object_path); + return fs::path(markers_path) / escapeForFileName(object_path); } -void BackupObjectStorage::removeObjectImpl(const std::string & object_path) const +void SoftDeleteObjectStorage::removeObjectImpl(const std::string & object_path) const { const std::string removed_marker_path = getRemovedMarkerPath(object_path); LOG_DEBUG(log, "adding removed marker: {}", removed_marker_path); diff --git a/src/Disks/DiskObjectStorage/ObjectStorages/Backup/BackupObjectStorage.h b/src/Disks/DiskObjectStorage/ObjectStorages/SoftDelete/SoftDeleteObjectStorage.h similarity index 93% rename from src/Disks/DiskObjectStorage/ObjectStorages/Backup/BackupObjectStorage.h rename to src/Disks/DiskObjectStorage/ObjectStorages/SoftDelete/SoftDeleteObjectStorage.h index f06e813081e8..685c5b36fe32 100644 --- a/src/Disks/DiskObjectStorage/ObjectStorages/Backup/BackupObjectStorage.h +++ b/src/Disks/DiskObjectStorage/ObjectStorages/SoftDelete/SoftDeleteObjectStorage.h @@ -12,13 +12,12 @@ namespace DB /// removing objects, it writes a local deletion-marker file per object so that an /// external Aiven backup/GC system can decide when to physically delete. Reads and /// listings filter out soft-deleted objects so the table view stays consistent. -class BackupObjectStorage final : public IObjectStorage +class SoftDeleteObjectStorage final : public IObjectStorage { public: - BackupObjectStorage( - const ObjectStoragePtr & object_storage_, const std::string & backup_base_path_, const std::string & backup_config_name_); + SoftDeleteObjectStorage(const ObjectStoragePtr & object_storage_, const std::string & markers_path_, const std::string & disk_name_); - std::string getName() const override { return fmt::format("BackupObjectStorage-{}({})", backup_config_name, object_storage->getName()); } + std::string getName() const override { return fmt::format("SoftDeleteObjectStorage-{}({})", disk_name, object_storage->getName()); } ObjectStorageType getType() const override { return object_storage->getType(); } @@ -158,8 +157,8 @@ class BackupObjectStorage final : public IObjectStorage void removeObjectImpl(const std::string & object_path) const; ObjectStoragePtr object_storage; - std::string backup_base_path; - std::string backup_config_name; + std::string markers_path; + std::string disk_name; LoggerPtr log; }; diff --git a/src/Disks/DiskObjectStorage/RegisterDiskObjectStorage.cpp b/src/Disks/DiskObjectStorage/RegisterDiskObjectStorage.cpp index e4c77719d8dc..62c067fa840b 100644 --- a/src/Disks/DiskObjectStorage/RegisterDiskObjectStorage.cpp +++ b/src/Disks/DiskObjectStorage/RegisterDiskObjectStorage.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -6,15 +7,50 @@ #include #include #include +#include +#include #include namespace DB { +namespace ErrorCodes +{ + extern const int BAD_ARGUMENTS; +} + void registerObjectStorages(); void registerMetadataStorages(); +namespace +{ + +ObjectStoragePtr wrapIfSoftDelete( + ObjectStoragePtr object_storage, + const String & disk_name, + const Poco::Util::AbstractConfiguration & config, + const String & config_prefix, + const ContextPtr & context) +{ + if (!config.getBool(config_prefix + ".soft_delete", false)) + return object_storage; + + auto markers_path = config.getString( + config_prefix + ".soft_delete_markers_path", + fs::path(context->getPath()) / "disks" / disk_name / "soft_deleted/"); + fs::create_directories(markers_path); + + LOG_INFO( + getLogger("registerDiskObjectStorage"), + "Disk `{}`: soft delete enabled, blob removals are recorded as markers under {} instead of deleting the blob", + disk_name, markers_path); + + return std::make_shared(std::move(object_storage), markers_path, disk_name); +} + +} + void registerDiskObjectStorage(DiskFactory & factory, bool global_skip_access_check) { registerObjectStorages(); @@ -38,20 +74,28 @@ void registerDiskObjectStorage(DiskFactory & factory, bool global_skip_access_ch config.keys(config_prefix + ".locations", locations); LOG_DEBUG(getLogger("registerDiskObjectStorage"), "Configuring DiskObjectStorage with multiple locations: [{}]", fmt::join(locations, ", ")); + /// Blob replication between locations has no notion of a soft-deleted object, so a marker + /// written at one location would not stop the others from resurrecting or dropping the blob. + if (locations.size() > 1 && config.getBool(config_prefix + ".soft_delete", false)) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Disk `{}`: `soft_delete` is only supported on single-location object storage disks, but {} locations are configured", + name, locations.size()); + for (const auto & location : locations) { const std::string object_storage_config_prefix = config_prefix + ".locations." + location; const bool local = config.getBool(object_storage_config_prefix + ".local"); const bool enabled = config.getBool(object_storage_config_prefix + ".enabled"); - const ObjectStoragePtr object_storage = ObjectStorageFactory::instance().create(fmt::format("{}.{}", name, location), config, object_storage_config_prefix, context, /*skip_access_check=*/skip_access_check || !enabled); - object_storage_registry[location] = object_storage; + ObjectStoragePtr object_storage = ObjectStorageFactory::instance().create(fmt::format("{}.{}", name, location), config, object_storage_config_prefix, context, /*skip_access_check=*/skip_access_check || !enabled); + object_storage_registry[location] = wrapIfSoftDelete(std::move(object_storage), name, config, config_prefix, context); cluster_registry[location] = {enabled, local, object_storage_config_prefix}; } } else { - const ObjectStoragePtr object_storage = ObjectStorageFactory::instance().create(name, config, config_prefix, context, skip_access_check); - object_storage_registry["main"] = object_storage; + ObjectStoragePtr object_storage = ObjectStorageFactory::instance().create(name, config, config_prefix, context, skip_access_check); + object_storage_registry["main"] = wrapIfSoftDelete(std::move(object_storage), name, config, config_prefix, context); cluster_registry["main"] = { .enabled = true, .local = true, .config_prefix = config_prefix }; } @@ -99,21 +143,29 @@ void registerDiskObjectStorage(DiskFactory & factory, bool global_skip_access_ch return disk; }; - factory.registerDiskType("object_storage", creator); + /// The creator above is the only one that reads `soft_delete`, so every type it backs is + /// registered as soft-delete capable and everything else is rejected by `DiskFactory::create`. + auto register_type = [&](const String & disk_type) + { + factory.registerDiskType(disk_type, creator); + factory.markSoftDeleteCapable(disk_type); + }; + + register_type("object_storage"); #if USE_AWS_S3 - factory.registerDiskType("s3", creator); /// For compatibility - factory.registerDiskType("s3_plain", creator); /// For compatibility - factory.registerDiskType("s3_with_keeper", creator); /// For compatibility - factory.registerDiskType("s3_plain_rewritable", creator); // For compatibility + register_type("s3"); /// For compatibility + register_type("s3_plain"); /// For compatibility + register_type("s3_with_keeper"); /// For compatibility + register_type("s3_plain_rewritable"); // For compatibility #endif #if USE_HDFS - factory.registerDiskType("hdfs", creator); /// For compatibility + register_type("hdfs"); /// For compatibility #endif #if USE_AZURE_BLOB_STORAGE - factory.registerDiskType("azure_blob_storage", creator); /// For compatibility + register_type("azure_blob_storage"); /// For compatibility #endif - factory.registerDiskType("local_blob_storage", creator); /// For compatibility - factory.registerDiskType("web", creator); /// For compatibility + register_type("local_blob_storage"); /// For compatibility + register_type("web"); /// For compatibility } } diff --git a/src/Disks/DiskObjectStorage/Replication/BlobKillerThread.cpp b/src/Disks/DiskObjectStorage/Replication/BlobKillerThread.cpp index 03677d21432c..fd7b47013e64 100644 --- a/src/Disks/DiskObjectStorage/Replication/BlobKillerThread.cpp +++ b/src/Disks/DiskObjectStorage/Replication/BlobKillerThread.cpp @@ -269,20 +269,6 @@ void BlobKillerThread::shutdown() task->deactivate(); - if (force_disabled) - { - /// A killer disabled by a backup layer must NOT physically unlink blobs. The final cleanup - /// below drains the WHOLE queue (max_to_remove=0) through this disk's RAW object storage; for - /// a wrapped disk that would physically delete blobs the backup layer only soft-deleted - /// (data loss). Suppression keeps this disk's own queue empty, but a wrapped disk may share - /// its `metadata_storage` (hence removal queue) with the backup disk — draining it here would - /// also steal entries the backup killer still has to mark. Skip: the backup killer flushes - /// that shared queue at its own shutdown, and any purely in-memory remainder is dropped with - /// the process without deleting anything. - LOG_INFO(log, "Skipping final cleanup: killer is disabled by a backup layer"); - return; - } - /// We need to execute it here explicitly because some blobs may be in the metadata storage queue. executeBlobsCleanup(/*max_to_remove=*/0, max_blobs_in_task.load(), remove_tasks_runner, cluster, metadata_storage, object_storages, log); } @@ -320,29 +306,9 @@ void BlobKillerThread::triggerAndWait() waitRound(expected_round); } -void BlobKillerThread::detachWrapped() -{ - wrapped_blob_killer = nullptr; -} - -void BlobKillerThread::disable() -{ - /// Called by a backup layer that takes over deletion for this disk (see - /// `DiskObjectStorage::wrapWithBackup`). `force_disabled` is sticky so a later - /// `SYSTEM RELOAD CONFIG` cannot re-enable the killer from config and physically unlink blobs - /// the backup layer only soft-deleted. The disk's removal queue is separately kept empty at the - /// source (`setRecordRemovals` on the concrete metadata storage), so there is nothing to drain. - force_disabled = true; - enabled = false; - task->deactivate(); -} - void BlobKillerThread::applyNewSettings(const Poco::Util::AbstractConfiguration & config, const std::string & config_prefix) { - /// A killer disabled by a backup layer (see `disable`) must stay disabled across config reloads: - /// otherwise the config default (enabled=true) would resurrect it and it would physically unlink - /// blobs the backup layer only soft-deleted. - enabled = force_disabled ? false : config.getBool(config_prefix + ".enabled", true); + enabled = config.getBool(config_prefix + ".enabled", true); reschedule_interval_sec = config.getUInt64(config_prefix + ".interval_sec", DEFAULT_RESCHEDULE_INTERVAL_SEC); metadata_request_batch = config.getUInt64(config_prefix + ".metadata_request_size", DEFAULT_METADATA_REQUEST_SIZE); max_blobs_in_task = std::clamp(config.getUInt64(config_prefix + ".max_blobs_in_task", DEFAULT_MAX_BLOBS_IN_TASK), 1, BLOBS_IN_TASK_HARDWARE_LIMIT); diff --git a/src/Disks/DiskObjectStorage/Replication/BlobKillerThread.h b/src/Disks/DiskObjectStorage/Replication/BlobKillerThread.h index 53296e292774..557448add2c8 100644 --- a/src/Disks/DiskObjectStorage/Replication/BlobKillerThread.h +++ b/src/Disks/DiskObjectStorage/Replication/BlobKillerThread.h @@ -30,33 +30,16 @@ class BlobKillerThread void triggerAndWait(); void applyNewSettings(const Poco::Util::AbstractConfiguration & config, const std::string & config_prefix); - /// Detach the wrapped (inner) killer so triggerAndWait stops chaining into it. - /// Used by the backup disk layer: the wrapped disk's killer routes blob removals to the - /// raw object storage (physical unlink), which must NOT run for a backup-wrapped disk. - void detachWrapped(); - - /// Permanently stop and disable this killer, STICKILY: a subsequent `applyNewSettings` (config - /// reload) will NOT resurrect it from the config default. Used to silence a wrapped disk's killer - /// once a backup layer takes over as the sole (soft-)deleter. The wrapped disk's removal queue is - /// separately kept empty at the source via `setRecordRemovals` on the concrete metadata storage, - /// so a disabled killer has nothing to drain. Idempotent and safe to call before/after startup - /// (the task is created in the constructor). - void disable(); - private: const std::string disk_name; const ClusterConfigurationPtr cluster; const MetadataStoragePtr metadata_storage; const ObjectStorageRouterPtr object_storages; - /// Not const: detachWrapped resets it to nullptr to break the killer chain (see above). - std::shared_ptr wrapped_blob_killer; + const std::shared_ptr wrapped_blob_killer; const LoggerPtr log; std::atomic started{false}; std::atomic enabled{true}; - /// Set by `disable()`. Once true this killer stays disabled across config reloads and skips the - /// shutdown final-cleanup, so it never physically unlinks blobs a backup layer soft-deleted. - std::atomic force_disabled{false}; std::atomic finished_rounds{0}; std::atomic reschedule_interval_sec{0}; std::atomic metadata_request_batch{0}; diff --git a/src/Disks/registerDisks.cpp b/src/Disks/registerDisks.cpp index 70955ffe7487..b8ce5530213e 100644 --- a/src/Disks/registerDisks.cpp +++ b/src/Disks/registerDisks.cpp @@ -15,8 +15,6 @@ void registerDiskLocal(DiskFactory & factory, bool global_skip_access_check); void registerDiskEncrypted(DiskFactory & factory, bool global_skip_access_check); #endif -void registerDiskBackup(DiskFactory & factory, bool global_skip_access_check); - void registerDiskCache(DiskFactory & factory, bool global_skip_access_check); void registerDiskObjectStorage(DiskFactory & factory, bool global_skip_access_check); @@ -31,8 +29,6 @@ void registerDisks(bool global_skip_access_check) registerDiskEncrypted(factory, global_skip_access_check); #endif - registerDiskBackup(factory, global_skip_access_check); - registerDiskCache(factory, global_skip_access_check); registerDiskObjectStorage(factory, global_skip_access_check); diff --git a/src/Disks/tests/gtest_metadata_local_disk.cpp b/src/Disks/tests/gtest_metadata_local_disk.cpp index a8cdef50b3f8..0dbdaba7521c 100644 --- a/src/Disks/tests/gtest_metadata_local_disk.cpp +++ b/src/Disks/tests/gtest_metadata_local_disk.cpp @@ -1465,9 +1465,9 @@ TEST_F(MetadataLocalDiskTest, TestNonExistingObjectsInTransaction) } } -TEST_F(MetadataLocalDiskTest, TestRecordRemovalsEnqueuesByDefault) +TEST_F(MetadataLocalDiskTest, TestUnlinkEnqueuesBlobForRemoval) { - auto metadata = getMetadataStorage("/TestRecordRemovalsEnqueuesByDefault"); + auto metadata = getMetadataStorage("/TestUnlinkEnqueuesBlobForRemoval"); { auto tx = metadata->createTransaction(); tx->createMetadataFile("f", {DB::StoredObject("blob-default", "f", 1)}); @@ -1479,40 +1479,7 @@ TEST_F(MetadataLocalDiskTest, TestRecordRemovalsEnqueuesByDefault) tx->commit(DB::NoCommitOptions{}); } - /// Control: with recording ON (the default) an orphaned blob is enqueued for deferred removal so - /// the disk's own BlobKillerThread can physically unlink it later. + /// An orphaned blob is enqueued for deferred removal so the disk's BlobKillerThread can + /// physically unlink it later. verifyBlobsToRemove(metadata, {"blob-default"}); } - -TEST_F(MetadataLocalDiskTest, TestSetRecordRemovalsSuppressesEnqueue) -{ - /// Models a disk sitting BELOW a backup layer (see DiskObjectStorage::wrapWithBackup): its killer - /// is disabled and nobody drains its removal queue, so it must not enqueue at all (the invariant - /// that keeps memory bounded) — yet the transaction-local removal list must still carry the blob - /// so the overlying backup layer, which reads getSubmittedForRemovalBlobs, still soft-deletes it. - auto metadata = getMetadataStorage("/TestSetRecordRemovalsSuppressesEnqueue"); - auto * from_disk = dynamic_cast(metadata.get()); - ASSERT_NE(from_disk, nullptr); - - from_disk->setRecordRemovals(false); - - { - auto tx = metadata->createTransaction(); - tx->createMetadataFile("f", {DB::StoredObject("blob-suppressed", "f", 1)}); - tx->commit(DB::NoCommitOptions{}); - } - - DB::MetadataTransactionPtr remove_tx = metadata->createTransaction(); - remove_tx->unlinkFile("f", /*if_exists=*/false, /*should_remove_objects=*/true); - remove_tx->commit(DB::NoCommitOptions{}); - - /// Never enqueued: the background removal queue stays empty regardless of scheduling rounds. - verifyBlobsToRemove(metadata, {}); - - /// The transaction-local removal list, however, still carries the blob (so soft-delete markers - /// are still produced upstream). This is what makes source-suppression safe for the backup stack. - std::set submitted_paths; - for (const auto & blob : remove_tx->getSubmittedForRemovalBlobs()) - submitted_paths.insert(blob.remote_path); - EXPECT_EQ(submitted_paths, (std::set{"blob-suppressed"})); -} diff --git a/tests/integration/test_aiven_backup_disk_cache_layer_reload/__init__.py b/tests/integration/test_aiven_backup_disk_cache_layer_reload/__init__.py deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/tests/integration/test_aiven_backup_disk_cache_layer_reload/config.d/reload_storage.xml b/tests/integration/test_aiven_backup_disk_cache_layer_reload/config.d/reload_storage.xml deleted file mode 100644 index 32cd57a2ec4a..000000000000 --- a/tests/integration/test_aiven_backup_disk_cache_layer_reload/config.d/reload_storage.xml +++ /dev/null @@ -1,36 +0,0 @@ - - - - - - object_storage - local - /var/lib/clickhouse/bc_reload_obj/ - - - cache - bc_base - bc_reload_cache - 1073741824 - - - backup - bc_cache - /var/lib/clickhouse/bc_reload_markers/ - - - - - -
- bc_backup -
- - - - - diff --git a/tests/integration/test_aiven_backup_disk_cache_layer_reload/test.py b/tests/integration/test_aiven_backup_disk_cache_layer_reload/test.py deleted file mode 100644 index 8ba1cb044293..000000000000 --- a/tests/integration/test_aiven_backup_disk_cache_layer_reload/test.py +++ /dev/null @@ -1,164 +0,0 @@ -"""Regression test for the STICKY-disable half of the tiered-storage blob-deletion fix -(the `force_disabled` flag in `BlobKillerThread`). - -Companion to `test_aiven_backup_disk_cache_layer` (which covers the transitive-disable half). -That sibling uses an inline SQL `disk(type=backup, disk=disk(type=cache, ...))`; the wrapped -layers of an inline disk are internal (`__tmp_internal_...`) and are NOT re-processed by -`SYSTEM RELOAD CONFIG`, so an inline disk cannot exercise the reload path. - -This test therefore uses a CONFIG-DEFINED stack (see config.d/reload_storage.xml): -backup(`bc_backup`) -> cache(`bc_cache`) -> object_storage(`bc_base`, local). On reload, -`DiskSelector::updateFromConfig` reuses each disk by name and calls -`DiskObjectStorage::applyNewSettings` IN PLACE (it does NOT reconstruct the disk, i.e. -`wrapWithBackup` does not re-run). `applyNewSettings` re-reads `data_background_cleanup.enabled` -(default true), so without the fix it re-enables the base/cache `BlobKillerThread`s that -`wrapWithBackup` disabled at construction — and the next removal physically unlinks the blobs -the backup layer only soft-deleted. - -`object_storage_type = local` stands in for S3 (identical `removeObjectsIfExist` path), so the -"bucket" is inspectable as plain files. - -EXPECTED RESULT - * Without the sticky-disable fix: after `SYSTEM RELOAD CONFIG` the base killer is live again, - so the DROP's soft-deleted base blobs are physically unlinked -> the survival assertion FAILs. - * With the fix (`disable()` sets `force_disabled`, honored by `applyNewSettings`): the killers - stay disabled across the reload -> the blobs survive -> PASS. - -The complementary "queue is never populated for a suppressed disk" invariant (which keeps memory -bounded) is asserted directly and deterministically by the `gtest_metadata_local_disk` unit tests -`TestSetRecordRemovalsSuppressesEnqueue` / `TestRecordRemovalsEnqueuesByDefault`. -""" - -import time - -import pytest - -from helpers.cluster import ClickHouseCluster - -cluster = ClickHouseCluster(__file__) - -node = cluster.add_instance( - "node", - main_configs=["config.d/reload_storage.xml"], - stay_alive=True, -) - -# Must match config.d/reload_storage.xml. For `object_storage_type = local` the blob -# `remote_path` is the absolute on-node path under OBJ_DIR, so it is directly comparable to a -# filesystem listing; the backup layer writes one deletion marker per removed key under MARKER_DIR. -OBJ_DIR = "/var/lib/clickhouse/bc_reload_obj" -MARKER_DIR = "/var/lib/clickhouse/bc_reload_markers" - -# In-container path of the config file shipped via main_configs (copied into config.d). -RELOAD_CONFIG_PATH = "/etc/clickhouse-server/config.d/reload_storage.xml" - - -@pytest.fixture(scope="module") -def start_cluster(): - try: - cluster.start() - yield cluster - finally: - cluster.shutdown() - - -def _list_files(path): - out = node.exec_in_container( - ["bash", "-c", f"find {path} -type f 2>/dev/null | sort"] - ).strip() - return set(line for line in out.splitlines() if line) - - -def _unescape_for_file_name(name): - """Mirror of DB::unescapeForFileName: turn %HH back into the raw byte.""" - res = [] - i = 0 - while i < len(name): - if name[i] == "%" and i + 2 < len(name): - res.append(chr(int(name[i + 1 : i + 3], 16))) - i += 3 - else: - res.append(name[i]) - i += 1 - return "".join(res) - - -def _marked_objects_under(marker_dir): - """Object paths that currently have a deletion marker under `marker_dir`.""" - return { - _unescape_for_file_name(m.rsplit("/", 1)[-1]) - for m in _list_files(marker_dir) - } - - -def _referenced_remote_paths(obj_dir): - """Object keys under `obj_dir` still referenced by some part's metadata.""" - out = node.query( - "SELECT remote_path FROM system.remote_data_paths " - f"WHERE remote_path LIKE '{obj_dir}/%'" - ).strip() - return set(line for line in out.splitlines() if line) - - -def _assert_blobs_survive(removed, obj_dir, marker_dir, path_label): - """Require `removed` non-empty, gate on the backup disk recording deletion markers for every - removed key (proves a killer drained the shared queue, so a survival pass is real), then - assert those blobs PHYSICALLY survive under the base object storage.""" - assert removed, f"the {path_label} path orphaned no base blobs; nothing was exercised" - - marked = set() - for _ in range(60): - marked = _marked_objects_under(marker_dir) - if removed.issubset(marked): - break - time.sleep(1) - unmarked = sorted(removed - marked) - assert not unmarked, ( - f"backup disk did not record deletion markers for the {path_label} removed blobs " - f"(killer never drained the queue?); unmarked: {unmarked}" - ) - - # Give a (mis)behaving base killer several scheduling rounds to unlink before asserting. - surviving = set() - for _ in range(15): - surviving = _list_files(obj_dir) - if not removed.issubset(surviving): - break - time.sleep(1) - - missing = sorted(removed - surviving) - assert not missing, ( - f"backup-wrapped object-storage blobs orphaned by the {path_label} path were physically " - "deleted despite the soft-delete markers: SYSTEM RELOAD CONFIG re-enabled a wrapped-disk " - f"BlobKillerThread that disable() should have kept sticky; missing blobs: {missing}" - ) - - -def test_backup_cache_disk_survives_config_reload(start_cluster): - node.query("DROP TABLE IF EXISTS bc_reload_tbl SYNC") - node.query( - """ - CREATE TABLE bc_reload_tbl (a UInt64) - ENGINE = MergeTree - ORDER BY a - SETTINGS storage_policy = 'bc_reload' - """ - ) - node.query("INSERT INTO bc_reload_tbl SELECT number FROM numbers(1000)") - - # All blobs backing the table; the drop below orphans them. - removed = _referenced_remote_paths(OBJ_DIR) - assert removed, "expected object-storage blobs after insert" - - # Force a config reload that re-applies disk settings IN PLACE. Bumping the cache max_size is a - # benign delta that guarantees DiskSelector re-processes the stack (and thus calls - # applyNewSettings on bc_base / bc_cache / bc_backup); without the sticky-disable fix this - # re-enables the base/cache killers that wrapWithBackup disabled. - node.replace_in_config(RELOAD_CONFIG_PATH, "1073741824", "2147483648") - node.query("SYSTEM RELOAD CONFIG") - - # Remove the blobs. Only the backup disk's killer may drain (soft-delete via markers); the - # base/cache killers must remain disabled across the reload. - node.query("DROP TABLE bc_reload_tbl SYNC") - - _assert_blobs_survive(removed, OBJ_DIR, MARKER_DIR, "drop-after-reload") diff --git a/tests/integration/test_aiven_backup_disk/__init__.py b/tests/integration/test_aiven_soft_delete/__init__.py similarity index 100% rename from tests/integration/test_aiven_backup_disk/__init__.py rename to tests/integration/test_aiven_soft_delete/__init__.py diff --git a/tests/integration/test_aiven_backup_disk/test.py b/tests/integration/test_aiven_soft_delete/test.py similarity index 65% rename from tests/integration/test_aiven_backup_disk/test.py rename to tests/integration/test_aiven_soft_delete/test.py index 5582656c70a7..089a9918acb5 100644 --- a/tests/integration/test_aiven_backup_disk/test.py +++ b/tests/integration/test_aiven_soft_delete/test.py @@ -1,10 +1,10 @@ -"""Integration test for the Aiven `backup` disk type (patch 026). +"""Integration test for the Aiven `soft_delete` object-storage disk setting. -The `backup` disk wraps an object-storage disk and turns object deletion into a -*soft delete*: instead of (only) removing the underlying object it writes a local +With `soft_delete = 1` an object-storage disk turns object deletion into a +*soft delete*: instead of removing the underlying object it writes a local deletion-marker file per object under -`/`. An external Aiven GC later -decides what is safe to physically remove. +`/`. An external Aiven +GC later decides what is safe to physically remove. We exercise the storage-agnostic soft-delete bookkeeping our single-location (tiered-storage) deployment relies on, using a local-filesystem object storage @@ -12,34 +12,22 @@ directly inspectable inside the node container. Shape: a single node, no ZooKeeper, with an inline SQL custom disk -(`disk(type=backup, disk=disk(type=object_storage, object_storage_type=local, ...), ...)`). -The inline form lets the server start on both the pre- and post-patch binary; the -divergence is captured at `CREATE TABLE` time: - - * post-patch: the `backup` disk type exists, the table is created, and removing - its objects (here via `DROP TABLE ... SYNC`) makes `BackupObjectStorage` write - deletion-marker files under the backup base path, one per removed object key. - * pre-patch: `disk(type=backup, ...)` is an unknown disk type, so `CREATE TABLE` - throws `unknown disk type: backup` and the test fails. - -The load-bearing invariant (Fix A): the soft-deleted blobs must PHYSICALLY SURVIVE -the drop. On 26.3 blob deletion is deferred through a single in-memory removal +(`disk(type=object_storage, object_storage_type=local, soft_delete=1, ...)`). + +The load-bearing invariant: the soft-deleted blobs must PHYSICALLY SURVIVE the +removal. On 26.3 blob deletion is deferred through a single in-memory removal queue owned by `metadata_storage`, drained by a per-disk background -`BlobKillerThread`. Because the backup layer reuses the same `metadata_storage`, -the backup disk and the wrapped/inner disk share that one queue. `wrapWithBackup` -must make the backup disk's killer the SOLE drainer: it unchains the inner killer -(`detachWrapped`) and disables the inner disk's own killer (`disable`). Otherwise -the inner killer routes to the RAW object storage and physically unlinks the blob, -defeating the marker and the external-GC contract. +`BlobKillerThread`. `soft_delete` wraps the object storage in +`SoftDeleteObjectStorage` at construction time, before the router, the metadata +storage, the killer and the copier are built from it, so every component that +could delete a blob routes through the soft-delete layer and no component holds +the undecorated object storage. This test therefore asserts BOTH: - * the backup disk records per-object deletion markers (soft-delete bookkeeping), and - * the marked (soft-deleted) blobs still physically exist under the wrapped - object storage after the drop and after the background killer has run. - -The second assertion FAILs without Fix A (inner killer physically deletes the -blob) and PASSes with it — see the dossier (§4) for the evidence pair. + * the disk records per-object deletion markers (soft-delete bookkeeping), and + * the marked (soft-deleted) blobs still physically exist under the object + storage after the removal and after the background killer has run. """ import time @@ -54,19 +42,19 @@ # Both directories live inside the node container (under the server data dir), # so they are inspectable via exec_in_container. -OBJ_DIR = "/var/lib/clickhouse/aiven_backup_obj" -MARKER_DIR = "/var/lib/clickhouse/aiven_backup_markers" +OBJ_DIR = "/var/lib/clickhouse/aiven_soft_delete_obj" +MARKER_DIR = "/var/lib/clickhouse/aiven_soft_delete_markers" # The extended removal-path tests (merge / mutation / TTL) each get their OWN # object-storage + marker directory pair so they cannot interfere with each # other or with the DROP test above (the blob/marker files would otherwise mix # in a shared directory and make the per-test survival sets ambiguous). -OBJ_DIR_MERGE = "/var/lib/clickhouse/aiven_backup_obj_merge" -MARKER_DIR_MERGE = "/var/lib/clickhouse/aiven_backup_markers_merge" -OBJ_DIR_MUT = "/var/lib/clickhouse/aiven_backup_obj_mut" -MARKER_DIR_MUT = "/var/lib/clickhouse/aiven_backup_markers_mut" -OBJ_DIR_TTL = "/var/lib/clickhouse/aiven_backup_obj_ttl" -MARKER_DIR_TTL = "/var/lib/clickhouse/aiven_backup_markers_ttl" +OBJ_DIR_MERGE = "/var/lib/clickhouse/aiven_soft_delete_obj_merge" +MARKER_DIR_MERGE = "/var/lib/clickhouse/aiven_soft_delete_markers_merge" +OBJ_DIR_MUT = "/var/lib/clickhouse/aiven_soft_delete_obj_mut" +MARKER_DIR_MUT = "/var/lib/clickhouse/aiven_soft_delete_markers_mut" +OBJ_DIR_TTL = "/var/lib/clickhouse/aiven_soft_delete_obj_ttl" +MARKER_DIR_TTL = "/var/lib/clickhouse/aiven_soft_delete_markers_ttl" @pytest.fixture(scope="module") @@ -104,37 +92,36 @@ def _marked_objects(): return {_unescape_for_file_name(m.rsplit("/", 1)[-1]) for m in _list_files(MARKER_DIR)} -def test_backup_disk_soft_delete(start_cluster): +def test_soft_delete_on_drop(start_cluster): node.query( f""" - CREATE TABLE backup_tbl (a UInt64) + CREATE TABLE soft_delete_tbl (a UInt64) ENGINE = MergeTree ORDER BY a SETTINGS disk = disk( - type = backup, - path = '{MARKER_DIR}/', - disk = disk( - type = object_storage, - object_storage_type = local, - path = '{OBJ_DIR}/')) + type = object_storage, + object_storage_type = local, + path = '{OBJ_DIR}/', + soft_delete = 1, + soft_delete_markers_path = '{MARKER_DIR}/') """ ) - node.query("INSERT INTO backup_tbl SELECT number FROM numbers(1000)") + node.query("INSERT INTO soft_delete_tbl SELECT number FROM numbers(1000)") # The part's blobs are now stored under the wrapped local object storage. obj_files = _list_files(OBJ_DIR) assert obj_files, "expected object-storage blobs after insert" - # Dropping the table removes the part; the backup layer turns each object + # Dropping the table removes the part; the soft-delete layer turns each object # removal into a deletion-marker write. With wait_for_blob_removal (default on) - # this drains synchronously via the backup disk's BlobKillerThread, but we poll + # this drains synchronously via the disk's BlobKillerThread, but we poll # below (test-side, no server-side sleeps) so the assertions stay robust to any # async scheduling. - node.query("DROP TABLE backup_tbl SYNC") + node.query("DROP TABLE soft_delete_tbl SYNC") - # Drain: wait until the backup disk's killer has recorded deletion markers for - # the dropped objects, i.e. it has processed the shared removal queue. + # Drain: wait until the disk's killer has recorded deletion markers for + # the dropped objects, i.e. it has processed the removal queue. marked_under_obj = [] for _ in range(30): marked_under_obj = sorted( @@ -144,20 +131,16 @@ def test_backup_disk_soft_delete(start_cluster): break time.sleep(1) - # (1) The backup disk recorded deletion markers naming real object keys that + # (1) The disk recorded deletion markers naming real object keys that # live under the wrapped object storage. assert ( marked_under_obj ), f"expected deletion-marker files referencing objects under {OBJ_DIR}" - # (2) The soft-deleted blobs MUST physically survive the drop. A backup disk - # never unlinks the blob; it only records a marker and lets the external GC - # decide later. The shared removal queue must be drained ONLY by the backup - # disk's killer (Fix A unchains + disables the inner disk's killer). Without - # Fix A the inner killer routes to the RAW object storage and physically - # unlinks the blob, defeating the marker. Give a (mis)behaving inner killer - # several scheduling rounds to act before asserting, so survival is not a - # mere timing artifact. + # (2) The soft-deleted blobs MUST physically survive the drop. A disk with + # `soft_delete` never unlinks the blob; it only records a marker and lets + # the external GC decide later. Give the background killer several scheduling + # rounds to act before asserting, so survival is not a mere timing artifact. surviving = set() for _ in range(15): surviving = _list_files(OBJ_DIR) @@ -167,9 +150,8 @@ def test_backup_disk_soft_delete(start_cluster): missing = sorted(set(marked_under_obj) - surviving) assert not missing, ( - "backup-wrapped object-storage blobs were physically deleted despite the " - "soft-delete markers (the inner BlobKillerThread defeated the backup " - f"layer); missing blobs: {missing}" + "object-storage blobs were physically deleted despite the soft-delete " + f"markers; missing blobs: {missing}" ) @@ -181,10 +163,10 @@ def test_backup_disk_soft_delete(start_cluster): # through `DiskObjectStorageTransaction` -> the single shared `metadata_storage` # removal queue -> the per-disk `BlobKillerThread` (`DiskObjectStorageTransaction # ::waitBlobRemoval` loops `blob_killer->triggerAndWait()`; the same queue is -# drained by the background killer for async removals). Fix A makes the backup -# disk's killer the SOLE drainer of that queue, so the survival guarantee should -# hold for ALL removal paths, not just DROP. The three tests below prove it for -# merge (OPTIMIZE FINAL), mutation (ALTER ... DELETE) and TTL-delete. +# drained by the background killer for async removals). The killer routes through +# the soft-delete layer, so the survival guarantee should hold for ALL removal +# paths, not just DROP. The three tests below prove it for merge (OPTIMIZE FINAL), +# mutation (ALTER ... DELETE) and TTL-delete. # # Determinism: outdated/source/old parts are normally removed only after # `old_parts_lifetime` (default 480s). Each table sets `old_parts_lifetime = 1` @@ -207,14 +189,13 @@ def test_backup_disk_soft_delete(start_cluster): ) -def _backup_disk_clause(marker_dir, obj_dir): +def _soft_delete_disk_clause(marker_dir, obj_dir): return ( - "disk = disk(" - "type = backup, " - f"path = '{marker_dir}/', " "disk = disk(" "type = object_storage, object_storage_type = local, " - f"path = '{obj_dir}/'))" + f"path = '{obj_dir}/', " + "soft_delete = 1, " + f"soft_delete_markers_path = '{marker_dir}/')" ) @@ -324,12 +305,12 @@ def _assert_blobs_survive(source_blobs, obj_dir, marker_dir, path_label): went through the removal path (`removed`). A blob still referenced after the operation (e.g. a column blob hardlinked into the rewritten part by a mutation) survives trivially via metadata refcount and is NOT evidence for - the backup guarantee, so it is excluded from both the marker gate and the - survival assertion. + the soft-delete guarantee, so it is excluded from both the marker gate and + the survival assertion. We require `removed` to be non-empty (the path really removed blobs), then - wait until the backup killer has recorded deletion markers for every blob in - `removed` (proving the shared removal queue was actually drained for them — + wait until the killer has recorded deletion markers for every blob in + `removed` (proving the removal queue was actually drained for them — without this gate a survival pass could be a mere "the killer never ran" artifact), then assert those blobs PHYSICALLY survive (markers, not unlink). """ @@ -345,10 +326,7 @@ def _assert_blobs_survive(source_blobs, obj_dir, marker_dir, path_label): f"{sorted(source_blobs & referenced_after)}); the removal path was not exercised" ) - # Gate: deletion markers recorded for every removed source key. Pre-fix the - # inner killer ALSO runs (both killers drain the shared queue), so markers - # appear pre-fix too — only the physical-survival assertion below flips, - # which pins the failure to the inner killer's raw unlink. + # Gate: deletion markers recorded for every removed source key. marked = set() for _ in range(60): marked = _marked_objects_under(marker_dir) @@ -357,12 +335,12 @@ def _assert_blobs_survive(source_blobs, obj_dir, marker_dir, path_label): time.sleep(1) unmarked = sorted(removed - marked) assert not unmarked, ( - f"backup disk did not record deletion markers for the {path_label} " + f"disk did not record deletion markers for the {path_label} " f"removed blobs (killer never drained the queue?); unmarked: {unmarked}" ) - # Give a (mis)behaving inner killer several scheduling rounds to physically - # unlink before asserting survival, so survival is not a timing artifact. + # Give the killer several scheduling rounds to physically unlink before + # asserting survival, so survival is not a timing artifact. surviving = set() for _ in range(15): surviving = _list_files(obj_dir) @@ -372,85 +350,84 @@ def _assert_blobs_survive(source_blobs, obj_dir, marker_dir, path_label): missing = sorted(removed - surviving) assert not missing, ( - f"backup-wrapped object-storage blobs orphaned by the {path_label} path " - "were physically deleted despite the soft-delete markers (the inner " - f"BlobKillerThread defeated the backup layer); missing blobs: {missing}" + f"object-storage blobs orphaned by the {path_label} path were physically " + f"deleted despite the soft-delete markers; missing blobs: {missing}" ) -def test_backup_disk_merge_soft_delete(start_cluster): +def test_soft_delete_on_merge(start_cluster): """Merge path: OPTIMIZE FINAL merges parts; the outdated SOURCE parts' blobs must physically survive (markers), not be unlinked.""" - node.query("DROP TABLE IF EXISTS backup_merge_tbl SYNC") + node.query("DROP TABLE IF EXISTS soft_delete_merge_tbl SYNC") node.query( f""" - CREATE TABLE backup_merge_tbl (a UInt64) + CREATE TABLE soft_delete_merge_tbl (a UInt64) ENGINE = MergeTree ORDER BY a - SETTINGS {_backup_disk_clause(MARKER_DIR_MERGE, OBJ_DIR_MERGE)}, {_PROMPT_CLEANUP} + SETTINGS {_soft_delete_disk_clause(MARKER_DIR_MERGE, OBJ_DIR_MERGE)}, {_PROMPT_CLEANUP} """ ) # Hold merges so the two INSERTs land as two distinct source parts that we # can capture before OPTIMIZE merges (and orphans) them. - node.query("SYSTEM STOP MERGES backup_merge_tbl") - node.query("INSERT INTO backup_merge_tbl SELECT number FROM numbers(1000)") - node.query("INSERT INTO backup_merge_tbl SELECT number FROM numbers(1000, 1000)") + node.query("SYSTEM STOP MERGES soft_delete_merge_tbl") + node.query("INSERT INTO soft_delete_merge_tbl SELECT number FROM numbers(1000)") + node.query("INSERT INTO soft_delete_merge_tbl SELECT number FROM numbers(1000, 1000)") assert ( node.query( "SELECT count() FROM system.parts " - "WHERE database = currentDatabase() AND table = 'backup_merge_tbl' AND active" + "WHERE database = currentDatabase() AND table = 'soft_delete_merge_tbl' AND active" ).strip() == "2" ), "expected two active source parts before the merge" # Capture the SOURCE parts' object keys (these are the blobs the merge orphans). - source_blobs = _capture_source_part_blobs("backup_merge_tbl", OBJ_DIR_MERGE) + source_blobs = _capture_source_part_blobs("soft_delete_merge_tbl", OBJ_DIR_MERGE) - node.query("SYSTEM START MERGES backup_merge_tbl") + node.query("SYSTEM START MERGES soft_delete_merge_tbl") # optimize_throw_if_noop guards against a silent no-op (which would orphan # nothing and make the survival check vacuous). node.query( - "OPTIMIZE TABLE backup_merge_tbl FINAL", + "OPTIMIZE TABLE soft_delete_merge_tbl FINAL", settings={"optimize_throw_if_noop": 1}, ) - _wait_outdated_parts_gone("backup_merge_tbl") + _wait_outdated_parts_gone("soft_delete_merge_tbl") _assert_blobs_survive(source_blobs, OBJ_DIR_MERGE, MARKER_DIR_MERGE, "merge") -def test_backup_disk_mutation_soft_delete(start_cluster): +def test_soft_delete_on_mutation(start_cluster): """Mutation path: ALTER ... DELETE rewrites the affected part and orphans the old one; the old part's blobs must physically survive (markers).""" - node.query("DROP TABLE IF EXISTS backup_mut_tbl SYNC") + node.query("DROP TABLE IF EXISTS soft_delete_mut_tbl SYNC") node.query( f""" - CREATE TABLE backup_mut_tbl (a UInt64) + CREATE TABLE soft_delete_mut_tbl (a UInt64) ENGINE = MergeTree ORDER BY a - SETTINGS {_backup_disk_clause(MARKER_DIR_MUT, OBJ_DIR_MUT)}, {_PROMPT_CLEANUP} + SETTINGS {_soft_delete_disk_clause(MARKER_DIR_MUT, OBJ_DIR_MUT)}, {_PROMPT_CLEANUP} """ ) - node.query("INSERT INTO backup_mut_tbl SELECT number FROM numbers(2000)") + node.query("INSERT INTO soft_delete_mut_tbl SELECT number FROM numbers(2000)") # Capture the OLD part's object keys before the mutation rewrites it. - source_blobs = _capture_source_part_blobs("backup_mut_tbl", OBJ_DIR_MUT) + source_blobs = _capture_source_part_blobs("soft_delete_mut_tbl", OBJ_DIR_MUT) # mutations_sync = 2 blocks until the mutation has fully materialized the new # part (and thus orphaned the old one). node.query( - "ALTER TABLE backup_mut_tbl DELETE WHERE a < 1000", + "ALTER TABLE soft_delete_mut_tbl DELETE WHERE a < 1000", settings={"mutations_sync": 2}, ) - _wait_mutation_done("backup_mut_tbl") - _wait_outdated_parts_gone("backup_mut_tbl") + _wait_mutation_done("soft_delete_mut_tbl") + _wait_outdated_parts_gone("soft_delete_mut_tbl") _assert_blobs_survive(source_blobs, OBJ_DIR_MUT, MARKER_DIR_MUT, "mutation") -def test_backup_disk_ttl_soft_delete(start_cluster): +def test_soft_delete_on_ttl(start_cluster): """TTL-delete path: a row TTL expires part of the data; the TTL merge rewrites the part dropping the expired rows and orphans the source part, whose blobs must physically survive (markers). @@ -461,22 +438,22 @@ def test_backup_disk_ttl_soft_delete(start_cluster): source part. Keeping the rewrite non-empty avoids the empty-TTL-part cleanup edge case (an all-expired part rewrites to an empty part whose removal is governed by separate, slower bookkeeping).""" - node.query("DROP TABLE IF EXISTS backup_ttl_tbl SYNC") + node.query("DROP TABLE IF EXISTS soft_delete_ttl_tbl SYNC") node.query( f""" - CREATE TABLE backup_ttl_tbl (a UInt64, d DateTime) + CREATE TABLE soft_delete_ttl_tbl (a UInt64, d DateTime) ENGINE = MergeTree ORDER BY a TTL d + INTERVAL 1 SECOND DELETE - SETTINGS {_backup_disk_clause(MARKER_DIR_TTL, OBJ_DIR_TTL)}, {_PROMPT_CLEANUP} + SETTINGS {_soft_delete_disk_clause(MARKER_DIR_TTL, OBJ_DIR_TTL)}, {_PROMPT_CLEANUP} """ ) # Hold merges so the background TTL merge cannot fire before we capture the # source part. One INSERT => one part with mixed expired / live rows. - node.query("SYSTEM STOP MERGES backup_ttl_tbl") + node.query("SYSTEM STOP MERGES soft_delete_ttl_tbl") node.query( - "INSERT INTO backup_ttl_tbl " + "INSERT INTO soft_delete_ttl_tbl " "SELECT number, if(number < 1000, now() - INTERVAL 1 DAY, now() + INTERVAL 1 DAY) " "FROM numbers(2000)" ) @@ -484,27 +461,27 @@ def test_backup_disk_ttl_soft_delete(start_cluster): assert ( node.query( "SELECT count() FROM system.parts " - "WHERE database = currentDatabase() AND table = 'backup_ttl_tbl' AND active" + "WHERE database = currentDatabase() AND table = 'soft_delete_ttl_tbl' AND active" ).strip() == "1" ), "expected one active source part before the TTL merge" # Capture the SOURCE part's object keys (the TTL merge orphans these). - source_blobs = _capture_source_part_blobs("backup_ttl_tbl", OBJ_DIR_TTL) + source_blobs = _capture_source_part_blobs("soft_delete_ttl_tbl", OBJ_DIR_TTL) - node.query("SYSTEM START MERGES backup_ttl_tbl") + node.query("SYSTEM START MERGES soft_delete_ttl_tbl") # OPTIMIZE FINAL materializes the row TTL, dropping the expired rows into a # new part and orphaning the source part. optimize_throw_if_noop guards # against a silent no-op (which would orphan nothing). node.query( - "OPTIMIZE TABLE backup_ttl_tbl FINAL", + "OPTIMIZE TABLE soft_delete_ttl_tbl FINAL", settings={"optimize_throw_if_noop": 1}, ) # Sanity: the TTL merge actually dropped the expired rows. assert ( - node.query("SELECT count() FROM backup_ttl_tbl").strip() == "1000" + node.query("SELECT count() FROM soft_delete_ttl_tbl").strip() == "1000" ), "expected the TTL merge to drop the 1000 expired rows" - _wait_outdated_parts_gone("backup_ttl_tbl") + _wait_outdated_parts_gone("soft_delete_ttl_tbl") _assert_blobs_survive(source_blobs, OBJ_DIR_TTL, MARKER_DIR_TTL, "TTL") diff --git a/tests/integration/test_aiven_backup_disk_cache_layer/__init__.py b/tests/integration/test_aiven_soft_delete_cache_layer/__init__.py similarity index 100% rename from tests/integration/test_aiven_backup_disk_cache_layer/__init__.py rename to tests/integration/test_aiven_soft_delete_cache_layer/__init__.py diff --git a/tests/integration/test_aiven_backup_disk_cache_layer/test.py b/tests/integration/test_aiven_soft_delete_cache_layer/test.py similarity index 80% rename from tests/integration/test_aiven_backup_disk_cache_layer/test.py rename to tests/integration/test_aiven_soft_delete_cache_layer/test.py index 32d9f04592b5..7f3b1c735d3b 100644 --- a/tests/integration/test_aiven_backup_disk_cache_layer/test.py +++ b/tests/integration/test_aiven_soft_delete_cache_layer/test.py @@ -1,42 +1,25 @@ -"""Integration test reproducing the production tiered-storage stack shape for a -cloud provider's `backup` disk (patch 026): backup -> cache -> object_storage. - -WHY A SEPARATE TEST (sibling to `test_aiven_backup_disk`): -The existing `test_aiven_backup_disk` builds `backup -> object_storage` (backup -directly over the base). In that shape the disk immediately wrapped by `backup` -*is* the base, so patch 026 Fix A (`wrapWithBackup` disables the immediately -wrapped disk's `BlobKillerThread`) disables the base killer and the soft-deleted -blobs survive. That test passes today. - -A cloud provider's real tiered stack is `remote (encrypted) -> remote_backup (backup) -> -remote_cache (cache) -> remote_storage (s3)` — there is a CACHE layer between -backup and the base object storage. With a cache layer: - - * the cache disk has its OWN `metadata_storage` (a second removal queue), and - * `backup` wraps the CACHE disk, so Fix A disables the *cache* killer, NOT the - base object-storage disk's killer one level deeper. - -The base disk's killer stays enabled and drains its own queue (populated on every -commit, because the cache metadata transaction commits the underlying/base -transaction first) through the RAW object storage — physically deleting blobs the -backup layer only soft-deleted. That defeats the external-GC contract (silent -backup corruption; and, because the provider relies on the backup soft-delete as -the zero-copy-replication safety net, potential live-data loss). - -`object_storage_type = local` stands in for S3: the base killer's physical delete -goes through the identical `removeObjectsIfExist` path regardless of backend, and -local lets us inspect the "bucket" as plain files with no MinIO/S3. - -EXPECTED RESULT - * On the CURRENT binary (patch 026 Fix A only disables one level): these tests - FAIL on the physical-survival assertion — the base killer unlinks the blobs. - That RED is the reproduction of finding §3.3. - * After the fix (disable the transitive inner killer chain in `wrapWithBackup`, - and make `disable()` sticky against config reload): these tests PASS. - -The marker gate still passes in both cases (the backup disk's killer writes the -markers), so it is the physical-survival assertion that flips — pinning the -failure to the base killer's raw unlink, exactly as in the Fix A evidence pair. +"""Integration test for the production tiered-storage stack shape with soft delete: +cache -> object_storage(soft_delete). + +WHY A SEPARATE TEST (sibling to `test_aiven_soft_delete`): +`test_aiven_soft_delete` builds a bare `object_storage` disk with `soft_delete = 1`. +The real tiered stack additionally has a CACHE layer on top, and a cache disk owns +its OWN `metadata_storage` (a second removal queue) and its OWN `BlobKillerThread`. + +This is the shape that used to be unsound: when soft delete was applied by wrapping +an already-built disk, the killer of the disk one level deeper still drained its own +queue through the RAW object storage and physically unlinked blobs that the layer +above had only soft-deleted. + +Because `soft_delete` now wraps the object storage at CONSTRUCTION time, the base +object storage IS the `SoftDeleteObjectStorage`, and the cache layer wraps that +(`CachedObjectStorage(SoftDeleteObjectStorage(LocalObjectStorage))`). Every killer in +the stack therefore routes deletions through the soft-delete layer, and the failure +mode is structurally impossible rather than suppressed. This test pins that. + +`object_storage_type = local` stands in for S3: the physical delete would go through +the identical `removeObjectsIfExist` path regardless of backend, and local lets us +inspect the "bucket" as plain files with no MinIO/S3. """ import time @@ -100,23 +83,22 @@ def start_cluster(): def _backup_cache_disk_clause(marker_dir, cache_path, obj_dir): - """Production stack shape: backup -> cache -> object_storage(local). + """Production stack shape: cache -> object_storage(local) with soft delete. - Inline nested `disk(...)` flattens post-order, so the base object_storage disk - is created (and its BlobKillerThread started) first, then the cache disk wraps - it, then the backup disk wraps the cache disk. + Inline nested `disk(...)` flattens post-order, so the base object_storage disk is + created first (already wrapped in `SoftDeleteObjectStorage`), then the cache disk + wraps it. """ return ( - "disk = disk(" - "type = backup, " - f"path = '{marker_dir}/', " "disk = disk(" "type = cache, " f"path = '{cache_path}', " f"max_size = {CACHE_MAX_SIZE}, " "disk = disk(" "type = object_storage, object_storage_type = local, " - f"path = '{obj_dir}/')))" + f"path = '{obj_dir}/', " + "soft_delete = 1, " + f"soft_delete_markers_path = '{marker_dir}/'))" ) @@ -259,7 +241,7 @@ def _assert_blobs_survive(removed, obj_dir, marker_dir, path_label): f"removed blobs (killer never drained the queue?); unmarked: {unmarked}" ) - # Give a (mis)behaving base killer several scheduling rounds to unlink before + # Give every killer in the stack several scheduling rounds to unlink before # asserting survival, so survival is not merely a timing artifact. surviving = set() for _ in range(15): @@ -270,10 +252,9 @@ def _assert_blobs_survive(removed, obj_dir, marker_dir, path_label): missing = sorted(removed - surviving) assert not missing, ( - f"backup-wrapped object-storage blobs orphaned by the {path_label} path " - "were physically deleted despite the soft-delete markers (the BASE " - "object-storage disk's BlobKillerThread, one level below the cache layer, " - f"is not disabled by Fix A and unlinked them); missing blobs: {missing}" + f"object-storage blobs orphaned by the {path_label} path were physically " + "deleted despite the soft-delete markers (a BlobKillerThread in the stack " + f"bypassed the soft-delete layer); missing blobs: {missing}" )