[ONOS-8123] Fix an issue related to the component properties.
Properties are not inherited by the derived classes. This means that
a subclass cannot change a property defined in the super class.
We are seeing an issue with FObj managers where the FlowObjectiveManager
is not enabled and ConfigAdmin suppresses the calls "cfg set" to its
properties. Moreover, the InOrderFlowObjective is not able to change
its properties because it does not receive calls to "modified".
The last issue happens because the properties defined in the Component
annotation are not inherited.
Additionally, this patch makes configurable the objective timeout in
InOrderFlowObjectiveManager.
Change-Id: Ibd84be914b15a17f82186b5013c0c0697d2657c3
diff --git a/core/net/src/main/java/org/onosproject/net/OsgiPropertyConstants.java b/core/net/src/main/java/org/onosproject/net/OsgiPropertyConstants.java
index fe6ac63..64b3cc2 100644
--- a/core/net/src/main/java/org/onosproject/net/OsgiPropertyConstants.java
+++ b/core/net/src/main/java/org/onosproject/net/OsgiPropertyConstants.java
@@ -161,4 +161,6 @@
public static final String FOM_ACCUMULATOR_MAX_BATCH_MILLIS = "accumulatorMaxBatchMillis";
public static final int FOM_ACCUMULATOR_MAX_BATCH_MILLIS_DEFAULT = 500;
+ public static final String IFOM_OBJ_TIMEOUT_MS = "objectiveTimeoutMs";
+ public static final int IFOM_OBJ_TIMEOUT_MS_DEFAULT = 15000;
}
diff --git a/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java b/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java
index 9e334ad..6cc355d 100644
--- a/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java
+++ b/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java
@@ -118,14 +118,13 @@
// its own accumulation logic. The following parameters are used
// to control the accumulator.
- // Maximum number of objectives to accumulate before processing is triggered
+ /** Max number of objs to accumulate before processing is triggered. */
private int accumulatorMaxObjectives = FOM_ACCUMULATOR_MAX_OBJECTIVES_DEFAULT;
- // Maximum number of millis between objectives before processing is triggered
+ /** Max of ms between objs before processing is triggered. */
private int accumulatorMaxIdleMillis = FOM_ACCUMULATOR_MAX_IDLE_MILLIS_DEFAULT;
- // Maximum number of millis allowed since the first objective before processing is triggered
+ /** Max number of ms allowed since the first obj before processing is triggered. */
private int accumulatorMaxBatchMillis = FOM_ACCUMULATOR_MAX_BATCH_MILLIS_DEFAULT;
-
@Reference(cardinality = ReferenceCardinality.MANDATORY)
protected DriverService driverService;
@@ -224,7 +223,7 @@
*
* @param context the component context
*/
- private void readComponentConfiguration(ComponentContext context) {
+ protected void readComponentConfiguration(ComponentContext context) {
String propertyValue = Tools.get(context.getProperties(), FOM_NUM_THREADS);
int newNumThreads = isNullOrEmpty(propertyValue) ? numThreads : Integer.parseInt(propertyValue);
diff --git a/core/net/src/main/java/org/onosproject/net/flowobjective/impl/InOrderFlowObjectiveManager.java b/core/net/src/main/java/org/onosproject/net/flowobjective/impl/InOrderFlowObjectiveManager.java
index 0999698..81e224b 100644
--- a/core/net/src/main/java/org/onosproject/net/flowobjective/impl/InOrderFlowObjectiveManager.java
+++ b/core/net/src/main/java/org/onosproject/net/flowobjective/impl/InOrderFlowObjectiveManager.java
@@ -56,17 +56,28 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
+import static com.google.common.base.Strings.isNullOrEmpty;
import static java.util.concurrent.Executors.newSingleThreadExecutor;
import static java.util.concurrent.Executors.newSingleThreadScheduledExecutor;
import static org.onlab.util.Tools.groupedThreads;
+import static org.onosproject.net.OsgiPropertyConstants.IFOM_OBJ_TIMEOUT_MS;
+import static org.onosproject.net.OsgiPropertyConstants.IFOM_OBJ_TIMEOUT_MS_DEFAULT;
-@Component(immediate = true, service = FlowObjectiveService.class)
+/**
+ * Provides implementation of the flow objective programming service.
+ */
+@Component(
+ immediate = true,
+ service = FlowObjectiveService.class,
+ property = {
+ IFOM_OBJ_TIMEOUT_MS + ":Integer=" + IFOM_OBJ_TIMEOUT_MS_DEFAULT
+ }
+)
public class InOrderFlowObjectiveManager extends FlowObjectiveManager {
private final Logger log = LoggerFactory.getLogger(getClass());
- // TODO Make queue timeout configurable
- static final int DEFAULT_OBJ_TIMEOUT = 15000;
- int objTimeoutMs = DEFAULT_OBJ_TIMEOUT;
+ /** Objective timeout. */
+ int objectiveTimeoutMs = IFOM_OBJ_TIMEOUT_MS_DEFAULT;
private Cache<FilteringObjQueueKey, Objective> filtObjQueueHead;
private Cache<ForwardingObjQueueKey, Objective> fwdObjQueueHead;
@@ -85,38 +96,41 @@
final FlowObjectiveStoreDelegate delegate = new InternalStoreDelegate();
+ final RemovalListener<ObjectiveQueueKey, Objective> removalListener = notification -> {
+ Objective obj = notification.getValue();
+ switch (notification.getCause()) {
+ case EXPIRED:
+ case COLLECTED:
+ case SIZE:
+ obj.context().ifPresent(c -> c.onError(obj, ObjectiveError.INSTALLATIONTIMEOUT));
+ break;
+ case EXPLICIT: // No action when the objective completes correctly
+ case REPLACED: // No action when a pending forward or next objective gets executed
+ default:
+ break;
+ }
+ };
+
@Activate
protected void activate(ComponentContext context) {
super.activate(context);
+ cfgService.registerProperties(InOrderFlowObjectiveManager.class);
+
filtCacheEventExecutor = newSingleThreadExecutor(groupedThreads("onos/flowobj", "cache-event-filt", log));
fwdCacheEventExecutor = newSingleThreadExecutor(groupedThreads("onos/flowobj", "cache-event-fwd", log));
nextCacheEventExecutor = newSingleThreadExecutor(groupedThreads("onos/flowobj", "cache-event-next", log));
- RemovalListener<ObjectiveQueueKey, Objective> removalListener = notification -> {
- Objective obj = notification.getValue();
- switch (notification.getCause()) {
- case EXPIRED:
- case COLLECTED:
- case SIZE:
- obj.context().ifPresent(c -> c.onError(obj, ObjectiveError.INSTALLATIONTIMEOUT));
- break;
- case EXPLICIT: // No action when the objective completes correctly
- case REPLACED: // No action when a pending forward or next objective gets executed
- default:
- break;
- }
- };
filtObjQueueHead = CacheBuilder.newBuilder()
- .expireAfterWrite(objTimeoutMs, TimeUnit.MILLISECONDS)
+ .expireAfterWrite(objectiveTimeoutMs, TimeUnit.MILLISECONDS)
.removalListener(RemovalListeners.asynchronous(removalListener, filtCacheEventExecutor))
.build();
fwdObjQueueHead = CacheBuilder.newBuilder()
- .expireAfterWrite(objTimeoutMs, TimeUnit.MILLISECONDS)
+ .expireAfterWrite(objectiveTimeoutMs, TimeUnit.MILLISECONDS)
.removalListener(RemovalListeners.asynchronous(removalListener, fwdCacheEventExecutor))
.build();
nextObjQueueHead = CacheBuilder.newBuilder()
- .expireAfterWrite(objTimeoutMs, TimeUnit.MILLISECONDS)
+ .expireAfterWrite(objectiveTimeoutMs, TimeUnit.MILLISECONDS)
.removalListener(RemovalListeners.asynchronous(removalListener, nextCacheEventExecutor))
.build();
@@ -125,7 +139,7 @@
filtObjQueueHead.cleanUp();
fwdObjQueueHead.cleanUp();
nextObjQueueHead.cleanUp();
- }, 0, objTimeoutMs, TimeUnit.MILLISECONDS);
+ }, 0, objectiveTimeoutMs, TimeUnit.MILLISECONDS);
// Replace store delegate to make sure pendingForward and pendingNext are resubmitted to
// execute()
@@ -135,6 +149,8 @@
@Deactivate
protected void deactivate() {
+ cfgService.unregisterProperties(getClass(), false);
+
cacheCleaner.shutdown();
clearQueue();
@@ -146,6 +162,62 @@
}
/**
+ * Extracts properties from the component configuration context.
+ *
+ * @param context the component context
+ */
+ @Override
+ protected void readComponentConfiguration(ComponentContext context) {
+ super.readComponentConfiguration(context);
+
+ // objective timeout handling
+ String propertyValue = Tools.get(context.getProperties(), IFOM_OBJ_TIMEOUT_MS);
+ int newObjectiveTimeoutMs = isNullOrEmpty(propertyValue) ?
+ objectiveTimeoutMs : Integer.parseInt(propertyValue);
+ if (newObjectiveTimeoutMs != objectiveTimeoutMs && newObjectiveTimeoutMs > 0) {
+ objectiveTimeoutMs = newObjectiveTimeoutMs;
+ log.info("Reconfigured timeout of the objectives to {}", objectiveTimeoutMs);
+ // Recreates the queues
+ if (filtObjQueueHead != null) {
+ filtObjQueueHead.invalidateAll();
+ filtObjQueueHead = null;
+ }
+ filtObjQueueHead = CacheBuilder.newBuilder()
+ .expireAfterWrite(objectiveTimeoutMs, TimeUnit.MILLISECONDS)
+ .removalListener(RemovalListeners.asynchronous(removalListener, filtCacheEventExecutor))
+ .build();
+ if (fwdObjQueueHead != null) {
+ fwdObjQueueHead.invalidateAll();
+ fwdObjQueueHead = null;
+ }
+ fwdObjQueueHead = CacheBuilder.newBuilder()
+ .expireAfterWrite(objectiveTimeoutMs, TimeUnit.MILLISECONDS)
+ .removalListener(RemovalListeners.asynchronous(removalListener, fwdCacheEventExecutor))
+ .build();
+ if (nextObjQueueHead != null) {
+ nextObjQueueHead.invalidateAll();
+ nextObjQueueHead = null;
+ }
+ nextObjQueueHead = CacheBuilder.newBuilder()
+ .expireAfterWrite(objectiveTimeoutMs, TimeUnit.MILLISECONDS)
+ .removalListener(RemovalListeners.asynchronous(removalListener, nextCacheEventExecutor))
+ .build();
+ // Restart the cleanup thread
+ if (cacheCleaner != null) {
+ cacheCleaner.shutdownNow();
+ cacheCleaner = null;
+ }
+ cacheCleaner = newSingleThreadScheduledExecutor(groupedThreads("onos/flowobj", "cache-cleaner", log));
+ cacheCleaner.scheduleAtFixedRate(() -> {
+ filtObjQueueHead.cleanUp();
+ fwdObjQueueHead.cleanUp();
+ nextObjQueueHead.cleanUp();
+ }, 0, objectiveTimeoutMs, TimeUnit.MILLISECONDS);
+ }
+ }
+
+
+ /**
* Processes given objective on given device.
* Objectives submitted through this method are guaranteed to be executed in order.
*
diff --git a/core/net/src/test/java/org/onosproject/net/flowobjective/impl/InOrderFlowObjectiveManagerTest.java b/core/net/src/test/java/org/onosproject/net/flowobjective/impl/InOrderFlowObjectiveManagerTest.java
index 8e88e77..e230550 100644
--- a/core/net/src/test/java/org/onosproject/net/flowobjective/impl/InOrderFlowObjectiveManagerTest.java
+++ b/core/net/src/test/java/org/onosproject/net/flowobjective/impl/InOrderFlowObjectiveManagerTest.java
@@ -63,6 +63,7 @@
import static org.junit.Assert.assertEquals;
import static org.onlab.junit.TestTools.assertAfter;
import static org.onlab.util.Tools.groupedThreads;
+import static org.onosproject.net.OsgiPropertyConstants.IFOM_OBJ_TIMEOUT_MS_DEFAULT;
import java.util.Collection;
import java.util.List;
@@ -186,12 +187,12 @@
@Before
public void setUp() {
- internalSetup(InOrderFlowObjectiveManager.DEFAULT_OBJ_TIMEOUT);
+ internalSetup(IFOM_OBJ_TIMEOUT_MS_DEFAULT);
}
private void internalSetup(int objTimeoutMs) {
mgr = new InOrderFlowObjectiveManager();
- mgr.objTimeoutMs = objTimeoutMs;
+ mgr.objectiveTimeoutMs = objTimeoutMs;
mgr.pipeliners.put(DEV1, pipeliner);
mgr.installerExecutor = newFixedThreadPool(4, groupedThreads("foo", "bar"));
mgr.cfgService = createMock(ComponentConfigService.class);
@@ -263,7 +264,7 @@
replay(mgr.flowObjectiveStore);
// Force this objective to time out
- offset = mgr.objTimeoutMs * 3;
+ offset = mgr.objectiveTimeoutMs * 3;
expectFwdObjsTimeout.forEach(fwdObj -> mgr.forward(DEV1, fwdObj));
diff --git a/tools/build/cfgdef/src/main/java/org/onosproject/cfgdef/CfgDefGenerator.java b/tools/build/cfgdef/src/main/java/org/onosproject/cfgdef/CfgDefGenerator.java
index 6fd7331..3e3d109 100644
--- a/tools/build/cfgdef/src/main/java/org/onosproject/cfgdef/CfgDefGenerator.java
+++ b/tools/build/cfgdef/src/main/java/org/onosproject/cfgdef/CfgDefGenerator.java
@@ -15,6 +15,7 @@
*/
package org.onosproject.cfgdef;
+import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.thoughtworks.qdox.JavaProjectBuilder;
import com.thoughtworks.qdox.model.JavaAnnotation;
@@ -55,6 +56,7 @@
private final JavaProjectBuilder builder;
private final Map<String, String> constants = Maps.newHashMap();
+ private final Map<JavaClass, List<String>> pendingProperties = Maps.newHashMap();
public static void main(String[] args) throws IOException {
if (args.length < 2) {
@@ -114,7 +116,29 @@
}
if (!lines.isEmpty()) {
+ // FIXME this might not work if we have multiple inheritance stages
+ for (JavaClass derivedClass : javaClass.getDerivedClasses()) {
+ // Temporary appends the properties - jar stream cannot be opened multiple times
+ pendingProperties.compute(derivedClass, (k, v) -> {
+ if (v == null) {
+ v = Lists.newArrayList();
+ }
+ v.addAll(lines);
+ return v;
+ });
+ }
+ // Get the attributes stored by the super class
+ List<String> superProperties = pendingProperties.remove(javaClass);
+ if (superProperties != null && !superProperties.isEmpty()) {
+ lines.addAll(superProperties);
+ }
writeCatalog(jar, javaClass, lines);
+ } else {
+ // Get the attributes stored by the super class
+ List<String> superProperties = pendingProperties.remove(javaClass);
+ if (superProperties != null && !superProperties.isEmpty()) {
+ writeCatalog(jar, javaClass, superProperties);
+ }
}
}
}
@@ -151,7 +175,8 @@
String comment = field.getComment();
return comment != null ? comment : NO_DESCRIPTION;
}
- throw new IllegalStateException("cfgdef could not find a variable named " + name + " in " + javaClass.getName());
+ throw new IllegalStateException("cfgdef could not find a variable named " + name + " in "
+ + javaClass.getName());
}
private String elaborate(AnnotationValue value) {