Enabling role timeout configuration on the Device Manager

Change-Id: Ie8664189d236431a5f2ee42476d0f99fdfe5f1d7
(cherry picked from commit 32762fa3a3fce3f0cc6d97e9fadca2959080ad08)

core/net/src/main/java/org/onosproject/net/device/impl/DeviceManager.java

@@ -22,6 +22,7 @@ import com.google.common.collect.Multimap;
 import com.google.common.util.concurrent.Futures;
 import org.onlab.util.KryoNamespace;
 import org.onlab.util.Tools;
+import org.onosproject.cfg.ComponentConfigService;
 import org.onosproject.cluster.ClusterService;
 import org.onosproject.cluster.ControllerNode;
 import org.onosproject.cluster.NodeId;
@@ -70,21 +71,25 @@ import org.onosproject.store.cluster.messaging.MessageSubject;
 import org.onosproject.store.serializers.KryoNamespaces;
 import org.onosproject.store.service.Serializer;
 import org.onosproject.upgrade.UpgradeService;
+import org.osgi.service.component.ComponentContext;
 import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
 import org.osgi.service.component.annotations.Deactivate;
 import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.Modified;
 import org.osgi.service.component.annotations.ReferenceCardinality;
 import org.slf4j.Logger;
 
 import java.time.Instant;
 import java.util.Collection;
+import java.util.Dictionary;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import java.util.Properties;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -97,15 +102,19 @@ import java.util.stream.Collectors;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.common.collect.Multimaps.newListMultimap;
 import static com.google.common.collect.Multimaps.synchronizedListMultimap;
 import static java.util.concurrent.Executors.newSingleThreadExecutor;
 import static java.util.concurrent.Executors.newSingleThreadScheduledExecutor;
 import static java.lang.System.currentTimeMillis;
+import static org.onlab.util.Tools.get;
 import static org.onlab.util.Tools.groupedThreads;
 import static org.onosproject.net.MastershipRole.MASTER;
 import static org.onosproject.net.MastershipRole.NONE;
 import static org.onosproject.net.MastershipRole.STANDBY;
+import static org.onosproject.net.device.impl.OsgiPropertyConstants.ROLE_TIMEOUT_SECONDS;
+import static org.onosproject.net.device.impl.OsgiPropertyConstants.ROLE_TIMEOUT_SECONDS_DEFAULT;
 import static org.onosproject.security.AppGuard.checkPermission;
 import static org.onosproject.security.AppPermission.Type.DEVICE_READ;
 import static org.slf4j.LoggerFactory.getLogger;
@@ -115,7 +124,10 @@ import static org.slf4j.LoggerFactory.getLogger;
  */
 @Component(immediate = true,
            service = {DeviceService.class, DeviceAdminService.class,
-                      DeviceProviderRegistry.class, PortConfigOperatorRegistry.class })
+                      DeviceProviderRegistry.class, PortConfigOperatorRegistry.class },
+           property = {
+                ROLE_TIMEOUT_SECONDS + ":Integer=" + ROLE_TIMEOUT_SECONDS_DEFAULT
+           })
 public class DeviceManager
         extends AbstractListenerProviderRegistry<DeviceEvent, DeviceListener, DeviceProvider, DeviceProviderService>
         implements DeviceService, DeviceAdminService, DeviceProviderRegistry, PortConfigOperatorRegistry {
@@ -162,6 +174,9 @@ public class DeviceManager
     @Reference(cardinality = ReferenceCardinality.MANDATORY)
     protected ClusterCommunicationService communicationService;
 
+    @Reference(cardinality = ReferenceCardinality.MANDATORY)
+    protected ComponentConfigService cfgService;
+
     private ExecutorService clusterRequestExecutor;
     /**
      * List of all registered PortConfigOperator.
@@ -214,14 +229,22 @@ public class DeviceManager
     private final Map<DeviceId, Long> roleToAcknowledge =
             Maps.newConcurrentMap();
     private ScheduledExecutorService backgroundRoleChecker;
-    private static final int ROLE_TIMEOUT_SECONDS = 10;
+
+
+    /**
+     * Timeout for role acknowledgement check.
+     **/
+    protected int roleTimeoutSeconds = ROLE_TIMEOUT_SECONDS_DEFAULT;
 
     // FIXME join this map with roleToAcknowledge and fix the back to back event issue here
     private final Map<DeviceId, MastershipRole> lastAcknowledgedRole =
             Maps.newConcurrentMap();
 
     @Activate
-    public void activate() {
+    public void activate(ComponentContext context) {
+        cfgService.registerProperties(getClass());
+
+        modified(context);
         portAnnotationOp = new PortAnnotationOperator(networkConfigService);
         deviceAnnotationOp = new DeviceAnnotationOperator(networkConfigService);
         portOpsIndex.put(PortAnnotationConfig.class, portAnnotationOp);
@@ -271,8 +294,25 @@ public class DeviceManager
         log.info("Started");
     }
 
+    @Modified
+    public void modified(ComponentContext context) {
+        Dictionary<?, ?> properties = context != null ? context.getProperties() : new Properties();
+        String roleTimeoutSec = get(properties, ROLE_TIMEOUT_SECONDS);
+        int oldRoleTimeoutSeconds = roleTimeoutSeconds;
+        try {
+            roleTimeoutSeconds = isNullOrEmpty(roleTimeoutSec) ?
+                    oldRoleTimeoutSeconds : Integer.parseInt(roleTimeoutSec.trim());
+        } catch (NumberFormatException e) {
+            log.warn("Can't parse {}, setting the old value {}", roleTimeoutSec, oldRoleTimeoutSeconds, e);
+            roleTimeoutSeconds = oldRoleTimeoutSeconds;
+        }
+        log.info("Modified. Values = {}: {}",
+                ROLE_TIMEOUT_SECONDS, roleTimeoutSeconds);
+    }
+
     @Deactivate
-    public void deactivate() {
+    public void deactivate(ComponentContext context) {
+        cfgService.unregisterProperties(getClass(), true);
         backgroundService.shutdown();
         networkConfigService.removeListener(networkConfigListener);
         store.unsetDelegate(delegate);
@@ -613,7 +653,7 @@ public class DeviceManager
                     return null;
                 }
                 exists.set(true);
-                if (currentTimeMillis() - value < (ROLE_TIMEOUT_SECONDS * 1000)) {
+                if (currentTimeMillis() - value < (roleTimeoutSeconds * 1000)) {
                     return value;
                 }
                 return null;
@@ -625,7 +665,7 @@ public class DeviceManager
             }
             // Timeout still on
             if (ts != null) {
-                log.debug("Timeout expires in {} ms", ((ROLE_TIMEOUT_SECONDS * 1000) - currentTimeMillis() + ts));
+                log.debug("Timeout expires in {} ms", ((roleTimeoutSeconds * 1000) - currentTimeMillis() + ts));
                 continue;
             }
             if (myRole != MASTER) {

core/net/src/main/java/org/onosproject/net/device/impl/OsgiPropertyConstants.java

@@ -0,0 +1,28 @@
+/*
+ * Copyright 2022-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.net.device.impl;
+
+/**
+ * Constants for default values of configurable properties.
+ */
+public final class OsgiPropertyConstants {
+
+    private OsgiPropertyConstants() {
+    }
+
+    public static final String ROLE_TIMEOUT_SECONDS = "roleTimeoutSeconds";
+    public static final int ROLE_TIMEOUT_SECONDS_DEFAULT = 10;
+}

core/net/src/test/java/org/onosproject/net/device/impl/DeviceManagerTest.java

@@ -19,8 +19,10 @@ import com.google.common.collect.Sets;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.onlab.osgi.ComponentContextAdapter;
 import org.onlab.packet.ChassisId;
 import org.onlab.packet.IpAddress;
+import org.onosproject.cfg.ComponentConfigAdapter;
 import org.onosproject.cluster.ClusterServiceAdapter;
 import org.onosproject.cluster.ControllerNode;
 import org.onosproject.cluster.DefaultControllerNode;
@@ -100,6 +102,7 @@ public class DeviceManagerTest {
         service = mgr;
         admin = mgr;
         registry = mgr;
+        mgr.cfgService = new ComponentConfigAdapter();
         mgr.store = new SimpleDeviceStore();
         injectEventDispatcher(mgr, new TestEventDispatcher());
         TestMastershipManager mastershipManager = new TestMastershipManager();
@@ -108,7 +111,7 @@ public class DeviceManagerTest {
         mgr.clusterService = new TestClusterService();
         mgr.networkConfigService = new TestNetworkConfigService();
         mgr.communicationService = new TestClusterCommunicationService();
-        mgr.activate();
+        mgr.activate(new ComponentContextAdapter());
 
 
         service.addListener(listener);
@@ -125,7 +128,7 @@ public class DeviceManagerTest {
         assertFalse("provider should not be registered",
                     registry.getProviders().contains(provider.id()));
         service.removeListener(listener);
-        mgr.deactivate();
+        mgr.deactivate(new ComponentContextAdapter());
     }
 
     private void connectDevice(DeviceId deviceId, String swVersion) {

protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OFChannelHandler.java

@@ -244,6 +244,9 @@ class OFChannelHandler extends ChannelInboundHandlerAdapter
      * Executor is instantiated as a single thread executor guaranteeing processing
      * of device status messages in order.
      */
+    // TODO With a huge number of ports per device congestion can be created at the runtimeExecutor,
+    //  leading to mastership roles check going into timeout, with issues in multi-instance mastership handling.
+    //  An option is to Experiment with priority tasks - to give priority (under certain conditions) to the ROLE_REPLY
     protected ExecutorService runtimeStatusExecutor;
 
     /**