From: Robert Varga Date: Tue, 26 Jan 2016 10:04:35 +0000 (+0100) Subject: BUG-5001: clean up state transitions X-Git-Tag: release/boron~347 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=4003bf8b41ce11c5cb898fadd4beab11791c694c;p=openflowplugin.git BUG-5001: clean up state transitions This is preliminary code cleanup, reducing visibility and moving methods around so their scope and assumptions can be analyzed. Change-Id: I2699a19a897635447c54eb4beeac3cc70c64f1f5 Signed-off-by: Robert Varga --- diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/services/SalRoleServiceImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/services/SalRoleServiceImpl.java index 2d3c40dbac..28d0d526af 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/services/SalRoleServiceImpl.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/services/SalRoleServiceImpl.java @@ -8,12 +8,12 @@ package org.opendaylight.openflowplugin.impl.services; import com.google.common.base.Function; +import com.google.common.base.Preconditions; import com.google.common.util.concurrent.CheckedFuture; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; -import com.google.common.util.concurrent.SettableFuture; import java.math.BigInteger; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; @@ -22,7 +22,7 @@ import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicReference; -import org.opendaylight.openflowplugin.api.openflow.connection.ConnectionContext; +import org.opendaylight.openflowplugin.api.openflow.connection.ConnectionContext.CONNECTION_STATE; import org.opendaylight.openflowplugin.api.openflow.device.DeviceContext; import org.opendaylight.openflowplugin.api.openflow.device.RequestContextStack; import org.opendaylight.openflowplugin.api.openflow.device.Xid; @@ -40,7 +40,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class SalRoleServiceImpl extends AbstractSimpleService implements SalRoleService { +public final class SalRoleServiceImpl extends AbstractSimpleService implements SalRoleService { private static final Logger LOG = LoggerFactory.getLogger(SalRoleServiceImpl.class); @@ -48,6 +48,22 @@ public class SalRoleServiceImpl extends AbstractSimpleService EXCEPTION_FUNCTION = new Function() { + @Override + public RoleChangeException apply(final Exception input) { + if (input instanceof ExecutionException) { + final Throwable cause = input.getCause(); + if (cause instanceof RoleChangeException) { + return (RoleChangeException) cause; + } + } else if (input instanceof RoleChangeException) { + return (RoleChangeException) input; + } + + return new RoleChangeException(input.getMessage(), input); + } + }; + private final DeviceContext deviceContext; private final RoleService roleService; private final AtomicReference lastKnownRoleRef = new AtomicReference<>(OfpRole.NOCHANGE); @@ -57,134 +73,103 @@ public class SalRoleServiceImpl extends AbstractSimpleService> setRole(final SetRoleInput input) { LOG.info("SetRole called with input:{}", input); - OfpRole lastKnownRole = lastKnownRoleRef.get(); // compare with last known role and set if different. If they are same, then return. if (lastKnownRoleRef.compareAndSet(input.getControllerRole(), input.getControllerRole())) { LOG.info("Role to be set is same as the last known role for the device:{}. Hence ignoring.", input.getControllerRole()); - SettableFuture> resultFuture = SettableFuture.create(); - resultFuture.set(RpcResultBuilder.success().build()); - return resultFuture; + return Futures.immediateFuture(RpcResultBuilder.success().build()); } - final SettableFuture> resultFuture = SettableFuture.create(); - - RoleChangeTask roleChangeTask = new RoleChangeTask(nodeId, input.getControllerRole(), version, roleService); + RoleChangeTask roleChangeTask = new RoleChangeTask(input.getControllerRole()); do { - ListenableFuture> deviceCheck = deviceConnectionCheck(); - if (deviceCheck != null) { - LOG.info("Device {} is disconnected or state is not valid. Giving up on role change", input.getNode()); - return deviceCheck; + // Check current connection state + final CONNECTION_STATE state = deviceContext.getPrimaryConnectionContext().getConnectionState(); + switch (state) { + case RIP: + LOG.info("Device {} has been disconnected", input.getNode()); + return Futures.immediateFailedFuture(new Exception(String.format( + "Device connection doesn't exist anymore. Primary connection status : %s", state))); + case WORKING: + // We can proceed + break; + default: + LOG.info("Device {} is in state {}, role change is not allowed", input.getNode(), state); + return Futures.immediateCheckedFuture(RpcResultBuilder.failed().build()); } ListenableFuture taskFuture = listeningExecutorService.submit(roleChangeTask); LOG.info("RoleChangeTask submitted for execution"); - CheckedFuture taskFutureChecked = makeCheckedFuture(taskFuture); + CheckedFuture taskFutureChecked = Futures.makeChecked(taskFuture, EXCEPTION_FUNCTION); try { SetRoleOutput setRoleOutput = taskFutureChecked.checkedGet(10, TimeUnit.SECONDS); LOG.info("setRoleOutput received after roleChangeTask execution:{}", setRoleOutput); - resultFuture.set(RpcResultBuilder.success().withResult(setRoleOutput).build()); lastKnownRoleRef.set(input.getControllerRole()); - return resultFuture; + return Futures.immediateFuture(RpcResultBuilder.success().withResult(setRoleOutput).build()); } catch (TimeoutException | RoleChangeException e) { roleChangeTask.incrementRetryCounter(); - LOG.info("Exception in setRole(), will retry:" + (MAX_RETRIES - roleChangeTask.getRetryCounter()) + " times.", e); + LOG.info("Exception in setRole(), will retry: {} times.", + MAX_RETRIES - roleChangeTask.getRetryCounter(), e); } } while (roleChangeTask.getRetryCounter() < MAX_RETRIES); - resultFuture.setException(new RoleChangeException("Set Role failed after " + MAX_RETRIES + "tries on device " + input.getNode().getValue())); - - return resultFuture; + return Futures.immediateFailedFuture(new RoleChangeException( + "Set Role failed after " + MAX_RETRIES + "tries on device " + input.getNode().getValue())); } - private ListenableFuture> deviceConnectionCheck() { - if (!ConnectionContext.CONNECTION_STATE.WORKING.equals(deviceContext.getPrimaryConnectionContext().getConnectionState())) { - ListenableFuture> resultingFuture = SettableFuture.create(); - switch (deviceContext.getPrimaryConnectionContext().getConnectionState()) { - case RIP: - final String errMsg = String.format("Device connection doesn't exist anymore. Primary connection status : %s", - deviceContext.getPrimaryConnectionContext().getConnectionState()); - resultingFuture = Futures.immediateFailedFuture(new Throwable(errMsg)); - break; - default: - resultingFuture = Futures.immediateCheckedFuture(RpcResultBuilder.failed().build()); - break; - } - return resultingFuture; + private static BigInteger getNextGenerationId(final BigInteger generationId) { + if (generationId.compareTo(MAX_GENERATION_ID) < 0) { + return generationId.add(BigInteger.ONE); + } else { + return BigInteger.ZERO; } - return null; } - class RoleChangeTask implements Callable { + private final class RoleChangeTask implements Callable { - private final NodeId nodeId; private final OfpRole ofpRole; - private final Short version; - private final RoleService roleService; private int retryCounter = 0; - public RoleChangeTask(NodeId nodeId, OfpRole ofpRole, Short version, RoleService roleService) { - this.nodeId = nodeId; - this.ofpRole = ofpRole; - this.version = version; - this.roleService = roleService; + RoleChangeTask(final OfpRole ofpRole) { + this.ofpRole = Preconditions.checkNotNull(ofpRole); } @Override public SetRoleOutput call() throws RoleChangeException { - LOG.info("RoleChangeTask called on device:{} OFPRole:{}", this.nodeId.getValue(), ofpRole); + LOG.info("RoleChangeTask called on device:{} OFPRole:{}", nodeId.getValue(), ofpRole); // we cannot move ahead without having the generation id, so block the thread till we get it. - BigInteger generationId = null; - SetRoleOutput setRoleOutput = null; - + final BigInteger generationId; try { - generationId = this.roleService.getGenerationIdFromDevice(version).get(10, TimeUnit.SECONDS); + generationId = roleService.getGenerationIdFromDevice(version).get(10, TimeUnit.SECONDS); LOG.info("RoleChangeTask, GenerationIdFromDevice from device is {}", generationId); - } catch (Exception e ) { - LOG.info("Exception in getting generationId for device:{}. Ex:{}" + this.nodeId.getValue(), e); - throw new RoleChangeException("Exception in getting generationId for device:"+ this.nodeId.getValue(), e); + LOG.info("Exception in getting generationId for device:{}", nodeId.getValue(), e); + throw new RoleChangeException("Exception in getting generationId for device:"+ nodeId.getValue(), e); } - LOG.info("GenerationId received from device:{} is {}", nodeId.getValue(), generationId); - final BigInteger nextGenerationId = getNextGenerationId(generationId); - LOG.info("nextGenerationId received from device:{} is {}", nodeId.getValue(), nextGenerationId); + final SetRoleOutput setRoleOutput; try { setRoleOutput = roleService.submitRoleChange(ofpRole, version, nextGenerationId).get(10 , TimeUnit.SECONDS); LOG.info("setRoleOutput after submitRoleChange:{}", setRoleOutput); @@ -195,36 +180,14 @@ public class SalRoleServiceImpl extends AbstractSimpleService makeCheckedFuture(ListenableFuture rolePushResult) { - return Futures.makeChecked(rolePushResult, - new Function() { - @Override - public RoleChangeException apply(Exception input) { - RoleChangeException output = null; - if (input instanceof ExecutionException) { - if (input.getCause() instanceof RoleChangeException) { - output = (RoleChangeException) input.getCause(); - } - } - - if (output == null) { - output = new RoleChangeException(input.getMessage(), input); - } - - return output; - } - }); - } }