AMBARI-19055 - Removing Tasks From host_role_command Causes Upgrades To Show As PENDING (jonathanhurley)

ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CalculatedStatus.java

@@ -53,6 +53,17 @@ public class CalculatedStatus {
    */
   private final double percent;
 
+  /**
+   * A status which represents a COMPLETED state at 0%
+   */
+  public static final CalculatedStatus COMPLETED = new CalculatedStatus(HostRoleStatus.COMPLETED,
+      HostRoleStatus.COMPLETED, 100.0);
+
+  /**
+   * A status which represents a PENDING state at 0%
+   */
+  public static final CalculatedStatus PENDING = new CalculatedStatus(HostRoleStatus.PENDING,
+      HostRoleStatus.PENDING, 0.0);
 
   // ----- Constructors ------------------------------------------------------
 
@@ -79,12 +90,6 @@ public class CalculatedStatus {
     this.percent = percent;
   }
 
-  /**
-   * Static factory method to get Status that represents a Completed state
-   */
-  public static CalculatedStatus getCompletedStatus() {
-    return new CalculatedStatus(HostRoleStatus.COMPLETED, HostRoleStatus.COMPLETED, 100.0);
-  }
 
   // ----- CalculatedStatus --------------------------------------------------
 
@@ -291,14 +296,25 @@ public class CalculatedStatus {
   }
 
   /**
-   * Calculates the overall status of an upgrade.
-   * @param stageDto  the map of stage-to-summary value objects
-   * @param stageIds  the stage ids to consider from the value objects
+   * Calculates the overall status of an upgrade. If there are no tasks, then a
+   * status of {@link HostRoleStatus#COMPLETED} is returned.
+   *
+   * @param stageDto
+   *          the map of stage-to-summary value objects
+   * @param stageIds
+   *          the stage ids to consider from the value objects
    * @return the calculated status
    */
   public static CalculatedStatus statusFromStageSummary(Map<Long, HostRoleCommandStatusSummaryDTO> stageDto,
       Set<Long> stageIds) {
 
+    // if either are empty, then we have no tasks and therefore no status - we
+    // should return COMPLETED. This can happen if someone removes all tasks but
+    // leaves the stages and request
+    if (stageDto.isEmpty() || stageIds.isEmpty()) {
+      return COMPLETED;
+    }
+
     Collection<HostRoleStatus> stageStatuses = new HashSet<>();
     Collection<HostRoleStatus> stageDisplayStatuses = new HashSet<>();
     Collection<HostRoleStatus> taskStatuses = new ArrayList<>();
@@ -378,19 +394,28 @@ public class CalculatedStatus {
    */
   public static HostRoleStatus calculateSummaryStatusOfStage(Map<HostRoleStatus, Integer> counters,
       int total, boolean skippable) {
+
+    // when there are 0 tasks, return COMPLETED
+    if (total == 0) {
+      return HostRoleStatus.COMPLETED;
+    }
+
     if (counters.get(HostRoleStatus.PENDING) == total) {
       return HostRoleStatus.PENDING;
     }
+
     // By definition, any tasks in a future stage must be held in a PENDING status.
     if (counters.get(HostRoleStatus.HOLDING) > 0 || counters.get(HostRoleStatus.HOLDING_FAILED) > 0 || counters.get(HostRoleStatus.HOLDING_TIMEDOUT) > 0) {
       return counters.get(HostRoleStatus.HOLDING) > 0 ? HostRoleStatus.HOLDING :
       counters.get(HostRoleStatus.HOLDING_FAILED) > 0 ? HostRoleStatus.HOLDING_FAILED :
       HostRoleStatus.HOLDING_TIMEDOUT;
     }
+
     // Because tasks are not skippable, guaranteed to be FAILED
     if (counters.get(HostRoleStatus.FAILED) > 0 && !skippable) {
       return HostRoleStatus.FAILED;
     }
+
     // Because tasks are not skippable, guaranteed to be TIMEDOUT
     if (counters.get(HostRoleStatus.TIMEDOUT) > 0  && !skippable) {
       return HostRoleStatus.TIMEDOUT;
@@ -401,9 +426,11 @@ public class CalculatedStatus {
     if (counters.get(HostRoleStatus.ABORTED) > 0 && numActiveTasks == 0) {
       return HostRoleStatus.ABORTED;
     }
+
     if (counters.get(HostRoleStatus.COMPLETED) == total) {
       return HostRoleStatus.COMPLETED;
     }
+
     return HostRoleStatus.IN_PROGRESS;
   }
 
@@ -415,7 +442,8 @@ public class CalculatedStatus {
    *
    * @return summary request status based on statuses of tasks in different states.
    */
-  private static HostRoleStatus calculateSummaryStatusOfUpgrade(Map<HostRoleStatus, Integer> counters, int total) {
+  protected static HostRoleStatus calculateSummaryStatusOfUpgrade(
+      Map<HostRoleStatus, Integer> counters, int total) {
     return calculateSummaryStatusOfStage(counters, total, false);
   }
 
@@ -428,8 +456,8 @@ public class CalculatedStatus {
    *
    * @return summary request status based on statuses of tasks in different states.
    */
-  private static HostRoleStatus calculateSummaryDisplayStatus(Map<HostRoleStatus, Integer> counters,
-                                                              int total, boolean skippable) {
+  protected static HostRoleStatus calculateSummaryDisplayStatus(
+      Map<HostRoleStatus, Integer> counters, int total, boolean skippable) {
     return counters.get(HostRoleStatus.SKIPPED_FAILED) > 0 ? HostRoleStatus.SKIPPED_FAILED :
            counters.get(HostRoleStatus.FAILED) > 0 ? HostRoleStatus.FAILED:
            calculateSummaryStatusOfStage(counters, total, skippable);

ambari-server/src/main/java/org/apache/ambari/server/controller/internal/RequestResourceProvider.java

@@ -69,10 +69,10 @@ import org.apache.ambari.server.state.Cluster;
 import org.apache.ambari.server.state.Clusters;
 import org.apache.ambari.server.topology.LogicalRequest;
 import org.apache.ambari.server.topology.TopologyManager;
+import org.apache.commons.lang.StringUtils;
 
 import com.google.common.collect.Sets;
 import com.google.inject.Inject;
-import org.apache.commons.lang.StringUtils;
 
 /**
  * Resource provider for request resources.
@@ -566,13 +566,18 @@ public class RequestResourceProvider extends AbstractControllerResourceProvider
 
     @Override
     public boolean equals(Object o) {
-      if (this == o) return true;
-      if (o == null || getClass() != o.getClass()) return false;
+      if (this == o) {
+        return true;
+      }
+      if (o == null || getClass() != o.getClass()) {
+        return false;
+      }
 
       ServiceComponentTuple that = (ServiceComponentTuple) o;
 
-      if (serviceName != null ? !serviceName.equals(that.serviceName) : that.serviceName != null)
+      if (serviceName != null ? !serviceName.equals(that.serviceName) : that.serviceName != null) {
         return false;
+      }
       return !(componentName != null ? !componentName.equals(that.componentName) : that.componentName != null);
 
     }
@@ -739,15 +744,20 @@ public class RequestResourceProvider extends AbstractControllerResourceProvider
     // get summaries from TopologyManager for logical requests
     summary.putAll(topologyManager.getStageSummaries(entity.getRequestId()));
 
+    // summary might be empty due to delete host have cleared all
+    // HostRoleCommands or due to hosts haven't registered yet with the cluster
+    // when the cluster is provisioned with a Blueprint
+    final CalculatedStatus status;
     LogicalRequest logicalRequest = topologyManager.getRequest(entity.getRequestId());
-
-    CalculatedStatus status = CalculatedStatus.statusFromStageSummary(summary, summary.keySet());
-    if (summary.isEmpty() && logicalRequest == null) {
-
-      // summary might be empty due to delete host have cleared all HostRoleCommands
-      // or due to hosts haven't registered yet with the cluster when the cluster is provisioned
-      // with a Blueprint
-      status = CalculatedStatus.getCompletedStatus();
+    if (summary.isEmpty() && null != logicalRequest) {
+      // in this case, it appears that there are no tasks but this is a logical
+      // topology request, so it's a matter of hosts simply not registering yet
+      // for tasks to be created
+      status = CalculatedStatus.PENDING;
+    } else {
+      // there are either tasks or this is not a logical request, so do normal
+      // status calculations
+      status = CalculatedStatus.statusFromStageSummary(summary, summary.keySet());
     }
 
     setResourceProperty(resource, REQUEST_STATUS_PROPERTY_ID, status.getStatus().toString(), requestedPropertyIds);

ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StageResourceProvider.java

@@ -339,7 +339,7 @@ public class StageResourceProvider extends AbstractControllerResourceProvider im
     CalculatedStatus status;
     if (summary.isEmpty()) {
       // Delete host might have cleared all HostRoleCommands
-      status = CalculatedStatus.getCompletedStatus();
+      status = CalculatedStatus.COMPLETED;
     } else {
       status = CalculatedStatus.statusFromStageSummary(summary, Collections.singleton(entity.getStageId()));
     }

ambari-server/src/main/java/org/apache/ambari/server/topology/TopologyManager.java

@@ -596,6 +596,14 @@ public class TopologyManager {
     return clusterTopologyMap.get(clusterId);
   }
 
+  /**
+   * Gets a map of components keyed by host which have operations in the
+   * {@link HostRoleStatus#PENDING} state. This could either be because hosts
+   * have not registered or becuase the operations are actually waiting to be
+   * queued.
+   *
+   * @return a mapping of host with pending components.
+   */
   public Map<String, Collection<String>> getPendingHostComponents() {
     ensureInitialized();
     Map<String, Collection<String>> hostComponentMap = new HashMap<String, Collection<String>>();
@@ -603,7 +611,16 @@ public class TopologyManager {
     for (LogicalRequest logicalRequest : allRequests.values()) {
       Map<Long, HostRoleCommandStatusSummaryDTO> summary = logicalRequest.getStageSummaries();
       final CalculatedStatus status = CalculatedStatus.statusFromStageSummary(summary, summary.keySet());
-      if (status.getStatus().isInProgress()) {
+
+      // either use the calculated status of the stage or the fact that there
+      // are no tasks and the request has no end time to determine if the
+      // request is still in progress
+      boolean logicalRequestInProgress = false;
+      if (status.getStatus().isInProgress() || (summary.isEmpty() && logicalRequest.getEndTime() <= 0) ) {
+        logicalRequestInProgress = true;
+      }
+
+      if (logicalRequestInProgress) {
         Map<String, Collection<String>> requestTopology = logicalRequest.getProjectedTopology();
         for (Map.Entry<String, Collection<String>> entry : requestTopology.entrySet()) {
           String host = entry.getKey();

ambari-server/src/test/java/org/apache/ambari/server/controller/internal/CalculatedStatusTest.java

@@ -657,6 +657,37 @@ public class CalculatedStatusTest {
     assertEquals(HostRoleStatus.IN_PROGRESS, calc.getStatus());
   }
 
+  /**
+   * Tests that when there are no tasks and all counts are 0, that the returned
+   * status is {@link HostRoleStatus#COMPLETED}.
+   *
+   * @throws Exception
+   */
+  @Test
+  public void testGetCompletedStatusForNoTasks() throws Exception {
+    // no status / no tasks
+    CalculatedStatus status = CalculatedStatus.statusFromTaskEntities(
+        new ArrayList<HostRoleCommandEntity>(), false);
+
+    assertEquals(HostRoleStatus.COMPLETED, status.getStatus());
+
+    // empty summaries
+    status = CalculatedStatus.statusFromStageSummary(
+        new HashMap<Long, HostRoleCommandStatusSummaryDTO>(), new HashSet<Long>());
+
+    assertEquals(HostRoleStatus.COMPLETED, status.getStatus());
+
+    // generate a map of 0's - COMPLETED=0, IN_PROGRESS=0, etc
+    Map<HostRoleStatus, Integer> counts = CalculatedStatus.calculateStatusCounts(new ArrayList<HostRoleStatus>());
+    Map<HostRoleStatus, Integer> displayCounts = CalculatedStatus.calculateStatusCounts(new ArrayList<HostRoleStatus>());
+
+    HostRoleStatus hostRoleStatus = CalculatedStatus.calculateSummaryStatusOfUpgrade(counts, 0);
+    HostRoleStatus hostRoleDisplayStatus = CalculatedStatus.calculateSummaryDisplayStatus(displayCounts, 0, false);
+
+    assertEquals(HostRoleStatus.COMPLETED, hostRoleStatus);
+    assertEquals(HostRoleStatus.COMPLETED, hostRoleDisplayStatus);
+  }
+
   private Collection<HostRoleCommandEntity> getTaskEntities(HostRoleStatus... statuses) {
     Collection<HostRoleCommandEntity> entities = new LinkedList<HostRoleCommandEntity>();
 

ambari-server/src/test/java/org/apache/ambari/server/controller/internal/RequestResourceProviderTest.java

@@ -20,11 +20,10 @@ package org.apache.ambari.server.controller.internal;
 
 
 import static org.apache.ambari.server.controller.internal.HostComponentResourceProvider.HOST_COMPONENT_STALE_CONFIGS_PROPERTY_ID;
-import org.apache.ambari.server.topology.Blueprint;
 import static org.easymock.EasyMock.anyObject;
 import static org.easymock.EasyMock.capture;
-import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.eq;
+import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.newCapture;
 import static org.powermock.api.easymock.PowerMock.createMock;
 import static org.powermock.api.easymock.PowerMock.createNiceMock;
@@ -77,6 +76,7 @@ import org.apache.ambari.server.security.authorization.AuthorizationHelperInitia
 import org.apache.ambari.server.security.authorization.RoleAuthorization;
 import org.apache.ambari.server.state.Cluster;
 import org.apache.ambari.server.state.Clusters;
+import org.apache.ambari.server.topology.Blueprint;
 import org.apache.ambari.server.topology.ClusterTopology;
 import org.apache.ambari.server.topology.HostGroupInfo;
 import org.apache.ambari.server.topology.LogicalRequest;
@@ -1200,7 +1200,7 @@ public class RequestResourceProviderTest {
       }
     }
     Assert.assertNotNull(propertyIdToAssert);
-    Assert.assertEquals("true", (String) propertyValueToAssert);
+    Assert.assertEquals("true", propertyValueToAssert);
   }
 
   @Test
@@ -1624,6 +1624,12 @@ public class RequestResourceProviderTest {
     verify(managementController, actionManager, clusters, requestMock, requestDAO, hrcDAO);
   }
 
+  /**
+   * Tests that topology requests return different status (PENDING) if there are
+   * no tasks. Normal requests should return COMPLETED.
+   *
+   * @throws Exception
+   */
   @Test
   @PrepareForTest(AmbariServer.class)
   public void testGetLogicalRequestStatusWithNoTasks() throws Exception {