sureshanaparti commented on a change in pull request #5008:
URL: https://github.com/apache/cloudstack/pull/5008#discussion_r698999668
##########
File path: ui/public/locales/en.json
##########
@@ -1414,6 +1419,7 @@
"label.migrate.to.host": "Migrate to host",
"label.migrate.to.storage": "Migrate to storage",
"label.migrate.volume": "Migrate Volume",
+"message.migrate.volume.tooltip": "Volume can be migrated to any suitable
storagepool. Admin has to choose the appropriate disk offering to replace, that
supports the new storage pool",
Review comment:
```suggestion
"message.migrate.volume.tooltip": "Volume can be migrated to any suitable
storage pool. Admin has to choose the appropriate disk offering to replace,
that supports the new storage pool",
```
##########
File path: ui/public/locales/en.json
##########
@@ -844,6 +846,9 @@
"label.duration.in.sec": "Duration (in sec)",
"label.dynamicscalingenabled": "Dynamic Scaling Enabled",
"label.dynamicscalingenabled.tooltip": "VM can dynamically scale only when
dynamic scaling is enabled on template, service offering and global setting",
+"label.diskofferingstrictness": "Disk Offering Strictness",
+"label.disksizestrictness": "Disk Size Strictness",
+"label.iscomputeonly.offering": "Compute only Disk Offering",
Review comment:
```suggestion
"label.computeonly.offering": "Compute only Disk Offering",
```
##########
File path:
api/src/main/java/org/apache/cloudstack/api/command/user/volume/MigrateVolumeCmd.java
##########
@@ -74,6 +75,15 @@ public boolean isLiveMigrate() {
return (liveMigrate != null) ? liveMigrate : false;
}
+ public MigrateVolumeCmd() {
+
+ }
Review comment:
```suggestion
}
```
##########
File path:
api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java
##########
@@ -486,4 +504,35 @@ public Boolean getDynamicScalingEnabled() {
public void setDynamicScalingEnabled(Boolean dynamicScalingEnabled) {
this.dynamicScalingEnabled = dynamicScalingEnabled;
}
+
+ public Boolean getDiskOfferingStrictness() {
+ return diskOfferingStrictness;
+ }
+
+ public void setDiskOfferingStrictness(Boolean diskOfferingStrictness) {
+ this.diskOfferingStrictness = diskOfferingStrictness;
+ }
Review comment:
```suggestion
}
```
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java
##########
@@ -453,8 +453,14 @@ private Long
getImportingVMGuestOs(VirtualMachineConfigSummary configSummary) {
*/
private ServiceOfferingVO createServiceOfferingForVMImporting(Integer
cpus, Integer memory, Integer maxCpuUsage) {
String name = "Imported-" + cpus + "-" + memory;
- ServiceOfferingVO vo = new ServiceOfferingVO(name, cpus, memory,
maxCpuUsage, null, null, false, name, Storage.ProvisioningType.THIN, false,
false, null, false, Type.User,
+
+ DiskOfferingVO diskOfferingVO = new DiskOfferingVO(name, name,
Storage.ProvisioningType.THIN, false, null, false, false, true);
+ diskOfferingVO =
diskOfferingDao.persistDefaultDiskOffering(diskOfferingVO);
+
Review comment:
double white spaces
##########
File path:
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2501,14 +2501,23 @@ public ServiceOffering createServiceOffering(final
CreateServiceOfferingCmd cmd)
}
}
+ // Check that the the disk offering is specified
Review comment:
```suggestion
// Check that the disk offering is specified
```
##########
File path:
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2501,14 +2501,23 @@ public ServiceOffering createServiceOffering(final
CreateServiceOfferingCmd cmd)
}
}
+ // Check that the the disk offering is specified
Review comment:
```suggestion
// Check that the disk offering is specified
```
can remove this comment, as it is evident from code ?
##########
File path:
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2501,14 +2501,23 @@ public ServiceOffering createServiceOffering(final
CreateServiceOfferingCmd cmd)
}
}
+ // Check that the the disk offering is specified
+ final Long diskOfferingId = cmd.getDiskOfferingId();
+ if (diskOfferingId != null) {
+ DiskOfferingVO diskOffering =
_diskOfferingDao.findById(diskOfferingId);
+ if ((diskOffering == null) || diskOffering.getRemoved() != null ||
diskOffering.isComputeOnly()) {
Review comment:
`diskOffering.getRemoved()` is null all time, may not be required in the
cond.
##########
File path:
server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java
##########
@@ -1448,10 +1452,15 @@ public int compare(Volume v1, Volume v2) {
else
requestVolumes = new ArrayList<Volume>();
requestVolumes.add(vol);
-
+ List<Pair<Volume, DiskProfile>>
volumeDiskProfilePair = new ArrayList<>();
+ for (Volume volume: requestVolumes) {
+ DiskOfferingVO diskOffering =
_diskOfferingDao.findById(volume.getDiskOfferingId());
+ DiskProfile diskProfile = new
DiskProfile(volume, diskOffering, _volsDao.getHypervisorType(volume.getId()));
+ volumeDiskProfilePair.add(new
Pair<>(volume, diskProfile));
Review comment:
possible to move creation of volume disk profile pair (from the
volumes), to a common method and use it across?
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3453,6 +3491,7 @@ public long getMemoryOrCpuCapacityByHost(final Long
hostId, final short capacity
cmdList.add(IssueOutOfBandManagementPowerActionCmd.class);
cmdList.add(ChangeOutOfBandManagementPasswordCmd.class);
cmdList.add(GetUserKeysCmd.class);
+ cmdList.add(ChangeOfferingForVolumeCmd.class);
Review comment:
Better to move this to the volume cmds added above.
https://github.com/apache/cloudstack/blob/3ddcf858767ff5f79986457cbbac4c856b73e0bd/server/src/main/java/com/cloud/server/ManagementServerImpl.java#L3260-L3271
##########
File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java
##########
@@ -2317,13 +2322,13 @@ public boolean storagePoolHasEnoughIops(List<Volume>
requestedVolumes, StoragePo
}
@Override
- public boolean storagePoolHasEnoughSpace(List<Volume> volumes, StoragePool
pool) {
- return storagePoolHasEnoughSpace(volumes, pool, null);
+ public boolean storagePoolHasEnoughSpace(List<Pair<Volume, DiskProfile>>
volumeDiskProfilePairs, StoragePool pool) {
+ return storagePoolHasEnoughSpace(volumeDiskProfilePairs, pool, null);
}
@Override
- public boolean storagePoolHasEnoughSpace(List<Volume> volumes, StoragePool
pool, Long clusterId) {
- if (volumes == null || volumes.isEmpty()) {
+ public boolean storagePoolHasEnoughSpace(List<Pair<Volume, DiskProfile>>
volumeDiskProfilesList, StoragePool pool, Long clusterId) {
+ if (volumeDiskProfilesList == null ||
volumeDiskProfilesList.isEmpty()) {
Review comment:
```suggestion
if (CollectionUtils.isEmpty(volumeDiskProfilesList)) {
```
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1596,6 +1622,341 @@ public Volume recoverVolume(long volumeId) {
return volume;
}
+ @Override
+ @ActionEvent(eventType = EventTypes.EVENT_VOLUME_CHANGE_DISK_OFFERING,
eventDescription = "Changing disk offering of a volume")
+ public Volume changeDiskOfferingForVolume(ChangeOfferingForVolumeCmd cmd)
throws ResourceAllocationException {
+ Long newSize = cmd.getSize();
+ Long newMinIops = cmd.getMinIops();
+ Long newMaxIops = cmd.getMaxIops();
+ Integer newHypervisorSnapshotReserve = null;
+ boolean autoMigrateVolume = cmd.getAutoMigrate();
+
+ boolean volumeMigrateRequired = false;
+ boolean volumeResizeRequired = false;
+
+ VolumeVO volume = _volsDao.findById(cmd.getId());
+ if (volume == null) {
+ throw new InvalidParameterValueException("No such volume");
+ }
+ long currentSize = volume.getSize();
+
+ DiskOfferingVO diskOffering =
_diskOfferingDao.findById(volume.getDiskOfferingId());
+ DiskOfferingVO newDiskOffering =
_diskOfferingDao.findById(cmd.getNewDiskOfferingId());
+
+ /* Does the caller have authority to act on this volume? */
+ _accountMgr.checkAccess(CallContext.current().getCallingAccount(),
null, true, volume);
+
+ // VALIDATIONS
+
+ if (newDiskOffering.getId() == diskOffering.getId()) {
+ throw new InvalidParameterValueException(String.format("Volume %s
already have the new disk offering %s provided", volume.getUuid(),
diskOffering.getUuid()));
+ }
+
+ Long updateNewSize[] = {newSize};
+ Long updateNewMinIops[] = {newMinIops};
+ Long updateNewMaxIops[] = {newMaxIops};
+ Integer updateNewHypervisorSnapshotReserve[] =
{newHypervisorSnapshotReserve};
+ validateVolumeResizeWithNewDiskOfferingAndLoad(volume, diskOffering,
newDiskOffering, updateNewSize, updateNewMinIops, updateNewMaxIops,
updateNewHypervisorSnapshotReserve);
+ newSize = updateNewSize[0];
+ newMinIops = updateNewMinIops[0];
+ newMaxIops = updateNewMaxIops[0];
+ newHypervisorSnapshotReserve = updateNewHypervisorSnapshotReserve[0];
+ validateVolumeResizeWithSize(volume, currentSize, newSize, true);
+
+ /* If this volume has never been beyond allocated state, short circuit
everything and simply update the database. */
+ // We need to publish this event to usage_volume table
+ if (volume.getState() == Volume.State.Allocated) {
+ s_logger.debug("Volume is in the allocated state, but has never
been created. Simply updating database with new size and IOPS.");
Review comment:
Better to keep volume id / name in the log here
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1596,6 +1622,341 @@ public Volume recoverVolume(long volumeId) {
return volume;
}
+ @Override
+ @ActionEvent(eventType = EventTypes.EVENT_VOLUME_CHANGE_DISK_OFFERING,
eventDescription = "Changing disk offering of a volume")
+ public Volume changeDiskOfferingForVolume(ChangeOfferingForVolumeCmd cmd)
throws ResourceAllocationException {
+ Long newSize = cmd.getSize();
+ Long newMinIops = cmd.getMinIops();
+ Long newMaxIops = cmd.getMaxIops();
+ Integer newHypervisorSnapshotReserve = null;
+ boolean autoMigrateVolume = cmd.getAutoMigrate();
+
+ boolean volumeMigrateRequired = false;
+ boolean volumeResizeRequired = false;
+
+ VolumeVO volume = _volsDao.findById(cmd.getId());
+ if (volume == null) {
+ throw new InvalidParameterValueException("No such volume");
+ }
+ long currentSize = volume.getSize();
+
+ DiskOfferingVO diskOffering =
_diskOfferingDao.findById(volume.getDiskOfferingId());
+ DiskOfferingVO newDiskOffering =
_diskOfferingDao.findById(cmd.getNewDiskOfferingId());
+
+ /* Does the caller have authority to act on this volume? */
+ _accountMgr.checkAccess(CallContext.current().getCallingAccount(),
null, true, volume);
+
+ // VALIDATIONS
+
+ if (newDiskOffering.getId() == diskOffering.getId()) {
+ throw new InvalidParameterValueException(String.format("Volume %s
already have the new disk offering %s provided", volume.getUuid(),
diskOffering.getUuid()));
+ }
+
+ Long updateNewSize[] = {newSize};
+ Long updateNewMinIops[] = {newMinIops};
+ Long updateNewMaxIops[] = {newMaxIops};
+ Integer updateNewHypervisorSnapshotReserve[] =
{newHypervisorSnapshotReserve};
+ validateVolumeResizeWithNewDiskOfferingAndLoad(volume, diskOffering,
newDiskOffering, updateNewSize, updateNewMinIops, updateNewMaxIops,
updateNewHypervisorSnapshotReserve);
+ newSize = updateNewSize[0];
+ newMinIops = updateNewMinIops[0];
+ newMaxIops = updateNewMaxIops[0];
+ newHypervisorSnapshotReserve = updateNewHypervisorSnapshotReserve[0];
+ validateVolumeResizeWithSize(volume, currentSize, newSize, true);
+
+ /* If this volume has never been beyond allocated state, short circuit
everything and simply update the database. */
+ // We need to publish this event to usage_volume table
+ if (volume.getState() == Volume.State.Allocated) {
+ s_logger.debug("Volume is in the allocated state, but has never
been created. Simply updating database with new size and IOPS.");
+
+ volume.setSize(newSize);
+ volume.setMinIops(newMinIops);
+ volume.setMaxIops(newMaxIops);
+ volume.setHypervisorSnapshotReserve(newHypervisorSnapshotReserve);
+
+ if (newDiskOffering != null) {
+ volume.setDiskOfferingId(cmd.getNewDiskOfferingId());
+ }
+
+ _volsDao.update(volume.getId(), volume);
+ if (currentSize != newSize) {
+
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_RESIZE,
volume.getAccountId(), volume.getDataCenterId(), volume.getId(),
volume.getName(),
+ volume.getDiskOfferingId(), volume.getTemplateId(),
volume.getSize(), Volume.class.getName(), volume.getUuid());
+ }
+ return volume;
+ }
+
+ if
(MatchStoragePoolTagsWithDiskOffering.valueIn(volume.getDataCenterId())) {
+ if (!doesNewDiskOfferingHasTagsAsOldDiskOffering(diskOffering,
newDiskOffering)) {
+ throw new
InvalidParameterValueException(String.format("Selected disk offering %s does
not have tags as in existing disk offering of volume", diskOffering.getUuid(),
volume.getUuid()));
Review comment:
```suggestion
throw new
InvalidParameterValueException(String.format("Selected disk offering %s does
not have tags as in existing disk offering of volume %s",
diskOffering.getUuid(), volume.getUuid()));
```
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1596,6 +1622,341 @@ public Volume recoverVolume(long volumeId) {
return volume;
}
+ @Override
+ @ActionEvent(eventType = EventTypes.EVENT_VOLUME_CHANGE_DISK_OFFERING,
eventDescription = "Changing disk offering of a volume")
+ public Volume changeDiskOfferingForVolume(ChangeOfferingForVolumeCmd cmd)
throws ResourceAllocationException {
+ Long newSize = cmd.getSize();
+ Long newMinIops = cmd.getMinIops();
+ Long newMaxIops = cmd.getMaxIops();
+ Integer newHypervisorSnapshotReserve = null;
+ boolean autoMigrateVolume = cmd.getAutoMigrate();
+
+ boolean volumeMigrateRequired = false;
+ boolean volumeResizeRequired = false;
+
+ VolumeVO volume = _volsDao.findById(cmd.getId());
+ if (volume == null) {
+ throw new InvalidParameterValueException("No such volume");
+ }
+ long currentSize = volume.getSize();
+
+ DiskOfferingVO diskOffering =
_diskOfferingDao.findById(volume.getDiskOfferingId());
+ DiskOfferingVO newDiskOffering =
_diskOfferingDao.findById(cmd.getNewDiskOfferingId());
+
+ /* Does the caller have authority to act on this volume? */
+ _accountMgr.checkAccess(CallContext.current().getCallingAccount(),
null, true, volume);
+
+ // VALIDATIONS
+
+ if (newDiskOffering.getId() == diskOffering.getId()) {
+ throw new InvalidParameterValueException(String.format("Volume %s
already have the new disk offering %s provided", volume.getUuid(),
diskOffering.getUuid()));
+ }
+
+ Long updateNewSize[] = {newSize};
+ Long updateNewMinIops[] = {newMinIops};
+ Long updateNewMaxIops[] = {newMaxIops};
+ Integer updateNewHypervisorSnapshotReserve[] =
{newHypervisorSnapshotReserve};
+ validateVolumeResizeWithNewDiskOfferingAndLoad(volume, diskOffering,
newDiskOffering, updateNewSize, updateNewMinIops, updateNewMaxIops,
updateNewHypervisorSnapshotReserve);
+ newSize = updateNewSize[0];
+ newMinIops = updateNewMinIops[0];
+ newMaxIops = updateNewMaxIops[0];
+ newHypervisorSnapshotReserve = updateNewHypervisorSnapshotReserve[0];
+ validateVolumeResizeWithSize(volume, currentSize, newSize, true);
+
+ /* If this volume has never been beyond allocated state, short circuit
everything and simply update the database. */
+ // We need to publish this event to usage_volume table
+ if (volume.getState() == Volume.State.Allocated) {
+ s_logger.debug("Volume is in the allocated state, but has never
been created. Simply updating database with new size and IOPS.");
+
+ volume.setSize(newSize);
+ volume.setMinIops(newMinIops);
+ volume.setMaxIops(newMaxIops);
+ volume.setHypervisorSnapshotReserve(newHypervisorSnapshotReserve);
+
+ if (newDiskOffering != null) {
+ volume.setDiskOfferingId(cmd.getNewDiskOfferingId());
+ }
+
+ _volsDao.update(volume.getId(), volume);
+ if (currentSize != newSize) {
+
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_RESIZE,
volume.getAccountId(), volume.getDataCenterId(), volume.getId(),
volume.getName(),
+ volume.getDiskOfferingId(), volume.getTemplateId(),
volume.getSize(), Volume.class.getName(), volume.getUuid());
+ }
+ return volume;
+ }
+
+ if
(MatchStoragePoolTagsWithDiskOffering.valueIn(volume.getDataCenterId())) {
+ if (!doesNewDiskOfferingHasTagsAsOldDiskOffering(diskOffering,
newDiskOffering)) {
+ throw new
InvalidParameterValueException(String.format("Selected disk offering %s does
not have tags as in existing disk offering of volume", diskOffering.getUuid(),
volume.getUuid()));
+ }
+ }
+
+ if (currentSize != newSize || newMaxIops != volume.getMaxIops() ||
newMinIops != volume.getMinIops()) {
+ volumeResizeRequired = true;
+ validateVolumeResizeChecks(volume, currentSize, newSize);
+ }
+
+ StoragePoolVO existingStoragePool =
_storagePoolDao.findById(volume.getPoolId());
+
+ Pair<List<? extends StoragePool>, List<? extends StoragePool>>
poolsPair =
managementService.listStoragePoolsForMigrationOfVolumeInternal(volume.getId(),
newDiskOffering.getId(), newSize, newMinIops, newMaxIops, true);
+ List<? extends StoragePool> suitableStoragePools = poolsPair.second();
+
+ if (!suitableStoragePools.stream().anyMatch(p -> (p.getId() ==
existingStoragePool.getId()))) {
+ volumeMigrateRequired = true;
+ if (!autoMigrateVolume) {
+ throw new InvalidParameterValueException("Failed to change
offering for volume since automigrate is set to false but volume needs to
migrated");
Review comment:
Keep volume id / name in the log here
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5367,12 +5414,17 @@ public UserVm createVirtualMachine(DeployVMCmd cmd)
throws InsufficientCapacityE
}
Long serviceOfferingId = cmd.getServiceOfferingId();
+ Long overrideDiskOfferingId = cmd.getOverrideDiskOfferingId();
ServiceOffering serviceOffering =
_entityMgr.findById(ServiceOffering.class, serviceOfferingId);
if (serviceOffering == null) {
throw new InvalidParameterValueException("Unable to find service
offering: " + serviceOfferingId);
}
+ if (serviceOffering.getDiskOfferingStrictness() &&
overrideDiskOfferingId != null) {
+ throw new InvalidParameterValueException(String.format("Cannot
user override disk offering id %d since provided service offering is strictly
mapped to its disk offering", overrideDiskOfferingId));
Review comment:
```suggestion
throw new InvalidParameterValueException(String.format("Cannot
override disk offering id %d since provided service offering is strictly mapped
to its disk offering", overrideDiskOfferingId));
```
--
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]
