Repository: brooklyn-server Updated Branches: refs/heads/master a569463dd -> d7eb99e10
BROOKLYN-396: test and fix When rebinding a location, if a config flag corresponds to a field with â@SetFromFlagâ then just set the field, and donât leave the key-value inside âconfig().getBag()â. Some classes use fields to store internal state, which should not be passed to child locations (e.g. `LocalhostMachineProvisioningLocation.origConfigs`). Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/3563d479 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/3563d479 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/3563d479 Branch: refs/heads/master Commit: 3563d4794064b2c2eecf516d7fb96c7e1556e478 Parents: a569463 Author: Aled Sage <aled.s...@gmail.com> Authored: Fri Nov 18 16:07:53 2016 +0000 Committer: Aled Sage <aled.s...@gmail.com> Committed: Fri Nov 18 16:07:53 2016 +0000 ---------------------------------------------------------------------- .../mgmt/rebind/BasicLocationRebindSupport.java | 52 ++++++---- .../core/mgmt/rebind/RebindLocationTest.java | 24 +++++ .../byon/ByonLocationResolverRebindTest.java | 31 +++++- .../LocalhostLocationResolverRebindTest.java | 101 +++++++++++++++++++ .../MachineLifecycleEffectorTasks.java | 3 +- 5 files changed, 183 insertions(+), 28 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3563d479/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicLocationRebindSupport.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicLocationRebindSupport.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicLocationRebindSupport.java index 99f5a24..e603395 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicLocationRebindSupport.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicLocationRebindSupport.java @@ -70,33 +70,41 @@ public class BasicLocationRebindSupport extends AbstractBrooklynObjectRebindSupp // Note that the flags have been set in the constructor // Sept 2016 - now ignores unused and config description - location.config().putAll(memento.getLocationConfig()); - for (Map.Entry<String, Object> entry : memento.getLocationConfig().entrySet()) { String flagName = entry.getKey(); + Object value = entry.getValue(); + + Field field; try { - Field field = FlagUtils.findFieldForFlag(flagName, location); - Class<?> fieldType = field.getType(); - Object value = entry.getValue(); - - // Field is either of type ConfigKey, or it's a vanilla java field annotated with @SetFromFlag. - // If the former, need to look up the field value (i.e. the ConfigKey) to find out the type. - // If the latter, just want to look at the field itself to get the type. - // Then coerce the value we have to that type. - // And use magic of setFieldFromFlag's magic to either set config or field as appropriate. - if (ConfigKey.class.isAssignableFrom(fieldType)) { - ConfigKey<?> configKey = (ConfigKey<?>) FlagUtils.getField(location, field); - location.config().set((ConfigKey<Object>)configKey, entry.getValue()); - } else { - value = TypeCoercions.coerce(entry.getValue(), fieldType); - if (value != null) { - location.config().putAll(MutableMap.of(flagName, value)); - FlagUtils.setFieldFromFlag(location, flagName, value); - } - } - + field = FlagUtils.findFieldForFlag(flagName, location); } catch (NoSuchElementException e) { // FIXME How to do findFieldForFlag without throwing exception if it's not there? + field = null; + } + if (field == null) { + // This is anonymous config: just add it using the string key + location.config().putAll(MutableMap.of(flagName, value)); + continue; + } + + Class<?> fieldType = field.getType(); + + // Field is either of type ConfigKey, or it's a vanilla java field annotated with @SetFromFlag. + // If the former, need to look up the field value (i.e. the ConfigKey) to find out the type. + // If the latter, just want to look at the field itself to get the type. + // Then coerce the value we have to that type. + // And use magic of setFieldFromFlag's magic to either set config or field as appropriate. + if (ConfigKey.class.isAssignableFrom(fieldType)) { + ConfigKey<?> configKey = (ConfigKey<?>) FlagUtils.getField(location, field); + location.config().set((ConfigKey<Object>)configKey, entry.getValue()); + } else { + // Fields annotated with `@SetFromFlag` are very "special" - don't treat them + // like normal config (because that's not how they are normally treated before + // rebind - see https://issues.apache.org/jira/browse/BROOKLYN-396 + value = TypeCoercions.coerce(entry.getValue(), fieldType); + if (value != null) { + FlagUtils.setFieldFromFlag(location, flagName, value); + } } } } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3563d479/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindLocationTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindLocationTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindLocationTest.java index 327dd96..53e9a5c 100644 --- a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindLocationTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindLocationTest.java @@ -270,6 +270,25 @@ public class RebindLocationTest extends RebindTestFixtureWithApp { Asserts.assertEqualsIgnoringOrder(newLoc.tags().getTags(), ImmutableSet.of("foo", newApp)); } + // See https://issues.apache.org/jira/browse/BROOKLYN-396 + @Test + public void testFlagFieldsNotReturnedInConfig() throws Exception { + MyLocation origLoc = mgmt().getLocationManager().createLocation(LocationSpec.create(MyLocation.class)); + origLoc.myfield = "myval"; + origLoc.requestPersist(); + + // Check (before rebind) that the 'myfield' isn't also in the config + assertNull(origLoc.config().getBag().getStringKey("myfield")); + + // Check after rebind that we are the same: 'myfield' isn't also in the config + rebind(); + MyLocation newLoc = (MyLocation) mgmt().getLocationManager().getLocation(origLoc.getId()); + + assertEquals(newLoc.myfield, "myval"); + assertNull(newLoc.config().getBag().getStringKey("myfield")); + } + + public static class LocationChecksIsRebinding extends AbstractLocation { boolean isRebindingValWhenRebinding; @@ -328,6 +347,11 @@ public class RebindLocationTest extends RebindTestFixtureWithApp { public MyLocation(Map<?,?> flags) { super(flags); } + + @Override + public void requestPersist() { + super.requestPersist(); + } } public static class MyLocationReffingOthers extends AbstractLocation { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3563d479/core/src/test/java/org/apache/brooklyn/location/byon/ByonLocationResolverRebindTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/location/byon/ByonLocationResolverRebindTest.java b/core/src/test/java/org/apache/brooklyn/location/byon/ByonLocationResolverRebindTest.java index 9f3b3a3..ccf72d3 100644 --- a/core/src/test/java/org/apache/brooklyn/location/byon/ByonLocationResolverRebindTest.java +++ b/core/src/test/java/org/apache/brooklyn/location/byon/ByonLocationResolverRebindTest.java @@ -19,11 +19,13 @@ package org.apache.brooklyn.location.byon; import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertTrue; +import static org.testng.Assert.assertNull; import org.apache.brooklyn.api.location.LocationSpec; import org.apache.brooklyn.api.location.MachineLocation; import org.apache.brooklyn.api.location.MachineProvisioningLocation; +import org.apache.brooklyn.core.config.ConfigKeys; +import org.apache.brooklyn.core.location.internal.LocationInternal; import org.apache.brooklyn.core.mgmt.rebind.RebindTestFixtureWithApp; import org.apache.brooklyn.location.ssh.SshMachineLocation; import org.testng.annotations.Test; @@ -34,15 +36,34 @@ public class ByonLocationResolverRebindTest extends RebindTestFixtureWithApp { @Test public void testRebindByon() throws Exception { - String spec = "byon(hosts=\"1.1.1.1\")"; + String spec = "byon(hosts=\"1.1.1.1,1.1.1.2\")"; MachineProvisioningLocation<MachineLocation> provisioner = resolve(spec); - + MachineLocation machine1 = provisioner.obtain(ImmutableMap.of()); + machine1.config().set(ConfigKeys.newStringConfigKey("mykey1"), "myval1"); + rebind(); @SuppressWarnings("unchecked") MachineProvisioningLocation<MachineLocation> newProvisioner = (MachineProvisioningLocation<MachineLocation>) mgmt().getLocationManager().getLocation(provisioner.getId()); - MachineLocation newLocation = newProvisioner.obtain(ImmutableMap.of()); - assertTrue(newLocation instanceof SshMachineLocation, "Expected location to be SshMachineLocation, found " + newLocation); + + SshMachineLocation newMachine1 = (SshMachineLocation) mgmt().getLocationManager().getLocation(machine1.getId()); + assertEquals(newMachine1.config().get(ConfigKeys.newStringConfigKey("mykey1")), "myval1"); + + SshMachineLocation newMachine2 = (SshMachineLocation) newProvisioner.obtain(ImmutableMap.of()); + + // See https://issues.apache.org/jira/browse/BROOKLYN-396 (though didn't fail for byon, only localhost) + ((LocationInternal)newProvisioner).config().getLocalBag().toString(); + ((LocationInternal)newMachine1).config().getLocalBag().toString(); + ((LocationInternal)newMachine2).config().getLocalBag().toString(); + + // Confirm when machine is released that we get it back (without the modifications to config) + newMachine1.config().set(ConfigKeys.newStringConfigKey("mykey2"), "myval2"); + newProvisioner.release(newMachine1); + + MachineLocation newMachine1b = newProvisioner.obtain(ImmutableMap.of()); + assertEquals(newMachine1b.getAddress(), machine1.getAddress()); + assertNull(newMachine1b.config().get(ConfigKeys.newStringConfigKey("mykey1"))); + assertNull(newMachine1b.config().get(ConfigKeys.newStringConfigKey("mykey2"))); } @Test http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3563d479/core/src/test/java/org/apache/brooklyn/location/localhost/LocalhostLocationResolverRebindTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/location/localhost/LocalhostLocationResolverRebindTest.java b/core/src/test/java/org/apache/brooklyn/location/localhost/LocalhostLocationResolverRebindTest.java new file mode 100644 index 0000000..43c6c6e --- /dev/null +++ b/core/src/test/java/org/apache/brooklyn/location/localhost/LocalhostLocationResolverRebindTest.java @@ -0,0 +1,101 @@ +/* + * 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.location.localhost; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNull; +import static org.testng.Assert.assertTrue; + +import org.apache.brooklyn.api.location.LocationSpec; +import org.apache.brooklyn.api.location.MachineLocation; +import org.apache.brooklyn.core.config.ConfigKeys; +import org.apache.brooklyn.core.location.internal.LocationInternal; +import org.apache.brooklyn.core.mgmt.rebind.RebindTestFixtureWithApp; +import org.apache.brooklyn.location.ssh.SshMachineLocation; +import org.testng.annotations.Test; + +import com.google.common.collect.ImmutableMap; + +public class LocalhostLocationResolverRebindTest extends RebindTestFixtureWithApp { + + // Test is motivated by https://issues.apache.org/jira/browse/BROOKLYN-396, to + // compare behaviour with and without rebind. + @Test + public void testWithoutRebind() throws Exception { + runTest(false); + } + + @Test + public void testRebind() throws Exception { + runTest(true); + } + + protected void runTest(boolean doRebind) throws Exception { + LocalhostMachineProvisioningLocation provisioner = resolve("localhost"); + MachineLocation machine1 = provisioner.obtain(ImmutableMap.of()); + machine1.config().set(ConfigKeys.newStringConfigKey("mykey1"), "myval1"); + + if (doRebind) { + rebind(); + } + + LocalhostMachineProvisioningLocation newProvisioner = (LocalhostMachineProvisioningLocation) mgmt().getLocationManager().getLocation(provisioner.getId()); + + SshMachineLocation newMachine1 = (SshMachineLocation) mgmt().getLocationManager().getLocation(machine1.getId()); + assertEquals(newMachine1.config().get(ConfigKeys.newStringConfigKey("mykey1")), "myval1"); + + SshMachineLocation newMachine2 = newProvisioner.obtain(ImmutableMap.of()); + + // See https://issues.apache.org/jira/browse/BROOKLYN-396 + ((LocationInternal)newProvisioner).config().getLocalBag().toString(); + ((LocationInternal)newMachine1).config().getLocalBag().toString(); + ((LocationInternal)newMachine2).config().getLocalBag().toString(); + + // Release a machine, and get a new one. + // With a normal BYON, it would give us the same machine. + // For localhost, we don't care if it's the same or different - as long as it doesn't + // have the specific config set on the original MachineLocation + newMachine1.config().set(ConfigKeys.newStringConfigKey("mykey2"), "myval2"); + newProvisioner.release(newMachine1); + + MachineLocation newMachine1b = newProvisioner.obtain(ImmutableMap.of()); + assertNull(newMachine1b.config().get(ConfigKeys.newStringConfigKey("mykey1"))); + assertNull(newMachine1b.config().get(ConfigKeys.newStringConfigKey("mykey2"))); + } + + @Test + public void testRebindWhenOnlyByonLocationSpec() throws Exception { + int before = mgmt().getLocationManager().getLocations().size(); + getLocationSpec("localhost"); + + rebind(); + + int after = mgmt().getLocationManager().getLocations().size(); + assertEquals(after, before); + } + + @SuppressWarnings("unchecked") + private LocationSpec<LocalhostMachineProvisioningLocation> getLocationSpec(String val) { + return (LocationSpec<LocalhostMachineProvisioningLocation>) mgmt().getLocationRegistry().getLocationSpec(val).get(); + } + + private LocalhostMachineProvisioningLocation resolve(String val) { + return (LocalhostMachineProvisioningLocation) mgmt().getLocationRegistry().getLocationManaged(val); + } +} http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3563d479/software/base/src/main/java/org/apache/brooklyn/entity/software/base/lifecycle/MachineLifecycleEffectorTasks.java ---------------------------------------------------------------------- diff --git a/software/base/src/main/java/org/apache/brooklyn/entity/software/base/lifecycle/MachineLifecycleEffectorTasks.java b/software/base/src/main/java/org/apache/brooklyn/entity/software/base/lifecycle/MachineLifecycleEffectorTasks.java index bb7da92..83a34de 100644 --- a/software/base/src/main/java/org/apache/brooklyn/entity/software/base/lifecycle/MachineLifecycleEffectorTasks.java +++ b/software/base/src/main/java/org/apache/brooklyn/entity/software/base/lifecycle/MachineLifecycleEffectorTasks.java @@ -420,11 +420,12 @@ public abstract class MachineLifecycleEffectorTasks { if (machine == null) { throw new NoMachinesAvailableException("Failed to obtain machine in " + location.toString()); } - if (log.isDebugEnabled()) + if (log.isDebugEnabled()) { log.debug("While starting {}, obtained new location instance {}", entity(), (machine instanceof SshMachineLocation ? machine + ", details " + ((SshMachineLocation) machine).getUser() + ":" + Sanitizer.sanitize(((SshMachineLocation) machine).config().getLocalBag()) : machine)); + } return machine; } }