From 1e36c3e503f76c4d9d55949fd05842d9a677b4e8 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 17 Jul 2014 12:37:29 -0400 Subject: [PATCH 01/11] suppress warning of DTO with no contents, when it has a description which says where the content comes from --- .../internal/BasicBrooklynCatalog.java | 3 ++- .../brooklyn/catalog/internal/CatalogDto.java | 25 ++++++++++++++++--- .../catalog/internal/CatalogDtoUtils.java | 5 +--- .../internal/AbstractManagementContext.java | 4 +-- .../catalog/internal/CatalogDtoTest.java | 9 ++++--- 5 files changed, 32 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java index dfa966a61c..176975f370 100644 --- a/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java @@ -355,7 +355,8 @@ private synchronized void loadManualAdditionsCatalog() { if (manualAdditionsCatalog!=null) return; CatalogDto manualAdditionsCatalogDto = CatalogDto.newNamedInstance( "Manual Catalog Additions", "User-additions to the catalog while Brooklyn is running, " + - "created "+Time.makeDateString()); + "created "+Time.makeDateString(), + "manual-additions"); CatalogDo manualAdditionsCatalog = catalog.addCatalog(manualAdditionsCatalogDto); if (manualAdditionsCatalog==null) { // not hard to support, but slightly messy -- probably have to use ID's to retrieve the loaded instance diff --git a/core/src/main/java/brooklyn/catalog/internal/CatalogDto.java b/core/src/main/java/brooklyn/catalog/internal/CatalogDto.java index d84b2f9c3b..4a74ed9416 100644 --- a/core/src/main/java/brooklyn/catalog/internal/CatalogDto.java +++ b/core/src/main/java/brooklyn/catalog/internal/CatalogDto.java @@ -30,8 +30,10 @@ import brooklyn.util.exceptions.PropagatedRuntimeException; import brooklyn.util.stream.Streams; +import com.google.common.annotations.Beta; import com.google.common.base.Objects; +@Beta public class CatalogDto { private static final Logger LOG = LoggerFactory.getLogger(CatalogDto.class); @@ -53,7 +55,7 @@ public class CatalogDto { public static CatalogDto newDefaultLocalScanningDto(CatalogClasspathDo.CatalogScanningModes scanMode) { CatalogDo result = new CatalogDo( - CatalogDto.newNamedInstance("Local Scanned Catalog", "All annotated Brooklyn entities detected in the default classpath") ); + newNamedInstance("Local Scanned Catalog", "All annotated Brooklyn entities detected in the default classpath", "scanning-local-classpath") ); result.setClasspathScanForEntities(scanMode); return result.dto; } @@ -78,10 +80,24 @@ public static CatalogDto newDtoFromXmlContents(String xmlContents, String origin return result; } - public static CatalogDto newNamedInstance(String name, String description) { + /** + * Creates a DTO. + *

+ * The way contents is treated may change; thus this (and much of catalog) should be treated as beta. + * + * @param name + * @param description + * @param optionalContentsDescription optional description of contents; if null, we normally expect source 'contents' to be set later; + * if the DTO has no 'contents' (ie XML source) then a description should be supplied so we know who is populating it + * (e.g. manual additions); without this, warnings may be generated + * + * @return a new Catalog DTO + */ + public static CatalogDto newNamedInstance(String name, String description, String optionalContentsDescription) { CatalogDto result = new CatalogDto(); result.name = name; result.description = description; + if (optionalContentsDescription!=null) result.contentsDescription = optionalContentsDescription; return result; } @@ -97,8 +113,11 @@ void populate() { if (url != null) { contents = ResourceUtils.create().getResourceAsString(url); contentsDescription = url; + } else if (contentsDescription==null) { + LOG.warn("Catalog DTO has no contents and no description; ignoring call to populate it. Description should be set to suppress this warning."); + return; } else { - LOG.warn("Catalog DTO has no contents); ignoring call to populate it."); + LOG.debug("Nothing needs doing (no contents or URL) for catalog with contents described as "+contentsDescription+"."); return; } } diff --git a/core/src/main/java/brooklyn/catalog/internal/CatalogDtoUtils.java b/core/src/main/java/brooklyn/catalog/internal/CatalogDtoUtils.java index 237a4736c4..cb44cb0e56 100644 --- a/core/src/main/java/brooklyn/catalog/internal/CatalogDtoUtils.java +++ b/core/src/main/java/brooklyn/catalog/internal/CatalogDtoUtils.java @@ -33,10 +33,7 @@ public class CatalogDtoUtils { private static final Logger log = LoggerFactory.getLogger(CatalogDtoUtils.class); public static CatalogDto newDefaultLocalScanningDto(CatalogScanningModes scanMode) { - CatalogDo result = new CatalogDo( - CatalogDto.newNamedInstance("Local Scanned Catalog", "All annotated Brooklyn entities detected in the default classpath") ); - result.setClasspathScanForEntities(scanMode); - return result.dto; + return CatalogDto.newDefaultLocalScanningDto(scanMode); } /** throws if there are any problems in retrieving or copying */ diff --git a/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java b/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java index 2eb695ae16..46e751d1d3 100644 --- a/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java +++ b/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java @@ -336,7 +336,7 @@ protected synchronized void loadCatalog() { if (!Strings.isEmpty(catalogUrl)) { catalog = new BasicBrooklynCatalog(this, CatalogDto.newDtoFromUrl(catalogUrl)); if (log.isDebugEnabled()) - log.debug("Loaded catalog from "+catalogUrl+": "+catalog); + log.debug("Loading catalog from "+catalogUrl+": "+catalog); } } catch (Exception e) { if (Throwables.getRootCause(e) instanceof FileNotFoundException) { @@ -345,7 +345,7 @@ protected synchronized void loadCatalog() { log.warn("Could not find catalog XML specified at "+nonDefaultUrl+"; using default (local classpath) catalog. Error was: "+e); } else { if (log.isDebugEnabled()) - log.debug("No default catalog file available; trying again using local classpath to populate catalog. Error was: "+e); + log.debug("No default catalog file available at "+catalogUrl+"; trying again using local classpath to populate catalog. Error was: "+e); } } else { log.warn("Error importing catalog XML at "+catalogUrl+"; using default (local classpath) catalog. Error was: "+e, e); diff --git a/core/src/test/java/brooklyn/catalog/internal/CatalogDtoTest.java b/core/src/test/java/brooklyn/catalog/internal/CatalogDtoTest.java index e580182c73..4c07e5d144 100644 --- a/core/src/test/java/brooklyn/catalog/internal/CatalogDtoTest.java +++ b/core/src/test/java/brooklyn/catalog/internal/CatalogDtoTest.java @@ -83,10 +83,11 @@ protected void checkCatalogHealthy(CatalogDto root) { public static CatalogDto buildExampleCatalog() { CatalogDo root = new CatalogDo(CatalogDto.newNamedInstance("My Local Catalog", "My favourite local settings, including remote catalogs -- " + - "intended partly as a teaching example for what can be expressed, and how")); + "intended partly as a teaching example for what can be expressed, and how", + "contents-built-in-test")); root.setClasspathScanForEntities(CatalogScanningModes.NONE); - CatalogDo testEntitiesJavaCatalog = new CatalogDo(CatalogDto.newNamedInstance("Test Entities from Java", null)); + CatalogDo testEntitiesJavaCatalog = new CatalogDo(CatalogDto.newNamedInstance("Test Entities from Java", null, "test-java")); testEntitiesJavaCatalog.setClasspathScanForEntities(CatalogScanningModes.NONE); testEntitiesJavaCatalog.addToClasspath(MavenRetriever.localUrl(BrooklynMavenArtifacts.artifact("", "brooklyn-core", "jar", "tests"))); testEntitiesJavaCatalog.addEntry(CatalogItems.newTemplateFromJava( @@ -95,13 +96,13 @@ public static CatalogDto buildExampleCatalog() { TestEntity.class.getCanonicalName(), "Test Entity from JAR")); root.addCatalog(testEntitiesJavaCatalog.dto); - CatalogDo testEntitiesJavaCatalogScanning = new CatalogDo(CatalogDto.newNamedInstance("Test Entities from Java Scanning", null)); + CatalogDo testEntitiesJavaCatalogScanning = new CatalogDo(CatalogDto.newNamedInstance("Test Entities from Java Scanning", null, "test-java-scan")); testEntitiesJavaCatalogScanning.addToClasspath(MavenRetriever.localUrl(BrooklynMavenArtifacts.artifact("", "brooklyn-core", "jar", "tests"))); testEntitiesJavaCatalogScanning.setClasspathScanForEntities(CatalogScanningModes.ANNOTATIONS); root.addCatalog(testEntitiesJavaCatalogScanning.dto); CatalogDo osgiCatalog = new CatalogDo(CatalogDto.newNamedInstance("Test Entities from OSGi", - "A catalog whose entries define their libraries as a list of OSGi bundles")); + "A catalog whose entries define their libraries as a list of OSGi bundles", "test-osgi-defined")); osgiCatalog.setClasspathScanForEntities(CatalogScanningModes.NONE); CatalogEntityItemDto osgiEntity = CatalogItems.newEntityFromJava(TestEntity.class.getCanonicalName(), "Test Entity from OSGi"); // NB: this is not actually an OSGi bundle, but it's okay as we don't instantiate the bundles ahead of time (currently) From fa8cdb4c54b5a6f46a313366ae10ec31ce287c3b Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 17 Jul 2014 12:54:03 -0400 Subject: [PATCH 02/11] better warnings when icons are missing --- .../rest/resources/CatalogResource.java | 24 +++++++++++++++++-- .../brooklyn/rest/util/WebResourceUtils.java | 2 +- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/usage/rest-server/src/main/java/brooklyn/rest/resources/CatalogResource.java b/usage/rest-server/src/main/java/brooklyn/rest/resources/CatalogResource.java index 007c19c2fe..b553bf936c 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/resources/CatalogResource.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/resources/CatalogResource.java @@ -24,6 +24,7 @@ import java.net.URI; import java.util.ArrayList; import java.util.List; +import java.util.Set; import javax.annotation.Nullable; import javax.ws.rs.core.MediaType; @@ -49,6 +50,8 @@ import brooklyn.rest.transform.CatalogTransformer; import brooklyn.rest.util.WebResourceUtils; import brooklyn.util.ResourceUtils; +import brooklyn.util.collections.MutableSet; +import brooklyn.util.exceptions.Exceptions; import brooklyn.util.text.StringPredicates; import brooklyn.util.text.Strings; @@ -78,6 +81,8 @@ public Response createFromMultipart(InputStream uploadedInputStream, FormDataCon return create(CharStreams.toString(new InputStreamReader(uploadedInputStream, Charsets.UTF_8))); } + static Set missingIcons = MutableSet.of(); + @Override public Response create(String yaml) { CatalogItem item; @@ -202,8 +207,23 @@ public Response getIcon(String itemId) { log.debug("Loading and returning "+url+" as icon for "+result); MediaType mime = WebResourceUtils.getImageMediaTypeFromExtension(Files.getFileExtension(url)); - Object content = ResourceUtils.create(result.newClassLoadingContext(mgmt())).getResourceFromUrl(url); - return Response.ok(content, mime).build(); + try { + Object content = ResourceUtils.create(result.newClassLoadingContext(mgmt())).getResourceFromUrl(url); + return Response.ok(content, mime).build(); + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + synchronized (missingIcons) { + if (missingIcons.add(url)) { + // note: this can be quite common when running from an IDE, as resources may not be copied; + // a mvn build should sort it out (the IDE will then find the resources, until you clean or maybe refresh...) + log.warn("Missing icon data for "+itemId+", expected at: "+url+" (subsequent messages will log debug only)"); + log.debug("Trace for missing icon data at "+url+": "+e, e); + } else { + log.debug("Missing icon data for "+itemId+", expected at: "+url+" (already logged WARN and error details)"); + } + } + throw WebResourceUtils.notFound("Icon unavailable for %s", itemId); + } } log.debug("Returning redirect to "+url+" as icon for "+result); diff --git a/usage/rest-server/src/main/java/brooklyn/rest/util/WebResourceUtils.java b/usage/rest-server/src/main/java/brooklyn/rest/util/WebResourceUtils.java index 61e4217335..6c1c2c163f 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/util/WebResourceUtils.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/util/WebResourceUtils.java @@ -59,7 +59,7 @@ public static WebApplicationException unauthorized(String format, Object... args /** @throws WebApplicationException With code 404 not found */ public static WebApplicationException notFound(String format, Object... args) { String msg = String.format(format, args); - if (log.isDebugEnabled()) log.debug("returning 404 notFound("+msg+") - may be a stale browser session"); + if (log.isDebugEnabled()) log.debug("returning 404 notFound("+msg+")"); throw new WebApplicationException(Response.status(Response.Status.NOT_FOUND) .type(MediaType.APPLICATION_JSON_TYPE) .entity(ApiError.builder().message(msg).build()).build()); From 9d616595b75416682e9dede1bbd045b22cc1a9d4 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 17 Jul 2014 14:08:47 -0400 Subject: [PATCH 03/11] prevent warning of entitlement context on forwarded requests (eg /) and improve detail for logging REST info --- .../management/entitlement/Entitlements.java | 1 + .../entitlement/WebEntitlementContext.java | 9 ++++- .../AcmeEntitlementManagerTest.java | 8 ++-- .../BrooklynPropertiesSecurityFilter.java | 37 +++++++++++++++++-- 4 files changed, 45 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/brooklyn/management/entitlement/Entitlements.java b/core/src/main/java/brooklyn/management/entitlement/Entitlements.java index 21b6b907ec..f9e188ee79 100644 --- a/core/src/main/java/brooklyn/management/entitlement/Entitlements.java +++ b/core/src/main/java/brooklyn/management/entitlement/Entitlements.java @@ -228,6 +228,7 @@ public static void setEntitlementContext(EntitlementContext context) { EntitlementContext oldContext = PerThreadEntitlementContextHolder.perThreadEntitlementsContextHolder.get(); if (oldContext!=null && context!=null) { log.warn("Changing entitlement context from "+oldContext+" to "+context+"; context should have been reset or extended, not replaced"); + log.debug("Trace for entitlement context duplicate overwrite", new Throwable("Trace for entitlement context overwrite")); } PerThreadEntitlementContextHolder.perThreadEntitlementsContextHolder.set(context); } diff --git a/core/src/main/java/brooklyn/management/entitlement/WebEntitlementContext.java b/core/src/main/java/brooklyn/management/entitlement/WebEntitlementContext.java index 657df22d61..0ae3d90925 100644 --- a/core/src/main/java/brooklyn/management/entitlement/WebEntitlementContext.java +++ b/core/src/main/java/brooklyn/management/entitlement/WebEntitlementContext.java @@ -28,18 +28,23 @@ public class WebEntitlementContext implements EntitlementContext { final String sourceIp; final String requestUri; - public WebEntitlementContext(String user, String sourceIp, String requestUri) { + /** a mostly-unique identifier for the inbound request, to distinguish between duplicate requests and for cross-referencing with URI's */ + final String requestUniqueIdentifier; + + public WebEntitlementContext(String user, String sourceIp, String requestUri, String requestUniqueIdentifier) { this.user = user; this.sourceIp = sourceIp; this.requestUri = requestUri; + this.requestUniqueIdentifier = requestUniqueIdentifier; } @Override public String user() { return user; } public String sourceIp() { return sourceIp; } public String requestUri() { return requestUri; } + public String requestUniqueIdentifier() { return requestUniqueIdentifier; } @Override public String toString() { - return JavaClassNames.simpleClassName(getClass())+"["+user+"@"+sourceIp+":"+requestUri+"]"; + return JavaClassNames.simpleClassName(getClass())+"["+user+"@"+sourceIp+":"+requestUniqueIdentifier+"]"; } } diff --git a/core/src/test/java/brooklyn/management/entitlement/AcmeEntitlementManagerTest.java b/core/src/test/java/brooklyn/management/entitlement/AcmeEntitlementManagerTest.java index 2dc2caa4f6..de9efbb064 100644 --- a/core/src/test/java/brooklyn/management/entitlement/AcmeEntitlementManagerTest.java +++ b/core/src/test/java/brooklyn/management/entitlement/AcmeEntitlementManagerTest.java @@ -68,7 +68,7 @@ public void tearDown() { @Test public void testUserWithMinimal() { setup(configBag); - WebEntitlementContext entitlementContext = new WebEntitlementContext("hacker", "127.0.0.1", URI.create("/applications").toString()); + WebEntitlementContext entitlementContext = new WebEntitlementContext("hacker", "127.0.0.1", URI.create("/applications").toString(), "H"); Entitlements.setEntitlementContext(entitlementContext); Assert.assertFalse(Entitlements.isEntitled(mgmt.getEntitlementManager(), Entitlements.ROOT, null)); Assert.assertFalse(Entitlements.isEntitled(mgmt.getEntitlementManager(), Entitlements.SEE_ENTITY, app)); @@ -81,7 +81,7 @@ public void testUserWithMinimal() { @Test public void testUserWithReadOnly() { setup(configBag); - WebEntitlementContext entitlementContext = new WebEntitlementContext("bob", "127.0.0.1", URI.create("/applications").toString()); + WebEntitlementContext entitlementContext = new WebEntitlementContext("bob", "127.0.0.1", URI.create("/applications").toString(), "B"); Entitlements.setEntitlementContext(entitlementContext); Assert.assertFalse(Entitlements.isEntitled(mgmt.getEntitlementManager(), Entitlements.ROOT, null)); Assert.assertTrue(Entitlements.isEntitled(mgmt.getEntitlementManager(), Entitlements.SEE_ENTITY, app)); @@ -94,7 +94,7 @@ public void testUserWithReadOnly() { @Test public void testUserWithAllPermissions() { setup(configBag); - WebEntitlementContext entitlementContext = new WebEntitlementContext("alice", "127.0.0.1", URI.create("/applications").toString()); + WebEntitlementContext entitlementContext = new WebEntitlementContext("alice", "127.0.0.1", URI.create("/applications").toString(), "A"); Entitlements.setEntitlementContext(entitlementContext); Assert.assertTrue(Entitlements.isEntitled(mgmt.getEntitlementManager(), Entitlements.ROOT, null)); Assert.assertTrue(Entitlements.isEntitled(mgmt.getEntitlementManager(), Entitlements.SEE_ENTITY, app)); @@ -107,7 +107,7 @@ public void testUserWithAllPermissions() { @Test public void testNullHasAllPermissions() { setup(configBag); - WebEntitlementContext entitlementContext = new WebEntitlementContext(null, "127.0.0.1", URI.create("/applications").toString()); + WebEntitlementContext entitlementContext = new WebEntitlementContext(null, "127.0.0.1", URI.create("/applications").toString(), "X"); Entitlements.setEntitlementContext(entitlementContext); Assert.assertTrue(Entitlements.isEntitled(mgmt.getEntitlementManager(), Entitlements.ROOT, null)); Assert.assertTrue(Entitlements.isEntitled(mgmt.getEntitlementManager(), Entitlements.SEE_ENTITY, app)); diff --git a/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java b/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java index 1097d8cf1f..74ab303977 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java @@ -57,6 +57,8 @@ public class BrooklynPropertiesSecurityFilter implements Filter { protected ManagementContext mgmt; protected DelegatingSecurityProvider provider; + + private static ThreadLocal originalRequest = new ThreadLocal(); @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { @@ -73,18 +75,45 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha // do nothing here, fall through to below } else { WebEntitlementContext entitlementContext = null; + String uri = ((HttpServletRequest)request).getRequestURI(); try { - entitlementContext = new WebEntitlementContext(user, ((HttpServletRequest)request).getRemoteAddr(), ((HttpServletRequest)request).getRequestURI()); + String uid = Integer.toHexString(hashCode()); + entitlementContext = new WebEntitlementContext(user, ((HttpServletRequest)request).getRemoteAddr(), uri, uid); + if (originalRequest.get()==null) { + // initial filter application + originalRequest.set(uri); + } else { + // this filter is being applied *again*, probably due to forwarding (e.g. from '/' to '/index.html') + log.debug("REST request {} being forwarded from {} to {}", new Object[] { uid, originalRequest.get(), uri }); + // clear the entitlement context before setting to avoid warnings + Entitlements.clearEntitlementContext(); + } Entitlements.setEntitlementContext(entitlementContext); - log.debug("REST starting processing request {}", entitlementContext); + + log.debug("REST starting processing request {} with {}", uri, entitlementContext); + if (!((HttpServletRequest)request).getParameterMap().isEmpty()) { + log.debug(" REST req {} parameters: {}", uid, ((HttpServletRequest)request).getParameterMap()); + } + if (((HttpServletRequest)request).getContentLength()>0) { + log.debug(" REST req {} upload content type {}", uid, ((HttpServletRequest)request).getContentType()+" length "+((HttpServletRequest)request).getContentLength()); + } + chain.doFilter(request, response); - log.debug("REST completed processing request {}", entitlementContext); + + log.debug("REST completed, code {}, processing request {} with {}", + new Object[] { ((HttpServletResponse)response).getStatus(), uri, entitlementContext } ); return; + } catch (Throwable e) { + // NB errors are typically already caught at this point if (log.isDebugEnabled()) - log.debug("REST failed processing request "+entitlementContext+": "+e, e); + log.debug("REST` failed processing request "+uri+" with "+entitlementContext+": "+e, e); + + log.warn("REST` failed processing request "+uri+" with "+entitlementContext+": "+e, e); throw Exceptions.propagate(e); + } finally { + originalRequest.remove(); Entitlements.clearEntitlementContext(); } } From e8f7283c20f34a8060400e20cc9d7a5974c96917 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 17 Jul 2014 14:55:32 -0400 Subject: [PATCH 04/11] Add a UserFacingException which can be used to indicate to suppress stack traces, and tidy up how REST errors are reported and returned --- .../cli/src/main/java/brooklyn/cli/Main.java | 4 +- .../java/brooklyn/rest/domain/ApiError.java | 24 ++++++++ .../rest/resources/UsageResource.java | 5 +- .../BrooklynPropertiesSecurityFilter.java | 11 +++- .../rest/util/DefaultExceptionMapper.java | 57 ++++++++++++------- .../brooklyn/util/exceptions/Exceptions.java | 21 +++---- .../exceptions/FatalRuntimeException.java | 2 +- .../util/exceptions/UserFacingException.java | 21 +++++++ 8 files changed, 106 insertions(+), 39 deletions(-) create mode 100644 utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java diff --git a/usage/cli/src/main/java/brooklyn/cli/Main.java b/usage/cli/src/main/java/brooklyn/cli/Main.java index ae2696d521..601ded256e 100644 --- a/usage/cli/src/main/java/brooklyn/cli/Main.java +++ b/usage/cli/src/main/java/brooklyn/cli/Main.java @@ -66,6 +66,7 @@ import brooklyn.util.exceptions.Exceptions; import brooklyn.util.exceptions.FatalConfigurationRuntimeException; import brooklyn.util.exceptions.FatalRuntimeException; +import brooklyn.util.exceptions.UserFacingException; import brooklyn.util.guava.Maybe; import brooklyn.util.javalang.Enums; import brooklyn.util.net.Networking; @@ -763,7 +764,8 @@ protected void execCli(Cli parser, String ...args) { } catch (Exception e) { // unexpected error during command execution log.error("Execution error: " + e.getMessage(), e); System.err.println("Execution error: " + e.getMessage()); - e.printStackTrace(); + if (!(e instanceof UserFacingException)) + e.printStackTrace(); System.exit(EXECUTION_ERROR); } } diff --git a/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java b/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java index c293294e11..dd3dcbc3c6 100644 --- a/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java +++ b/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java @@ -20,7 +20,13 @@ import static com.google.common.base.Preconditions.checkNotNull; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.Response.Status; + import org.codehaus.jackson.annotate.JsonProperty; +import org.codehaus.jackson.map.annotate.JsonSerialize; +import org.codehaus.jackson.map.annotate.JsonSerialize.Inclusion; import brooklyn.util.text.Strings; @@ -76,6 +82,11 @@ public Builder details(String details) { return this; } + /** as {@link #prefixMessage(String, String)} with default separator of `: ` */ + public Builder prefixMessage(String prefix) { + return prefixMessage(prefix, ": "); + } + /** puts a prefix in front of the message, with the given separator if there is already a message; * if there is no message, it simply sets the prefix as the message */ @@ -107,6 +118,8 @@ public String getMessage() { } private final String message; + + @JsonSerialize(include=Inclusion.NON_EMPTY) private final String details; public ApiError( @@ -145,4 +158,15 @@ public String toString() { .add("details", details) .toString(); } + + public Response asBadRequestResponseJson() { + return asResponse(Status.BAD_REQUEST, MediaType.APPLICATION_JSON_TYPE); + } + + public Response asResponse(Status status, MediaType type) { + return Response.status(status) + .type(type) + .entity(this) + .build(); + } } diff --git a/usage/rest-server/src/main/java/brooklyn/rest/resources/UsageResource.java b/usage/rest-server/src/main/java/brooklyn/rest/resources/UsageResource.java index b8a1c781f2..88dfe3a6fb 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/resources/UsageResource.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/resources/UsageResource.java @@ -40,6 +40,7 @@ import brooklyn.rest.domain.UsageStatistic; import brooklyn.rest.domain.UsageStatistics; import brooklyn.rest.transform.ApplicationTransformer; +import brooklyn.util.exceptions.UserFacingException; import brooklyn.util.time.Time; import com.google.common.base.Objects; @@ -249,8 +250,8 @@ private List retrieveMachineUsage(LocationUsage usage, Date star private void checkDates(Date startDate, Date endDate) { if (startDate.compareTo(endDate) > 0) { - throw new IllegalArgumentException("Start must be less than or equal to end: " + startDate + " > " + endDate + - " (" + startDate.getTime() + " > " + endDate.getTime() + ")"); + throw new UserFacingException(new IllegalArgumentException("Start must be less than or equal to end: " + startDate + " > " + endDate + + " (" + startDate.getTime() + " > " + endDate.getTime() + ")")); } } diff --git a/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java b/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java index 74ab303977..1331e1fd12 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java @@ -95,7 +95,12 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha log.debug(" REST req {} parameters: {}", uid, ((HttpServletRequest)request).getParameterMap()); } if (((HttpServletRequest)request).getContentLength()>0) { - log.debug(" REST req {} upload content type {}", uid, ((HttpServletRequest)request).getContentType()+" length "+((HttpServletRequest)request).getContentLength()); + int len = ((HttpServletRequest)request).getContentLength(); + log.debug(" REST req {} upload content type {}", uid, ((HttpServletRequest)request).getContentType()+" length "+len + // would be nice to do this, but the stream can only be read once; + // TODO figure out how we could peek at it +// +(len>0 && len<4096 ? ""+Streams.readFullyString(((HttpServletRequest)request).getInputStream()) : "") + ); } chain.doFilter(request, response); @@ -107,9 +112,9 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } catch (Throwable e) { // NB errors are typically already caught at this point if (log.isDebugEnabled()) - log.debug("REST` failed processing request "+uri+" with "+entitlementContext+": "+e, e); + log.debug("REST failed processing request "+uri+" with "+entitlementContext+": "+e, e); - log.warn("REST` failed processing request "+uri+" with "+entitlementContext+": "+e, e); + log.warn("REST failed processing request "+uri+" with "+entitlementContext+": "+e, e); throw Exceptions.propagate(e); } finally { diff --git a/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java b/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java index 02719981eb..4a458ce3db 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java @@ -18,6 +18,8 @@ */ package brooklyn.rest.util; +import java.util.Set; + import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; @@ -29,10 +31,12 @@ import org.slf4j.LoggerFactory; import org.yaml.snakeyaml.error.YAMLException; -import com.google.common.base.Optional; - +import brooklyn.management.entitlement.Entitlements; import brooklyn.rest.domain.ApiError; import brooklyn.rest.domain.ApiError.Builder; +import brooklyn.util.collections.MutableSet; +import brooklyn.util.exceptions.Exceptions; +import brooklyn.util.exceptions.UserFacingException; import brooklyn.util.flags.ClassCoercionException; import brooklyn.util.text.Strings; @@ -41,6 +45,8 @@ public class DefaultExceptionMapper implements ExceptionMapper { private static final Logger LOG = LoggerFactory.getLogger(DefaultExceptionMapper.class); + static Set> warnedUnknownExceptions = MutableSet.of(); + /** * Maps a throwable to a response. *

@@ -51,38 +57,45 @@ public class DefaultExceptionMapper implements ExceptionMapper { @Override public Response toResponse(Throwable throwable) { + LOG.debug("REST request running as {} threw: {}", Entitlements.getEntitlementContext(), throwable); if (LOG.isTraceEnabled()) { - String message = Optional.fromNullable(throwable.getMessage()).or(throwable.getClass().getName()); - LOG.trace("Request threw: " + message); + LOG.trace("Full details of "+Entitlements.getEntitlementContext()+" "+throwable, throwable); } + // Some methods will throw this, which gets converted automatically if (throwable instanceof WebApplicationException) { WebApplicationException wae = (WebApplicationException) throwable; return wae.getResponse(); } - // Assume ClassCoercionExceptions are caused by TypeCoercions from input parameters gone wrong. - if (throwable instanceof ClassCoercionException) - return responseBadRequestJson(ApiError.of(throwable)); + // The nicest way for methods to provide errors, wrap as this, and the stack trace will be suppressed + if (throwable instanceof UserFacingException) { + return ApiError.of(throwable.getMessage()).asBadRequestResponseJson(); + } - if (throwable instanceof YAMLException) - return responseBadRequestJson(ApiError.builderFromThrowable(throwable).prefixMessage("Invalid YAML", ": ").build()); + // For everything else, a trace is supplied + + // Assume ClassCoercionExceptions are caused by TypeCoercions from input parameters gone wrong + // And IllegalArgumentException for malformed input parameters. + if (throwable instanceof ClassCoercionException || throwable instanceof IllegalArgumentException) { + return ApiError.of(throwable).asBadRequestResponseJson(); + } + + // YAML exception + if (throwable instanceof YAMLException) { + return ApiError.builder().message(throwable.getMessage()).prefixMessage("Invalid YAML").build().asBadRequestResponseJson(); + } + + if (!Exceptions.isPrefixBoring(throwable)) { + if ( warnedUnknownExceptions.add( throwable.getClass() )) { + LOG.info("No exception mapping for " + throwable.getClass() + "; consider adding to "+getClass()+" (future warnings logged at debug)"); + } + } - LOG.info("No exception mapping for " + throwable.getClass() + ", responding 500", throwable); Builder rb = ApiError.builderFromThrowable(throwable); if (Strings.isBlank(rb.getMessage())) - rb.message("Internal error. Check server logs for details."); - return Response.status(Status.INTERNAL_SERVER_ERROR) - .type(MediaType.APPLICATION_JSON) - .entity(rb.build()) - .build(); - } - - private Response responseBadRequestJson(ApiError build) { - return Response.status(Status.BAD_REQUEST) - .type(MediaType.APPLICATION_JSON) - .entity(build) - .build(); + rb.message("Internal error. Contact server administrator to consult logs for more details."); + return rb.build().asResponse(Status.INTERNAL_SERVER_ERROR, MediaType.APPLICATION_JSON_TYPE); } } diff --git a/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java b/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java index 9a33566730..a233b0c02a 100644 --- a/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java +++ b/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java @@ -27,7 +27,6 @@ import java.util.List; import java.util.concurrent.ExecutionException; -import brooklyn.util.collections.MutableList; import brooklyn.util.text.Strings; import com.google.common.base.Predicate; @@ -38,11 +37,11 @@ public class Exceptions { - private static final List> BORING_THROWABLES = ImmutableList.>of( + private static final List> BORING_THROWABLE_SUPERTYPES = ImmutableList.>of( ExecutionException.class, InvocationTargetException.class, PropagatedRuntimeException.class); private static boolean isBoring(Throwable t) { - for (Class type: BORING_THROWABLES) + for (Class type: BORING_THROWABLE_SUPERTYPES) if (type.isInstance(t)) return true; return false; } @@ -54,13 +53,15 @@ public boolean apply(Throwable input) { } }; - private static List> BORING_PREFIX_THROWABLES = MutableList.copyOf(BORING_THROWABLES) - .append(IllegalStateException.class).append(RuntimeException.class).append(CompoundRuntimeException.class) - .toImmutable(); + private static List> BORING_PREFIX_THROWABLE_EXACT_TYPES = ImmutableList.>of( + IllegalStateException.class, RuntimeException.class, CompoundRuntimeException.class); - private static boolean isPrefixBoring(Throwable t) { - for (Class type: BORING_PREFIX_THROWABLES) - if (type.isInstance(t)) return true; + /** Returns whether this is throwable either known to be boring or to have an unuseful prefix */ + public static boolean isPrefixBoring(Throwable t) { + if (isBoring(t)) + return true; + for (Class type: BORING_PREFIX_THROWABLE_EXACT_TYPES) + if (t.getClass().equals(type)) return true; return false; } @@ -68,7 +69,7 @@ private static String stripBoringPrefixes(String s) { String was; do { was = s; - for (Class type: BORING_PREFIX_THROWABLES) { + for (Class type: BORING_PREFIX_THROWABLE_EXACT_TYPES) { s = Strings.removeAllFromStart(type.getCanonicalName(), type.getName(), type.getSimpleName(), ":", " "); } } while (!was.equals(s)); diff --git a/utils/common/src/main/java/brooklyn/util/exceptions/FatalRuntimeException.java b/utils/common/src/main/java/brooklyn/util/exceptions/FatalRuntimeException.java index 28f78286e9..3ef9909cd3 100644 --- a/utils/common/src/main/java/brooklyn/util/exceptions/FatalRuntimeException.java +++ b/utils/common/src/main/java/brooklyn/util/exceptions/FatalRuntimeException.java @@ -20,7 +20,7 @@ /** Exception indicating a fatal error, typically used in CLI routines. * The message supplied here should be suitable for display in a CLI response (without stack trace / exception class). */ -public class FatalRuntimeException extends RuntimeException { +public class FatalRuntimeException extends UserFacingException { private static final long serialVersionUID = -3359163414517503809L; diff --git a/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java b/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java new file mode 100644 index 0000000000..1d53d11cc4 --- /dev/null +++ b/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java @@ -0,0 +1,21 @@ +package brooklyn.util.exceptions; + +/** marker interface, to show that an exception is suitable for pretty-printing to an end-user, + * without including a stack trace */ +public class UserFacingException extends RuntimeException { + + private static final long serialVersionUID = 2216885527195571323L; + + public UserFacingException(String message) { + super(message); + } + + public UserFacingException(Throwable cause) { + super(cause.getMessage(), cause); + } + + public UserFacingException(String message, Throwable cause) { + super(message, cause); + } + +} From 920a0928445799d01aef6d88654553125837803e Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 17 Jul 2014 16:06:58 -0400 Subject: [PATCH 05/11] demote dull log messages to 'trace' --- .../brooklyn/entity/basic/EntityDynamicType.java | 16 ++++++++-------- .../management/internal/LocalUsageManager.java | 11 ++++++----- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/brooklyn/entity/basic/EntityDynamicType.java b/core/src/main/java/brooklyn/entity/basic/EntityDynamicType.java index 2d298d790f..468e538ee5 100644 --- a/core/src/main/java/brooklyn/entity/basic/EntityDynamicType.java +++ b/core/src/main/java/brooklyn/entity/basic/EntityDynamicType.java @@ -300,7 +300,7 @@ public static Map> findEffectors(Class claz Effector overwritten = result.put(eff.getName(), eff); Field overwrittenFieldSource = fieldSources.put(eff.getName(), f); if (overwritten!=null && !Effectors.sameInstance(overwritten, eff)) { - LOG.debug("multiple definitions for effector {} on {}; preferring {} from {} to {} from {}", new Object[] { + LOG.trace("multiple definitions for effector {} on {}; preferring {} from {} to {} from {}", new Object[] { eff.getName(), (optionalEntity != null ? optionalEntity : clazz), eff, f, overwritten, overwrittenFieldSource}); } @@ -326,7 +326,7 @@ public static Map> findEffectors(Class claz result.put(eff.getName(), eff); Method overwrittenMethodSource = methodSources.put(eff.getName(), m); Field overwrittenFieldSource = fieldSources.remove(eff.getName()); - LOG.debug("multiple definitions for effector {} on {}; preferring {} from {} to {} from {}", new Object[] { + LOG.trace("multiple definitions for effector {} on {}; preferring {} from {} to {} from {}", new Object[] { eff.getName(), (optionalEntity != null ? optionalEntity : clazz), eff, m, overwritten, (overwrittenMethodSource != null ? overwrittenMethodSource : overwrittenFieldSource)}); } @@ -359,8 +359,8 @@ public static Map> findSensors(Class clazz, E Field source = sources.put(sens.getName(), f); if (overwritten!=null && overwritten != sens) { if (sens instanceof HasConfigKey) { - // probably overriding defaults, just log as debug (there will be add'l logging in config key section) - LOG.debug("multiple definitions for config sensor {} on {}; preferring {} from {} to {} from {}", new Object[] { + // probably overriding defaults, just log low level (there will be add'l logging in config key section) + LOG.trace("multiple definitions for config sensor {} on {}; preferring {} from {} to {} from {}", new Object[] { sens.getName(), optionalEntity!=null ? optionalEntity : clazz, sens, f, overwritten, source}); } else { LOG.warn("multiple definitions for sensor {} on {}; preferring {} from {} to {} from {}", new Object[] { @@ -441,8 +441,8 @@ protected static void buildConfigKeys(Class clazz, AbstractEnt ConfigKey lowerV = lower==null ? null : lower.equals(k.field) ? k.value : best.value; if (best.value == k.value) { // same value doesn't matter which we take (but take lower if there is one) - if (LOG.isDebugEnabled()) - LOG.debug("multiple definitions for config key {} on {}; same value {}; " + + if (LOG.isTraceEnabled()) + LOG.trace("multiple definitions for config key {} on {}; same value {}; " + "from {} and {}, preferring {}", new Object[] { best.value.getName(), optionalEntity!=null ? optionalEntity : clazz, @@ -451,8 +451,8 @@ protected static void buildConfigKeys(Class clazz, AbstractEnt best = new FieldAndValue>(lower!=null ? lower : best.field, best.value); } else if (lower!=null) { // different value, but one clearly lower (in type hierarchy) - if (LOG.isDebugEnabled()) - LOG.debug("multiple definitions for config key {} on {}; " + + if (LOG.isTraceEnabled()) + LOG.trace("multiple definitions for config key {} on {}; " + "from {} and {}, preferring lower {}, value {}", new Object[] { best.value.getName(), optionalEntity!=null ? optionalEntity : clazz, diff --git a/core/src/main/java/brooklyn/management/internal/LocalUsageManager.java b/core/src/main/java/brooklyn/management/internal/LocalUsageManager.java index f825bc3d1c..65efe4489f 100644 --- a/core/src/main/java/brooklyn/management/internal/LocalUsageManager.java +++ b/core/src/main/java/brooklyn/management/internal/LocalUsageManager.java @@ -69,7 +69,7 @@ public LocalUsageManager(LocalManagementContext managementContext) { @Override public void recordApplicationEvent(Application app, Lifecycle state) { - log.debug("Storing location lifecycle event: application {} in state {};", new Object[] {app, state}); + log.debug("Storing application lifecycle usage event: application {} in state {}", new Object[] {app, state}); ConcurrentMap eventMap = managementContext.getStorage().getMap(APPLICATION_USAGE_KEY); synchronized (mutex) { ApplicationUsage usage = eventMap.get(app.getId()); @@ -100,18 +100,18 @@ public void recordLocationEvent(Location loc, Lifecycle state) { checkNotNull(loc, "location"); checkNotNull(state, "state of location %s", loc); if (loc.getId() == null) { - log.error("Ignoring location lifecycle event for {} (state {}), because location has no id", loc, state); + log.error("Ignoring location lifecycle usage event for {} (state {}), because location has no id", loc, state); return; } if (managementContext.getStorage() == null) { - log.warn("Cannot store location lifecycle event for {} (state {}), because storage not available", loc, state); + log.warn("Cannot store location lifecycle usage event for {} (state {}), because storage not available", loc, state); return; } Object callerContext = loc.getConfig(LocationConfigKeys.CALLER_CONTEXT); if (callerContext != null && callerContext instanceof Entity) { - log.debug("Storing location lifecycle event: location {} in state {}; caller context {}", new Object[] {loc, state, callerContext}); + log.debug("Storing location lifecycle usage event: location {} in state {}; caller context {}", new Object[] {loc, state, callerContext}); Entity caller = (Entity) callerContext; String entityTypeName = caller.getEntityType().getName(); @@ -128,7 +128,8 @@ public void recordLocationEvent(Location loc, Lifecycle state) { usageMap.put(loc.getId(), usage); } } else { - log.debug("Not recording location-event for {} in state {}, because no caller context", new Object[] {loc, state}); + // normal for high-level locations + log.trace("Not recording location lifecycle usage event for {} in state {}, because no caller context", new Object[] {loc, state}); } } From 965a3d1f92e878ad52f3cce389ad0dbad017bfc0 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 17 Jul 2014 16:07:59 -0400 Subject: [PATCH 06/11] allow a CREATE_UNMANAGED mode for locations, used when peeking, so that we don't get lots of logging on the frequent location listings from REST API --- .../brooklyn/location/LocationRegistry.java | 2 +- .../java/brooklyn/location/LocationSpec.java | 7 ++++++- .../brooklyn/management/LocationManager.java | 6 +++++- .../location/basic/BasicLocationRegistry.java | 10 ++++++---- .../internal/LocalLocationManager.java | 17 ++++++++++++++++- .../java/brooklyn/util/config/ConfigBag.java | 15 ++++++++++----- 6 files changed, 44 insertions(+), 13 deletions(-) diff --git a/api/src/main/java/brooklyn/location/LocationRegistry.java b/api/src/main/java/brooklyn/location/LocationRegistry.java index 34a163790e..e9db3f2182 100644 --- a/api/src/main/java/brooklyn/location/LocationRegistry.java +++ b/api/src/main/java/brooklyn/location/LocationRegistry.java @@ -58,7 +58,7 @@ public interface LocationRegistry { * but callers should prefer this when they don't wish to create a new location which will be managed in perpetuity! * * @since 0.7.0, but beta and likely to change as the semantics of this class are tuned */ - @Beta + @Beta // see impl for notes public Location resolveForPeeking(LocationDefinition l); /** returns fully populated (config etc) location from the given definition */ diff --git a/api/src/main/java/brooklyn/location/LocationSpec.java b/api/src/main/java/brooklyn/location/LocationSpec.java index d9027b5640..32bf7c2372 100644 --- a/api/src/main/java/brooklyn/location/LocationSpec.java +++ b/api/src/main/java/brooklyn/location/LocationSpec.java @@ -150,11 +150,16 @@ public LocationSpec configure(HasConfigKey key, Task val) return this; } + public LocationSpec removeConfig(ConfigKey key) { + config.remove( checkNotNull(key, "key") ); + return this; + } + public LocationSpec extension(Class extensionType, E extension) { extensions.put(checkNotNull(extensionType, "extensionType"), checkNotNull(extension, "extension")); return this; } - + /** * @return The type of the location */ diff --git a/api/src/main/java/brooklyn/management/LocationManager.java b/api/src/main/java/brooklyn/management/LocationManager.java index ef6f990deb..4708474b66 100644 --- a/api/src/main/java/brooklyn/management/LocationManager.java +++ b/api/src/main/java/brooklyn/management/LocationManager.java @@ -21,7 +21,10 @@ import java.util.Collection; import java.util.Map; +import javax.annotation.Nullable; + import brooklyn.location.Location; +import brooklyn.location.LocationDefinition; import brooklyn.location.LocationSpec; /** @@ -35,7 +38,7 @@ public interface LocationManager { * @param spec */ T createLocation(LocationSpec spec); - + /** * Convenience (particularly for groovy code) to create a location. * Equivalent to {@code createLocation(LocationSpec.create(type).configure(config))} @@ -83,4 +86,5 @@ public interface LocationManager { * (though it may be logged so duplicate calls are best avoided). */ void unmanage(Location loc); + } diff --git a/core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java b/core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java index b3b5d03801..2c7cf36e67 100644 --- a/core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java +++ b/core/src/main/java/brooklyn/location/basic/BasicLocationRegistry.java @@ -42,6 +42,7 @@ import brooklyn.location.LocationResolver; import brooklyn.location.LocationResolver.EnableableLocationResolver; import brooklyn.management.ManagementContext; +import brooklyn.management.internal.LocalLocationManager; import brooklyn.util.collections.MutableMap; import brooklyn.util.config.ConfigBag; import brooklyn.util.javalang.JavaClassNames; @@ -333,10 +334,11 @@ public Location resolve(LocationDefinition ld) { @Override public Location resolveForPeeking(LocationDefinition ld) { - // TODO actually look it up - Location l = resolve(ld, Collections.emptyMap()); - mgmt.getLocationManager().unmanage(l); - return l; + // TODO should clean up how locations are stored, figuring out whether they are shared or not; + // or maybe better, the API calls to this might just want to get the LocationSpec objects back + + // for now we use a 'CREATE_UNMANGED' flag to prevent management (leaks and logging) + return resolve(ld, ConfigBag.newInstance().configure(LocalLocationManager.CREATE_UNMANAGED, true).getAllConfig()); } @Override diff --git a/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java b/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java index 72372bb81c..6c59f3b5c5 100644 --- a/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java +++ b/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java @@ -26,6 +26,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import brooklyn.config.ConfigKey; +import brooklyn.entity.basic.ConfigKeys; import brooklyn.entity.basic.Lifecycle; import brooklyn.entity.proxying.InternalLocationFactory; import brooklyn.internal.storage.BrooklynStorage; @@ -35,6 +37,7 @@ import brooklyn.location.basic.LocationInternal; import brooklyn.management.AccessController; import brooklyn.management.LocationManager; +import brooklyn.util.config.ConfigBag; import brooklyn.util.exceptions.Exceptions; import brooklyn.util.exceptions.RuntimeInterruptedException; @@ -44,6 +47,9 @@ public class LocalLocationManager implements LocationManager { + public static final ConfigKey CREATE_UNMANAGED = ConfigKeys.newBooleanConfigKey("brooklyn.internal.location.createUnmanaged", + "If set on a location or spec, causes the manager to create it in an unmanaged state (for peeking)", false); + private static final Logger log = LoggerFactory.getLogger(LocalLocationManager.class); private final LocalManagementContext managementContext; @@ -71,8 +77,17 @@ public InternalLocationFactory getLocationFactory() { @Override public T createLocation(LocationSpec spec) { try { + boolean createUnmanaged = ConfigBag.coerceFirstNonNullKeyValue(CREATE_UNMANAGED, + spec.getConfig().get(CREATE_UNMANAGED), spec.getFlags().get(CREATE_UNMANAGED.getName())); + if (createUnmanaged) { + spec.removeConfig(CREATE_UNMANAGED); + } + T loc = locationFactory.createLocation(spec); - manage(loc); + if (!createUnmanaged) { + manage(loc); + } + return loc; } catch (Throwable e) { log.warn("Failed to create location using spec "+spec+" (rethrowing)", e); diff --git a/core/src/main/java/brooklyn/util/config/ConfigBag.java b/core/src/main/java/brooklyn/util/config/ConfigBag.java index 575c8e25ff..d35fb78787 100644 --- a/core/src/main/java/brooklyn/util/config/ConfigBag.java +++ b/core/src/main/java/brooklyn/util/config/ConfigBag.java @@ -399,12 +399,17 @@ public Object getWithDeprecation(ConfigKey[] currentKeysInOrderOfPreference, protected T get(ConfigKey key, boolean remove) { // TODO for now, no evaluation -- closure content / smart (self-extracting) keys are NOT supported // (need a clean way to inject that behaviour, as well as desired TypeCoercions) - Object value; if (config.containsKey(key.getName())) - value = getStringKey(key.getName(), remove); - else - value = key.getDefaultValue(); - return TypeCoercions.coerce(value, key.getTypeToken()); + return coerceFirstNonNullKeyValue(key, getStringKey(key.getName(), remove)); + + return coerceFirstNonNullKeyValue(key); + } + + /** returns the first non-null value to be the type indicated by the key, or the keys default value if no non-null values are supplied */ + public static T coerceFirstNonNullKeyValue(ConfigKey key, Object ...values) { + for (Object o: values) + if (o!=null) return TypeCoercions.coerce(o, key.getTypeToken()); + return TypeCoercions.coerce(key.getDefaultValue(), key.getTypeToken()); } protected Object getStringKey(String key, boolean markUsed) { From 2f4367e0d6b4cd2b5f3e0be61ce079586145d915 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 18 Jul 2014 01:09:33 -0400 Subject: [PATCH 07/11] port WebAppRunnerTest to java and improve shutdown --- ...unnerTest.groovy => WebAppRunnerTest.java} | 69 +++++++++---------- 1 file changed, 33 insertions(+), 36 deletions(-) rename usage/launcher/src/test/java/brooklyn/launcher/{WebAppRunnerTest.groovy => WebAppRunnerTest.java} (74%) diff --git a/usage/launcher/src/test/java/brooklyn/launcher/WebAppRunnerTest.groovy b/usage/launcher/src/test/java/brooklyn/launcher/WebAppRunnerTest.java similarity index 74% rename from usage/launcher/src/test/java/brooklyn/launcher/WebAppRunnerTest.groovy rename to usage/launcher/src/test/java/brooklyn/launcher/WebAppRunnerTest.java index af62480e0d..2fbd1319e0 100644 --- a/usage/launcher/src/test/java/brooklyn/launcher/WebAppRunnerTest.groovy +++ b/usage/launcher/src/test/java/brooklyn/launcher/WebAppRunnerTest.java @@ -16,38 +16,37 @@ * specific language governing permissions and limitations * under the License. */ -package brooklyn.launcher +package brooklyn.launcher; -import static java.util.concurrent.TimeUnit.* -import static org.testng.Assert.* +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.fail; -import org.slf4j.Logger -import org.slf4j.LoggerFactory -import org.testng.annotations.AfterMethod -import org.testng.annotations.Test +import java.util.List; +import java.util.Map; -import brooklyn.config.BrooklynProperties -import brooklyn.entity.basic.Entities -import brooklyn.management.internal.LocalManagementContext -import brooklyn.test.HttpTestUtils -import brooklyn.util.internal.TimeExtras -import brooklyn.util.net.Networking -import brooklyn.util.time.Duration +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.Test; -import com.google.common.collect.Lists +import brooklyn.config.BrooklynProperties; +import brooklyn.entity.basic.Entities; +import brooklyn.management.internal.LocalManagementContext; +import brooklyn.management.internal.ManagementContextInternal; +import brooklyn.test.HttpTestUtils; +import brooklyn.util.collections.MutableMap; +import brooklyn.util.net.Networking; + +import com.google.common.collect.Lists; /** * These tests require the brooklyn.war to work. (Should be placed by maven build.) */ public class WebAppRunnerTest { - static { TimeExtras.init() } public static final Logger log = LoggerFactory.getLogger(WebAppRunnerTest.class); - private static Duration TIMEOUT_MS; - static { TIMEOUT_MS = Duration.THIRTY_SECONDS; } - List managementContexts = Lists.newCopyOnWriteArrayList(); @AfterMethod(alwaysRun=true) @@ -64,28 +63,24 @@ LocalManagementContext newManagementContext(BrooklynProperties brooklynPropertie return result; } + @SuppressWarnings({ "unchecked", "rawtypes" }) BrooklynWebServer createWebServer(Map properties) { - Map bigProps = [:] + properties; - Map attributes = bigProps.attributes - if (attributes==null) { - attributes = [:] - } else { - attributes = [:] + attributes; //copy map, don't change what was supplied - } - bigProps.attributes = attributes; + Map bigProps = MutableMap.copyOf(properties); + Map attributes = MutableMap.copyOf( (Map) bigProps.get("attributes") ); + bigProps.put("attributes", attributes); BrooklynProperties brooklynProperties = BrooklynProperties.Factory.newDefault(); brooklynProperties.putAll(bigProps); - brooklynProperties.put("brooklyn.webconsole.security.provider","brooklyn.rest.security.provider.AnyoneSecurityProvider") + brooklynProperties.put("brooklyn.webconsole.security.provider","brooklyn.rest.security.provider.AnyoneSecurityProvider"); brooklynProperties.put("brooklyn.webconsole.security.https.required","false"); return new BrooklynWebServer(bigProps, newManagementContext(brooklynProperties)); } @Test - public void testStartWar1() { + public void testStartWar1() throws Exception { if (!Networking.isPortAvailable(8090)) fail("Another process is using port 8090 which is required for this test."); - BrooklynWebServer server = createWebServer(port:8090); + BrooklynWebServer server = createWebServer(MutableMap.of("port", 8090)); assertNotNull(server); try { @@ -101,10 +96,11 @@ public static void assertBrooklynEventuallyAt(String url) { } @Test - public void testStartSecondaryWar() { + public void testStartSecondaryWar() throws Exception { if (!Networking.isPortAvailable(8090)) fail("Another process is using port 8090 which is required for this test."); - BrooklynWebServer server = createWebServer(port:8090, war:"brooklyn.war", wars:["hello":"hello-world.war"]); + BrooklynWebServer server = createWebServer( + MutableMap.of("port", 8090, "war", "brooklyn.war", "wars", MutableMap.of("hello", "hello-world.war")) ); assertNotNull(server); try { @@ -120,10 +116,10 @@ public void testStartSecondaryWar() { } @Test - public void testStartSecondaryWarAfter() { + public void testStartSecondaryWarAfter() throws Exception { if (!Networking.isPortAvailable(8090)) fail("Another process is using port 8090 which is required for this test."); - BrooklynWebServer server = createWebServer(port:8090, war:"brooklyn.war"); + BrooklynWebServer server = createWebServer(MutableMap.of("port", 8090, "war", "brooklyn.war")); assertNotNull(server); try { @@ -140,9 +136,9 @@ public void testStartSecondaryWarAfter() { } @Test - public void testStartWithLauncher() { + public void testStartWithLauncher() throws Exception { BrooklynLauncher launcher = BrooklynLauncher.newInstance() - .brooklynProperties("brooklyn.webconsole.security.provider",'brooklyn.rest.security.provider.AnyoneSecurityProvider') + .brooklynProperties("brooklyn.webconsole.security.provider","brooklyn.rest.security.provider.AnyoneSecurityProvider") .webapp("/hello", "hello-world.war") .start(); BrooklynServerDetails details = launcher.getServerDetails(); @@ -157,6 +153,7 @@ public void testStartWithLauncher() { } finally { details.getWebServer().stop(); + ((ManagementContextInternal)details.getManagementContext()).terminate(); } } From a882a87b0055ec06fa66eaad749935399504d1d3 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 18 Jul 2014 00:38:46 -0400 Subject: [PATCH 08/11] test and rat fixes, and log message tidy --- .../fixtures/api-error-no-details.json | 3 +-- .../rest/util/DefaultExceptionMapper.java | 3 ++- .../rest/resources/UsageResourceTest.java | 2 +- .../util/exceptions/UserFacingException.java | 18 ++++++++++++++++++ 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/usage/rest-api/src/test/resources/fixtures/api-error-no-details.json b/usage/rest-api/src/test/resources/fixtures/api-error-no-details.json index 4cd16ca246..5762f6f1b9 100644 --- a/usage/rest-api/src/test/resources/fixtures/api-error-no-details.json +++ b/usage/rest-api/src/test/resources/fixtures/api-error-no-details.json @@ -1,4 +1,3 @@ { - "message": "explanatory message", - "details": "" + "message": "explanatory message" } \ No newline at end of file diff --git a/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java b/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java index 4a458ce3db..71bc945d64 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java @@ -88,7 +88,8 @@ public Response toResponse(Throwable throwable) { if (!Exceptions.isPrefixBoring(throwable)) { if ( warnedUnknownExceptions.add( throwable.getClass() )) { - LOG.info("No exception mapping for " + throwable.getClass() + "; consider adding to "+getClass()+" (future warnings logged at debug)"); + throwable.printStackTrace(); + LOG.warn("REST call generated exception type "+throwable.getClass()+" unrecognized in "+getClass()+" (subsequent occurrences will be logged debug only): " + throwable); } } diff --git a/usage/rest-server/src/test/java/brooklyn/rest/resources/UsageResourceTest.java b/usage/rest-server/src/test/java/brooklyn/rest/resources/UsageResourceTest.java index 82629da441..1cadbcd347 100644 --- a/usage/rest-server/src/test/java/brooklyn/rest/resources/UsageResourceTest.java +++ b/usage/rest-server/src/test/java/brooklyn/rest/resources/UsageResourceTest.java @@ -164,7 +164,7 @@ public void testGetApplicationUsage() throws Exception { assertAppUsage(usage, appId, ImmutableList.of(Status.STARTING, Status.RUNNING), roundDown(preStart), postStart); response = client().resource("/v1/usage/applications/" + appId + "?start=9999-01-01T00:00:00+0100").get(ClientResponse.class); - assertTrue(response.getStatus() >= 500, "end defaults to NOW, so future start should fail"); + assertTrue(response.getStatus() >= 400, "end defaults to NOW, so future start should fail, instead got code "+response.getStatus()); response = client().resource("/v1/usage/applications/" + appId + "?start=9999-01-01T00:00:00%2B0100&end=9999-01-02T00:00:00%2B0100").get(ClientResponse.class); usage = response.getEntity(new GenericType() {}); diff --git a/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java b/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java index 1d53d11cc4..b7237ac3d2 100644 --- a/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java +++ b/utils/common/src/main/java/brooklyn/util/exceptions/UserFacingException.java @@ -1,3 +1,21 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ package brooklyn.util.exceptions; /** marker interface, to show that an exception is suitable for pretty-printing to an end-user, From b15a4713d308302c698689d9e50dc41497773684 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 18 Jul 2014 01:26:47 -0400 Subject: [PATCH 09/11] deprecte ApplicationBuilder.newManagedInstance to encourage mgmt context to be supplied, and convert many existing usages to conveniences which load LocalManagementContextForTests (to speed up tests by preventng catalog parsing) --- .../java/brooklyn/entity/basic/ApplicationBuilder.java | 6 ++++++ .../CustomAggregatingEnricherDeprecatedTest.groovy | 3 ++- .../enricher/TransformingEnricherDeprecatedTest.groovy | 2 +- .../java/brooklyn/entity/EffectorSayHiGroovyTest.groovy | 3 +-- .../brooklyn/entity/basic/AbstractEntityLegacyTest.java | 4 ++-- .../java/brooklyn/entity/basic/ConfigMapGroovyTest.groovy | 2 +- .../brooklyn/entity/basic/EntitySubscriptionTest.groovy | 2 +- .../basic/MapListAndOtherStructuredConfigKeyTest.groovy | 2 +- core/src/test/java/brooklyn/entity/group/GroupTest.java | 3 +-- .../brooklyn/entity/rebind/RebindManagerSorterTest.java | 4 ++-- .../basic/AggregatingMachineProvisioningLocationTest.java | 3 ++- .../location/basic/TestPortSupplierLocation.groovy | 3 +-- .../brooklyn/policy/basic/PolicySubscriptionTest.groovy | 2 +- .../test/java/brooklyn/test/entity/TestApplication.java | 8 +++++++- .../java/brooklyn/enricher/HttpLatencyDetectorTest.java | 2 +- .../brooklyn/enricher/TimeFractionDeltaEnricherTest.java | 2 +- .../test/java/brooklyn/policy/ha/ServiceReplacerTest.java | 3 +-- .../brooklyn/entity/brooklynnode/BrooklynNodeTest.java | 4 +--- .../test/java/brooklyn/entity/java/EntityPollingTest.java | 2 +- .../java/brooklyn/entity/java/VanillaJavaAppTest.java | 8 ++++---- .../brooklyn/camp/brooklyn/JavaWebAppsMatchingTest.java | 4 ++-- .../test/java/brooklyn/rest/domain/SensorSummaryTest.java | 3 +-- 22 files changed, 41 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/brooklyn/entity/basic/ApplicationBuilder.java b/core/src/main/java/brooklyn/entity/basic/ApplicationBuilder.java index dd0c650d34..f3be0e078e 100644 --- a/core/src/main/java/brooklyn/entity/basic/ApplicationBuilder.java +++ b/core/src/main/java/brooklyn/entity/basic/ApplicationBuilder.java @@ -70,6 +70,9 @@ public abstract class ApplicationBuilder { @SuppressWarnings("unchecked") @Beta + /** @deprecated since 0.7.0 the management context should normally be passed in; + * for TestApplication also see TestApplication.Factory.newManagedInstanceForTests() */ + @Deprecated public static T newManagedApp(Class type) { if (type.isInterface()) { return (T) newManagedApp(EntitySpec.create(type)); @@ -80,6 +83,9 @@ public static T newManagedApp(Class type) { @SuppressWarnings("unchecked") @Beta + /** @deprecated since 0.7.0 the management context should normally be passed in; + * for TestApplication also see TestApplication.Factory.newManagedInstanceForTests() */ + @Deprecated public static T newManagedApp(EntitySpec spec) { return (T) new ApplicationBuilder(spec) { @Override protected void doBuild() { diff --git a/core/src/test/java/brooklyn/enricher/CustomAggregatingEnricherDeprecatedTest.groovy b/core/src/test/java/brooklyn/enricher/CustomAggregatingEnricherDeprecatedTest.groovy index c51a0e7e6f..adba7098fb 100644 --- a/core/src/test/java/brooklyn/enricher/CustomAggregatingEnricherDeprecatedTest.groovy +++ b/core/src/test/java/brooklyn/enricher/CustomAggregatingEnricherDeprecatedTest.groovy @@ -34,6 +34,7 @@ import brooklyn.event.AttributeSensor import brooklyn.event.basic.BasicAttributeSensor import brooklyn.location.basic.SimulatedLocation import brooklyn.test.TestUtils +import brooklyn.test.entity.LocalManagementContextForTests; import brooklyn.test.entity.TestApplication import brooklyn.test.entity.TestEntity @@ -54,7 +55,7 @@ class CustomAggregatingEnricherDeprecatedTest { @BeforeMethod(alwaysRun=true) public void setUp() { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); producer = app.createAndManageChild(EntitySpec.create(TestEntity.class)); intSensor = new BasicAttributeSensor(Integer.class, "int sensor") target = new BasicAttributeSensor(Long.class, "target sensor") diff --git a/core/src/test/java/brooklyn/enricher/TransformingEnricherDeprecatedTest.groovy b/core/src/test/java/brooklyn/enricher/TransformingEnricherDeprecatedTest.groovy index f0103a50bc..f880232bd3 100644 --- a/core/src/test/java/brooklyn/enricher/TransformingEnricherDeprecatedTest.groovy +++ b/core/src/test/java/brooklyn/enricher/TransformingEnricherDeprecatedTest.groovy @@ -53,7 +53,7 @@ public class TransformingEnricherDeprecatedTest { @BeforeMethod() public void before() { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); producer = app.createAndManageChild(EntitySpec.create(TestEntity.class)); intSensorA = new BasicAttributeSensor(Integer.class, "int.sensor.a"); target = new BasicAttributeSensor(Long.class, "long.sensor.target"); diff --git a/core/src/test/java/brooklyn/entity/EffectorSayHiGroovyTest.groovy b/core/src/test/java/brooklyn/entity/EffectorSayHiGroovyTest.groovy index e13e2ea7de..9061a99d7f 100644 --- a/core/src/test/java/brooklyn/entity/EffectorSayHiGroovyTest.groovy +++ b/core/src/test/java/brooklyn/entity/EffectorSayHiGroovyTest.groovy @@ -28,7 +28,6 @@ import org.testng.annotations.Test import brooklyn.entity.annotation.EffectorParam import brooklyn.entity.basic.AbstractEntity -import brooklyn.entity.basic.ApplicationBuilder import brooklyn.entity.basic.BasicParameterType import brooklyn.entity.basic.BrooklynTaskTags import brooklyn.entity.basic.Entities @@ -55,7 +54,7 @@ public class EffectorSayHiGroovyTest { @BeforeMethod(alwaysRun=true) public void setUp() { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); e = app.createAndManageChild(EntitySpec.create(MyEntity.class)); } diff --git a/core/src/test/java/brooklyn/entity/basic/AbstractEntityLegacyTest.java b/core/src/test/java/brooklyn/entity/basic/AbstractEntityLegacyTest.java index 55077101cf..3c0bb1e1da 100644 --- a/core/src/test/java/brooklyn/entity/basic/AbstractEntityLegacyTest.java +++ b/core/src/test/java/brooklyn/entity/basic/AbstractEntityLegacyTest.java @@ -108,7 +108,7 @@ public void testLegacyConstructionCallsConfigureMethod() throws Exception { @Test public void testNewStyleCallsConfigureAfterConstruction() throws Exception { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); MyEntity entity = app.addChild(EntitySpec.create(MyEntity.class)); assertEquals(entity.getConfigureCount(), 1); @@ -139,7 +139,7 @@ public void testLegacyConstructionUsesCustomDisplayName() throws Exception { @Test public void testNewStyleSetsDefaultDisplayName() throws Exception { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); MyEntity entity = app.addChild(EntitySpec.create(MyEntity.class)); assertTrue(entity.getDisplayName().startsWith("MyEntity:"+entity.getId().substring(0,4)), "displayName="+entity.getDisplayName()); diff --git a/core/src/test/java/brooklyn/entity/basic/ConfigMapGroovyTest.groovy b/core/src/test/java/brooklyn/entity/basic/ConfigMapGroovyTest.groovy index f9d905cb61..627b6f2661 100644 --- a/core/src/test/java/brooklyn/entity/basic/ConfigMapGroovyTest.groovy +++ b/core/src/test/java/brooklyn/entity/basic/ConfigMapGroovyTest.groovy @@ -36,7 +36,7 @@ public class ConfigMapGroovyTest { @BeforeMethod(alwaysRun=true) public void setUp() { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); entity = new MySubEntity(app); Entities.manage(entity); } diff --git a/core/src/test/java/brooklyn/entity/basic/EntitySubscriptionTest.groovy b/core/src/test/java/brooklyn/entity/basic/EntitySubscriptionTest.groovy index 23a612cfa7..1317c123a1 100644 --- a/core/src/test/java/brooklyn/entity/basic/EntitySubscriptionTest.groovy +++ b/core/src/test/java/brooklyn/entity/basic/EntitySubscriptionTest.groovy @@ -56,7 +56,7 @@ public class EntitySubscriptionTest { @BeforeMethod(alwaysRun=true) public void setUp() { loc = new SimulatedLocation(); - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); entity = app.createAndManageChild(EntitySpec.create(TestEntity.class)); observedEntity = app.createAndManageChild(EntitySpec.create(TestEntity.class)); observedChildEntity = observedEntity.createAndManageChild(EntitySpec.create(TestEntity.class)); diff --git a/core/src/test/java/brooklyn/entity/basic/MapListAndOtherStructuredConfigKeyTest.groovy b/core/src/test/java/brooklyn/entity/basic/MapListAndOtherStructuredConfigKeyTest.groovy index f60d33023e..64621991ab 100644 --- a/core/src/test/java/brooklyn/entity/basic/MapListAndOtherStructuredConfigKeyTest.groovy +++ b/core/src/test/java/brooklyn/entity/basic/MapListAndOtherStructuredConfigKeyTest.groovy @@ -49,7 +49,7 @@ public class MapListAndOtherStructuredConfigKeyTest { @BeforeMethod(alwaysRun=true) public void setUp() { locs = ImmutableList.of(new SimulatedLocation()); - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); entity = app.createAndManageChild(EntitySpec.create(TestEntity.class)); } diff --git a/core/src/test/java/brooklyn/entity/group/GroupTest.java b/core/src/test/java/brooklyn/entity/group/GroupTest.java index 139217583a..43a31c0214 100644 --- a/core/src/test/java/brooklyn/entity/group/GroupTest.java +++ b/core/src/test/java/brooklyn/entity/group/GroupTest.java @@ -25,7 +25,6 @@ import org.testng.annotations.Test; import brooklyn.entity.Entity; -import brooklyn.entity.basic.ApplicationBuilder; import brooklyn.entity.basic.BasicGroup; import brooklyn.entity.basic.Entities; import brooklyn.entity.proxying.EntitySpec; @@ -52,7 +51,7 @@ public class GroupTest { @BeforeMethod public void setUp() { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); loc = app.getManagementContext().getLocationManager().createLocation(LocationSpec.create(SimulatedLocation.class)); group = app.createAndManageChild(EntitySpec.create(BasicGroup.class)); entity1 = app.createAndManageChild(EntitySpec.create(TestEntity.class)); diff --git a/core/src/test/java/brooklyn/entity/rebind/RebindManagerSorterTest.java b/core/src/test/java/brooklyn/entity/rebind/RebindManagerSorterTest.java index 1b71e6ab2c..991a0003be 100644 --- a/core/src/test/java/brooklyn/entity/rebind/RebindManagerSorterTest.java +++ b/core/src/test/java/brooklyn/entity/rebind/RebindManagerSorterTest.java @@ -54,7 +54,7 @@ public class RebindManagerSorterTest { @BeforeMethod(alwaysRun=true) public void setUp() throws Exception { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); mgmts.add(managementContext = app.getManagementContext()); rebindManager = (RebindManagerImpl) managementContext.getRebindManager(); } @@ -95,7 +95,7 @@ public void testSortOrderMultipleBranches() throws Exception { @Test public void testSortOrderMultipleApps() throws Exception { - TestApplication app2 = ApplicationBuilder.newManagedApp(TestApplication.class); + TestApplication app2 = TestApplication.Factory.newManagedInstanceForTests(); mgmts.add(app2.getManagementContext()); TestEntity e1a = app.createAndManageChild(EntitySpec.create(TestEntity.class)); diff --git a/core/src/test/java/brooklyn/location/basic/AggregatingMachineProvisioningLocationTest.java b/core/src/test/java/brooklyn/location/basic/AggregatingMachineProvisioningLocationTest.java index 32ca4ea9f3..e494a1f279 100644 --- a/core/src/test/java/brooklyn/location/basic/AggregatingMachineProvisioningLocationTest.java +++ b/core/src/test/java/brooklyn/location/basic/AggregatingMachineProvisioningLocationTest.java @@ -34,6 +34,7 @@ import brooklyn.location.NoMachinesAvailableException; import brooklyn.location.basic.LocalhostMachineProvisioningLocation.LocalhostMachine; import brooklyn.management.internal.LocalManagementContext; +import brooklyn.test.entity.LocalManagementContextForTests; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -53,7 +54,7 @@ public class AggregatingMachineProvisioningLocationTest { @BeforeMethod(alwaysRun=true) @SuppressWarnings("unchecked") public void setUp() { - managementContext = new LocalManagementContext(); + managementContext = new LocalManagementContextForTests(); machine1a = newLocation(LocalhostMachine.class, "1a"); machine1b = newLocation(LocalhostMachine.class, "1b"); machine2a = newLocation(LocalhostMachine.class, "2a"); diff --git a/core/src/test/java/brooklyn/location/basic/TestPortSupplierLocation.groovy b/core/src/test/java/brooklyn/location/basic/TestPortSupplierLocation.groovy index 7d2fcb66c9..51579d630d 100644 --- a/core/src/test/java/brooklyn/location/basic/TestPortSupplierLocation.groovy +++ b/core/src/test/java/brooklyn/location/basic/TestPortSupplierLocation.groovy @@ -23,7 +23,6 @@ import org.testng.annotations.AfterMethod import org.testng.annotations.BeforeMethod import org.testng.annotations.Test -import brooklyn.entity.basic.ApplicationBuilder import brooklyn.entity.basic.Entities import brooklyn.entity.proxying.EntitySpec import brooklyn.event.basic.PortAttributeSensorAndConfigKey @@ -41,7 +40,7 @@ public class TestPortSupplierLocation { @BeforeMethod public void setup() { l = new SimulatedLocation(); - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); e = app.createAndManageChild(EntitySpec.create(TestEntity.class)); app.start([l]); diff --git a/core/src/test/java/brooklyn/policy/basic/PolicySubscriptionTest.groovy b/core/src/test/java/brooklyn/policy/basic/PolicySubscriptionTest.groovy index 415d1c41b0..2290c07bd2 100644 --- a/core/src/test/java/brooklyn/policy/basic/PolicySubscriptionTest.groovy +++ b/core/src/test/java/brooklyn/policy/basic/PolicySubscriptionTest.groovy @@ -55,7 +55,7 @@ public class PolicySubscriptionTest { @BeforeMethod(alwaysRun=true) public void setUp() { loc = new SimulatedLocation(); - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); entity = app.createAndManageChild(EntitySpec.create(TestEntity.class)); entity2 = app.createAndManageChild(EntitySpec.create(TestEntity.class)); listener = new RecordingSensorEventListener(); diff --git a/core/src/test/java/brooklyn/test/entity/TestApplication.java b/core/src/test/java/brooklyn/test/entity/TestApplication.java index 7614b35ceb..cc0a6282ef 100644 --- a/core/src/test/java/brooklyn/test/entity/TestApplication.java +++ b/core/src/test/java/brooklyn/test/entity/TestApplication.java @@ -19,6 +19,7 @@ package brooklyn.test.entity; import brooklyn.entity.Entity; +import brooklyn.entity.basic.ApplicationBuilder; import brooklyn.entity.basic.EntityInternal; import brooklyn.entity.basic.StartableApplication; import brooklyn.entity.proxying.EntitySpec; @@ -39,5 +40,10 @@ public interface TestApplication extends StartableApplication, EntityInternal { public T createAndManageChild(EntitySpec spec); public LocalhostMachineProvisioningLocation newLocalhostProvisioningLocation(); - + + public static class Factory { + public static TestApplication newManagedInstanceForTests() { + return ApplicationBuilder.newManagedApp(TestApplication.class, new LocalManagementContextForTests()); + } + } } diff --git a/policy/src/test/java/brooklyn/enricher/HttpLatencyDetectorTest.java b/policy/src/test/java/brooklyn/enricher/HttpLatencyDetectorTest.java index 989b1cebb8..b8504ad3b0 100644 --- a/policy/src/test/java/brooklyn/enricher/HttpLatencyDetectorTest.java +++ b/policy/src/test/java/brooklyn/enricher/HttpLatencyDetectorTest.java @@ -57,7 +57,7 @@ public class HttpLatencyDetectorTest { @BeforeMethod(alwaysRun=true) public void setUp() throws Exception { loc = new LocalhostMachineProvisioningLocation(); - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); entity = app.createAndManageChild(EntitySpec.create(TestEntity.class)); app.start(ImmutableList.of(loc)); diff --git a/policy/src/test/java/brooklyn/enricher/TimeFractionDeltaEnricherTest.java b/policy/src/test/java/brooklyn/enricher/TimeFractionDeltaEnricherTest.java index f87a3049c0..57abb98fd0 100644 --- a/policy/src/test/java/brooklyn/enricher/TimeFractionDeltaEnricherTest.java +++ b/policy/src/test/java/brooklyn/enricher/TimeFractionDeltaEnricherTest.java @@ -51,7 +51,7 @@ public class TimeFractionDeltaEnricherTest { @BeforeMethod(alwaysRun=true) public void before() { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); producer = app.createAndManageChild(EntitySpec.create(TestEntity.class)); intSensor = Sensors.newIntegerSensor("int sensor"); diff --git a/policy/src/test/java/brooklyn/policy/ha/ServiceReplacerTest.java b/policy/src/test/java/brooklyn/policy/ha/ServiceReplacerTest.java index 830f7e9a3d..4b5a0d9804 100644 --- a/policy/src/test/java/brooklyn/policy/ha/ServiceReplacerTest.java +++ b/policy/src/test/java/brooklyn/policy/ha/ServiceReplacerTest.java @@ -184,8 +184,7 @@ public void testDoesNotOnFireWhenFailToReplaceMember() throws Exception { assertEventuallyHasEntityReplacementFailedEvent(cluster); } - - @Test + @Test(groups="Integration") // 1s wait public void testStopFailureOfOldEntityDoesNotSetClusterOnFire() throws Exception { app.subscribe(null, ServiceReplacer.ENTITY_REPLACEMENT_FAILED, eventListener); diff --git a/software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeTest.java b/software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeTest.java index 4d3739106c..7c9641620e 100644 --- a/software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeTest.java +++ b/software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeTest.java @@ -27,9 +27,7 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; -import brooklyn.entity.basic.ApplicationBuilder; import brooklyn.entity.basic.Entities; -import brooklyn.entity.basic.EntityLocal; import brooklyn.entity.drivers.downloads.DownloadResolver; import brooklyn.event.feed.ConfigToAttributes; import brooklyn.location.basic.SshMachineLocation; @@ -48,7 +46,7 @@ public class BrooklynNodeTest { @BeforeMethod(alwaysRun=true) public void setUp() throws Exception { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); loc = new SshMachineLocation(MutableMap.of("address", "localhost")); } diff --git a/software/base/src/test/java/brooklyn/entity/java/EntityPollingTest.java b/software/base/src/test/java/brooklyn/entity/java/EntityPollingTest.java index cfabf980b0..7960a075d6 100644 --- a/software/base/src/test/java/brooklyn/entity/java/EntityPollingTest.java +++ b/software/base/src/test/java/brooklyn/entity/java/EntityPollingTest.java @@ -127,7 +127,7 @@ public VanillaJavaAppSshDriver newDriver(MachineLocation loc) { @BeforeMethod(alwaysRun=true) public void setUp() { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); /* * Create an entity, using real entity code, but that swaps out the external process diff --git a/software/base/src/test/java/brooklyn/entity/java/VanillaJavaAppTest.java b/software/base/src/test/java/brooklyn/entity/java/VanillaJavaAppTest.java index aaac6b8931..851aebd5bc 100644 --- a/software/base/src/test/java/brooklyn/entity/java/VanillaJavaAppTest.java +++ b/software/base/src/test/java/brooklyn/entity/java/VanillaJavaAppTest.java @@ -45,7 +45,6 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; -import brooklyn.entity.basic.ApplicationBuilder; import brooklyn.entity.basic.Entities; import brooklyn.entity.basic.EntityLocal; import brooklyn.entity.basic.Lifecycle; @@ -81,8 +80,8 @@ public class VanillaJavaAppTest { private static final Object LONG_TIMEOUT_MS = 61*1000; private static String BROOKLYN_THIS_CLASSPATH = null; - private static Class MAIN_CLASS = ExampleVanillaMain.class; - private static Class MAIN_CPU_HUNGRY_CLASS = ExampleVanillaMainCpuHungry.class; + private static Class MAIN_CLASS = ExampleVanillaMain.class; + private static Class MAIN_CPU_HUNGRY_CLASS = ExampleVanillaMainCpuHungry.class; private TestApplication app; private LocalhostMachineProvisioningLocation loc; @@ -92,7 +91,7 @@ public void setUp() throws Exception { if (BROOKLYN_THIS_CLASSPATH==null) { BROOKLYN_THIS_CLASSPATH = ResourceUtils.create(MAIN_CLASS).getClassLoaderDir(); } - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); loc = new LocalhostMachineProvisioningLocation(MutableMap.of("address", "localhost")); } @@ -307,6 +306,7 @@ private static class AsserterForJmxConnection { final JMXServiceURL url; final Map env; + @SuppressWarnings("unchecked") public AsserterForJmxConnection(VanillaJavaApp e) throws MalformedURLException { this.entity = e; diff --git a/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/JavaWebAppsMatchingTest.java b/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/JavaWebAppsMatchingTest.java index e405d5ec24..28e8ef5191 100644 --- a/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/JavaWebAppsMatchingTest.java +++ b/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/JavaWebAppsMatchingTest.java @@ -38,7 +38,7 @@ import brooklyn.entity.basic.Entities; import brooklyn.management.ManagementContext; -import brooklyn.management.internal.LocalManagementContext; +import brooklyn.test.entity.LocalManagementContextForTests; import brooklyn.util.ResourceUtils; import brooklyn.util.collections.MutableMap; import brooklyn.util.stream.Streams; @@ -54,7 +54,7 @@ public class JavaWebAppsMatchingTest { @BeforeMethod(alwaysRun=true) public void setup() { - brooklynMgmt = new LocalManagementContext(); + brooklynMgmt = new LocalManagementContextForTests(); platform = new BrooklynCampPlatform( PlatformRootSummary.builder().name("Brooklyn CAMP Platform").build(), brooklynMgmt); diff --git a/usage/rest-server/src/test/java/brooklyn/rest/domain/SensorSummaryTest.java b/usage/rest-server/src/test/java/brooklyn/rest/domain/SensorSummaryTest.java index ba6ee4c5ae..ccf132007f 100644 --- a/usage/rest-server/src/test/java/brooklyn/rest/domain/SensorSummaryTest.java +++ b/usage/rest-server/src/test/java/brooklyn/rest/domain/SensorSummaryTest.java @@ -31,7 +31,6 @@ import org.testng.annotations.Test; import brooklyn.config.render.RendererHints; -import brooklyn.entity.basic.ApplicationBuilder; import brooklyn.entity.basic.Entities; import brooklyn.entity.proxying.EntitySpec; import brooklyn.event.AttributeSensor; @@ -56,7 +55,7 @@ public class SensorSummaryTest { @BeforeMethod(alwaysRun=true) public void setUp() throws Exception { - app = ApplicationBuilder.newManagedApp(TestApplication.class); + app = TestApplication.Factory.newManagedInstanceForTests(); mgmt = app.getManagementContext(); entity = app.createAndManageChild(EntitySpec.create(TestEntity.class)); } From 01d6136a6255a86c4ddb36c9840dfc62083478b6 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 18 Jul 2014 02:02:50 -0400 Subject: [PATCH 10/11] move some many-times test to integration group so build runs faster --- core/src/test/java/brooklyn/entity/rebind/RebindEntityTest.java | 2 +- .../management/ha/HighAvailabilityManagerSplitBrainTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/brooklyn/entity/rebind/RebindEntityTest.java b/core/src/test/java/brooklyn/entity/rebind/RebindEntityTest.java index d16bea68cd..1292dea11f 100644 --- a/core/src/test/java/brooklyn/entity/rebind/RebindEntityTest.java +++ b/core/src/test/java/brooklyn/entity/rebind/RebindEntityTest.java @@ -194,7 +194,7 @@ public void testRestoresEntityIdAndDisplayName() throws Exception { // Saw this fail during development (fixed now); but want at least one of these tests to be run // many times for stress testing purposes - @Test(invocationCount=100, groups="Integeration") + @Test(invocationCount=100, groups="Integration") public void testRestoresEntityIdAndDisplayNameManyTimes() throws Exception { testRestoresEntityIdAndDisplayName(); } diff --git a/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerSplitBrainTest.java b/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerSplitBrainTest.java index 26627d6dba..ddf6b73a02 100644 --- a/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerSplitBrainTest.java +++ b/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerSplitBrainTest.java @@ -274,7 +274,7 @@ public void testIfNodeStopsBeingAbleToWrite() throws Exception { assertEquals(n2.mgmt.getApplications().size(), 1); } - @Test(invocationCount=50) + @Test(invocationCount=50, groups="Integration") public void testIfNodeStopsBeingAbleToWriteManyTimes() throws Exception { testIfNodeStopsBeingAbleToWrite(); } From 3fc4370f9dec8df26d4f86bf815048cb3ad9cce2 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 18 Jul 2014 09:18:19 -0400 Subject: [PATCH 11/11] address review comments --- .../brooklyn/management/internal/LocalLocationManager.java | 2 ++ .../rest/security/BrooklynPropertiesSecurityFilter.java | 4 ++-- .../main/java/brooklyn/rest/util/DefaultExceptionMapper.java | 1 - 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java b/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java index 6c59f3b5c5..d2fb3d7eed 100644 --- a/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java +++ b/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java @@ -41,12 +41,14 @@ import brooklyn.util.exceptions.Exceptions; import brooklyn.util.exceptions.RuntimeInterruptedException; +import com.google.common.annotations.Beta; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.Maps; public class LocalLocationManager implements LocationManager { + @Beta /* expect to remove when API returns LocationSpec or similar */ public static final ConfigKey CREATE_UNMANAGED = ConfigKeys.newBooleanConfigKey("brooklyn.internal.location.createUnmanaged", "If set on a location or spec, causes the manager to create it in an unmanaged state (for peeking)", false); diff --git a/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java b/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java index 1331e1fd12..801344d4d7 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java @@ -111,10 +111,10 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } catch (Throwable e) { // NB errors are typically already caught at this point - if (log.isDebugEnabled()) + if (log.isDebugEnabled()) { log.debug("REST failed processing request "+uri+" with "+entitlementContext+": "+e, e); + } - log.warn("REST failed processing request "+uri+" with "+entitlementContext+": "+e, e); throw Exceptions.propagate(e); } finally { diff --git a/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java b/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java index 71bc945d64..ea395bc552 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/util/DefaultExceptionMapper.java @@ -88,7 +88,6 @@ public Response toResponse(Throwable throwable) { if (!Exceptions.isPrefixBoring(throwable)) { if ( warnedUnknownExceptions.add( throwable.getClass() )) { - throwable.printStackTrace(); LOG.warn("REST call generated exception type "+throwable.getClass()+" unrecognized in "+getClass()+" (subsequent occurrences will be logged debug only): " + throwable); } }