This is an automated email from the ASF dual-hosted git repository. jgolieb pushed a commit to branch branch-feature-AMBARI-14714-mpack-advisor in repository https://gitbox.apache.org/repos/asf/ambari.git
commit e2d13e9db5f7ad80d787e7114dbf316155e79800 Author: Doroszlai, Attila <[email protected]> AuthorDate: Tue May 29 17:06:58 2018 +0200 AMBARI-23746. Cannot create same named service in different service groups in same request --- .../internal/ServiceResourceProvider.java | 38 +++---- .../internal/ServiceResourceProviderTest.java | 112 ++++++++++++++++++++- 2 files changed, 129 insertions(+), 21 deletions(-) diff --git a/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java b/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java index ebaf14c..98eff23 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ServiceResourceProvider.java @@ -76,6 +76,7 @@ import org.apache.ambari.server.state.State; import org.apache.ambari.server.topology.TopologyDeleteFormer; import org.apache.commons.lang.StringUtils; import org.apache.commons.lang.Validate; +import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -178,8 +179,10 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider */ @AssistedInject public ServiceResourceProvider( - @Assisted AmbariManagementController managementController, - MaintenanceStateHelper maintenanceStateHelper, TopologyDeleteFormer topologyDeleteFormer) { + @Assisted AmbariManagementController managementController, + MaintenanceStateHelper maintenanceStateHelper, + TopologyDeleteFormer topologyDeleteFormer + ) { super(Resource.Type.Service, PROPERTY_IDS, KEY_PROPERTY_IDS, managementController); this.maintenanceStateHelper = maintenanceStateHelper; this.topologyDeleteFormer = topologyDeleteFormer; @@ -565,7 +568,7 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider if(request.getServiceGroupName() != null){ clusterServices = cluster.getServicesByServiceGroup(serviceGroupName); }else{ - clusterServices = cluster.getServicesById().values(); + clusterServices = cluster.getServices().values(); } for (Service s : clusterServices) { if (checkDesiredState @@ -1067,8 +1070,8 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider throws AuthorizationException, AmbariException { AmbariMetaInfo ambariMetaInfo = getManagementController().getAmbariMetaInfo(); - Map<String, Set<String>> serviceNames = new HashMap<>(); - Set<String> duplicates = new HashSet<>(); + Map<String, Set<Pair<String, String>>> serviceNames = new HashMap<>(); + Set<Pair<String, String>> duplicates = new HashSet<>(); for (ServiceRequest request : requests) { final String clusterName = request.getClusterName(); @@ -1079,24 +1082,21 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider Validate.notNull(serviceGroupName, "Service group 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, clusterId={}, serviceName={}, request={}", clusterName, serviceName, request); - } + LOG.debug("Received a createService request, clusterId={}, serviceGroupName={}, serviceName={}, request={}", + clusterName, serviceGroupName, serviceName, 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>()); - } + Pair<String, String> serviceID = Pair.of(serviceGroupName, serviceName); + Set<Pair<String, String>> services = serviceNames.computeIfAbsent(clusterName, __ -> new HashSet<>()); - if (serviceNames.get(clusterName).contains(serviceName)) { + if (!services.add(serviceID)) { // throw error later for dup - duplicates.add(serviceName); + duplicates.add(serviceID); continue; } - serviceNames.get(clusterName).add(serviceName); if (StringUtils.isNotEmpty(request.getDesiredState())) { State state = State.valueOf(request.getDesiredState()); @@ -1117,7 +1117,7 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider Service s = cluster.getService(serviceName); if (s != null && (s.getServiceGroupName().equals(serviceGroupName))) { // throw error later for dup - duplicates.add(serviceName); + duplicates.add(serviceID); continue; } } catch (ServiceNotFoundException e) { @@ -1139,10 +1139,10 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider } // validate the credential store input provided - ServiceInfo serviceInfo = ambariMetaInfo.getService(stackId.getStackName(), + if (StringUtils.isNotEmpty(request.getCredentialStoreEnabled())) { + ServiceInfo serviceInfo = ambariMetaInfo.getService(stackId.getStackName(), stackId.getStackVersion(), stackServiceName); - if (StringUtils.isNotEmpty(request.getCredentialStoreEnabled())) { boolean credentialStoreEnabled = Boolean.parseBoolean(request.getCredentialStoreEnabled()); if (!serviceInfo.isCredentialStoreSupported() && credentialStoreEnabled) { throw new IllegalArgumentException("Invalid arguments, cannot enable credential store " + @@ -1159,8 +1159,8 @@ public class ServiceResourceProvider extends AbstractControllerResourceProvider // 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, ","); + String msg = "Attempted to create services which already exist: " + + ", clusterName=" + clusterName + " " + StringUtils.join(duplicates, ", "); throw new DuplicateResourceException(msg); } diff --git a/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java b/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java index de2152d..e82ab51 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java +++ b/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ServiceResourceProviderTest.java @@ -18,8 +18,10 @@ package org.apache.ambari.server.controller.internal; +import static java.util.Arrays.asList; import static org.easymock.EasyMock.anyBoolean; import static org.easymock.EasyMock.anyObject; +import static org.easymock.EasyMock.anyString; import static org.easymock.EasyMock.capture; import static org.easymock.EasyMock.createMock; import static org.easymock.EasyMock.createNiceMock; @@ -55,6 +57,7 @@ import org.apache.ambari.server.controller.ServiceResponse; import org.apache.ambari.server.controller.spi.Predicate; import org.apache.ambari.server.controller.spi.Request; import org.apache.ambari.server.controller.spi.Resource; +import org.apache.ambari.server.controller.spi.ResourceAlreadyExistsException; import org.apache.ambari.server.controller.spi.ResourceProvider; import org.apache.ambari.server.controller.spi.SystemException; import org.apache.ambari.server.controller.utilities.PredicateBuilder; @@ -86,6 +89,7 @@ import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; /** * ServiceResourceProvider tests. @@ -186,6 +190,111 @@ public class ServiceResourceProviderTest { } @Test + public void createServicesInDifferentServiceGroups() throws Exception { + String clusterName = "cl1"; + String serviceName = "HADOOP_CLIENTS"; + StackId hdpcoreStackId = new StackId("HDPCORE", "1.0.0-b123"); + StackId odsStackId = new StackId("ODS", "1.0.0-b111"); + + AmbariManagementController managementController = createNiceMock(AmbariManagementController.class); + Clusters clusters = createNiceMock(Clusters.class); + Cluster cluster = createNiceMock(Cluster.class); + AmbariMetaInfo ambariMetaInfo = createNiceMock(AmbariMetaInfo.class); + ServiceInfo serviceInfo = createNiceMock(ServiceInfo.class); + + expect(managementController.getClusters()).andReturn(clusters).anyTimes(); + expect(managementController.getAmbariMetaInfo()).andReturn(ambariMetaInfo).anyTimes(); + + expect(clusters.getCluster(clusterName)).andReturn(cluster).anyTimes(); + expect(cluster.getService(anyString())).andReturn(null); + expect(cluster.getClusterId()).andReturn(2L).anyTimes(); + + Set<Map<String, Object>> propertySet = new HashSet<>(); + long serviceGroupId = 1; + for (StackId stackId : asList(hdpcoreStackId, odsStackId)) { + ServiceGroup serviceGroup = createNiceMock(ServiceGroup.class); + Service service = createNiceMock(Service.class); + ServiceResponse response = createNiceMock(ServiceResponse.class); + + expect(cluster.addService(eq(serviceGroup), eq(serviceName), eq(serviceName))).andReturn(service).anyTimes(); + expect(cluster.getServiceGroup(stackId.getStackName())).andReturn(serviceGroup).anyTimes(); + expect(cluster.getServiceGroup(serviceGroupId)).andReturn(serviceGroup).anyTimes(); + expect(serviceGroup.getStackId()).andReturn(hdpcoreStackId).anyTimes(); + expect(service.getServiceGroupId()).andReturn(serviceGroupId).anyTimes(); + expect(service.convertToResponse()).andReturn(response).anyTimes(); + + replay(serviceGroup, service, response); + ++serviceGroupId; + + propertySet.add(ImmutableMap.of( + ServiceResourceProvider.SERVICE_CLUSTER_NAME_PROPERTY_ID, clusterName, + ServiceResourceProvider.SERVICE_SERVICE_NAME_PROPERTY_ID, serviceName, + ServiceResourceProvider.SERVICE_SERVICE_GROUP_NAME_PROPERTY_ID, stackId.getStackName() + )); + } + + expect(ambariMetaInfo.isValidService(anyString(), anyString(), anyString())).andReturn(true).anyTimes(); + expect(ambariMetaInfo.getService(anyString(), anyString(), anyString())).andReturn(serviceInfo).anyTimes(); + + // replay + replay(managementController, clusters, cluster, ambariMetaInfo, serviceInfo); + + SecurityContextHolder.getContext().setAuthentication(TestAuthenticationFactory.createAdministrator()); + + Request request = PropertyHelper.getCreateRequest(propertySet, null); + ResourceProvider provider = getServiceProvider(managementController); + provider.createResources(request); + } + + @Test(expected = ResourceAlreadyExistsException.class) + public void createDuplicateServices() throws Exception { + String clusterName = "cl1"; + String serviceName = "SOME_SERVICE"; + StackId stackId = new StackId("HDPCORE", "1.0.0-b123"); + + AmbariManagementController managementController = createNiceMock(AmbariManagementController.class); + Clusters clusters = createNiceMock(Clusters.class); + Cluster cluster = createNiceMock(Cluster.class); + AmbariMetaInfo ambariMetaInfo = createNiceMock(AmbariMetaInfo.class); + ServiceInfo serviceInfo = createNiceMock(ServiceInfo.class); + + expect(managementController.getClusters()).andReturn(clusters).anyTimes(); + expect(managementController.getAmbariMetaInfo()).andReturn(ambariMetaInfo).anyTimes(); + + expect(clusters.getCluster(clusterName)).andReturn(cluster).anyTimes(); + expect(cluster.getService(anyString())).andReturn(null); + expect(cluster.getClusterId()).andReturn(2L).anyTimes(); + + ServiceGroup serviceGroup = createNiceMock(ServiceGroup.class); + Service service = createNiceMock(Service.class); + + expect(cluster.getServiceGroup(stackId.getStackName())).andReturn(serviceGroup).anyTimes(); + expect(cluster.getServiceGroup(1L)).andReturn(serviceGroup).anyTimes(); + expect(serviceGroup.getStackId()).andReturn(stackId).anyTimes(); + expect(service.getServiceGroupId()).andReturn(1L).anyTimes(); + + Map<String, Object> serviceRequestProperties = ImmutableMap.of( + ServiceResourceProvider.SERVICE_CLUSTER_NAME_PROPERTY_ID, clusterName, + ServiceResourceProvider.SERVICE_SERVICE_NAME_PROPERTY_ID, serviceName, + ServiceResourceProvider.SERVICE_SERVICE_GROUP_NAME_PROPERTY_ID, stackId.getStackName() + ); + Map<String, Object> almostIdenticalCopy = ImmutableMap.<String, Object>builder().putAll(serviceRequestProperties).put(ServiceResourceProvider.SERVICE_CREDENTIAL_STORE_ENABLED_PROPERTY_ID, "false").build(); + Set<Map<String, Object>> propertySet = ImmutableSet.of(almostIdenticalCopy, serviceRequestProperties); + + expect(ambariMetaInfo.isValidService(anyString(), anyString(), anyString())).andReturn(true).anyTimes(); + expect(ambariMetaInfo.getService(anyString(), anyString(), anyString())).andReturn(serviceInfo).anyTimes(); + + // replay + replay(managementController, clusters, cluster, ambariMetaInfo, serviceInfo, serviceGroup, service); + + SecurityContextHolder.getContext().setAuthentication(TestAuthenticationFactory.createAdministrator()); + + Request request = PropertyHelper.getCreateRequest(propertySet, null); + ResourceProvider provider = getServiceProvider(managementController); + provider.createResources(request); + } + + @Test public void testGetResourcesAsAdministrator() throws Exception{ testGetResources(TestAuthenticationFactory.createAdministrator()); } @@ -891,7 +1000,7 @@ public class ServiceResourceProviderTest { expect(clusters.getCluster("Cluster100")).andReturn(cluster).anyTimes(); expect(cluster.getClusterId()).andReturn(2L).anyTimes(); expect(cluster.getService(serviceName)).andReturn(service).anyTimes(); - expect(cluster.getService(null, serviceName)).andReturn(service).anyTimes(); + expect(cluster.getService("SERVICE_GROUP", serviceName)).andReturn(service).anyTimes(); expect(service.getDesiredState()).andReturn(State.INSTALLED).anyTimes(); expect(service.getName()).andReturn(serviceName).anyTimes(); expect(service.getServiceComponents()).andReturn(new HashMap<>()); @@ -1152,7 +1261,6 @@ public class ServiceResourceProviderTest { @Test public void testCheckPropertyIds() throws Exception { AmbariManagementController managementController = createMock(AmbariManagementController.class); - AbstractResourceProvider provider = getServiceProvider(managementController); Set<String> unsupported = provider.checkPropertyIds( -- To stop receiving notification emails like this one, please contact [email protected].
