[ https://issues.apache.org/jira/browse/BROOKLYN-259?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15276418#comment-15276418 ]
ASF GitHub Bot commented on BROOKLYN-259: ----------------------------------------- Github user sjcorbett commented on a diff in the pull request: https://github.com/apache/brooklyn-server/pull/132#discussion_r62505553 --- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsByonLocationResolver.java --- @@ -61,129 +56,140 @@ * @author aled */ @SuppressWarnings({"unchecked","rawtypes"}) -public class JcloudsByonLocationResolver implements LocationResolver { +public class JcloudsByonLocationResolver extends AbstractLocationResolver implements LocationResolver { public static final Logger log = LoggerFactory.getLogger(JcloudsByonLocationResolver.class); public static final String BYON = "jcloudsByon"; - private static final Pattern PATTERN = Pattern.compile("("+BYON+"|"+BYON.toUpperCase()+")" + ":" + "\\((.*)\\)$"); - - private ManagementContext managementContext; - @Override - public void init(ManagementContext managementContext) { - this.managementContext = checkNotNull(managementContext, "managementContext"); + public boolean isEnabled() { + return LocationConfigUtils.isResolverPrefixEnabled(managementContext, getPrefix()); } @Override - public boolean isEnabled() { - return LocationConfigUtils.isResolverPrefixEnabled(managementContext, getPrefix()); + public String getPrefix() { + return BYON; } - // TODO Remove some duplication from JcloudsResolver; needs more careful review @Override - public LocationSpec<? extends Location> newLocationSpecFromString(String spec, Map<?, ?> locationFlags, LocationRegistry registry) { - Map globalProperties = registry.getProperties(); + protected Class<? extends Location> getLocationType() { + return FixedListMachineProvisioningLocation.class; + } - Matcher matcher = PATTERN.matcher(spec); - if (!matcher.matches()) { - throw new IllegalArgumentException("Invalid location '"+spec+"'; must specify something like jcloudsByon(provider=\"aws-ec2\",region=\"us-east-1\",hosts=\"i-f2014593,i-d1234567\")"); - } - - String argsPart = matcher.group(2); - Map<String, String> argsMap = KeyValueParser.parseMap(argsPart); - - // prefer args map over location flags - - String namedLocation = (String) locationFlags.get(LocationInternal.NAMED_SPEC_NAME.getName()); + @Override + protected SpecParser getSpecParser() { + return new AbstractLocationResolver.SpecParser(getPrefix()).setExampleUsage("\"jcloudsByon(provider='aws-ec2',region='us-east-1',hosts='i-12345678,i-90123456')\""); + } + + @Override + protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, final LocationRegistry registry) { + ConfigBag config = super.extractConfig(locationFlags, spec, registry); - String providerOrApi = argsMap.containsKey("provider") ? argsMap.get("provider") : (String)locationFlags.get("provider"); + String providerOrApi = (String) config.getStringKey("provider"); + String regionName = (String) config.getStringKey("region"); + String endpoint = (String) config.getStringKey("endpoint"); + String namedLocation = (String) config.get(LocationInternal.NAMED_SPEC_NAME); + config.remove(LocationInternal.NAMED_SPEC_NAME.getName()); - String regionName = argsMap.containsKey("region") ? argsMap.get("region") : (String)locationFlags.get("region"); - - String endpoint = argsMap.containsKey("endpoint") ? argsMap.get("endpoint") : (String)locationFlags.get("endpoint"); - - String name = argsMap.containsKey("name") ? argsMap.get("name") : (String)locationFlags.get("name"); + Object hosts = config.getStringKey("hosts"); + config.remove("hosts"); - String user = argsMap.containsKey("user") ? argsMap.get("user") : (String)locationFlags.get("user"); + Map jcloudsProperties = new JcloudsPropertiesFromBrooklynProperties().getJcloudsProperties(providerOrApi, regionName, namedLocation, config.getAllConfig()); + config.putIfAbsent(jcloudsProperties); - String privateKeyFile = argsMap.containsKey("privateKeyFile") ? argsMap.get("privateKeyFile") : (String)locationFlags.get("privateKeyFile"); - - String hosts = argsMap.get("hosts"); - if (Strings.isEmpty(providerOrApi)) { throw new IllegalArgumentException("Invalid location '"+spec+"'; provider must be defined"); } - if (hosts == null || hosts.isEmpty()) { + if (hosts == null || (hosts instanceof String && Strings.isBlank((String)hosts))) { throw new IllegalArgumentException("Invalid location '"+spec+"'; at least one host must be defined"); } - if (argsMap.containsKey("name") && (Strings.isEmpty(name))) { - throw new IllegalArgumentException("Invalid location '"+spec+"'; if name supplied then value must be non-empty"); + + final String jcloudsSpec = "jclouds:"+providerOrApi + (regionName != null ? ":"+regionName : "") + (endpoint != null ? ":"+endpoint : ""); + final Maybe<LocationSpec<? extends Location>> jcloudsLocationSpec = registry.getLocationSpec(jcloudsSpec, config.getAllConfig()); + if (jcloudsLocationSpec.isAbsent()) { + throw new IllegalArgumentException("Invalid location '"+spec+"'; referrenced jclouds spec '"+jcloudsSpec+"' cannot be resolved"); + } else if (!JcloudsLocation.class.isAssignableFrom(jcloudsLocationSpec.get().getType())) { + throw new IllegalArgumentException("Invalid location '"+spec+"'; referrenced spec '"+jcloudsSpec+"' of type "+jcloudsLocationSpec.get().getType()+" rather than JcloudsLocation"); } - // For everything in brooklyn.properties, only use things with correct prefix (and remove that prefix). - // But for everything passed in via locationFlags, pass those as-is. - // TODO Should revisit the locationFlags: where are these actually used? Reason accepting properties without - // full prefix is that the map's context is explicitly this location, rather than being generic properties. - Map allProperties = getAllProperties(registry, globalProperties); - Map jcloudsProperties = new JcloudsPropertiesFromBrooklynProperties().getJcloudsProperties(providerOrApi, regionName, namedLocation, allProperties); - jcloudsProperties.putAll(locationFlags); - jcloudsProperties.putAll(argsMap); + List<?> hostIds; - String jcloudsSpec = "jclouds:"+providerOrApi + (regionName != null ? ":"+regionName : "") + (endpoint != null ? ":"+endpoint : ""); - JcloudsLocation jcloudsLocation = (JcloudsLocation) registry.resolve(jcloudsSpec, jcloudsProperties); + if (hosts instanceof String) { + if (((String) hosts).isEmpty()) { + hostIds = ImmutableList.of(); + } else { + hostIds = WildcardGlobs.getGlobsAfterBraceExpansion("{"+hosts+"}", + true /* numeric */, /* no quote support though */ PhraseTreatment.NOT_A_SPECIAL_CHAR, PhraseTreatment.NOT_A_SPECIAL_CHAR); + } + } else if (hosts instanceof Iterable) { + hostIds = ImmutableList.copyOf((Iterable<?>)hosts); + } else { + throw new IllegalArgumentException("Invalid location '"+spec+"'; at least one host must be defined"); + } + if (hostIds.isEmpty()) { + throw new IllegalArgumentException("Invalid location '"+spec+"'; at least one host must be defined"); + } - List<String> hostIdentifiers = WildcardGlobs.getGlobsAfterBraceExpansion("{"+hosts+"}", - true /* numeric */, /* no quote support though */ PhraseTreatment.NOT_A_SPECIAL_CHAR, PhraseTreatment.NOT_A_SPECIAL_CHAR); - List<JcloudsSshMachineLocation> machines = Lists.newArrayList(); - - for (String hostIdentifier : hostIdentifiers) { - Map<?, ?> machineFlags = MutableMap.builder() - .put("id", hostIdentifier) - .putIfNotNull("user", user) - .putIfNotNull("privateKeyFile", privateKeyFile) - .build(); - try { - // TODO management of these machines may be odd, as it is passed in as a key as config to a spec - JcloudsSshMachineLocation machine = jcloudsLocation.rebindMachine(jcloudsLocation.config().getBag().putAll(machineFlags)); - machines.add(machine); - } catch (NoMachinesAvailableException e) { - log.warn("Error rebinding to jclouds machine "+hostIdentifier+" in "+jcloudsLocation, e); - Exceptions.propagate(e); + final List<Map<?,?>> machinesFlags = Lists.newArrayList(); + for (Object hostId : hostIds) { + Map<?, ?> machineFlags; + if (hostId instanceof String) { + machineFlags = parseMachineFlags((String)hostId, config); + } else if (hostId instanceof Map) { + machineFlags = parseMachineFlags((Map<String,?>)hostId, config); + } else { + throw new IllegalArgumentException("Invalid host type '"+(hostId == null ? null : hostId.getClass().getName())+", referrenced in spec '"+spec); } + machinesFlags.add(machineFlags); } + Supplier<List<JcloudsMachineLocation>> machinesFactory = new Supplier<List<JcloudsMachineLocation>>() { + @Override + public List<JcloudsMachineLocation> get() { + // FIXME Can't change the parent of the JcloudsMachineLocation; but the FixedListMachineLocation will expect it to be the parent? --- End diff -- Is this tracked in Jira? > jcloudsByon location spec leaks location instances > -------------------------------------------------- > > Key: BROOKLYN-259 > URL: https://issues.apache.org/jira/browse/BROOKLYN-259 > Project: Brooklyn > Issue Type: Bug > Affects Versions: 0.9.0 > Reporter: Aled Sage > Fix For: 0.10.0 > > > When declaring in brooklyn.properties a location spec such as > {noformat} > jcloudsByon(provider="aws-ec2",region="us-east-1",user="brooklyn",password="pa55w0rd",hosts="i-12345678") > {noformat} > It caused new locations to be created and persisted every few seconds, but > for those locations to be never deleted. > --- > This was caused by changes made in 0.9.0 to {{LocationResolver}} > implementations, so that it creates a {{LocationSpec}} rather than > instantiating a location directly. Unfortunately, > {{JcloudsByonLocationResolver}} had not been updated. The REST api would poll > this regularly to find out about the types of location, and on every poll it > would create new locations. > This is similar to the problem encountered for localhost, fixed in time for > 0.9.0 in https://github.com/apache/brooklyn-server/pull/97. -- This message was sent by Atlassian JIRA (v6.3.4#6332)