Skip to content

Commit 2e30734

Browse files
yadvryadvr
committed
CLOUDSTACK-9595: Fix another regression introduced in apache#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 <[email protected]>
1 parent da2278c commit 2e30734

File tree

1 file changed

+71
-56
lines changed

1 file changed

+71
-56
lines changed

‎server/src/com/cloud/network/IpAddressManagerImpl.java‎

Lines changed: 71 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,6 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
292292
SearchBuilder<IPAddressVO> AssignIpAddressSearch;
293293
SearchBuilder<IPAddressVO> AssignIpAddressFromPodVlanSearch;
294294
private static final Object allocatedLock = new Object();
295-
private static final Object allocatingLock = new Object();
296295

297296
static Boolean rulesContinueOnErrFlag = true;
298297

@@ -799,28 +798,55 @@ public IPAddressVO doInTransaction(TransactionStatus status) throws Insufficient
799798
throw new AccountLimitException("Maximum number of public IP addresses for account: " + owner.getAccountName() + " has been exceeded.");
800799
}
801800
}
802-
IPAddressVO addr = addrs.get(0);
803-
addr.setSourceNat(sourceNat);
804-
addr.setAllocatedTime(new Date());
805-
addr.setAllocatedInDomainId(owner.getDomainId());
806-
addr.setAllocatedToAccountId(owner.getId());
807-
addr.setSystem(isSystem);
808-
809-
if (displayIp != null) {
810-
addr.setDisplay(displayIp);
801+
802+
IPAddressVO finalAddr = null;
803+
for (final IPAddressVO possibleAddr: addrs) {
804+
if (possibleAddr.getState() != IpAddress.State.Free) {
805+
continue;
806+
}
807+
final IPAddressVO addr = possibleAddr;
808+
addr.setSourceNat(sourceNat);
809+
addr.setAllocatedTime(new Date());
810+
addr.setAllocatedInDomainId(owner.getDomainId());
811+
addr.setAllocatedToAccountId(owner.getId());
812+
addr.setSystem(isSystem);
813+
814+
if (displayIp != null) {
815+
addr.setDisplay(displayIp);
816+
}
817+
818+
if (vlanUse != VlanType.DirectAttached) {
819+
addr.setAssociatedWithNetworkId(guestNetworkId);
820+
addr.setVpcId(vpcId);
821+
}
822+
if (_ipAddressDao.lockRow(possibleAddr.getId(), true) != null) {
823+
final IPAddressVO userIp = _ipAddressDao.findById(addr.getId());
824+
if (userIp.getState() == IpAddress.State.Free) {
825+
addr.setState(IpAddress.State.Allocating);
826+
if (_ipAddressDao.update(addr.getId(), addr)) {
827+
finalAddr = _ipAddressDao.findById(addr.getId());
828+
break;
829+
}
830+
}
831+
}
811832
}
812833

813-
if (vlanUse != VlanType.DirectAttached) {
814-
addr.setAssociatedWithNetworkId(guestNetworkId);
815-
addr.setVpcId(vpcId);
834+
if (finalAddr == null) {
835+
s_logger.error("Failed to fetch any free public IP address");
836+
throw new CloudRuntimeException("Failed to fetch any free public IP address");
816837
}
817838

818839
if (assign) {
819-
markPublicIpAsAllocated(addr);
820-
} else {
821-
markPublicIpAsAllocating(addr);
840+
markPublicIpAsAllocated(finalAddr);
841+
}
842+
843+
final State expectedAddressState = assign ? State.Allocated : State.Allocating;
844+
if (finalAddr.getState() != expectedAddressState) {
845+
s_logger.error("Failed to fetch new public IP and get in expected state=" + expectedAddressState);
846+
throw new CloudRuntimeException("Failed to fetch new public IP with expected state " + expectedAddressState);
822847
}
823-
return addr;
848+
849+
return finalAddr;
824850
}
825851
});
826852

@@ -840,7 +866,7 @@ public void markPublicIpAsAllocated(final IPAddressVO addr) {
840866
public void doInTransactionWithoutResult(TransactionStatus status) {
841867
Account owner = _accountMgr.getAccount(addr.getAllocatedToAccountId());
842868
if (_ipAddressDao.lockRow(addr.getId(), true) != null) {
843-
IPAddressVO userIp = _ipAddressDao.findById(addr.getId());
869+
final IPAddressVO userIp = _ipAddressDao.findById(addr.getId());
844870
if (userIp.getState() == IpAddress.State.Allocating || addr.getState() == IpAddress.State.Free) {
845871
addr.setState(IpAddress.State.Allocated);
846872
if (_ipAddressDao.update(addr.getId(), addr)) {
@@ -861,26 +887,8 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
861887
s_logger.error("Failed to mark public IP as allocated with id=" + addr.getId() + " address=" + addr.getAddress());
862888
}
863889
}
864-
}
865-
}
866-
});
867-
}
868-
}
869-
870-
@DB
871-
private void markPublicIpAsAllocating(final IPAddressVO addr) {
872-
synchronized (allocatingLock) {
873-
Transaction.execute(new TransactionCallbackNoReturn() {
874-
@Override
875-
public void doInTransactionWithoutResult(TransactionStatus status) {
876-
877-
if (_ipAddressDao.lockRow(addr.getId(), true) != null) {
878-
addr.setState(IpAddress.State.Allocating);
879-
if (!_ipAddressDao.update(addr.getId(), addr)) {
880-
s_logger.error("Failed to update public IP as allocating with id=" + addr.getId() + " and address=" + addr.getAddress());
881-
}
882890
} else {
883-
s_logger.error("Failed to lock row to mark public IP as allocating with id=" + addr.getId() + " and address=" + addr.getAddress());
891+
s_logger.error("Failed to acquire row lock to mark public IP as allocated with id=" + addr.getId() + " address=" + addr.getAddress());
884892
}
885893
}
886894
});
@@ -921,27 +929,34 @@ public PublicIp assignDedicateIpAddress(Account owner, final Long guestNtwkId, f
921929

922930
PublicIp ip = null;
923931
try {
924-
Account ownerAccount = _accountDao.acquireInLockTable(ownerId);
932+
ip = Transaction.execute(new TransactionCallbackWithException<PublicIp, InsufficientAddressCapacityException>() {
933+
@Override
934+
public PublicIp doInTransaction(TransactionStatus status) throws InsufficientAddressCapacityException {
935+
Account owner = _accountDao.acquireInLockTable(ownerId);
925936

926-
if (ownerAccount == null) {
927-
// this ownerId comes from owner or type Account. See the class "AccountVO" and the annotations in that class
928-
// to get the table name and field name that is queried to fill this ownerid.
929-
ConcurrentOperationException ex = new ConcurrentOperationException("Unable to lock account");
930-
throw ex;
931-
}
932-
if (s_logger.isDebugEnabled()) {
933-
s_logger.debug("lock account " + ownerId + " is acquired");
934-
}
935-
boolean displayIp = true;
936-
if (guestNtwkId != null) {
937-
Network ntwk = _networksDao.findById(guestNtwkId);
938-
displayIp = ntwk.getDisplayNetwork();
939-
} else if (vpcId != null) {
940-
VpcVO vpc = _vpcDao.findById(vpcId);
941-
displayIp = vpc.isDisplay();
937+
if (owner == null) {
938+
// this ownerId comes from owner or type Account. See the class "AccountVO" and the annotations in that class
939+
// to get the table name and field name that is queried to fill this ownerid.
940+
ConcurrentOperationException ex = new ConcurrentOperationException("Unable to lock account");
941+
throw ex;
942+
}
943+
if (s_logger.isDebugEnabled()) {
944+
s_logger.debug("lock account " + ownerId + " is acquired");
945+
}
946+
boolean displayIp = true;
947+
if (guestNtwkId != null) {
948+
Network ntwk = _networksDao.findById(guestNtwkId);
949+
displayIp = ntwk.getDisplayNetwork();
950+
} else if (vpcId != null) {
951+
VpcVO vpc = _vpcDao.findById(vpcId);
952+
displayIp = vpc.isDisplay();
953+
}
954+
return fetchNewPublicIp(dcId, null, null, owner, VlanType.VirtualNetwork, guestNtwkId, isSourceNat, true, null, false, vpcId, displayIp);
955+
}
956+
});
957+
if (ip.getState() != State.Allocated) {
958+
s_logger.error("Failed to fetch new IP and allocate it for ip with id=" + ip.getId() + ", address=" + ip.getAddress());
942959
}
943-
944-
ip = fetchNewPublicIp(dcId, null, null, owner, VlanType.VirtualNetwork, guestNtwkId, isSourceNat, true, null, false, vpcId, displayIp);
945960
return ip;
946961
} finally {
947962
if (owner != null) {

0 commit comments

Comments
 (0)