AMBARI-14965: Ambari server lists service even though service creation fails (Ajit Kumar via jluniya)

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

@@ -64,6 +64,8 @@ import org.apache.ambari.server.state.ServiceFactory;
 import org.apache.ambari.server.state.StackId;
 import org.apache.ambari.server.state.State;
 import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang.Validate;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -148,7 +150,7 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider
              ResourceAlreadyExistsException,
              NoSuchParentResourceException {
 
-    final Set<ServiceRequest> requests = new HashSet<ServiceRequest>();
+    final Set<ServiceRequest> requests = new HashSet<>();
     for (Map<String, Object> propertyMap : request.getProperties()) {
       requests.add(getRequest(propertyMap));
     }
@@ -338,115 +340,11 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider
       LOG.warn("Received an empty requests set");
       return;
     }
-
-    Clusters       clusters       = getManagementController().getClusters();
-    AmbariMetaInfo ambariMetaInfo = getManagementController().getAmbariMetaInfo();
-
+    Clusters clusters = getManagementController().getClusters();
     // do all validation checks
-    Map<String, Set<String>> serviceNames = new HashMap<String, Set<String>>();
-    Set<String> duplicates = new HashSet<String>();
-    for (ServiceRequest request : requests) {
-      if (request.getClusterName() == null
-          || request.getClusterName().isEmpty()
-          || request.getServiceName() == null
-          || request.getServiceName().isEmpty()) {
-        throw new IllegalArgumentException("Cluster name and service name"
-            + " should be provided when creating a service");
-      }
-
-      if (LOG.isDebugEnabled()) {
-        LOG.debug("Received a createService request"
-            + ", clusterName=" + request.getClusterName()
-            + ", serviceName=" + request.getServiceName()
-            + ", request=" + request);
-      }
-
-      if(!AuthorizationHelper.isAuthorized(ResourceType.CLUSTER, getClusterResourceId(request.getClusterName()), RoleAuthorization.SERVICE_ADD_DELETE_SERVICES)) {
-        throw new AuthorizationException("The user is not authorized to create services");
-      }
-
-      if (!serviceNames.containsKey(request.getClusterName())) {
-        serviceNames.put(request.getClusterName(), new HashSet<String>());
-      }
-      if (serviceNames.get(request.getClusterName())
-          .contains(request.getServiceName())) {
-        // throw error later for dup
-        duplicates.add(request.getServiceName());
-        continue;
-      }
-      serviceNames.get(request.getClusterName()).add(request.getServiceName());
-
-      if (request.getDesiredState() != null
-          && !request.getDesiredState().isEmpty()) {
-        State state = State.valueOf(request.getDesiredState());
-        if (!state.isValidDesiredState()
-            || state != State.INIT) {
-          throw new IllegalArgumentException("Invalid desired state"
-              + " only INIT state allowed during creation"
-              + ", providedDesiredState=" + request.getDesiredState());
-        }
-      }
-
-      Cluster cluster;
-      try {
-        cluster = clusters.getCluster(request.getClusterName());
-      } catch (ClusterNotFoundException e) {
-        throw new ParentObjectNotFoundException("Attempted to add a service to a cluster which doesn't exist", e);
-      }
-      try {
-        Service s = cluster.getService(request.getServiceName());
-        if (s != null) {
-          // throw error later for dup
-          duplicates.add(request.getServiceName());
-          continue;
-        }
-      } catch (ServiceNotFoundException e) {
-        // Expected
-      }
-
-      StackId stackId = cluster.getDesiredStackVersion();
-      if (!ambariMetaInfo.isValidService(stackId.getStackName(),
-          stackId.getStackVersion(), request.getServiceName())) {
-        throw new IllegalArgumentException("Unsupported or invalid service"
-            + " in stack"
-            + ", clusterName=" + request.getClusterName()
-            + ", serviceName=" + request.getServiceName()
-            + ", stackInfo=" + stackId.getStackId());
-      }
-    }
-
-    // ensure only a single cluster update
-    if (serviceNames.size() != 1) {
-      throw new IllegalArgumentException("Invalid arguments, updates allowed"
-          + "on only one cluster at a time");
-    }
-
-    // Validate dups
-    if (!duplicates.isEmpty()) {
-      StringBuilder svcNames = new StringBuilder();
-      boolean first = true;
-      for (String svcName : duplicates) {
-        if (!first) {
-          svcNames.append(",");
-        }
-        first = false;
-        svcNames.append(svcName);
-      }
-      String clusterName = requests.iterator().next().getClusterName();
-      String msg;
-      if (duplicates.size() == 1) {
-        msg = "Attempted to create a service which already exists: "
-            + ", clusterName=" + clusterName  + " serviceName=" + svcNames.toString();
-      } else {
-        msg = "Attempted to create services which already exist: "
-            + ", clusterName=" + clusterName  + " serviceNames=" + svcNames.toString();
-      }
-      throw new DuplicateResourceException(msg);
-    }
+    validateCreateRequests(requests, clusters);
 
     ServiceFactory serviceFactory = getManagementController().getServiceFactory();
-
-    // now to the real work
     for (ServiceRequest request : requests) {
       Cluster cluster = clusters.getCluster(request.getClusterName());
 
@@ -457,11 +355,10 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider
 
       s.setDesiredState(state);
       s.setDesiredStackVersion(cluster.getDesiredStackVersion());
+      s.persist();
       cluster.addService(s);
       // Initialize service widgets
       getManagementController().initializeWidgetsAndLayouts(cluster, s);
-
-      s.persist();
     }
   }
 
@@ -974,4 +871,86 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider
 
     return serviceSpecificProperties;
   }
+
+  private void validateCreateRequests(Set<ServiceRequest> requests, Clusters clusters)
+          throws AuthorizationException, AmbariException {
+
+    AmbariMetaInfo ambariMetaInfo = getManagementController().getAmbariMetaInfo();
+    Map<String, Set<String>> serviceNames = new HashMap<>();
+    Set<String> duplicates = new HashSet<>();
+    for (ServiceRequest request : requests) {
+      final String clusterName = request.getClusterName();
+      final String serviceName = request.getServiceName();
+      Validate.notEmpty(clusterName, "Cluster name should be provided when creating a service");
+      Validate.notEmpty(serviceName, "Service name should be provided when creating a service");
+
+      if (LOG.isDebugEnabled()) {
+        LOG.debug("Received a createService request"
+                + ", clusterName=" + clusterName + ", serviceName=" + serviceName + ", request=" + request);
+      }
+
+      if(!AuthorizationHelper.isAuthorized(ResourceType.CLUSTER, getClusterResourceId(clusterName), RoleAuthorization.SERVICE_ADD_DELETE_SERVICES)) {
+        throw new AuthorizationException("The user is not authorized to create services");
+      }
+
+      if (!serviceNames.containsKey(clusterName)) {
+        serviceNames.put(clusterName, new HashSet<String>());
+      }
+
+      if (serviceNames.get(clusterName).contains(serviceName)) {
+        // throw error later for dup
+        duplicates.add(serviceName);
+        continue;
+      }
+      serviceNames.get(clusterName).add(serviceName);
+
+      if (StringUtils.isNotEmpty(request.getDesiredState())) {
+        State state = State.valueOf(request.getDesiredState());
+        if (!state.isValidDesiredState() || state != State.INIT) {
+          throw new IllegalArgumentException("Invalid desired state"
+                  + " only INIT state allowed during creation"
+                  + ", providedDesiredState=" + request.getDesiredState());
+        }
+      }
+
+      Cluster cluster;
+      try {
+        cluster = clusters.getCluster(clusterName);
+      } catch (ClusterNotFoundException e) {
+        throw new ParentObjectNotFoundException("Attempted to add a service to a cluster which doesn't exist", e);
+      }
+      try {
+        Service s = cluster.getService(serviceName);
+        if (s != null) {
+          // throw error later for dup
+          duplicates.add(serviceName);
+          continue;
+        }
+      } catch (ServiceNotFoundException e) {
+        // Expected
+      }
+
+      StackId stackId = cluster.getDesiredStackVersion();
+      if (!ambariMetaInfo.isValidService(stackId.getStackName(),
+              stackId.getStackVersion(), request.getServiceName())) {
+        throw new IllegalArgumentException("Unsupported or invalid service in stack, clusterName=" + clusterName
+                + ", serviceName=" + serviceName + ", stackInfo=" + stackId.getStackId());
+      }
+    }
+    // ensure only a single cluster update
+    if (serviceNames.size() != 1) {
+      throw new IllegalArgumentException("Invalid arguments, updates allowed"
+              + "on only one cluster at a time");
+    }
+
+    // Validate dups
+    if (!duplicates.isEmpty()) {
+      String clusterName = requests.iterator().next().getClusterName();
+      String msg = "Attempted to create a service which already exists: "
+              + ", clusterName=" + clusterName  + " serviceName=" + StringUtils.join(duplicates, ",");
+
+      throw new DuplicateResourceException(msg);
+    }
+
+  }
 }