AMBARI-11399 - Merged Stack Configurations Don't Use Overrides On Upgrade (jonathanhurley)

ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java

@@ -77,7 +77,7 @@ import org.apache.ambari.server.stack.MasterHostResolver;
 import org.apache.ambari.server.state.Cluster;
 import org.apache.ambari.server.state.Config;
 import org.apache.ambari.server.state.ConfigHelper;
-import org.apache.ambari.server.state.PropertyInfo;
+import org.apache.ambari.server.state.DesiredConfig;
 import org.apache.ambari.server.state.ServiceInfo;
 import org.apache.ambari.server.state.StackId;
 import org.apache.ambari.server.state.StackInfo;
@@ -637,7 +637,10 @@ public class UpgradeResourceProvider extends AbstractControllerResourceProvider
    * then this will perform the following:
    * <ul>
    * <li>Upgrade: Create new configurations that are a merge between the current
-   * stack and the desired stack.</li>
+   * stack and the desired stack. If a value has changed between stacks, then
+   * the target stack value should be taken unless the cluster's value differs
+   * from the old stack. This can occur if a property has been customized after
+   * installation.</li>
    * <li>Downgrade: Reset the latest configurations from the cluster's original
    * stack. The new configurations that were created on upgrade must be left
    * intact until all components have been reverted, otherwise heartbeats will
@@ -651,7 +654,8 @@ public class UpgradeResourceProvider extends AbstractControllerResourceProvider
    *          the version
    * @throws AmbariException
    */
-  private void processConfigurations(Cluster cluster, String version, Direction direction) throws AmbariException {
+  void processConfigurations(Cluster cluster, String version, Direction direction)
+      throws AmbariException {
     RepositoryVersionEntity targetRve = s_repoVersionDAO.findMaxByVersion(version);
     if (null == targetRve) {
       LOG.info("Could not find version entity for {}; not setting new configs",
@@ -680,52 +684,74 @@ public class UpgradeResourceProvider extends AbstractControllerResourceProvider
         break;
     }
 
-
-    Map<String, Map<String, String>> clusterConfigs = null;
+    Map<String, Map<String, String>> newConfigurationsByType = null;
     ConfigHelper configHelper = getManagementController().getConfigHelper();
 
     if (direction == Direction.UPGRADE) {
-      clusterConfigs = new HashMap<String, Map<String, String>>();
-
-      // !!! stack
-      Set<org.apache.ambari.server.state.PropertyInfo> pi = s_metaProvider.get().getStackProperties(
-          targetStack.getStackName(), targetStack.getStackVersion());
-
-      for (PropertyInfo stackProperty : pi) {
-        String type = ConfigHelper.fileNameToConfigType(stackProperty.getFilename());
-
-        if (!clusterConfigs.containsKey(type)) {
-          clusterConfigs.put(type, new HashMap<String, String>());
+      // populate a map of default configurations for the old stack (this is
+      // used when determining if a property has been customized and should be
+      // overriden with the new stack value)
+      Map<String, Map<String, String>> oldStackDefaultConfigurationsByType = configHelper.getDefaultProperties(
+          currentStackId, cluster);
+
+      // populate a map with default configurations from the new stack
+      newConfigurationsByType = configHelper.getDefaultProperties(targetStackId, cluster);
+
+      // now that the map has been populated with the default configurations
+      // from the stack/service, overlay the existing configurations on top
+      Map<String, DesiredConfig> existingDesiredConfigurationsByType = cluster.getDesiredConfigs();
+      for (Map.Entry<String, DesiredConfig> existingEntry : existingDesiredConfigurationsByType.entrySet()) {
+        String configurationType = existingEntry.getKey();
+
+        // NPE sanity, althought shouldn't even happen since we are iterating
+        // over the desired configs to start with
+        Config currentClusterConfig = cluster.getDesiredConfigByType(configurationType);
+        if (null == currentClusterConfig) {
+          continue;
         }
 
-        clusterConfigs.get(type).put(stackProperty.getName(),
-            stackProperty.getValue());
-      }
-
-      // !!! by service
-      for (String serviceName : cluster.getServices().keySet()) {
-        pi = s_metaProvider.get().getServiceProperties(
-            targetStack.getStackName(), targetStack.getStackVersion(),
-            serviceName);
+        // get the existing configurations
+        Map<String,String> existingConfigurations = currentClusterConfig.getProperties();
 
-        // !!! use new stack as the basis
-        for (PropertyInfo stackProperty : pi) {
-          String type = ConfigHelper.fileNameToConfigType(stackProperty.getFilename());
-
-          if (!clusterConfigs.containsKey(type)) {
-            clusterConfigs.put(type, new HashMap<String, String>());
-          }
-
-          clusterConfigs.get(type).put(stackProperty.getName(),
-              stackProperty.getValue());
+        // if the new stack configurations don't have the type, then simple add
+        // all of the existing in
+        Map<String, String> newDefaultConfigurations = newConfigurationsByType.get(configurationType);
+        if (null == newDefaultConfigurations) {
+          newConfigurationsByType.put(configurationType, existingConfigurations);
+          continue;
         }
-      }
 
-      // !!! overlay the currently defined values per type
-      for (Map.Entry<String, Map<String, String>> entry : clusterConfigs.entrySet()) {
-        Config config = cluster.getDesiredConfigByType(entry.getKey());
-        if (null != config) {
-          entry.getValue().putAll(config.getProperties());
+        // for every existing configuration, see if an entry exists; if it does
+        // not exist, then put it in the map, otherwise we'll have to compare
+        // the existing value to the original stack value to see if its been
+        // customized
+        for (Map.Entry<String, String> existingConfigurationEntry : existingConfigurations.entrySet()) {
+          String existingConfigurationKey = existingConfigurationEntry.getKey();
+          String existingConfigurationValue = existingConfigurationEntry.getValue();
+
+          // if there is already an entry, we now have to try to determine if
+          // the value was customized after stack installation
+          if (newDefaultConfigurations.containsKey(existingConfigurationKey)) {
+            String newDefaultConfigurationValue = newDefaultConfigurations.get(existingConfigurationKey);
+            if (!StringUtils.equals(existingConfigurationValue, newDefaultConfigurationValue)) {
+              // the new default is different from the existing cluster value;
+              // only override the default value if the existing value differs
+              // from the original stack
+              Map<String, String> configurationTypeDefaultConfigurations = oldStackDefaultConfigurationsByType.get(configurationType);
+              if (null != configurationTypeDefaultConfigurations) {
+                String oldDefaultValue = configurationTypeDefaultConfigurations.get(existingConfigurationKey);
+                if (!StringUtils.equals(existingConfigurationValue, oldDefaultValue)) {
+                  // at this point, we've determined that there is a difference
+                  // between default values between stacks, but the value was
+                  // also customized, so keep the customized value
+                  newDefaultConfigurations.put(existingConfigurationKey, existingConfigurationValue);
+                }
+              }
+            }
+          } else {
+            // there is no entry in the map, so add the existing key/value pair
+            newDefaultConfigurations.put(existingConfigurationKey, existingConfigurationValue);
+          }
         }
       }
     } else {
@@ -738,9 +764,9 @@ public class UpgradeResourceProvider extends AbstractControllerResourceProvider
         targetStack.getStackVersion()), true);
 
     // !!! configs must be created after setting the stack version
-    if (null != clusterConfigs) {
-      configHelper.createConfigTypes(cluster, getManagementController(),
-          clusterConfigs, getManagementController().getAuthName(),
+    if (null != newConfigurationsByType) {
+      configHelper.createConfigTypes(cluster, getManagementController(), newConfigurationsByType,
+          getManagementController().getAuthName(),
           "Configuration created for Upgrade");
     }
   }

ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java

@@ -867,6 +867,59 @@ public class ConfigHelper {
     }
   }
 
+  /**
+   * Gets the default properties from the specified stack and services when a
+   * cluster is first installed.
+   *
+   * @param stack
+   *          the stack to pull stack-values from (not {@code null})
+   * @param cluster
+   *          the cluster to use when determining which services default
+   *          configurations to include (not {@code null}).
+   * @return a mapping of configuration type to map of key/value pairs for the
+   *         default configurations.
+   * @throws AmbariException
+   */
+  public Map<String, Map<String, String>> getDefaultProperties(StackId stack, Cluster cluster)
+      throws AmbariException {
+    Map<String, Map<String, String>> defaultPropertiesByType = new HashMap<String, Map<String, String>>();
+
+    // populate the stack (non-service related) properties first
+    Set<org.apache.ambari.server.state.PropertyInfo> stackConfigurationProperties = ambariMetaInfo.getStackProperties(
+        stack.getStackName(), stack.getStackVersion());
+
+    for (PropertyInfo stackDefaultProperty : stackConfigurationProperties) {
+      String type = ConfigHelper.fileNameToConfigType(stackDefaultProperty.getFilename());
+
+      if (!defaultPropertiesByType.containsKey(type)) {
+        defaultPropertiesByType.put(type, new HashMap<String, String>());
+      }
+
+      defaultPropertiesByType.get(type).put(stackDefaultProperty.getName(),
+          stackDefaultProperty.getValue());
+    }
+
+    // for every installed service, populate the default service properties
+    for (String serviceName : cluster.getServices().keySet()) {
+      Set<org.apache.ambari.server.state.PropertyInfo> serviceConfigurationProperties = ambariMetaInfo.getServiceProperties(
+          stack.getStackName(), stack.getStackVersion(), serviceName);
+
+      // !!! use new stack as the basis
+      for (PropertyInfo serviceDefaultProperty : serviceConfigurationProperties) {
+        String type = ConfigHelper.fileNameToConfigType(serviceDefaultProperty.getFilename());
+
+        if (!defaultPropertiesByType.containsKey(type)) {
+          defaultPropertiesByType.put(type, new HashMap<String, String>());
+        }
+
+        defaultPropertiesByType.get(type).put(serviceDefaultProperty.getName(),
+            serviceDefaultProperty.getValue());
+      }
+    }
+
+    return defaultPropertiesByType;
+  }
+
   private boolean calculateIsStaleConfigs(ServiceComponentHost sch) throws AmbariException {
 
     if (sch.isRestartRequired()) {

ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeResourceProviderTest.java

@@ -68,6 +68,7 @@ import org.apache.ambari.server.state.Clusters;
 import org.apache.ambari.server.state.Config;
 import org.apache.ambari.server.state.ConfigHelper;
 import org.apache.ambari.server.state.ConfigImpl;
+import org.apache.ambari.server.state.DesiredConfig;
 import org.apache.ambari.server.state.Host;
 import org.apache.ambari.server.state.HostState;
 import org.apache.ambari.server.state.RepositoryVersionState;
@@ -79,6 +80,7 @@ import org.apache.ambari.server.state.stack.upgrade.Direction;
 import org.apache.ambari.server.topology.TopologyManager;
 import org.apache.ambari.server.utils.StageUtils;
 import org.apache.ambari.server.view.ViewRegistry;
+import org.easymock.Capture;
 import org.easymock.EasyMock;
 import org.junit.After;
 import org.junit.Assert;
@@ -111,11 +113,18 @@ public class UpgradeResourceProviderTest {
   public void before() throws Exception {
     // setup the config helper for placeholder resolution
     configHelper = EasyMock.createNiceMock(ConfigHelper.class);
+
     expect(
         configHelper.getPlaceholderValueFromDesiredConfigurations(
             EasyMock.anyObject(Cluster.class), EasyMock.eq("{{foo/bar}}"))).andReturn(
         "placeholder-rendered-properly").anyTimes();
 
+    expect(
+        configHelper.getDefaultProperties(EasyMock.anyObject(StackId.class),
+            EasyMock.anyObject(Cluster.class))).andReturn(
+        new HashMap<String, Map<String, String>>()).anyTimes();
+
+
     EasyMock.replay(configHelper);
 
     // create an injector which will inject the mocks
@@ -692,9 +701,109 @@ public class UpgradeResourceProviderTest {
         }
       }
     }
+  }
+
+  /**
+   * Tests merging configurations between existing and new stack values on
+   * upgrade.
+   *
+   * @throws Exception
+   */
+  @Test
+  public void testMergeConfigurations() throws Exception {
+    StackId stack211 = new StackId("HDP-2.1.1");
+    StackId stack220 = new StackId("HDP-2.2.0");
+
+    Map<String, Map<String, String>> stack211Configs = new HashMap<String, Map<String, String>>();
+    Map<String, String> stack211FooType = new HashMap<String, String>();
+    Map<String, String> stack211BarType = new HashMap<String, String>();
+    Map<String, String> stack211BazType = new HashMap<String, String>();
+    stack211Configs.put("foo-site", stack211FooType);
+    stack211Configs.put("bar-site", stack211BarType);
+    stack211Configs.put("baz-site", stack211BazType);
+    stack211FooType.put("1", "one");
+    stack211FooType.put("11", "one-one");
+    stack211BarType.put("2", "two");
+    stack211BazType.put("3", "three");
+
+    Map<String, Map<String, String>> stack220Configs = new HashMap<String, Map<String, String>>();
+    Map<String, String> stack220FooType = new HashMap<String, String>();
+    Map<String, String> stack220BazType = new HashMap<String, String>();
+    stack220Configs.put("foo-site", stack220FooType);
+    stack220Configs.put("baz-site", stack220BazType);
+    stack220FooType.put("1", "one-new");
+    stack220FooType.put("111", "one-one-one");
+    stack220BazType.put("3", "three-new");
+
+    Map<String, String> clusterFooType = new HashMap<String, String>();
+    Map<String, String> clusterBarType = new HashMap<String, String>();
+    Map<String, String> clusterBazType = new HashMap<String, String>();
+
+    Config fooConfig = EasyMock.createNiceMock(Config.class);
+    Config barConfig = EasyMock.createNiceMock(Config.class);
+    Config bazConfig = EasyMock.createNiceMock(Config.class);
+
+    clusterFooType.put("1", "one");
+    clusterFooType.put("11", "one-one");
+    clusterBarType.put("2", "two");
+    clusterBazType.put("3", "three-changed");
+
+    expect(fooConfig.getProperties()).andReturn(clusterFooType);
+    expect(barConfig.getProperties()).andReturn(clusterBarType);
+    expect(bazConfig.getProperties()).andReturn(clusterBazType);
+
+    Map<String, DesiredConfig> desiredConfigurations = new HashMap<String, DesiredConfig>();
+    desiredConfigurations.put("foo-site", null);
+    desiredConfigurations.put("bar-site", null);
+    desiredConfigurations.put("baz-site", null);
+
+    Cluster cluster = EasyMock.createNiceMock(Cluster.class);
+    expect(cluster.getCurrentStackVersion()).andReturn(stack211);
+    expect(cluster.getDesiredStackVersion()).andReturn(stack220);
+    expect(cluster.getDesiredConfigs()).andReturn(desiredConfigurations);
+    expect(cluster.getDesiredConfigByType("foo-site")).andReturn(fooConfig);
+    expect(cluster.getDesiredConfigByType("bar-site")).andReturn(barConfig);
+    expect(cluster.getDesiredConfigByType("baz-site")).andReturn(bazConfig);
+
+    // setup the config helper for placeholder resolution
+    EasyMock.reset(configHelper);
 
+    expect(
+        configHelper.getDefaultProperties(EasyMock.eq(stack211), EasyMock.anyObject(Cluster.class))).andReturn(
+        stack211Configs).anyTimes();
+
+    expect(
+        configHelper.getDefaultProperties(EasyMock.eq(stack220), EasyMock.anyObject(Cluster.class))).andReturn(
+        stack220Configs).anyTimes();
+
+    Capture<Map<String, Map<String, String>>> expectedConfigurationsCapture = new Capture<Map<String, Map<String, String>>>();
+
+    configHelper.createConfigTypes(EasyMock.anyObject(Cluster.class),
+        EasyMock.anyObject(AmbariManagementController.class),
+        EasyMock.capture(expectedConfigurationsCapture),
+        EasyMock.anyObject(String.class), EasyMock.anyObject(String.class));
+
+    EasyMock.expectLastCall().once();
 
+    EasyMock.replay(configHelper, cluster, fooConfig, barConfig, bazConfig);
 
+    UpgradeResourceProvider upgradeResourceProvider = createProvider(amc);
+
+    upgradeResourceProvider.processConfigurations(cluster, "2.2.0.0", Direction.UPGRADE);
+
+    Map<String, Map<String, String>> expectedConfigurations = expectedConfigurationsCapture.getValue();
+    Map<String, String> expectedFooType = expectedConfigurations.get("foo-site");
+    Map<String, String> expectedBarType = expectedConfigurations.get("bar-site");
+    Map<String, String> expectedBazType = expectedConfigurations.get("baz-site");
+
+    // the really important values are one-new and three-changed; one-new
+    // indicates that the new stack value is changed since it was not customized
+    // while three-changed represents that the customized value was preserved
+    // even though the stack value changed
+    assertEquals("one-new", expectedFooType.get("1"));
+    assertEquals("one-one", expectedFooType.get("11"));
+    assertEquals("two", expectedBarType.get("2"));
+    assertEquals("three-changed", expectedBazType.get("3"));
   }
 
   /**
@@ -717,7 +826,4 @@ public class UpgradeResourceProviderTest {
       binder.bind(ConfigHelper.class).toInstance(configHelper);
     }
   }
-
-
-
 }

ambari-server/src/test/java/org/apache/ambari/server/state/ConfigHelperTest.java

@@ -377,7 +377,7 @@ public class ConfigHelperTest {
     }
 
     @Test
-    public void testCloneAttributesMap_sourceIsNull() throws Exception {
+    public void testCloneAttributesMapSourceIsNull() throws Exception {
       // init
       Map<String, Map<String, String>> targetAttributesMap = new HashMap<String, Map<String, String>>();
       Map<String, String> attributesValues = new HashMap<String, String>();
@@ -405,7 +405,7 @@ public class ConfigHelperTest {
     }
 
     @Test
-    public void testCloneAttributesMap_targetIsNull() throws Exception {
+    public void testCloneAttributesMapTargetIsNull() throws Exception {
       // init
       Map<String, Map<String, String>> targetAttributesMap = null;
       Map<String, Map<String, String>> sourceAttributesMap = new HashMap<String, Map<String, String>>();
@@ -469,7 +469,7 @@ public class ConfigHelperTest {
     }
 
     @Test
-    public void testMergeAttributes_noAttributeOverrides() throws Exception {
+    public void testMergeAttributesWithNoAttributeOverrides() throws Exception {
       Map<String, Map<String, String>> persistedAttributes = new HashMap<String, Map<String, String>>();
       Map<String, String> persistedFinalAttrs = new HashMap<String, String>();
       persistedFinalAttrs.put("a", "true");
@@ -496,7 +496,7 @@ public class ConfigHelperTest {
     }
 
     @Test
-    public void testMergeAttributes_nullAttributes() throws Exception {
+    public void testMergeAttributesWithNullAttributes() throws Exception {
       Map<String, Map<String, String>> persistedAttributes = new HashMap<String, Map<String, String>>();
       Map<String, String> persistedFinalAttrs = new HashMap<String, String>();
       persistedFinalAttrs.put("a", "true");
@@ -524,7 +524,7 @@ public class ConfigHelperTest {
     }
 
     @Test
-    public void testMergeAttributes_nullProperties() throws Exception {
+    public void testMergeAttributesWithNullProperties() throws Exception {
       Map<String, Map<String, String>> persistedAttributes = new HashMap<String, Map<String, String>>();
       Map<String, String> persistedFinalAttrs = new HashMap<String, String>();
       persistedFinalAttrs.put("a", "true");
@@ -620,7 +620,7 @@ public class ConfigHelperTest {
     }
 
     @Test
-    public void testGetServiceProperties_SimpleInvocation() throws Exception {
+    public void testGetServicePropertiesSimpleInvocation() throws Exception {
       Cluster mockCluster = createStrictMock(Cluster.class);
       StackId mockStackVersion = createStrictMock(StackId.class);
       AmbariMetaInfo mockAmbariMetaInfo = injector.getInstance(AmbariMetaInfo.class);
@@ -654,7 +654,7 @@ public class ConfigHelperTest {
     }
 
     @Test
-    public void testGetServiceProperties_DoNoRemoveExcluded() throws Exception {
+    public void testGetServicePropertiesDoNoRemoveExcluded() throws Exception {
       StackId mockStackVersion = createStrictMock(StackId.class);
       AmbariMetaInfo mockAmbariMetaInfo = injector.getInstance(AmbariMetaInfo.class);
       ServiceInfo mockServiceInfo = createStrictMock(ServiceInfo.class);
@@ -685,7 +685,7 @@ public class ConfigHelperTest {
     }
 
     @Test
-    public void testGetServiceProperties_RemoveExcluded() throws Exception {
+    public void testGetServicePropertiesRemoveExcluded() throws Exception {
       StackId mockStackVersion = createStrictMock(StackId.class);
       AmbariMetaInfo mockAmbariMetaInfo = injector.getInstance(AmbariMetaInfo.class);
       ServiceInfo mockServiceInfo = createStrictMock(ServiceInfo.class);