Copilot commented on code in PR #14073:
URL: https://github.com/apache/cloudstack/pull/14073#discussion_r3950623234
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5042,8 +5043,10 @@ private Long getDeviceId(UserVmVO vm, Long deviceId) {
int maxDevices = getMaxDataVolumesSupported(vm) + 2; // add 2 to
consider devices root volume and cdrom
int maxDeviceId = maxDevices - 1;
List<VolumeVO> vols = _volsDao.findByInstance(vm.getId());
+ boolean vmHasConfigDrive = vmInstanceDetailsDao.findDetail(vm.getId(),
VmDetailConstants.CONFIG_DRIVE_LOCATION) != null;
if (deviceId != null) {
Review Comment:
The max device-id range is computed as maxDataVolumesSupported + 2 (root +
CD-ROM), but when a VM has a config drive you also reserve CONFIGDRIVEDISKSEQ.
Without increasing maxDevices by 1, this reduces the number of attachable data
volumes by one compared to the hypervisor capability. Also, the validation
error message still claims deviceId can be in the 4-… range even though 4 is
invalid when config drive is present.
##########
server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java:
##########
@@ -142,7 +142,7 @@ public class ConfigDriveNetworkElement extends AdapterBase
implements NetworkEle
@Inject
private HypervisorGuruManager _hvGuruMgr;
- private final static Integer CONFIGDRIVEDISKSEQ = 4;
+ public final static Integer CONFIGDRIVEDISKSEQ = 4;
Review Comment:
Modifier order for constants in this codebase is consistently `public static
final ...`; `public final static` is an outlier and makes it harder to scan for
constants (and is inconsistent with the other constant in this class).
##########
server/src/main/java/com/cloud/hypervisor/KVMGuru.java:
##########
@@ -103,6 +108,9 @@ protected Long getNextAvailableDeviceId(List<VolumeVO>
vmVolumes) {
devIds.add(String.valueOf(i));
}
devIds.remove("3");
+ if (_vmInstanceDetailsDao.findDetail(vmId,
VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) {
+
devIds.remove(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.toString());
+ }
Review Comment:
The config-drive reservation here removes another device id (in addition to
the CD-ROM’s reserved id 3), but maxDeviceId is still computed as
maxDataVolumesSupported + 2 (root + CD-ROM). This makes the allocator run out
of device ids one volume earlier than the hypervisor’s max data volumes limit
when config drive is enabled; consider increasing maxDevices by 1 when
vmHasConfigDrive is true.
--
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]
