From 3563d4794064b2c2eecf516d7fb96c7e1556e478 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 18 Nov 2016 16:07:53 +0000 Subject: [PATCH] BROOKLYN-396: test and fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`). --- .../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(-) create mode 100644 core/src/test/java/org/apache/brooklyn/location/localhost/LocalhostLocationResolverRebindTest.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 99f5a24642..e6033957f6 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 @@ protected void addConfig(RebindContext rebindContext, LocationMemento memento) { // 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 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)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)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); + } } } } 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 327dd96609..53e9a5cd11 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 void testLocationTags() throws Exception { 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 MyLocation() { public MyLocation(Map flags) { super(flags); } + + @Override + public void requestPersist() { + super.requestPersist(); + } } public static class MyLocationReffingOthers extends AbstractLocation { 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 9f3b3a3809..ccf72d3550 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 provisioner = resolve(spec); - + MachineLocation machine1 = provisioner.obtain(ImmutableMap.of()); + machine1.config().set(ConfigKeys.newStringConfigKey("mykey1"), "myval1"); + rebind(); @SuppressWarnings("unchecked") MachineProvisioningLocation newProvisioner = (MachineProvisioningLocation) 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 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 0000000000..43c6c6e539 --- /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 getLocationSpec(String val) { + return (LocationSpec) mgmt().getLocationRegistry().getLocationSpec(val).get(); + } + + private LocalhostMachineProvisioningLocation resolve(String val) { + return (LocalhostMachineProvisioningLocation) mgmt().getLocationRegistry().getLocationManaged(val); + } +} 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 bb7da928a1..83a34de44e 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 MachineLocation call() throws Exception { 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; } }