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..7f51fa4aff4c --- /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: Remove S3 directory marker dependency from backup/restore — use virtual-directory convention instead +type: fixed +authors: + - name: Samuel Verstraete +links: + - name: GitHub PR #4182 + url: https://github.com/apache/solr/pull/4182 diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java index 151cbeeee1a8..3a0861cbd959 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java @@ -198,15 +198,13 @@ private URI createAndValidateBackupPath( throws IOException { final URI backupNamePath = repository.resolveDirectory(location, backupName); - if ((!incremental) && repository.exists(backupNamePath)) { + if ((!incremental) && repository.listAllOrEmpty(backupNamePath).length > 0) { throw new SolrException( SolrException.ErrorCode.BAD_REQUEST, "The backup directory already exists: " + backupNamePath); } - if (!repository.exists(backupNamePath)) { - repository.createDirectory(backupNamePath); - } else if (incremental) { + if (incremental && repository.listAllOrEmpty(backupNamePath).length > 0) { final String[] directoryContents = repository.listAll(backupNamePath); if (directoryContents.length == 1) { String directoryContentsName = directoryContents[0]; @@ -238,9 +236,6 @@ private URI createAndValidateBackupPath( // Incremental backups have an additional directory named after the collection that needs // created final URI backupPathWithCollection = repository.resolveDirectory(backupNamePath, collection); - if (!repository.exists(backupPathWithCollection)) { - repository.createDirectory(backupPathWithCollection); - } BackupFilePaths incBackupFiles = new BackupFilePaths(repository, backupPathWithCollection); incBackupFiles.createIncrementalBackupFolders(); return backupPathWithCollection; 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 403dd589b0f9..b6ad9e613e7f 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 @@ -131,7 +131,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); } @@ -258,7 +258,7 @@ public void uploadConfigDir( String sourceConfigName, String targetConfigName, ConfigSetService configSetService) throws IOException { URI source = repository.resolveDirectory(getZkStateDir(), CONFIG_STATE_DIR, sourceConfigName); - if (!repository.exists(source)) { + if (repository.listAll(source).length == 0) { throw new IllegalArgumentException("Configset expected at " + source + " does not exist"); } uploadConfigToSolrCloud(configSetService, source, targetConfigName, ""); @@ -275,9 +275,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); } @@ -368,9 +365,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. } } } @@ -431,6 +427,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 4fe1f0c74272..afd06ebf0f5c 100644 --- a/solr/core/src/java/org/apache/solr/handler/SnapShooter.java +++ b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java @@ -162,12 +162,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 new file mode 100644 index 000000000000..cf7c1d739d92 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/core/backup/BackupManagerUploadConfigTest.java @@ -0,0 +1,179 @@ +/* + * 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/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 0add42da2d6b..0ae5cce44a19 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 @@ -39,10 +39,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.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; @@ -61,7 +59,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; @@ -181,27 +178,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. } /** @@ -236,9 +215,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); } @@ -315,6 +291,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. */ @@ -329,7 +312,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); } @@ -405,10 +411,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) { @@ -515,34 +517,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) throws S3Exception { // 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 18e30d194388..c2f35639c771 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 @@ -90,7 +90,13 @@ public void testURI() throws IOException { directoryUri.toString()); repo.createDirectory(directoryUri); - assertTrue(repo.exists(directoryUri)); + assertFalse(repo.exists(directoryUri)); + try (OutputStream os = + repo.createOutput(repo.resolve(directoryUri, "markerless-child.txt"))) { + os.write(1); + } + assertFalse(repo.exists(directoryUri)); + assertEquals(1, repo.listAll(directoryUri).length); directoryUri = repo.createDirectoryURI("d/"); assertEquals( "createDirectoryURI should have a single trailing slash, even if one is provided", @@ -110,16 +116,22 @@ 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(repo.exists(path)); assertEquals("No files should exist in dir yet", repo.listAll(path).length, 0); URI subDir = new URI("/test/dir/"); repo.createDirectory(subDir); - assertTrue(repo.exists(subDir)); - assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(subDir)); + assertFalse(repo.exists(subDir)); assertEquals("No files should exist in subdir yet", repo.listAll(subDir).length, 0); + try (OutputStream os = repo.createOutput(new URI("/test/dir/file.txt"))) { + os.write(1); + } + assertFalse(repo.exists(path)); + assertFalse(repo.exists(subDir)); + assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(path)); + assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(subDir)); + assertEquals( "subDir should now be returned when listing all in parent dir", repo.listAll(path).length, @@ -246,6 +258,134 @@ public void testRandomAccessInput() throws Exception { doRandomAccessTest(content, content.indexOf("content")); } + @Override + @Test + public void testCanDetermineWhetherFilesAndDirectoriesExist() throws Exception { + try (BackupRepository repo = getRepository()) { + final URI emptyDirUri = repo.resolve(getBaseUri(), "emptyDir/"); + final URI nonEmptyDirUri = repo.resolve(getBaseUri(), "nonEmptyDir/"); + final URI nestedFileUri = repo.resolve(nonEmptyDirUri, "file.txt"); + repo.createDirectory(emptyDirUri); + repo.createDirectory(nonEmptyDirUri); + try (OutputStream os = repo.createOutput(nestedFileUri)) { + os.write(100); + } + + assertFalse(repo.exists(emptyDirUri)); + assertFalse(repo.exists(nonEmptyDirUri)); + assertEquals(1, repo.listAll(nonEmptyDirUri).length); + assertTrue(repo.exists(nestedFileUri)); + final URI nonexistedDirUri = repo.resolve(getBaseUri(), "nonexistentDir/"); + assertFalse(repo.exists(nonexistedDirUri)); + } + } + + @Override + @Test + public void testCanDistinguishBetweenFilesAndDirectories() throws Exception { + try (BackupRepository repo = getRepository()) { + final URI nonEmptyDirUri = repo.resolve(getBaseUri(), "nonEmptyDir/"); + final URI nestedFileUri = repo.resolve(nonEmptyDirUri, "file.txt"); + repo.createDirectory(nonEmptyDirUri); + try (OutputStream os = repo.createOutput(nestedFileUri)) { + os.write(100); + } + + assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(nonEmptyDirUri)); + assertEquals(BackupRepository.PathType.FILE, repo.getPathType(nestedFileUri)); + } + } + + @Override + @Test + public void testCanDeleteEmptyOrFullDirectories() throws Exception { + try (BackupRepository repo = getRepository()) { + final URI emptyDirUri = repo.resolve(getBaseUri(), "emptyDir"); + final URI nonEmptyDirUri = repo.resolve(getBaseUri(), "nonEmptyDir"); + final URI fileUri = repo.resolve(nonEmptyDirUri, "file.txt"); + repo.createDirectory(emptyDirUri); + repo.createDirectory(nonEmptyDirUri); + try (OutputStream os = repo.createOutput(fileUri)) { + os.write(100); + } + repo.deleteDirectory(emptyDirUri); + repo.deleteDirectory(nonEmptyDirUri); + assertFalse(repo.exists(emptyDirUri)); + assertFalse(repo.exists(nonEmptyDirUri)); + assertFalse(repo.exists(fileUri)); + + final URI level1DeeplyNestedUri = repo.resolve(getBaseUri(), "nest1/"); + final URI level2DeeplyNestedUri = repo.resolve(level1DeeplyNestedUri, "nest2/"); + final URI level3DeeplyNestedUri = repo.resolve(level2DeeplyNestedUri, "nest3/"); + final URI level4DeeplyNestedUri = repo.resolve(level3DeeplyNestedUri, "nest4/"); + final URI keepFileUri = repo.resolve(level2DeeplyNestedUri, "keep.txt"); + final URI nestedFileUri = repo.resolve(level4DeeplyNestedUri, "file.txt"); + repo.createDirectory(level1DeeplyNestedUri); + repo.createDirectory(level2DeeplyNestedUri); + repo.createDirectory(level3DeeplyNestedUri); + repo.createDirectory(level4DeeplyNestedUri); + try (OutputStream os = repo.createOutput(keepFileUri)) { + os.write(100); + } + try (OutputStream os = repo.createOutput(nestedFileUri)) { + os.write(101); + } + + repo.deleteDirectory(level3DeeplyNestedUri); + assertFalse(repo.exists(level1DeeplyNestedUri)); + assertFalse(repo.exists(level2DeeplyNestedUri)); + assertEquals(1, repo.listAll(level2DeeplyNestedUri).length); + assertFalse(repo.exists(level3DeeplyNestedUri)); + assertFalse(repo.exists(level4DeeplyNestedUri)); + } + } + + @Override + @Test + public void testCanListFullOrEmptyDirectories() throws Exception { + try (BackupRepository repo = getRepository()) { + final URI rootUri = repo.resolve(getBaseUri(), "containsOtherDirs"); + final URI otherDir1Uri = repo.resolve(rootUri, "otherDir1"); + final URI otherDir2Uri = repo.resolve(rootUri, "otherDir2"); + final URI otherDir3Uri = repo.resolve(rootUri, "otherDir3"); + final URI file1Uri = repo.resolve(otherDir3Uri, "file1.txt"); + final URI file2Uri = repo.resolve(otherDir3Uri, "file2.txt"); + repo.createDirectory(rootUri); + repo.createDirectory(otherDir1Uri); + repo.createDirectory(otherDir2Uri); + repo.createDirectory(otherDir3Uri); + try (OutputStream os = repo.createOutput(repo.resolve(otherDir1Uri, "a.txt"))) { + os.write(100); + } + try (OutputStream os = repo.createOutput(repo.resolve(otherDir2Uri, "b.txt"))) { + os.write(101); + } + try (OutputStream os = repo.createOutput(file1Uri)) { + os.write(102); + } + try (OutputStream os = repo.createOutput(file2Uri)) { + os.write(103); + } + + final java.util.List rootChildren = java.util.List.of(repo.listAll(rootUri)); + assertEquals(3, rootChildren.size()); + assertTrue(rootChildren.contains("otherDir1")); + assertTrue(rootChildren.contains("otherDir2")); + assertTrue(rootChildren.contains("otherDir3")); + + final java.util.List otherDir2Children = + java.util.List.of(repo.listAll(otherDir2Uri)); + assertEquals(1, otherDir2Children.size()); + assertTrue(otherDir2Children.contains("b.txt")); + + final java.util.List otherDir3Children = + java.util.List.of(repo.listAll(otherDir3Uri)); + assertEquals(2, otherDir3Children.size()); + assertTrue(otherDir3Children.contains("file1.txt")); + assertTrue(otherDir3Children.contains("file2.txt")); + } + } + /** * Check implementation of {@link S3BackupRepository#openInput(URI, String, IOContext)}. Open an * index input and seek to an absolute position. 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 80c5207505b1..bcc4ca934936 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,10 +20,19 @@ import com.adobe.testing.s3mock.junit4.S3MockRule; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakLingering; import java.lang.invoke.MethodHandles; +import java.util.stream.Collectors; +import org.apache.solr.client.solrj.impl.CloudSolrClient; +import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.SolrQuery; +import org.apache.solr.cloud.AbstractFullDistribZkTestBase; import org.apache.lucene.tests.util.LuceneTestCase; import org.apache.solr.cloud.api.collections.AbstractIncrementalBackupTest; +import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.core.TrackingBackupRepository; +import org.apache.solr.core.backup.repository.BackupRepository; import org.junit.BeforeClass; import org.junit.ClassRule; +import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import software.amazon.awssdk.regions.Region; @@ -122,4 +131,114 @@ public String getCollectionNamePrefix() { public String getBackupLocation() { return "/"; } + + @Override + @Test + public void testSkipConfigset() throws Exception { + setTestSuffix("testskipconfigset"); + final String backupCollectionName = getCollectionName(); + final String restoreCollectionName = backupCollectionName + "-restore"; + + CloudSolrClient solrClient = cluster.getSolrClient(); + + CollectionAdminRequest.createCollection(backupCollectionName, "conf1", NUM_SHARDS, 1) + .process(solrClient); + int numDocs = indexDocs(backupCollectionName, true); + String backupName = BACKUPNAME_PREFIX + testSuffix; + try (BackupRepository repository = + cluster.getJettySolrRunner(0).getCoreContainer().newBackupRepository(BACKUP_REPO_NAME)) { + String backupLocation = repository.getBackupLocation(getBackupLocation()); + CollectionAdminRequest.backupCollection(backupCollectionName, backupName) + .setLocation(backupLocation) + .setBackupConfigset(false) + .setRepositoryName(BACKUP_REPO_NAME) + .processAndWait(cluster.getSolrClient(), 100); + + assertFalse( + "Configset shouldn't be part of the backup but found:\n" + + TrackingBackupRepository.directoriesCreated().stream() + .map(java.net.URI::toString) + .collect(Collectors.joining("\n")), + TrackingBackupRepository.directoriesCreated().stream() + .anyMatch(f -> f.getPath().contains("configs/conf1"))); + assertFalse( + "Configset shouldn't be part of the backup but found:\n" + + TrackingBackupRepository.outputsCreated().stream() + .map(java.net.URI::toString) + .collect(Collectors.joining("\n")), + TrackingBackupRepository.outputsCreated().stream() + .anyMatch(f -> f.getPath().contains("configs/conf1"))); + assertFalse( + "solrconfig.xml shouldn't be part of the backup but found:\n" + + TrackingBackupRepository.outputsCreated().stream() + .map(java.net.URI::toString) + .collect(Collectors.joining("\n")), + TrackingBackupRepository.outputsCreated().stream() + .anyMatch(f -> f.getPath().contains("solrconfig.xml"))); + assertFalse( + "schema.xml shouldn't be part of the backup but found:\n" + + TrackingBackupRepository.outputsCreated().stream() + .map(java.net.URI::toString) + .collect(Collectors.joining("\n")), + TrackingBackupRepository.outputsCreated().stream() + .anyMatch(f -> f.getPath().contains("schema.xml"))); + + CollectionAdminRequest.restoreCollection(restoreCollectionName, backupName) + .setLocation(backupLocation) + .setRepositoryName(BACKUP_REPO_NAME) + .processAndWait(solrClient, 500); + + AbstractFullDistribZkTestBase.waitForRecoveriesToFinish( + restoreCollectionName, ZkStateReader.from(solrClient), log.isDebugEnabled(), false, 3); + assertEquals( + numDocs, + cluster + .getSolrClient() + .query(restoreCollectionName, new SolrQuery("*:*")) + .getResults() + .getNumFound()); + } + + TrackingBackupRepository.clear(); + + try (BackupRepository repository = + cluster.getJettySolrRunner(0).getCoreContainer().newBackupRepository(BACKUP_REPO_NAME)) { + String backupLocation = repository.getBackupLocation(getBackupLocation()); + CollectionAdminRequest.backupCollection(backupCollectionName, backupName) + .setLocation(backupLocation) + .setBackupConfigset(true) + .setRepositoryName(BACKUP_REPO_NAME) + .processAndWait(cluster.getSolrClient(), 100); + + // S3 backups should never create explicit directory markers. + assertFalse( + "Configset directory creation should not happen for S3 backups:\n" + + TrackingBackupRepository.directoriesCreated().stream() + .map(java.net.URI::toString) + .collect(Collectors.joining("\n")), + TrackingBackupRepository.directoriesCreated().stream() + .anyMatch(f -> f.getPath().contains("configs/conf1"))); + assertTrue( + "Configset should be part of the backup but not found:\n" + + TrackingBackupRepository.outputsCreated().stream() + .map(java.net.URI::toString) + .collect(Collectors.joining("\n")), + TrackingBackupRepository.outputsCreated().stream() + .anyMatch(f -> f.getPath().contains("configs/conf1"))); + assertTrue( + "solrconfig.xml should be part of the backup but not found:\n" + + TrackingBackupRepository.outputsCreated().stream() + .map(java.net.URI::toString) + .collect(Collectors.joining("\n")), + TrackingBackupRepository.outputsCreated().stream() + .anyMatch(f -> f.getPath().contains("solrconfig.xml"))); + assertTrue( + "schema.xml should be part of the backup but not found:\n" + + TrackingBackupRepository.outputsCreated().stream() + .map(java.net.URI::toString) + .collect(Collectors.joining("\n")), + TrackingBackupRepository.outputsCreated().stream() + .anyMatch(f -> f.getPath().contains("schema.xml"))); + } + } } 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..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", @@ -163,6 +167,31 @@ 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 { + // 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"); + 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")); + } + /** 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 d4d13c717580..3e18f7cc31c2 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( 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 e6fa3e4d4039..5a53da952472 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 @@ -672,6 +672,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: diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index c9199fdafff2..9631f229553b 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java @@ -1663,6 +1663,11 @@ private CompletableFuture triggerCollectionRefresh(String collect DocCollection cached = cacheEntry == null ? null : cacheEntry.cached; return CompletableFuture.completedFuture(cached); } + // Remove any already-completed future that hasn't been cleaned up by whenCompleteAsync yet. + // Without this, computeIfAbsent below would reuse the stale done future instead of submitting + // a genuinely new refresh, causing callers (e.g. stale-state retry) to observe no new + // ref.get(). + collectionRefreshes.computeIfPresent(collection, (k, v) -> v.isDone() ? null : v); return collectionRefreshes.computeIfAbsent( collection, key -> {