Skip to content

Fix hardcoded max data volumes when VM has been created but not started before #3494

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 5 commits into from
Jul 22, 2019
Merged

Fix hardcoded max data volumes when VM has been created but not started before #3494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c327286
Prevent NullPointerException when vm has not run before
nvazquez Jul 15, 2019
5e2c301
Add hypervisor default max volumes if vm has not run before
nvazquez Jul 15, 2019
5fa201c
Check database for default hypervisor entries instead of hardcoded
nvazquez Jul 16, 2019
6a33826
Fix failing test
nvazquez Jul 16, 2019
dd1b028
Reduce number of invokations
nvazquez Jul 16, 2019
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/schema/src/main/java/com/cloud/hypervisor/dao/HypervisorCapabilitiesDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,6 @@ public interface HypervisorCapabilitiesDao extends GenericDao<HypervisorCapabili
Integer getMaxHostsPerCluster(HypervisorType hypervisorType, String hypervisorVersion);

Boolean isVmSnapshotEnabled(HypervisorType hypervisorType, String hypervisorVersion);

List<HypervisorType> getHypervisorsWithDefaultEntries();
}
13 changes: 13 additions & 0 deletions engine/schema/src/main/java/com/cloud/hypervisor/dao/HypervisorCapabilitiesDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.
package com.cloud.hypervisor.dao;

import java.util.ArrayList;
import java.util.List;

import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -106,4 +107,16 @@ public Boolean isVmSnapshotEnabled(HypervisorType hypervisorType, String hypervi
HypervisorCapabilitiesVO result = getCapabilities(hypervisorType, hypervisorVersion);
return result.getVmSnapshotEnabled();
}

@Override
public List<HypervisorType> getHypervisorsWithDefaultEntries() {
Copy link
Contributor

@anuragaw anuragaw Jul 16, 2019

Choose a reason for hiding this comment

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

Is there a possibility of high frequency calls to this method? If so we can perhaps optimize query calls with some caching? If not LGTM

Copy link
Contributor Author

@nvazquez nvazquez Jul 16, 2019

Choose a reason for hiding this comment

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

Thanks @anuragaw, have refactored it to invoke this only once

SearchCriteria<HypervisorCapabilitiesVO> sc = HypervisorTypeAndVersionSearch.create();
sc.setParameters("hypervisorVersion", DEFAULT_VERSION);
List<HypervisorCapabilitiesVO> hypervisorCapabilitiesVOS = listBy(sc);
List<HypervisorType> hvs = new ArrayList<>();
for (HypervisorCapabilitiesVO capabilitiesVO : hypervisorCapabilitiesVOS) {
hvs.add(capabilitiesVO.getHypervisorType());
}
return hvs;
}
}
12 changes: 11 additions & 1 deletion server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic

private List<StoragePoolAllocator> _storagePoolAllocators;

private List<HypervisorType> supportingDefaultHV;

VmWorkJobHandlerProxy _jobHandlerProxy = new VmWorkJobHandlerProxy(this);

static final ConfigKey<Long> VmJobCheckInterval = new ConfigKey<Long>("Advanced", Long.class, "vm.job.check.interval", "3000", "Interval in milliseconds to check if the job is complete", false);
Expand Down Expand Up @@ -2861,7 +2863,9 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
}
}

verifyManagedStorage(volumeToAttachStoragePool.getId(), hostId);
if (volumeToAttachStoragePool != null) {
verifyManagedStorage(volumeToAttachStoragePool.getId(), hostId);
}

// volumeToAttachStoragePool should be null if the VM we are attaching the disk to has never been started before
DataStore dataStore = volumeToAttachStoragePool != null ? dataStoreMgr.getDataStore(volumeToAttachStoragePool.getId(), DataStoreRole.Primary) : null;
Expand Down Expand Up @@ -3007,6 +3011,11 @@ private int getMaxDataVolumesSupported(UserVmVO vm) {
if (host != null) {
_hostDao.loadDetails(host);
maxDataVolumesSupported = _hypervisorCapabilitiesDao.getMaxDataVolumesLimit(host.getHypervisorType(), host.getDetail("product_version"));
} else {
HypervisorType hypervisorType = vm.getHypervisorType();
if (hypervisorType != null && CollectionUtils.isNotEmpty(supportingDefaultHV) && supportingDefaultHV.contains(hypervisorType)) {
maxDataVolumesSupported = _hypervisorCapabilitiesDao.getMaxDataVolumesLimit(hypervisorType, "default");
Copy link
Member

@yadvr yadvr Jul 16, 2019

Choose a reason for hiding this comment

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

Could version specific values cause regression or bug?

Copy link
Contributor

@DaanHoogland DaanHoogland Jul 16, 2019

Choose a reason for hiding this comment

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

if these are lower the "default", yes @rhtyd. But that should never be the case, should it?

Copy link
Contributor Author

@nvazquez nvazquez Jul 16, 2019

Choose a reason for hiding this comment

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

Good point, I've verified any version contains max limit lower than the default for each hypervisor

}
}
if (maxDataVolumesSupported == null || maxDataVolumesSupported.intValue() <= 0) {
maxDataVolumesSupported = 6; // 6 data disks by default if nothing
Expand Down Expand Up @@ -3054,6 +3063,7 @@ private Long getDeviceId(UserVmVO vm, Long deviceId) {
public boolean configure(String name, Map<String, Object> params) {
String maxVolumeSizeInGbString = _configDao.getValue(Config.MaxVolumeSize.toString());
_maxVolumeSizeInGb = NumbersUtil.parseLong(maxVolumeSizeInGbString, 2000);
supportingDefaultHV = _hypervisorCapabilitiesDao.getHypervisorsWithDefaultEntries();
return true;
}

Expand Down
3 changes: 3 additions & 0 deletions server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.UUID;
import java.util.concurrent.ExecutionException;

import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd;
Expand Down Expand Up @@ -149,6 +150,8 @@ public class VolumeApiServiceImplTest {
private HostDao _hostDao;
@Mock
private StoragePoolTagsDao storagePoolTagsDao;
@Mock
private HypervisorCapabilitiesDao hypervisorCapabilitiesDao;

private DetachVolumeCmd detachCmd = new DetachVolumeCmd();
private Class<?> _detachCmdClass = detachCmd.getClass();
Expand Down