[ 
https://issues.apache.org/jira/browse/CLOUDSTACK-10188?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16320603#comment-16320603
 ] 

ASF GitHub Bot commented on CLOUDSTACK-10188:
---------------------------------------------

rhtyd closed pull request #2362: CLOUDSTACK-10188 - Resource Accounting for 
primary storage is Broken when Domains are in use
URL: https://github.com/apache/cloudstack/pull/2362
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/api/src/com/cloud/user/ResourceLimitService.java 
b/api/src/com/cloud/user/ResourceLimitService.java
index 52e03568984..2df9b037e3d 100644
--- a/api/src/com/cloud/user/ResourceLimitService.java
+++ b/api/src/com/cloud/user/ResourceLimitService.java
@@ -28,7 +28,7 @@
 public interface ResourceLimitService {
 
     static final ConfigKey<Long> ResourceCountCheckInterval = new 
ConfigKey<Long>("Advanced", Long.class, "resourcecount.check.interval", "300",
-            "Time (in seconds) to wait before retrying resource count check 
task. Default is 300, Setting this to 0 will not run the task", false);
+            "Time (in seconds) to wait before running resource recalculation 
and fixing task. Default is 300 seconds, Setting this to 0 disables execution 
of the task", false);
 
     /**
      * Updates an existing resource limit with the specified details. If a 
limit doesn't exist, will create one.
diff --git 
a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
 
b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
index 7669b3b98a9..4a85346495e 100644
--- 
a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
+++ 
b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
@@ -1571,8 +1571,7 @@ public void destroyVolume(Volume volume) {
             UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_DELETE, 
volume.getAccountId(), volume.getDataCenterId(), volume.getId(), 
volume.getName(),
                     Volume.class.getName(), volume.getUuid(), 
volume.isDisplayVolume());
             _resourceLimitMgr.decrementResourceCount(volume.getAccountId(), 
ResourceType.volume, volume.isDisplay());
-            //FIXME - why recalculate and not decrement
-            _resourceLimitMgr.recalculateResourceCount(volume.getAccountId(), 
volume.getDomainId(), ResourceType.primary_storage.getOrdinal());
+            _resourceLimitMgr.decrementResourceCount(volume.getAccountId(), 
ResourceType.primary_storage, volume.isDisplay(), new Long(volume.getSize()));
         } catch (Exception e) {
             s_logger.debug("Failed to destroy volume" + volume.getId(), e);
             throw new CloudRuntimeException("Failed to destroy volume" + 
volume.getId(), e);
diff --git a/server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java 
b/server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
index a1879297dd4..86fa46b6c26 100644
--- a/server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
+++ b/server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
@@ -107,7 +107,7 @@
 import com.cloud.vm.dao.VMInstanceDao;
 
 @Component
-public class ResourceLimitManagerImpl extends ManagerBase implements 
ResourceLimitService, Configurable{
+public class ResourceLimitManagerImpl extends ManagerBase implements 
ResourceLimitService, Configurable {
     public static final Logger s_logger = 
Logger.getLogger(ResourceLimitManagerImpl.class);
 
     @Inject
@@ -410,11 +410,86 @@ public long findCorrectResourceLimitForDomain(Domain 
domain, ResourceType type)
         return max;
     }
 
+    private void checkDomainResourceLimit(final Account account, final Project 
project, final ResourceType type, long numResources) throws 
ResourceAllocationException {
+        // check all domains in the account's domain hierarchy
+        Long domainId = null;
+        if (project != null) {
+            domainId = project.getDomainId();
+        } else {
+            domainId = account.getDomainId();
+        }
+
+        while (domainId != null) {
+            DomainVO domain = _domainDao.findById(domainId);
+            // no limit check if it is ROOT domain
+            if (domainId != Domain.ROOT_DOMAIN) {
+                long domainResourceLimit = 
findCorrectResourceLimitForDomain(domain, type);
+                long currentDomainResourceCount = 
_resourceCountDao.getResourceCount(domainId, ResourceOwnerType.Domain, type);
+                long requestedDomainResourceCount = currentDomainResourceCount 
+ numResources;
+                String messageSuffix = " domain resource limits of Type '" + 
type + "'" +
+                    " for Domain Id = " + domainId +
+                    " is exceeded: Domain Resource Limit = " + 
domainResourceLimit +
+                    ", Current Domain Resource Amount = " + 
currentDomainResourceCount +
+                    ", Requested Resource Amount = " + numResources + ".";
+
+                if(s_logger.isDebugEnabled()) {
+                    s_logger.debug("Checking if" + messageSuffix);
+                }
+
+                if (domainResourceLimit != Resource.RESOURCE_UNLIMITED && 
requestedDomainResourceCount > domainResourceLimit) {
+                    String message = "Maximum" + messageSuffix;
+                    ResourceAllocationException e = new 
ResourceAllocationException(message, type);
+                    s_logger.error(message, e);
+                    throw e;
+                }
+            }
+            domainId = domain.getParent();
+        }
+    }
+
+    private void checkAccountResourceLimit(final Account account, final 
Project project, final ResourceType type, long numResources) throws 
ResourceAllocationException {
+        // Check account limits
+        long accountResourceLimit = 
findCorrectResourceLimitForAccount(account, type);
+        long currentResourceCount = 
_resourceCountDao.getResourceCount(account.getId(), ResourceOwnerType.Account, 
type);
+        long requestedResourceCount = currentResourceCount + numResources;
+        String messageSuffix = " amount of resources of Type = '" + type + "' 
for " +
+            (project == null ? "Account Name = " + account.getAccountName() : 
"Project Name = " + project.getName()) +
+            " in Domain Id = " + account.getDomainId() +
+            " is exceeded: Account Resource Limit = " + accountResourceLimit +
+            ", Current Account Resource Amount = " + currentResourceCount +
+            ", Requested Resource Amount = " + numResources + ".";
+
+        if(s_logger.isDebugEnabled()) {
+            s_logger.debug("Checking if" + messageSuffix);
+        }
+
+        if (accountResourceLimit != Resource.RESOURCE_UNLIMITED && 
requestedResourceCount > accountResourceLimit) {
+            String message = "Maximum" + messageSuffix;
+            ResourceAllocationException e = new 
ResourceAllocationException(message, type);
+            s_logger.error(message, e);
+            throw e;
+        }
+    }
+
+    private List<ResourceCountVO> lockAccountAndOwnerDomainRows(long 
accountId, final ResourceType type) {
+        Set<Long> rowIdsToLock = 
_resourceCountDao.listAllRowsToUpdate(accountId, ResourceOwnerType.Account, 
type);
+        SearchCriteria<ResourceCountVO> sc = ResourceCountSearch.create();
+        sc.setParameters("id", rowIdsToLock.toArray());
+        return _resourceCountDao.lockRows(sc, null, true);
+    }
+
+    private List<ResourceCountVO> lockDomainRows(long domainId, final 
ResourceType type) {
+        Set<Long> rowIdsToLock = 
_resourceCountDao.listAllRowsToUpdate(domainId, ResourceOwnerType.Domain, type);
+        SearchCriteria<ResourceCountVO> sc = ResourceCountSearch.create();
+        sc.setParameters("id", rowIdsToLock.toArray());
+        return _resourceCountDao.lockRows(sc, null, true);
+    }
+
     public long findDefaultResourceLimitForDomain(ResourceType resourceType) {
         Long resourceLimit = null;
         resourceLimit = domainResourceLimitMap.get(resourceType);
         if (resourceLimit != null && (resourceType == 
ResourceType.primary_storage || resourceType == 
ResourceType.secondary_storage)) {
-                resourceLimit = resourceLimit * ResourceType.bytesToGiB;
+            resourceLimit = resourceLimit * ResourceType.bytesToGiB;
         } else {
             resourceLimit = Long.valueOf(Resource.RESOURCE_UNLIMITED);
         }
@@ -440,51 +515,14 @@ public void checkResourceLimit(final Account account, 
final ResourceType type, l
         Transaction.execute(new 
TransactionCallbackWithExceptionNoReturn<ResourceAllocationException>() {
             @Override
             public void doInTransactionWithoutResult(TransactionStatus status) 
throws ResourceAllocationException {
-            // Lock all rows first so nobody else can read it
-            Set<Long> rowIdsToLock = 
_resourceCountDao.listAllRowsToUpdate(account.getId(), 
ResourceOwnerType.Account, type);
-            SearchCriteria<ResourceCountVO> sc = ResourceCountSearch.create();
-            sc.setParameters("id", rowIdsToLock.toArray());
-            _resourceCountDao.lockRows(sc, null, true);
-
-            // Check account limits
-            long accountLimit = findCorrectResourceLimitForAccount(account, 
type);
-            long potentialCount = 
_resourceCountDao.getResourceCount(account.getId(), ResourceOwnerType.Account, 
type) + numResources;
-            if (accountLimit != Resource.RESOURCE_UNLIMITED && potentialCount 
> accountLimit) {
-                    String message =
-                        "Maximum number of resources of type '" + type + "' 
for account name=" + account.getAccountName() + " in domain id=" + 
account.getDomainId() +
-                            " has been exceeded.";
-                    if (projectFinal != null) {
-                        message =
-                            "Maximum number of resources of type '" + type + 
"' for project name=" + projectFinal.getName() + " in domain id=" + 
account.getDomainId() +
-                                " has been exceeded.";
-                }
-                ResourceAllocationException e=  new 
ResourceAllocationException(message, type);;
-                s_logger.error(message, e);
-                throw e;
-            }
-
-            // check all domains in the account's domain hierarchy
-            Long domainId = null;
-                if (projectFinal != null) {
-                    domainId = projectFinal.getDomainId();
-            } else {
-                domainId = account.getDomainId();
-            }
-
-            while (domainId != null) {
-                DomainVO domain = _domainDao.findById(domainId);
-                // no limit check if it is ROOT domain
-                if (domainId != Domain.ROOT_DOMAIN) {
-                    long domainLimit = 
findCorrectResourceLimitForDomain(domain, type);
-                    long domainCount = 
_resourceCountDao.getResourceCount(domainId, ResourceOwnerType.Domain, type) + 
numResources;
-                    if (domainLimit != Resource.RESOURCE_UNLIMITED && 
domainCount > domainLimit) {
-                        throw new ResourceAllocationException("Maximum number 
of resources of type '" + type + "' for domain id=" + domainId + " has been 
exceeded.", type);
-                    }
-                }
-                domainId = domain.getParent();
+                // Lock all rows first so nobody else can read it
+                lockAccountAndOwnerDomainRows(account.getId(), type);
+                // Check account limits
+                checkAccountResourceLimit(account, projectFinal, type, 
numResources);
+                // check all domains in the account's domain hierarchy
+                checkDomainResourceLimit(account, projectFinal, type, 
numResources);
             }
-        }
-        });
+            });
     }
 
     @Override
@@ -650,7 +688,7 @@ public ResourceLimitVO updateResourceLimit(Long accountId, 
Long domainId, Intege
 
         //Convert max storage size from GiB to bytes
         if ((resourceType == ResourceType.primary_storage || resourceType == 
ResourceType.secondary_storage) && max >= 0) {
-            max = max * ResourceType.bytesToGiB;
+            max *= ResourceType.bytesToGiB;
         }
 
         ResourceOwnerType ownerType = null;
@@ -780,28 +818,25 @@ public ResourceLimitVO updateResourceLimit(Long 
accountId, Long domainId, Intege
 
     @DB
     protected boolean updateResourceCountForAccount(final long accountId, 
final ResourceType type, final boolean increment, final long delta) {
+        if(s_logger.isDebugEnabled()) {
+            s_logger.debug("Updating resource Type = " + type + " count for 
Account = " + accountId +
+                           " Operation = " + (increment ? "increasing" : 
"decreasing") + " Amount = " + delta);
+        }
         try {
             return Transaction.execute(new TransactionCallback<Boolean>() {
-                @Override
-                public Boolean doInTransaction(TransactionStatus status) {
-                    boolean result = true;
-            Set<Long> rowsToLock = 
_resourceCountDao.listAllRowsToUpdate(accountId, ResourceOwnerType.Account, 
type);
-
-            // Lock rows first
-            SearchCriteria<ResourceCountVO> sc = ResourceCountSearch.create();
-            sc.setParameters("id", rowsToLock.toArray());
-            List<ResourceCountVO> rowsToUpdate = 
_resourceCountDao.lockRows(sc, null, true);
-
-            for (ResourceCountVO rowToUpdate : rowsToUpdate) {
-                if (!_resourceCountDao.updateById(rowToUpdate.getId(), 
increment, delta)) {
-                    s_logger.trace("Unable to update resource count for the 
row " + rowToUpdate);
-                    result = false;
-                }
-            }
-
-                    return result;
-                }
-            });
+                    @Override
+                    public Boolean doInTransaction(TransactionStatus status) {
+                        boolean result = true;
+                        List<ResourceCountVO> rowsToUpdate = 
lockAccountAndOwnerDomainRows(accountId, type);
+                        for (ResourceCountVO rowToUpdate : rowsToUpdate) {
+                            if 
(!_resourceCountDao.updateById(rowToUpdate.getId(), increment, delta)) {
+                                s_logger.trace("Unable to update resource 
count for the row " + rowToUpdate);
+                                result = false;
+                            }
+                        }
+                        return result;
+                    }
+                });
         } catch (Exception ex) {
             s_logger.error("Failed to update resource count for account id=" + 
accountId);
             return false;
@@ -811,114 +846,102 @@ public Boolean doInTransaction(TransactionStatus 
status) {
     @DB
     protected long recalculateDomainResourceCount(final long domainId, final 
ResourceType type) {
         return Transaction.execute(new TransactionCallback<Long>() {
-            @Override
-            public Long doInTransaction(TransactionStatus status) {
-        long newCount = 0;
-
-            // Lock all rows first so nobody else can read it
-            Set<Long> rowIdsToLock = 
_resourceCountDao.listAllRowsToUpdate(domainId, ResourceOwnerType.Domain, type);
-            SearchCriteria<ResourceCountVO> sc = ResourceCountSearch.create();
-            sc.setParameters("id", rowIdsToLock.toArray());
-            _resourceCountDao.lockRows(sc, null, true);
+                @Override
+                public Long doInTransaction(TransactionStatus status) {
+                    long newResourceCount = 0;
+                    lockDomainRows(domainId, type);
+                    ResourceCountVO domainRC = 
_resourceCountDao.findByOwnerAndType(domainId, ResourceOwnerType.Domain, type);
+                    long oldResourceCount = domainRC.getCount();
+
+                    List<DomainVO> domainChildren = 
_domainDao.findImmediateChildrenForParent(domainId);
+                    // for each child domain update the resource count
+                    if (type.supportsOwner(ResourceOwnerType.Domain)) {
 
-            ResourceCountVO domainRC = 
_resourceCountDao.findByOwnerAndType(domainId, ResourceOwnerType.Domain, type);
-            long oldCount = domainRC.getCount();
+                        // calculate project count here
+                        if (type == ResourceType.project) {
+                            newResourceCount += 
_projectDao.countProjectsForDomain(domainId);
+                        }
 
-            List<DomainVO> domainChildren = 
_domainDao.findImmediateChildrenForParent(domainId);
-            // for each child domain update the resource count
-            if (type.supportsOwner(ResourceOwnerType.Domain)) {
+                        for (DomainVO childDomain : domainChildren) {
+                            long childDomainResourceCount = 
recalculateDomainResourceCount(childDomain.getId(), type);
+                            newResourceCount += childDomainResourceCount; // 
add the child domain count to parent domain count
+                        }
+                    }
 
-                // calculate project count here
-                if (type == ResourceType.project) {
-                    newCount = newCount + 
_projectDao.countProjectsForDomain(domainId);
-                }
+                    if (type.supportsOwner(ResourceOwnerType.Account)) {
+                        List<AccountVO> accounts = 
_accountDao.findActiveAccountsForDomain(domainId);
+                        for (AccountVO account : accounts) {
+                            long accountResourceCount = 
recalculateAccountResourceCount(account.getId(), type);
+                            newResourceCount += accountResourceCount; // add 
account's resource count to parent domain count
+                        }
+                    }
+                    _resourceCountDao.setResourceCount(domainId, 
ResourceOwnerType.Domain, type, newResourceCount);
 
-                for (DomainVO domainChild : domainChildren) {
-                    long domainCount = 
recalculateDomainResourceCount(domainChild.getId(), type);
-                    newCount = newCount + domainCount; // add the child domain 
count to parent domain count
-                }
-            }
+                    if (oldResourceCount != newResourceCount) {
+                        s_logger.warn("Discrepency in the resource count has 
been detected " + "(original count = " + oldResourceCount +
+                                      " correct count = " + newResourceCount + 
") for Type = " + type +
+                                      " for Domain ID = " + domainId + " is 
fixed during resource count recalculation.");
+                    }
 
-            if (type.supportsOwner(ResourceOwnerType.Account)) {
-                List<AccountVO> accounts = 
_accountDao.findActiveAccountsForDomain(domainId);
-                for (AccountVO account : accounts) {
-                    long accountCount = 
recalculateAccountResourceCount(account.getId(), type);
-                    newCount = newCount + accountCount; // add account's 
resource count to parent domain count
+                    return newResourceCount;
                 }
-            }
-            _resourceCountDao.setResourceCount(domainId, 
ResourceOwnerType.Domain, type, newCount);
-
-            if (oldCount != newCount) {
-                    s_logger.info("Discrepency in the resource count " + 
"(original count=" + oldCount + " correct count = " + newCount + ") for type " 
+ type +
-                        " for domain ID " + domainId + " is fixed during 
resource count recalculation.");
-            }
-
-        return newCount;
-    }
-        });
+            });
     }
 
     @DB
     protected long recalculateAccountResourceCount(final long accountId, final 
ResourceType type) {
         Long newCount = Transaction.execute(new TransactionCallback<Long>() {
-            @Override
-            public Long doInTransaction(TransactionStatus status) {
-        Long newCount = null;
-
-        // this lock guards against the updates to user_vm, volume, snapshot, 
public _ip and template table
-        // as any resource creation precedes with the resourceLimitExceeded 
check which needs this lock too
-        Set rowIdsToLock = _resourceCountDao.listAllRowsToUpdate(accountId, 
Resource.ResourceOwnerType.Account, type);
-        SearchCriteria<ResourceCountVO> sc = ResourceCountSearch.create();
-        sc.setParameters("id", rowIdsToLock.toArray());
-        _resourceCountDao.lockRows(sc, null, true);
-
-        ResourceCountVO accountRC = 
_resourceCountDao.findByOwnerAndType(accountId, ResourceOwnerType.Account, 
type);
-        long oldCount = 0;
-        if (accountRC != null) {
-            oldCount = accountRC.getCount();
-        }
-
-        if (type == Resource.ResourceType.user_vm) {
-            newCount = _userVmDao.countAllocatedVMsForAccount(accountId);
-        } else if (type == Resource.ResourceType.volume) {
-            newCount = _volumeDao.countAllocatedVolumesForAccount(accountId);
-            long virtualRouterCount = 
_vmDao.findIdsOfAllocatedVirtualRoutersForAccount(accountId).size();
-            newCount = newCount - virtualRouterCount; // don't count the 
volumes of virtual router
-        } else if (type == Resource.ResourceType.snapshot) {
-            newCount = _snapshotDao.countSnapshotsForAccount(accountId);
-        } else if (type == Resource.ResourceType.public_ip) {
-            newCount = calculatePublicIpForAccount(accountId);
-        } else if (type == Resource.ResourceType.template) {
-            newCount = _vmTemplateDao.countTemplatesForAccount(accountId);
-        } else if (type == Resource.ResourceType.project) {
-            newCount = _projectAccountDao.countByAccountIdAndRole(accountId, 
Role.Admin);
-        } else if (type == Resource.ResourceType.network) {
-            newCount = _networkDao.countNetworksUserCanCreate(accountId);
-        } else if (type == Resource.ResourceType.vpc) {
-            newCount = _vpcDao.countByAccountId(accountId);
-        } else if (type == Resource.ResourceType.cpu) {
-            newCount = countCpusForAccount(accountId);
-        } else if (type == Resource.ResourceType.memory) {
-            newCount = calculateMemoryForAccount(accountId);
-        } else if (type == Resource.ResourceType.primary_storage) {
-            List<Long> virtualRouters = 
_vmDao.findIdsOfAllocatedVirtualRoutersForAccount(accountId);
-            newCount = _volumeDao.primaryStorageUsedForAccount(accountId, 
virtualRouters);
-        } else if (type == Resource.ResourceType.secondary_storage) {
-            newCount = calculateSecondaryStorageForAccount(accountId);
-        } else {
-            throw new InvalidParameterValueException("Unsupported resource 
type " + type);
-        }
-        _resourceCountDao.setResourceCount(accountId, 
ResourceOwnerType.Account, type, (newCount == null) ? 0 : newCount.longValue());
-
-                // No need to log message for primary and secondary storage 
because both are recalculating the resource count which will not lead to any 
discrepancy.
-                if (!Long.valueOf(oldCount).equals(newCount) && (type != 
Resource.ResourceType.primary_storage && type != 
Resource.ResourceType.secondary_storage)) {
-                    s_logger.info("Discrepency in the resource count " + 
"(original count=" + oldCount + " correct count = " + newCount + ") for type " 
+ type +
-                        " for account ID " + accountId + " is fixed during 
resource count recalculation.");
-        }
-
-                return newCount;
-            }
-        });
+                @Override
+                public Long doInTransaction(TransactionStatus status) {
+                    Long newCount = null;
+                    lockAccountAndOwnerDomainRows(accountId, type);
+                    ResourceCountVO accountRC = 
_resourceCountDao.findByOwnerAndType(accountId, ResourceOwnerType.Account, 
type);
+                    long oldCount = 0;
+                    if (accountRC != null)
+                        oldCount = accountRC.getCount();
+
+                    if (type == Resource.ResourceType.user_vm) {
+                        newCount = 
_userVmDao.countAllocatedVMsForAccount(accountId);
+                    } else if (type == Resource.ResourceType.volume) {
+                        newCount = 
_volumeDao.countAllocatedVolumesForAccount(accountId);
+                        long virtualRouterCount = 
_vmDao.findIdsOfAllocatedVirtualRoutersForAccount(accountId).size();
+                        newCount = newCount - virtualRouterCount; // don't 
count the volumes of virtual router
+                    } else if (type == Resource.ResourceType.snapshot) {
+                        newCount = 
_snapshotDao.countSnapshotsForAccount(accountId);
+                    } else if (type == Resource.ResourceType.public_ip) {
+                        newCount = calculatePublicIpForAccount(accountId);
+                    } else if (type == Resource.ResourceType.template) {
+                        newCount = 
_vmTemplateDao.countTemplatesForAccount(accountId);
+                    } else if (type == Resource.ResourceType.project) {
+                        newCount = 
_projectAccountDao.countByAccountIdAndRole(accountId, Role.Admin);
+                    } else if (type == Resource.ResourceType.network) {
+                        newCount = 
_networkDao.countNetworksUserCanCreate(accountId);
+                    } else if (type == Resource.ResourceType.vpc) {
+                        newCount = _vpcDao.countByAccountId(accountId);
+                    } else if (type == Resource.ResourceType.cpu) {
+                        newCount = countCpusForAccount(accountId);
+                    } else if (type == Resource.ResourceType.memory) {
+                        newCount = calculateMemoryForAccount(accountId);
+                    } else if (type == Resource.ResourceType.primary_storage) {
+                        List<Long> virtualRouters = 
_vmDao.findIdsOfAllocatedVirtualRoutersForAccount(accountId);
+                        newCount = 
_volumeDao.primaryStorageUsedForAccount(accountId, virtualRouters);
+                    } else if (type == 
Resource.ResourceType.secondary_storage) {
+                        newCount = 
calculateSecondaryStorageForAccount(accountId);
+                    } else {
+                        throw new InvalidParameterValueException("Unsupported 
resource type " + type);
+                    }
+                    _resourceCountDao.setResourceCount(accountId, 
ResourceOwnerType.Account, type, (newCount == null) ? 0 : newCount.longValue());
+
+                    // No need to log message for primary and secondary 
storage because both are recalculating the
+                    // resource count which will not lead to any discrepancy.
+                    if (!Long.valueOf(oldCount).equals(newCount) &&
+                        (type != Resource.ResourceType.primary_storage && type 
!= Resource.ResourceType.secondary_storage)) {
+                        s_logger.warn("Discrepency in the resource count " + 
"(original count=" + oldCount + " correct count = " + newCount + ") for type " 
+ type +
+                                      " for account ID " + accountId + " is 
fixed during resource count recalculation.");
+                    }
+                    return newCount;
+                }
+            });
 
         return (newCount == null) ? 0 : newCount.longValue();
     }
@@ -1082,7 +1105,7 @@ public ResourceCountCheckTask() {
 
         @Override
         protected void runInContext() {
-            s_logger.info("Running resource count check periodic task");
+            s_logger.info("Started resource counters recalculation periodic 
task.");
             List<DomainVO> domains = 
_domainDao.findImmediateChildrenForParent(Domain.ROOT_DOMAIN);
 
             // recalculateDomainResourceCount will take care of re-calculation 
of resource counts for sub-domains
diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java 
b/server/src/com/cloud/storage/VolumeApiServiceImpl.java
index 37b72a68d14..224fa78370f 100644
--- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java
+++ b/server/src/com/cloud/storage/VolumeApiServiceImpl.java
@@ -816,7 +816,7 @@ public VolumeVO createVolume(CreateVolumeCmd cmd) {
             if (!created) {
                 s_logger.trace("Decrementing volume resource count for account 
id=" + volume.getAccountId() + " as volume failed to create on the backend");
                 
_resourceLimitMgr.decrementResourceCount(volume.getAccountId(), 
ResourceType.volume, cmd.getDisplayVolume());
-                
_resourceLimitMgr.recalculateResourceCount(volume.getAccountId(), 
volume.getDomainId(), ResourceType.primary_storage.getOrdinal());
+                
_resourceLimitMgr.decrementResourceCount(volume.getAccountId(), 
ResourceType.primary_storage, cmd.getDisplayVolume(), new 
Long(volume.getSize()));
             }
         }
     }
@@ -1262,6 +1262,7 @@ public boolean deleteVolume(long volumeId, Account 
caller) throws ConcurrentOper
                 if (instanceId == null || 
(vmInstance.getType().equals(VirtualMachine.Type.User))) {
                     // Decrement the resource count for volumes and primary 
storage belonging user VM's only
                     
_resourceLimitMgr.decrementResourceCount(volume.getAccountId(), 
ResourceType.volume, volume.isDisplayVolume());
+                    
_resourceLimitMgr.decrementResourceCount(volume.getAccountId(), 
ResourceType.primary_storage, volume.isDisplayVolume(), new 
Long(volume.getSize()));
                 }
             }
             // Mark volume as removed if volume has not been created on 
primary or secondary
@@ -1277,7 +1278,8 @@ public boolean deleteVolume(long volumeId, Account 
caller) throws ConcurrentOper
                 AsyncCallFuture<VolumeApiResult> future = 
volService.expungeVolumeAsync(volOnPrimary);
                 future.get();
                 //decrement primary storage count
-                
_resourceLimitMgr.recalculateResourceCount(volume.getAccountId(), 
volume.getDomainId(), ResourceType.primary_storage.getOrdinal());
+                
_resourceLimitMgr.decrementResourceCount(volOnPrimary.getAccountId(), 
ResourceType.volume, volOnPrimary.isDisplayVolume());
+                
_resourceLimitMgr.decrementResourceCount(volOnPrimary.getAccountId(), 
ResourceType.primary_storage, volOnPrimary.isDisplayVolume(), new 
Long(volOnPrimary.getSize()));
             }
             // expunge volume from secondary if volume is on image store
             VolumeInfo volOnSecondary = volFactory.getVolume(volume.getId(), 
DataStoreRole.Image);
@@ -1286,7 +1288,7 @@ public boolean deleteVolume(long volumeId, Account 
caller) throws ConcurrentOper
                 AsyncCallFuture<VolumeApiResult> future2 = 
volService.expungeVolumeAsync(volOnSecondary);
                 future2.get();
                 //decrement secondary storage count
-                
_resourceLimitMgr.recalculateResourceCount(volume.getAccountId(), 
volume.getDomainId(), ResourceType.secondary_storage.getOrdinal());
+                
_resourceLimitMgr.decrementResourceCount(volOnSecondary.getAccountId(), 
ResourceType.secondary_storage, new Long(volOnSecondary.getSize()));
             }
             // delete all cache entries for this volume
             List<VolumeInfo> cacheVols = 
volFactory.listVolumeOnCache(volume.getId());
diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 
b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
index 1f2ca6421be..5e981464672 100755
--- a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
+++ b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
@@ -306,8 +306,7 @@ public Snapshot revertSnapshot(Long snapshotId) {
         boolean result = snapshotStrategy.revertSnapshot(snapshotInfo);
         if (result) {
             // update volume size and primary storage count
-            _resourceLimitMgr.decrementResourceCount(snapshot.getAccountId(), 
ResourceType.primary_storage,
-                    new Long(volume.getSize() - snapshot.getSize()));
+            _resourceLimitMgr.decrementResourceCount(snapshot.getAccountId(), 
ResourceType.primary_storage, new Long(volume.getSize() - snapshot.getSize()));
             volume.setSize(snapshot.getSize());
             _volsDao.update(volume.getId(), volume);
             return snapshotInfo;
diff --git a/server/src/com/cloud/vm/UserVmManagerImpl.java 
b/server/src/com/cloud/vm/UserVmManagerImpl.java
index df9130c682d..6ec59acc4f3 100644
--- a/server/src/com/cloud/vm/UserVmManagerImpl.java
+++ b/server/src/com/cloud/vm/UserVmManagerImpl.java
@@ -2129,7 +2129,7 @@ public boolean expunge(UserVmVO vm, long callerUserId, 
Account caller) {
             // Update Resource count
             if (vm.getAccountId() != Account.ACCOUNT_ID_SYSTEM && 
!rootVol.isEmpty()) {
                 _resourceLimitMgr.decrementResourceCount(vm.getAccountId(), 
ResourceType.volume);
-                _resourceLimitMgr.recalculateResourceCount(vm.getAccountId(), 
vm.getDomainId(), ResourceType.primary_storage.getOrdinal());
+                _resourceLimitMgr.decrementResourceCount(vm.getAccountId(), 
ResourceType.primary_storage, new Long(rootVol.get(0).getSize()));
             }
 
             // Only if vm is not expunged already, cleanup it's resources
@@ -5500,41 +5500,43 @@ public UserVm moveVMToUser(final AssignVMCmd cmd) 
throws ResourceAllocationExcep
         Transaction.execute(new TransactionCallbackNoReturn() {
             @Override
             public void doInTransactionWithoutResult(TransactionStatus status) 
{
-        //generate destroy vm event for usage
-                UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_DESTROY, 
vm.getAccountId(), vm.getDataCenterId(), vm.getId(), vm.getHostName(), 
vm.getServiceOfferingId(),
-                        vm.getTemplateId(), vm.getHypervisorType().toString(), 
VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
-
-        // update resource counts for old account
+                //generate destroy vm event for usage
+                UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_DESTROY, 
vm.getAccountId(), vm.getDataCenterId(),
+                                                  vm.getId(), 
vm.getHostName(), vm.getServiceOfferingId(), vm.getTemplateId(),
+                                                  
vm.getHypervisorType().toString(), VirtualMachine.class.getName(), 
vm.getUuid(), vm.isDisplayVm());
+                // update resource counts for old account
                 resourceCountDecrement(oldAccount.getAccountId(), 
vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
 
-        // OWNERSHIP STEP 1: update the vm owner
-        vm.setAccountId(newAccount.getAccountId());
-        vm.setDomainId(cmd.getDomainId());
-        _vmDao.persist(vm);
+                // OWNERSHIP STEP 1: update the vm owner
+                vm.setAccountId(newAccount.getAccountId());
+                vm.setDomainId(cmd.getDomainId());
+                _vmDao.persist(vm);
 
-        // OS 2: update volume
-        for (VolumeVO volume : volumes) {
-            UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_DELETE, 
volume.getAccountId(), volume.getDataCenterId(), volume.getId(), 
volume.getName(),
-                    Volume.class.getName(), volume.getUuid(), 
volume.isDisplayVolume());
-            
_resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), 
ResourceType.volume);
-            
_resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), 
ResourceType.primary_storage, new Long(volume.getSize()));
-            volume.setAccountId(newAccount.getAccountId());
-            volume.setDomainId(newAccount.getDomainId());
-            _volsDao.persist(volume);
-            
_resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), 
ResourceType.volume);
-            
_resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), 
ResourceType.primary_storage, new Long(volume.getSize()));
-            UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_CREATE, 
volume.getAccountId(), volume.getDataCenterId(), volume.getId(), 
volume.getName(),
-                    volume.getDiskOfferingId(), volume.getTemplateId(), 
volume.getSize(), Volume.class.getName(), volume.getUuid(), 
volume.isDisplayVolume());
-        }
-
-        //update resource count of new account
+                // OS 2: update volume
+                for (VolumeVO volume : volumes) {
+                    
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_DELETE, 
volume.getAccountId(), volume.getDataCenterId(), volume.getId(), 
volume.getName(),
+                                                      Volume.class.getName(), 
volume.getUuid(), volume.isDisplayVolume());
+                    
_resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), 
ResourceType.volume);
+                    
_resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), 
ResourceType.primary_storage, new Long(volume.getSize()));
+                    volume.setAccountId(newAccount.getAccountId());
+                    volume.setDomainId(newAccount.getDomainId());
+                    _volsDao.persist(volume);
+                    
_resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), 
ResourceType.volume);
+                    
_resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), 
ResourceType.primary_storage, new Long(volume.getSize()));
+                    
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_CREATE, 
volume.getAccountId(), volume.getDataCenterId(), volume.getId(), 
volume.getName(),
+                                                      
volume.getDiskOfferingId(), volume.getTemplateId(), volume.getSize(), 
Volume.class.getName(),
+                                                      volume.getUuid(), 
volume.isDisplayVolume());
+                }
+
+                //update resource count of new account
                 resourceCountIncrement(newAccount.getAccountId(), 
vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
 
-        //generate usage events to account for this change
-                UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_CREATE, 
vm.getAccountId(), vm.getDataCenterId(), vm.getId(), vm.getHostName(), 
vm.getServiceOfferingId(),
-                        vm.getTemplateId(), vm.getHypervisorType().toString(), 
VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
+                //generate usage events to account for this change
+                UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_CREATE, 
vm.getAccountId(), vm.getDataCenterId(), vm.getId(),
+                                                  vm.getHostName(), 
vm.getServiceOfferingId(), vm.getTemplateId(), 
vm.getHypervisorType().toString(),
+                                                  
VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
             }
-        });
+            });
 
         VirtualMachine vmoi = _itMgr.findById(vm.getId());
         VirtualMachineProfileImpl vmOldProfile = new 
VirtualMachineProfileImpl(vmoi);


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> Resource Accounting for primary storage is Broken
> -------------------------------------------------
>
>                 Key: CLOUDSTACK-10188
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-10188
>             Project: CloudStack
>          Issue Type: Bug
>      Security Level: Public(Anyone can view this level - this is the 
> default.) 
>    Affects Versions: 4.9.0, 4.10.0.0, 4.11.0.0
>            Reporter: Ivan Kudryavtsev
>
> During storage expunge domain resource statistics for primary storage space 
> resource counter is not updated for domain. This leads to the situation when 
> domain resource statistics for primary storage is overfilled (statistics only 
> increase but not decrease).
> Global scheduled task resourcecount.check.interval > 0 provides a workaround 
> but not fixes the problem truly because when accounts inside domains use 
> primary_storage allocation/deallocation intensively it leads to service block 
> of operation.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Reply via email to