Fix leftover S3 files when deleting native ClickHouse backups

ChangeLog.md

@@ -3,6 +3,7 @@ NEW FEATURES
 - `use_embedded_backup_restore_cluster` now orchestrates the whole cluster instead of only adding `ON CLUSTER` to the BACKUP / RESTORE SQL: `create`, `create_remote`, `restore`, `restore_remote` on the initiator node send the same command with the new `--embedded-on-cluster-worker` flag to every other node of the cluster through its `system.backup_actions` table (`clickhouse-backup server` with `api.create_integration_tables: true` is required on every node) and wait for them, a worker skips the BACKUP / RESTORE SQL and only describes, uploads, downloads and fixes its own `shards/N/replicas/M/` part of the shared destination, per table `metadata/<db>/<table>.json` moved under `shards/N/replicas/M/` next to the `.sql` of each node (such backups are tagged `embedded,cluster=<name>`, older backups without the tag keep the flat layout), `.backup`, `metadata.json`, rbac, configs and named collections stay on the initiator, `delete local` is propagated to the other nodes as `delete local --embedded-on-cluster-worker`, so `delete` accepts the flag too, a single node cluster behaves as before, fix [#928](https://github.com/Altinity/clickhouse-backup/issues/928)
 
 BUG FIXES
+- `delete local|remote` and `backups_to_keep_remote` retention of backups created with `use_embedded_backup_restore: true` and `embedded_backup_disk` on an object disk (`s3`, `azure_blob_storage`) now delete the native BACKUP objects once both the local and the remote copy are gone: `delete remote` never detected the backup as embedded (the cleanup ran over `object_disk_path` and removed nothing), files of the native backup named `*.json` (e.g. `serialization.json`) were skipped, `tar`/`gzip` data archives, `access`, `configs` and `named_collections` were parsed as object pointers and failed the cleanup; note `delete remote` now removes the objects even when another host still keeps a downloaded local copy of the same backup, the same way `delete local` already did, contributed by @cryptoque, fix [#1590](https://github.com/Altinity/clickhouse-backup/pull/1590)
 - `list` and `tables` text output is now written to stdout as a single write, so a log line on stderr can no longer land in the middle of a backup row when both streams are merged (`docker exec`, `2>&1`), the same fix `json` / `yaml` / `csv` / `tsv` formats got in v2.8.1
 - `restore` / `restore_remote` with `--restore-database-mapping` / `--restore-table-mapping` of tables on object disks (`s3`, `azure_blob_storage`, including `cache` and `encrypted` wrappers) no longer leaves the restored copy sharing object storage keys with the source table, so `DROP TABLE|DATABASE ... SYNC` of the copy no longer deletes the objects of the source (`Code: 499 ... The specified key does not exist (S3_ERROR)` on the source afterwards): the key rewrite was skipped when the disk endpoint key prefix contained the backup name (e.g. backup `regression` on the disk `https://s3/bucket/regression-test/live/`), and the metadata files were linked into `detached` / the table directory before the keys were rewritten, so an interrupted restore left the copy pointing to the source keys; the rewritten key now also carries a hash of the destination table (`<dir>_<backup_name>_<crc32(db.table)>/<object>`), so several copies of one backup restored next to each other never share objects, the source key of the copy is always derived from the backup, so the same local backup can be restored several times, and metadata files are written via temp file + rename, so a repeated restore never modifies the parts of an already restored table through their shared hardlink, fix [#1568](https://github.com/Altinity/clickhouse-backup/issues/1568)
 - `upload --resume` no longer writes an incomplete `manifest.bolt.gz`: files of parts, archives and table metadata which the resumable state marks as already processed were skipped without being recorded in the manifest, so the manifest of a resumed upload listed only the files of the last attempt, overwrote the fuller one and `download` / `restore_remote` fell back to a `Walk` (ListObjectsV2) per skipped part. Skipped files are now recorded from the local backup without any extra remote listing; with `--delete-source` (`create_remote`) the local part files are already removed, so parts skipped on resume stay out of the manifest and still use the `Walk` fallback, fix [#1569](https://github.com/Altinity/clickhouse-backup/issues/1569)

pkg/backup/delete.go

@@ -3,6 +3,7 @@ package backup
 import (
 	"context"
 	"fmt"
+	"io"
 	"io/fs"
 	"os"
 	"path"
@@ -17,6 +18,7 @@ import (
 	"github.com/Altinity/clickhouse-backup/v2/pkg/pidlock"
 
 	"github.com/Altinity/clickhouse-backup/v2/pkg/clickhouse"
+	"github.com/Altinity/clickhouse-backup/v2/pkg/config"
 	"github.com/Altinity/clickhouse-backup/v2/pkg/custom"
 	"github.com/Altinity/clickhouse-backup/v2/pkg/metadata"
 	"github.com/Altinity/clickhouse-backup/v2/pkg/status"
@@ -421,6 +423,39 @@ func (b *Backuper) hasObjectDisksLocal(backupList []LocalBackup, backupName stri
 	return false
 }
 
+// shouldCleanEmbeddedFile excludes clickhouse-backup's records, but keeps
+// native data files such as serialization.json: on object disks those files
+// contain pointers to remote objects just like the other native backup files.
+func shouldCleanEmbeddedFile(relativePath string) bool {
+	// S3 Walk can return names with a leading slash after removing its prefix.
+	relativePath = strings.TrimLeft(filepath.ToSlash(relativePath), "/")
+	if relativePath == storage.ManifestFileName {
+		return false
+	}
+	// Access rules, server configuration and named collections are copied as
+	// ordinary files or archives, not as object-disk pointers.
+	for _, directory := range []string{"access", "configs", "named_collections"} {
+		if strings.HasPrefix(relativePath, directory+"/") {
+			return false
+		}
+		for _, extension := range config.ArchiveExtensions {
+			if relativePath == directory+"."+extension {
+				return false
+			}
+		}
+	}
+	if !strings.HasSuffix(relativePath, ".json") {
+		return true
+	}
+	// ON CLUSTER backups put each replica's files under shards/N/replicas/M/.
+	parts := strings.SplitN(relativePath, "/", 6)
+	if len(parts) == 6 && parts[0] == "shards" && parts[2] == "replicas" {
+		relativePath = parts[4] + "/" + parts[5]
+	}
+	// Local native payloads use data/; uploaded payloads use shadow/.
+	return strings.HasPrefix(relativePath, "data/") || strings.HasPrefix(relativePath, "shadow/")
+}
+
 func (b *Backuper) cleanLocalEmbedded(ctx context.Context, backup LocalBackup, disks []clickhouse.Disk) error {
 	for _, disk := range disks {
 		if disk.Name == b.cfg.ClickHouse.EmbeddedBackupDisk && disk.Type != "local" {
@@ -432,7 +467,11 @@ func (b *Backuper) cleanLocalEmbedded(ctx context.Context, backup LocalBackup, d
 				if err != nil {
 					return err
 				}
-				if !info.IsDir() && !strings.HasSuffix(filePath, ".json") && !strings.HasPrefix(filePath, path.Join(backupPath, "access")) {
+				relativePath, err := filepath.Rel(backupPath, filePath)
+				if err != nil {
+					return errors.Wrap(err, "filepath.Rel backupPath")
+				}
+				if !info.IsDir() && shouldCleanEmbeddedFile(relativePath) && !strings.HasPrefix(filePath, path.Join(backupPath, "access")) {
 					log.Debug().Msgf("object_disk.ReadMetadataFromFile(%s)", filePath)
 					meta, err := object_disk.ReadMetadataFromFile(filePath)
 					if err != nil {
@@ -616,21 +655,23 @@ func (b *Backuper) dryRunRemoveBackupRemote(ctx context.Context, backupName stri
 }
 
 func (b *Backuper) cleanEmbeddedAndObjectDiskRemoteIfSameLocalNotPresent(ctx context.Context, backup storage.Backup) error {
+	// local variable, b.isEmbedded belongs to the current command, retention inside `upload` walks other backups
+	isEmbedded := strings.Contains(backup.Tags, "embedded")
 	var skip bool
 	var err error
 	if skip, err = b.skipIfSameLocalBackupPresent(ctx, backup.BackupName, backup.Tags); err != nil {
 		return errors.Wrap(err, "b.skipIfSameLocalBackupPresent")
 	}
 	log.Debug().Str("backupName", backup.BackupName).Str("tags", backup.Tags).Msgf("b.skipIfSameLocalBackupPresent return skip=%v", skip)
 	if !skip {
-		if b.isEmbedded && b.cfg.ClickHouse.EmbeddedBackupDisk != "" {
+		if isEmbedded && b.cfg.ClickHouse.EmbeddedBackupDisk != "" {
 			if err = b.cleanRemoteEmbedded(ctx, backup); err != nil {
 				log.Warn().Msgf("b.cleanRemoteEmbedded return error: %v", err)
 				return err
 			}
 			return nil
 		}
-		if b.hasObjectDisksRemote(backup) || (b.isEmbedded && b.cfg.ClickHouse.EmbeddedBackupDisk == "") {
+		if b.hasObjectDisksRemote(backup) || (isEmbedded && b.cfg.ClickHouse.EmbeddedBackupDisk == "") {
 			startTime := time.Now()
 			if deletedKeys, deleteErr := b.cleanBackupObjectDisks(ctx, backup.BackupName); deleteErr != nil {
 				log.Warn().Msgf("b.cleanBackupObjectDisks return error: %v", deleteErr)
@@ -653,30 +694,48 @@ func (b *Backuper) hasObjectDisksRemote(backup storage.Backup) bool {
 }
 
 func (b *Backuper) cleanRemoteEmbedded(ctx context.Context, backup storage.Backup) error {
+	// Native backups made on local disks contain ordinary files, not object pointers.
+	if backup.DiskTypes[b.cfg.ClickHouse.EmbeddedBackupDisk] == "local" {
+		return nil
+	}
 	if err := object_disk.InitCredentialsAndConnections(ctx, b.ch, b.cfg, b.cfg.ClickHouse.EmbeddedBackupDisk); err != nil {
 		return errors.Wrap(err, "object_disk.InitCredentialsAndConnections")
 	}
 	return b.dst.Walk(ctx, backup.BackupName+"/", true, func(ctx context.Context, f storage.RemoteFile) error {
-		if !strings.HasSuffix(f.Name(), ".json") {
-			r, err := b.dst.GetFileReader(ctx, path.Join(backup.BackupName, f.Name()))
-			if err != nil {
-				return errors.Wrap(err, "b.dst.GetFileReader")
-			}
-			log.Debug().Msgf("object_disk.ReadMetadataFromReader(%s)", f.Name())
-			meta, err := object_disk.ReadMetadataFromReader(r, f.Name())
-			if err != nil {
-				return errors.Wrap(err, "object_disk.ReadMetadataFromReader")
-			}
-			for _, o := range meta.StorageObjects {
-				if err = object_disk.DeleteFile(ctx, b.cfg.ClickHouse.EmbeddedBackupDisk, o.ObjectPath); err != nil {
-					return errors.Wrap(err, "object_disk.DeleteFile")
-				}
-			}
+		if !shouldCleanEmbeddedFile(f.Name()) {
+			return nil
 		}
-		return nil
+		remotePath := path.Join(backup.BackupName, f.Name())
+		if extension, compressed := config.ArchiveExtensions[backup.DataFormat]; compressed && strings.HasSuffix(f.Name(), "."+extension) {
+			return b.dst.WalkCompressedStream(ctx, remotePath, func(ctx context.Context, name string, content io.Reader) error {
+				// Every file inside a native data archive is an object-disk pointer,
+				// including serialization.json. WalkCompressedStream closes each entry.
+				return b.cleanEmbeddedObjectsFromReader(ctx, io.NopCloser(content), name)
+			})
+		}
+		r, err := b.dst.GetFileReader(ctx, remotePath)
+		if err != nil {
+			return errors.Wrap(err, "b.dst.GetFileReader")
+		}
+		return b.cleanEmbeddedObjectsFromReader(ctx, r, f.Name())
 	})
 }
 
+func (b *Backuper) cleanEmbeddedObjectsFromReader(ctx context.Context, reader io.ReadCloser, name string) error {
+	log.Debug().Msgf("object_disk.ReadMetadataFromReader(%s)", name)
+	// ReadMetadataFromReader closes the reader on both success and failure.
+	meta, err := object_disk.ReadMetadataFromReader(reader, name)
+	if err != nil {
+		return errors.Wrapf(err, "object_disk.ReadMetadataFromReader(%s)", name)
+	}
+	for _, object := range meta.StorageObjects {
+		if err = object_disk.DeleteFile(ctx, b.cfg.ClickHouse.EmbeddedBackupDisk, object.ObjectPath); err != nil {
+			return errors.Wrap(err, "object_disk.DeleteFile")
+		}
+	}
+	return nil
+}
+
 // cleanBackupObjectDisks - recursive delete <object_disks_path>/<backupName>
 func (b *Backuper) cleanBackupObjectDisks(ctx context.Context, backupName string) (uint, error) {
 	objectDiskPath, err := b.getObjectDiskPath()