From: Maros Marsalek Date: Mon, 18 May 2015 14:11:57 +0000 (+0200) Subject: BUG-2976: Resolve clash between current and feature cfg pusher X-Git-Tag: release/beryllium~542 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=6da2893d4a3a939a1f5d4079f6ffe04a305754e6 BUG-2976: Resolve clash between current and feature cfg pusher Current config pusher now stores list of features present in karaf with the config snapshot itself. Feature pusher now looks at the list and ignores features listed in current config in the initial push. A WARN log message is emitted for each ignored config file + feature. Change-Id: I6e71e692e56d5219cf02cd704dd78215f4a7f5a2 Signed-off-by: Maros Marsalek (cherry picked from commit fa8b972c6da7e3a24af396ea4bebe6d06f6a8d89) --- diff --git a/opendaylight/config/config-persister-feature-adapter/src/main/java/org/opendaylight/controller/configpusherfeature/internal/FeatureConfigPusher.java b/opendaylight/config/config-persister-feature-adapter/src/main/java/org/opendaylight/controller/configpusherfeature/internal/FeatureConfigPusher.java index 2d3a011469..2e24fa9013 100644 --- a/opendaylight/config/config-persister-feature-adapter/src/main/java/org/opendaylight/controller/configpusherfeature/internal/FeatureConfigPusher.java +++ b/opendaylight/config/config-persister-feature-adapter/src/main/java/org/opendaylight/controller/configpusherfeature/internal/FeatureConfigPusher.java @@ -7,6 +7,7 @@ */ package org.opendaylight.controller.configpusherfeature.internal; +import com.google.common.base.Optional; import com.google.common.collect.LinkedHashMultimap; import java.util.ArrayList; import java.util.Arrays; @@ -16,6 +17,7 @@ import org.apache.karaf.features.Feature; import org.apache.karaf.features.FeaturesService; import org.opendaylight.controller.config.persist.api.ConfigPusher; import org.opendaylight.controller.config.persist.api.ConfigSnapshotHolder; +import org.opendaylight.controller.config.persist.storage.file.xml.XmlFileStorageAdapter; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -57,19 +59,21 @@ public class FeatureConfigPusher { * If a Feature is not in the returned LinkedHashMultimap then we couldn't push its configs * (Ususally because it was not yet installed) */ - public LinkedHashMultimap pushConfigs(final List features) throws Exception, InterruptedException { - LinkedHashMultimap pushedFeatures = LinkedHashMultimap.create(); - for(Feature feature: features) { + public LinkedHashMultimap pushConfigs(final List features) throws Exception { + LinkedHashMultimap pushedFeatures = LinkedHashMultimap.create(); + for (Feature feature : features) { + + LinkedHashSet configSnapShots = pushConfig(feature); - if(!configSnapShots.isEmpty()) { - pushedFeatures.putAll(feature,configSnapShots); + if (!configSnapShots.isEmpty()) { + pushedFeatures.putAll(feature, configSnapShots); } } return pushedFeatures; } - private LinkedHashSet pushConfig(final Feature feature) throws Exception, InterruptedException { - LinkedHashSet configs = new LinkedHashSet(); + private LinkedHashSet pushConfig(final Feature feature) throws Exception { + LinkedHashSet configs = new LinkedHashSet<>(); if(isInstalled(feature)) { // FIXME Workaround for BUG-2836, features service returns null for feature: standard-condition-webconsole_0_0_0, 3.0.1 if(featuresService.getFeature(feature.getName(), feature.getVersion()) == null) { @@ -78,7 +82,7 @@ public class FeatureConfigPusher { ChildAwareFeatureWrapper wrappedFeature = new ChildAwareFeatureWrapper(feature, featuresService); configs = wrappedFeature.getFeatureConfigSnapshotHolders(); if (!configs.isEmpty()) { - configs = pushConfig(configs); + configs = pushConfig(configs, feature); feature2configs.putAll(feature, configs); } } @@ -113,11 +117,21 @@ public class FeatureConfigPusher { return false; } - private LinkedHashSet pushConfig(final LinkedHashSet configs) throws InterruptedException { + private LinkedHashSet pushConfig(final LinkedHashSet configs, final Feature feature) throws InterruptedException { LinkedHashSet configsToPush = new LinkedHashSet(configs); configsToPush.removeAll(pushedConfigs); - if(!configsToPush.isEmpty()) { - pusher.pushConfigs(new ArrayList(configsToPush)); + if (!configsToPush.isEmpty()) { + + // Ignore features that are present in persisted current config + final Optional currentCfgPusher = XmlFileStorageAdapter.getInstance(); + if (currentCfgPusher.isPresent() && + currentCfgPusher.get().getPersistedFeatures().contains(feature.getId())) { + LOG.warn("Ignoring default configuration {} for feature {}, the configuration is present in {}", + configsToPush, feature.getId(), currentCfgPusher.get()); + } else { + pusher.pushConfigs(new ArrayList(configsToPush)); + } + pushedConfigs.addAll(configsToPush); } LinkedHashSet configsPushed = new LinkedHashSet(pushedConfigs); diff --git a/opendaylight/config/config-persister-feature-adapter/src/main/java/org/opendaylight/controller/configpusherfeature/internal/FeatureServiceCustomizer.java b/opendaylight/config/config-persister-feature-adapter/src/main/java/org/opendaylight/controller/configpusherfeature/internal/FeatureServiceCustomizer.java index e72c8278e5..c941e89435 100644 --- a/opendaylight/config/config-persister-feature-adapter/src/main/java/org/opendaylight/controller/configpusherfeature/internal/FeatureServiceCustomizer.java +++ b/opendaylight/config/config-persister-feature-adapter/src/main/java/org/opendaylight/controller/configpusherfeature/internal/FeatureServiceCustomizer.java @@ -7,9 +7,15 @@ */ package org.opendaylight.controller.configpusherfeature.internal; +import com.google.common.base.Optional; +import com.google.common.collect.Sets; +import java.util.Set; +import org.apache.karaf.features.Feature; import org.apache.karaf.features.FeaturesListener; import org.apache.karaf.features.FeaturesService; import org.opendaylight.controller.config.persist.api.ConfigPusher; +import org.opendaylight.controller.config.persist.storage.file.xml.FeatureListProvider; +import org.opendaylight.controller.config.persist.storage.file.xml.XmlFileStorageAdapter; import org.osgi.framework.BundleContext; import org.osgi.framework.ServiceReference; import org.osgi.framework.ServiceRegistration; @@ -28,7 +34,22 @@ public class FeatureServiceCustomizer implements ServiceTrackerCustomizer reference) { BundleContext bc = reference.getBundle().getBundleContext(); - FeaturesService featureService = bc.getService(reference); + final FeaturesService featureService = bc.getService(reference); + final Optional currentPersister = XmlFileStorageAdapter.getInstance(); + + if(XmlFileStorageAdapter.getInstance().isPresent()) { + final Set installedFeatureIds = Sets.newHashSet(); + for (final Feature installedFeature : featureService.listInstalledFeatures()) { + installedFeatureIds.add(installedFeature.getId()); + } + + currentPersister.get().setFeaturesService(new FeatureListProvider() { + @Override + public Set listFeatures() { + return installedFeatureIds; + } + }); + } configFeaturesListener = new ConfigFeaturesListener(configPusher,featureService); registration = bc.registerService(FeaturesListener.class.getCanonicalName(), configFeaturesListener, null); return featureService; diff --git a/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/FeatureListProvider.java b/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/FeatureListProvider.java new file mode 100644 index 0000000000..c10279ac3e --- /dev/null +++ b/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/FeatureListProvider.java @@ -0,0 +1,11 @@ +package org.opendaylight.controller.config.persist.storage.file.xml; + +import java.util.Set; + +/** + * Wrapper for services providing list of features to be stored along with the config snapshot + */ +public interface FeatureListProvider { + + Set listFeatures(); +} diff --git a/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/XmlFileStorageAdapter.java b/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/XmlFileStorageAdapter.java index 8bd420865c..a714fdc9d2 100644 --- a/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/XmlFileStorageAdapter.java +++ b/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/XmlFileStorageAdapter.java @@ -16,6 +16,7 @@ import java.io.File; import java.io.IOException; import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.SortedSet; import org.opendaylight.controller.config.persist.api.ConfigSnapshotHolder; import org.opendaylight.controller.config.persist.api.Persister; @@ -38,8 +39,23 @@ public class XmlFileStorageAdapter implements StorageAdapter, Persister { private static Integer numberOfStoredBackups; private File storage; + private static volatile XmlFileStorageAdapter instance; + private volatile ConfigSnapshot lastCfgSnapshotCache; + private volatile Optional featuresService = Optional.absent(); + + @VisibleForTesting + public void reset() { + instance = null; + lastCfgSnapshotCache = null; + featuresService = null; + } + @Override public Persister instantiate(PropertiesProvider propertiesProvider) { + if(instance != null) { + return instance; + } + File storage = extractStorageFileFromProperties(propertiesProvider); LOG.debug("Using file {}", storage.getAbsolutePath()); // Create file if it does not exist @@ -64,9 +80,23 @@ public class XmlFileStorageAdapter implements StorageAdapter, Persister { + " property should be either set to positive value, or ommited. Can not be set to 0."); } setFileStorage(storage); + + instance = this; return this; } + public static Optional getInstance() { + return Optional.fromNullable(instance); + } + + public Set getPersistedFeatures() { + return lastCfgSnapshotCache == null ? Collections.emptySet() : lastCfgSnapshotCache.getFeatures(); + } + + public void setFeaturesService(final FeatureListProvider featuresService) { + this.featuresService = Optional.of(featuresService); + } + @VisibleForTesting public void setFileStorage(File storage) { this.storage = storage; @@ -95,8 +125,13 @@ public class XmlFileStorageAdapter implements StorageAdapter, Persister { public void persistConfig(ConfigSnapshotHolder holder) throws IOException { Preconditions.checkNotNull(storage, "Storage file is null"); + Set installedFeatureIds = Collections.emptySet(); + if(featuresService.isPresent()) { + installedFeatureIds = featuresService.get().listFeatures(); + } + Config cfg = Config.fromXml(storage); - cfg.addConfigSnapshot(ConfigSnapshot.fromConfigSnapshot(holder), numberOfStoredBackups); + cfg.addConfigSnapshot(ConfigSnapshot.fromConfigSnapshot(holder, installedFeatureIds), numberOfStoredBackups); cfg.toXml(storage); } @@ -111,7 +146,8 @@ public class XmlFileStorageAdapter implements StorageAdapter, Persister { Optional lastSnapshot = Config.fromXml(storage).getLastSnapshot(); if (lastSnapshot.isPresent()) { - return Lists.newArrayList(toConfigSnapshot(lastSnapshot.get())); + lastCfgSnapshotCache = lastSnapshot.get(); + return Lists.newArrayList(toConfigSnapshot(lastCfgSnapshotCache)); } else { return Collections.emptyList(); } diff --git a/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/model/ConfigSnapshot.java b/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/model/ConfigSnapshot.java index 589a644f3d..02ebb061f0 100644 --- a/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/model/ConfigSnapshot.java +++ b/opendaylight/config/config-persister-file-xml-adapter/src/main/java/org/opendaylight/controller/config/persist/storage/file/xml/model/ConfigSnapshot.java @@ -7,6 +7,8 @@ */ package org.opendaylight.controller.config.persist.storage.file.xml.model; +import java.util.HashSet; +import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; import javax.xml.bind.annotation.XmlAnyElement; @@ -23,12 +25,19 @@ public class ConfigSnapshot { private String configSnapshot; private SortedSet capabilities = new TreeSet<>(); + private Set features = new HashSet<>(); ConfigSnapshot(String configXml, SortedSet capabilities) { this.configSnapshot = configXml; this.capabilities = capabilities; } + ConfigSnapshot(String configXml, SortedSet capabilities, Set features) { + this.configSnapshot = configXml; + this.capabilities = capabilities; + this.features = features; + } + public ConfigSnapshot() { } @@ -36,6 +45,9 @@ public class ConfigSnapshot { return new ConfigSnapshot(cfg.getConfigSnapshot(), cfg.getCapabilities()); } + public static ConfigSnapshot fromConfigSnapshot(ConfigSnapshotHolder cfg, Set features) { + return new ConfigSnapshot(cfg.getConfigSnapshot(), cfg.getCapabilities(), features); + } @XmlAnyElement(SnapshotHandler.class) public String getConfigSnapshot() { @@ -57,11 +69,23 @@ public class ConfigSnapshot { this.capabilities = capabilities; } + @XmlElement(name = "feature") + @XmlElementWrapper(name = "features") + @XmlJavaTypeAdapter(value=StringTrimAdapter.class) + public Set getFeatures() { + return features; + } + + public void setFeatures(final Set features) { + this.features = features; + } + @Override public String toString() { final StringBuilder sb = new StringBuilder("ConfigSnapshot{"); sb.append("configSnapshot='").append(configSnapshot).append('\''); sb.append(", capabilities=").append(capabilities); + sb.append(", features=").append(features); sb.append('}'); return sb.toString(); } diff --git a/opendaylight/config/config-persister-file-xml-adapter/src/test/java/org/opendaylight/controller/config/persist/storage/file/xml/FileStorageAdapterTest.java b/opendaylight/config/config-persister-file-xml-adapter/src/test/java/org/opendaylight/controller/config/persist/storage/file/xml/FileStorageAdapterTest.java index 16ea1b8431..de5f41fff0 100644 --- a/opendaylight/config/config-persister-file-xml-adapter/src/test/java/org/opendaylight/controller/config/persist/storage/file/xml/FileStorageAdapterTest.java +++ b/opendaylight/config/config-persister-file-xml-adapter/src/test/java/org/opendaylight/controller/config/persist/storage/file/xml/FileStorageAdapterTest.java @@ -8,13 +8,17 @@ package org.opendaylight.controller.config.persist.storage.file.xml; +import static junit.framework.Assert.assertTrue; import static org.custommonkey.xmlunit.XMLAssert.assertXMLEqual; import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; import com.google.common.base.Charsets; +import com.google.common.collect.Sets; import java.io.File; import java.io.FileNotFoundException; import java.io.IOException; @@ -22,9 +26,9 @@ import java.nio.file.Files; import java.util.List; import java.util.SortedSet; import java.util.TreeSet; +import org.junit.After; import org.junit.Before; import org.junit.Test; -import org.mockito.Mockito; import org.opendaylight.controller.config.persist.api.ConfigSnapshotHolder; import org.opendaylight.controller.config.persist.test.PropertiesProviderTest; @@ -34,6 +38,7 @@ public class FileStorageAdapterTest { private File file; private static final String NON_EXISTENT_DIRECTORY = "./nonExistentDir/"; private static final String NON_EXISTENT_FILE = "nonExistent.txt"; + private XmlFileStorageAdapter storage; @Before public void setUp() throws Exception { @@ -44,11 +49,16 @@ public class FileStorageAdapterTest { } com.google.common.io.Files.write("", file, Charsets.UTF_8); i = 1; + storage = new XmlFileStorageAdapter(); + } + + @After + public void tearDown() throws Exception { + storage.reset(); } @Test public void testNewFile() throws Exception { - XmlFileStorageAdapter storage = new XmlFileStorageAdapter(); PropertiesProviderTest pp = new PropertiesProviderTest(); pp.addProperty("fileStorage",NON_EXISTENT_DIRECTORY+NON_EXISTENT_FILE); pp.addProperty("numberOfBackups",Integer.toString(Integer.MAX_VALUE)); @@ -72,9 +82,9 @@ public class FileStorageAdapterTest { assertEquals(storage.toString().replace("\\","/"),"XmlFileStorageAdapter [storage="+NON_EXISTENT_DIRECTORY+NON_EXISTENT_FILE+"]"); delete(new File(NON_EXISTENT_DIRECTORY)); } + @Test public void testFileAdapter() throws Exception { - XmlFileStorageAdapter storage = new XmlFileStorageAdapter(); PropertiesProviderTest pp = new PropertiesProviderTest(); pp.addProperty("fileStorage",file.getPath()); pp.addProperty("numberOfBackups",Integer.toString(Integer.MAX_VALUE)); @@ -95,7 +105,7 @@ public class FileStorageAdapterTest { storage.persistConfig(holder); - assertEquals(27, com.google.common.io.Files.readLines(file, Charsets.UTF_8).size()); + assertEquals(29, com.google.common.io.Files.readLines(file, Charsets.UTF_8).size()); List lastConf = storage.loadLastConfigs(); assertEquals(1, lastConf.size()); ConfigSnapshotHolder configSnapshotHolder = lastConf.get(0); @@ -122,8 +132,6 @@ public class FileStorageAdapterTest { @Test public void testFileAdapterOneBackup() throws Exception { - XmlFileStorageAdapter storage = new XmlFileStorageAdapter(); - PropertiesProviderTest pp = new PropertiesProviderTest(); pp.addProperty("fileStorage",file.getPath()); pp.addProperty("numberOfBackups",Integer.toString(Integer.MAX_VALUE)); @@ -144,7 +152,7 @@ public class FileStorageAdapterTest { storage.persistConfig(holder); - assertEquals(27, com.google.common.io.Files.readLines(file, Charsets.UTF_8).size()); + assertEquals(29, com.google.common.io.Files.readLines(file, Charsets.UTF_8).size()); List lastConf = storage.loadLastConfigs(); assertEquals(1, lastConf.size()); @@ -152,9 +160,69 @@ public class FileStorageAdapterTest { assertXMLEqual("2", configSnapshotHolder.getConfigSnapshot()); } + @Test + public void testWithFeatures() throws Exception { + PropertiesProviderTest pp = new PropertiesProviderTest(); + pp.addProperty("fileStorage",file.getPath()); + pp.addProperty("numberOfBackups",Integer.toString(Integer.MAX_VALUE)); + storage.instantiate(pp); + + final ConfigSnapshotHolder holder = new ConfigSnapshotHolder() { + @Override + public String getConfigSnapshot() { + return createConfig(); + } + + @Override + public SortedSet getCapabilities() { + return createCaps(); + } + }; + final FeatureListProvider mock = mock(FeatureListProvider.class); + + doReturn(Sets.newHashSet("f1-11", "f2-22")).when(mock).listFeatures(); + storage.setFeaturesService(mock); + storage.persistConfig(holder); + + assertEquals(20, com.google.common.io.Files.readLines(file, Charsets.UTF_8).size()); + + List lastConf = storage.loadLastConfigs(); + assertEquals(1, lastConf.size()); + ConfigSnapshotHolder configSnapshotHolder = lastConf.get(0); + assertXMLEqual("1", configSnapshotHolder.getConfigSnapshot()); + assertEquals(Sets.newHashSet("f1-11", "f2-22"), storage.getPersistedFeatures()); + } + + @Test + public void testNoFeaturesStored() throws Exception { + PropertiesProviderTest pp = new PropertiesProviderTest(); + pp.addProperty("fileStorage",file.getPath()); + pp.addProperty("numberOfBackups",Integer.toString(Integer.MAX_VALUE)); + storage.instantiate(pp); + + com.google.common.io.Files.write("\n" + + "\n" + + " \n" + + " \n" + + " \n" + + " cap12\n" + + " \n" + + " \n" + + " 1\n" + + " \n" + + " \n" + + " \n" + + "", file, Charsets.UTF_8); + + List lastConf = storage.loadLastConfigs(); + assertEquals(1, lastConf.size()); + ConfigSnapshotHolder configSnapshotHolder = lastConf.get(0); + assertXMLEqual("1", configSnapshotHolder.getConfigSnapshot()); + assertTrue(storage.getPersistedFeatures().isEmpty()); + } + @Test public void testFileAdapterOnlyTwoBackups() throws Exception { - XmlFileStorageAdapter storage = new XmlFileStorageAdapter(); storage.setFileStorage(file); storage.setNumberOfBackups(2); final ConfigSnapshotHolder holder = new ConfigSnapshotHolder() { @@ -174,13 +242,14 @@ public class FileStorageAdapterTest { storage.persistConfig(holder); List readLines = com.google.common.io.Files.readLines(file, Charsets.UTF_8); - assertEquals(27, readLines.size()); + assertEquals(29, readLines.size()); List lastConf = storage.loadLastConfigs(); assertEquals(1, lastConf.size()); ConfigSnapshotHolder configSnapshotHolder = lastConf.get(0); assertXMLEqual("3", configSnapshotHolder.getConfigSnapshot()); assertFalse(readLines.contains(holder.getConfigSnapshot())); + assertTrue(storage.getPersistedFeatures().isEmpty()); } @Test @@ -209,7 +278,7 @@ public class FileStorageAdapterTest { storage.persistConfig(new ConfigSnapshotHolder() { @Override public String getConfigSnapshot() { - return Mockito.mock(String.class); + return mock(String.class); } @Override diff --git a/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/PersisterAggregatorTest.java b/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/PersisterAggregatorTest.java index e6464f8403..c962a46bb1 100644 --- a/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/PersisterAggregatorTest.java +++ b/opendaylight/netconf/config-persister-impl/src/test/java/org/opendaylight/controller/netconf/persist/impl/PersisterAggregatorTest.java @@ -23,6 +23,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Properties; +import org.junit.Before; import org.junit.Test; import org.opendaylight.controller.config.persist.api.ConfigSnapshotHolder; import org.opendaylight.controller.config.persist.api.Persister; @@ -67,6 +68,13 @@ public class PersisterAggregatorTest { } } + @Before + public void setUp() throws Exception { + if(XmlFileStorageAdapter.getInstance().isPresent()) { + XmlFileStorageAdapter.getInstance().get().reset(); + } + } + @Test public void testDummyAdapter() throws Exception { PersisterAggregator persisterAggregator = PersisterAggregator.createFromProperties(TestingPropertiesProvider.loadFile("test1.properties"));