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}" )