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 4338e0f4f182429f3151fe78628c7fc944820ffe
Author: Rohit Yadav <rohit.ya...@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.ya...@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>.
