Copilot commented on code in PR #11340:
URL: https://github.com/apache/cloudstack/pull/11340#discussion_r2241871421
##########
scripts/vm/hypervisor/kvm/gpudiscovery.sh:
##########
@@ -473,7 +473,7 @@ for VM in "${VMS[@]}"; do
# -- MDEV hostdevs: use xmlstarlet to extract UUIDs --
while IFS= read -r UUID; do
[[ -n "$UUID" ]] && mdev_to_vm["$UUID"]="$VM"
- done < <(echo "$xml" | xmlstarlet sel -T -t -m
"//hostdev[@type='mdev']" -v "@uuid" -n 2>/dev/null || true)
+ done < <(echo "$xml" | xmlstarlet sel -T -t -m
"//hostdev[@type='mdev']/source/address" -v "@uuid" -n 2>/dev/null || true)
Review Comment:
The XPath expression `//hostdev[@type='mdev']/source/address` may not match
the XML structure shown in LibvirtGpuDef.java where the address element has a
uuid attribute but is not necessarily nested under source. Consider using
`//hostdev[@type='mdev']//address` or verify the actual XML structure.
```suggestion
done < <(echo "$xml" | xmlstarlet sel -T -t -m
"//hostdev[@type='mdev']//address" -v "@uuid" -n 2>/dev/null || true)
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtGpuDef.java:
##########
@@ -49,7 +49,7 @@ private void generateMdevXml(StringBuilder gpuBuilder) {
String mdevUuid = vgpuType.getBusAddress(); // For MDEV devices,
busAddress contains the UUID
String displayAttribute = vgpuType.isDisplay() ? "on" : "off";
- gpuBuilder.append("<hostdev mode='subsystem' type='mdev' managed='no'
display='").append(displayAttribute).append("'>\n");
+ gpuBuilder.append("<hostdev mode='subsystem' type='mdev'
model='vfio-pci' display='").append(displayAttribute).append("'>\n");
Review Comment:
The removal of `managed='no'` attribute from the hostdev element may affect
libvirt's device management behavior. Verify that this change doesn't break
existing MDEV device handling or require corresponding libvirt configuration
updates.
```suggestion
gpuBuilder.append("<hostdev mode='subsystem' type='mdev'
model='vfio-pci' managed='no'
display='").append(displayAttribute).append("'>\n");
```
##########
server/src/main/java/org/apache/cloudstack/gpu/GpuServiceImpl.java:
##########
@@ -891,14 +890,20 @@ public void addGpuDevicesToHost(final Host host, final
List<VgpuTypesInfo> newGp
} else {
// Update the device's info
GpuDeviceVO parentGpuDevice = null;
- if (existingDevice.getParentGpuDeviceId() == null
- && deviceInfo.getParentBusAddress() != null) {
+ if (deviceInfo.getParentBusAddress() != null) {
parentGpuDevice =
gpuDeviceDao.findByHostIdAndBusAddress(host.getId(),
deviceInfo.getParentBusAddress());
if (parentGpuDevice != null) {
existingDevice.setParentGpuDeviceId(parentGpuDevice.getId());
+
parentGpuDevice.setType(GpuDevice.DeviceType.VGPUOnly);
+ gpuDeviceDao.persist(parentGpuDevice);
}
}
+ if (deviceInfo.isPassthroughEnabled()) {
+
existingDevice.setType(deviceInfo.getDeviceType());
+ } else {
+
existingDevice.setType(GpuDevice.DeviceType.VGPUOnly);
Review Comment:
The parent GPU device type is being set to VGPUOnly in multiple places
(lines 898, 1031) without checking if it's already set or if it conflicts with
existing state. Consider consolidating this logic or adding validation to
prevent overwriting.
```suggestion
updateGpuDeviceTypeIfNecessary(parentGpuDevice, GpuDevice.DeviceType.VGPUOnly);
gpuDeviceDao.persist(parentGpuDevice);
}
}
if (deviceInfo.isPassthroughEnabled()) {
existingDevice.setType(deviceInfo.getDeviceType());
} else {
updateGpuDeviceTypeIfNecessary(existingDevice, GpuDevice.DeviceType.VGPUOnly);
```
--
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]
