From ed06d250fbb1072bb62e9819b481e27b8f88cdf2 Mon Sep 17 00:00:00 2001 From: Naorem Khogendro Singh Date: Mon, 11 Sep 2023 16:59:33 -0700 Subject: [PATCH] [BACKPORT 2.14][PLAT-9814] Do not garbage collect the customer task which is still being referred in the universe task or the last task for a provider Summary: Original diff - https://phorge.dev.yugabyte.com/D28457 (0bd85937dbfe15bf6b9f3d288d0fe506c93dd193) This change makes sure that the task garbage collector does not delete the lastest task for provider or if the task UUID is being referenced in a universe. No change in API. Test Plan: Unit tests Reviewers: cwang, amalyshev, sanketh Reviewed By: amalyshev Subscribers: yugaware Differential Revision: https://phorge.dev.yugabyte.com/D28596 --- .../yw/commissioner/Commissioner.java | 35 ++++-- .../yw/commissioner/TaskExecutor.java | 48 ++++---- .../yw/commissioner/TaskGarbageCollector.java | 29 +++-- .../yw/common/CustomerTaskManager.java | 70 ++++++------ .../com/yugabyte/yw/models/CustomerTask.java | 50 ++++++--- .../java/com/yugabyte/yw/models/TaskInfo.java | 13 ++- .../common/V290__Alter_Task_Info_Table.sql | 8 ++ .../V291__Alter_Customer_Task_Table.sql | 7 ++ .../TaskGarbageCollectorTest.java | 106 ++++++++++++++---- .../yugabyte/yw/models/CustomerTaskTest.java | 16 --- 10 files changed, 246 insertions(+), 136 deletions(-) create mode 100644 managed/src/main/resources/db/migration/default/common/V290__Alter_Task_Info_Table.sql create mode 100644 managed/src/main/resources/db/migration/default/common/V291__Alter_Customer_Task_Table.sql diff --git a/managed/src/main/java/com/yugabyte/yw/commissioner/Commissioner.java b/managed/src/main/java/com/yugabyte/yw/commissioner/Commissioner.java index 29689d4e2508..16268270eefe 100644 --- a/managed/src/main/java/com/yugabyte/yw/commissioner/Commissioner.java +++ b/managed/src/main/java/com/yugabyte/yw/commissioner/Commissioner.java @@ -2,6 +2,8 @@ package com.yugabyte.yw.commissioner; +import static play.mvc.Http.Status.BAD_REQUEST; + import akka.actor.ActorSystem; import akka.actor.Scheduler; import com.fasterxml.jackson.databind.JsonNode; @@ -21,6 +23,23 @@ import com.yugabyte.yw.models.TaskInfo; import com.yugabyte.yw.models.Universe; import com.yugabyte.yw.models.helpers.TaskType; +import java.time.Duration; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Iterator; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Optional; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.CancellationException; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; import lombok.extern.slf4j.Slf4j; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -29,15 +48,6 @@ import play.libs.Json; import scala.concurrent.ExecutionContext; -import java.time.Duration; -import java.util.*; -import java.util.Map.Entry; -import java.util.concurrent.*; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Consumer; - -import static play.mvc.Http.Status.BAD_REQUEST; - @Singleton public class Commissioner { @@ -114,7 +124,12 @@ public UUID submit(TaskType taskType, ITaskParams taskParams) { } catch (Throwable t) { if (taskRunnable != null) { // Destroy the task initialization in case of failure. - taskRunnable.task.terminate(); + taskRunnable.getTask().terminate(); + TaskInfo taskInfo = taskRunnable.getTaskInfo(); + if (taskInfo.getTaskState() != TaskInfo.State.Failure) { + taskInfo.setTaskState(TaskInfo.State.Failure); + taskInfo.save(); + } } String msg = "Error processing " + taskType + " task for " + taskParams.toString(); LOG.error(msg, t); diff --git a/managed/src/main/java/com/yugabyte/yw/commissioner/TaskExecutor.java b/managed/src/main/java/com/yugabyte/yw/commissioner/TaskExecutor.java index 1301ea7115a0..be265209d09a 100644 --- a/managed/src/main/java/com/yugabyte/yw/commissioner/TaskExecutor.java +++ b/managed/src/main/java/com/yugabyte/yw/commissioner/TaskExecutor.java @@ -402,7 +402,7 @@ public Optional abort(UUID taskUUID) { return Optional.empty(); } RunnableTask runnableTask = optional.get(); - ITask task = runnableTask.task; + ITask task = runnableTask.getTask(); if (!isTaskAbortable(task.getClass())) { throw new RuntimeException("Task " + task.getName() + " is not abortable"); } @@ -415,7 +415,7 @@ public Optional abort(UUID taskUUID) { // Update the task state in the memory and DB. runnableTask.compareAndSetTaskState( Sets.immutableEnumSet(State.Initializing, State.Created, State.Running), State.Abort); - return Optional.of(runnableTask.taskInfo); + return Optional.of(runnableTask.getTaskInfo()); } /** @@ -598,7 +598,7 @@ private void removeCompletedSubTask( RunnableSubTask runnableSubTask, Throwable throwable) { if (throwable != null) { - log.error("Error occurred in subtask " + runnableSubTask.taskInfo, throwable); + log.error("Error occurred in subtask " + runnableSubTask.getTaskInfo(), throwable); } taskIterator.remove(); numTasksCompleted.incrementAndGet(); @@ -651,7 +651,7 @@ private void waitForSubTasks() { } else if (abortTime != null && Duration.between(abortTime, Instant.now()).compareTo(defaultAbortTaskTimeout) > 0 && (skipSubTaskAbortableCheck - || isTaskAbortable(runnableSubTask.task.getClass()))) { + || isTaskAbortable(runnableSubTask.getTask().getClass()))) { future.cancel(true); // Report aborted to the parent task. // Update the subtask state to aborted if the execution timed out. @@ -719,15 +719,15 @@ public String toString() { * started running. Synchronization is on the this object for taskInfo. */ public abstract class AbstractRunnableTask implements Runnable { - final ITask task; - final TaskInfo taskInfo; + private final ITask task; + private final TaskInfo taskInfo; // Timeout limit for this task. - final Duration timeLimit; - final String[] creatorCallstack; + private final Duration timeLimit; + private final String[] creatorCallstack; - Instant taskScheduledTime; - Instant taskStartTime; - Instant taskCompletionTime; + private Instant taskScheduledTime; + private Instant taskStartTime; + private Instant taskCompletionTime; // Future of the task that is set after it is submitted to the ExecutorService. Future future = null; @@ -761,6 +761,14 @@ protected AbstractRunnableTask(ITask task, TaskInfo taskInfo) { } } + public ITask getTask() { + return task; + } + + public TaskInfo getTaskInfo() { + return taskInfo; + } + @VisibleForTesting String[] getCreatorCallstack() { return creatorCallstack; @@ -957,9 +965,9 @@ public void setTaskExecutionListener(TaskExecutionListener taskExecutionListener /** Invoked by the ExecutorService. Do not invoke this directly. */ @Override public void run() { - UUID taskUUID = taskInfo.getTaskUUID(); + UUID taskUUID = getTaskInfo().getTaskUUID(); try { - task.setUserTaskUUID(taskUUID); + getTask().setUserTaskUUID(taskUUID); super.run(); } catch (Exception e) { Throwables.propagate(e); @@ -1127,7 +1135,7 @@ private void executeWith(ExecutorService executorService) { @Override public void run() { // Sets the top-level user task UUID. - task.setUserTaskUUID(parentRunnableTask.getTaskUUID()); + getTask().setUserTaskUUID(parentRunnableTask.getTaskUUID()); super.run(); } @@ -1142,18 +1150,18 @@ protected synchronized TaskExecutionListener getTaskExecutionListener() { } public synchronized void setSubTaskGroupType(SubTaskGroupType subTaskGroupType) { - if (taskInfo.getSubTaskGroupType() != subTaskGroupType) { - taskInfo.setSubTaskGroupType(subTaskGroupType); - taskInfo.save(); + if (getTaskInfo().getSubTaskGroupType() != subTaskGroupType) { + getTaskInfo().setSubTaskGroupType(subTaskGroupType); + getTaskInfo().save(); } } private synchronized void setRunnableTaskContext( RunnableTask parentRunnableTask, int position) { this.parentRunnableTask = parentRunnableTask; - taskInfo.setParentUuid(parentRunnableTask.getTaskUUID()); - taskInfo.setPosition(position); - taskInfo.save(); + getTaskInfo().setParentUuid(parentRunnableTask.getTaskUUID()); + getTaskInfo().setPosition(position); + getTaskInfo().save(); } } } diff --git a/managed/src/main/java/com/yugabyte/yw/commissioner/TaskGarbageCollector.java b/managed/src/main/java/com/yugabyte/yw/commissioner/TaskGarbageCollector.java index 5aafe3252d4b..629ae11b14ae 100644 --- a/managed/src/main/java/com/yugabyte/yw/commissioner/TaskGarbageCollector.java +++ b/managed/src/main/java/com/yugabyte/yw/commissioner/TaskGarbageCollector.java @@ -85,7 +85,7 @@ public void start() { } else { log.info("Scheduling TaskGC every " + gcInterval); scheduler.schedule( - Duration.ZERO, // InitialDelay + Duration.ofMinutes(5), // InitialDelay gcInterval, this::scheduleRunner, this.executionContext); @@ -108,17 +108,22 @@ private void checkCustomer(Customer c) { @VisibleForTesting void purgeStaleTasks(Customer c, List staleTasks) { NUM_TASK_GC_RUNS_COUNT.inc(); - int numRowsGCdInThisRun = 0; - for (CustomerTask customerTask : staleTasks) { - int numRowsDeleted = customerTask.cascadeDeleteCompleted(); - numRowsGCdInThisRun += numRowsDeleted; - if (numRowsDeleted > 0) { - PURGED_CUSTOMER_TASK_COUNT.labels(c.getUuid().toString()).inc(); - PURGED_TASK_INFO_COUNT.labels(c.getUuid().toString()).inc(numRowsDeleted - 1); - } else { - NUM_TASK_GC_ERRORS_COUNT.inc(); - } - } + int numRowsGCdInThisRun = + staleTasks + .stream() + .filter(CustomerTask::isDeletable) + .map( + customerTask -> { + int numRowsDeleted = customerTask.cascadeDeleteCompleted(); + if (numRowsDeleted > 0) { + PURGED_CUSTOMER_TASK_COUNT.labels(c.getUuid().toString()).inc(); + PURGED_TASK_INFO_COUNT.labels(c.getUuid().toString()).inc(numRowsDeleted - 1); + } else { + NUM_TASK_GC_ERRORS_COUNT.inc(); + } + return numRowsDeleted; + }) + .reduce(0, Integer::sum); log.info("Garbage collected {} rows", numRowsGCdInThisRun); } diff --git a/managed/src/main/java/com/yugabyte/yw/common/CustomerTaskManager.java b/managed/src/main/java/com/yugabyte/yw/common/CustomerTaskManager.java index ba1654d91cec..76b4d1ffccc8 100644 --- a/managed/src/main/java/com/yugabyte/yw/common/CustomerTaskManager.java +++ b/managed/src/main/java/com/yugabyte/yw/common/CustomerTaskManager.java @@ -2,18 +2,13 @@ package com.yugabyte.yw.common; -import static com.yugabyte.yw.models.CustomerTask.TargetType; - import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ObjectNode; -import com.yugabyte.yw.commissioner.tasks.subtasks.LoadBalancerStateChange; -import com.yugabyte.yw.forms.RestoreBackupParams; -import com.yugabyte.yw.forms.UniverseDefinitionTaskParams; import com.yugabyte.yw.common.services.YBClientService; -import org.yb.client.ChangeLoadBalancerStateResponse; -import org.yb.client.YBClient; +import com.yugabyte.yw.forms.UniverseDefinitionTaskParams; import com.yugabyte.yw.models.Backup; import com.yugabyte.yw.models.CustomerTask; +import com.yugabyte.yw.models.CustomerTask.TargetType; import com.yugabyte.yw.models.ScheduleTask; import com.yugabyte.yw.models.TaskInfo; import com.yugabyte.yw.models.Universe; @@ -25,11 +20,12 @@ import java.util.Optional; import java.util.UUID; import java.util.stream.Collectors; -import javax.inject.Singleton; import javax.inject.Inject; +import javax.inject.Singleton; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import play.api.Play; +import org.yb.client.ChangeLoadBalancerStateResponse; +import org.yb.client.YBClient; @Singleton public class CustomerTaskManager { @@ -67,13 +63,16 @@ public void failPendingTask(CustomerTask customerTask, TaskInfo taskInfo) { subtask.save(); }); + Optional optUniv = + customerTask.getTarget().isUniverseTarget() + ? Universe.maybeGet(customerTask.getTargetUUID()) + : Optional.empty(); if (LOAD_BALANCER_TASK_TYPES.contains(taskInfo.getTaskType())) { Boolean isLoadBalanceAltered = false; JsonNode node = taskInfo.getTaskDetails(); if (node.has(ALTER_LOAD_BALANCER)) { isLoadBalanceAltered = node.path(ALTER_LOAD_BALANCER).asBoolean(false); } - Optional optUniv = Universe.maybeGet(customerTask.getTargetUUID()); if (optUniv.isPresent() && isLoadBalanceAltered) { enableLoadBalancer(optUniv.get()); } @@ -110,25 +109,23 @@ public void failPendingTask(CustomerTask customerTask, TaskInfo taskInfo) { if (unlockUniverse) { // Unlock the universe for future operations. - Universe.maybeGet(customerTask.getTargetUUID()) - .ifPresent( - u -> { - UniverseDefinitionTaskParams details = u.getUniverseDetails(); - if (details.backupInProgress || details.updateInProgress) { - // Create the update lambda. - Universe.UniverseUpdater updater = - universe -> { - UniverseDefinitionTaskParams universeDetails = - universe.getUniverseDetails(); - universeDetails.backupInProgress = false; - universeDetails.updateInProgress = false; - universe.setUniverseDetails(universeDetails); - }; - - Universe.saveDetails(customerTask.getTargetUUID(), updater, false); - LOG.debug("Unlocked universe {}.", customerTask.getTargetUUID()); - } - }); + optUniv.ifPresent( + u -> { + UniverseDefinitionTaskParams details = u.getUniverseDetails(); + if (details.backupInProgress || details.updateInProgress) { + // Create the update lambda. + Universe.UniverseUpdater updater = + universe -> { + UniverseDefinitionTaskParams universeDetails = universe.getUniverseDetails(); + universeDetails.backupInProgress = false; + universeDetails.updateInProgress = false; + universe.setUniverseDetails(universeDetails); + }; + + Universe.saveDetails(customerTask.getTargetUUID(), updater, false); + LOG.debug("Unlocked universe {}.", customerTask.getTargetUUID()); + } + }); } // Mark task as a failure after the universe is unlocked. @@ -153,9 +150,18 @@ public void failAllPendingTasks() { .stream() .map(Objects::toString) .collect(Collectors.joining("','")); - // Retrieve all incomplete customer tasks or task in incomplete state. Task state update and - // customer completion time update are not transactional. + + // Delete orphaned parent tasks which do not have any associated customer task. + // It is rare but can happen as a customer task and task info are not saved in transaction. + // TODO It can be handled better with transaction but involves bigger change. String query = + "DELETE FROM task_info WHERE parent_uuid IS NULL AND uuid NOT IN " + + "(SELECT task_uuid FROM customer_task)"; + Ebean.createSqlUpdate(query).execute(); + + // Retrieve all incomplete customer tasks or task in incomplete state. Task state update + // and customer completion time update are not transactional. + query = "SELECT ti.uuid AS task_uuid, ct.id AS customer_task_id " + "FROM task_info ti, customer_task ct " + "WHERE ti.uuid = ct.task_uuid " @@ -168,7 +174,7 @@ public void failAllPendingTasks() { .findList() .forEach( row -> { - TaskInfo taskInfo = TaskInfo.get(row.getUUID("task_uuid")); + TaskInfo taskInfo = TaskInfo.getOrBadRequest(row.getUUID("task_uuid")); CustomerTask customerTask = CustomerTask.get(row.getLong("customer_task_id")); failPendingTask(customerTask, taskInfo); }); diff --git a/managed/src/main/java/com/yugabyte/yw/models/CustomerTask.java b/managed/src/main/java/com/yugabyte/yw/models/CustomerTask.java index 111ec286f36f..6a7abb8f78d0 100644 --- a/managed/src/main/java/com/yugabyte/yw/models/CustomerTask.java +++ b/managed/src/main/java/com/yugabyte/yw/models/CustomerTask.java @@ -9,6 +9,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.yugabyte.yw.common.PlatformServiceException; +import com.yugabyte.yw.forms.UniverseDefinitionTaskParams; import io.ebean.Finder; import io.ebean.Model; import io.ebean.annotation.EnumValue; @@ -21,6 +22,7 @@ import java.util.Arrays; import java.util.Date; import java.util.List; +import java.util.Optional; import java.util.UUID; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -543,7 +545,7 @@ public String getFriendlyDescription() { } /** - * deletes customer_task, task_info and all its subtasks of a given task. Assumes task_info tree + * Deletes customer_task, task_info and all its subtasks of a given task. Assumes task_info tree * is one level deep. If this assumption changes then this code needs to be reworked to recurse. * When successful; it deletes at least 2 rows because there is always customer_task and * associated task_info row that get deleted. @@ -555,7 +557,12 @@ public String getFriendlyDescription() { public int cascadeDeleteCompleted() { Preconditions.checkNotNull( completionTime, String.format("CustomerTask %s has not completed", id)); - TaskInfo rootTaskInfo = TaskInfo.get(taskUUID); + Optional optional = TaskInfo.maybeGet(taskUUID); + if (!optional.isPresent()) { + delete(); + return 1; + } + TaskInfo rootTaskInfo = optional.get(); if (!rootTaskInfo.hasCompleted()) { LOG.warn( "Completed CustomerTask(id:{}, type:{}) has incomplete task_info {}", @@ -564,23 +571,11 @@ public int cascadeDeleteCompleted() { rootTaskInfo); return 0; } - List subTasks = rootTaskInfo.getSubTasks(); - List incompleteSubTasks = - subTasks.stream().filter(taskInfo -> !taskInfo.hasCompleted()).collect(Collectors.toList()); - if (rootTaskInfo.getTaskState() == TaskInfo.State.Success && !incompleteSubTasks.isEmpty()) { - LOG.warn( - "For a customer_task.id: {}, Successful task_info.uuid ({}) has {} incomplete subtasks {}", - id, - rootTaskInfo.getTaskUUID(), - incompleteSubTasks.size(), - incompleteSubTasks); - return 0; - } - // Note: delete leaf nodes first to preserve referential integrity. - subTasks.forEach(Model::delete); + int subTaskSize = rootTaskInfo.getSubTasks().size(); + // This performs cascade delete. rootTaskInfo.delete(); - this.delete(); - return 2 + subTasks.size(); + delete(); + return 2 + subTaskSize; } public static CustomerTask findByTaskUUID(UUID taskUUID) { @@ -623,4 +618,23 @@ public String getNotificationTargetName() { return getTargetName(); } } + + public boolean isDeletable() { + if (targetType.isUniverseTarget()) { + Optional optional = Universe.maybeGet(targetUUID); + if (!optional.isPresent()) { + return true; + } + UniverseDefinitionTaskParams taskParams = optional.get().getUniverseDetails(); + if (taskUUID.equals(taskParams.updatingTaskUUID)) { + LOG.debug("Universe task {} is not deletable", targetUUID); + return false; + } + if (taskUUID.equals(taskParams.placementModificationTaskUuid)) { + LOG.debug("Universe task {} is not deletable", targetUUID); + return false; + } + } + return true; + } } diff --git a/managed/src/main/java/com/yugabyte/yw/models/TaskInfo.java b/managed/src/main/java/com/yugabyte/yw/models/TaskInfo.java index a6bb797e65b4..04a3481003d0 100644 --- a/managed/src/main/java/com/yugabyte/yw/models/TaskInfo.java +++ b/managed/src/main/java/com/yugabyte/yw/models/TaskInfo.java @@ -20,10 +20,10 @@ import io.ebean.Finder; import io.ebean.Model; import io.ebean.Query; -import io.ebean.annotation.CreatedTimestamp; +import io.ebean.annotation.WhenCreated; import io.ebean.annotation.DbJson; import io.ebean.annotation.EnumValue; -import io.ebean.annotation.UpdatedTimestamp; +import io.ebean.annotation.WhenModified; import io.swagger.annotations.ApiModel; import io.swagger.annotations.ApiModelProperty; import java.util.Collection; @@ -33,6 +33,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.UUID; import java.util.stream.Collectors; @@ -120,12 +121,12 @@ public enum State { private UserTaskDetails.SubTaskGroupType subTaskGroupType; // The task creation time. - @CreatedTimestamp + @WhenCreated @ApiModelProperty(value = "Creation time", accessMode = READ_ONLY, example = "1624295239113") private Date createTime; // The task update time. Time of the latest update (including heartbeat updates) on this task. - @UpdatedTimestamp + @WhenModified @ApiModelProperty(value = "Updated time", accessMode = READ_ONLY, example = "1624295239113") private Date updateTime; @@ -252,6 +253,10 @@ public static TaskInfo get(UUID taskUUID) { return find.byId(taskUUID); } + public static Optional maybeGet(UUID taskUUID) { + return Optional.ofNullable(get(taskUUID)); + } + public static TaskInfo getOrBadRequest(UUID taskUUID) { TaskInfo taskInfo = get(taskUUID); if (taskInfo == null) { diff --git a/managed/src/main/resources/db/migration/default/common/V290__Alter_Task_Info_Table.sql b/managed/src/main/resources/db/migration/default/common/V290__Alter_Task_Info_Table.sql new file mode 100644 index 000000000000..9e2da41d166a --- /dev/null +++ b/managed/src/main/resources/db/migration/default/common/V290__Alter_Task_Info_Table.sql @@ -0,0 +1,8 @@ +-- Copyright (c) YugaByte, Inc. + +-- Delete invalid subtasks from task_info before creating the contraint. +DELETE FROM task_info WHERE parent_uuid IS NOT NULL AND parent_uuid NOT IN + (SELECT uuid FROM task_info WHERE parent_uuid IS NULL); + +ALTER TABLE task_info ADD CONSTRAINT fk_task_info_parent_uuid FOREIGN KEY (parent_uuid) + REFERENCES task_info (uuid) ON DELETE CASCADE ON UPDATE CASCADE; diff --git a/managed/src/main/resources/db/migration/default/common/V291__Alter_Customer_Task_Table.sql b/managed/src/main/resources/db/migration/default/common/V291__Alter_Customer_Task_Table.sql new file mode 100644 index 000000000000..15a0821d9ed6 --- /dev/null +++ b/managed/src/main/resources/db/migration/default/common/V291__Alter_Customer_Task_Table.sql @@ -0,0 +1,7 @@ +-- Copyright (c) YugaByte, Inc. + +CREATE INDEX ix_customer_task_task_uuid ON customer_task (customer_uuid, task_uuid); + +CREATE INDEX ix_customer_task_target_uuid ON customer_task (target_uuid); + + diff --git a/managed/src/test/java/com/yugabyte/yw/commissioner/TaskGarbageCollectorTest.java b/managed/src/test/java/com/yugabyte/yw/commissioner/TaskGarbageCollectorTest.java index c80a4be964af..0db3270ba8d5 100644 --- a/managed/src/test/java/com/yugabyte/yw/commissioner/TaskGarbageCollectorTest.java +++ b/managed/src/test/java/com/yugabyte/yw/commissioner/TaskGarbageCollectorTest.java @@ -7,8 +7,13 @@ import static com.yugabyte.yw.commissioner.TaskGarbageCollector.TASK_INFO_METRIC_NAME; import static com.yugabyte.yw.commissioner.TaskGarbageCollector.YB_TASK_GC_GC_CHECK_INTERVAL; import static io.prometheus.client.CollectorRegistry.defaultRegistry; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.eq; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyZeroInteractions; @@ -16,14 +21,20 @@ import akka.actor.ActorSystem; import akka.actor.Scheduler; +import com.fasterxml.jackson.databind.ObjectMapper; import com.typesafe.config.Config; +import com.yugabyte.yw.common.FakeDBApplication; +import com.yugabyte.yw.common.ModelFactory; import com.yugabyte.yw.common.config.RuntimeConfigFactory; import com.yugabyte.yw.models.Customer; import com.yugabyte.yw.models.CustomerTask; +import com.yugabyte.yw.models.CustomerTask.TargetType; +import com.yugabyte.yw.models.TaskInfo; +import com.yugabyte.yw.models.helpers.TaskType; import java.time.Duration; import java.util.Collections; +import java.util.Date; import java.util.UUID; -import junit.framework.TestCase; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -32,7 +43,7 @@ import scala.concurrent.ExecutionContext; @RunWith(MockitoJUnitRunner.class) -public class TaskGarbageCollectorTest extends TestCase { +public class TaskGarbageCollectorTest extends FakeDBApplication { private void checkCounters( UUID customerUuid, @@ -72,10 +83,15 @@ private void checkCounters( @Mock CustomerTask mockCustomerTask; + private final ObjectMapper mapper = new ObjectMapper(); + private TaskGarbageCollector taskGarbageCollector; + private Customer defaultCustomer; + @Before public void setUp() { + defaultCustomer = ModelFactory.testCustomer(); when(mockRuntimeConfigFactory.staticApplicationConf()).thenReturn(mockAppConfig); when(mockActorSystem.scheduler()).thenReturn(mockScheduler); taskGarbageCollector = @@ -85,52 +101,94 @@ public void setUp() { } @Test - public void testStart_disabled() { + public void testStartDisabled() { when(mockAppConfig.getDuration(YB_TASK_GC_GC_CHECK_INTERVAL)).thenReturn(Duration.ZERO); taskGarbageCollector.start(); verifyZeroInteractions(mockScheduler); } @Test - public void testStart_enabled() { + public void testStartEnabled() { when(mockAppConfig.getDuration(YB_TASK_GC_GC_CHECK_INTERVAL)).thenReturn(Duration.ofDays(1)); taskGarbageCollector.start(); verify(mockScheduler, times(1)) - .schedule(eq(Duration.ZERO), eq(Duration.ofDays(1)), any(), eq(mockExecutionContext)); + .schedule( + eq(Duration.ofMinutes(5)), eq(Duration.ofDays(1)), any(), eq(mockExecutionContext)); } @Test - public void testPurge_noneStale() { - UUID customerUuid = UUID.randomUUID(); - - taskGarbageCollector.purgeStaleTasks(mockCustomer, Collections.emptyList()); - - checkCounters(customerUuid, 1.0, 0.0, null, null); + public void testPurgeNoneStale() { + taskGarbageCollector.purgeStaleTasks(defaultCustomer, Collections.emptyList()); + checkCounters(defaultCustomer.getUuid(), 1.0, 0.0, null, null); } @Test public void testPurge() { - UUID customerUuid = UUID.randomUUID(); - when(mockCustomer.getUuid()).thenReturn(customerUuid); - // Pretend we deleted 5 rows in all: + // Pretend we deleted 5 rows in all. when(mockCustomerTask.cascadeDeleteCompleted()).thenReturn(5); + when(mockCustomerTask.isDeletable()).thenReturn(true); + taskGarbageCollector.purgeStaleTasks( + defaultCustomer, Collections.singletonList(mockCustomerTask)); + checkCounters(defaultCustomer.getUuid(), 1.0, 0.0, 1.0, 4.0); + } - taskGarbageCollector.purgeStaleTasks(mockCustomer, Collections.singletonList(mockCustomerTask)); - - checkCounters(customerUuid, 1.0, 0.0, 1.0, 4.0); + // Test that if we do not delete when there are referential integrity issues; then we report such + // error in counter. + @Test + public void testPurgeNonDeletable() { + // Pretend we deleted no rows. + when(mockCustomerTask.isDeletable()).thenReturn(false); + taskGarbageCollector.purgeStaleTasks( + defaultCustomer, Collections.singletonList(mockCustomerTask)); + checkCounters(defaultCustomer.getUuid(), 1.0, 0.0, null, null); } // Test that if we do not delete when there are referential integrity issues; then we report such // error in counter. @Test - public void testPurge_invalidData() { - UUID customerUuid = UUID.randomUUID(); - // Pretend we deleted 5 rows in all: + public void testPurgeInvalidData() { + // Pretend we deleted no rows. when(mockCustomerTask.cascadeDeleteCompleted()).thenReturn(0); + when(mockCustomerTask.isDeletable()).thenReturn(true); + taskGarbageCollector.purgeStaleTasks( + defaultCustomer, Collections.singletonList(mockCustomerTask)); + checkCounters(defaultCustomer.getUuid(), 1.0, 1.0, null, null); + } - taskGarbageCollector.purgeStaleTasks(mockCustomer, Collections.singletonList(mockCustomerTask)); - - checkCounters(customerUuid, 1.0, 1.0, null, null); + @Test + public void testDeletableDBConstraints() { + TaskInfo parentTask = new TaskInfo(TaskType.CreateUniverse); + parentTask.setOwner("test"); + parentTask.setTaskState(TaskInfo.State.Success); + parentTask.setTaskDetails(mapper.createObjectNode()); + parentTask.save(); + + TaskInfo subTask = new TaskInfo(TaskType.CreateUniverse); + subTask.setOwner("test"); + subTask.setParentUuid(parentTask.getTaskUUID()); + subTask.setPosition(0); + subTask.setTaskState(TaskInfo.State.Success); + subTask.setTaskDetails(mapper.createObjectNode()); + subTask.save(); + + UUID targetUuid = UUID.randomUUID(); + CustomerTask customerTask = + spy( + CustomerTask.create( + defaultCustomer, + targetUuid, + parentTask.getTaskUUID(), + TargetType.Universe, + CustomerTask.TaskType.Create, + "test-universe")); + customerTask.markAsCompleted(); + customerTask.save(); + doReturn(true).when(customerTask).isDeletable(); + taskGarbageCollector.purgeStaleTasks(defaultCustomer, Collections.singletonList(customerTask)); + checkCounters(defaultCustomer.getUuid(), 1.0, 0.0, 1.0, 2.0); + assertFalse(TaskInfo.maybeGet(parentTask.getTaskUUID()).isPresent()); + assertFalse(TaskInfo.maybeGet(subTask.getTaskUUID()).isPresent()); + assertTrue(CustomerTask.get(customerTask.getId()) == null); } private String getTotalCounterName(String name) { diff --git a/managed/src/test/java/com/yugabyte/yw/models/CustomerTaskTest.java b/managed/src/test/java/com/yugabyte/yw/models/CustomerTaskTest.java index 0038e9b963ff..8e86bc045179 100644 --- a/managed/src/test/java/com/yugabyte/yw/models/CustomerTaskTest.java +++ b/managed/src/test/java/com/yugabyte/yw/models/CustomerTaskTest.java @@ -200,22 +200,6 @@ public void testCascadeDelete_taskInfoIncomplete_skipped() { assertEquals(th, CustomerTask.findByTaskUUID(th.getTaskUUID())); } - @Test - public void testCascadeDeleteSuccessfulTask_subtasksIncomplete_skipped() { - UUID targetUUID = UUID.randomUUID(); - CustomerTask th = - createTaskTree( - CustomerTask.TargetType.Table, - targetUUID, - Create, - 3, - Optional.of(TaskInfo.State.Success), - false); - th.markAsCompleted(); - assertEquals(0, th.cascadeDeleteCompleted()); - assertEquals(th, CustomerTask.findByTaskUUID(th.getTaskUUID())); - } - @Test public void testCascadeDeleteFailedTask_subtasksIncomplete_success() { UUID targetUUID = UUID.randomUUID();