cloudstack-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ro...@apache.org
Subject [cloudstack] 02/02: CLOUDSTACK-9595: Fix another regression introduced in #1762
Date Fri, 22 Dec 2017 13:35:58 GMT
This is an automated email from the ASF dual-hosted git repository.

rohit pushed a commit to branch debian9-systemvmtemplate
in repository https://gitbox.apache.org/repos/asf/cloudstack.git

commit 1902086238ad83d6ca2ecde6016c0a9996873e2f
Author: Rohit Yadav <rohit.yadav@shapeblue.com>
AuthorDate: Fri Dec 22 18:59:06 2017 +0530

    CLOUDSTACK-9595: Fix another regression introduced in #1762
    
    In a VMware 55u3 environment it was found that CPVM and SSVM would
    get the same public IP. After another investigative review of
    fetchNewPublicIp method, it was found that it would always pick up the
    first IP from the sql query list/result.
    
    The cause was found to be that with the new changes no table/row locks
    are done and first item is used without looping through the list of
    available free IPs. The previously implementation method that put
    IP address in allocating state did not check that it was a free IP.
    
    In this refactoring/fix, the first free IP is first marked as allocating
    and if assign is requested that is changed into Allocated state.
    
    Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
---
 .../com/cloud/network/IpAddressManagerImpl.java    | 127 ++++++++++++---------
 1 file changed, 71 insertions(+), 56 deletions(-)

diff --git a/server/src/com/cloud/network/IpAddressManagerImpl.java b/server/src/com/cloud/network/IpAddressManagerImpl.java
index 4f3fc51..208394a 100644
--- a/server/src/com/cloud/network/IpAddressManagerImpl.java
+++ b/server/src/com/cloud/network/IpAddressManagerImpl.java
@@ -292,7 +292,6 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
     SearchBuilder<IPAddressVO> AssignIpAddressSearch;
     SearchBuilder<IPAddressVO> AssignIpAddressFromPodVlanSearch;
     private static final Object allocatedLock = new Object();
-    private static final Object allocatingLock = new Object();
 
     static Boolean rulesContinueOnErrFlag = true;
 
@@ -799,28 +798,55 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
                         throw new AccountLimitException("Maximum number of public IP addresses
for account: " + owner.getAccountName() + " has been exceeded.");
                     }
                 }
-                IPAddressVO addr = addrs.get(0);
-                addr.setSourceNat(sourceNat);
-                addr.setAllocatedTime(new Date());
-                addr.setAllocatedInDomainId(owner.getDomainId());
-                addr.setAllocatedToAccountId(owner.getId());
-                addr.setSystem(isSystem);
-
-                if (displayIp != null) {
-                    addr.setDisplay(displayIp);
+
+                IPAddressVO finalAddr = null;
+                for (final IPAddressVO possibleAddr: addrs) {
+                    if (possibleAddr.getState() != IpAddress.State.Free) {
+                        continue;
+                    }
+                    final IPAddressVO addr = possibleAddr;
+                    addr.setSourceNat(sourceNat);
+                    addr.setAllocatedTime(new Date());
+                    addr.setAllocatedInDomainId(owner.getDomainId());
+                    addr.setAllocatedToAccountId(owner.getId());
+                    addr.setSystem(isSystem);
+
+                    if (displayIp != null) {
+                        addr.setDisplay(displayIp);
+                    }
+
+                    if (vlanUse != VlanType.DirectAttached) {
+                        addr.setAssociatedWithNetworkId(guestNetworkId);
+                        addr.setVpcId(vpcId);
+                    }
+                    if (_ipAddressDao.lockRow(possibleAddr.getId(), true) != null) {
+                        final IPAddressVO userIp = _ipAddressDao.findById(addr.getId());
+                        if (userIp.getState() == IpAddress.State.Free) {
+                            addr.setState(IpAddress.State.Allocating);
+                            if (_ipAddressDao.update(addr.getId(), addr)) {
+                                finalAddr = _ipAddressDao.findById(addr.getId());
+                                break;
+                            }
+                        }
+                    }
                 }
 
-                if (vlanUse != VlanType.DirectAttached) {
-                    addr.setAssociatedWithNetworkId(guestNetworkId);
-                    addr.setVpcId(vpcId);
+                if (finalAddr == null) {
+                    s_logger.error("Failed to fetch any free public IP address");
+                    throw new CloudRuntimeException("Failed to fetch any free public IP address");
                 }
 
                 if (assign) {
-                    markPublicIpAsAllocated(addr);
-                } else {
-                    markPublicIpAsAllocating(addr);
+                    markPublicIpAsAllocated(finalAddr);
+                }
+
+                final State expectedAddressState = assign ? State.Allocated : State.Allocating;
+                if (finalAddr.getState() != expectedAddressState) {
+                    s_logger.error("Failed to fetch new public IP and get in expected state="
+ expectedAddressState);
+                    throw new CloudRuntimeException("Failed to fetch new public IP with expected
state " + expectedAddressState);
                 }
-                return addr;
+
+                return finalAddr;
             }
         });
 
@@ -840,7 +866,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
                 public void doInTransactionWithoutResult(TransactionStatus status) {
                     Account owner = _accountMgr.getAccount(addr.getAllocatedToAccountId());
                     if (_ipAddressDao.lockRow(addr.getId(), true) != null) {
-                        IPAddressVO userIp = _ipAddressDao.findById(addr.getId());
+                        final IPAddressVO userIp = _ipAddressDao.findById(addr.getId());
                         if (userIp.getState() == IpAddress.State.Allocating || addr.getState()
== IpAddress.State.Free) {
                             addr.setState(IpAddress.State.Allocated);
                             if (_ipAddressDao.update(addr.getId(), addr)) {
@@ -861,26 +887,8 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
                                 s_logger.error("Failed to mark public IP as allocated with
id=" + addr.getId() + " address=" + addr.getAddress());
                             }
                         }
-                    }
-                }
-            });
-        }
-    }
-
-    @DB
-    private void markPublicIpAsAllocating(final IPAddressVO addr) {
-        synchronized (allocatingLock) {
-            Transaction.execute(new TransactionCallbackNoReturn() {
-                @Override
-                public void doInTransactionWithoutResult(TransactionStatus status) {
-
-                    if (_ipAddressDao.lockRow(addr.getId(), true) != null) {
-                        addr.setState(IpAddress.State.Allocating);
-                        if (!_ipAddressDao.update(addr.getId(), addr)) {
-                            s_logger.error("Failed to update public IP as allocating with
id=" + addr.getId() + " and address=" + addr.getAddress());
-                        }
                     } else {
-                        s_logger.error("Failed to lock row to mark public IP as allocating
with id=" + addr.getId() + " and address=" + addr.getAddress());
+                        s_logger.error("Failed to acquire row lock to mark public IP as allocated
with id=" + addr.getId() + " address=" + addr.getAddress());
                     }
                 }
             });
@@ -921,27 +929,34 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
 
         PublicIp ip = null;
         try {
-            Account ownerAccount = _accountDao.acquireInLockTable(ownerId);
+            ip = Transaction.execute(new TransactionCallbackWithException<PublicIp, InsufficientAddressCapacityException>()
{
+                @Override
+                public PublicIp doInTransaction(TransactionStatus status) throws InsufficientAddressCapacityException
{
+                    Account owner = _accountDao.acquireInLockTable(ownerId);
 
-            if (ownerAccount == null) {
-                // this ownerId comes from owner or type Account. See the class "AccountVO"
and the annotations in that class
-                // to get the table name and field name that is queried to fill this ownerid.
-                ConcurrentOperationException ex = new ConcurrentOperationException("Unable
to lock account");
-                throw ex;
-            }
-            if (s_logger.isDebugEnabled()) {
-                s_logger.debug("lock account " + ownerId + " is acquired");
-            }
-            boolean displayIp = true;
-            if (guestNtwkId != null) {
-                Network ntwk = _networksDao.findById(guestNtwkId);
-                displayIp = ntwk.getDisplayNetwork();
-            } else if (vpcId != null) {
-                VpcVO vpc = _vpcDao.findById(vpcId);
-                displayIp = vpc.isDisplay();
+                    if (owner == null) {
+                        // this ownerId comes from owner or type Account. See the class "AccountVO"
and the annotations in that class
+                        // to get the table name and field name that is queried to fill this
ownerid.
+                        ConcurrentOperationException ex = new ConcurrentOperationException("Unable
to lock account");
+                        throw ex;
+                    }
+                    if (s_logger.isDebugEnabled()) {
+                        s_logger.debug("lock account " + ownerId + " is acquired");
+                    }
+                    boolean displayIp = true;
+                    if (guestNtwkId != null) {
+                        Network ntwk = _networksDao.findById(guestNtwkId);
+                        displayIp = ntwk.getDisplayNetwork();
+                    } else if (vpcId != null) {
+                        VpcVO vpc = _vpcDao.findById(vpcId);
+                        displayIp = vpc.isDisplay();
+                    }
+                    return fetchNewPublicIp(dcId, null, null, owner, VlanType.VirtualNetwork,
guestNtwkId, isSourceNat, true, null, false, vpcId, displayIp);
+                }
+            });
+            if (ip.getState() != State.Allocated) {
+                s_logger.error("Failed to fetch new IP and allocate it for ip with id=" +
ip.getId() + ", address=" + ip.getAddress());
             }
-
-            ip = fetchNewPublicIp(dcId, null, null, owner, VlanType.VirtualNetwork, guestNtwkId,
isSourceNat, true, null, false, vpcId, displayIp);
             return ip;
         } finally {
             if (owner != null) {

-- 
To stop receiving notification emails like this one, please contact
"commits@cloudstack.apache.org" <commits@cloudstack.apache.org>.

Mime
View raw message