This is an automated email from the ASF dual-hosted git repository. dsmiley pushed a commit to branch branch_10x in repository https://gitbox.apache.org/repos/asf/solr.git
commit 13575bfaeb4db047d75c16f611bf090b2a6ede8a Author: Samuel Verstraete <[email protected]> AuthorDate: Mon Oct 26 12:06:46 2026 +0100 S3 backup repository: stop writing and depending on directory marker objects (#5128) Co-authored-by: Samuel Verstraete <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]> Co-authored-by: Eric Pugh <[email protected]> (cherry picked from commit 8c440065c87d12d72a413b16c67481e5aab31dfc) --- .../fix-s3-restore-missing-directory-marker.yml | 9 + .../core/backup/repository/BackupRepository.java | 14 ++ .../repository/DelegatingBackupRepository.java | 5 + .../java/org/apache/solr/handler/SnapShooter.java | 3 +- .../solr/handler/admin/api/BackupAPIBase.java | 22 ++- .../handler/admin/api/CreateCollectionBackup.java | 2 +- .../solr/handler/admin/api/CreateCoreBackup.java | 5 +- .../org/apache/solr/s3/S3BackupRepository.java | 6 +- .../java/org/apache/solr/s3/S3StorageClient.java | 117 ++++--------- .../org/apache/solr/s3/S3BackupRepositoryTest.java | 69 +++++++- .../apache/solr/s3/S3IncrementalBackupTest.java | 193 +++++++++++++++++++++ .../src/test/org/apache/solr/s3/S3PathsTest.java | 22 +++ .../test/org/apache/solr/s3/S3ReadWriteTest.java | 1 + .../deployment-guide/pages/backup-restore.adoc | 12 +- .../collections/AbstractBackupRepositoryTest.java | 26 ++- 15 files changed, 401 insertions(+), 105 deletions(-) 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 00000000000..3c1274dfc20 --- /dev/null +++ b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml @@ -0,0 +1,9 @@ +# 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. A backup can also be written to an S3 location that does not exist yet. +type: fixed +authors: + - name: Samuel Verstraete + nick: elangelo +links: + - name: PR#5128 + url: https://github.com/apache/solr/pull/5128 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 b8f86af5e3e..3d13ea6ac53 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 @@ public interface BackupRepository extends NamedListInitializedPlugin, Closeable */ void deleteDirectory(URI path) throws IOException; + /** + * Whether this repository can hold an empty directory. + * + * <p>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 <code>sourceDir</code> 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 e3b27cb073c..54cd5845842 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 class DelegatingBackupRepository extends AbstractBackupRepository { 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/SnapShooter.java b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java index 81b2e9bc397..9de40d225f1 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 @@ public class SnapShooter { 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/BackupAPIBase.java b/solr/core/src/java/org/apache/solr/handler/admin/api/BackupAPIBase.java index b7028ff0405..14656547a5f 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 @@ public abstract class BackupAPIBase extends AdminAPIBase { } } + /** + * 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 @@ public abstract class BackupAPIBase extends AdminAPIBase { + " 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 @@ public abstract class BackupAPIBase extends AdminAPIBase { 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 5e856012819..a9c251172c1 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 @@ -92,7 +92,7 @@ public class CreateCollectionBackup extends BackupAPIBase implements CollectionB collectionName, Boolean.TRUE.equals(requestBody.followAliases)); requestBody.location = - getAndValidateBackupLocation(requestBody.repository, requestBody.location); + getAndValidateNewBackupLocation(requestBody.repository, requestBody.location); if (requestBody.incremental == null) { requestBody.incremental = Boolean.TRUE; 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 9299657daf8..d47260d9279 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 class CreateCoreBackup extends CoreAdminAPIBase implements CreateCoreBack // 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/java/org/apache/solr/s3/S3BackupRepository.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java index 23b1ed4c509..a4f8f14211e 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 class S3BackupRepository extends AbstractBackupRepository { 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"); @@ -285,7 +290,6 @@ public class S3BackupRepository extends AbstractBackupRepository { 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 bf076226685..527fb03b63b 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,10 +41,8 @@ import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; 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.RequestBody; import software.amazon.awssdk.core.sync.ResponseTransformer; import software.amazon.awssdk.http.apache.ApacheHttpClient; import software.amazon.awssdk.http.apache.ProxyConfiguration; @@ -64,7 +62,6 @@ import software.amazon.awssdk.services.s3.model.HeadObjectResponse; 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; @@ -84,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; - // Metadata name used to identify flag directory entries in S3 - 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<String> NOT_FOUND_CODES = Set.of("NoSuchKey", "404 Not Found"); @@ -182,30 +176,13 @@ public class S3StorageClient { return clientBuilder.build(); } - /** 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); - } - } - } + /** + * 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. @@ -239,9 +216,6 @@ public class S3StorageClient { // Get all the files and subdirectories Set<String> entries = listAll(path); - if (pathExists(path)) { - entries.add(path); - } deleteObjects(entries); } @@ -292,10 +266,11 @@ public class S3StorageClient { } /** - * 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); @@ -305,6 +280,10 @@ public class S3StorageClient { return true; } + if (s3Path.endsWith(S3_FILE_PATH_DELIMITER)) { + return hasObjectsUnderPrefix(s3Path); + } + try { s3Client.headObject(builder -> builder.bucket(bucketName).key(s3Path)); return true; @@ -316,23 +295,22 @@ public class S3StorageClient { } /** - * Check if path is directory. + * 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); + return hasObjectsUnderPrefix(sanitizedDirPath(path)); + } + private boolean hasObjectsUnderPrefix(String prefix) throws S3Exception { 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) { - return false; + // listObjectsV2's prefix already guarantees every returned key starts with it. + return !s3Client + .listObjectsV2(b -> b.bucket(bucketName).prefix(prefix).maxKeys(1)) + .contents() + .isEmpty(); } catch (SdkException sdke) { throw handleAmazonException(sdke); } @@ -346,19 +324,18 @@ public class S3StorageClient { */ long length(String path) throws S3Exception { String s3Path = sanitizedFilePath(path); - if (isDirectory(s3Path)) { - throw new S3Exception("Path is Directory"); - } 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(); + return objectMetadata.contentLength(); + } catch (NoSuchKeyException nske) { + // Only pay for the extra listing when the file is missing + if (isDirectory(s3Path)) { + throw new S3Exception("Path is Directory"); } - throw new S3Exception("Path is Directory"); + throw handleAmazonException(nske); } catch (SdkException sdke) { throw handleAmazonException(sdke); } @@ -409,10 +386,6 @@ public class S3StorageClient { 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) { @@ -519,34 +492,6 @@ public class S3StorageClient { } } - 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/S3BackupRepositoryTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java index bd24ac0a7cc..ae09e1ae8fd 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.Files; 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.RequestBody; 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 class S3BackupRepositoryTest extends AbstractBackupRepositoryTest { "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 class S3BackupRepositoryTest extends AbstractBackupRepositoryTest { 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 class S3BackupRepositoryTest extends AbstractBackupRepositoryTest { } } + /** + * 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 { @@ -348,6 +388,25 @@ public class S3BackupRepositoryTest extends AbstractBackupRepositoryTest { } } + 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<String> 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 9546012190b..cc3a4474d5c 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,33 @@ package org.apache.solr.s3; 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; +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 +160,178 @@ public class S3IncrementalBackupTest extends AbstractIncrementalBackupTest { 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<String> 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()); + } + + /** + * 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")); + } + + /** + * 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<String, String> 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)); + } } 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 d314855c3b1..012b564b9e7 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 @@ -48,6 +48,9 @@ public class S3PathsTest extends AbstractS3ClientTest { public void testDirectory() throws S3Exception { 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/")); @@ -163,6 +166,25 @@ public class S3PathsTest extends AbstractS3ClientTest { assertTrue(client.pathExists("/my-file2")); } + /** A directory is any key prefix that has objects under it; no marker object is needed. */ + @Test + public void testVirtualDirectoryWithoutMarker() throws S3Exception { + pushContent("/virtual-dir-sibling", "not under the prefix"); + pushContent("/virtual-dir/file1", "file1"); + pushContent("/virtual-dir/sub-dir/file2", "file2"); + + 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")); + 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. */ @Test public void testListDir() throws S3Exception { 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 3574587b4fa..9ff6fa17858 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 class S3ReadWriteTest extends AbstractS3ClientTest { @Test public void testDirectoryLength() throws Exception { client.createDirectory("/directory"); + pushContent("/directory/file", "child"); S3Exception exception = assertThrows( 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 a0158af5a73..1a59d6d4877 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 @@ -772,14 +772,18 @@ 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. -+ -**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 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:: +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: 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 af8f5dbc3ce..eebdab230ef 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 @@ -111,7 +111,7 @@ public abstract class AbstractBackupRepositoryTest extends SolrTestCaseJ4 { repo.createDirectory(nonEmptyDirUri); addFile(repo, nestedFileUri); - assertTrue(repo.exists(emptyDirUri)); + assertEquals(repo.supportsEmptyDirectories(), repo.exists(emptyDirUri)); assertTrue(repo.exists(nonEmptyDirUri)); assertTrue(repo.exists(nestedFileUri)); final URI nonexistedDirUri = repo.resolve(getBaseUri(), "nonexistentDir/"); @@ -129,7 +129,9 @@ public abstract class AbstractBackupRepositoryTest extends SolrTestCaseJ4 { repo.createDirectory(nonEmptyDirUri); addFile(repo, nestedFileUri); - assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(emptyDirUri)); + if (repo.supportsEmptyDirectories()) { + assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(emptyDirUri)); + } assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(nonEmptyDirUri)); assertEquals(BackupRepository.PathType.FILE, repo.getPathType(nestedFileUri)); } @@ -186,6 +188,8 @@ public abstract class AbstractBackupRepositoryTest extends SolrTestCaseJ4 { repo.createDirectory(level2DeeplyNestedUri); repo.createDirectory(level3DeeplyNestedUri); repo.createDirectory(level4DeeplyNestedUri); + // 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)); @@ -241,6 +245,8 @@ public abstract class AbstractBackupRepositoryTest extends SolrTestCaseJ4 { repo.createDirectory(otherDir1Uri); repo.createDirectory(otherDir2Uri); repo.createDirectory(otherDir3Uri); + // Without empty directories, otherDir1 and otherDir2 only show up once they hold a file + keepNonEmptyIfNeeded(repo, otherDir1Uri, otherDir2Uri); addFile(repo, file1Uri); addFile(repo, file2Uri); @@ -251,7 +257,7 @@ public abstract class AbstractBackupRepositoryTest extends SolrTestCaseJ4 { assertTrue(rootChildren.contains("otherDir3")); final String[] otherDir2Children = repo.listAll(otherDir2Uri); - assertEquals(0, otherDir2Children.length); + assertEquals(repo.supportsEmptyDirectories() ? 0 : 1, otherDir2Children.length); final List<String> otherDir3Children = List.of(repo.listAll(otherDir3Uri)); assertEquals(2, otherDir3Children.size()); @@ -391,6 +397,20 @@ public abstract class AbstractBackupRepositoryTest extends SolrTestCaseJ4 { } } + /** + * 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}.
