[#7988][2.4][Platform] Make delete backup task best effort.

Summary:
Backport for ignoring backup deletion failures.
 Original commit: D11207 / 9d00b8b

Test Plan: Unit test

Reviewers: spotachev, wesley

Reviewed By: wesley

Subscribers: jenkins-bot, yugaware

Differential Revision: https://phabricator.dev.yugabyte.com/D11224

managed/src/main/java/com/yugabyte/yw/commissioner/tasks/DeleteBackup.java

@@ -5,23 +5,24 @@
  * may not use this file except in compliance with the License. You
  * may obtain a copy of the License at
  *
- *     https://github.com/YugaByte/yugabyte-db/blob/master/licenses/POLYFORM-FREE-TRIAL-LICENSE-1.0.0.txt
+ * http://github.com/YugaByte/yugabyte-db/blob/master/licenses/POLYFORM-FREE-TRIAL-LICENSE-1.0.0.txt
  */
 
 package com.yugabyte.yw.commissioner.tasks;
 
 import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.collect.ImmutableList;
 import com.yugabyte.yw.commissioner.AbstractTaskBase;
-import com.yugabyte.yw.common.ShellProcessHandler;
+import com.yugabyte.yw.common.ShellProcessHandler.ShellResponse;
 import com.yugabyte.yw.common.TableManager;
 import com.yugabyte.yw.forms.AbstractTaskParams;
 import com.yugabyte.yw.forms.BackupTableParams;
 import com.yugabyte.yw.forms.ITaskParams;
 import com.yugabyte.yw.models.Backup;
-import com.yugabyte.yw.models.Universe;
 import play.api.Play;
 import play.libs.Json;
 
+import java.util.List;
 import java.util.UUID;
 
 
@@ -33,8 +34,9 @@ public static class Params extends AbstractTaskParams {
   }
 
   public Params params() {
-    return (Params)taskParams;
+    return (Params) taskParams;
   }
+
   private TableManager tableManager;
 
   @Override
@@ -45,51 +47,56 @@ public void initialize(ITaskParams params) {
 
   @Override
   public void run() {
-    Backup backup = null;
+    Backup backup = Backup.get(params().customerUUID, params().backupUUID);
+    if (backup.state != Backup.BackupState.Completed) {
+      // TODO: Allow deletion of InProgress backups. But not sure if backend supports it
+      //  and may not be worth the effort.
+      LOG.error("Cannot delete backup in any other state other than completed.");
+      return;
+    }
     try {
-      backup = Backup.get(params().customerUUID, params().backupUUID);
-      if (backup.state != Backup.BackupState.Completed) {
-        LOG.error("Cannot delete backup in any other state other than completed.");
-        throw new RuntimeException("Backup cannot be deleted");
-      }
-      backup.transitionState(Backup.BackupState.Deleted);
       BackupTableParams backupParams = Json.fromJson(backup.backupInfo, BackupTableParams.class);
-      if (backupParams.backupList != null) {
-        for (BackupTableParams childBackupParams : backupParams.backupList) {
-          childBackupParams.actionType = BackupTableParams.ActionType.DELETE;
-          ShellProcessHandler.ShellResponse response = tableManager.deleteBackup(childBackupParams);
-          JsonNode jsonNode = Json.parse(response.message);
-          if (response.code != 0 || jsonNode.has("error")) {
-            // Revert state to completed since it couldn't get deleted.
-            backup.transitionState(Backup.BackupState.Completed);
-            LOG.error("Delete Backup failed for {}. Response code={}, hasError={}.",
-                      childBackupParams.storageLocation, response.code, jsonNode.has("error"));
-            throw new RuntimeException(response.message);
-          } else {
-            LOG.info("[" + getName() + "] STDOUT: " + response.message);
-          }
-        }
-      } else {
-        backupParams.actionType = BackupTableParams.ActionType.DELETE;
-        ShellProcessHandler.ShellResponse response = tableManager.deleteBackup(backupParams);
-        JsonNode jsonNode = Json.parse(response.message);
-        if (response.code != 0 || jsonNode.has("error")) {
-          // Revert state to completed since it couldn't get deleted.
-          backup.transitionState(Backup.BackupState.Completed);
-          LOG.error("Delete Backup failed for {}. Response code={}, hasError={}.",
-                    backupParams.storageLocation, response.code, jsonNode.has("error"));
-          throw new RuntimeException(response.message);
-        } else {
-          LOG.info("[" + getName() + "] STDOUT: " + response.message);
-        }
+      List<BackupTableParams> backupList =
+        backupParams.backupList == null ? ImmutableList.of(backupParams) : backupParams.backupList;
+      if (deleteAllBackups(backupList)) {
+        transitionState(backup, Backup.BackupState.Deleted);
+        return;
       }
-    } catch (Exception e) {
-      LOG.error("Errored out with: " + e);
-      if (backup != null) {
-        // Revert state to completed since it couldn't get deleted.
-        backup.transitionState(Backup.BackupState.Completed);
+    } catch (Exception ex) {
+      LOG.error("Unexpected error in DeleteBackup {}. We will ignore the error and Mark the " +
+        "backup as failed to be deleted and remove it from scheduled cleanup.",
+        params().backupUUID, ex);
+    }
+    transitionState(backup, Backup.BackupState.FailedToDelete);
+  }
+
+  private static void transitionState(Backup backup, Backup.BackupState newState) {
+    if (backup != null) {
+      backup.transitionState(newState);
+    }
+  }
+
+  private boolean deleteAllBackups(List<BackupTableParams> backupList) {
+    boolean success = true;
+    for (BackupTableParams childBackupParams : backupList) {
+      if (!deleteBackup(childBackupParams)) {
+        success = false;
       }
-      throw new RuntimeException(e);
+    }
+    return success;
+  }
+
+  private boolean deleteBackup(BackupTableParams backupTableParams) {
+    backupTableParams.actionType = BackupTableParams.ActionType.DELETE;
+    ShellResponse response = tableManager.deleteBackup(backupTableParams);
+    JsonNode jsonNode = Json.parse(response.message);
+    if (response.code != 0 || jsonNode.has("error")) {
+      LOG.error("Delete Backup failed for {}. Response code={}, hasError={}.",
+        backupTableParams.storageLocation, response.code, jsonNode.has("error"));
+      return false;
+    } else {
+      LOG.info("[" + getName() + "] STDOUT: " + response.message);
+      return true;
     }
   }
 }

managed/src/main/java/com/yugabyte/yw/models/Backup.java

@@ -42,7 +42,11 @@ public enum BackupState {
     Deleted,
 
     @EnumValue("Skipped")
-    Skipped
+    Skipped,
+
+    // Complete or partial failure to delete
+    @EnumValue("FailedToDelete")
+    FailedToDelete,
   }
 
   @Id
@@ -216,11 +220,11 @@ public void transitionState(BackupState newState) {
     // Or completed to deleted state.
     if ((this.state == BackupState.InProgress && this.state != newState) ||
         (this.state == BackupState.Completed && newState == BackupState.Deleted) ||
-        // This condition is for the case in which the delete fails and we need
-        // to reset the state.
-        (this.state == BackupState.Deleted && newState == BackupState.Completed)) {
+        (this.state == BackupState.Completed && newState == BackupState.FailedToDelete)) {
       this.state = newState;
       save();
+    } else {
+      LOG.error("Ignored INVALID STATE TRANSITION  {} -> {}", state, newState);
     }
   }
 

managed/src/test/java/com/yugabyte/yw/commissioner/tasks/DeleteBackupTest.java

@@ -0,0 +1,125 @@
+/*
+ * Copyright 2021 YugaByte, Inc. and Contributors
+ *
+ * Licensed under the Polyform Free Trial License 1.0.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://github.com/YugaByte/yugabyte-db/blob/master/licenses/POLYFORM-FREE-TRIAL-LICENSE-1.0.0.txt
+ */
+
+package com.yugabyte.yw.commissioner.tasks;
+
+import com.yugabyte.yw.common.FakeDBApplication;
+import com.yugabyte.yw.common.ModelFactory;
+import com.yugabyte.yw.common.ShellProcessHandler.ShellResponse;
+import com.yugabyte.yw.models.Backup;
+import com.yugabyte.yw.models.Customer;
+import com.yugabyte.yw.models.CustomerConfig;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.util.UUID;
+
+import static com.yugabyte.yw.models.Backup.BackupState.*;
+import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.*;
+
+@RunWith(MockitoJUnitRunner.class)
+public class DeleteBackupTest  extends FakeDBApplication {
+
+  private Customer defaultCustomer;
+  private Backup backup;
+
+  @Before
+  public void setUp() {
+    UUID universeUUID = UUID.randomUUID();
+    defaultCustomer = ModelFactory.testCustomer();
+    CustomerConfig s3StorageConfig = ModelFactory.createS3StorageConfig(defaultCustomer);
+    backup = ModelFactory.createBackup(defaultCustomer.uuid,
+      universeUUID, s3StorageConfig.configUUID);
+  }
+
+  // Test that only backups in Complete state can be deleted.
+  // Otherwise the run of backup task is a no-op
+  @Test
+  public void invalid() {
+    assertEquals(InProgress, backup.state);
+    DeleteBackup.Params params = new DeleteBackup.Params();
+    params.backupUUID = backup.backupUUID;
+    params.customerUUID = defaultCustomer.uuid;
+
+    DeleteBackup deleteBackupTask = new DeleteBackup();
+    deleteBackupTask.initialize(params);
+    deleteBackupTask.run();
+
+    Backup backup = Backup.get(params.customerUUID, params.backupUUID);
+    assertEquals(InProgress, backup.state);
+  }
+
+  @Test
+  public void success() {
+    backup.transitionState(Completed);
+    DeleteBackup.Params params = new DeleteBackup.Params();
+    params.backupUUID = backup.backupUUID;
+    params.customerUUID = defaultCustomer.uuid;
+
+    ShellResponse shellResponse =  new ShellResponse();
+    shellResponse.message = "{\"success\": true}";
+    shellResponse.code = 0;
+    when(mockTableManager.deleteBackup(any())).thenReturn(shellResponse);
+
+    DeleteBackup deleteBackupTask = new DeleteBackup();
+    deleteBackupTask.initialize(params);
+    deleteBackupTask.run();
+
+    verify(mockTableManager, times(1)).deleteBackup(any());
+    Backup backup = Backup.get(params.customerUUID, params.backupUUID);
+    assertEquals(Deleted, backup.state);
+  }
+
+  @Test
+  public void failure() {
+    backup.transitionState(Completed);
+    DeleteBackup.Params params = new DeleteBackup.Params();
+    params.backupUUID = backup.backupUUID;
+    params.customerUUID = defaultCustomer.uuid;
+
+    ShellResponse shellResponse =  new ShellResponse();
+    shellResponse.message = "{\"success\": false}";
+    shellResponse.code = 22;
+    when(mockTableManager.deleteBackup(any())).thenReturn(shellResponse);
+
+    DeleteBackup deleteBackupTask = new DeleteBackup();
+    deleteBackupTask.initialize(params);
+    deleteBackupTask.run();
+
+    verify(mockTableManager, times(1)).deleteBackup(any());
+    Backup backup = Backup.get(params.customerUUID, params.backupUUID);
+    assertEquals(FailedToDelete, backup.state);
+  }
+
+  @Test
+  public void unexpectedException() {
+    backup.transitionState(Completed);
+    DeleteBackup.Params params = new DeleteBackup.Params();
+    params.backupUUID = backup.backupUUID;
+    params.customerUUID = defaultCustomer.uuid;
+
+    ShellResponse shellResponse =  new ShellResponse();
+    shellResponse.message = "{\"success\": false}";
+    shellResponse.code = 22;
+    when(mockTableManager.deleteBackup(any())).thenThrow(new RuntimeException("expected"));
+
+    DeleteBackup deleteBackupTask = new DeleteBackup();
+    deleteBackupTask.initialize(params);
+    deleteBackupTask.run();
+
+    verify(mockTableManager, times(1)).deleteBackup(any());
+    Backup backup = Backup.get(params.customerUUID, params.backupUUID);
+    assertEquals(FailedToDelete, backup.state);
+  }
+}

managed/src/test/java/com/yugabyte/yw/common/FakeDBApplication.java

@@ -43,6 +43,7 @@ public class FakeDBApplication extends WithApplication {
   public DnsManager mockDnsManager;
   public NetworkManager mockNetworkManager;
   public YamlWrapper mockYamlWrapper;
+  public final TableManager mockTableManager = mock(TableManager.class);
 
   @Override
   protected Application provideApplication() {
@@ -84,6 +85,7 @@ protected Application provideApplication() {
         .overrides(bind(NetworkManager.class).toInstance(mockNetworkManager))
         .overrides(bind(DnsManager.class).toInstance(mockDnsManager))
         .overrides(bind(YamlWrapper.class).toInstance(mockYamlWrapper))
+        .overrides(bind(TableManager.class).toInstance(mockTableManager))
         .build();
   }
 }

managed/src/test/java/com/yugabyte/yw/models/BackupTest.java

@@ -7,8 +7,11 @@
 import com.yugabyte.yw.common.ModelFactory;
 import com.yugabyte.yw.common.RegexMatcher;
 import com.yugabyte.yw.forms.BackupTableParams;
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import play.libs.Json;
 
 import java.util.*;
@@ -27,6 +30,7 @@
 import static org.junit.Assert.assertThat;
 
 
+@RunWith(JUnitParamsRunner.class)
 public class BackupTest extends FakeDBApplication {
   private Customer defaultCustomer;
   private CustomerConfig s3StorageConfig;
@@ -164,33 +168,34 @@ public void testGetWithInvalidCustomerUUID() {
 
 
   @Test
-  public void testTransitionStateValid() throws InterruptedException {
+  @Parameters({
+    "InProgress, Completed",
+    "Completed, Deleted",
+    "Completed, FailedToDelete"})
+  public void testTransitionStateValid(Backup.BackupState from, Backup.BackupState to) {
     Universe u = ModelFactory.createUniverse(defaultCustomer.getCustomerId());
     Backup b = ModelFactory.createBackup(defaultCustomer.uuid,
         u.universeUUID, s3StorageConfig.configUUID);
-    Date beforeUpdateTime  = b.getUpdateTime();
-    assertNotNull(beforeUpdateTime);
-    Thread.sleep(1);
-    b.transitionState(Backup.BackupState.Completed);
-    assertEquals(Completed, b.state);
-    assertNotNull(b.getUpdateTime());
-    assertNotEquals(beforeUpdateTime, b.getUpdateTime());
-    b.transitionState(Deleted);
-    assertEquals(Deleted, b.state);
+    b.transitionState(from);
+    assertEquals(from, b.state);
+    b.transitionState(to);
+    assertEquals(to, b.state);
   }
 
   @Test
-  public void testTransitionStateInvalid() throws InterruptedException {
+  @Parameters({"Completed, Failed"})
+  public void testTransitionStateInvalid(
+    Backup.BackupState from, Backup.BackupState to) throws InterruptedException {
     Universe u = ModelFactory.createUniverse(defaultCustomer.getCustomerId());
     Backup b = ModelFactory.createBackup(defaultCustomer.uuid,
         u.universeUUID, s3StorageConfig.configUUID);
     Date beforeUpdateTime  = b.getUpdateTime();
     assertNotNull(b.getUpdateTime());
     Thread.sleep(1);
-    b.transitionState(Backup.BackupState.Completed);
+    b.transitionState(from);
     assertNotNull(b.getUpdateTime());
     assertNotEquals(beforeUpdateTime, b.getUpdateTime());
-    b.transitionState(Failed);
+    b.transitionState(to);
     assertNotEquals(Failed, b.state);
   }