KVM: fix UEFI disk-only instance snapshot NVRAM handling

PendingReleaseNotes

@@ -39,3 +39,12 @@ example.ver.1 > example.ver.2:
     which can now be attached to Instances. This is to prevent the Secondary
     Storage to grow to enormous sizes as Linux Distributions keep growing in
     size while a stripped down Linux should fit on a 2.88MB floppy.
+
+4.22.0.0 > 4.22.0.1:
+  * Disk-only instance snapshots for KVM UEFI VMs now include a sidecar copy of
+    the active NVRAM state so revert operations restore both disk and firmware
+    boot state consistently.
+
+  * UEFI disk-only instance snapshots taken before this change do not contain an
+    NVRAM sidecar and cannot be safely reverted. Take a new snapshot after
+    upgrading before relying on revert for UEFI VMs.

agent/conf/agent.properties

@@ -472,4 +472,3 @@ iscsi.session.cleanup.enabled=false
 # Optional vCenter SHA1 thumbprint for VMware to KVM conversion via VDDK, passed as
 # -io vddk-thumbprint=<value>. If unset, CloudStack computes it on the KVM host via openssl.
 #vddk.thumbprint=
-

api/src/main/java/com/cloud/host/Host.java

@@ -55,6 +55,7 @@ public static String[] toStrings(Host.Type... types) {
     }
 
     String HOST_UEFI_ENABLE = "host.uefi.enable";
+    String HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM = "host.kvm.diskonlyvmsnapshot.nvram";
     String HOST_VOLUME_ENCRYPTION = "host.volume.encryption";
     String HOST_INSTANCE_CONVERSION = "host.instance.conversion";
     String HOST_VDDK_SUPPORT = "host.vddk.support";

core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotAnswer.java

@@ -27,13 +27,24 @@
 public class CreateDiskOnlyVmSnapshotAnswer extends Answer {
 
     protected Map<String, Pair<Long, String>> mapVolumeToSnapshotSizeAndNewVolumePath;
+    private String nvramSnapshotPath;
 
     public CreateDiskOnlyVmSnapshotAnswer(Command command, boolean success, String details, Map<String, Pair<Long, String>> mapVolumeToSnapshotSizeAndNewVolumePath) {
+        this(command, success, details, mapVolumeToSnapshotSizeAndNewVolumePath, null);
+    }
+
+    public CreateDiskOnlyVmSnapshotAnswer(Command command, boolean success, String details, Map<String, Pair<Long, String>> mapVolumeToSnapshotSizeAndNewVolumePath,
+            String nvramSnapshotPath) {
         super(command, success, details);
         this.mapVolumeToSnapshotSizeAndNewVolumePath = mapVolumeToSnapshotSizeAndNewVolumePath;
+        this.nvramSnapshotPath = nvramSnapshotPath;
     }
 
     public Map<String, Pair<Long, String>> getMapVolumeToSnapshotSizeAndNewVolumePath() {
         return mapVolumeToSnapshotSizeAndNewVolumePath;
     }
+
+    public String getNvramSnapshotPath() {
+        return nvramSnapshotPath;
+    }
 }

core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotCommand.java

@@ -29,13 +29,30 @@
 public class CreateDiskOnlyVmSnapshotCommand extends VMSnapshotBaseCommand {
 
     protected VirtualMachine.State vmState;
+    private final String vmUuid;
+    private final boolean uefiEnabled;
 
     public CreateDiskOnlyVmSnapshotCommand(String vmName, VMSnapshotTO snapshot, List<VolumeObjectTO> volumeTOs, String guestOSType, VirtualMachine.State vmState) {
+        this(vmName, null, snapshot, volumeTOs, guestOSType, vmState, false);
+    }
+
+    public CreateDiskOnlyVmSnapshotCommand(String vmName, String vmUuid, VMSnapshotTO snapshot, List<VolumeObjectTO> volumeTOs, String guestOSType,
+            VirtualMachine.State vmState, boolean uefiEnabled) {
         super(vmName, snapshot, volumeTOs, guestOSType);
+        this.vmUuid = vmUuid;
         this.vmState = vmState;
+        this.uefiEnabled = uefiEnabled;
     }
 
     public VirtualMachine.State getVmState() {
         return vmState;
     }
+
+    public String getVmUuid() {
+        return vmUuid;
+    }
+
+    public boolean isUefiEnabled() {
+        return uefiEnabled;
+    }
 }

core/src/main/java/com/cloud/agent/api/storage/DeleteDiskOnlyVmSnapshotCommand.java

@@ -19,24 +19,43 @@
 package com.cloud.agent.api.storage;
 
 import com.cloud.agent.api.Command;
-
 import com.cloud.agent.api.to.DataTO;
-
+import org.apache.cloudstack.storage.to.PrimaryDataStoreTO;
 
 import java.util.List;
 
 public class DeleteDiskOnlyVmSnapshotCommand extends Command {
 
-    List<DataTO> snapshots;
+    private final List<DataTO> snapshots;
+    private final String nvramSnapshotPath;
+    private final PrimaryDataStoreTO primaryDataStore;
 
     public DeleteDiskOnlyVmSnapshotCommand(List<DataTO> snapshots) {
+        this(snapshots, null);
+    }
+
+    public DeleteDiskOnlyVmSnapshotCommand(List<DataTO> snapshots, String nvramSnapshotPath) {
+        this(snapshots, nvramSnapshotPath, null);
+    }
+
+    public DeleteDiskOnlyVmSnapshotCommand(List<DataTO> snapshots, String nvramSnapshotPath, PrimaryDataStoreTO primaryDataStore) {
         this.snapshots = snapshots;
+        this.nvramSnapshotPath = nvramSnapshotPath;
+        this.primaryDataStore = primaryDataStore;
     }
 
     public List<DataTO> getSnapshots() {
         return snapshots;
     }
 
+    public String getNvramSnapshotPath() {
+        return nvramSnapshotPath;
+    }
+
+    public PrimaryDataStoreTO getPrimaryDataStore() {
+        return primaryDataStore;
+    }
+
     @Override
     public boolean executeInSequence() {
         return false;

core/src/main/java/com/cloud/agent/api/storage/RevertDiskOnlyVmSnapshotCommand.java

@@ -27,11 +27,21 @@ public class RevertDiskOnlyVmSnapshotCommand extends Command {
 
     private List<SnapshotObjectTO> snapshotObjectTos;
     private String vmName;
+    private final String vmUuid;
+    private final boolean uefiEnabled;
+    private final String nvramSnapshotPath;
 
     public RevertDiskOnlyVmSnapshotCommand(List<SnapshotObjectTO> snapshotObjectTos, String vmName) {
+        this(snapshotObjectTos, vmName, null, false, null);
+    }
+
+    public RevertDiskOnlyVmSnapshotCommand(List<SnapshotObjectTO> snapshotObjectTos, String vmName, String vmUuid, boolean uefiEnabled, String nvramSnapshotPath) {
         super();
         this.snapshotObjectTos = snapshotObjectTos;
         this.vmName = vmName;
+        this.vmUuid = vmUuid;
+        this.uefiEnabled = uefiEnabled;
+        this.nvramSnapshotPath = nvramSnapshotPath;
     }
 
     public List<SnapshotObjectTO> getSnapshotObjectTos() {
@@ -42,6 +52,18 @@ public String getVmName() {
         return vmName;
     }
 
+    public String getVmUuid() {
+        return vmUuid;
+    }
+
+    public boolean isUefiEnabled() {
+        return uefiEnabled;
+    }
+
+    public String getNvramSnapshotPath() {
+        return nvramSnapshotPath;
+    }
+
     @Override
     public boolean executeInSequence() {
         return false;

engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java

@@ -108,10 +108,12 @@
 import com.cloud.exception.UnsupportedVersionException;
 import com.cloud.ha.HighAvailabilityManager;
 import com.cloud.host.Host;
+import com.cloud.host.DetailVO;
 import com.cloud.host.HostVO;
 import com.cloud.host.Status;
 import com.cloud.host.Status.Event;
 import com.cloud.host.dao.HostDao;
+import com.cloud.host.dao.HostDetailsDao;
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.hypervisor.HypervisorGuruManager;
 import com.cloud.org.Cluster;
@@ -167,6 +169,8 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl
     @Inject
     protected HostDao _hostDao = null;
     @Inject
+    protected HostDetailsDao _hostDetailsDao = null;
+    @Inject
     private ManagementServerHostDao _mshostDao;
     @Inject
     protected OutOfBandManagementDao outOfBandManagementDao;
@@ -802,18 +806,24 @@ protected AgentAttache notifyMonitorsOfConnection(final AgentAttache attache, fi
             ReadyAnswer readyAnswer = (ReadyAnswer)answer;
             Map<String, String> detailsMap = readyAnswer.getDetailsMap();
             if (detailsMap != null) {
+                _hostDao.loadDetails(host);
+                if (host.getDetails() == null) {
+                    host.setDetails(new HashMap<>());
+                }
                 String uefiEnabled = detailsMap.get(Host.HOST_UEFI_ENABLE);
+                String diskOnlyVmSnapshotNvramSupport = detailsMap.get(Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM);
                 String virtv2vVersion = detailsMap.get(Host.HOST_VIRTV2V_VERSION);
                 String ovftoolVersion = detailsMap.get(Host.HOST_OVFTOOL_VERSION);
                 String vddkSupport = detailsMap.get(Host.HOST_VDDK_SUPPORT);
                 String vddkLibDir = detailsMap.get(Host.HOST_VDDK_LIB_DIR);
                 String vddkVersion = detailsMap.get(Host.HOST_VDDK_VERSION);
                 logger.debug("Got HOST_UEFI_ENABLE [{}] for host [{}]:", uefiEnabled, host);
-                if (ObjectUtils.anyNotNull(uefiEnabled, virtv2vVersion, ovftoolVersion, vddkSupport, vddkLibDir, vddkVersion)) {
-                    _hostDao.loadDetails(host);
+                if (ObjectUtils.anyNotNull(uefiEnabled, diskOnlyVmSnapshotNvramSupport, virtv2vVersion, ovftoolVersion, vddkSupport, vddkLibDir, vddkVersion)) {
                     boolean updateNeeded = false;
-                    if (StringUtils.isNotBlank(uefiEnabled) && !uefiEnabled.equals(host.getDetails().get(Host.HOST_UEFI_ENABLE))) {
-                        host.getDetails().put(Host.HOST_UEFI_ENABLE, uefiEnabled);
+                    if (syncBooleanHostCapability(host, Host.HOST_UEFI_ENABLE, uefiEnabled)) {
+                        updateNeeded = true;
+                    }
+                    if (syncBooleanHostCapability(host, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, diskOnlyVmSnapshotNvramSupport)) {
                         updateNeeded = true;
                     }
                     if (StringUtils.isNotBlank(virtv2vVersion) && !virtv2vVersion.equals(host.getDetails().get(Host.HOST_VIRTV2V_VERSION))) {
@@ -856,6 +866,26 @@ protected AgentAttache notifyMonitorsOfConnection(final AgentAttache attache, fi
         return attache;
     }
 
+    protected boolean syncBooleanHostCapability(HostVO host, String capabilityName, String advertisedValue) {
+        if (StringUtils.isNotBlank(advertisedValue)) {
+            if (!advertisedValue.equals(host.getDetails().get(capabilityName))) {
+                host.getDetails().put(capabilityName, advertisedValue);
+                return true;
+            }
+            return false;
+        }
+
+        if (host.getDetails().containsKey(capabilityName)) {
+            host.getDetails().remove(capabilityName);
+            DetailVO hostDetail = _hostDetailsDao.findDetail(host.getId(), capabilityName);
+            if (hostDetail != null) {
+                _hostDetailsDao.remove(hostDetail.getId());
+            }
+            return true;
+        }
+        return false;
+    }
+
     @Override
     public boolean start() {
         ManagementServerHostVO msHost = _mshostDao.findByMsid(_nodeId);

engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java

@@ -18,14 +18,17 @@
 
 import com.cloud.agent.Listener;
 import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.ReadyAnswer;
 import com.cloud.agent.api.ReadyCommand;
 import com.cloud.agent.api.StartupCommand;
 import com.cloud.agent.api.StartupRoutingCommand;
 import com.cloud.exception.ConnectionException;
+import com.cloud.host.DetailVO;
 import com.cloud.host.Host;
 import com.cloud.host.HostVO;
 import com.cloud.host.Status;
 import com.cloud.host.dao.HostDao;
+import com.cloud.host.dao.HostDetailsDao;
 import com.cloud.hypervisor.Hypervisor;
 import com.cloud.utils.Pair;
 import org.junit.Assert;
@@ -34,10 +37,13 @@
 import org.mockito.Mockito;
 
 import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Map;
 
 public class AgentManagerImplTest {
 
     private HostDao hostDao;
+    private HostDetailsDao hostDetailsDao;
     private Listener storagePoolMonitor;
     private AgentAttache attache;
     private AgentManagerImpl mgr = Mockito.spy(new AgentManagerImpl());
@@ -46,15 +52,18 @@ public class AgentManagerImplTest {
 
     @Before
     public void setUp() throws Exception {
-        host = new HostVO("some-Uuid");
+        host = Mockito.spy(new HostVO("some-Uuid"));
+        Mockito.when(host.getId()).thenReturn(1L);
         host.setDataCenterId(1L);
         cmds = new StartupCommand[]{new StartupRoutingCommand()};
         attache = new ConnectedAgentAttache(null, 1L, "uuid", "kvm-attache", Hypervisor.HypervisorType.KVM, null, false);
 
         hostDao = Mockito.mock(HostDao.class);
+        hostDetailsDao = Mockito.mock(HostDetailsDao.class);
         storagePoolMonitor = Mockito.mock(Listener.class);
 
         mgr._hostDao = hostDao;
+        mgr._hostDetailsDao = hostDetailsDao;
         mgr._hostMonitors = new ArrayList<>();
         mgr._hostMonitors.add(new Pair<>(0, storagePoolMonitor));
     }
@@ -86,6 +95,32 @@ public void testNotifyMonitorsOfConnectionWhenStoragePoolConnectionHostFailure()
         Mockito.verify(mgr, Mockito.times(1)).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.eq(Status.Event.AgentDisconnected), Mockito.eq(true), Mockito.eq(true));
     }
 
+    @Test
+    public void testNotifyMonitorsOfConnectionClearsStaleNvramCapabilityOnReconnect() throws ConnectionException {
+        DetailVO staleNvramCapability = Mockito.mock(DetailVO.class);
+        ReadyAnswer readyAnswer = Mockito.mock(ReadyAnswer.class);
+        host.setDetails(new HashMap<>(Map.of(Host.HOST_UEFI_ENABLE, Boolean.TRUE.toString(),
+                Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())));
+
+        Mockito.when(staleNvramCapability.getId()).thenReturn(11L);
+        Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(host);
+        Mockito.doNothing().when(hostDao).loadDetails(host);
+        Mockito.when(hostDetailsDao.findDetail(host.getId(), Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)).thenReturn(staleNvramCapability);
+        Mockito.doNothing().when(storagePoolMonitor).processConnect(Mockito.eq(host), Mockito.eq(cmds[0]), Mockito.eq(false));
+        Mockito.doReturn(true).when(mgr).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.any(Status.Event.class), Mockito.anyBoolean(), Mockito.anyBoolean());
+        Mockito.when(readyAnswer.getResult()).thenReturn(true);
+        Mockito.when(readyAnswer.getDetailsMap()).thenReturn(Map.of(Host.HOST_UEFI_ENABLE, Boolean.TRUE.toString()));
+        Mockito.doReturn(readyAnswer).when(mgr).easySend(Mockito.anyLong(), Mockito.any(ReadyCommand.class));
+        Mockito.doReturn(true).when(mgr).agentStatusTransitTo(Mockito.eq(host), Mockito.eq(Status.Event.Ready), Mockito.anyLong());
+
+        final AgentAttache agentAttache = mgr.notifyMonitorsOfConnection(attache, cmds, false);
+
+        Assert.assertTrue(agentAttache.isReady());
+        Assert.assertFalse(host.getDetails().containsKey(Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM));
+        Mockito.verify(hostDetailsDao).remove(11L);
+        Mockito.verify(hostDao).saveDetails(host);
+    }
+
     @Test
     public void testGetTimeoutWithPositiveTimeout() {
         Commands commands = Mockito.mock(Commands.class);