Damans227 commented on code in PR #14348:
URL: https://github.com/apache/cloudstack/pull/14348#discussion_r4223813292
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -420,22 +420,21 @@ private IPAddressVO assignAndAllocateIpAddressEntry(final
Account owner, final V
}
private IPAddressVO assignIpAddressWithLock(IPAddressVO possibleAddr) {
- IPAddressVO finalAddress = null;
- IPAddressVO userIp =
_ipAddressDao.acquireInLockTable(possibleAddr.getId());
- if (userIp != null) {
- logger.debug("locked row for ip address {} (id: {})",
possibleAddr.getAddress(), possibleAddr.getUuid());
- if (userIp.getState() == State.Free) {
- possibleAddr.setState(State.Allocating);
- if (_ipAddressDao.update(possibleAddr.getId(), possibleAddr)) {
- logger.info("successfully allocated ip address {}",
possibleAddr.getAddress());
- finalAddress = possibleAddr;
- }
- } else {
- logger.debug("locked ip address {} is not free {}",
possibleAddr.getAddress(), userIp.getState());
- }
- _ipAddressDao.releaseFromLockTable(possibleAddr.getId());
+ IPAddressVO userIp = _ipAddressDao.lockRow(possibleAddr.getId(), true);
+ if (userIp == null) {
+ return null;
+ }
+ logger.debug("locked row for ip address {} (id: {})",
possibleAddr.getAddress(), possibleAddr.getUuid());
+ if (userIp.getState() != State.Free) {
+ logger.debug("locked ip address {} is not free {}",
possibleAddr.getAddress(), userIp.getState());
+ return null;
Review Comment:
that covers it, thanks
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -950,11 +955,30 @@ public PublicIp fetchNewPublicIp(final long dcId, final
Long podId, final List<L
public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final
List<Long> vlanDbIds, final Account owner, final VlanType vlanUse, final Long
guestNetworkId,
final boolean sourceNat, final boolean assign, final boolean
allocate, final String requestedIp, final String requestedGateway, final
boolean isSystem, final Long vpcId, final Boolean displayIp, final boolean
forSystemVms)
throws InsufficientAddressCapacityException {
- List<IPAddressVO> addrs = listAvailablePublicIps(dcId, podId,
vlanDbIds, owner, vlanUse, guestNetworkId, sourceNat, assign, allocate,
requestedIp, requestedGateway, isSystem, vpcId, displayIp, forSystemVms, true);
- IPAddressVO addr = addrs.get(0);
- if (assign) {
+ // Two allocations (for example the console proxy and secondary
storage system VMs started at the same
+ // time) can be handed the same free address because
listAvailablePublicIps selects a candidate in a
+ // separate transaction from the one that marks it Allocating.
assignIpAddressWithLock now locks the row
+ // and re-checks it is still Free, so the loser gets back null;
re-select a different free IP and retry
+ // rather than failing with no-free-IP while free addresses still
exist.
+ IPAddressVO addr = null;
+ for (int attempt = 1; attempt <= MAX_PUBLIC_IP_ALLOCATION_ATTEMPTS;
attempt++) {
Review Comment:
can we add a test where the first pick is taken and the second try gets
another ip? the new tests only cover the lock part
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
