S3 backup repository: stop writing and depending on directory marker objects

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

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.
+   *
+   * <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).
    *

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);

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

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;
   }
 

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;

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: "

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");
@@ -285,7 +290,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];

solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java

@@ -41,10 +41,8 @@
 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.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 @@ private static S3Client createInternalClient(
     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 @@ void deleteDirectory(String path) throws S3Exception {
 
     // Get all the files and subdirectories
     Set<String> entries = listAll(path);
-    if (pathExists(path)) {
-      entries.add(path);
-    }
 
     deleteObjects(entries);
   }
@@ -292,10 +266,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);
@@ -305,6 +280,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;
@@ -316,23 +295,22 @@ boolean pathExists(String path) throws S3Exception {
   }
 
   /**
-   * 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 @@ boolean isDirectory(String path) throws S3Exception {
    */
   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 @@ 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) {
@@ -519,34 +492,6 @@ private Set<String> 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