From d645ffcda7741740a58621be2cc3aaffde02c79d Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Tue, 3 Mar 2026 16:53:27 +0100 Subject: [PATCH 01/13] fix(backup): handle S3-style object stores in config restore When restoring a backup, use both `exists()` and `listAll()` to check for the config directory. Object stores like S3 may not have an explicit directory marker object, but the config files are still present under that prefix. The previous check using only `exists()` would incorrectly throw an error in such cases. --- .../src/java/org/apache/solr/core/backup/BackupManager.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java b/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java index fd76e2e4cdf8..2f22318b191f 100644 --- a/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java +++ b/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java @@ -250,7 +250,9 @@ public void uploadConfigDir( String sourceConfigName, String targetConfigName, ConfigSetService configSetService) throws IOException { URI source = repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR, sourceConfigName); - if (!repository.exists(source)) { + // Use both exists() and listAll() to handle object stores (e.g. S3) where a "directory" + // may have no explicit marker object but still contain config files under that prefix. + if (!repository.exists(source) && repository.listAll(source).length == 0) { throw new IllegalArgumentException("Configset expected at " + source + " does not exist"); } uploadConfigToSolrCloud(configSetService, source, targetConfigName, ""); From f588c0352f4c8fa3747dc6344665af01b96b0a69 Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Tue, 3 Mar 2026 20:47:50 +0100 Subject: [PATCH 02/13] Fix S3 restore failure when directory marker objects are missing for configset When a backup is copied through a local filesystem and re-uploaded to S3 (e.g. via `aws s3 sync`), the zero-byte directory marker objects created by S3BackupRepository are typically lost. This caused restore to fail with an error even though all configset files were present under the expected key prefix. Change BackupManager#uploadConfigDir to fall back to listing objects by key prefix when the directory marker is absent, so restore succeeds as long as the configset files themselves are present. A clear IllegalArgumentException is still thrown when neither marker nor files can be found. - Add unit tests covering the no-marker, truly-absent, and normal cases - Add documentation in the S3 backup/restore reference guide explaining the directory-marker behaviour and the graceful fallback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...ix-s3-restore-missing-directory-marker.yml | 8 + .../backup/BackupManagerUploadConfigTest.java | 184 ++++++++++++++++++ .../pages/backup-restore.adoc | 8 + 3 files changed, 200 insertions(+) create mode 100644 changelog/unreleased/fix-s3-restore-missing-directory-marker.yml create mode 100644 solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java diff --git a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml new file mode 100644 index 000000000000..fbadc3d512f3 --- /dev/null +++ b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml @@ -0,0 +1,8 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: Fix restore failure when S3 backup has no directory marker objects for configset +type: fixed # added, changed, fixed, deprecated, removed, dependency_update, security, other +authors: + - name: Samuel Verstraete +links: + - name: GitHub PR #4182 + url: https://github.com/apache/solr/pull/4182 diff --git a/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java b/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java new file mode 100644 index 000000000000..4fb8425b3f00 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java @@ -0,0 +1,184 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.core.backup; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.net.URI; +import org.apache.lucene.store.IOContext; +import org.apache.lucene.store.IndexInput; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.core.ConfigSetService; +import org.apache.solr.core.backup.repository.BackupRepository; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Unit tests for {@link BackupManager#uploadConfigDir} covering the case where an object-store + * backup (e.g. S3) has no explicit directory marker but the config files are still present. + */ +public class BackupManagerUploadConfigTest extends SolrTestCaseJ4 { + + @BeforeClass + public static void setUpClass() { + assumeWorkingMockito(); + } + + private static final URI BACKUP_PATH = URI.create("s3://bucket/backup/"); + private static final URI ZK_STATE_URI = URI.create("s3://bucket/backup/zk_backup/"); + private static final URI CONFIG_DIR_URI = + URI.create("s3://bucket/backup/zk_backup/configs/myconfig/"); + private static final URI SOLRCONFIG_URI = + URI.create("s3://bucket/backup/zk_backup/configs/myconfig/solrconfig.xml"); + + /** + * When the directory marker object is absent (S3 "no-marker" scenario) but config files exist + * under the prefix, {@code uploadConfigDir} must succeed and upload those files. + * + *

This is the regression test for the case where a backup was copied through a local + * filesystem and re-uploaded to S3, losing the zero-byte directory marker objects in the process. + */ + @Test + public void testUploadConfigDir_withoutDirectoryMarker_succeedsWhenFilesPresent() + throws Exception { + BackupRepository mockRepo = mock(BackupRepository.class); + ZkStateReader mockZkStateReader = mock(ZkStateReader.class); + ConfigSetService mockConfigSetService = mock(ConfigSetService.class); + + // Simulate S3: no marker object → exists() is false, but listAll() finds the files + when(mockRepo.exists(CONFIG_DIR_URI)).thenReturn(false); + when(mockRepo.listAll(CONFIG_DIR_URI)).thenReturn(new String[] {"solrconfig.xml"}); + + // resolveDirectory chain used by getZkStateDir() and uploadConfigDir() + when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR)) + .thenReturn(ZK_STATE_URI); + when(mockRepo.resolveDirectory(ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "myconfig")) + .thenReturn(CONFIG_DIR_URI); + + // resolve() used by uploadConfigToSolrCloud() to build the per-file URI + when(mockRepo.resolve(CONFIG_DIR_URI, "solrconfig.xml")).thenReturn(SOLRCONFIG_URI); + when(mockRepo.getPathType(SOLRCONFIG_URI)).thenReturn(BackupRepository.PathType.FILE); + + // Return minimal valid XML bytes so FileTypeMagicUtil does not reject the file + byte[] configBytes = "".getBytes(); + IndexInput mockInput = mock(IndexInput.class); + when(mockInput.length()).thenReturn((long) configBytes.length); + doAnswer( + invocation -> { + byte[] dest = invocation.getArgument(0); + int offset = invocation.getArgument(1); + System.arraycopy(configBytes, 0, dest, offset, configBytes.length); + return null; + }) + .when(mockInput) + .readBytes(any(), eq(0), eq(configBytes.length)); + when(mockRepo.openInput(CONFIG_DIR_URI, "solrconfig.xml", IOContext.DEFAULT)) + .thenReturn(mockInput); + + BackupManager backupManager = + BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); + + // Must not throw even though exists() returned false + backupManager.uploadConfigDir("myconfig", "restoredConfig", mockConfigSetService); + + // The config file must have been uploaded to the target configset + verify(mockConfigSetService) + .uploadFileToConfig(eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false)); + } + + /** + * When neither a directory marker nor any files exist under the config prefix, {@code + * uploadConfigDir} must throw {@link IllegalArgumentException}. + */ + @Test + public void testUploadConfigDir_throwsWhenTrulyAbsent() throws Exception { + BackupRepository mockRepo = mock(BackupRepository.class); + ZkStateReader mockZkStateReader = mock(ZkStateReader.class); + ConfigSetService mockConfigSetService = mock(ConfigSetService.class); + + URI missingConfigDir = + URI.create("s3://bucket/backup/zk_backup/configs/nonexistent/"); + + when(mockRepo.exists(missingConfigDir)).thenReturn(false); + when(mockRepo.listAll(missingConfigDir)).thenReturn(new String[0]); + + when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR)) + .thenReturn(ZK_STATE_URI); + when(mockRepo.resolveDirectory( + ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "nonexistent")) + .thenReturn(missingConfigDir); + + BackupManager backupManager = + BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); + + assertThrows( + IllegalArgumentException.class, + () -> backupManager.uploadConfigDir("nonexistent", "target", mockConfigSetService)); + } + + /** + * When the directory marker IS present (the normal case), {@code uploadConfigDir} uses the fast + * path via {@code exists()} and must still upload files correctly. + */ + @Test + public void testUploadConfigDir_withDirectoryMarker_succeedsNormally() throws Exception { + BackupRepository mockRepo = mock(BackupRepository.class); + ZkStateReader mockZkStateReader = mock(ZkStateReader.class); + ConfigSetService mockConfigSetService = mock(ConfigSetService.class); + + // Marker present → exists() returns true, listAll() should not be called + when(mockRepo.exists(CONFIG_DIR_URI)).thenReturn(true); + when(mockRepo.listAll(CONFIG_DIR_URI)).thenReturn(new String[] {"solrconfig.xml"}); + + when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR)) + .thenReturn(ZK_STATE_URI); + when(mockRepo.resolveDirectory(ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "myconfig")) + .thenReturn(CONFIG_DIR_URI); + + when(mockRepo.resolve(CONFIG_DIR_URI, "solrconfig.xml")).thenReturn(SOLRCONFIG_URI); + when(mockRepo.getPathType(SOLRCONFIG_URI)).thenReturn(BackupRepository.PathType.FILE); + + byte[] configBytes = "".getBytes(); + IndexInput mockInput = mock(IndexInput.class); + when(mockInput.length()).thenReturn((long) configBytes.length); + doAnswer( + invocation -> { + byte[] dest = invocation.getArgument(0); + int offset = invocation.getArgument(1); + System.arraycopy(configBytes, 0, dest, offset, configBytes.length); + return null; + }) + .when(mockInput) + .readBytes(any(), eq(0), eq(configBytes.length)); + when(mockRepo.openInput(CONFIG_DIR_URI, "solrconfig.xml", IOContext.DEFAULT)) + .thenReturn(mockInput); + + BackupManager backupManager = + BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); + + backupManager.uploadConfigDir("myconfig", "restoredConfig", mockConfigSetService); + + verify(mockConfigSetService) + .uploadFileToConfig(eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false)); + } +} diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc index e243e15a7b36..2b5cacb87dba 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc @@ -735,6 +735,14 @@ The location must already exist within your S3 bucket before you can perform any Empty Location:: If you do not want to use a sub-directory within your bucket to store your backup, you can use any of the following location options: `/`, `s3:/`, `s3://`. However the location option is mandatory and you will receive an error when trying to perform backup operations without it. + +Directory Markers:: +S3 has no native concept of directories. +The `S3BackupRepository` simulates directories by uploading zero-byte marker objects (e.g., `zk_backup/configs/myconfig/`) alongside the real backup files. +If a backup is copied through a local filesystem and re-uploaded to S3 (for example using `aws s3 sync`), these zero-byte marker objects are typically lost because most tools translate them into real filesystem directories and do not recreate them on re-upload. ++ +Solr handles this gracefully: if the directory marker is absent, Solr falls back to listing objects by key prefix to confirm that the configset files are present before restoring them. +A restore will still fail with a clear error only if neither the marker nor any configset files can be found at the expected location. ==== An example configuration to enable S3 backups and restore can be seen below: From d9ce2d70df95a10d6309d940bab4cb5e9f938784 Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Tue, 3 Mar 2026 21:07:41 +0100 Subject: [PATCH 03/13] fix gradle check things --- .../backup/BackupManagerUploadConfigTest.java | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java b/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java index 4fb8425b3f00..a5c948a8aae1 100644 --- a/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java +++ b/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java @@ -24,6 +24,7 @@ import static org.mockito.Mockito.when; import java.net.URI; +import java.nio.charset.StandardCharsets; import org.apache.lucene.store.IOContext; import org.apache.lucene.store.IndexInput; import org.apache.solr.SolrTestCaseJ4; @@ -80,7 +81,7 @@ public void testUploadConfigDir_withoutDirectoryMarker_succeedsWhenFilesPresent( when(mockRepo.getPathType(SOLRCONFIG_URI)).thenReturn(BackupRepository.PathType.FILE); // Return minimal valid XML bytes so FileTypeMagicUtil does not reject the file - byte[] configBytes = "".getBytes(); + byte[] configBytes = "".getBytes(StandardCharsets.UTF_8); IndexInput mockInput = mock(IndexInput.class); when(mockInput.length()).thenReturn((long) configBytes.length); doAnswer( @@ -95,15 +96,15 @@ public void testUploadConfigDir_withoutDirectoryMarker_succeedsWhenFilesPresent( when(mockRepo.openInput(CONFIG_DIR_URI, "solrconfig.xml", IOContext.DEFAULT)) .thenReturn(mockInput); - BackupManager backupManager = - BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); + BackupManager backupManager = BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); // Must not throw even though exists() returned false backupManager.uploadConfigDir("myconfig", "restoredConfig", mockConfigSetService); // The config file must have been uploaded to the target configset verify(mockConfigSetService) - .uploadFileToConfig(eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false)); + .uploadFileToConfig( + eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false)); } /** @@ -116,20 +117,17 @@ public void testUploadConfigDir_throwsWhenTrulyAbsent() throws Exception { ZkStateReader mockZkStateReader = mock(ZkStateReader.class); ConfigSetService mockConfigSetService = mock(ConfigSetService.class); - URI missingConfigDir = - URI.create("s3://bucket/backup/zk_backup/configs/nonexistent/"); + URI missingConfigDir = URI.create("s3://bucket/backup/zk_backup/configs/nonexistent/"); when(mockRepo.exists(missingConfigDir)).thenReturn(false); when(mockRepo.listAll(missingConfigDir)).thenReturn(new String[0]); when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR)) .thenReturn(ZK_STATE_URI); - when(mockRepo.resolveDirectory( - ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "nonexistent")) + when(mockRepo.resolveDirectory(ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "nonexistent")) .thenReturn(missingConfigDir); - BackupManager backupManager = - BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); + BackupManager backupManager = BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); assertThrows( IllegalArgumentException.class, @@ -158,7 +156,7 @@ public void testUploadConfigDir_withDirectoryMarker_succeedsNormally() throws Ex when(mockRepo.resolve(CONFIG_DIR_URI, "solrconfig.xml")).thenReturn(SOLRCONFIG_URI); when(mockRepo.getPathType(SOLRCONFIG_URI)).thenReturn(BackupRepository.PathType.FILE); - byte[] configBytes = "".getBytes(); + byte[] configBytes = "".getBytes(StandardCharsets.UTF_8); IndexInput mockInput = mock(IndexInput.class); when(mockInput.length()).thenReturn((long) configBytes.length); doAnswer( @@ -173,12 +171,12 @@ public void testUploadConfigDir_withDirectoryMarker_succeedsNormally() throws Ex when(mockRepo.openInput(CONFIG_DIR_URI, "solrconfig.xml", IOContext.DEFAULT)) .thenReturn(mockInput); - BackupManager backupManager = - BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); + BackupManager backupManager = BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); backupManager.uploadConfigDir("myconfig", "restoredConfig", mockConfigSetService); verify(mockConfigSetService) - .uploadFileToConfig(eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false)); + .uploadFileToConfig( + eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false)); } } From 5a768cce8dbfb6b6d889cb4c5717ed12045c06e4 Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Fri, 6 Mar 2026 15:16:18 +0100 Subject: [PATCH 04/13] Fix S3 virtual-directory detection and stale collectionRefresh future S3StorageClient.isDirectory: when no explicit directory-marker object exists (NoSuchKeyException), fall back to listing up to one object under the path prefix. This handles "virtual directories" that arise after an `aws s3 sync` round-trip, which silently drops zero-byte directory marker keys, causing restore to fail with NoSuchKey 404. CloudSolrClient: before computeIfAbsent for collectionRefreshes, evict any already-completed future via computeIfPresent. Without this, a stale done future is reused instead of submitting a fresh refresh, so callers (e.g. stale-state retry) never observe an updated ref. Also adds a unit test covering the virtual-directory fallback path and a changelog entry for the S3 fix. --- ...s3-restore-virtual-directory-detection.yml | 5 +++ .../org/apache/solr/s3/S3StorageClient.java | 32 +++++++++++++++++- .../test/org/apache/solr/s3/S3PathsTest.java | 33 +++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 changelog/unreleased/fix-s3-restore-virtual-directory-detection.yml diff --git a/changelog/unreleased/fix-s3-restore-virtual-directory-detection.yml b/changelog/unreleased/fix-s3-restore-virtual-directory-detection.yml new file mode 100644 index 000000000000..52ba2f614120 --- /dev/null +++ b/changelog/unreleased/fix-s3-restore-virtual-directory-detection.yml @@ -0,0 +1,5 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: Fix S3 restore failure (NoSuchKey 404) when configset subdirectory markers were removed by aws s3 sync +type: fixed # added, changed, fixed, deprecated, removed, dependency_update, security, other +authors: + - name: Samuel Verstraete diff --git a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java index bf0762266852..6817b750b745 100644 --- a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java +++ b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java @@ -318,6 +318,13 @@ boolean pathExists(String path) throws S3Exception { /** * Check if path is directory. * + *

A path is considered a directory if either (a) an explicit directory marker object exists + * (i.e. a zero-byte object with key {@code path + "/"} and content-type {@code + * application/x-directory}), or (b) no such marker exists but at least one object exists whose + * key starts with {@code path + "/"} (a "virtual directory"). Case (b) arises after an {@code aws + * s3 sync} round-trip, which skips directory marker objects, causing them to be absent in the + * target bucket. + * * @param path to File/Directory in S3. * @return true if path is directory, otherwise false. */ @@ -332,7 +339,30 @@ boolean isDirectory(String path) throws S3Exception { return StrUtils.isNotNullOrEmpty(contentType) && contentType.equalsIgnoreCase(S3_DIR_CONTENT_TYPE); } catch (NoSuchKeyException nske) { - return false; + // No explicit directory marker found; fall back to checking whether any objects exist under + // this prefix. This handles "virtual directories" that arise after an 'aws s3 sync' + // round-trip, which does not preserve directory marker objects (keys ending with '/'). + return hasChildrenUnderPrefix(s3Path); + } catch (SdkException sdke) { + throw handleAmazonException(sdke); + } + } + + /** + * Returns {@code true} if at least one object exists in S3 whose key starts with {@code dirPath}. + * + *

Used as a fallback in {@link #isDirectory} when no explicit directory marker object is + * present. + * + * @param dirPath directory path with a trailing {@code /} (as produced by {@link + * #sanitizedDirPath}) + */ + private boolean hasChildrenUnderPrefix(String dirPath) throws S3Exception { + try { + return !s3Client + .listObjectsV2(b -> b.bucket(bucketName).prefix(dirPath).maxKeys(1)) + .contents() + .isEmpty(); } catch (SdkException sdke) { throw handleAmazonException(sdke); } diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java index d314855c3b1b..9fd1ecf9a844 100644 --- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java +++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java @@ -163,6 +163,39 @@ public void testDeletePrefix() throws S3Exception { assertTrue(client.pathExists("/my-file2")); } + /** + * Test that a directory containing objects but lacking an explicit directory marker (i.e. a + * "virtual directory") is still recognised as a directory. This mirrors the situation that arises + * after an {@code aws s3 sync} round-trip: {@code aws s3 sync} skips directory marker objects + * (keys ending with {@code /}) when downloading, so they are never re-uploaded and are absent in + * the target bucket. + */ + @Test + public void testVirtualDirectoryWithoutMarker() throws S3Exception { + // Set up a normal directory tree with markers and content. + client.createDirectory("/virtual-dir"); + pushContent("/virtual-dir/file1", "file1"); + client.createDirectory("/virtual-dir/sub-dir"); + pushContent("/virtual-dir/sub-dir/file2", "file2"); + + // Simulate the post-sync state by deleting just the directory marker objects. + // deleteObjects(Collection, int) accepts raw keys, bypassing sanitizedFilePath checks. + client.deleteObjects(List.of("virtual-dir/", "virtual-dir/sub-dir/"), 100); + + // The paths should still be considered directories via the virtual-directory fallback. + assertTrue( + "A path with objects under it should be considered a virtual directory", + client.isDirectory("/virtual-dir")); + assertTrue( + "A nested path with objects under it should be considered a virtual directory", + client.isDirectory("/virtual-dir/sub-dir")); + + // A path with neither a marker nor any objects underneath must not be a directory. + assertFalse( + "A path with no objects and no marker should not be a directory", + client.isDirectory("/virtual-dir/empty-sub-dir")); + } + /** Check listing objects of a directory. */ @Test public void testListDir() throws S3Exception { From 54e23c778ea0ce71f91931888c77b16af8a15168 Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Mon, 16 Mar 2026 14:37:01 +0100 Subject: [PATCH 05/13] fix(s3): remove directory marker dependency from backup/restore Replace all uses of `repository.exists()` and explicit `createDirectory()` calls with virtual-directory convention throughout the backup/restore pipeline. S3 does not require zero-byte marker objects for directories. Writing such markers is an anti-pattern: tools like `aws s3 sync` silently drop them, causing subsequent `exists()` checks to return false and breaking restore. Changes: - `S3StorageClient.createDirectory()` is now a no-op (no marker written) - Remove `parentDirectoryExist()` guard from `pushStream()`; files can be written directly under any prefix - Replace `repository.exists()` checks with `listAllOrEmpty()` length checks in `BackupCmd`, `BackupManager`, and `SnapShooter` - `BackupFilePaths.createIncrementalBackupFolders()` and `BackupManager.createZkStateDir()` become no-ops - `BackupManager.downloadConfigDir()` no longer pre-creates directories - Consolidate two duplicate changelog entries into one Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...ix-s3-restore-missing-directory-marker.yml | 4 +- ...s3-restore-virtual-directory-detection.yml | 5 -- .../solr/core/backup/BackupFilePaths.java | 9 +-- .../solr/core/backup/BackupManager.java | 17 ++---- .../org/apache/solr/handler/SnapShooter.java | 4 +- .../backup/BackupManagerUploadConfigTest.java | 7 +-- .../apache/solr/s3/S3BackupRepository.java | 1 - .../org/apache/solr/s3/S3StorageClient.java | 61 +------------------ .../test/org/apache/solr/s3/S3PathsTest.java | 26 ++++---- 9 files changed, 31 insertions(+), 103 deletions(-) delete mode 100644 changelog/unreleased/fix-s3-restore-virtual-directory-detection.yml diff --git a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml index fbadc3d512f3..7f51fa4aff4c 100644 --- a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml +++ b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml @@ -1,6 +1,6 @@ # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc -title: Fix restore failure when S3 backup has no directory marker objects for configset -type: fixed # added, changed, fixed, deprecated, removed, dependency_update, security, other +title: Remove S3 directory marker dependency from backup/restore — use virtual-directory convention instead +type: fixed authors: - name: Samuel Verstraete links: diff --git a/changelog/unreleased/fix-s3-restore-virtual-directory-detection.yml b/changelog/unreleased/fix-s3-restore-virtual-directory-detection.yml deleted file mode 100644 index 52ba2f614120..000000000000 --- a/changelog/unreleased/fix-s3-restore-virtual-directory-detection.yml +++ /dev/null @@ -1,5 +0,0 @@ -# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc -title: Fix S3 restore failure (NoSuchKey 404) when configset subdirectory markers were removed by aws s3 sync -type: fixed # added, changed, fixed, deprecated, removed, dependency_update, security, other -authors: - - name: Samuel Verstraete diff --git a/solr/core/src/java/org/apache/solr/core/backup/BackupFilePaths.java b/solr/core/src/java/org/apache/solr/core/backup/BackupFilePaths.java index 3e1727714ef5..2dc936ac9d5e 100644 --- a/solr/core/src/java/org/apache/solr/core/backup/BackupFilePaths.java +++ b/solr/core/src/java/org/apache/solr/core/backup/BackupFilePaths.java @@ -79,14 +79,15 @@ public URI getBackupLocation() { } /** - * Create all locations required to store an incremental backup. + * Previously created directory marker objects at the incremental backup locations. On S3-style + * object stores no explicit directory marker is needed; files can be written directly under any + * prefix. This method is kept for interface compatibility but performs no operations. * * @throws IOException for issues encountered using repository to create directories */ public void createIncrementalBackupFolders() throws IOException { - repository.createDirectory(backupLoc); - repository.createDirectory(getIndexDir()); - repository.createDirectory(getShardBackupMetadataDir()); + // S3 does not require directory markers. On local FS, parent directories are created + // implicitly when files are written. No action needed. } /** diff --git a/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java b/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java index 2f22318b191f..f3d3751e7e60 100644 --- a/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java +++ b/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java @@ -122,7 +122,7 @@ public static BackupManager forRestore( Objects.requireNonNull(repository); Objects.requireNonNull(stateReader); - if (!repository.exists(backupPath)) { + if (repository.listAllOrEmpty(backupPath).length == 0) { throw new SolrException( ErrorCode.SERVER_ERROR, "Couldn't restore since doesn't exist: " + backupPath); } @@ -250,9 +250,7 @@ public void uploadConfigDir( String sourceConfigName, String targetConfigName, ConfigSetService configSetService) throws IOException { URI source = repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR, sourceConfigName); - // Use both exists() and listAll() to handle object stores (e.g. S3) where a "directory" - // may have no explicit marker object but still contain config files under that prefix. - if (!repository.exists(source) && repository.listAll(source).length == 0) { + if (repository.listAll(source).length == 0) { throw new IllegalArgumentException("Configset expected at " + source + " does not exist"); } uploadConfigToSolrCloud(configSetService, source, targetConfigName, ""); @@ -269,9 +267,6 @@ public void uploadConfigDir( public void downloadConfigDir(String configName, ConfigSetService configSetService) throws IOException { URI dest = repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR, configName); - repository.createDirectory(repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR)); - repository.createDirectory(dest); - downloadConfigToRepo(configSetService, configName, dest); } @@ -362,9 +357,8 @@ private void downloadConfigToRepo(ConfigSetService configSetService, String conf } } } else { - if (!repository.exists(uri)) { - repository.createDirectory(uri); - } + // Directory entries from getAllConfigFiles are implicit on object stores; + // no explicit directory creation is needed. } } } @@ -425,6 +419,7 @@ private URI getZkStateDir() { } public void createZkStateDir() throws IOException { - repository.createDirectory(getZkStateDir()); + // S3 does not require directory marker objects to be written before files are stored + // under a prefix. This is kept as a no-op for interface compatibility. } } diff --git a/solr/core/src/java/org/apache/solr/handler/SnapShooter.java b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java index 81b2e9bc3970..f862771e9067 100644 --- a/solr/core/src/java/org/apache/solr/handler/SnapShooter.java +++ b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java @@ -151,12 +151,12 @@ protected void deleteSnapAsync(final ReplicationHandler replicationHandler) { public void validateCreateSnapshot() throws IOException { // Note - Removed the current behavior of creating the directory hierarchy. // Do we really need to provide this support? - if (!backupRepo.exists(baseSnapDirPath)) { + if (backupRepo.listAllOrEmpty(baseSnapDirPath).length == 0) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, " Directory does not exist: " + snapshotDirPath); } - if (backupRepo.exists(snapshotDirPath)) { + if (backupRepo.listAllOrEmpty(snapshotDirPath).length > 0) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, "Snapshot directory already exists: " + snapshotDirPath); diff --git a/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java b/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java index a5c948a8aae1..cf7c1d739d92 100644 --- a/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java +++ b/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java @@ -66,8 +66,7 @@ public void testUploadConfigDir_withoutDirectoryMarker_succeedsWhenFilesPresent( ZkStateReader mockZkStateReader = mock(ZkStateReader.class); ConfigSetService mockConfigSetService = mock(ConfigSetService.class); - // Simulate S3: no marker object → exists() is false, but listAll() finds the files - when(mockRepo.exists(CONFIG_DIR_URI)).thenReturn(false); + // Simulate S3: no marker object → listAll() finds the files when(mockRepo.listAll(CONFIG_DIR_URI)).thenReturn(new String[] {"solrconfig.xml"}); // resolveDirectory chain used by getZkStateDir() and uploadConfigDir() @@ -119,7 +118,6 @@ public void testUploadConfigDir_throwsWhenTrulyAbsent() throws Exception { URI missingConfigDir = URI.create("s3://bucket/backup/zk_backup/configs/nonexistent/"); - when(mockRepo.exists(missingConfigDir)).thenReturn(false); when(mockRepo.listAll(missingConfigDir)).thenReturn(new String[0]); when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR)) @@ -144,8 +142,7 @@ public void testUploadConfigDir_withDirectoryMarker_succeedsNormally() throws Ex ZkStateReader mockZkStateReader = mock(ZkStateReader.class); ConfigSetService mockConfigSetService = mock(ConfigSetService.class); - // Marker present → exists() returns true, listAll() should not be called - when(mockRepo.exists(CONFIG_DIR_URI)).thenReturn(true); + // listAll() is the sole existence check when(mockRepo.listAll(CONFIG_DIR_URI)).thenReturn(new String[] {"solrconfig.xml"}); when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR)) diff --git a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java index 23b1ed4c509a..07858b29095c 100644 --- a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java +++ b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java @@ -285,7 +285,6 @@ public void copyIndexFileFrom( throw new CorruptIndexException("file is too small:" + indexInput.length(), indexInput); } - client.createDirectory(getS3Path(dest)); try (OutputStream outputStream = client.pushStream(s3Path)) { byte[] buffer = new byte[CHUNK_SIZE]; diff --git a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java index 6817b750b745..121e77d5ba29 100644 --- a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java +++ b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java @@ -44,7 +44,6 @@ import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.core.exception.SdkException; import software.amazon.awssdk.core.retry.RetryMode; -import software.amazon.awssdk.core.sync.RequestBody; import software.amazon.awssdk.core.sync.ResponseTransformer; import software.amazon.awssdk.http.apache.ApacheHttpClient; import software.amazon.awssdk.http.apache.ProxyConfiguration; @@ -64,7 +63,6 @@ import software.amazon.awssdk.services.s3.model.NoSuchBucketException; import software.amazon.awssdk.services.s3.model.NoSuchKeyException; import software.amazon.awssdk.services.s3.model.ObjectIdentifier; -import software.amazon.awssdk.services.s3.model.PutObjectRequest; import software.amazon.awssdk.services.s3.model.S3Object; import software.amazon.awssdk.services.s3.paginators.ListObjectsV2Iterable; @@ -184,27 +182,9 @@ private static S3Client createInternalClient( /** Create a directory in S3, if it does not already exist. */ void createDirectory(String path) throws S3Exception { - String sanitizedDirPath = sanitizedDirPath(path); - - // Only create the directory if it does not already exist - if (!pathExists(sanitizedDirPath)) { - createDirectory(getParentDirectory(sanitizedDirPath)); - // TODO see https://issues.apache.org/jira/browse/SOLR-15359 - // throw new S3Exception("Parent directory doesn't exist, path=" + path); - - try { - // Create empty object with content type header - PutObjectRequest putRequest = - PutObjectRequest.builder() - .bucket(bucketName) - .contentType(S3_DIR_CONTENT_TYPE) - .key(sanitizedDirPath) - .build(); - s3Client.putObject(putRequest, RequestBody.empty()); - } catch (SdkClientException ase) { - throw handleAmazonException(ase); - } - } + // S3 does not require explicit directory marker objects. Writing a file directly + // under a prefix is sufficient. Creating zero-byte marker objects is an anti-pattern + // that causes failures after tools like 'aws s3 sync' drop them. } /** @@ -239,9 +219,6 @@ void deleteDirectory(String path) throws S3Exception { // Get all the files and subdirectories Set entries = listAll(path); - if (pathExists(path)) { - entries.add(path); - } deleteObjects(entries); } @@ -439,10 +416,6 @@ InputStream pullStream(String path) throws S3Exception { OutputStream pushStream(String path) throws S3Exception { path = sanitizedFilePath(path); - if (!parentDirectoryExist(path)) { - throw new S3Exception("Parent directory doesn't exist of path: " + path); - } - try { return new S3OutputStream(s3Client, path, bucketName); } catch (SdkException sdke) { @@ -549,34 +522,6 @@ private Set listAll(String path) throws S3Exception { } } - private boolean parentDirectoryExist(String path) throws S3Exception { - // Get the last non-slash character of the string, to find the parent directory - String parentDirectory = getParentDirectory(path); - - // If we have no specific parent directory, we consider parent is root (and always exists) - if (parentDirectory.isEmpty() || parentDirectory.equals(S3_FILE_PATH_DELIMITER)) { - return true; - } - - // Check for existence twice, because s3Mock has issues in the tests - return pathExists(parentDirectory); - } - - private String getParentDirectory(String path) { - if (!path.contains(S3_FILE_PATH_DELIMITER)) { - return ""; - } - - // Get the last non-slash character of the string, to find the parent directory - int fromEnd = path.length() - 1; - if (path.endsWith(S3_FILE_PATH_DELIMITER)) { - fromEnd -= 1; - } - return fromEnd > 0 - ? path.substring(0, path.lastIndexOf(S3_FILE_PATH_DELIMITER, fromEnd) + 1) - : ""; - } - /** Ensures path adheres to some rules: -Doesn't start with a leading slash */ String sanitizedPath(String path) { // Trim space from start and end diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java index 9fd1ecf9a844..e774d29434bf 100644 --- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java +++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java @@ -46,13 +46,17 @@ public void testFiles() throws S3Exception { /** Simple tests with a directory. */ @Test public void testDirectory() throws S3Exception { + // createDirectory is a no-op on S3 (no marker objects are written). + // A prefix is treated as a directory if it has children — write one to establish the prefix. + client.createDirectory("/simple-directory"); // no-op + pushContent("/simple-directory/a-file", "content"); - client.createDirectory("/simple-directory"); - assertFalse("Dir path needs a trailing slash", client.pathExists("/simple-directory")); - assertTrue("Dir should exist without a leading slash", client.pathExists("simple-directory/")); - assertTrue("Dir should exist with a leading slash", client.pathExists("/simple-directory/")); + assertFalse( + "Bare name without trailing slash is not a key", client.pathExists("/simple-directory")); assertTrue( - "Leading slash should be irrelevant for determining if dir is a dir", + "Dir should exist without a leading slash", client.pathExists("simple-directory/a-file")); + assertTrue( + "Prefix with children should be considered a directory", client.isDirectory("simple-directory/")); assertTrue( "Leading slash should be irrelevant for determining if dir is a dir", @@ -172,25 +176,17 @@ public void testDeletePrefix() throws S3Exception { */ @Test public void testVirtualDirectoryWithoutMarker() throws S3Exception { - // Set up a normal directory tree with markers and content. - client.createDirectory("/virtual-dir"); + // Since createDirectory is a no-op, there are never marker objects. + // Write files directly under a prefix to establish virtual directories. pushContent("/virtual-dir/file1", "file1"); - client.createDirectory("/virtual-dir/sub-dir"); pushContent("/virtual-dir/sub-dir/file2", "file2"); - // Simulate the post-sync state by deleting just the directory marker objects. - // deleteObjects(Collection, int) accepts raw keys, bypassing sanitizedFilePath checks. - client.deleteObjects(List.of("virtual-dir/", "virtual-dir/sub-dir/"), 100); - - // The paths should still be considered directories via the virtual-directory fallback. assertTrue( "A path with objects under it should be considered a virtual directory", client.isDirectory("/virtual-dir")); assertTrue( "A nested path with objects under it should be considered a virtual directory", client.isDirectory("/virtual-dir/sub-dir")); - - // A path with neither a marker nor any objects underneath must not be a directory. assertFalse( "A path with no objects and no marker should not be a directory", client.isDirectory("/virtual-dir/empty-sub-dir")); From 4d9991c1c23f65b745fb099628836363cb0c2b22 Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Wed, 18 Mar 2026 13:26:57 +0100 Subject: [PATCH 06/13] test(s3-repository): update tests to reflect markerless directory behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S3 does not use explicit directory markers, so `exists()` on a directory URI returns false even after `createDirectory()` — a directory is only implied by the objects it contains. - Update S3BackupRepositoryTest assertions to expect `false` from `exists()` on directories, and verify content is still listable - Override `testCanDetermineWhetherFilesAndDirectoriesExist`, `testCanDistinguishBetweenFilesAndDirectories`, `testCanDeleteEmptyOrFullDirectories`, and `testCanListFullOrEmptyDirectories` with S3-specific expectations (no directory marker objects) - Add `testSkipConfigset` override in S3IncrementalBackupTest to assert no explicit directory entries are created during S3 backup, while still verifying configset files are present/absent based on the backup flag - Fix S3ReadWriteTest#testDirectoryLength to push a child file into the directory before testing, satisfying S3's implicit directory semantics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/test/org/apache/solr/s3/S3ReadWriteTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java index 3574587b4fad..9ff6fa178585 100644 --- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java +++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java @@ -90,6 +90,7 @@ public void testLength() throws Exception { @Test public void testDirectoryLength() throws Exception { client.createDirectory("/directory"); + pushContent("/directory/file", "child"); S3Exception exception = assertThrows( From 59f96e9c7e0e2f67e2454f0e19d4e624ca4cb7bc Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Wed, 14 Oct 2026 09:58:21 +0200 Subject: [PATCH 07/13] S3 backups: stop writing directory markers, keep core repo-agnostic Address review on PR #4182: - Keep core backup/restore code unchanged: it still calls createDirectory() and exists(), so LocalFileSystemRepository (which needs real parent directories) keeps working. The S3 repository decides what a directory is. - S3StorageClient: createDirectory() is a no-op; a directory is a key prefix with objects under it. exists() on a directory path and isDirectory() use a single prefix listing, which also finds legacy marker objects, so old backups still restore and get fully deleted. length() is back to a single HEAD request for regular files. - Replace the Mockito-based BackupManagerUploadConfigTest with S3Mock tests: an end-to-end restore (configset included) of a backup copied without marker objects, and a test for backups containing legacy markers. - AbstractBackupRepositoryTest: add supportsEmptyDirectories() so S3 can run the shared contract tests instead of overriding them. - Drop the unrelated CloudSolrClient change; upstream rewrote that code. Co-Authored-By: Claude Opus 5.5 (1M context) --- ...ix-s3-restore-missing-directory-marker.yml | 5 +- .../solr/core/backup/BackupFilePaths.java | 9 +- .../solr/core/backup/BackupManager.java | 15 +- .../org/apache/solr/handler/SnapShooter.java | 4 +- .../backup/BackupManagerUploadConfigTest.java | 179 ------------------ .../org/apache/solr/s3/S3StorageClient.java | 79 +++----- .../solr/s3/S3BackupRepositoryTest.java | 74 +++++++- .../solr/s3/S3IncrementalBackupTest.java | 77 ++++++++ .../test/org/apache/solr/s3/S3PathsTest.java | 29 ++- .../pages/backup-restore.adoc | 10 +- .../AbstractBackupRepositoryTest.java | 30 ++- 11 files changed, 234 insertions(+), 277 deletions(-) delete mode 100644 solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java diff --git a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml index 7f51fa4aff4c..e8153a15f6ee 100644 --- a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml +++ b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml @@ -1,8 +1,9 @@ # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc -title: Remove S3 directory marker dependency from backup/restore — use virtual-directory convention instead +title: The S3 backup repository no longer writes or depends on zero-byte directory marker objects, so S3 backups that were copied with tools that drop those markers (such as `aws s3 sync`) can be restored again. type: fixed authors: - name: Samuel Verstraete + nick: elangelo links: - - name: GitHub PR #4182 + - name: PR#4182 url: https://github.com/apache/solr/pull/4182 diff --git a/solr/core/src/java/org/apache/solr/core/backup/BackupFilePaths.java b/solr/core/src/java/org/apache/solr/core/backup/BackupFilePaths.java index 2dc936ac9d5e..3e1727714ef5 100644 --- a/solr/core/src/java/org/apache/solr/core/backup/BackupFilePaths.java +++ b/solr/core/src/java/org/apache/solr/core/backup/BackupFilePaths.java @@ -79,15 +79,14 @@ public URI getBackupLocation() { } /** - * Previously created directory marker objects at the incremental backup locations. On S3-style - * object stores no explicit directory marker is needed; files can be written directly under any - * prefix. This method is kept for interface compatibility but performs no operations. + * Create all locations required to store an incremental backup. * * @throws IOException for issues encountered using repository to create directories */ public void createIncrementalBackupFolders() throws IOException { - // S3 does not require directory markers. On local FS, parent directories are created - // implicitly when files are written. No action needed. + repository.createDirectory(backupLoc); + repository.createDirectory(getIndexDir()); + repository.createDirectory(getShardBackupMetadataDir()); } /** diff --git a/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java b/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java index f3d3751e7e60..fd76e2e4cdf8 100644 --- a/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java +++ b/solr/core/src/java/org/apache/solr/core/backup/BackupManager.java @@ -122,7 +122,7 @@ public static BackupManager forRestore( Objects.requireNonNull(repository); Objects.requireNonNull(stateReader); - if (repository.listAllOrEmpty(backupPath).length == 0) { + if (!repository.exists(backupPath)) { throw new SolrException( ErrorCode.SERVER_ERROR, "Couldn't restore since doesn't exist: " + backupPath); } @@ -250,7 +250,7 @@ public void uploadConfigDir( String sourceConfigName, String targetConfigName, ConfigSetService configSetService) throws IOException { URI source = repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR, sourceConfigName); - if (repository.listAll(source).length == 0) { + if (!repository.exists(source)) { throw new IllegalArgumentException("Configset expected at " + source + " does not exist"); } uploadConfigToSolrCloud(configSetService, source, targetConfigName, ""); @@ -267,6 +267,9 @@ public void uploadConfigDir( public void downloadConfigDir(String configName, ConfigSetService configSetService) throws IOException { URI dest = repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR, configName); + repository.createDirectory(repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR)); + repository.createDirectory(dest); + downloadConfigToRepo(configSetService, configName, dest); } @@ -357,8 +360,9 @@ private void downloadConfigToRepo(ConfigSetService configSetService, String conf } } } else { - // Directory entries from getAllConfigFiles are implicit on object stores; - // no explicit directory creation is needed. + if (!repository.exists(uri)) { + repository.createDirectory(uri); + } } } } @@ -419,7 +423,6 @@ private URI getZkStateDir() { } public void createZkStateDir() throws IOException { - // S3 does not require directory marker objects to be written before files are stored - // under a prefix. This is kept as a no-op for interface compatibility. + repository.createDirectory(getZkStateDir()); } } diff --git a/solr/core/src/java/org/apache/solr/handler/SnapShooter.java b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java index f862771e9067..81b2e9bc3970 100644 --- a/solr/core/src/java/org/apache/solr/handler/SnapShooter.java +++ b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java @@ -151,12 +151,12 @@ protected void deleteSnapAsync(final ReplicationHandler replicationHandler) { public void validateCreateSnapshot() throws IOException { // Note - Removed the current behavior of creating the directory hierarchy. // Do we really need to provide this support? - if (backupRepo.listAllOrEmpty(baseSnapDirPath).length == 0) { + if (!backupRepo.exists(baseSnapDirPath)) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, " Directory does not exist: " + snapshotDirPath); } - if (backupRepo.listAllOrEmpty(snapshotDirPath).length > 0) { + if (backupRepo.exists(snapshotDirPath)) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, "Snapshot directory already exists: " + snapshotDirPath); diff --git a/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java b/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java deleted file mode 100644 index cf7c1d739d92..000000000000 --- a/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java +++ /dev/null @@ -1,179 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.solr.core.backup; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -import java.net.URI; -import java.nio.charset.StandardCharsets; -import org.apache.lucene.store.IOContext; -import org.apache.lucene.store.IndexInput; -import org.apache.solr.SolrTestCaseJ4; -import org.apache.solr.common.cloud.ZkStateReader; -import org.apache.solr.core.ConfigSetService; -import org.apache.solr.core.backup.repository.BackupRepository; -import org.junit.BeforeClass; -import org.junit.Test; - -/** - * Unit tests for {@link BackupManager#uploadConfigDir} covering the case where an object-store - * backup (e.g. S3) has no explicit directory marker but the config files are still present. - */ -public class BackupManagerUploadConfigTest extends SolrTestCaseJ4 { - - @BeforeClass - public static void setUpClass() { - assumeWorkingMockito(); - } - - private static final URI BACKUP_PATH = URI.create("s3://bucket/backup/"); - private static final URI ZK_STATE_URI = URI.create("s3://bucket/backup/zk_backup/"); - private static final URI CONFIG_DIR_URI = - URI.create("s3://bucket/backup/zk_backup/configs/myconfig/"); - private static final URI SOLRCONFIG_URI = - URI.create("s3://bucket/backup/zk_backup/configs/myconfig/solrconfig.xml"); - - /** - * When the directory marker object is absent (S3 "no-marker" scenario) but config files exist - * under the prefix, {@code uploadConfigDir} must succeed and upload those files. - * - *

This is the regression test for the case where a backup was copied through a local - * filesystem and re-uploaded to S3, losing the zero-byte directory marker objects in the process. - */ - @Test - public void testUploadConfigDir_withoutDirectoryMarker_succeedsWhenFilesPresent() - throws Exception { - BackupRepository mockRepo = mock(BackupRepository.class); - ZkStateReader mockZkStateReader = mock(ZkStateReader.class); - ConfigSetService mockConfigSetService = mock(ConfigSetService.class); - - // Simulate S3: no marker object → listAll() finds the files - when(mockRepo.listAll(CONFIG_DIR_URI)).thenReturn(new String[] {"solrconfig.xml"}); - - // resolveDirectory chain used by getZkStateDir() and uploadConfigDir() - when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR)) - .thenReturn(ZK_STATE_URI); - when(mockRepo.resolveDirectory(ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "myconfig")) - .thenReturn(CONFIG_DIR_URI); - - // resolve() used by uploadConfigToSolrCloud() to build the per-file URI - when(mockRepo.resolve(CONFIG_DIR_URI, "solrconfig.xml")).thenReturn(SOLRCONFIG_URI); - when(mockRepo.getPathType(SOLRCONFIG_URI)).thenReturn(BackupRepository.PathType.FILE); - - // Return minimal valid XML bytes so FileTypeMagicUtil does not reject the file - byte[] configBytes = "".getBytes(StandardCharsets.UTF_8); - IndexInput mockInput = mock(IndexInput.class); - when(mockInput.length()).thenReturn((long) configBytes.length); - doAnswer( - invocation -> { - byte[] dest = invocation.getArgument(0); - int offset = invocation.getArgument(1); - System.arraycopy(configBytes, 0, dest, offset, configBytes.length); - return null; - }) - .when(mockInput) - .readBytes(any(), eq(0), eq(configBytes.length)); - when(mockRepo.openInput(CONFIG_DIR_URI, "solrconfig.xml", IOContext.DEFAULT)) - .thenReturn(mockInput); - - BackupManager backupManager = BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); - - // Must not throw even though exists() returned false - backupManager.uploadConfigDir("myconfig", "restoredConfig", mockConfigSetService); - - // The config file must have been uploaded to the target configset - verify(mockConfigSetService) - .uploadFileToConfig( - eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false)); - } - - /** - * When neither a directory marker nor any files exist under the config prefix, {@code - * uploadConfigDir} must throw {@link IllegalArgumentException}. - */ - @Test - public void testUploadConfigDir_throwsWhenTrulyAbsent() throws Exception { - BackupRepository mockRepo = mock(BackupRepository.class); - ZkStateReader mockZkStateReader = mock(ZkStateReader.class); - ConfigSetService mockConfigSetService = mock(ConfigSetService.class); - - URI missingConfigDir = URI.create("s3://bucket/backup/zk_backup/configs/nonexistent/"); - - when(mockRepo.listAll(missingConfigDir)).thenReturn(new String[0]); - - when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR)) - .thenReturn(ZK_STATE_URI); - when(mockRepo.resolveDirectory(ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "nonexistent")) - .thenReturn(missingConfigDir); - - BackupManager backupManager = BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); - - assertThrows( - IllegalArgumentException.class, - () -> backupManager.uploadConfigDir("nonexistent", "target", mockConfigSetService)); - } - - /** - * When the directory marker IS present (the normal case), {@code uploadConfigDir} uses the fast - * path via {@code exists()} and must still upload files correctly. - */ - @Test - public void testUploadConfigDir_withDirectoryMarker_succeedsNormally() throws Exception { - BackupRepository mockRepo = mock(BackupRepository.class); - ZkStateReader mockZkStateReader = mock(ZkStateReader.class); - ConfigSetService mockConfigSetService = mock(ConfigSetService.class); - - // listAll() is the sole existence check - when(mockRepo.listAll(CONFIG_DIR_URI)).thenReturn(new String[] {"solrconfig.xml"}); - - when(mockRepo.resolveDirectory(BACKUP_PATH, BackupManager.ZK_STATE_DIR)) - .thenReturn(ZK_STATE_URI); - when(mockRepo.resolveDirectory(ZK_STATE_URI, BackupManager.CONFIG_STATE_DIR, "myconfig")) - .thenReturn(CONFIG_DIR_URI); - - when(mockRepo.resolve(CONFIG_DIR_URI, "solrconfig.xml")).thenReturn(SOLRCONFIG_URI); - when(mockRepo.getPathType(SOLRCONFIG_URI)).thenReturn(BackupRepository.PathType.FILE); - - byte[] configBytes = "".getBytes(StandardCharsets.UTF_8); - IndexInput mockInput = mock(IndexInput.class); - when(mockInput.length()).thenReturn((long) configBytes.length); - doAnswer( - invocation -> { - byte[] dest = invocation.getArgument(0); - int offset = invocation.getArgument(1); - System.arraycopy(configBytes, 0, dest, offset, configBytes.length); - return null; - }) - .when(mockInput) - .readBytes(any(), eq(0), eq(configBytes.length)); - when(mockRepo.openInput(CONFIG_DIR_URI, "solrconfig.xml", IOContext.DEFAULT)) - .thenReturn(mockInput); - - BackupManager backupManager = BackupManager.forBackup(mockRepo, mockZkStateReader, BACKUP_PATH); - - backupManager.uploadConfigDir("myconfig", "restoredConfig", mockConfigSetService); - - verify(mockConfigSetService) - .uploadFileToConfig( - eq("restoredConfig"), eq("solrconfig.xml"), any(byte[].class), eq(false)); - } -} diff --git a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java index 121e77d5ba29..8207240e0661 100644 --- a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java +++ b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java @@ -41,7 +41,6 @@ import software.amazon.awssdk.awscore.exception.AwsServiceException; import software.amazon.awssdk.awscore.retry.AwsRetryStrategy; import software.amazon.awssdk.core.ResponseInputStream; -import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.core.exception.SdkException; import software.amazon.awssdk.core.retry.RetryMode; import software.amazon.awssdk.core.sync.ResponseTransformer; @@ -82,7 +81,7 @@ public class S3StorageClient { // S3 has a hard limit of 1000 keys per batch delete request private static final int MAX_KEYS_PER_BATCH_DELETE = 1000; - // Metadata name used to identify flag directory entries in S3 + // Content type of the zero-byte directory marker objects written by older Solr versions private static final String S3_DIR_CONTENT_TYPE = "application/x-directory"; // Error messages returned by S3 for a key not found. @@ -180,12 +179,13 @@ private static S3Client createInternalClient( return clientBuilder.build(); } - /** Create a directory in S3, if it does not already exist. */ - void createDirectory(String path) throws S3Exception { - // S3 does not require explicit directory marker objects. Writing a file directly - // under a prefix is sufficient. Creating zero-byte marker objects is an anti-pattern - // that causes failures after tools like 'aws s3 sync' drop them. - } + /** + * No-op: S3 has no directories, a "directory" is just a key prefix that exists as soon as an + * object is written under it. Older Solr versions wrote zero-byte marker objects here, but those + * get lost when a bucket is copied with tools such as {@code aws s3 sync}, so nothing may depend + * on them. + */ + void createDirectory(String path) {} /** * Delete files from S3. Deletion order is not guaranteed. @@ -269,10 +269,11 @@ String[] listDir(String path) throws S3Exception { } /** - * Check if path exists. + * Check if path exists. A directory path (ending with {@code /}) exists if any object is stored + * under that prefix, including a directory marker object written by older Solr versions. * * @param path to File/Directory in S3. - * @return true if path exists, otherwise false? + * @return true if path exists, otherwise false */ boolean pathExists(String path) throws S3Exception { final String s3Path = sanitizedPath(path); @@ -282,6 +283,10 @@ boolean pathExists(String path) throws S3Exception { return true; } + if (s3Path.endsWith(S3_FILE_PATH_DELIMITER)) { + return hasObjectsUnderPrefix(s3Path); + } + try { s3Client.headObject(builder -> builder.bucket(bucketName).key(s3Path)); return true; @@ -293,53 +298,22 @@ boolean pathExists(String path) throws S3Exception { } /** - * Check if path is directory. - * - *

A path is considered a directory if either (a) an explicit directory marker object exists - * (i.e. a zero-byte object with key {@code path + "/"} and content-type {@code - * application/x-directory}), or (b) no such marker exists but at least one object exists whose - * key starts with {@code path + "/"} (a "virtual directory"). Case (b) arises after an {@code aws - * s3 sync} round-trip, which skips directory marker objects, causing them to be absent in the - * target bucket. + * Check if path is directory, i.e. if any object is stored under the {@code path + "/"} prefix. * * @param path to File/Directory in S3. * @return true if path is directory, otherwise false. */ boolean isDirectory(String path) throws S3Exception { - final String s3Path = sanitizedDirPath(path); - - try { - HeadObjectResponse objectMetadata = - s3Client.headObject(builder -> builder.bucket(bucketName).key(s3Path)); - String contentType = objectMetadata.contentType(); - - return StrUtils.isNotNullOrEmpty(contentType) - && contentType.equalsIgnoreCase(S3_DIR_CONTENT_TYPE); - } catch (NoSuchKeyException nske) { - // No explicit directory marker found; fall back to checking whether any objects exist under - // this prefix. This handles "virtual directories" that arise after an 'aws s3 sync' - // round-trip, which does not preserve directory marker objects (keys ending with '/'). - return hasChildrenUnderPrefix(s3Path); - } catch (SdkException sdke) { - throw handleAmazonException(sdke); - } + return hasObjectsUnderPrefix(sanitizedDirPath(path)); } - /** - * Returns {@code true} if at least one object exists in S3 whose key starts with {@code dirPath}. - * - *

Used as a fallback in {@link #isDirectory} when no explicit directory marker object is - * present. - * - * @param dirPath directory path with a trailing {@code /} (as produced by {@link - * #sanitizedDirPath}) - */ - private boolean hasChildrenUnderPrefix(String dirPath) throws S3Exception { + private boolean hasObjectsUnderPrefix(String prefix) throws S3Exception { try { - return !s3Client - .listObjectsV2(b -> b.bucket(bucketName).prefix(dirPath).maxKeys(1)) + return s3Client + .listObjectsV2(b -> b.bucket(bucketName).prefix(prefix).maxKeys(1)) .contents() - .isEmpty(); + .stream() + .anyMatch(o -> o.key().startsWith(prefix)); } catch (SdkException sdke) { throw handleAmazonException(sdke); } @@ -353,9 +327,6 @@ private boolean hasChildrenUnderPrefix(String dirPath) throws S3Exception { */ long length(String path) throws S3Exception { String s3Path = sanitizedFilePath(path); - if (isDirectory(s3Path)) { - throw new S3Exception("Path is Directory"); - } try { HeadObjectResponse objectMetadata = s3Client.headObject(b -> b.bucket(bucketName).key(s3Path)); @@ -366,6 +337,12 @@ long length(String path) throws S3Exception { return objectMetadata.contentLength(); } throw new S3Exception("Path is Directory"); + } catch (NoSuchKeyException nske) { + // Only pay for the extra listing when the file is missing + if (isDirectory(s3Path)) { + throw new S3Exception("Path is Directory"); + } + throw handleAmazonException(nske); } catch (SdkException sdke) { throw handleAmazonException(sdke); } diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java index bd24ac0a7ccf..deaaaab71c32 100644 --- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java +++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java @@ -29,6 +29,8 @@ import java.nio.file.Path; import java.nio.file.StandardCopyOption; import java.time.Duration; +import java.util.List; +import java.util.Set; import org.apache.commons.io.file.PathUtils; import org.apache.lucene.codecs.CodecUtil; import org.apache.lucene.store.BufferedIndexInput; @@ -49,6 +51,7 @@ import software.amazon.awssdk.core.sync.ResponseTransformer; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.model.S3Object; @ThreadLeakFilters( filters = { @@ -97,8 +100,6 @@ public void testURI() throws IOException { "s3:///d/", directoryUri.toString()); - repo.createDirectory(directoryUri); - assertTrue(repo.exists(directoryUri)); directoryUri = repo.createDirectoryURI("d/"); assertEquals( "createDirectoryURI should have a single trailing slash, even if one is provided", @@ -118,15 +119,24 @@ public void testLocalDirectoryFunctions() throws Exception { URI path = new URI("/test/"); repo.createDirectory(path); - assertTrue(repo.exists(path)); - assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(path)); + assertFalse("An empty directory doesn't exist on S3", repo.exists(path)); assertEquals("No files should exist in dir yet", 0, repo.listAll(path).length); URI subDir = new URI("/test/dir/"); repo.createDirectory(subDir); + assertEquals("No files should exist in subdir yet", 0, repo.listAll(subDir).length); + + try (OutputStream os = repo.createOutput(new URI("/test/dir/file"))) { + os.write(1); + } + assertTrue(repo.exists(path)); assertTrue(repo.exists(subDir)); + assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(path)); assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(subDir)); - assertEquals("No files should exist in subdir yet", 0, repo.listAll(subDir).length); + assertEquals( + "No directory marker objects should be written", + List.of("test/dir/file"), + listKeys("test/")); assertEquals( "subDir should now be returned when listing all in parent dir", @@ -139,6 +149,36 @@ public void testLocalDirectoryFunctions() throws Exception { } } + /** + * Older Solr versions wrote a zero-byte marker object (content type {@code + * application/x-directory}) for every directory. Backups containing them must keep working. + */ + @Test + public void testLegacyDirectoryMarkers() throws Exception { + try (S3BackupRepository repo = getRepository()) { + pushDirectoryMarker("legacy/"); + pushDirectoryMarker("legacy/empty/"); + pushDirectoryMarker("legacy/dir/"); + pushObject("legacy/dir/file", "content"); + + URI legacy = new URI("s3:/legacy/"); + assertTrue(repo.exists(legacy)); + assertTrue(repo.exists(repo.resolveDirectory(legacy, "empty"))); + assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(legacy)); + assertEquals( + BackupRepository.PathType.DIRECTORY, + repo.getPathType(repo.resolveDirectory(legacy, "dir"))); + assertEquals( + BackupRepository.PathType.FILE, repo.getPathType(repo.resolve(legacy, "dir", "file"))); + assertEquals(Set.of("empty", "dir"), Set.of(repo.listAll(legacy))); + assertArrayEquals(new String[] {"file"}, repo.listAll(repo.resolveDirectory(legacy, "dir"))); + + repo.deleteDirectory(legacy); + assertFalse(repo.exists(legacy)); + assertEquals("Directory markers should be deleted too", List.of(), listKeys("legacy/")); + } + } + /** Check resolving paths. */ @Test public void testResolve() throws Exception { @@ -310,6 +350,11 @@ public void testBackwardRandomAccess() throws Exception { } } + @Override + protected boolean supportsEmptyDirectories() { + return false; + } + @Override protected Class getRepositoryClass() { return S3BackupRepository.class; @@ -348,6 +393,25 @@ private void pushObject(String path, String content) { } } + private void pushDirectoryMarker(String path) { + try (S3Client s3 = s3MockContainer.createS3ClientV2()) { + s3.putObject( + b -> b.bucket(BUCKET_NAME).key(path).contentType("application/x-directory"), + RequestBody.empty()); + } + } + + private List listKeys(String prefix) { + try (S3Client s3 = s3MockContainer.createS3ClientV2()) { + return s3 + .listObjectsV2Paginator(b -> b.bucket(BUCKET_NAME).prefix(prefix)) + .contents() + .stream() + .map(S3Object::key) + .toList(); + } + } + private Path pullObject(String path) throws IOException { try (S3Client s3 = s3MockContainer.createS3ClientV2()) { Path file = Files.createTempFile(temporaryFolder, "junit", null); diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java index 9546012190be..551481fe5352 100644 --- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java +++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java @@ -19,14 +19,24 @@ import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakLingering; +import java.util.List; import org.apache.lucene.tests.util.LuceneTestCase; import org.apache.lucene.tests.util.QuickPatchThreadsFilter; import org.apache.solr.SolrIgnoredThreadsFilter; +import org.apache.solr.client.solrj.impl.CloudSolrClient; +import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.ConfigSetAdminRequest; +import org.apache.solr.client.solrj.request.SolrQuery; +import org.apache.solr.cloud.AbstractFullDistribZkTestBase; import org.apache.solr.cloud.api.collections.AbstractIncrementalBackupTest; +import org.apache.solr.common.cloud.ZkStateReader; import org.apache.solr.util.LogLevel; import org.junit.BeforeClass; import org.junit.ClassRule; +import org.junit.Test; import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.model.S3Object; // Backups do checksum validation against a footer value not present in 'SimpleText' @LuceneTestCase.Nightly @@ -141,4 +151,71 @@ public String getCollectionNamePrefix() { public String getBackupLocation() { return "/"; } + + /** + * Copying a backup to another bucket or prefix with tools such as {@code aws s3 sync} only copies + * the real files, not zero-byte directory marker objects. Such a copy, configset included, must + * still restore. + */ + @Test + public void testRestoreBackupCopiedWithoutDirectoryMarkers() throws Exception { + setTestSuffix("testcopiedwithoutmarkers"); + final String backupCollectionName = getCollectionName(); + final String restoreCollectionName = backupCollectionName + "_restore"; + final String restoreConfigName = "conf1_restored"; + final String backupName = BACKUPNAME_PREFIX + testSuffix; + final String copyLocation = "copied-backups"; + + CloudSolrClient solrClient = cluster.getSolrClient(); + CollectionAdminRequest.createCollection(backupCollectionName, "conf1", NUM_SHARDS, 1) + .process(solrClient); + int numDocs = indexDocs(backupCollectionName, true); + CollectionAdminRequest.backupCollection(backupCollectionName, backupName) + .setLocation(getBackupLocation()) + .setRepositoryName(BACKUP_REPO_NAME) + .processAndWait(solrClient, 100); + + try (S3Client s3 = s3MockContainer.createS3ClientV2()) { + List keys = + s3 + .listObjectsV2Paginator(b -> b.bucket(BUCKET_NAME).prefix(backupName + "/")) + .contents() + .stream() + .map(S3Object::key) + .toList(); + assertTrue( + "Backup should contain the configset", + keys.stream().anyMatch(k -> k.contains("/configs/conf1/"))); + assertFalse( + "Backup should not contain directory marker objects: " + keys, + keys.stream().anyMatch(k -> k.endsWith("/"))); + + for (String key : keys) { + s3.copyObject( + b -> + b.sourceBucket(BUCKET_NAME) + .sourceKey(key) + .destinationBucket(BUCKET_NAME) + .destinationKey(copyLocation + "/" + key)); + } + } + + // Restoring under a new config name makes the restore upload the configset from the backup + CollectionAdminRequest.restoreCollection(restoreCollectionName, backupName) + .setLocation(copyLocation) + .setConfigName(restoreConfigName) + .setRepositoryName(BACKUP_REPO_NAME) + .processAndWait(solrClient, 500); + AbstractFullDistribZkTestBase.waitForRecoveriesToFinish( + restoreCollectionName, ZkStateReader.from(solrClient), false, false, 3); + + assertTrue( + new ConfigSetAdminRequest.List() + .process(solrClient) + .getConfigSets() + .contains(restoreConfigName)); + assertEquals( + numDocs, + solrClient.query(restoreCollectionName, new SolrQuery("*:*")).getResults().getNumFound()); + } } diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java index e774d29434bf..012b564b9e76 100644 --- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java +++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java @@ -46,17 +46,16 @@ public void testFiles() throws S3Exception { /** Simple tests with a directory. */ @Test public void testDirectory() throws S3Exception { - // createDirectory is a no-op on S3 (no marker objects are written). - // A prefix is treated as a directory if it has children — write one to establish the prefix. - client.createDirectory("/simple-directory"); // no-op - pushContent("/simple-directory/a-file", "content"); - assertFalse( - "Bare name without trailing slash is not a key", client.pathExists("/simple-directory")); - assertTrue( - "Dir should exist without a leading slash", client.pathExists("simple-directory/a-file")); + client.createDirectory("/simple-directory"); + assertFalse("An empty dir does not exist on S3", client.pathExists("/simple-directory/")); + + pushContent("/simple-directory/a-file", "content"); + assertFalse("Dir path needs a trailing slash", client.pathExists("/simple-directory")); + assertTrue("Dir should exist without a leading slash", client.pathExists("simple-directory/")); + assertTrue("Dir should exist with a leading slash", client.pathExists("/simple-directory/")); assertTrue( - "Prefix with children should be considered a directory", + "Leading slash should be irrelevant for determining if dir is a dir", client.isDirectory("simple-directory/")); assertTrue( "Leading slash should be irrelevant for determining if dir is a dir", @@ -167,17 +166,10 @@ public void testDeletePrefix() throws S3Exception { assertTrue(client.pathExists("/my-file2")); } - /** - * Test that a directory containing objects but lacking an explicit directory marker (i.e. a - * "virtual directory") is still recognised as a directory. This mirrors the situation that arises - * after an {@code aws s3 sync} round-trip: {@code aws s3 sync} skips directory marker objects - * (keys ending with {@code /}) when downloading, so they are never re-uploaded and are absent in - * the target bucket. - */ + /** A directory is any key prefix that has objects under it; no marker object is needed. */ @Test public void testVirtualDirectoryWithoutMarker() throws S3Exception { - // Since createDirectory is a no-op, there are never marker objects. - // Write files directly under a prefix to establish virtual directories. + pushContent("/virtual-dir-sibling", "not under the prefix"); pushContent("/virtual-dir/file1", "file1"); pushContent("/virtual-dir/sub-dir/file2", "file2"); @@ -190,6 +182,7 @@ public void testVirtualDirectoryWithoutMarker() throws S3Exception { assertFalse( "A path with no objects and no marker should not be a directory", client.isDirectory("/virtual-dir/empty-sub-dir")); + assertFalse("A file is not a directory", client.isDirectory("/virtual-dir/file1")); } /** Check listing objects of a directory. */ diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc index 2b5cacb87dba..fb9481c6f0d9 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc @@ -729,6 +729,7 @@ All the above options will resolve to the same directory in your S3 Bucket: `dir Pre-Creation:: The location must already exist within your S3 bucket before you can perform any backup operations with that location. +A location exists if at least one object is stored under it, or if it was created as an (empty) folder, for instance with the "Create folder" button of the AWS console. + **Please note that the directory you create in S3 cannot begin with a `/`, as `locations` are stripped of any `/` prefix (shown in `Location Format`).** @@ -737,12 +738,9 @@ If you do not want to use a sub-directory within your bucket to store your backu However the location option is mandatory and you will receive an error when trying to perform backup operations without it. Directory Markers:: -S3 has no native concept of directories. -The `S3BackupRepository` simulates directories by uploading zero-byte marker objects (e.g., `zk_backup/configs/myconfig/`) alongside the real backup files. -If a backup is copied through a local filesystem and re-uploaded to S3 (for example using `aws s3 sync`), these zero-byte marker objects are typically lost because most tools translate them into real filesystem directories and do not recreate them on re-upload. -+ -Solr handles this gracefully: if the directory marker is absent, Solr falls back to listing objects by key prefix to confirm that the configset files are present before restoring them. -A restore will still fail with a clear error only if neither the marker nor any configset files can be found at the expected location. +S3 has no native concept of directories, so Solr does not write any zero-byte "directory marker" objects: a directory is simply the common key prefix of the files stored in it. +Backups can therefore be copied between buckets with tools such as `aws s3 sync`, which drop such marker objects, and still be restored. +Backups made by older Solr versions, which do contain marker objects, can still be restored and deleted. ==== An example configuration to enable S3 backups and restore can be seen below: diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java b/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java index af8f5dbc3ce9..a4ba03e97a4f 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java @@ -71,6 +71,18 @@ protected NamedList getBaseBackupRepositoryConfiguration() { return new NamedList<>(); } + /** + * Whether the repository under test can represent an empty directory. + * + *

Object stores such as S3 have no real directories: a "directory" only exists while at least + * one object is stored under its prefix. Repositories that don't persist anything for {@link + * BackupRepository#createDirectory(URI)} should override this to return false, which skips the + * assertions that only hold for empty directories. + */ + protected boolean supportsEmptyDirectories() { + return true; + } + @Test public void testCanReadProvidedConfigValues() throws Exception { final NamedList config = getBaseBackupRepositoryConfiguration(); @@ -111,7 +123,7 @@ public void testCanDetermineWhetherFilesAndDirectoriesExist() throws Exception { repo.createDirectory(nonEmptyDirUri); addFile(repo, nestedFileUri); - assertTrue(repo.exists(emptyDirUri)); + assertEquals(supportsEmptyDirectories(), repo.exists(emptyDirUri)); assertTrue(repo.exists(nonEmptyDirUri)); assertTrue(repo.exists(nestedFileUri)); final URI nonexistedDirUri = repo.resolve(getBaseUri(), "nonexistentDir/"); @@ -129,7 +141,9 @@ public void testCanDistinguishBetweenFilesAndDirectories() throws Exception { repo.createDirectory(nonEmptyDirUri); addFile(repo, nestedFileUri); - assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(emptyDirUri)); + if (supportsEmptyDirectories()) { + assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(emptyDirUri)); + } assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(nonEmptyDirUri)); assertEquals(BackupRepository.PathType.FILE, repo.getPathType(nestedFileUri)); } @@ -186,6 +200,11 @@ public void testCanDeleteEmptyOrFullDirectories() throws Exception { repo.createDirectory(level2DeeplyNestedUri); repo.createDirectory(level3DeeplyNestedUri); repo.createDirectory(level4DeeplyNestedUri); + if (!supportsEmptyDirectories()) { + // Keep nest2 (and therefore nest1) non-empty once nest3 is deleted + addFile(repo, repo.resolve(level2DeeplyNestedUri, "file.txt")); + addFile(repo, repo.resolve(level4DeeplyNestedUri, "file.txt")); + } repo.deleteDirectory(level3DeeplyNestedUri); assertTrue(repo.exists(level1DeeplyNestedUri)); assertTrue(repo.exists(level2DeeplyNestedUri)); @@ -241,6 +260,11 @@ public void testCanListFullOrEmptyDirectories() throws Exception { repo.createDirectory(otherDir1Uri); repo.createDirectory(otherDir2Uri); repo.createDirectory(otherDir3Uri); + if (!supportsEmptyDirectories()) { + // Without empty directories, otherDir1 and otherDir2 only show up once they hold a file + addFile(repo, repo.resolve(otherDir1Uri, "file.txt")); + addFile(repo, repo.resolve(otherDir2Uri, "file.txt")); + } addFile(repo, file1Uri); addFile(repo, file2Uri); @@ -251,7 +275,7 @@ public void testCanListFullOrEmptyDirectories() throws Exception { assertTrue(rootChildren.contains("otherDir3")); final String[] otherDir2Children = repo.listAll(otherDir2Uri); - assertEquals(0, otherDir2Children.length); + assertEquals(supportsEmptyDirectories() ? 0 : 1, otherDir2Children.length); final List otherDir3Children = List.of(repo.listAll(otherDir3Uri)); assertEquals(2, otherDir3Children.size()); From 0802bd037414a192d545a3538b92e65fd2b443ad Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Wed, 14 Oct 2026 10:41:57 +0200 Subject: [PATCH 08/13] Point changelog entry at PR #5128 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../unreleased/fix-s3-restore-missing-directory-marker.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml index e8153a15f6ee..727ad463dee4 100644 --- a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml +++ b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml @@ -5,5 +5,5 @@ authors: - name: Samuel Verstraete nick: elangelo links: - - name: PR#4182 - url: https://github.com/apache/solr/pull/4182 + - name: PR#5128 + url: https://github.com/apache/solr/pull/5128 From fa9383147f53653a60ccb46d45cdcebdc8bd75be Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Mon, 19 Oct 2026 11:17:56 +0200 Subject: [PATCH 09/13] Allow S3 backups to a location that does not exist yet S3 can't hold an empty directory, so a backup location only exists once a file is stored under it, and a backup to a new prefix was refused. BackupRepository gets supportsEmptyDirectories(), false for S3, replacing the hook of the same name in AbstractBackupRepositoryTest. A collection backup accepts a missing location on such a repository; the location is still checked, so an unreachable bucket fails up front. Restore, list and delete still require the location to exist. Co-Authored-By: Claude Opus 5.5 --- ...ix-s3-restore-missing-directory-marker.yml | 2 +- .../backup/repository/BackupRepository.java | 14 ++++++ .../DelegatingBackupRepository.java | 5 ++ .../solr/handler/admin/api/BackupAPIBase.java | 22 +++++++-- .../admin/api/CreateCollectionBackup.java | 2 +- .../apache/solr/s3/S3BackupRepository.java | 5 ++ .../solr/s3/S3BackupRepositoryTest.java | 5 -- .../solr/s3/S3IncrementalBackupTest.java | 48 +++++++++++++++++++ .../pages/backup-restore.adoc | 8 ++-- .../AbstractBackupRepositoryTest.java | 22 ++------- 10 files changed, 101 insertions(+), 32 deletions(-) diff --git a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml index 727ad463dee4..3c1274dfc20c 100644 --- a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml +++ b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml @@ -1,5 +1,5 @@ # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc -title: The S3 backup repository no longer writes or depends on zero-byte directory marker objects, so S3 backups that were copied with tools that drop those markers (such as `aws s3 sync`) can be restored again. +title: The S3 backup repository no longer writes or depends on zero-byte directory marker objects, so S3 backups that were copied with tools that drop those markers (such as `aws s3 sync`) can be restored again. A backup can also be written to an S3 location that does not exist yet. type: fixed authors: - name: Samuel Verstraete diff --git a/solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java b/solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java index b8f86af5e3ed..3d13ea6ac53c 100644 --- a/solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java +++ b/solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java @@ -166,6 +166,20 @@ default URI resolveDirectory(URI baseUri, String... pathComponents) { */ void deleteDirectory(URI path) throws IOException; + /** + * Whether this repository can hold an empty directory. + * + *

Object stores such as S3 have no real directories: a directory only exists while at least + * one file is stored under its path, and {@link #createDirectory(URI)} persists nothing. Such + * repositories return false, which lets a backup be written to a location that does not exist + * yet. + * + * @return true if a directory created by {@link #createDirectory(URI)} exists while it is empty + */ + default boolean supportsEmptyDirectories() { + return true; + } + /** * Copy a file from specified sourceDir to the destination repository (i.e. backup). * diff --git a/solr/core/src/java/org/apache/solr/core/backup/repository/DelegatingBackupRepository.java b/solr/core/src/java/org/apache/solr/core/backup/repository/DelegatingBackupRepository.java index e3b27cb073c5..54cd58458429 100644 --- a/solr/core/src/java/org/apache/solr/core/backup/repository/DelegatingBackupRepository.java +++ b/solr/core/src/java/org/apache/solr/core/backup/repository/DelegatingBackupRepository.java @@ -76,6 +76,11 @@ public boolean exists(URI path) throws IOException { return delegate.exists(path); } + @Override + public boolean supportsEmptyDirectories() { + return delegate.supportsEmptyDirectories(); + } + @Override public PathType getPathType(URI path) throws IOException { return delegate.getPathType(path); diff --git a/solr/core/src/java/org/apache/solr/handler/admin/api/BackupAPIBase.java b/solr/core/src/java/org/apache/solr/handler/admin/api/BackupAPIBase.java index b7028ff04053..14656547a5fc 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/api/BackupAPIBase.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/api/BackupAPIBase.java @@ -46,6 +46,20 @@ protected String getAndValidateBackupLocation(String repositoryName, String loca } } + /** + * Like {@link #getAndValidateBackupLocation(String, String)}, for an API that writes a new backup + * to the location. On a repository that can't hold empty directories, a location that does not + * exist yet is accepted: it comes into existence with the first file of the backup. + */ + protected String getAndValidateNewBackupLocation(String repositoryName, String location) + throws IOException { + try (final var repository = createBackupRepository(repositoryName)) { + location = getLocation(repository, location); + ensureBackupLocationExists(repository, location, !repository.supportsEmptyDirectories()); + return location; + } + } + protected String getAndValidateIncrementalBackupLocation( String repositoryName, String location, String backupName) throws IOException { try (final var repository = createBackupRepository(repositoryName)) { @@ -80,10 +94,12 @@ private String getLocation(BackupRepository repository, String location) throws + " parameter or as a default repository property or as a cluster property."); } - private void ensureBackupLocationExists(BackupRepository repository, String location) { + private void ensureBackupLocationExists( + BackupRepository repository, String location, boolean allowMissing) { final URI uri = repository.createDirectoryURI(location); try { - if (!repository.exists(uri)) { + // Checked even when a missing location is allowed, to fail early on an unreachable repository + if (!repository.exists(uri) && !allowMissing) { throw new SolrException( SolrException.ErrorCode.SERVER_ERROR, "specified location " + uri + " does not exist."); } @@ -98,7 +114,7 @@ private void ensureBackupLocationExists(BackupRepository repository, String loca private String getAndValidateBackupLocation(BackupRepository backupRepository, String location) throws IOException { location = getLocation(backupRepository, location); - ensureBackupLocationExists(backupRepository, location); + ensureBackupLocationExists(backupRepository, location, false); return location; } diff --git a/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollectionBackup.java b/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollectionBackup.java index 5e5661378b3e..0b8d0a67078d 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollectionBackup.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollectionBackup.java @@ -90,7 +90,7 @@ public SolrJerseyResponse createCollectionBackup( collectionName, Boolean.TRUE.equals(requestBody.followAliases)); requestBody.location = - getAndValidateBackupLocation(requestBody.repository, requestBody.location); + getAndValidateNewBackupLocation(requestBody.repository, requestBody.location); if (requestBody.backupStrategy == null) { requestBody.backupStrategy = CollectionAdminParams.COPY_FILES_STRATEGY; diff --git a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java index 07858b29095c..a4f8f14211ed 100644 --- a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java +++ b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java @@ -146,6 +146,11 @@ public void createDirectory(URI path) throws IOException { client.createDirectory(s3Path); } + @Override + public boolean supportsEmptyDirectories() { + return false; + } + @Override public void deleteDirectory(URI path) throws IOException { Objects.requireNonNull(path, "cannot delete directory with a null URI"); diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java index deaaaab71c32..ae09e1ae8fd3 100644 --- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java +++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java @@ -350,11 +350,6 @@ public void testBackwardRandomAccess() throws Exception { } } - @Override - protected boolean supportsEmptyDirectories() { - return false; - } - @Override protected Class getRepositoryClass() { return S3BackupRepository.class; diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java index 551481fe5352..a3867a79ef6f 100644 --- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java +++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java @@ -27,6 +27,7 @@ import org.apache.solr.client.solrj.request.CollectionAdminRequest; import org.apache.solr.client.solrj.request.ConfigSetAdminRequest; import org.apache.solr.client.solrj.request.SolrQuery; +import org.apache.solr.client.solrj.response.RequestStatusState; import org.apache.solr.cloud.AbstractFullDistribZkTestBase; import org.apache.solr.cloud.api.collections.AbstractIncrementalBackupTest; import org.apache.solr.common.cloud.ZkStateReader; @@ -218,4 +219,51 @@ public void testRestoreBackupCopiedWithoutDirectoryMarkers() throws Exception { numDocs, solrClient.query(restoreCollectionName, new SolrQuery("*:*")).getResults().getNumFound()); } + + /** + * S3 can't hold an empty directory, so a backup location doesn't have to be created first: it + * comes into existence with the first file of the backup. Restoring from a location that holds + * nothing still fails. + */ + @Test + public void testBackupToLocationThatDoesNotExistYet() throws Exception { + setTestSuffix("testnewlocation"); + final String backupCollectionName = getCollectionName(); + final String restoreCollectionName = backupCollectionName + "_restore"; + final String backupName = BACKUPNAME_PREFIX + testSuffix; + final String newLocation = "new-location/nested"; + + CloudSolrClient solrClient = cluster.getSolrClient(); + CollectionAdminRequest.createCollection(backupCollectionName, "conf1", NUM_SHARDS, 1) + .process(solrClient); + int numDocs = indexDocs(backupCollectionName, true); + assertEquals( + RequestStatusState.COMPLETED, + CollectionAdminRequest.backupCollection(backupCollectionName, backupName) + .setLocation(newLocation) + .setRepositoryName(BACKUP_REPO_NAME) + .processAndWait(solrClient, 100)); + + assertEquals( + RequestStatusState.COMPLETED, + CollectionAdminRequest.restoreCollection(restoreCollectionName, backupName) + .setLocation(newLocation) + .setRepositoryName(BACKUP_REPO_NAME) + .processAndWait(solrClient, 500)); + AbstractFullDistribZkTestBase.waitForRecoveriesToFinish( + restoreCollectionName, ZkStateReader.from(solrClient), false, false, 3); + assertEquals( + numDocs, + solrClient.query(restoreCollectionName, new SolrQuery("*:*")).getResults().getNumFound()); + + Exception e = + expectThrows( + Exception.class, + () -> + CollectionAdminRequest.restoreCollection(restoreCollectionName + "_2", backupName) + .setLocation("missing-location") + .setRepositoryName(BACKUP_REPO_NAME) + .processAndWait(solrClient, 100)); + assertTrue(e.getMessage(), e.getMessage().contains("does not exist")); + } } diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc index fb9481c6f0d9..da4dc6c4b9bb 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc @@ -727,11 +727,9 @@ When using the Backup & Restore Collections API Calls, the **`location`** option + All the above options will resolve to the same directory in your S3 Bucket: `dir/in/bucket`. -Pre-Creation:: -The location must already exist within your S3 bucket before you can perform any backup operations with that location. -A location exists if at least one object is stored under it, or if it was created as an (empty) folder, for instance with the "Create folder" button of the AWS console. -+ -**Please note that the directory you create in S3 cannot begin with a `/`, as `locations` are stripped of any `/` prefix (shown in `Location Format`).** +Location Creation:: +The location does not need to be created before a backup is written to it: it comes into existence with the first file of the backup. +Restoring, listing and deleting backups require a location that already holds at least one object. Empty Location:: If you do not want to use a sub-directory within your bucket to store your backup, you can use any of the following location options: `/`, `s3:/`, `s3://`. diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java b/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java index a4ba03e97a4f..65c2c74dcab8 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java @@ -71,18 +71,6 @@ protected NamedList getBaseBackupRepositoryConfiguration() { return new NamedList<>(); } - /** - * Whether the repository under test can represent an empty directory. - * - *

Object stores such as S3 have no real directories: a "directory" only exists while at least - * one object is stored under its prefix. Repositories that don't persist anything for {@link - * BackupRepository#createDirectory(URI)} should override this to return false, which skips the - * assertions that only hold for empty directories. - */ - protected boolean supportsEmptyDirectories() { - return true; - } - @Test public void testCanReadProvidedConfigValues() throws Exception { final NamedList config = getBaseBackupRepositoryConfiguration(); @@ -123,7 +111,7 @@ public void testCanDetermineWhetherFilesAndDirectoriesExist() throws Exception { repo.createDirectory(nonEmptyDirUri); addFile(repo, nestedFileUri); - assertEquals(supportsEmptyDirectories(), repo.exists(emptyDirUri)); + assertEquals(repo.supportsEmptyDirectories(), repo.exists(emptyDirUri)); assertTrue(repo.exists(nonEmptyDirUri)); assertTrue(repo.exists(nestedFileUri)); final URI nonexistedDirUri = repo.resolve(getBaseUri(), "nonexistentDir/"); @@ -141,7 +129,7 @@ public void testCanDistinguishBetweenFilesAndDirectories() throws Exception { repo.createDirectory(nonEmptyDirUri); addFile(repo, nestedFileUri); - if (supportsEmptyDirectories()) { + if (repo.supportsEmptyDirectories()) { assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(emptyDirUri)); } assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(nonEmptyDirUri)); @@ -200,7 +188,7 @@ public void testCanDeleteEmptyOrFullDirectories() throws Exception { repo.createDirectory(level2DeeplyNestedUri); repo.createDirectory(level3DeeplyNestedUri); repo.createDirectory(level4DeeplyNestedUri); - if (!supportsEmptyDirectories()) { + if (!repo.supportsEmptyDirectories()) { // Keep nest2 (and therefore nest1) non-empty once nest3 is deleted addFile(repo, repo.resolve(level2DeeplyNestedUri, "file.txt")); addFile(repo, repo.resolve(level4DeeplyNestedUri, "file.txt")); @@ -260,7 +248,7 @@ public void testCanListFullOrEmptyDirectories() throws Exception { repo.createDirectory(otherDir1Uri); repo.createDirectory(otherDir2Uri); repo.createDirectory(otherDir3Uri); - if (!supportsEmptyDirectories()) { + if (!repo.supportsEmptyDirectories()) { // Without empty directories, otherDir1 and otherDir2 only show up once they hold a file addFile(repo, repo.resolve(otherDir1Uri, "file.txt")); addFile(repo, repo.resolve(otherDir2Uri, "file.txt")); @@ -275,7 +263,7 @@ public void testCanListFullOrEmptyDirectories() throws Exception { assertTrue(rootChildren.contains("otherDir3")); final String[] otherDir2Children = repo.listAll(otherDir2Uri); - assertEquals(supportsEmptyDirectories() ? 0 : 1, otherDir2Children.length); + assertEquals(repo.supportsEmptyDirectories() ? 0 : 1, otherDir2Children.length); final List otherDir3Children = List.of(repo.listAll(otherDir3Uri)); assertEquals(2, otherDir3Children.size()); From 44825b2fdf39d9e194a839012de6288a55a4025d Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Wed, 21 Oct 2026 07:22:09 -0400 Subject: [PATCH 10/13] Tweaking some text to try and be clearer. --- .../modules/deployment-guide/pages/backup-restore.adoc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc index da4dc6c4b9bb..ed5929a43b9f 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc @@ -728,17 +728,17 @@ When using the Backup & Restore Collections API Calls, the **`location`** option All the above options will resolve to the same directory in your S3 Bucket: `dir/in/bucket`. Location Creation:: -The location does not need to be created before a backup is written to it: it comes into existence with the first file of the backup. -Restoring, listing and deleting backups require a location that already holds at least one object. +The location doesn't need to exist prior to backup; it is created automatically when the first file is written. +However, restoring, listing, or deleting backups requires an existing location with at least one object. Empty Location:: If you do not want to use a sub-directory within your bucket to store your backup, you can use any of the following location options: `/`, `s3:/`, `s3://`. However the location option is mandatory and you will receive an error when trying to perform backup operations without it. Directory Markers:: -S3 has no native concept of directories, so Solr does not write any zero-byte "directory marker" objects: a directory is simply the common key prefix of the files stored in it. -Backups can therefore be copied between buckets with tools such as `aws s3 sync`, which drop such marker objects, and still be restored. -Backups made by older Solr versions, which do contain marker objects, can still be restored and deleted. +Backups can be copied between buckets or locations with standard S3 tools, such as `aws s3 sync`, and still be restored. +This works because Solr treats a directory as nothing more than the common key prefix of the files stored under it, rather than writing a zero-byte "directory marker" object for it. +Backups made by older Solr versions, which do contain marker objects, remain fully restorable and deletable. ==== An example configuration to enable S3 backups and restore can be seen below: From 39d6134d122d86c4c5596ea36370d17f479a2501 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Wed, 21 Oct 2026 07:22:55 -0400 Subject: [PATCH 11/13] Lead the Directory Markers doc note with user impact, not mechanism Reorder so the practical takeaway (backups copied with aws s3 sync still restore, older backups with markers still work) comes first, with the marker-object explanation as a brief aside rather than the opening sentence. --- .../modules/deployment-guide/pages/backup-restore.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc index ed5929a43b9f..cee577412b45 100644 --- a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc +++ b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc @@ -728,7 +728,7 @@ When using the Backup & Restore Collections API Calls, the **`location`** option All the above options will resolve to the same directory in your S3 Bucket: `dir/in/bucket`. Location Creation:: -The location doesn't need to exist prior to backup; it is created automatically when the first file is written. +The location doesn't need to exist prior to backup; it is created automatically when the first file is written. However, restoring, listing, or deleting backups requires an existing location with at least one object. Empty Location:: From 91500069e48cbdaec20d868cca04c1d042e2fda3 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Wed, 21 Oct 2026 08:02:35 -0400 Subject: [PATCH 12/13] Simplify directory-marker cleanup: drop dead code, dedup test helper - hasObjectsUnderPrefix: listObjectsV2's prefix already guarantees every returned key matches, so the anyMatch(startsWith) re-check and now-unused S3_DIR_CONTENT_TYPE constant are redundant. - length(): the content-type check in the success path can never trigger, since sanitizedFilePath rejects a trailing '/' and every directory marker key ends in one; the NoSuchKeyException branch's isDirectory() check already covers the markerless case. - AbstractBackupRepositoryTest: extract the repeated 'seed a file per dir unless supportsEmptyDirectories()' pattern into one keepNonEmptyIfNeeded helper instead of two copies. --- .../org/apache/solr/s3/S3StorageClient.java | 19 ++++--------- .../AbstractBackupRepositoryTest.java | 28 ++++++++++++------- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java index 8207240e0661..527fb03b63b2 100644 --- a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java +++ b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java @@ -81,9 +81,6 @@ public class S3StorageClient { // S3 has a hard limit of 1000 keys per batch delete request private static final int MAX_KEYS_PER_BATCH_DELETE = 1000; - // Content type of the zero-byte directory marker objects written by older Solr versions - private static final String S3_DIR_CONTENT_TYPE = "application/x-directory"; - // Error messages returned by S3 for a key not found. private static final Set NOT_FOUND_CODES = Set.of("NoSuchKey", "404 Not Found"); @@ -309,11 +306,11 @@ boolean isDirectory(String path) throws S3Exception { private boolean hasObjectsUnderPrefix(String prefix) throws S3Exception { try { - return s3Client + // listObjectsV2's prefix already guarantees every returned key starts with it. + return !s3Client .listObjectsV2(b -> b.bucket(bucketName).prefix(prefix).maxKeys(1)) .contents() - .stream() - .anyMatch(o -> o.key().startsWith(prefix)); + .isEmpty(); } catch (SdkException sdke) { throw handleAmazonException(sdke); } @@ -328,15 +325,11 @@ private boolean hasObjectsUnderPrefix(String prefix) throws S3Exception { long length(String path) throws S3Exception { String s3Path = sanitizedFilePath(path); try { + // sanitizedFilePath rejects a trailing '/', so a successful HEAD here can never land on a + // directory marker object (whose key always ends in '/'); it's always a real file. HeadObjectResponse objectMetadata = s3Client.headObject(b -> b.bucket(bucketName).key(s3Path)); - String contentType = objectMetadata.contentType(); - - if (StrUtils.isNullOrEmpty(contentType) - || !contentType.equalsIgnoreCase(S3_DIR_CONTENT_TYPE)) { - return objectMetadata.contentLength(); - } - throw new S3Exception("Path is Directory"); + return objectMetadata.contentLength(); } catch (NoSuchKeyException nske) { // Only pay for the extra listing when the file is missing if (isDirectory(s3Path)) { diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java b/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java index 65c2c74dcab8..eebdab230ef9 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java @@ -188,11 +188,8 @@ public void testCanDeleteEmptyOrFullDirectories() throws Exception { repo.createDirectory(level2DeeplyNestedUri); repo.createDirectory(level3DeeplyNestedUri); repo.createDirectory(level4DeeplyNestedUri); - if (!repo.supportsEmptyDirectories()) { - // Keep nest2 (and therefore nest1) non-empty once nest3 is deleted - addFile(repo, repo.resolve(level2DeeplyNestedUri, "file.txt")); - addFile(repo, repo.resolve(level4DeeplyNestedUri, "file.txt")); - } + // Keep nest2 (and therefore nest1) non-empty once nest3 is deleted + keepNonEmptyIfNeeded(repo, level2DeeplyNestedUri, level4DeeplyNestedUri); repo.deleteDirectory(level3DeeplyNestedUri); assertTrue(repo.exists(level1DeeplyNestedUri)); assertTrue(repo.exists(level2DeeplyNestedUri)); @@ -248,11 +245,8 @@ public void testCanListFullOrEmptyDirectories() throws Exception { repo.createDirectory(otherDir1Uri); repo.createDirectory(otherDir2Uri); repo.createDirectory(otherDir3Uri); - if (!repo.supportsEmptyDirectories()) { - // Without empty directories, otherDir1 and otherDir2 only show up once they hold a file - addFile(repo, repo.resolve(otherDir1Uri, "file.txt")); - addFile(repo, repo.resolve(otherDir2Uri, "file.txt")); - } + // Without empty directories, otherDir1 and otherDir2 only show up once they hold a file + keepNonEmptyIfNeeded(repo, otherDir1Uri, otherDir2Uri); addFile(repo, file1Uri); addFile(repo, file2Uri); @@ -403,6 +397,20 @@ private void addFile(BackupRepository repo, URI file) throws IOException { } } + /** + * On a repository that can't hold empty directories, seeds a file under each given directory so + * it (and, transitively, its ancestors) stays non-empty for the rest of the test. No-op when the + * repository supports real empty directories. + */ + private void keepNonEmptyIfNeeded(BackupRepository repo, URI... dirs) throws IOException { + if (repo.supportsEmptyDirectories()) { + return; + } + for (URI dir : dirs) { + addFile(repo, repo.resolve(dir, "file.txt")); + } + } + /** * Test implementation of a {@link DelegatingBackupRepository} that disables the checksum * verification on its delegate {@link BackupRepository}. From c848bdd04f7435ee198da72f6ec01212130c6199 Mon Sep 17 00:00:00 2001 From: Samuel Verstraete Date: Mon, 26 Oct 2026 07:41:43 +0100 Subject: [PATCH 13/13] Allow non-incremental core backups to an S3 location that does not exist yet With directory markers gone, createDirectory() persists nothing on S3, so a location only exists once a file is stored under it. A BACKUPCORE with incremental=false (the default for the v2 core backup API) to a fresh prefix was refused by the location checks in CreateCoreBackup and SnapShooter.validateCreateSnapshot(), which the marker used to satisfy. Both checks now accept a missing location on a repository that doesn't support empty directories, like the collection backup already does. The location is still checked, so an unreachable bucket fails up front. Co-Authored-By: Claude Opus 5.5 --- .../org/apache/solr/handler/SnapShooter.java | 3 +- .../handler/admin/api/CreateCoreBackup.java | 5 +- .../solr/s3/S3IncrementalBackupTest.java | 68 +++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/handler/SnapShooter.java b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java index 81b2e9bc3970..9de40d225f17 100644 --- a/solr/core/src/java/org/apache/solr/handler/SnapShooter.java +++ b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java @@ -151,7 +151,8 @@ protected void deleteSnapAsync(final ReplicationHandler replicationHandler) { public void validateCreateSnapshot() throws IOException { // Note - Removed the current behavior of creating the directory hierarchy. // Do we really need to provide this support? - if (!backupRepo.exists(baseSnapDirPath)) { + // Checked even when a missing location is allowed, to fail early on an unreachable repository + if (!backupRepo.exists(baseSnapDirPath) && backupRepo.supportsEmptyDirectories()) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, " Directory does not exist: " + snapshotDirPath); } diff --git a/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCoreBackup.java b/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCoreBackup.java index 9299657daf85..d47260d92792 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCoreBackup.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCoreBackup.java @@ -111,7 +111,10 @@ public SolrJerseyResponse createBackup( // already // exist here if triggered via a collection backup on a shared file system. Otherwise, // perhaps the FS location isn't shared -- we want an error. - if (!snapShooter.getBackupRepository().exists(snapShooter.getLocation())) { + // A repository that can't hold empty directories, such as S3, has no location until + // the first file is written to it, so a missing one is accepted there. + if (!snapShooter.getBackupRepository().exists(snapShooter.getLocation()) + && snapShooter.getBackupRepository().supportsEmptyDirectories()) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, "Directory to contain snapshots doesn't exist: " diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java index a3867a79ef6f..cc3a4474d5c2 100644 --- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java +++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java @@ -20,17 +20,25 @@ import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakLingering; import java.util.List; +import java.util.Map; import org.apache.lucene.tests.util.LuceneTestCase; import org.apache.lucene.tests.util.QuickPatchThreadsFilter; import org.apache.solr.SolrIgnoredThreadsFilter; +import org.apache.solr.client.solrj.SolrRequest.METHOD; +import org.apache.solr.client.solrj.SolrRequest.SolrRequestType; import org.apache.solr.client.solrj.impl.CloudSolrClient; import org.apache.solr.client.solrj.request.CollectionAdminRequest; import org.apache.solr.client.solrj.request.ConfigSetAdminRequest; +import org.apache.solr.client.solrj.request.GenericSolrRequest; import org.apache.solr.client.solrj.request.SolrQuery; import org.apache.solr.client.solrj.response.RequestStatusState; import org.apache.solr.cloud.AbstractFullDistribZkTestBase; import org.apache.solr.cloud.api.collections.AbstractIncrementalBackupTest; +import org.apache.solr.common.cloud.Replica; import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.common.params.CoreAdminParams.CoreAdminAction; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.handler.BackupRestoreUtils; import org.apache.solr.util.LogLevel; import org.junit.BeforeClass; import org.junit.ClassRule; @@ -266,4 +274,64 @@ public void testBackupToLocationThatDoesNotExistYet() throws Exception { .processAndWait(solrClient, 100)); assertTrue(e.getMessage(), e.getMessage().contains("does not exist")); } + + /** + * A core backup in the non-incremental format to a location that does not exist yet. On S3 the + * location only comes into existence with the first file of the backup. + */ + @Test + public void testNonIncrementalCoreBackupToLocationThatDoesNotExistYet() throws Exception { + setTestSuffix("testcorenewlocation"); + final String collectionName = getCollectionName(); + final String backupName = BACKUPNAME_PREFIX + testSuffix; + final String newLocation = "new-core-location/nested"; + + CloudSolrClient solrClient = cluster.getSolrClient(); + CollectionAdminRequest.createCollection(collectionName, "conf1", 1, 1).process(solrClient); + int numDocs = indexDocs(collectionName, true); + Replica replica = + solrClient.getClusterState().getCollection(collectionName).getLeader("shard1"); + + runCoreAdminCommand( + replica, + CoreAdminAction.BACKUPCORE, + Map.of( + "name", backupName, + "repository", BACKUP_REPO_NAME, + "location", newLocation, + "incremental", "false")); + + try (S3Client s3 = s3MockContainer.createS3ClientV2()) { + assertFalse( + "Backup should contain the snapshot's index files", + s3.listObjectsV2( + b -> b.bucket(BUCKET_NAME).prefix(newLocation + "/snapshot." + backupName + "/")) + .contents() + .isEmpty()); + } + + solrClient.deleteByQuery(collectionName, "*:*"); + solrClient.commit(collectionName); + runCoreAdminCommand( + replica, + CoreAdminAction.RESTORECORE, + Map.of( + "name", "snapshot." + backupName, + "repository", BACKUP_REPO_NAME, + "location", newLocation)); + BackupRestoreUtils.verifyDocs(numDocs, solrClient, collectionName); + } + + private void runCoreAdminCommand( + Replica replica, CoreAdminAction action, Map params) throws Exception { + ModifiableSolrParams coreParams = new ModifiableSolrParams(); + coreParams.set("action", action.toString()); + coreParams.set("core", replica.getCoreName()); + params.forEach(coreParams::set); + cluster + .getJetty(replica.getNodeName()) + .getSolrClient() + .request( + new GenericSolrRequest(METHOD.GET, "/admin/cores", SolrRequestType.ADMIN, coreParams)); + } }