Skip to content

CLOUDSTACK-9473: storage pool capacity check when volume is resized or migrated #2829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yadvr merged 2 commits into from
Sep 7, 2018
Merged

CLOUDSTACK-9473: storage pool capacity check when volume is resized or migrated #2829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2a3410
CLOUDSTACK-9473: storage pool capacity check when volume is resized o…
Jan 2, 2017
3921420
fix review comments
yadvr Sep 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions engine/components-api/src/com/cloud/storage/StorageManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ public interface StorageManager extends StorageService {
*/
boolean storagePoolHasEnoughSpace(List<Volume> volume, StoragePool pool, Long clusterId);

boolean storagePoolHasEnoughSpaceForResize(StoragePool pool, long currentSize, long newSiz);

boolean registerHostListener(String providerUuid, HypervisorHostListener listener);

void connectHostToSharedPool(long hostId, long poolId) throws StorageUnavailableException, StorageConflictException;
Expand Down
50 changes: 40 additions & 10 deletions server/src/com/cloud/storage/StorageManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1788,8 +1788,8 @@ public boolean storagePoolHasEnoughSpace(List<Volume> volumes, StoragePool pool,
if (s_logger.isDebugEnabled()) {
s_logger.debug("Destination pool id: " + pool.getId());
}

StoragePoolVO poolVO = _storagePoolDao.findById(pool.getId());
// allocated space includes templates
final StoragePoolVO poolVO = _storagePoolDao.findById(pool.getId());
long allocatedSizeWithTemplate = _capacityMgr.getAllocatedPoolCapacity(poolVO, null);
long totalAskingSize = 0;

Expand Down Expand Up @@ -1830,12 +1830,37 @@ public boolean storagePoolHasEnoughSpace(List<Volume> volumes, StoragePool pool,
}
}

return checkPoolforSpace(pool, allocatedSizeWithTemplate, totalAskingSize);
}

@Override
public boolean storagePoolHasEnoughSpaceForResize(StoragePool pool, long currentSize, long newSiz) {
if (!checkUsagedSpace(pool)) {
return false;
}
if (s_logger.isDebugEnabled()) {
Copy link
Copy Markdown
Contributor

@wido wido Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this if really needed? Can't you just log to debug and let the filter of log4j handle it?

Copy link
Copy Markdown
Member Author

@yadvr yadvr Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If log4j does not have debug enabled, then the string concat operation will still be performed by the JVM only when the string is passed to the debug() method it will be ignored. So, it depends if we can afford the CPU/time spent on the operation.

Copy link
Copy Markdown
Member

@rafaelweingartner rafaelweingartner Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can afford this tiny bit of extra CPU.

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as a matter of principle we should alwas add the check. You never no for what reason someone will alter the message. I like using String.format to construct the message, btw, and then pass the single resulting string to the logger

Copy link
Copy Markdown
Member

@rafaelweingartner rafaelweingartner Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What principle?

I also use String.format, and I do not see the problem.

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"bla" + "blob" is a costly operation, basically to memcpy()s. String.format() as well in comparison. We don't want those operations performed if we don't end up using the result in a log statement. so the boolean check potentially saves us a lot of cpu cycles, especially in the debug() and trace() cases.

Copy link
Copy Markdown
Member

@rafaelweingartner rafaelweingartner Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I do know that. However, we should ask ourselves, if the addition of a if conditions (it will clutter the code) is worth the CPU cycles. For me, I would not trade readability for this small CPU cost in a web system (ACS is a at the end of the day a Java web application).

s_logger.debug("Destination pool id: " + pool.getId());
}
long totalAskingSize = newSiz - currentSize;

if (totalAskingSize <= 0) {
Comment thread
DaanHoogland marked this conversation as resolved.
return true;
} else {
final StoragePoolVO poolVO = _storagePoolDao.findById(pool.getId());
final long allocatedSizeWithTemplate = _capacityMgr.getAllocatedPoolCapacity(poolVO, null);
return checkPoolforSpace(pool, allocatedSizeWithTemplate, totalAskingSize);
}
}

private boolean checkPoolforSpace(StoragePool pool, long allocatedSizeWithTemplate, long totalAskingSize) {
// allocated space includes templates
StoragePoolVO poolVO = _storagePoolDao.findById(pool.getId());

long totalOverProvCapacity;

if (pool.getPoolType().supportsOverProvisioning()) {
BigDecimal overProvFactor = getStorageOverProvisioningFactor(pool.getId());
totalOverProvCapacity = overProvFactor.multiply(new BigDecimal(pool.getCapacityBytes())).longValue();

s_logger.debug("Found storage pool " + poolVO.getName() + " of type " + pool.getPoolType().toString() + " with overprovisioning factor " + overProvFactor.toString());
s_logger.debug("Total over provisioned capacity calculated is " + overProvFactor + " * " + pool.getCapacityBytes());
} else {
Expand All @@ -1847,21 +1872,26 @@ public boolean storagePoolHasEnoughSpace(List<Volume> volumes, StoragePool pool,
s_logger.debug("Total capacity of the pool " + poolVO.getName() + " with ID " + pool.getId() + " is " + totalOverProvCapacity);

double storageAllocatedThreshold = CapacityManager.StorageAllocatedCapacityDisableThreshold.valueIn(pool.getDataCenterId());

s_logger.debug("Checking pool: " + pool.getId() + " for volume allocation " + volumes.toString() + ", maxSize : " + totalOverProvCapacity + ", totalAllocatedSize : "
+ allocatedSizeWithTemplate + ", askingSize : " + totalAskingSize + ", allocated disable threshold: " + storageAllocatedThreshold);
if (s_logger.isDebugEnabled()) {
Copy link
Copy Markdown
Member

@rafaelweingartner rafaelweingartner Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why checking before logging?

We are removing this from other places. Let's remove conditionals from our code base.

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please let's always check before logging any lower priority (below error)

Copy link
Copy Markdown
Member

@rafaelweingartner rafaelweingartner Sep 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see no real benefits of adding this log level checks in our context. On the other hand, I do see problems with readability of the code.

I cannot object though, if you want to proceed with this ;)

s_logger.debug("Checking pool: " + pool.getId() + " for storage allocation , maxSize : " + totalOverProvCapacity + ", totalAllocatedSize : " + allocatedSizeWithTemplate
+ ", askingSize : " + totalAskingSize + ", allocated disable threshold: " + storageAllocatedThreshold);
}

double usedPercentage = (allocatedSizeWithTemplate + totalAskingSize) / (double)(totalOverProvCapacity);

if (usedPercentage > storageAllocatedThreshold) {
s_logger.debug("Insufficient un-allocated capacity on: " + pool.getId() + " for volume allocation: " + volumes.toString() + " since its allocated percentage: " + usedPercentage
+ " has crossed the allocated pool.storage.allocated.capacity.disablethreshold: " + storageAllocatedThreshold + ", skipping this pool");
if (s_logger.isDebugEnabled()) {
s_logger.debug("Insufficient un-allocated capacity on: " + pool.getId() + " for storage allocation since its allocated percentage: " + usedPercentage
+ " has crossed the allocated pool.storage.allocated.capacity.disablethreshold: " + storageAllocatedThreshold + ", skipping this pool");
}
return false;
}

if (totalOverProvCapacity < (allocatedSizeWithTemplate + totalAskingSize)) {
s_logger.debug("Insufficient un-allocated capacity on: " + pool.getId() + " for volume allocation: " + volumes.toString() + ", not enough storage, maxSize : " + totalOverProvCapacity
+ ", totalAllocatedSize : " + allocatedSizeWithTemplate + ", askingSize : " + totalAskingSize);
if (s_logger.isDebugEnabled()) {
s_logger.debug("Insufficient un-allocated capacity on: " + pool.getId() + " for storage allocation, not enough storage, maxSize : " + totalOverProvCapacity
+ ", totalAllocatedSize : " + allocatedSizeWithTemplate + ", askingSize : " + totalAskingSize);
}
return false;
}

Expand Down
9 changes: 9 additions & 0 deletions server/src/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -1137,6 +1138,10 @@ private VolumeVO orchestrateResizeVolume(long volumeId, long currentSize, long n
UserVmVO userVm = _userVmDao.findById(volume.getInstanceId());
StoragePoolVO storagePool = _storagePoolDao.findById(volume.getPoolId());
boolean isManaged = storagePool.isManaged();

if (!storageMgr.storagePoolHasEnoughSpaceForResize(storagePool, currentSize, newSize)) {
throw new CloudRuntimeException("Storage pool " + storagePool.getName() + " does not have enough space to resize volume " + volume.getName());
}
/*
* get a list of hosts to send the commands to, try the system the
* associated vm is running on first, then the last known place it ran.
Expand Down Expand Up @@ -2061,6 +2066,10 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) {
throw new InvalidParameterValueException("Cannot migrate volume " + vol + "to the destination storage pool " + destPool.getName() + " as the storage pool is in maintenance mode.");
}

if (!storageMgr.storagePoolHasEnoughSpace(Collections.singletonList(vol), destPool)) {
throw new CloudRuntimeException("Storage pool " + destPool.getName() + " does not have enough space to migrate volume " + vol.getName());
}

if (_volumeMgr.volumeOnSharedStoragePool(vol)) {
if (destPool.isLocal()) {
throw new InvalidParameterValueException("Migration of volume from shared to local storage pool is not supported");
Expand Down