X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=blobdiff_plain;f=neutronvpn%2Fimpl%2Fsrc%2Fmain%2Fjava%2Forg%2Fopendaylight%2Fnetvirt%2Fneutronvpn%2FNeutronFloatingToFixedIpMappingChangeListener.java;h=1c1abd60eb317f8b80bd551d202416287ff8afe6;hb=4478da5eb9679ca59a1bef5178ec5d861bfd5e0a;hp=c1d6176fb169b0ca2b2cc75eab9ad1dbef2567b7;hpb=82fc8f6178fa492fc7aa52219a56f85f78316492;p=netvirt.git diff --git a/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronFloatingToFixedIpMappingChangeListener.java b/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronFloatingToFixedIpMappingChangeListener.java index c1d6176fb1..1c1abd60eb 100644 --- a/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronFloatingToFixedIpMappingChangeListener.java +++ b/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronFloatingToFixedIpMappingChangeListener.java @@ -10,8 +10,10 @@ package org.opendaylight.netvirt.neutronvpn; import static org.opendaylight.netvirt.neutronvpn.NeutronvpnUtils.buildfloatingIpIdToPortMappingIdentifier; import com.google.common.base.Optional; +import edu.umd.cs.findbugs.annotations.CheckReturnValue; import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.concurrent.TimeUnit; import javax.annotation.PostConstruct; import javax.inject.Inject; @@ -22,7 +24,8 @@ import org.opendaylight.controller.md.sal.common.api.data.ReadFailedException; import org.opendaylight.genius.datastoreutils.AsyncDataTreeChangeListenerBase; import org.opendaylight.genius.datastoreutils.SingleTransactionDataBroker; import org.opendaylight.genius.mdsalutil.MDSALUtil; -import org.opendaylight.infrautils.utils.concurrent.KeyedLocks; +import org.opendaylight.infrautils.utils.concurrent.NamedLocks; +import org.opendaylight.infrautils.utils.concurrent.NamedSimpleReentrantLock.AcquireResult; import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.inet.types.rev130715.IpAddress; import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.yang.types.rev130715.Uuid; import org.opendaylight.yang.gen.v1.urn.opendaylight.netvirt.natservice.rev160111.FloatingIpInfo; @@ -48,10 +51,10 @@ import org.slf4j.LoggerFactory; public class NeutronFloatingToFixedIpMappingChangeListener extends AsyncDataTreeChangeListenerBase { private static final Logger LOG = LoggerFactory.getLogger(NeutronFloatingToFixedIpMappingChangeListener.class); - private static long LOCK_WAIT_TIME = 10L; + private static final long LOCK_WAIT_TIME = 10L; private final DataBroker dataBroker; - private final KeyedLocks routerLock = new KeyedLocks<>(); + private final NamedLocks routerLock = new NamedLocks<>(); @Inject public NeutronFloatingToFixedIpMappingChangeListener(final DataBroker dataBroker) { @@ -127,7 +130,6 @@ public class NeutronFloatingToFixedIpMappingChangeListener extends AsyncDataTree private void addToFloatingIpInfo(String routerName, Uuid extNetworkId, String fixedNeutronPortName, String fixedIpAddress, String floatingIpAddress, Uuid floatingIpId) { RouterPortsBuilder routerPortsBuilder; - boolean isLockAcquired = false; InstanceIdentifier routerPortsIdentifier = InstanceIdentifier.builder(FloatingIpInfo.class) .child(RouterPorts.class, new RouterPortsKey(routerName)).build(); try { @@ -178,25 +180,27 @@ public class NeutronFloatingToFixedIpMappingChangeListener extends AsyncDataTree portsList.add(fixedNeutronPortBuilder.build()); routerPortsBuilder.setPorts(portsList); } - isLockAcquired = routerLock.tryLock(routerName, LOCK_WAIT_TIME, TimeUnit.SECONDS); - LOG.debug("Creating/Updating routerPorts node {} in floatingIpInfo DS for floating IP {} on fixed " - + "neutron port {} : ", routerName, floatingIpAddress, fixedNeutronPortName); - MDSALUtil.syncWrite(dataBroker, LogicalDatastoreType.CONFIGURATION, routerPortsIdentifier, - routerPortsBuilder.build()); - LOG.debug("FloatingIpInfo DS updated for floating IP {} ", floatingIpAddress); + + try (AcquireResult lock = tryRouterLock(routerName)) { + if (!lock.wasAcquired()) { + // FIXME: why do we even bother with locking if we do not honor it?! + logTryLockFailure(routerName); + } + + LOG.debug("Creating/Updating routerPorts node {} in floatingIpInfo DS for floating IP {} on fixed " + + "neutron port {} : ", routerName, floatingIpAddress, fixedNeutronPortName); + MDSALUtil.syncWrite(dataBroker, LogicalDatastoreType.CONFIGURATION, routerPortsIdentifier, + routerPortsBuilder.build()); + LOG.debug("FloatingIpInfo DS updated for floating IP {} ", floatingIpAddress); + } } catch (ReadFailedException | RuntimeException e) { LOG.error("addToFloatingIpInfo failed for floating IP: {} ", floatingIpAddress, e); - } finally { - if (isLockAcquired) { - routerLock.unlock(routerName); - } } } // TODO Clean up the exception handling @SuppressWarnings("checkstyle:IllegalCatch") private void clearFromFloatingIpInfo(String routerName, String fixedNeutronPortName, String fixedIpAddress) { - boolean isLockAcquired = false; InstanceIdentifier.InstanceIdentifierBuilder routerPortsIdentifierBuilder = InstanceIdentifier .builder(FloatingIpInfo.class).child(RouterPorts.class, new RouterPortsKey(routerName)); try { @@ -205,35 +209,38 @@ public class NeutronFloatingToFixedIpMappingChangeListener extends AsyncDataTree routerPortsIdentifierBuilder.build()); if (optionalRouterPorts.isPresent()) { RouterPorts routerPorts = optionalRouterPorts.get(); - List portsList = routerPorts.getPorts(); + List portsList = routerPorts.nonnullPorts(); List intExtPortMap = new ArrayList<>(); for (Ports ports : portsList) { - if (ports.getPortName().equals(fixedNeutronPortName)) { - intExtPortMap = ports.getInternalToExternalPortMap(); + if (Objects.equals(ports.getPortName(), fixedNeutronPortName)) { + intExtPortMap = ports.nonnullInternalToExternalPortMap(); break; } } if (intExtPortMap.size() == 1) { removeRouterPortsOrPortsNode(routerName, routerPortsIdentifierBuilder, portsList, - fixedNeutronPortName, isLockAcquired); + fixedNeutronPortName); } else { for (InternalToExternalPortMap intToExtMap : intExtPortMap) { - if (intToExtMap.getInternalIp().equals(fixedIpAddress)) { + if (Objects.equals(intToExtMap.getInternalIp(), fixedIpAddress)) { InstanceIdentifier intExtPortMapIdentifier = routerPortsIdentifierBuilder.child(Ports .class, new PortsKey(fixedNeutronPortName)).child(InternalToExternalPortMap.class, new InternalToExternalPortMapKey(fixedIpAddress)).build(); - try { + try (AcquireResult lock = tryRouterLock(fixedIpAddress)) { + if (!lock.wasAcquired()) { + // FIXME: why do we even bother with locking if we do not honor it?! + logTryLockFailure(fixedIpAddress); + } + // remove particular internal-to-external-port-map - isLockAcquired = routerLock.tryLock(fixedIpAddress, LOCK_WAIT_TIME, TimeUnit.SECONDS); LOG.debug("removing particular internal-to-external-port-map {}", intExtPortMap); - MDSALUtil.syncDelete(dataBroker, LogicalDatastoreType.CONFIGURATION, + try { + MDSALUtil.syncDelete(dataBroker, LogicalDatastoreType.CONFIGURATION, intExtPortMapIdentifier); - } catch (Exception e) { - LOG.error("Failure in deletion of internal-to-external-port-map {}", intExtPortMap, e); - } finally { - if (isLockAcquired) { - routerLock.unlock(fixedIpAddress); + } catch (Exception e) { + LOG.error("Failure in deletion of internal-to-external-port-map {}", intExtPortMap, + e); } } } @@ -250,7 +257,6 @@ public class NeutronFloatingToFixedIpMappingChangeListener extends AsyncDataTree } protected void dissociatefixedIPFromFloatingIP(String fixedNeutronPortName) { - boolean isLockAcquired = false; InstanceIdentifier.InstanceIdentifierBuilder floatingIpInfoIdentifierBuilder = InstanceIdentifier.builder(FloatingIpInfo.class); try { @@ -264,13 +270,13 @@ public class NeutronFloatingToFixedIpMappingChangeListener extends AsyncDataTree List portsList = routerPorts.getPorts(); if (portsList != null && !portsList.isEmpty()) { for (Ports ports : portsList) { - if (ports.getPortName().equals(fixedNeutronPortName)) { + if (Objects.equals(ports.getPortName(), fixedNeutronPortName)) { String routerName = routerPorts.getRouterId(); InstanceIdentifier.InstanceIdentifierBuilder routerPortsIdentifierBuilder = floatingIpInfoIdentifierBuilder .child(RouterPorts.class, new RouterPortsKey(routerName)); removeRouterPortsOrPortsNode(routerName, routerPortsIdentifierBuilder, portsList, - fixedNeutronPortName, isLockAcquired); + fixedNeutronPortName); LOG.debug("Deletion from FloatingIpInfo DS successful for fixedIP neutron port {} ", fixedNeutronPortName); break; @@ -291,34 +297,32 @@ public class NeutronFloatingToFixedIpMappingChangeListener extends AsyncDataTree } } - // TODO Clean up the exception handling - @SuppressWarnings("checkstyle:IllegalCatch") - private void removeRouterPortsOrPortsNode(String routerName, InstanceIdentifier - .InstanceIdentifierBuilder routerPortsIdentifierBuilder, List portsList, - String fixedNeutronPortName, boolean isLockAcquired) { - String lockName = null; - try { - if (portsList.size() == 1) { - // remove entire routerPorts node - lockName = routerName; - isLockAcquired = routerLock.tryLock(lockName, LOCK_WAIT_TIME, TimeUnit.SECONDS); + private void removeRouterPortsOrPortsNode(String routerName, + InstanceIdentifier.InstanceIdentifierBuilder routerPortsIdentifierBuilder, + List portsList, String fixedNeutronPortName) { + if (portsList.size() == 1) { + // remove entire routerPorts node + try (AcquireResult lock = tryRouterLock(routerName)) { + if (!lock.wasAcquired()) { + // FIXME: why do we even bother with locking if we do not honor it?! + logTryLockFailure(routerName); + } + LOG.debug("removing routerPorts node: {} ", routerName); MDSALUtil.syncDelete(dataBroker, LogicalDatastoreType.CONFIGURATION, routerPortsIdentifierBuilder - .build()); - } else { - // remove entire ports node under this routerPorts node - lockName = fixedNeutronPortName; - isLockAcquired = routerLock.tryLock(lockName, LOCK_WAIT_TIME, TimeUnit.SECONDS); - LOG.debug("removing ports node {} under routerPorts node {}", fixedNeutronPortName, routerName); - InstanceIdentifier.InstanceIdentifierBuilder portsIdentifierBuilder = - routerPortsIdentifierBuilder.child(Ports.class, new PortsKey(fixedNeutronPortName)); - MDSALUtil.syncDelete(dataBroker, LogicalDatastoreType.CONFIGURATION, portsIdentifierBuilder.build()); + .build()); } - } catch (Exception e) { - LOG.error("Failure in deletion of routerPorts node {}", routerName, e); - } finally { - if (isLockAcquired) { - routerLock.unlock(lockName); + } else { + // remove entire ports node under this routerPorts node + try (AcquireResult lock = tryRouterLock(fixedNeutronPortName)) { + if (!lock.wasAcquired()) { + // FIXME: why do we even bother with locking if we do not honor it?! + logTryLockFailure(fixedNeutronPortName); + } + + LOG.debug("removing ports node {} under routerPorts node {}", fixedNeutronPortName, routerName); + MDSALUtil.syncDelete(dataBroker, LogicalDatastoreType.CONFIGURATION, + routerPortsIdentifierBuilder.child(Ports.class, new PortsKey(fixedNeutronPortName)).build()); } } } @@ -356,4 +360,13 @@ public class NeutronFloatingToFixedIpMappingChangeListener extends AsyncDataTree + "IP Port Info Config DS failed", floatingIpId.getValue(), e); } } + + @CheckReturnValue + private AcquireResult tryRouterLock(final String lockName) { + return routerLock.tryAcquire(lockName, LOCK_WAIT_TIME, TimeUnit.SECONDS); + } + + private static void logTryLockFailure(String lockName) { + LOG.warn("Lock for {} was not acquired, continuing anyway", lockName, new Throwable()); + } }