NETVIRT-1062: refactor SubnetmapChangeListener

* SubnetmapChangeListener:

    don't need to call vpnSubnetRouteHandler.onSubnetAddedToVpn method
    second time if Subnetmap.getInternetVpnId attribute was set.
    According to initial design,
    vpnSubnetRouteHandler.onSubnetAddedToVpn method doesn't take in
    account InternetVpnId, when it creates/updates/removes
    SubnetOpDataEntry for the given Subnetmap.

* ExternalSubnetVpnInstanceListener:

    fix potential race condition. It is better at first to create an
    Internal VPN instance for the given provider subnet and only then
    try to update all corresponding objects.

* NeutronvpnManager:

  ** updateVpnInternetForSubnet:
    should always keep set VpnId attribute for updating Subnetmap.

  ** associateExtNetworkToVpn and disassociateExtNetworkFromVpn:
    to avoid race conditions it is better at first to update BgpVpnType
    of VPN instance and only then to update associated/disassociated
    SubnetMaps with a new InternetVpnId/null.

JIRA: NETVIRT-1062
Change-Id: Ia388b9112f2ca683864c2c15d8ac44ed0e0c25b8
Signed-off-by: Valentina <valentina.krasnobaeva@6wind.com>

diff --git a/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronExternalSubnetHandler.java b/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronExternalSubnetHandler.java
index abf57f3eeaa7e01d1225e8caf8d7d51705432c79..9de986ba18d7fda1f4cd2f3ee357f68a2ece97d4 100644 (file)
--- a/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronExternalSubnetHandler.java
+++ b/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronExternalSubnetHandler.java
@@ -42,8 +42,8 @@ public class NeutronExternalSubnetHandler implements AutoCloseable {
         if (NeutronvpnUtils.getIsExternal(network) && NeutronvpnUtils.isFlatOrVlanNetwork(network)) {
             LOG.info("Added external subnet {} part of external network {} will create NAT external subnet",
                     subnetId.getValue(), networkId.getValue());
-            nvpnManager.updateSubnetNode(subnetId, null/* routerId */, subnetId, null /* internet-vpn-id */);
             nvpnNatManager.updateOrAddExternalSubnet(networkId, subnetId, routerIds);
+            nvpnManager.updateSubnetNode(subnetId, null/* routerId */, subnetId, null /* internet-vpn-id */);
             nvpnManager.createVpnInstanceForSubnet(subnetId);
         }
     }

diff --git a/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronvpnManager.java b/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronvpnManager.java
index bee549712b17b4d06cdbbe282cc99a612a438268..04f12949f52a8cf35b2c25a6f22fd21d399c072b 100644 (file)
--- a/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronvpnManager.java
+++ b/neutronvpn/impl/src/main/java/org/opendaylight/netvirt/neutronvpn/NeutronvpnManager.java
@@ -1635,13 +1635,19 @@ public class NeutronvpnManager implements NeutronvpnService, AutoCloseable, Even
     }
 
     protected void updateVpnInternetForSubnet(Subnetmap sm, Uuid vpn, boolean isBeingAssociated) {
-        LOG.debug("updateVpnInternetForSubnet : {} subnet {} with BGPVPN Internet {} ",
+        LOG.debug("updateVpnInternetForSubnet: {} subnet {} with BGPVPN Internet {} ",
              isBeingAssociated ? "associating" : "dissociating", sm.getSubnetIp(),
              vpn.getValue());
+        Uuid internalVpnId = sm.getVpnId();
+        if (internalVpnId == null) {
+            LOG.error("updateVpnInternetForSubnet: can not find Internal or BGPVPN Id for subnet {}, bailing out",
+                      sm.getId().getValue());
+            return;
+        }
         if (isBeingAssociated) {
-            updateSubnetNode(sm.getId(), null, null, vpn);
+            updateSubnetNode(sm.getId(), null, sm.getVpnId(), vpn);
         } else {
-            updateSubnetNode(sm.getId(), null, null, null);
+            updateSubnetNode(sm.getId(), null, sm.getVpnId(), null);
         }
 
         jobCoordinator.enqueueJob("VPN-" + vpn.getValue(), () -> singletonList(
@@ -2346,6 +2352,10 @@ public class NeutronvpnManager implements NeutronvpnService, AutoCloseable, Even
         if (!addExternalNetworkToVpn(extNet, vpnId)) {
             return false;
         }
+        if (!vpnOpDataEntry.getBgpvpnType().equals(BgpvpnType.BGPVPNInternet)) {
+            LOG.info("associateExtNetworkToVpn: set type {} for VPN {}", BgpvpnType.BGPVPNInternet, vpnId.getValue());
+            neutronvpnUtils.updateVpnInstanceOpWithType(BgpvpnType.BGPVPNInternet, vpnId);
+        }
         for (Uuid snId: neutronvpnUtils.getPrivateSubnetsToExport(extNet)) {
             Subnetmap sm = neutronvpnUtils.getSubnetmap(snId);
             if (sm == null) {
@@ -2359,10 +2369,6 @@ public class NeutronvpnManager implements NeutronvpnService, AutoCloseable, Even
                 neutronvpnUtils.updateVpnInstanceWithFallback(vpnId.getValue(), true);
             }
         }
-        if (!vpnOpDataEntry.getBgpvpnType().getName().equals(BgpvpnType.BGPVPNInternet.toString())) {
-            LOG.info("associateExtNetworkToVpn: set type {} for VPN {}", BgpvpnType.BGPVPNInternet, vpnId.getValue());
-            neutronvpnUtils.updateVpnInstanceOpWithType(BgpvpnType.BGPVPNInternet, vpnId);
-        }
         return true;
     }
 
@@ -2458,6 +2464,9 @@ public class NeutronvpnManager implements NeutronvpnService, AutoCloseable, Even
                 }
             }
         }
+        LOG.info("disassociateExtNetworkFromVpn: set type {} for VPN {}",
+                VpnInstanceOpDataEntry.BgpvpnType.BGPVPNExternal, vpnId.getValue());
+        neutronvpnUtils.updateVpnInstanceOpWithType(VpnInstanceOpDataEntry.BgpvpnType.BGPVPNExternal, vpnId);
         for (Uuid snId : neutronvpnUtils.getPrivateSubnetsToExport(extNet)) {
             Subnetmap sm = neutronvpnUtils.getSubnetmap(snId);
             if (sm == null) {
@@ -2469,9 +2478,6 @@ public class NeutronvpnManager implements NeutronvpnService, AutoCloseable, Even
         neutronvpnUtils.updateVpnInstanceWithIpFamily(vpnId.getValue(), IpVersionChoice.IPV6, false);
         LOG.info("disassociateExtNetworkFromVpn: withdraw IPv6 Internet default route from VPN {}", vpnId.getValue());
         neutronvpnUtils.updateVpnInstanceWithFallback(vpnId.getValue(), false);
-        LOG.info("disassociateExtNetworkFromVpn: set type {} for VPN {}",
-                 VpnInstanceOpDataEntry.BgpvpnType.BGPVPNExternal.toString(), vpnId.getValue());
-        neutronvpnUtils.updateVpnInstanceOpWithType(VpnInstanceOpDataEntry.BgpvpnType.VPN, vpnId);
         return true;
     }
 

diff --git a/vpnmanager/impl/src/main/java/org/opendaylight/netvirt/vpnmanager/SubnetmapChangeListener.java b/vpnmanager/impl/src/main/java/org/opendaylight/netvirt/vpnmanager/SubnetmapChangeListener.java
index 16611421c0cd7dde7ebd0e4bac60d0cae115ef06..aac45ea9efe68dae5b22a934a4e708198b1cd94d 100644 (file)
--- a/vpnmanager/impl/src/main/java/org/opendaylight/netvirt/vpnmanager/SubnetmapChangeListener.java
+++ b/vpnmanager/impl/src/main/java/org/opendaylight/netvirt/vpnmanager/SubnetmapChangeListener.java
@@ -11,6 +11,7 @@ package org.opendaylight.netvirt.vpnmanager;
 import com.google.common.base.Optional;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Objects;
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
 import javax.inject.Singleton;
@@ -21,7 +22,6 @@ import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.yang.types.
 import org.opendaylight.yang.gen.v1.urn.opendaylight.netvirt.elan.rev150602.ElanInstances;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.netvirt.elan.rev150602.elan.instances.ElanInstance;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.netvirt.elan.rev150602.elan.instances.ElanInstanceKey;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.netvirt.neutronvpn.rev150602.NetworkAttributes.NetworkType;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.netvirt.neutronvpn.rev150602.Subnetmaps;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.netvirt.neutronvpn.rev150602.subnetmaps.Subnetmap;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.neutron.networks.rev150712.networks.attributes.networks.Network;
@@ -66,60 +66,36 @@ public class SubnetmapChangeListener extends AsyncDataTreeChangeListenerBase<Sub
 
     @Override
     protected void add(InstanceIdentifier<Subnetmap> identifier, Subnetmap subnetmap) {
-        LOG.trace("add:SubnetmapChangeListener add subnetmap method - key: {}, value: {}", identifier, subnetmap);
+        LOG.debug("SubnetmapChangeListener add subnetmap method - key: {}, value: {}", identifier, subnetmap);
         Uuid subnetId = subnetmap.getId();
-        Uuid vpnId = subnetmap.getVpnId();
-
-        if (subnetmap.getVpnId() != null) {
-            // SubnetRoute for ExternalSubnets is handled in ExternalSubnetVpnInstanceListener.
-            // Here we must handle only InternalVpnSubnetRoute and BGPVPNBasedSubnetRoute
-            Network network = VpnUtil.getNeutronNetwork(dataBroker, subnetmap.getNetworkId());
-            if (network == null) {
-                LOG.info("update: vpnId {}, networkId: {}, subnetId: {}: network was not found",
-                    vpnId, subnetmap.getNetworkId(), subnetId);
-                return;
-            }
-            boolean isExtNetwork = VpnUtil.getIsExternal(VpnUtil.getNeutronNetwork(dataBroker,
-                                           subnetmap.getNetworkId()));
-            // SubnetRoute for ExternalNetwork is not impacting for internetvpn
-            if (isExtNetwork) {
-                return;
-            }
-            boolean isBgpVpn = !vpnId.equals(subnetmap.getRouterId());
-            String elanInstanceName = subnetmap.getNetworkId().getValue();
-            Long elanTag = getElanTag(elanInstanceName);
-            if (elanTag.equals(0L)) {
-                LOG.error("add:Unable to fetch elantag from ElanInstance {} and hence not proceeding with "
-                        + "subnetmapListener add for subnet {}", elanInstanceName, subnetId.getValue());
-                return;
-            }
-            if (subnetmap.getNetworkType().equals(NetworkType.VLAN)) {
-                VpnUtil.addRouterPortToElanDpnListForVlaninAllDpn(vpnId.getValue(), dataBroker);
-            }
-            // subnet added to VPN case upon config DS replay after reboot
-            // ports added to subnet upon config DS replay after reboot are handled implicitly by subnetAddedToVpn
-            // in SubnetRouteHandler
-            vpnSubnetRouteHandler.onSubnetAddedToVpn(subnetmap, isBgpVpn , elanTag);
+        Network network = VpnUtil.getNeutronNetwork(dataBroker, subnetmap.getNetworkId());
+        if (network == null) {
+            LOG.error("SubnetMapChangeListener:add: network was not found for subnetId {}", subnetId.getValue());
+            return;
         }
-        if (subnetmap.getInternetVpnId() != null) {
-            boolean isBgpVpn = !subnetmap.getInternetVpnId().equals(subnetmap.getRouterId());
-            String elanInstanceName = subnetmap.getNetworkId().getValue();
-            Long elanTag = getElanTag(elanInstanceName);
-            if (elanTag.equals(0L)) {
-                LOG.error("add:Unable to fetch elantag from ElanInstance {} and hence not proceeding with "
-                        + "subnetmapListener add for subnet {}", elanInstanceName, subnetId);
-                return;
-            }
-            // subnet added to VPN case upon config DS replay after reboot
-            // ports added to subnet upon config DS replay after reboot are handled implicitly by subnetAddedToVpn
-            // in SubnetRouteHandler
-            vpnSubnetRouteHandler.onSubnetAddedToVpn(subnetmap, isBgpVpn , elanTag);
+        if (VpnUtil.getIsExternal(network)) {
+            LOG.debug("SubnetmapListener:add: provider subnetwork {} is handling in "
+                      + "ExternalSubnetVpnInstanceListener", subnetId.getValue());
+            return;
+        }
+        String elanInstanceName = subnetmap.getNetworkId().getValue();
+        long elanTag = getElanTag(elanInstanceName);
+        if (elanTag == 0L) {
+            LOG.error("SubnetMapChangeListener:add: unable to fetch elantag from ElanInstance {} for subnet {}",
+                      elanInstanceName, subnetId.getValue());
+            return;
+        }
+        if (subnetmap.getVpnId() != null) {
+            boolean isBgpVpn = !subnetmap.getVpnId().equals(subnetmap.getRouterId());
+            LOG.info("SubnetMapChangeListener:add: subnetmap {} with elanTag {} to VPN {}", subnetmap, elanTag,
+                     subnetmap.getVpnId());
+            vpnSubnetRouteHandler.onSubnetAddedToVpn(subnetmap, isBgpVpn, elanTag);
         }
     }
 
     @Override
     protected void remove(InstanceIdentifier<Subnetmap> identifier, Subnetmap subnetmap) {
-        LOG.trace("remove:SubnetmapListener remove subnetmap method - key: {}, value: {}", identifier, subnetmap);
+        LOG.trace("SubnetmapListener:remove: subnetmap method - key: {}, value: {}", identifier, subnetmap);
     }
 
     @Override
@@ -127,49 +103,44 @@ public class SubnetmapChangeListener extends AsyncDataTreeChangeListenerBase<Sub
     @SuppressWarnings("checkstyle:IllegalCatch")
     protected void update(InstanceIdentifier<Subnetmap> identifier, Subnetmap subnetmapOriginal, Subnetmap
             subnetmapUpdate) {
-        LOG.trace("update:SubnetmapListener update subnetmap method - key {}, original {}, update {}", identifier,
-                subnetmapOriginal, subnetmapUpdate);
-        Uuid vpnIdNew = subnetmapUpdate.getVpnId();
-        Uuid vpnIdOld = subnetmapOriginal.getVpnId();
+        LOG.debug("SubnetMapChangeListener update method - key {}, original {}, update {}", identifier,
+                  subnetmapOriginal, subnetmapUpdate);
         Uuid subnetId = subnetmapUpdate.getId();
-        if ((subnetmapUpdate.getNetworkId() == null)
-            && subnetmapOriginal.getNetworkId() == null) {
-            // transition: subnetmap is removed with syncwrite.
-            // wait next write to do the update
-            LOG.error("subnetmap has no network for subnetmap {}", subnetmapUpdate);
+        Network network = VpnUtil.getNeutronNetwork(dataBroker, subnetmapUpdate.getNetworkId());
+        if (network == null) {
+            LOG.error("SubnetMapChangeListener:update: network was not found for subnetId {}", subnetId.getValue());
             return;
         }
-        boolean updateCapableForCreation = false;
-        String elanInstanceName = null;
-        Long elanTag = Long.valueOf(0L);
-        if (subnetmapUpdate.getNetworkId() != null) {
-            updateCapableForCreation = true;
-            elanInstanceName = subnetmapUpdate.getNetworkId().getValue();
-            elanTag = getElanTag(elanInstanceName);
-            if (elanTag.equals(0L)) {
-                LOG.error("update:Unable to fetch elantag from ElanInstance {} and "
-                    + "hence not proceeding with "
-                    + "subnetmapListener update for subnet {}", elanInstanceName, subnetId.getValue());
-                return;
-            }
-
-            // SubnetRoute for ExternalSubnets is handled in ExternalSubnetVpnInstanceListener.
-            // Here we must handle only InternalVpnSubnetRoute and BGPVPNBasedSubnetRoute
-            Network network = VpnUtil.getNeutronNetwork(dataBroker, subnetmapUpdate.getNetworkId());
-            if (VpnUtil.getIsExternal(network)) {
-                return;
-            }
+        if (VpnUtil.getIsExternal(network)) {
+            LOG.debug("SubnetMapChangeListener:update: provider subnetwork {} is handling in "
+                      + "ExternalSubnetVpnInstanceListener", subnetId.getValue());
+            return;
         }
-        Uuid vpnIdInternetNew = subnetmapUpdate.getInternetVpnId();
-        Uuid vpnIdInternetOld = subnetmapOriginal.getInternetVpnId();
-        boolean returnValue1 = updateSubnetmapOpDataEntry(vpnIdInternetOld, vpnIdInternetNew,
-                           subnetmapUpdate, subnetmapOriginal, elanTag, null, updateCapableForCreation);
-        boolean returnValue2 = updateSubnetmapOpDataEntry(vpnIdOld, vpnIdNew,
-                      subnetmapUpdate, subnetmapOriginal, elanTag, elanInstanceName, updateCapableForCreation);
-        if (!returnValue2 || !returnValue1) {
+        String elanInstanceName = subnetmapUpdate.getNetworkId().getValue();
+        long elanTag = getElanTag(elanInstanceName);
+        if (elanTag == 0L) {
+            LOG.error("SubnetMapChangeListener:update: unable to fetch elantag from ElanInstance {} for subnetId {}",
+                      elanInstanceName, subnetId);
             return;
         }
-        // port added/removed to/from subnet case
+        // update on BGPVPN or InternalVPN change
+        Uuid vpnIdOld = subnetmapOriginal.getVpnId();
+        Uuid vpnIdNew = subnetmapUpdate.getVpnId();
+        if (!Objects.equals(vpnIdOld, vpnIdNew)) {
+            LOG.info("SubnetMapChangeListener:update: update subnetOpDataEntry for subnet {} imported in VPN",
+                     subnetmapUpdate.getId().getValue());
+            updateSubnetmapOpDataEntry(subnetmapOriginal.getVpnId(), subnetmapUpdate.getVpnId(), subnetmapUpdate,
+                                       subnetmapOriginal, elanTag);
+        }
+        // update on Internet VPN Id change
+        Uuid inetVpnIdOld = subnetmapOriginal.getInternetVpnId();
+        Uuid inetVpnIdNew = subnetmapUpdate.getInternetVpnId();
+        if (!Objects.equals(inetVpnIdOld, inetVpnIdNew)) {
+            LOG.info("SubnetMapChangeListener:update: update subnetOpDataEntry for subnet {} imported in InternetVPN",
+                     subnetmapUpdate.getId().getValue());
+            updateSubnetmapOpDataEntry(inetVpnIdOld, inetVpnIdNew, subnetmapUpdate, subnetmapOriginal, elanTag);
+        }
+        // update on PortList change
         List<Uuid> oldPortList;
         List<Uuid> newPortList;
         newPortList = subnetmapUpdate.getPortList() != null ? subnetmapUpdate.getPortList() : new ArrayList<>();
@@ -177,6 +148,7 @@ public class SubnetmapChangeListener extends AsyncDataTreeChangeListenerBase<Sub
         if (newPortList.size() == oldPortList.size()) {
             return;
         }
+        LOG.info("SubnetMapChangeListener:update: update port list for subnet {}", subnetmapUpdate.getId().getValue());
         if (newPortList.size() > oldPortList.size()) {
             for (Uuid portId : newPortList) {
                 if (! oldPortList.contains(portId)) {
@@ -194,40 +166,27 @@ public class SubnetmapChangeListener extends AsyncDataTreeChangeListenerBase<Sub
         }
     }
 
-    private boolean updateSubnetmapOpDataEntry(Uuid vpnIdOld, Uuid vpnIdNew, Subnetmap subnetmapUpdate,
-                                    Subnetmap subnetmapOriginal, Long elanTag, String  elanInstanceName,
-                                    boolean updateCapableForCreation) {
-        // subnet added to VPN case
-        if (vpnIdNew != null && vpnIdOld == null && updateCapableForCreation) {
-            if (elanInstanceName != null && subnetmapUpdate.getNetworkType().equals(NetworkType.VLAN)) {
-                VpnUtil.addRouterPortToElanDpnListForVlaninAllDpn(vpnIdNew.getValue(), dataBroker);
-            }
-            boolean isBgpVpn = !vpnIdNew.equals(subnetmapUpdate.getRouterId());
-            if (!isBgpVpn) {
-                return false;
+    private void updateSubnetmapOpDataEntry(Uuid vpnIdOld, Uuid vpnIdNew, Subnetmap subnetmapUpdate,
+                                    Subnetmap subnetmapOriginal, Long elanTag) {
+
+        // subnet added to VPN
+        if (vpnIdNew != null && vpnIdOld == null) {
+            if (vpnIdNew.equals(subnetmapUpdate.getRouterId())) {
+                return;
             }
             vpnSubnetRouteHandler.onSubnetAddedToVpn(subnetmapUpdate, true, elanTag);
-            return true;
         }
-        // subnet removed from VPN case
+        // subnet removed from VPN
         if (vpnIdOld != null && vpnIdNew == null) {
-            if (subnetmapOriginal.getNetworkType().equals(NetworkType.VLAN)) {
-                VpnUtil.removeRouterPortFromElanDpnListForVlanInAllDpn(elanInstanceName, subnetmapOriginal
-                        .getRouterInterfacePortId().getValue(), vpnIdOld.getValue(), dataBroker);
-            }
-            boolean isBgpVpn = vpnIdOld.equals(subnetmapOriginal.getRouterId()) ? false : true;
-            if (!isBgpVpn) {
-                return false;
+            if (vpnIdOld.equals(subnetmapOriginal.getRouterId())) {
+                return;
             }
             vpnSubnetRouteHandler.onSubnetDeletedFromVpn(subnetmapOriginal, true);
-            return true;
         }
-        // subnet updated in VPN case
+        // subnet updated in VPN
         if (vpnIdOld != null && vpnIdNew != null && (!vpnIdNew.equals(vpnIdOld))) {
             vpnSubnetRouteHandler.onSubnetUpdatedInVpn(subnetmapUpdate, elanTag);
-            return true;
         }
-        return false;
     }
 
     @Override