From 83d8626d2fc19c4446ec73a025c30ec72eb4aea1 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Tue, 15 Nov 2016 07:43:38 +0000 Subject: [PATCH 1/3] BROOKLYN-386: Avoid NPE in JcloudsSshMachineLocation.getOptionalNode --- .../jclouds/JcloudsSshMachineLocation.java | 32 +++++++++++++++---- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsSshMachineLocation.java b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsSshMachineLocation.java index 0ec99e5236..3688aa891b 100644 --- a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsSshMachineLocation.java +++ b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsSshMachineLocation.java @@ -43,6 +43,7 @@ import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.net.Networking; import org.apache.brooklyn.util.text.Strings; +import org.jclouds.compute.ComputeService; import org.jclouds.compute.ComputeServiceContext; import org.jclouds.compute.callables.RunScriptOnNode; import org.jclouds.compute.domain.ExecResponse; @@ -212,11 +213,22 @@ protected void setTemplate(Template template) { _image = Optional.fromNullable(template.getImage()); } + protected ComputeService getComputeServiceOrNull() { + JcloudsLocation parent = getParent(); + return (parent != null) ? parent.getComputeService() : null; + } + @Override public Optional getOptionalNode() { if (_node == null) { try { - _node = Optional.fromNullable(getParent().getComputeService().getNodeMetadata(nodeId)); + ComputeService computeService = getComputeServiceOrNull(); + if (computeService == null) { + if (LOG.isDebugEnabled()) LOG.debug("Cannot get node for {}, because cannot get compute-service from parent {}", this, getParent()); + _node = Optional.absent(); + } else { + _node = Optional.fromNullable(computeService.getNodeMetadata(nodeId)); + } } catch (Exception e) { Exceptions.propagateIfFatal(e); if (LOG.isDebugEnabled()) LOG.debug("Problem getting node-metadata for " + this + ", node id " + nodeId + " (continuing)", e); @@ -232,7 +244,13 @@ protected Optional getOptionalImage() { _image = Optional.absent(); // can happen with JcloudsLocation.resumeMachine() usage } else { try { - _image = Optional.fromNullable(getParent().getComputeService().getImage(imageId)); + ComputeService computeService = getComputeServiceOrNull(); + if (computeService == null) { + if (LOG.isDebugEnabled()) LOG.debug("Cannot get image (with id {}) for {}, because cannot get compute-service from parent {}", new Object[] {imageId, this, getParent()}); + _node = Optional.absent(); + } else { + _image = Optional.fromNullable(computeService.getImage(imageId)); + } } catch (Exception e) { Exceptions.propagateIfFatal(e); if (LOG.isDebugEnabled()) LOG.debug("Problem getting image for " + this + ", image id " + imageId + " (continuing)", e); @@ -440,7 +458,7 @@ public ListenableFuture submitRunScript(Statement script, RunScrip Optional node = getOptionalNode(); if (!node.isPresent()) { - throw new IllegalStateException("Node "+nodeId+" not present in "+getParent()); + throw new IllegalStateException("Node "+nodeId+" not present in "+jcloudsParent); } if (jcloudsParent == null) { throw new IllegalStateException("No jclouds parent location for "+this+"; cannot retrieve jclouds script-runner"); @@ -596,16 +614,16 @@ protected MachineDetails inferMachineDetails() { @Override public Map toMetadataRecord() { + JcloudsLocation parent = getParent(); Optional node = getOptionalNode(); - Optional hardware = getOptionalHardware(); List processors = hardware.isPresent() ? hardware.get().getProcessors() : null; ImmutableMap.Builder builder = ImmutableMap.builder(); builder.putAll(super.toMetadataRecord()); - putIfNotNull(builder, "provider", getParent().getProvider()); - putIfNotNull(builder, "account", getParent().getIdentity()); - putIfNotNull(builder, "region", getParent().getRegion()); + putIfNotNull(builder, "provider", (parent != null) ? parent.getProvider() : null); + putIfNotNull(builder, "account", (parent != null) ? parent.getIdentity() : null); + putIfNotNull(builder, "region", (parent != null) ? parent.getRegion() : null); putIfNotNull(builder, "serverId", getJcloudsId()); putIfNotNull(builder, "imageId", getImageId()); putIfNotNull(builder, "instanceTypeName", (hardware.isPresent() ? hardware.get().getName() : null)); From b42210ee2556bc2a97a53387f6ffc4f4ee01efe6 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Tue, 15 Nov 2016 09:57:31 +0000 Subject: [PATCH 2/3] BROOKLYN-386: avoid locationAdded events on rebind --- .../brooklyn/core/entity/AbstractEntity.java | 30 ++++++++++++++----- .../brooklyn/core/entity/EntityInternal.java | 15 ++++++---- .../mgmt/rebind/BasicEntityRebindSupport.java | 4 +-- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java b/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java index fd7a40391d..26da5d61ef 100644 --- a/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java +++ b/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java @@ -891,6 +891,17 @@ public Collection getLocations() { @Override public void addLocations(Collection newLocations) { + addLocationsImpl(newLocations, true); + } + + @Override + @Beta + public void addLocationsWithoutPublishing(Collection newLocations) { + addLocationsImpl(newLocations, false); + } + + @Beta + protected void addLocationsImpl(Collection newLocations, boolean publish) { if (newLocations==null || newLocations.isEmpty()) { return; } @@ -918,18 +929,23 @@ public void addLocations(Collection newLocations) { locations.set(ImmutableList.builder().addAll(oldLocations).addAll(trulyNewLocations).build()); } - for (Location loc : trulyNewLocations) { - sensors().emit(AbstractEntity.LOCATION_ADDED, loc); + if (publish) { + for (Location loc : trulyNewLocations) { + sensors().emit(AbstractEntity.LOCATION_ADDED, loc); + } } } - if (getManagementSupport().isDeployed()) { - for (Location newLocation : newLocations) { - // Location is now reachable, so manage it - // TODO will not be required in future releases when creating locations always goes through LocationManager.createLocation(LocationSpec). - Locations.manage(newLocation, getManagementContext()); + if (publish) { + if (getManagementSupport().isDeployed()) { + for (Location newLocation : newLocations) { + // Location is now reachable, so manage it + // TODO will not be required in future releases when creating locations always goes through LocationManager.createLocation(LocationSpec). + Locations.manage(newLocation, getManagementContext()); + } } } + getManagementSupport().getEntityChangeListener().onLocationsChanged(); } diff --git a/core/src/main/java/org/apache/brooklyn/core/entity/EntityInternal.java b/core/src/main/java/org/apache/brooklyn/core/entity/EntityInternal.java index 56109038a2..bd0fcce3de 100644 --- a/core/src/main/java/org/apache/brooklyn/core/entity/EntityInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/entity/EntityInternal.java @@ -23,8 +23,6 @@ import javax.annotation.Nullable; -import com.google.common.annotations.Beta; - import org.apache.brooklyn.api.effector.Effector; import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.entity.EntityLocal; @@ -38,11 +36,10 @@ import org.apache.brooklyn.api.mgmt.rebind.mementos.EntityMemento; import org.apache.brooklyn.api.sensor.AttributeSensor; import org.apache.brooklyn.api.sensor.Feed; -import org.apache.brooklyn.config.ConfigKey; -import org.apache.brooklyn.core.entity.internal.EntityConfigMap; import org.apache.brooklyn.core.mgmt.internal.EntityManagementSupport; import org.apache.brooklyn.core.objs.BrooklynObjectInternal; -import org.apache.brooklyn.util.core.config.ConfigBag; + +import com.google.common.annotations.Beta; /** * Extended Entity interface with additional functionality that is purely-internal (i.e. intended @@ -55,6 +52,14 @@ public interface EntityInternal extends BrooklynObjectInternal, EntityLocal, Reb void removeLocations(Collection locations); + /** + * Adds the given locations to this entity, but without emitting {@link AbstractEntity#LOCATION_ADDED} + * events, and without auto-managing the locations. This is for internal purposes only; it is primarily + * intended for use during rebind. + */ + @Beta + void addLocationsWithoutPublishing(@Nullable Collection locations); + void clearLocations(); /** diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicEntityRebindSupport.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicEntityRebindSupport.java index 29a39f86f0..b85b768b66 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicEntityRebindSupport.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicEntityRebindSupport.java @@ -90,7 +90,7 @@ protected void addCustoms(RebindContext rebindContext, EntityMemento memento) { Object value = entry.getValue(); @SuppressWarnings("unused") // just to ensure we can load the declared type? or maybe not needed Class type = (key.getType() != null) ? key.getType() : rebindContext.loadClass(key.getTypeName()); - ((EntityInternal)entity).setAttributeWithoutPublishing((AttributeSensor)key, value); + ((EntityInternal)entity).sensors().setWithoutPublishing((AttributeSensor)key, value); } catch (Exception e) { LOG.warn("Error adding custom sensor "+entry+" when rebinding "+entity+" (rethrowing): "+e); throw Exceptions.propagate(e); @@ -232,7 +232,7 @@ protected void addLocations(RebindContext rebindContext, EntityMemento memento) for (String id : memento.getLocations()) { Location loc = rebindContext.lookup().lookupLocation(id); if (loc != null) { - ((EntityInternal)entity).addLocations(ImmutableList.of(loc)); + ((EntityInternal)entity).addLocationsWithoutPublishing(ImmutableList.of(loc)); } else { LOG.warn("Location not found; discarding location {} of entity {}({})", new Object[] {id, memento.getType(), memento.getId()}); From 67adc68623becc5c7f0a7ed62b9ade1b233e96c7 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Tue, 15 Nov 2016 09:57:56 +0000 Subject: [PATCH 3/3] BROOKLYN-386: adds CreateUserPolicyRebindTest --- .../mgmt/rebind/RebindTestFixtureWithApp.java | 13 +- .../os/CreateUserPolicyRebindTest.java | 115 ++++++++++++++++++ 2 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 locations/jclouds/src/test/java/org/apache/brooklyn/policy/jclouds/os/CreateUserPolicyRebindTest.java diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestFixtureWithApp.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestFixtureWithApp.java index e609a02baf..8c4bae7aa6 100644 --- a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestFixtureWithApp.java +++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindTestFixtureWithApp.java @@ -22,6 +22,7 @@ import org.apache.brooklyn.api.entity.Application; import org.apache.brooklyn.api.entity.EntitySpec; +import org.apache.brooklyn.core.entity.BrooklynConfigKeys; import org.apache.brooklyn.core.test.entity.TestApplication; import org.apache.brooklyn.core.test.entity.TestApplicationNoEnrichersImpl; @@ -31,8 +32,18 @@ public class RebindTestFixtureWithApp extends RebindTestFixture { + /** set null not to set, or a boolean to set explicitly */ + protected Boolean shouldSkipOnBoxBaseDirResolution() { + // TODO Change default to true; starting with this as null for backwards compatibility! + return null; + } + protected TestApplication createApp() { - return origManagementContext.getEntityManager().createEntity(EntitySpec.create(TestApplication.class, TestApplicationNoEnrichersImpl.class)); + EntitySpec spec = EntitySpec.create(TestApplication.class, TestApplicationNoEnrichersImpl.class); + if (shouldSkipOnBoxBaseDirResolution()!=null) { + spec.configure(BrooklynConfigKeys.SKIP_ON_BOX_BASE_DIR_RESOLUTION, shouldSkipOnBoxBaseDirResolution()); + } + return origManagementContext.getEntityManager().createEntity(spec); } @Override diff --git a/locations/jclouds/src/test/java/org/apache/brooklyn/policy/jclouds/os/CreateUserPolicyRebindTest.java b/locations/jclouds/src/test/java/org/apache/brooklyn/policy/jclouds/os/CreateUserPolicyRebindTest.java new file mode 100644 index 0000000000..7b677a149b --- /dev/null +++ b/locations/jclouds/src/test/java/org/apache/brooklyn/policy/jclouds/os/CreateUserPolicyRebindTest.java @@ -0,0 +1,115 @@ +/* + * 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 org.apache.brooklyn.policy.jclouds.os; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.fail; + +import org.apache.brooklyn.api.entity.EntitySpec; +import org.apache.brooklyn.api.location.LocationSpec; +import org.apache.brooklyn.api.policy.PolicySpec; +import org.apache.brooklyn.core.entity.EntityAsserts; +import org.apache.brooklyn.core.mgmt.rebind.RebindTestFixtureWithApp; +import org.apache.brooklyn.core.test.entity.TestEntity; +import org.apache.brooklyn.location.ssh.SshMachineLocation; +import org.apache.brooklyn.test.Asserts; +import org.apache.brooklyn.util.core.internal.ssh.RecordingSshTool; +import org.apache.brooklyn.util.core.internal.ssh.RecordingSshTool.ExecCmd; +import org.apache.brooklyn.util.time.Duration; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; + +public class CreateUserPolicyRebindTest extends RebindTestFixtureWithApp { + + @SuppressWarnings("unused") + private static final Logger LOG = LoggerFactory.getLogger(CreateUserPolicyRebindTest.class); + + @BeforeMethod(alwaysRun=true) + @Override + public void setUp() throws Exception { + super.setUp(); + RecordingSshTool.clear(); + } + + @AfterMethod(alwaysRun=true) + @Override + public void tearDown() throws Exception { + try { + super.tearDown(); + } finally { + RecordingSshTool.clear(); + } + } + + @Override + protected Boolean shouldSkipOnBoxBaseDirResolution() { + return true; + } + + protected boolean isUseraddExecuted() { + for (ExecCmd cmds : RecordingSshTool.getExecCmds()) { + if (cmds.commands.toString().contains("useradd")) { + return true; + } + } + return false; + } + + // See BROOKLYN-386 + @Test + public void testNotCallCreateUserOnRebind() throws Exception { + SshMachineLocation machine = mgmt().getLocationManager().createLocation(LocationSpec.create(SshMachineLocation.class) + .configure(SshMachineLocation.SSH_TOOL_CLASS, RecordingSshTool.class.getName()) + .configure("address", "1.2.3.4")); + + String newUsername = "mynewuser"; + + app().createAndManageChild(EntitySpec.create(TestEntity.class) + .policy(PolicySpec.create(CreateUserPolicy.class) + .configure(CreateUserPolicy.GRANT_SUDO, true) + .configure(CreateUserPolicy.RESET_LOGIN_USER, false) + .configure(CreateUserPolicy.VM_USERNAME, newUsername))); + TestEntity entity = (TestEntity) Iterables.getOnlyElement(app().getChildren()); + app().start(ImmutableList.of(machine)); + + String creds = EntityAsserts.assertAttributeEventuallyNonNull(entity, CreateUserPolicy.VM_USER_CREDENTIALS); + if (!isUseraddExecuted()) { + fail("useradd not found in: "+RecordingSshTool.getExecCmds()); + } + RecordingSshTool.clear(); + + rebind(); + TestEntity newEntity = (TestEntity) Iterables.getOnlyElement(app().getChildren()); + + Asserts.succeedsContinually(ImmutableMap.of("timeout", Duration.millis(250)), new Runnable() { + @Override public void run() { + if (isUseraddExecuted()) { + fail("useradd found in: "+RecordingSshTool.getExecCmds()); + } + }}); + assertEquals(newEntity.sensors().get(CreateUserPolicy.VM_USER_CREDENTIALS), creds); + } +}