Skip to content

<fix>[externalapiadapter]: fix VM NIC driver type fallback #3933

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
zstack-robot-1 wants to merge 1 commit into
base: 5.4.2
Choose a base branch
from
Open

<fix>[externalapiadapter]: fix VM NIC driver type fallback #3933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 10 additions & 3 deletions compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2396,21 +2396,29 @@ public void run(FlowTrigger trigger, Map data) {
int deviceId = deviceIdBitmap.nextClearBit(0);
deviceIdBitmap.set(deviceId);
String internalName = VmNicVO.generateNicInternalName(spec.getVmInventory().getInternalId(), deviceId);
String driverType = vmNicVO.getDriverType();
if (StringUtils.isEmpty(driverType)) {
VmNicInventory nicInv = VmNicInventory.valueOf(vmNicVO);
nicManager.setNicDriverType(nicInv, VmSystemTags.VIRTIO.hasTag(self.getUuid()),
ImagePlatform.valueOf(spec.getVmInventory().getPlatform()).isParaVirtualization(),
spec.getVmInventory());
driverType = nicInv.getDriverType();
}

UpdateQuery.New(VmNicVO.class)
.eq(VmNicVO_.uuid, vmNicUuid)
.set(VmNicVO_.vmInstanceUuid, self.getUuid())
.set(VmNicVO_.deviceId, deviceId)
.set(VmNicVO_.internalName, internalName)
.set(VmNicVO_.hypervisorType, spec.getVmInventory().getHypervisorType())
.set(VmNicVO_.driverType, driverType)
.update();

vmNicVO.setVmInstanceUuid(self.getUuid());
vmNicVO.setDeviceId(deviceId);
vmNicVO.setInternalName(internalName);
vmNicVO.setHypervisorType(spec.getVmInventory().getHypervisorType());
vmNicVO.setDriverType(VmSystemTags.VIRTIO.hasTag(self.getUuid()) ?
nicManager.getDefaultPVNicDriver() : nicManager.getDefaultNicDriver());
vmNicVO.setDriverType(driverType);
spec.getDestNics().add(0, VmNicInventory.valueOf(vmNicVO));

trigger.next();
Expand Down Expand Up @@ -8984,4 +8992,3 @@ public void run(MessageReply reply) {
});
}
}

26 changes: 13 additions & 13 deletions compute/src/main/java/org/zstack/compute/vm/VmNicManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.zstack.compute.vm;

import com.google.common.collect.Maps;
import org.apache.commons.collections.CollectionUtils;
import org.springframework.beans.factory.annotation.Autowire;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -119,39 +118,40 @@ public void afterDelIpAddress(String vmNicUUid, String usedIpUuid) {

@Override
public void prepareDbInitialValue() {
List<VmNicVO> nics = Q.New(VmNicVO.class).notNull(VmNicVO_.vmInstanceUuid).list();
List<VmNicVO> ns = nics.stream()
.filter(v -> v.getDriverType() == null
&& v.getType().equals(VmInstanceConstant.VIRTUAL_NIC_TYPE)
&& v.getVmInstanceUuid() != null)
.collect(Collectors.toList());
List<VmNicVO> nics = Q.New(VmNicVO.class)
.isNull(VmNicVO_.driverType)
.eq(VmNicVO_.type, VmInstanceConstant.VIRTUAL_NIC_TYPE)
.notNull(VmNicVO_.vmInstanceUuid)
.list();

if (CollectionUtils.isEmpty(ns)) {
if (CollectionUtils.isEmpty(nics)) {
return;
}

List<String> vmUuids = ns.stream()
List<String> vmUuids = nics.stream()
.map(VmNicVO::getVmInstanceUuid)
.distinct()
.collect(Collectors.toList());
Set<String> virtioVmUuids = new HashSet<>(VmSystemTags.VIRTIO.filterResourceHasTag(vmUuids));

List<Tuple> tupleList = Q.New(VmInstanceVO.class)
.select(VmInstanceVO_.uuid, VmInstanceVO_.platform)
.in(VmInstanceVO_.uuid, vmUuids)
.listTuple();

Map<String, String> vmPlatforms = Maps.newHashMap();
Map<String, String> vmDrivers = new HashMap<>();
for (Tuple vmTuple : tupleList) {
String vmUuid = vmTuple.get(0, String.class);
String vmPlatform = vmTuple.get(1, String.class);
vmPlatforms.put(vmUuid, ImagePlatform.valueOf(vmPlatform).isParaVirtualization() ?
vmDrivers.put(vmUuid, virtioVmUuids.contains(vmUuid) || ImagePlatform.valueOf(vmPlatform).isParaVirtualization() ?
defaultPVNicDriver : defaultNicDriver);
Comment on lines +146 to 147
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== 1) 检查 VmInstanceVO.platform 字段定义 =="
fd -i 'VmInstanceVO.java' | xargs -r rg -n -C3 'platform|@Column|nullable'

echo
echo "== 2) 检查 ImagePlatform 可接受取值 =="
fd -i 'ImagePlatform.java' | xargs -r rg -n -C2 'enum ImagePlatform|class ImagePlatform|\\b[A-Z0-9_]+\\b'

echo
echo "== 3) 检查平台字段历史写入/迁移路径（是否可能出现非枚举值或空值） =="
rg -n -C2 --iglob '*.{java,groovy,sql,xml}' 'vm_instance|VmInstanceVO_\\.platform|\\bplatform\\b\\s*=|insert\\s+into\\s+.*vm_instance'

Repository: MatheMatrix/zstack

Length of output: 103

🏁 Script executed:

git ls-files | grep -i "VmNicManagerImpl\|VmInstanceVO\|ImagePlatform" | head -20

Repository: MatheMatrix/zstack

Length of output: 418

🏁 Script executed:

sed -n '140,160p' compute/src/main/java/org/zstack/compute/vm/VmNicManagerImpl.java

Repository: MatheMatrix/zstack

Length of output: 1041

🏁 Script executed:

rg -n -C5 '@Column.*platform|private.*platform|public.*platform' header/src/main/java/org/zstack/header/vm/VmInstanceVO.java | head -30

Repository: MatheMatrix/zstack

Length of output: 44

🏁 Script executed:

wc -l header/src/main/java/org/zstack/header/vm/VmInstanceVO.java && grep -n "platform" header/src/main/java/org/zstack/header/vm/VmInstanceVO.java | head -10

Repository: MatheMatrix/zstack

Length of output: 126

🏁 Script executed:

cat -n header/src/main/java/org/zstack/header/vm/VmInstanceVO.java

Repository: MatheMatrix/zstack

Length of output: 5235

🏁 Script executed:

grep -r "class VmInstanceAO" --include="*.java" | head -5

Repository: MatheMatrix/zstack

Length of output: 279

🏁 Script executed:

rg -n "platform" header/src/main/java/org/zstack/header/vm/VmInstanceAO.java

Repository: MatheMatrix/zstack

Length of output: 256

🏁 Script executed:

sed -n '75,95p' header/src/main/java/org/zstack/header/vm/VmInstanceAO.java

Repository: MatheMatrix/zstack

Length of output: 385

🏁 Script executed:

cat -n header/src/main/java/org/zstack/header/image/ImagePlatform.java

Repository: MatheMatrix/zstack

Length of output: 946

🏁 Script executed:

rg -n "prepareDbInitialValue" compute/src/main/java/org/zstack/compute/vm/VmNicManagerImpl.java -B5 -A15

Repository: MatheMatrix/zstack

Length of output: 893

🏁 Script executed:

sed -n '120,170p' compute/src/main/java/org/zstack/compute/vm/VmNicManagerImpl.java

Repository: MatheMatrix/zstack

Length of output: 2231

避免在启动修复路径直接对平台字符串调用 valueOf，需要做安全解析

VmInstanceAO.platform 字段为可空（@Column 未设置 nullable=false），但 line 146 的 ImagePlatform.valueOf(vmPlatform) 在遇到 null 值或未知平台值时会抛 IllegalArgumentException，导致 prepareDbInitialValue() 启动修复被中断。需要安全解析：未知/空值平台降级为 defaultNicDriver 并记录告警。

建议修改（示例） 
        for (Tuple vmTuple : tupleList) {
            String vmUuid = vmTuple.get(0, String.class);
            String vmPlatform = vmTuple.get(1, String.class);
-           vmDrivers.put(vmUuid, virtioVmUuids.contains(vmUuid) || ImagePlatform.valueOf(vmPlatform).isParaVirtualization() ?
-                   defaultPVNicDriver : defaultNicDriver);
+           Optional<ImagePlatform> platformOpt = Arrays.stream(ImagePlatform.values())
+                   .filter(p -> p.toString().equals(vmPlatform))
+                   .findFirst();
+           boolean isParaVirtualization = platformOpt.map(ImagePlatform::isParaVirtualization).orElse(false);
+           if (vmPlatform != null && !platformOpt.isPresent()) {
+               logger.warn(String.format("unknown vm platform[%s] for vm[uuid:%s], fallback to default nic driver",
+                       vmPlatform, vmUuid));
+           }
+           vmDrivers.put(vmUuid, virtioVmUuids.contains(vmUuid) || isParaVirtualization
+                   ? defaultPVNicDriver : defaultNicDriver);
        }

符合向后兼容原则：启动修复应容错处理历史脏数据，避免因个别异常记录导致全局启动失败。

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@compute/src/main/java/org/zstack/compute/vm/VmNicManagerImpl.java` around
lines 146 - 147, The current vmDrivers.put call in VmNicManagerImpl (inside
prepareDbInitialValue) directly calls ImagePlatform.valueOf(vmPlatform) which
will throw on null/unknown platform; change this to safely parse the platform
string: first check if vmPlatform is null/blank, then attempt a guarded parse
(try-catch around valueOf or use a safe lookup method) and treat any exception
or unknown value as non-paravirtualized, falling back to defaultNicDriver;
ensure the put still uses virtioVmUuids.contains(vmUuid) ||
<isParaVirtualization> logic with the safe result, and log a warning/error
mentioning vmUuid and vmPlatform when falling back so startup repair tolerates
dirty/legacy data.

}

Map<Boolean, List<String>> nicGroups = nics.stream()
.filter(v -> vmPlatforms.containsKey(v.getVmInstanceUuid()))
.filter(v -> vmDrivers.containsKey(v.getVmInstanceUuid()))
.collect(
Collectors.groupingBy(
v -> vmPlatforms.get(v.getVmInstanceUuid()).equals(defaultPVNicDriver),
v -> vmDrivers.get(v.getVmInstanceUuid()).equals(defaultPVNicDriver),
Collectors.mapping(VmNicVO::getUuid, Collectors.toList()))
);

Expand Down
41 changes: 40 additions & 1 deletion test/src/test/groovy/org/zstack/test/integration/kvm/nic/ChangeWindowsVmNicDriverCase.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@ package org.zstack.test.integration.kvm.nic

import org.springframework.beans.factory.annotation.Autowired
import org.zstack.compute.vm.VmSystemTags
import org.zstack.compute.vm.VmNicManagerImpl
import org.zstack.header.image.ImagePlatform
import org.zstack.header.tag.SystemTagVO
import org.zstack.header.tag.SystemTagVO_
import org.zstack.sdk.VmInstanceInventory
import org.zstack.sdk.L3NetworkInventory
import org.zstack.sdk.VmNicInventory
import org.zstack.header.vm.VmNicVO
import org.zstack.header.vm.VmNicVO_
import org.zstack.tag.SystemTag
Expand All @@ -16,6 +19,7 @@ import org.zstack.test.integration.kvm.KvmTest
import org.zstack.testlib.EnvSpec
import org.zstack.testlib.SubCase
import org.zstack.core.db.Q
import org.zstack.core.db.SQL

class ChangeWindowsVmNicDriverCase extends SubCase {
EnvSpec env
Expand All @@ -39,6 +43,7 @@ class ChangeWindowsVmNicDriverCase extends SubCase {
void test() {
env.create {
testChangeWindowsVmNicDriver()
testPrepareDbInitialValueForVirtioTaggedWindowsVm()
}
}

Expand Down Expand Up @@ -88,4 +93,38 @@ class ChangeWindowsVmNicDriverCase extends SubCase {
assert VmSystemTags.VIRTIO.hasTag(vm.uuid)
assert Q.New(VmNicVO.class).eq(VmNicVO_.vmInstanceUuid, vm.uuid).select(VmNicVO_.driverType).findValue().equals("virtio")
}
}

void testPrepareDbInitialValueForVirtioTaggedWindowsVm() {
VmInstanceInventory vm = env.inventoryByName("vm") as VmInstanceInventory
L3NetworkInventory pubL3 = env.inventoryByName("pubL3") as L3NetworkInventory
VmNicVO originalNic = Q.New(VmNicVO.class)
.eq(VmNicVO_.vmInstanceUuid, vm.uuid)
.find()

assert VmSystemTags.VIRTIO.hasTag(vm.uuid)
assert originalNic.driverType == "virtio"

VmNicInventory newNic = createVmNic {
l3NetworkUuid = pubL3.uuid
}

attachVmNicToVm {
vmInstanceUuid = vm.uuid
vmNicUuid = newNic.uuid
}

SQL.New(VmNicVO.class)
.eq(VmNicVO_.uuid, newNic.uuid)
.set(VmNicVO_.driverType, null)
.update()
assert Q.New(VmNicVO.class)
.eq(VmNicVO_.uuid, newNic.uuid)
.select(VmNicVO_.driverType)
.findValue() == null

bean(VmNicManagerImpl.class).prepareDbInitialValue()

assert Q.New(VmNicVO.class).eq(VmNicVO_.uuid, originalNic.uuid).select(VmNicVO_.driverType).findValue() == "virtio"
assert Q.New(VmNicVO.class).eq(VmNicVO_.uuid, newNic.uuid).select(VmNicVO_.driverType).findValue() == "virtio"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}
4 changes: 4 additions & 0 deletions test/src/test/groovy/org/zstack/test/integration/kvm/nic/VmNicBasicCase.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class VmNicBasicCase extends SubCase {
assert nic.ip != null
assert nic.mac != null
assert nic.usedIps.size() != 0
assert dbFindByUuid(nic.uuid, VmNicVO.class).driverType == null

IpRangeInventory ipRangeInventory = pubL3.ipRanges.get(0)
assert nic.gateway == ipRangeInventory.gateway
Expand All @@ -101,6 +102,7 @@ class VmNicBasicCase extends SubCase {
void testAttachVmNicToVm () {
L3NetworkInventory l3 = env.inventoryByName("l3")
VmInstanceInventory vm = env.inventoryByName("vm")
String expectedDriverType = vm.vmNics[0].driverType

changeL3NetworkState {
uuid = nic.l3NetworkUuid
Expand Down Expand Up @@ -149,6 +151,8 @@ class VmNicBasicCase extends SubCase {
assert vmNicVO.deviceId == 1
assert vmNicVO.internalName == VmNicVO.generateNicInternalName(vmInstanceVO.getInternalId(), 1)
assert vmNicVO.vmInstanceUuid == vm.getUuid()
assert vmNicVO.driverType == expectedDriverType
assert vm.vmNics.find { it.uuid == nic.uuid }.driverType == expectedDriverType

usedIpUuid = vmNicVO.usedIpUuid

Expand Down