Skip to content

server: Avoid device id collision between config drive ISO and data volumes on KVM #14073

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

Open
weizhouapache wants to merge 2 commits into
base: 4.22
Choose a base branch
from
+19 −5
Open

server: Avoid device id collision between config drive ISO and data volumes on KVM #14073

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
826ea7a
server: Avoid device id collision between config drive ISO and data v…
weizhouapache Sep 7, 2026
9709308
Change CONFIGDRIVEDISKSEQ to static final
weizhouapache Oct 6, 2026
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
14 changes: 11 additions & 3 deletions server/src/main/java/com/cloud/hypervisor/KVMGuru.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.cloud.hypervisor.Hypervisor.HypervisorType;
import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao;
import com.cloud.hypervisor.kvm.dpdk.DpdkHelper;
import com.cloud.network.element.ConfigDriveNetworkElement;
import com.cloud.service.ServiceOfferingVO;
import com.cloud.storage.DataStoreRole;
import com.cloud.storage.GuestOSHypervisorVO;
Expand All @@ -42,7 +43,9 @@
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachine;
import com.cloud.vm.VirtualMachineProfile;
import com.cloud.vm.VmDetailConstants;
import com.cloud.vm.dao.VMInstanceDao;
import com.cloud.vm.dao.VMInstanceDetailsDao;
import org.apache.cloudstack.backup.Backup;
import org.apache.cloudstack.storage.command.CopyCommand;
import org.apache.cloudstack.storage.command.StorageSubSystemCommand;
Expand Down Expand Up @@ -71,6 +74,8 @@ public class KVMGuru extends HypervisorGuruBase implements HypervisorGuru {
VolumeDao _volumeDao;
@Inject
HypervisorCapabilitiesDao _hypervisorCapabilitiesDao;
@Inject
VMInstanceDetailsDao _vmInstanceDetailsDao;


@Override
Expand All @@ -86,7 +91,7 @@ protected KVMGuru() {
* Get next free DeviceId for a KVM Guest
*/

protected Long getNextAvailableDeviceId(List<VolumeVO> vmVolumes) {
protected Long getNextAvailableDeviceId(long vmId, List<VolumeVO> vmVolumes) {

int maxDataVolumesSupported;
int maxDeviceId;
Expand All @@ -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());
}
for (VolumeVO vmVolume : vmVolumes) {
devIds.remove(vmVolume.getDeviceId().toString().trim());
}
Expand Down Expand Up @@ -371,7 +379,7 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId,
} else if (VMVolToRestore.getType() == Volume.Type.DATADISK) {
List<VolumeVO> vmVolumes = _volumeDao.findByInstance(vm.getId());
_volumeDao.update(volume.getId(), volume);
_volumeDao.attachVolume(volume.getId(), vm.getId(), getNextAvailableDeviceId(vmVolumes));
_volumeDao.attachVolume(volume.getId(), vm.getId(), getNextAvailableDeviceId(vm.getId(), vmVolumes));
}
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_ATTACH, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(),
volume.getDiskOfferingId(), volume.getTemplateId(), volume.getSize(), Volume.class.getName(), volume.getUuid(), vm.getId(), volume.isDisplay());
Expand All @@ -389,7 +397,7 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId,
VolumeVO restoredVolume = _volumeDao.findByUuid(location);
if (restoredVolume != null) {
try {
_volumeDao.attachVolume(restoredVolume.getId(), vm.getId(), getNextAvailableDeviceId(vmVolumes));
_volumeDao.attachVolume(restoredVolume.getId(), vm.getId(), getNextAvailableDeviceId(vm.getId(), vmVolumes));
restoredVolume.setState(Volume.State.Ready);
_volumeDao.update(restoredVolume.getId(), restoredVolume);
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_ATTACH, restoredVolume.getAccountId(), restoredVolume.getDataCenterId(), restoredVolume.getId(), restoredVolume.getName(),
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle
@Inject
private HypervisorGuruManager _hvGuruMgr;

private final static Integer CONFIGDRIVEDISKSEQ = 4;
public static final Integer CONFIGDRIVEDISKSEQ = 4;

@Damans227 Damans227 Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what about vms that already have a data disk on 4? looks like they still clash with the config drive on the next start after upgrade

@weizhouapache weizhouapache Oct 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the configdrive ISO will still use device id 4, the other disks will use other device ids, I think.

@Damans227 since you developed the support for multiple CD-ROMs, do you know how the device ids are determined ? is it possible that one of the CD-ROM uses device id 4 ?

@Damans227 Damans227 Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

on 4.22 there is only one cd drive and it sits on 3, so nothing else takes 4. on main the extra cd drives count up from 3, so a second iso goes on 4, the same spot as the config drive. so this will clash once it merges forward to main

@weizhouapache weizhouapache Oct 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@Damans227
Thanks, agreed. This is also why I asked you about the configurations for multiple CD-ROMs. We do need to consider these scenarios.

In my opinion, we don't need to consider the device ID changing when a VM is stopped and started. Users should use the device UUID rather than the device name inside the guest OS, since the device name (/dev/sdX) may change as ACS orders the devices when the VM is started.

I think we should focus on preventing duplicate device IDs.

@Damans227 Damans227 Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

makes sense to focus on duplicate ids. a vm that already has a data disk on 4 still clashes after upgrade though, since nothing moves that disk


private boolean canHandle(TrafficType trafficType) {
return trafficType.equals(TrafficType.Guest);
Expand Down
8 changes: 7 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 @@ -160,6 +160,7 @@
import com.cloud.hypervisor.Hypervisor.HypervisorType;
import com.cloud.hypervisor.HypervisorCapabilitiesVO;
import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao;
import com.cloud.network.element.ConfigDriveNetworkElement;
import com.cloud.offering.DiskOffering;
import com.cloud.org.Cluster;
import com.cloud.org.Grouping;
Expand Down Expand Up @@ -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;

@Damans227 Damans227 Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what happens for a vm deployed with startvm=false and then given three data disks? the config drive detail only shows up on first start, so the third disk still lands on 4

@weizhouapache weizhouapache Oct 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

good point. I did not consider this case.
maybe we need to determine if configdrive ISO is needed by the network/offering providers.

@Damans227 Damans227 Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

makes sense, checking if the default network uses config drive for user data would work even before the first start

if (deviceId != null) {
if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || deviceId.longValue() == 3) {
if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || deviceId.longValue() == 3
|| (vmHasConfigDrive && deviceId.longValue() == ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ)) {
throw new RuntimeException("deviceId should be 0,1,2,4-" + maxDeviceId);
}
for (VolumeVO vol : vols) {
Expand All @@ -5058,6 +5061,9 @@ private Long getDeviceId(UserVmVO vm, Long deviceId) {
devIds.add(String.valueOf(i));
}
devIds.remove("3");
if (vmHasConfigDrive) {
devIds.remove(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.toString());
}
for (VolumeVO vol : vols) {
devIds.remove(vol.getDeviceId().toString().trim());
}
Expand Down
Loading