YARN-7955. Improve result of calling stop on an already stopped service. Contributed by Gour Saha

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services-api/src/main/java/org/apache/hadoop/yarn/service/webapp/ApiServer.java

@@ -59,7 +59,7 @@ import java.util.Map;
 
 import static org.apache.hadoop.yarn.service.api.records.ServiceState.ACCEPTED;
 import static org.apache.hadoop.yarn.service.conf.RestApiConstants.*;
-import static org.apache.hadoop.yarn.service.exceptions.LauncherExitCodes.EXIT_SUCCESS;
+import static org.apache.hadoop.yarn.service.exceptions.LauncherExitCodes.*;
 
 /**
  * The rest API endpoints for users to manage services on YARN.
@@ -240,7 +240,7 @@ public class ApiServer {
   private Response stopService(String appName, boolean destroy,
       final UserGroupInformation ugi) throws IOException,
       InterruptedException, YarnException, FileNotFoundException {
-    ugi.doAs(new PrivilegedExceptionAction<Integer>() {
+    int result = ugi.doAs(new PrivilegedExceptionAction<Integer>() {
       @Override
       public Integer run() throws IOException, YarnException,
           FileNotFoundException {
@@ -249,11 +249,12 @@ public class ApiServer {
         sc.init(YARN_CONFIG);
         sc.start();
         result = sc.actionStop(appName, destroy);
+        if (result == EXIT_SUCCESS) {
+          LOG.info("Successfully stopped service {}", appName);
+        }
         if (destroy) {
           result = sc.actionDestroy(appName);
           LOG.info("Successfully deleted service {}", appName);
-        } else {
-          LOG.info("Successfully stopped service {}", appName);
         }
         sc.close();
         return result;
@@ -264,8 +265,13 @@ public class ApiServer {
       serviceStatus.setDiagnostics("Successfully destroyed service " +
           appName);
     } else {
-      serviceStatus.setDiagnostics("Successfully stopped service " +
-          appName);
+      if (result == EXIT_COMMAND_ARGUMENT_ERROR) {
+        serviceStatus
+            .setDiagnostics("Service " + appName + " is already stopped");
+        return formatResponse(Status.BAD_REQUEST, serviceStatus);
+      } else {
+        serviceStatus.setDiagnostics("Successfully stopped service " + appName);
+      }
     }
     return formatResponse(Status.OK, serviceStatus);
   }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services-api/src/test/java/org/apache/hadoop/yarn/service/ServiceClientTest.java

@@ -88,6 +88,8 @@ public class ServiceClientTest extends ServiceClient {
     }
     if (serviceName.equals("jenkins")) {
       return EXIT_SUCCESS;
+    } else if (serviceName.equals("jenkins-second-stop")) {
+      return EXIT_COMMAND_ARGUMENT_ERROR;
     } else {
       throw new ApplicationNotFoundException("");
     }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services-api/src/test/java/org/apache/hadoop/yarn/service/TestApiServer.java

@@ -69,15 +69,14 @@ public class TestApiServer {
         this.apiServer.getClass().isAnnotationPresent(Path.class));
     final Path path = this.apiServer.getClass()
         .getAnnotation(Path.class);
-    assertEquals("The path has /v1 annotation", path.value(),
-        "/v1");
+    assertEquals("The path has /v1 annotation", "/v1", path.value());
   }
 
   @Test
   public void testGetVersion() {
     final Response actual = apiServer.getVersion();
-    assertEquals("Version number is", actual.getStatus(),
-        Response.ok().build().getStatus());
+    assertEquals("Version number is", Response.ok().build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
@@ -85,8 +84,9 @@ public class TestApiServer {
     Service service = new Service();
     // Test for invalid argument
     final Response actual = apiServer.createService(request, service);
-    assertEquals("Create service is ", actual.getStatus(),
-        Response.status(Status.BAD_REQUEST).build().getStatus());
+    assertEquals("Create service is ",
+        Response.status(Status.BAD_REQUEST).build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
@@ -109,52 +109,55 @@ public class TestApiServer {
     components.add(c);
     service.setComponents(components);
     final Response actual = apiServer.createService(request, service);
-    assertEquals("Create service is ", actual.getStatus(),
-        Response.status(Status.ACCEPTED).build().getStatus());
+    assertEquals("Create service is ",
+        Response.status(Status.ACCEPTED).build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
   public void testBadGetService() {
     final Response actual = apiServer.getService(request, "no-jenkins");
-    assertEquals("Get service is ", actual.getStatus(),
-        Response.status(Status.NOT_FOUND).build().getStatus());
+    assertEquals("Get service is ",
+        Response.status(Status.NOT_FOUND).build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
   public void testBadGetService2() {
     final Response actual = apiServer.getService(request, null);
-    assertEquals("Get service is ", actual.getStatus(),
-        Response.status(Status.NOT_FOUND)
-            .build().getStatus());
+    assertEquals("Get service is ",
+        Response.status(Status.NOT_FOUND).build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
   public void testGoodGetService() {
     final Response actual = apiServer.getService(request, "jenkins");
-    assertEquals("Get service is ", actual.getStatus(),
-        Response.status(Status.OK).build().getStatus());
+    assertEquals("Get service is ",
+        Response.status(Status.OK).build().getStatus(), actual.getStatus());
   }
 
   @Test
   public void testBadDeleteService() {
     final Response actual = apiServer.deleteService(request, "no-jenkins");
-    assertEquals("Delete service is ", actual.getStatus(),
-        Response.status(Status.BAD_REQUEST).build().getStatus());
+    assertEquals("Delete service is ",
+        Response.status(Status.BAD_REQUEST).build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
   public void testBadDeleteService2() {
     final Response actual = apiServer.deleteService(request, null);
-    assertEquals("Delete service is ", actual.getStatus(),
-        Response.status(Status.BAD_REQUEST)
-            .build().getStatus());
+    assertEquals("Delete service is ",
+        Response.status(Status.BAD_REQUEST).build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
   public void testGoodDeleteService() {
     final Response actual = apiServer.deleteService(request, "jenkins");
-    assertEquals("Delete service is ", actual.getStatus(),
-        Response.status(Status.OK).build().getStatus());
+    assertEquals("Delete service is ",
+        Response.status(Status.OK).build().getStatus(), actual.getStatus());
   }
 
   @Test
@@ -179,8 +182,8 @@ public class TestApiServer {
     service.setComponents(components);
     final Response actual = apiServer.updateService(request, "jenkins",
         service);
-    assertEquals("update service is ", actual.getStatus(),
-        Response.status(Status.OK).build().getStatus());
+    assertEquals("update service is ",
+        Response.status(Status.OK).build().getStatus(), actual.getStatus());
   }
 
   @Test
@@ -206,8 +209,9 @@ public class TestApiServer {
     System.out.println("before stop");
     final Response actual = apiServer.updateService(request, "no-jenkins",
         service);
-    assertEquals("flex service is ", actual.getStatus(),
-        Response.status(Status.BAD_REQUEST).build().getStatus());
+    assertEquals("flex service is ",
+        Response.status(Status.BAD_REQUEST).build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
@@ -232,8 +236,8 @@ public class TestApiServer {
     service.setComponents(components);
     final Response actual = apiServer.updateService(request, "jenkins",
         service);
-    assertEquals("flex service is ", actual.getStatus(),
-        Response.status(Status.OK).build().getStatus());
+    assertEquals("flex service is ",
+        Response.status(Status.OK).build().getStatus(), actual.getStatus());
   }
 
   @Test
@@ -258,9 +262,9 @@ public class TestApiServer {
     service.setComponents(components);
     final Response actual = apiServer.updateService(request, "no-jenkins",
         service);
-    assertEquals("start service is ", actual.getStatus(),
-        Response.status(Status.BAD_REQUEST).build()
-            .getStatus());
+    assertEquals("start service is ",
+        Response.status(Status.BAD_REQUEST).build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
@@ -285,8 +289,8 @@ public class TestApiServer {
     service.setComponents(components);
     final Response actual = apiServer.updateService(request, "jenkins",
         service);
-    assertEquals("start service is ", actual.getStatus(),
-        Response.status(Status.OK).build().getStatus());
+    assertEquals("start service is ",
+        Response.status(Status.OK).build().getStatus(), actual.getStatus());
   }
 
   @Test
@@ -312,35 +316,39 @@ public class TestApiServer {
     System.out.println("before stop");
     final Response actual = apiServer.updateService(request, "no-jenkins",
         service);
-    assertEquals("stop service is ", actual.getStatus(),
-        Response.status(Status.BAD_REQUEST).build().getStatus());
+    assertEquals("stop service is ",
+        Response.status(Status.BAD_REQUEST).build().getStatus(),
+        actual.getStatus());
   }
 
   @Test
   public void testGoodStopServices() {
     Service service = new Service();
-    service.setState(ServiceState.STARTED);
+    service.setState(ServiceState.STOPPED);
     service.setName("jenkins");
-    Artifact artifact = new Artifact();
-    artifact.setType(TypeEnum.DOCKER);
-    artifact.setId("jenkins:latest");
-    Resource resource = new Resource();
-    resource.setCpus(1);
-    resource.setMemory("2048");
-    List<Component> components = new ArrayList<Component>();
-    Component c = new Component();
-    c.setName("jenkins");
-    c.setNumberOfContainers(-1L);
-    c.setArtifact(artifact);
-    c.setLaunchCommand("");
-    c.setResource(resource);
-    components.add(c);
-    service.setComponents(components);
     System.out.println("before stop");
     final Response actual = apiServer.updateService(request, "jenkins",
         service);
-    assertEquals("stop service is ", actual.getStatus(),
-        Response.status(Status.OK).build().getStatus());
+    assertEquals("stop service is ",
+        Response.status(Status.OK).build().getStatus(), actual.getStatus());
+  }
+
+  @Test
+  public void testBadSecondStopServices() throws Exception {
+    Service service = new Service();
+    service.setState(ServiceState.STOPPED);
+    service.setName("jenkins-second-stop");
+    // simulates stop on an already stopped service
+    System.out.println("before second stop");
+    final Response actual = apiServer.updateService(request,
+        "jenkins-second-stop", service);
+    assertEquals("stop service should have thrown 400 Bad Request: ",
+        Response.status(Status.BAD_REQUEST).build().getStatus(),
+        actual.getStatus());
+    ServiceStatus serviceStatus = (ServiceStatus) actual.getEntity();
+    assertEquals("Stop service should have failed with service already stopped",
+        "Service jenkins-second-stop is already stopped",
+        serviceStatus.getDiagnostics());
   }
 
   @Test
@@ -366,9 +374,9 @@ public class TestApiServer {
     System.out.println("before stop");
     final Response actual = apiServer.updateService(request, "no-jenkins",
         service);
-    assertEquals("update service is ", actual.getStatus(),
+    assertEquals("update service is ",
         Response.status(Status.BAD_REQUEST)
-            .build().getStatus());
+            .build().getStatus(), actual.getStatus());
   }
 
   @Test

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/main/java/org/apache/hadoop/yarn/service/client/ServiceClient.java

@@ -363,7 +363,7 @@ public class ServiceClient extends AppAdminClient implements SliderExitCodes,
     if (terminatedStates.contains(report.getYarnApplicationState())) {
       LOG.info("Service {} is already in a terminated state {}", serviceName,
           report.getYarnApplicationState());
-      return EXIT_SUCCESS;
+      return EXIT_COMMAND_ARGUMENT_ERROR;
     }
     if (preRunningStates.contains(report.getYarnApplicationState())) {
       String msg = serviceName + " is at " + report.getYarnApplicationState()
@@ -779,7 +779,7 @@ public class ServiceClient extends AppAdminClient implements SliderExitCodes,
     Service service = ServiceApiUtil.loadService(fs, serviceName);
     ServiceApiUtil.validateAndResolveService(service, fs, getConfig());
     // see if it is actually running and bail out;
-    verifyNoLiveAppInRM(serviceName, "thaw");
+    verifyNoLiveAppInRM(serviceName, "start");
     ApplicationId appId = submitApp(service);
     service.setId(appId.toString());
     // write app definition on to hdfs