Fixed issue where component configuration would not propagate on node restart.
Change-Id: I431ce444025cc9a8021dd68fa23c6e8c29fa6c19
diff --git a/core/api/src/main/java/org/onosproject/cfg/ComponentConfigStore.java b/core/api/src/main/java/org/onosproject/cfg/ComponentConfigStore.java
index 0aa1b43..00e8eab 100644
--- a/core/api/src/main/java/org/onosproject/cfg/ComponentConfigStore.java
+++ b/core/api/src/main/java/org/onosproject/cfg/ComponentConfigStore.java
@@ -15,8 +15,11 @@
*/
package org.onosproject.cfg;
+import com.google.common.collect.ImmutableSet;
import org.onosproject.store.Store;
+import java.util.Set;
+
/**
* Service for storing and distributing system-wide configurations for various
* software components.
@@ -42,4 +45,26 @@
*/
void unsetProperty(String componentName, String name);
+
+ /**
+ * Returns set of component configuration property names.
+ *
+ * @param component component name
+ * @return set of property names
+ */
+ default Set<String> getProperties(String component) {
+ return ImmutableSet.of();
+ }
+
+ /**
+ * Returns the string value of the given component configuration property.
+ *
+ * @param component component name
+ * @param name property name; null if no property found
+ * @return set of property names
+ */
+ default String getProperty(String component, String name) {
+ return null;
+ }
+
}
diff --git a/core/net/src/main/java/org/onosproject/cfg/impl/ComponentConfigManager.java b/core/net/src/main/java/org/onosproject/cfg/impl/ComponentConfigManager.java
index 321f5d6..b53646f 100644
--- a/core/net/src/main/java/org/onosproject/cfg/impl/ComponentConfigManager.java
+++ b/core/net/src/main/java/org/onosproject/cfg/impl/ComponentConfigManager.java
@@ -38,11 +38,11 @@
import java.io.IOException;
import java.io.InputStream;
import java.util.Dictionary;
-import java.util.Enumeration;
import java.util.HashSet;
import java.util.Hashtable;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import static com.google.common.base.Preconditions.checkArgument;
@@ -84,7 +84,7 @@
protected ConfigurationAdmin cfgAdmin;
// Locally maintained catalog of definitions.
- private final Map<String, Map<String, ConfigProperty>> properties =
+ private final Map<String, Map<String, ConfigProperty>> properties =
Maps.newConcurrentMap();
@@ -103,7 +103,6 @@
@Override
public Set<String> getComponentNames() {
checkPermission(CONFIG_READ);
-
return ImmutableSet.copyOf(properties.keySet());
}
@@ -125,7 +124,7 @@
defs.forEach(p -> map.put(p.name(), p));
properties.put(componentName, map);
- loadExistingValues(componentName);
+ loadExistingValues(componentName, map);
} catch (IOException e) {
log.error("Unable to read property definitions from resource " + resourceName, e);
}
@@ -174,7 +173,6 @@
@Override
public void preSetProperty(String componentName, String name, String value) {
-
checkPermission(CONFIG_WRITE);
checkNotNull(componentName, COMPONENT_NULL);
@@ -246,9 +244,6 @@
return;
}
}
-
- // If definition doesn't exist in local catalog, cache the property.
- preSet(componentName, name, value);
}
// Locates the property in the component map and replaces it with an
@@ -267,21 +262,6 @@
}
}
- // Stores non-existent property so that loadExistingValues() can load in future.
- private void preSet(String componentName, String name, String value) {
- try {
- Configuration config = cfgAdmin.getConfiguration(componentName, null);
- Dictionary<String, Object> property = config.getProperties();
- if (property == null) {
- property = new Hashtable<>();
- }
- property.put(name, value);
- config.update(property);
- } catch (IOException e) {
- log.error("Failed to preset configuration for {}", componentName);
- }
- }
-
// Checks whether the value of the specified configuration property is a valid one or not.
private void checkValidity(String componentName, String name, String newValue) {
Map<String, ConfigProperty> map = properties.get(componentName);
@@ -306,7 +286,7 @@
break;
case BOOLEAN:
if (!((newValue != null) && (newValue.equalsIgnoreCase("true") ||
- newValue.equalsIgnoreCase("false")))) {
+ newValue.equalsIgnoreCase("false")))) {
throw new IllegalArgumentException("Invalid " + type + " value");
}
break;
@@ -320,30 +300,39 @@
}
} catch (NumberFormatException e) {
- throw new NumberFormatException("Invalid " + type + " value");
+ throw new NumberFormatException("Invalid " + type + " value");
}
}
- // Loads existing property values that may have been set.
- private void loadExistingValues(String componentName) {
+ // Loads existing property values that may have been learned from other
+ // nodes in the cluster before the local property registration.
+ private void loadExistingValues(String componentName,
+ Map<String, ConfigProperty> map) {
try {
Configuration cfg = cfgAdmin.getConfiguration(componentName, null);
- Map<String, ConfigProperty> map = properties.get(componentName);
- Dictionary<String, Object> props = cfg.getProperties();
- if (props != null) {
- Enumeration<String> it = props.keys();
- while (it.hasMoreElements()) {
- String name = it.nextElement();
- ConfigProperty p = map.get(name);
- try {
- if (p != null) {
- checkValidity(componentName, name, (String) props.get(name));
- map.put(name, ConfigProperty.setProperty(p, (String) props.get(name)));
- }
- } catch (IllegalArgumentException e) {
- log.warn("Default values will be applied " +
- "since presetted value is not a valid " + p.type());
+ Dictionary<String, Object> localProperties = cfg.getProperties();
+
+ // Iterate over all registered config properties...
+ for (ConfigProperty p : ImmutableSet.copyOf(map.values())) {
+ String name = p.name();
+ String globalValue = store.getProperty(componentName, name);
+ String localValue = localProperties != null ? (String) localProperties.get(name) : null;
+ try {
+ // If the global value is the same as default, reset to
+ // default locally.
+ if (Objects.equals(globalValue, p.defaultValue())) {
+ reset(componentName, name);
+
+ } else if (!Objects.equals(globalValue, localValue)) {
+ // If the local value is different from global value?
+ // validate the global value and apply it locally.
+ checkValidity(componentName, name, globalValue);
+ set(componentName, name, globalValue);
}
+ } catch (IllegalArgumentException e) {
+ log.warn("Value {} is not a valid {}; using default",
+ globalValue, p.type());
+ reset(componentName, name);
}
}
} catch (IOException e) {
@@ -360,7 +349,7 @@
Configuration cfg = cfgAdmin.getConfiguration(componentName, null);
Map<String, ConfigProperty> map = properties.get(componentName);
Dictionary<String, Object> props = new Hashtable<>();
- map.values().forEach(p -> props.put(p.name(), p.value()));
+ map.values().stream().filter(p -> p.value() != null).forEach(p -> props.put(p.name(), p.value()));
cfg.update(props);
} catch (IOException e) {
log.warn("Unable to update configuration for " + componentName, e);
diff --git a/core/store/dist/src/main/java/org/onosproject/store/cfg/DistributedComponentConfigStore.java b/core/store/dist/src/main/java/org/onosproject/store/cfg/DistributedComponentConfigStore.java
index 5a82a8a..85e826f 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/cfg/DistributedComponentConfigStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/cfg/DistributedComponentConfigStore.java
@@ -15,6 +15,7 @@
*/
package org.onosproject.store.cfg;
+import com.google.common.collect.ImmutableSet;
import org.apache.felix.scr.annotations.Activate;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Deactivate;
@@ -31,8 +32,12 @@
import org.onosproject.store.service.MapEventListener;
import org.onosproject.store.service.Serializer;
import org.onosproject.store.service.StorageService;
+import org.onosproject.store.service.Versioned;
import org.slf4j.Logger;
+import java.util.Objects;
+import java.util.Set;
+
import static org.onosproject.cfg.ComponentConfigEvent.Type.PROPERTY_SET;
import static org.onosproject.cfg.ComponentConfigEvent.Type.PROPERTY_UNSET;
import static org.onosproject.store.service.MapEvent.Type.INSERT;
@@ -59,7 +64,7 @@
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
protected StorageService storageService;
- InternalPropertiesListener propertiesListener = new InternalPropertiesListener();
+ private InternalPropertiesListener propertiesListener = new InternalPropertiesListener();
@Activate
public void activate() {
@@ -90,6 +95,22 @@
properties.remove(key(componentName, name));
}
+ @Override
+ public Set<String> getProperties(String componentName) {
+ ImmutableSet.Builder<String> names = ImmutableSet.builder();
+ properties.keySet().stream()
+ .filter((String k) -> Objects.equals(componentName, k.substring(0, k.indexOf(SEP))))
+ .map((String k) -> k.substring(k.indexOf(SEP) + 1))
+ .forEach(names::add);
+ return names.build();
+ }
+
+ @Override
+ public String getProperty(String componentName, String name) {
+ Versioned<String> v = properties.get(key(componentName, name));
+ return v != null ? v.value() : null;
+ }
+
/**
* Listener to component configuration properties distributed map changes.
*/
diff --git a/core/store/dist/src/test/java/org/onosproject/store/cfg/DistributedComponentConfigStoreTest.java b/core/store/dist/src/test/java/org/onosproject/store/cfg/DistributedComponentConfigStoreTest.java
new file mode 100644
index 0000000..ddefb02
--- /dev/null
+++ b/core/store/dist/src/test/java/org/onosproject/store/cfg/DistributedComponentConfigStoreTest.java
@@ -0,0 +1,81 @@
+/*
+ * Copyright 2018-present Open Networking Foundation
+ *
+ * Licensed 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.onosproject.store.cfg;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.onosproject.cfg.ComponentConfigEvent;
+import org.onosproject.store.service.TestStorageService;
+
+import static org.junit.Assert.*;
+
+public class DistributedComponentConfigStoreTest {
+
+ private static final String C1 = "c1";
+ private static final String C2 = "c2";
+
+ private TestStore store;
+ private ComponentConfigEvent event;
+
+ /**
+ * Sets up the device key store and the storage service test harness.
+ */
+ @Before
+ public void setUp() {
+ store = new TestStore();
+ store.storageService = new TestStorageService();
+ store.setDelegate(e -> this.event = e);
+ store.activate();
+ }
+
+ /**
+ * Tears down the device key store.
+ */
+ @After
+ public void tearDown() {
+ store.deactivate();
+ }
+
+ @Test
+ public void basics() {
+ assertNull("property should not be found", store.getProperty(C1, "bar"));
+ store.setProperty(C1, "foo", "yo");
+ store.setProperty(C1, "bar", "true");
+ store.setProperty(C2, "goo", "6.28");
+ assertEquals("incorrect event", ComponentConfigEvent.Type.PROPERTY_SET, event.type());
+ assertEquals("incorrect event key", "goo", event.name());
+ assertEquals("incorrect event value", "6.28", event.value());
+
+ assertEquals("incorrect property value", "true", store.getProperty(C1, "bar"));
+ assertEquals("incorrect property count", ImmutableSet.of("foo", "bar"),
+ store.getProperties(C1));
+
+ store.unsetProperty(C1, "bar");
+ assertEquals("incorrect event", ComponentConfigEvent.Type.PROPERTY_UNSET, event.type());
+ assertEquals("incorrect event key", "bar", event.name());
+ assertNull("incorrect event value", event.value());
+
+ assertNull("property should not be found", store.getProperty(C1, "bar"));
+ assertEquals("incorrect property count", ImmutableSet.of("foo"),
+ store.getProperties(C1));
+ }
+
+ class TestStore extends DistributedComponentConfigStore {
+ }
+}
\ No newline at end of file