Repository navigation
server: Avoid device id conflicts between data volumes, the CD-ROM and the config drive ISO #14073
+351
−29
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,6 +160,10 @@ | |
| import com.cloud.hypervisor.Hypervisor.HypervisorType; | ||
| import com.cloud.hypervisor.HypervisorCapabilitiesVO; | ||
| import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao; | ||
| import com.cloud.network.Network.Provider; | ||
| import com.cloud.network.Network.Service; | ||
| import com.cloud.network.NetworkModel; | ||
| import com.cloud.network.element.ConfigDriveNetworkElement; | ||
| import com.cloud.offering.DiskOffering; | ||
| import com.cloud.org.Cluster; | ||
| import com.cloud.org.Grouping; | ||
|
|
@@ -219,6 +223,7 @@ | |
| import com.cloud.utils.fsm.NoTransitionException; | ||
| import com.cloud.utils.fsm.StateMachine2; | ||
| import com.cloud.vm.DiskProfile; | ||
| import com.cloud.vm.NicVO; | ||
| import com.cloud.vm.VMInstanceDetailVO; | ||
| import com.cloud.vm.UserVmManager; | ||
| import com.cloud.vm.UserVmService; | ||
|
|
@@ -240,6 +245,7 @@ | |
| import com.cloud.vm.VmWorkResizeVolume; | ||
| import com.cloud.vm.VmWorkSerializer; | ||
| import com.cloud.vm.VmWorkTakeVolumeSnapshot; | ||
| import com.cloud.vm.dao.NicDao; | ||
| import com.cloud.vm.dao.UserVmDao; | ||
| import com.cloud.vm.dao.VMInstanceDetailsDao; | ||
| import com.cloud.vm.dao.VMInstanceDao; | ||
|
|
@@ -293,6 +299,10 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic | |
| @Inject | ||
| private VMInstanceDetailsDao vmInstanceDetailsDao; | ||
| @Inject | ||
| private NicDao _nicDao; | ||
| @Inject | ||
| private NetworkModel _networkModel; | ||
| @Inject | ||
| private UserVmService _userVmService; | ||
| @Inject | ||
| private VolumeDataStoreDao _volumeStoreDao; | ||
|
|
@@ -5037,14 +5047,69 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype | |
| return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion : minHwVersion; | ||
| } | ||
|
|
||
| /** | ||
| * The number of CD-ROM devices of the Instance. Each CD-ROM device occupies a device id, starting from | ||
| * {@link TemplateManager#CDROM_PRIMARY_DEVICE_SEQ}. | ||
| */ | ||
| protected int getVmCdromCount(VirtualMachine vm) { | ||
| return 1; | ||
|
Collaborator
There was a problem hiding this comment. on main a second cd drive sits on 4 too, so should the config drive move past the last cd slot when this merges forward?
Member
Author
There was a problem hiding this comment. @Damans227 |
||
| } | ||
|
|
||
| /** | ||
| * Checks if a config drive ISO is (or will be) attached to the Instance, which is the case when any of its networks | ||
| * has ConfigDrive as the provider of the Dhcp, Dns or UserData service. Checking the network providers (instead of | ||
| * only the config drive location detail, which is added when the ISO is created) also covers Instances which have | ||
| * never been started. | ||
| */ | ||
| protected boolean isConfigDriveUsed(VirtualMachine vm) { | ||
| if (vmInstanceDetailsDao.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) { | ||
| return true; | ||
| } | ||
| for (NicVO nic : _nicDao.listByVmId(vm.getId())) { | ||
| for (Service service : Arrays.asList(Service.UserData, Service.Dhcp, Service.Dns)) { | ||
| if (_networkModel.isProviderSupportServiceInNetwork(nic.getNetworkId(), service, Provider.ConfigDrive)) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public Set<Long> getReservedDeviceIds(VirtualMachine vm) { | ||
| Set<Long> reservedDeviceIds = new HashSet<>(); | ||
| for (int i = 0; i < getVmCdromCount(vm); i++) { | ||
| reservedDeviceIds.add((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + i); | ||
| } | ||
| if (isConfigDriveUsed(vm)) { | ||
| reservedDeviceIds.add(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue()); | ||
| } | ||
| return reservedDeviceIds; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the first {@code maxDataVolumes} device ids (starting from 1) which are not reserved. | ||
| */ | ||
| private List<Long> getAvailableDeviceIds(int maxDataVolumes, Set<Long> reservedDeviceIds) { | ||
| List<Long> availableDeviceIds = new ArrayList<>(); | ||
| for (long id = 1; availableDeviceIds.size() < maxDataVolumes; id++) { | ||
| if (!reservedDeviceIds.contains(id)) { | ||
| availableDeviceIds.add(id); | ||
| } | ||
| } | ||
| return availableDeviceIds; | ||
| } | ||
|
|
||
| private Long getDeviceId(UserVmVO vm, Long deviceId) { | ||
| // allocate deviceId | ||
| Set<Long> reservedDeviceIds = getReservedDeviceIds(vm); | ||
| List<Long> availableDeviceIds = getAvailableDeviceIds(getMaxDataVolumesSupported(vm), reservedDeviceIds); | ||
| long maxDeviceId = availableDeviceIds.get(availableDeviceIds.size() - 1); | ||
| List<VolumeVO> vols = _volsDao.findByInstance(vm.getId()); | ||
| if (deviceId != null) { | ||
| if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || reservedDeviceIds.contains(deviceId)) { | ||
| throw new RuntimeException(String.format("deviceId should be 0 or between 1 and %d, except the ones reserved for CD-ROM/config drive devices: %s", | ||
| maxDeviceId, reservedDeviceIds)); | ||
| } | ||
| for (VolumeVO vol : vols) { | ||
| if (vol.getDeviceId().equals(deviceId)) { | ||
|
|
@@ -5053,18 +5118,13 @@ private Long getDeviceId(UserVmVO vm, Long deviceId) { | |
| } | ||
| } else { | ||
| // allocate deviceId here | ||
| for (VolumeVO vol : vols) { | ||
| availableDeviceIds.remove(vol.getDeviceId()); | ||
| } | ||
| if (availableDeviceIds.isEmpty()) { | ||
| throw new RuntimeException(String.format("All device Ids are used by vm %s", vm)); | ||
| } | ||
| deviceId = availableDeviceIds.get(0); | ||
| } | ||
|
|
||
| return deviceId; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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