PXB-3609 : xbcloud delete leaves the .md5 file behind

storage/innobase/xtrabackup/src/xbcloud/azure.cc

@@ -225,7 +225,7 @@ void Azure_client::set_endpoint(const std::string &ep, bool development_storage,
 }
 
 bool Azure_client::delete_object(const std::string &container,
-                                 const std::string &name) {
+                                 const std::string &name, bool best_effort) {
   Http_request req(Http_request::DELETE, protocol, host,
                    "/" + container + "/" + name);
   signer->sign_request(container, name, req, time(0));
@@ -239,6 +239,11 @@ bool Azure_client::delete_object(const std::string &container,
     return true;
   }
 
+  /* Object is not there (BlobNotFound), or we have no permission on it. */
+  if (best_effort && (resp.http_code() == 404 || resp.http_code() == 403)) {
+    return true;
+  }
+
   Azure_response azure_resp;
   if (!azure_resp.parse_http_response(resp)) {
     msg_ts("%s: Failed to delete object. Failed to parse XML response.\n",

storage/innobase/xtrabackup/src/xbcloud/azure.h

@@ -124,7 +124,8 @@ class Azure_client {
   void set_endpoint(const std::string &ep, bool development_storage,
                     const std::string &storage_account);
 
-  bool delete_object(const std::string &container, const std::string &name);
+  bool delete_object(const std::string &container, const std::string &name,
+                     bool best_effort);
 
   bool create_container(const std::string &name);
 
@@ -251,8 +252,9 @@ class Azure_object_store : public Object_store {
                                             });
   }
   virtual bool delete_object(const std::string &container,
-                             const std::string &name) override {
-    return azure_client.delete_object(container, name);
+                             const std::string &name,
+                             bool best_effort) override {
+    return azure_client.delete_object(container, name, best_effort);
   }
   virtual Http_buffer download_object(const std::string &container,
                                       const std::string &name,

storage/innobase/xtrabackup/src/xbcloud/object_store.h

@@ -51,8 +51,18 @@ class Object_store {
   virtual bool async_delete_object(const std::string &container,
                                    const std::string &object, Event_handler *h,
                                    std::function<void(bool)> f = {}) = 0;
+  /**
+   * Delete a single object.
+   *
+   * @param container    Container/bucket name.
+   * @param name         Object name.
+   * @param best_effort  When true, an object we cannot delete because it is
+   *                     not there, or because we have no permission on it,
+   *                     is a success and is not logged.
+   * @return true on success, false on error.
+   */
   virtual bool delete_object(const std::string &container,
-                             const std::string &name) = 0;
+                             const std::string &name, bool best_effort) = 0;
   virtual Http_buffer download_object(const std::string &container,
                                       const std::string &name,
                                       bool &success) = 0;

storage/innobase/xtrabackup/src/xbcloud/s3.cc

@@ -293,7 +293,7 @@ void S3_signerV2::sign_request(const std::string &hostname,
 }
 
 bool S3_client::delete_object(const std::string &bucket,
-                              const std::string &name) {
+                              const std::string &name, bool best_effort) {
   Http_request req(Http_request::DELETE, protocol, hostname(bucket),
                    bucketname(bucket) + "/" + name);
   signer->sign_request(hostname(bucket), bucket, req, time(0));
@@ -307,6 +307,12 @@ bool S3_client::delete_object(const std::string &bucket,
     return true;
   }
 
+  /* Object is not there (S3 answers 204 for this, some S3-compatible
+  services 404), or we have no permission on it. Neither is our problem. */
+  if (best_effort && (resp.http_code() == 404 || resp.http_code() == 403)) {
+    return true;
+  }
+
   S3_response s3_resp;
   if (!s3_resp.parse_http_response(resp)) {
     msg_ts("%s: Failed to delete object. Failed to parse XML response.\n",

storage/innobase/xtrabackup/src/xbcloud/s3.h

@@ -229,7 +229,8 @@ class S3_client {
 
   bool probe_api_version_and_lookup(const std::string &bucket);
 
-  bool delete_object(const std::string &bucket, const std::string &name);
+  bool delete_object(const std::string &bucket, const std::string &name,
+                     bool best_effort);
 
   bool create_bucket(const std::string &name);
 
@@ -355,8 +356,9 @@ class S3_object_store : public Object_store {
                                          });
   }
   virtual bool delete_object(const std::string &container,
-                             const std::string &name) override {
-    return s3_client.delete_object(container, name);
+                             const std::string &name,
+                             bool best_effort) override {
+    return s3_client.delete_object(container, name, best_effort);
   }
   virtual Http_buffer download_object(const std::string &container,
                                       const std::string &name,

storage/innobase/xtrabackup/src/xbcloud/swift.cc

@@ -590,7 +590,7 @@ bool Swift_client::validate_response(const Http_request &req,
 }
 
 bool Swift_client::delete_object(const std::string &container,
-                                 const std::string &name) {
+                                 const std::string &name, bool best_effort) {
   Http_request req(Http_request::DELETE, protocol, host,
                    path + container + "/" + name);
   req.add_header("X-Auth-Token", token);
@@ -604,6 +604,11 @@ bool Swift_client::delete_object(const std::string &container,
     return true;
   }
 
+  /* Object is not there, or we have no permission on it. */
+  if (best_effort && (resp.http_code() == 404 || resp.http_code() == 403)) {
+    return true;
+  }
+
   msg_ts("%s: Failed to delete object. Http code: %ld\n", my_progname,
          resp.http_code());
 

storage/innobase/xtrabackup/src/xbcloud/swift.h

@@ -166,7 +166,8 @@ class Swift_client {
     split_url(url, protocol, host, path);
   }
 
-  bool delete_object(const std::string &container, const std::string &name);
+  bool delete_object(const std::string &container, const std::string &name,
+                     bool best_effort);
 
   Http_buffer download_object(const std::string &container,
                               const std::string &name, bool &success);
@@ -258,8 +259,9 @@ class Swift_object_store : public Object_store {
                                             });
   }
   virtual bool delete_object(const std::string &container,
-                             const std::string &name) override {
-    return swift_client.delete_object(container, name);
+                             const std::string &name,
+                             bool best_effort) override {
+    return swift_client.delete_object(container, name, best_effort);
   }
   virtual Http_buffer download_object(const std::string &container,
                                       const std::string &name,

storage/innobase/xtrabackup/src/xbcloud/xbcloud.cc

@@ -1120,7 +1120,7 @@ bool xbcloud_delete(Object_store *store, const std::string &container,
     std::sort(dirs.begin(), dirs.end(), std::greater<std::string>());
     for (const auto &d : dirs) {
       msg_ts("%s: Deleting directory %s.\n", my_progname, d.c_str());
-      if (!store->delete_object(container, d)) {
+      if (!store->delete_object(container, d, false)) {
         msg_ts("%s: Delete failed. Cannot delete directory %s.\n", my_progname,
                d.c_str());
         return false;
@@ -1129,12 +1129,29 @@ bool xbcloud_delete(Object_store *store, const std::string &container,
 
     // Delete the root directory of the backup
     msg_ts("%s: Deleting directory %s.\n", my_progname, backup_name.c_str());
-    if (!store->delete_object(container, backup_name)) {
+    if (!store->delete_object(container, backup_name, false)) {
       msg_ts("%s: Warning: Failed to delete root directory %s.\n", my_progname,
              backup_name.c_str());
     }
   }
 
+  /* put --md5 uploads the checksum file as <backup_name>.md5, next to the
+  backup directory and not inside it, so the listing above never returns it.
+  Delete it here.
+
+  We do not know whether this backup was taken with --md5, so the delete is
+  unconditional and best_effort: a .md5 file that is not there, or that we
+  are not allowed to delete, leaves us behaving as we did before.
+
+  Partial deletes keep the backup, so they keep its .md5 file too. */
+  if (partial_file_list.empty()) {
+    const std::string md5_object = backup_name + ".md5";
+    if (!store->delete_object(container, md5_object, true)) {
+      msg_ts("%s: Warning: Failed to delete %s.\n", my_progname,
+             md5_object.c_str());
+    }
+  }
+
   msg_ts("%s: Delete completed.\n", my_progname);
 
   return true;

storage/innobase/xtrabackup/test/suites/xbcloud/md5_delete.sh

@@ -0,0 +1,136 @@
+################################################################################
+# PXB-3609: xbcloud delete must remove the <backup_name>.md5 file.
+#
+# `xbcloud put --md5` uploads the checksum file as <backup_name>.md5, next to
+# the backup directory and not inside it.  `xbcloud delete` lists only what is
+# under <backup_name>/, so before the fix that file was never deleted and
+# stayed in the bucket after the backup itself was gone.
+#
+# Scenarios:
+#   1. put --md5 then delete  -> the .md5 file goes with the backup.
+#   2. put (no --md5) then delete -> completes cleanly; a .md5 file that was
+#      never created must not turn into an error.
+#
+# Needs the MinIO client (mc) to look at individual object names, which
+# xbcloud itself cannot show.  mc talks to MinIO over the network, so no
+# docker is involved.  In CI the pipeline copies mc out of the MinIO image and
+# points XBCLOUD_MC at it; developers can also just have mc in PATH.
+################################################################################
+. inc/xbcloud_common.sh
+is_xbcloud_credentials_set
+is_minio_server || skip_test "requires MinIO to list bucket contents"
+
+MC_BIN=${XBCLOUD_MC:-$(command -v mc 2>/dev/null || true)}
+[ -n "$MC_BIN" ] && [ -x "$MC_BIN" ] \
+    || skip_test "requires the MinIO client: set XBCLOUD_MC or put mc in PATH"
+
+ENDPOINT=$(echo "$XBCLOUD_CREDENTIALS" \
+    | awk -F's3-endpoint=' '{print $2}' | awk '{print $1}' | tr -d "'")
+ROOT_KEY=$(echo "$XBCLOUD_CREDENTIALS" \
+    | awk -F's3-access-key=' '{print $2}' | awk '{print $1}' | tr -d "'")
+ROOT_SECRET=$(echo "$XBCLOUD_CREDENTIALS" \
+    | awk -F's3-secret-key=' '{print $2}' | awk '{print $1}' | tr -d "'")
+[ -n "$ENDPOINT" ] && [ -n "$ROOT_KEY" ] && [ -n "$ROOT_SECRET" ] \
+    || skip_test "XBCLOUD_CREDENTIALS lacks s3-endpoint/access-key/secret-key"
+
+# mc keeps its configuration under $HOME, which is not writable in the test
+# image, so give it one of our own.  Credentials go in MC_HOST_<alias> rather
+# than on the command line, where they would end up in the test log.
+# $topdir only exists once the server has been set up, and mc needs a config
+# directory it can write to before that, so fall back to the worker's own
+# scratch space rather than ending up at /mcconf.
+MC="$MC_BIN --config-dir ${topdir:-${MYSQLD_VARDIR:-$PWD/var}}/mcconf.$$"
+export MC_HOST_pxb="${ENDPOINT%%://*}://${ROOT_KEY}:${ROOT_SECRET}@${ENDPOINT#*://}"
+
+# The test owns its bucket: nothing has to exist beforehand, and workers
+# running in parallel cannot collide.  mb -p is happy if it is already there.
+BUCKET="pxbmd5-${uuid}"
+XB_FLAGS="--storage=s3 --s3-endpoint=${ENDPOINT} --s3-bucket=${BUCKET} \
+--s3-access-key=${ROOT_KEY} --s3-secret-key=${ROOT_SECRET} --s3-bucket-lookup=path"
+
+$MC mb -p "pxb/${BUCKET}" >/dev/null 2>&1 || die "could not create bucket ${BUCKET}"
+
+# An alias mc cannot resolve is treated as a local path rather than an error,
+# which would make every listing come back empty instead of failing.  Check it
+# once here so the tests below cannot pass for that reason.
+$MC ls "pxb/${BUCKET}" >/dev/null 2>&1 \
+    || die "cannot reach bucket ${BUCKET} through mc -- check XBCLOUD_CREDENTIALS"
+
+cleanup_md5_delete() {
+    local rc=$?
+    $MC rb --force "pxb/${BUCKET}" >/dev/null 2>&1 || true
+    return $rc
+}
+trap cleanup_md5_delete EXIT
+
+start_server --innodb_file_per_table
+
+md5_backup="md5_backup_${uuid}"
+plain_backup="plain_backup_${uuid}"
+
+mysql -e "CREATE TABLE t (a INT PRIMARY KEY, b VARCHAR(64))" test
+mysql -e "INSERT INTO t VALUES (1,'one'),(2,'two'),(3,'three')" test
+
+# Every object key in the bucket, one per line.  mc ls prints the key last.
+list_files_in_bucket() {
+    $MC ls --recursive "pxb/${BUCKET}" 2>/dev/null | awk '{print $NF}'
+}
+
+# The keys belonging to one backup: everything under <name>/ plus the
+# <name>.md5 file.  Other tests share this bucket, so the question can only be
+# asked per backup, never by checking for an empty bucket.  grep exits 1 when
+# nothing matches, which is the success case here, hence || true.
+list_files_of_backup() {
+    list_files_in_bucket | grep -E "^${1}(/|\.md5$)" || true
+}
+
+################################################################################
+# Scenario 1: a backup taken with --md5 must lose its .md5 file on delete.
+################################################################################
+
+vlog "take a full backup and upload it with --md5"
+
+md5_dir=$topdir/md5_backup
+mkdir -p $md5_dir
+xtrabackup --backup --stream=xbstream --extra-lsndir=$md5_dir \
+    --target-dir=$md5_dir \
+    | run_cmd xbcloud put --md5 --parallel=4 $XB_FLAGS ${md5_backup}
+
+list_files_of_backup "$md5_backup" | grep -q "^${md5_backup}\.md5$" \
+    || die "expected ${md5_backup}.md5 to exist after put --md5"
+vlog "${md5_backup}.md5 present after put"
+
+run_cmd xbcloud delete --parallel=4 $XB_FLAGS ${md5_backup}
+
+remaining=$(list_files_of_backup "$md5_backup")
+if [ -n "$remaining" ]; then
+    echo "PXB-3609: objects left in bucket after delete:" >&2
+    echo "$remaining" >&2
+    die "PXB-3609: xbcloud delete left objects behind: $(echo $remaining)"
+fi
+vlog "delete removed the backup and ${md5_backup}.md5"
+
+################################################################################
+# Scenario 2: a backup taken without --md5 has no .md5 file.  The delete is
+# issued unconditionally -- checking first would make delete require
+# s3:GetObject -- so a file that is not there must not be treated as an error.
+################################################################################
+
+vlog "take a full backup and upload it without --md5"
+
+plain_dir=$topdir/plain_backup
+mkdir -p $plain_dir
+xtrabackup --backup --stream=xbstream --extra-lsndir=$plain_dir \
+    --target-dir=$plain_dir \
+    | run_cmd xbcloud put --parallel=4 $XB_FLAGS ${plain_backup}
+
+list_files_of_backup "$plain_backup" | grep -q "^${plain_backup}\.md5$" \
+    && die "put without --md5 unexpectedly wrote ${plain_backup}.md5"
+
+run_cmd xbcloud delete --parallel=4 $XB_FLAGS ${plain_backup}
+
+remaining=$(list_files_of_backup "$plain_backup")
+[ -z "$remaining" ] \
+    || die "objects left after deleting a backup without --md5: $(echo $remaining)"
+
+vlog "delete of a backup without --md5 completed cleanly"