Mark libvirt events experimental, add properties flag (#8825)

* Mark libvirt events experimental, add properties flag

* unit test fixes

---------

Co-authored-by: Marcus Sorensen <mls@apple.com>

agent/conf/agent.properties

@@ -426,3 +426,7 @@ iscsi.session.cleanup.enabled=false
 
 # Instance Conversion from Vmware to KVM through virt-v2v. Enable verbose mode
 # virtv2v.verbose.enabled=false
+
+# If set to "true", the agent will register for libvirt domain events, allowing for immediate updates on crashed or
+# unexpectedly stopped. Experimental, requires agent restart.
+# libvirt.events.enabled=false

agent/src/main/java/com/cloud/agent/properties/AgentProperties.java

@@ -695,6 +695,13 @@ public class AgentProperties{
      */
     public static final Property<Boolean> DEVELOPER = new Property<>("developer", false);
 
+    /**
+     * If set to "true", the agent will register for libvirt domain events, allowing for immediate updates on crashed or unexpectedly
+     * stopped VMs. Experimental, requires agent restart.
+     * Default value: <code>false</code>
+     */
+    public static final Property<Boolean> LIBVIRT_EVENTS_ENABLED = new Property<>("libvirt.events.enabled", false);
+
     /**
      * Can only be used if developer = true. This property is used to define the local bridge name and private network name.<br>
      * Data type: String.<br>

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

@@ -92,9 +92,6 @@ import org.libvirt.SchedParameter;
 import org.libvirt.SchedUlongParameter;
 import org.libvirt.Secret;
 import org.libvirt.VcpuInfo;
-import org.libvirt.event.DomainEvent;
-import org.libvirt.event.DomainEventDetail;
-import org.libvirt.event.StoppedDetail;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 import org.w3c.dom.Node;
@@ -468,7 +465,7 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv
     protected CPUStat cpuStat = new CPUStat();
     protected MemStat memStat = new MemStat(dom0MinMem, dom0OvercommitMem);
     private final LibvirtUtilitiesHelper libvirtUtilitiesHelper = new LibvirtUtilitiesHelper();
-    private AgentStatusUpdater _agentStatusUpdater;
+    private LibvirtDomainListener libvirtDomainListener;
 
     protected Boolean enableManuallySettingCpuTopologyOnKvmVm = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.ENABLE_MANUALLY_SETTING_CPU_TOPOLOGY_ON_KVM_VM);
 
@@ -502,8 +499,23 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv
     }
 
     @Override
-    public void registerStatusUpdater(AgentStatusUpdater updater) {
+    public synchronized void registerStatusUpdater(AgentStatusUpdater updater) {
-        _agentStatusUpdater = updater;
+        if (AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LIBVIRT_EVENTS_ENABLED)) {
+            try {
+                Connect conn = LibvirtConnection.getConnection();
+                if (libvirtDomainListener != null) {
+                    s_logger.debug("Clearing old domain listener");
+                    conn.removeLifecycleListener(libvirtDomainListener);
+                }
+                libvirtDomainListener = new LibvirtDomainListener(updater);
+                conn.addLifecycleListener(libvirtDomainListener);
+                s_logger.debug("Set up the libvirt domain event lifecycle listener");
+            } catch (LibvirtException e) {
+                s_logger.error("Failed to get libvirt connection for domain event lifecycle", e);
+            }
+        } else {
+            s_logger.debug("Libvirt event listening is disabled, not registering status updater");
+        }
     }
 
     @Override
@@ -1878,6 +1890,10 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv
     public boolean stop() {
         try {
             final Connect conn = LibvirtConnection.getConnection();
+            if (AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LIBVIRT_EVENTS_ENABLED) && libvirtDomainListener != null) {
+                s_logger.debug("Clearing old domain listener");
+                conn.removeLifecycleListener(libvirtDomainListener);
+            }
             conn.close();
         } catch (final LibvirtException e) {
             s_logger.trace("Ignoring libvirt error.", e);
@@ -3688,50 +3704,9 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv
         } catch (final CloudRuntimeException e) {
             s_logger.debug("Unable to initialize local storage pool: " + e);
         }
-        setupLibvirtEventListener();
         return sscmd;
     }
 
-    private void setupLibvirtEventListener() {
-        try {
-            Connect conn = LibvirtConnection.getConnection();
-            conn.addLifecycleListener(this::onDomainLifecycleChange);
-
-            s_logger.debug("Set up the libvirt domain event lifecycle listener");
-        } catch (LibvirtException e) {
-            s_logger.error("Failed to get libvirt connection for domain event lifecycle", e);
-        }
-    }
-
-    private int onDomainLifecycleChange(Domain domain, DomainEvent domainEvent) {
-        try {
-            s_logger.debug(String.format("Got event lifecycle change on Domain %s, event %s", domain.getName(), domainEvent));
-            if (domainEvent != null) {
-                switch (domainEvent.getType()) {
-                    case STOPPED:
-                        /* libvirt-destroyed VMs have detail StoppedDetail.DESTROYED, self shutdown guests are StoppedDetail.SHUTDOWN
-                         * Checking for this helps us differentiate between events where cloudstack or admin stopped the VM vs guest
-                         * initiated, and avoid pushing extra updates for actions we are initiating without a need for extra tracking */
-                        DomainEventDetail detail = domainEvent.getDetail();
-                        if (StoppedDetail.SHUTDOWN.equals(detail) || StoppedDetail.CRASHED.equals(detail) || StoppedDetail.FAILED.equals(detail)) {
-                            s_logger.info("Triggering out of band status update due to completed self-shutdown or crash of VM");
-                            _agentStatusUpdater.triggerUpdate();
-                        } else {
-                            s_logger.debug("Event detail: " + detail);
-                        }
-                        break;
-                    default:
-                        s_logger.debug(String.format("No handling for event %s", domainEvent));
-                }
-            }
-        } catch (LibvirtException e) {
-            s_logger.error("Libvirt exception while processing lifecycle event", e);
-        } catch (Throwable e) {
-            s_logger.error("Error during lifecycle", e);
-        }
-        return 0;
-    }
-
     public String diskUuidToSerial(String uuid) {
         String uuidWithoutHyphen = uuid.replace("-","");
         return uuidWithoutHyphen.substring(0, Math.min(uuidWithoutHyphen.length(), 20));

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtConnection.java

@@ -41,7 +41,7 @@ public class LibvirtConnection {
         return getConnection(s_hypervisorURI);
     }
 
-    static public Connect getConnection(String hypervisorURI) throws LibvirtException {
+    static synchronized public Connect getConnection(String hypervisorURI) throws LibvirtException {
         s_logger.debug("Looking for libvirtd connection at: " + hypervisorURI);
         Connect conn = s_connections.get(hypervisorURI);
 
@@ -121,6 +121,11 @@ public class LibvirtConnection {
      * @throws LibvirtException
      */
     private static synchronized void setupEventListener() throws LibvirtException {
+        if (!AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LIBVIRT_EVENTS_ENABLED)) {
+            s_logger.debug("Libvirt event listening is disabled, not setting up event loop");
+            return;
+        }
+
         if (libvirtEventThread == null || !libvirtEventThread.isAlive()) {
             // Registers a default event loop, must be called before connecting to hypervisor
             Library.initEventLoop();

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainListener.java

@@ -0,0 +1,65 @@
+/*
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.cloud.hypervisor.kvm.resource;
+
+import com.cloud.resource.AgentStatusUpdater;
+import org.apache.log4j.Logger;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+import org.libvirt.event.DomainEvent;
+import org.libvirt.event.DomainEventDetail;
+import org.libvirt.event.LifecycleListener;
+import org.libvirt.event.StoppedDetail;
+
+public class LibvirtDomainListener implements LifecycleListener {
+    private static final Logger LOGGER = Logger.getLogger(LibvirtDomainListener.class);
+
+    private final AgentStatusUpdater agentStatusUpdater;
+
+    public LibvirtDomainListener(AgentStatusUpdater updater) {
+        agentStatusUpdater = updater;
+    }
+
+    public int onLifecycleChange(Domain domain, DomainEvent domainEvent) {
+        try {
+            LOGGER.debug(String.format("Got event lifecycle change on Domain %s, event %s", domain.getName(), domainEvent));
+            if (domainEvent != null) {
+                switch (domainEvent.getType()) {
+                    case STOPPED:
+                        /* libvirt-destroyed VMs have detail StoppedDetail.DESTROYED, self shutdown guests are StoppedDetail.SHUTDOWN
+                         * Checking for this helps us differentiate between events where cloudstack or admin stopped the VM vs guest
+                         * initiated, and avoid pushing extra updates for actions we are initiating without a need for extra tracking */
+                        DomainEventDetail detail = domainEvent.getDetail();
+                        if (StoppedDetail.SHUTDOWN.equals(detail) || StoppedDetail.CRASHED.equals(detail) || StoppedDetail.FAILED.equals(detail)) {
+                            if (agentStatusUpdater != null) {
+                                LOGGER.info("Triggering out of band status update due to completed self-shutdown or crash of VM");
+                                agentStatusUpdater.triggerUpdate();
+                            }
+                        } else {
+                            LOGGER.debug("Event detail: " + detail);
+                        }
+                        break;
+                    default:
+                        LOGGER.debug(String.format("No handling for event %s", domainEvent));
+                }
+            }
+        } catch (LibvirtException e) {
+            LOGGER.error("Libvirt exception while processing lifecycle event", e);
+        } catch (Throwable e) {
+            LOGGER.error("Error during lifecycle", e);
+        }
+        return 0;
+    }
+}

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtScaleVmCommandWrapper.java

@@ -59,14 +59,6 @@ public class LibvirtScaleVmCommandWrapper extends CommandWrapper<ScaleVmCommand,
             String message = String.format("Unable to scale %s due to [%s].", scalingDetails, e.getMessage());
             logger.error(message, e);
             return new ScaleVmAnswer(command, false, message);
-        } finally {
-            if (conn != null) {
-                try {
-                    conn.close();
-                } catch (LibvirtException ex) {
-                    logger.warn(String.format("Error trying to close libvirt connection [%s]", ex.getMessage()), ex);
-                }
-            }
         }
     }
 

plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/linux/KVMHostInfo.java

@@ -222,7 +222,6 @@ public class KVMHostInfo {
                 We used to check if this was supported, but that is no longer required
             */
             this.capabilities.add("snapshot");
-            conn.close();
         } catch (final LibvirtException e) {
             LOGGER.error("Caught libvirt exception while fetching host information", e);
         }

plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/linux/KVMHostInfoTest.java

@@ -70,7 +70,6 @@ public class KVMHostInfoTest {
             Mockito.when(LibvirtConnection.getConnection()).thenReturn(conn);
             Mockito.when(conn.nodeInfo()).thenReturn(nodeInfo);
             Mockito.when(conn.getCapabilities()).thenReturn(capabilitiesXml);
-            Mockito.when(conn.close()).thenReturn(0);
             int manualSpeed = 500;
 
             KVMHostInfo kvmHostInfo = new KVMHostInfo(10, 10, manualSpeed, 0);
@@ -92,8 +91,6 @@ public class KVMHostInfoTest {
             Mockito.when(LibvirtConnection.getConnection()).thenReturn(conn);
             Mockito.when(conn.nodeInfo()).thenReturn(nodeInfo);
             Mockito.when(conn.getCapabilities()).thenReturn(capabilitiesXml);
-            Mockito.when(conn.close()).thenReturn(0);
-            int manualSpeed = 500;
 
             KVMHostInfo kvmHostInfo = new KVMHostInfo(10, 10, 100, 2);
             Assert.assertEquals("reserve two CPU cores", 8, kvmHostInfo.getAllocatableCpus());