From 30d07a50dfed2d61c86de451ab8ea7aaa13eb3be Mon Sep 17 00:00:00 2001
From: Thomas Florio <thomas.florio@suse.com>
Date: Mon, 11 Mar 2024 14:19:04 +0100
Subject: [PATCH 1/1] Handle fallback host identifier

This modification allows virtual-host-gatherer to change the field used
to identify the host. We had to do this for VMWare module, since the
current one was not reliable when the host was updated, causing system
duplication in uyuni
---
 .../taskomatic/task/gatherer/GathererJob.java | 13 +---
 .../gatherer/VirtualHostManagerProcessor.java | 75 ++++++++++++++-----
 .../suse/manager/gatherer/GathererRunner.java | 20 ++---
 .../com/suse/manager/gatherer/HostJson.java   | 22 ++++++
 4 files changed, 92 insertions(+), 38 deletions(-)
#L3PD: bsc#1218724

diff --git a/java/code/src/com/redhat/rhn/taskomatic/task/gatherer/GathererJob.java b/java/code/src/com/redhat/rhn/taskomatic/task/gatherer/GathererJob.java
index a1d7f233e96..c63842262cf 100644
--- a/java/code/src/com/redhat/rhn/taskomatic/task/gatherer/GathererJob.java
+++ b/java/code/src/com/redhat/rhn/taskomatic/task/gatherer/GathererJob.java
@@ -69,29 +69,24 @@ public class GathererJob extends RhnJavaJob {
             return;
         }
 
-        log.debug(String.format("Running gatherer for %d Virtual Host Managers",
-                managers.size()));
+        log.debug("Running gatherer for {} Virtual Host Managers", managers.size());
 
         try {
             Map<String, Map<String, HostJson>> results = new GathererRunner().run(managers);
             if (results == null) {
                 return;
             }
-            log.debug(String.format("Got %d Virtual Host Managers from gatherer",
-                    results.size()));
+            log.debug("Got {} Virtual Host Managers from gatherer", results.size());
 
             for (VirtualHostManager manager : managers) {
                 String label = manager.getLabel();
 
                 if (!results.containsKey(label)) {
-                    log.warn(String.format("Virtual Host Manager with label '%s' is not " +
-                            "contained in the results from gatherer - skipping it.",
-                            label));
+                    log.warn("Virtual Host Manager with label {} not found in gatherer results - skipping it.", label);
                     continue;
                 }
                 log.debug("Processing {}", label);
-                new VirtualHostManagerProcessor(manager, results.get(label))
-                        .processMapping();
+                new VirtualHostManagerProcessor(manager, results.get(label)).processMapping();
             }
         }
         catch (Throwable t) {
diff --git a/java/code/src/com/redhat/rhn/taskomatic/task/gatherer/VirtualHostManagerProcessor.java b/java/code/src/com/redhat/rhn/taskomatic/task/gatherer/VirtualHostManagerProcessor.java
index abfbd3850d2..43b12615cd1 100644
--- a/java/code/src/com/redhat/rhn/taskomatic/task/gatherer/VirtualHostManagerProcessor.java
+++ b/java/code/src/com/redhat/rhn/taskomatic/task/gatherer/VirtualHostManagerProcessor.java
@@ -31,6 +31,7 @@ import com.redhat.rhn.manager.system.entitling.SystemEntitlementManager;
 import com.suse.manager.gatherer.HostJson;
 
 import org.apache.commons.lang3.RandomStringUtils;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
@@ -38,6 +39,7 @@ import java.security.SecureRandom;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 
 /**
@@ -48,12 +50,12 @@ import java.util.Set;
  */
 public class VirtualHostManagerProcessor {
 
+    private static final Logger LOGGER = LogManager.getLogger(VirtualHostManagerProcessor.class);
     private final VirtualHostManager virtualHostManager;
     private final Map<String, HostJson> virtualHosts;
-    private Set<Server> serversToDelete;
-    private Set<VirtualHostManagerNodeInfo> nodesToDelete;
-    private Logger log;
-    private SystemEntitlementManager systemEntitlementManager = GlobalInstanceHolder.SYSTEM_ENTITLEMENT_MANAGER;
+    private final Set<Server> serversToDelete;
+    private final Set<VirtualHostManagerNodeInfo> nodesToDelete;
+    private final SystemEntitlementManager systemEntitlementManager;
 
     /**
      * Instantiates a new virtual host manager processor, will update a virtual
@@ -62,13 +64,12 @@ public class VirtualHostManagerProcessor {
      * @param managerIn the virtual host manager
      * @param virtualHostsIn the virtual hosts information from JSON
      */
-    public VirtualHostManagerProcessor(VirtualHostManager managerIn,
-            Map<String, HostJson> virtualHostsIn) {
-        this.log = LogManager.getLogger(VirtualHostManagerProcessor.class);
+    public VirtualHostManagerProcessor(VirtualHostManager managerIn, Map<String, HostJson> virtualHostsIn) {
         this.virtualHostManager = managerIn;
         this.virtualHosts = virtualHostsIn;
         this.serversToDelete = new HashSet<>();
         this.nodesToDelete = new HashSet<>();
+        this.systemEntitlementManager = GlobalInstanceHolder.SYSTEM_ENTITLEMENT_MANAGER;
     }
 
     /**
@@ -79,24 +80,24 @@ public class VirtualHostManagerProcessor {
      * mapping.
      */
     public void processMapping() {
-        log.debug("Processing Virtual Host Manager: {}", virtualHostManager);
+        LOGGER.debug("Processing Virtual Host Manager: {}", virtualHostManager);
         if (virtualHosts == null) {
-            log.error("Virtual Host Manager {}: Please check the virtual-host-gatherer logfile.",
+            LOGGER.error("Virtual Host Manager {}: Please check the virtual-host-gatherer logfile.",
                     virtualHostManager.getLabel());
             return;
         }
         serversToDelete.addAll(virtualHostManager.getServers());
         nodesToDelete.addAll(virtualHostManager.getNodes());
         virtualHosts.forEach((key, value) -> {
-            log.debug("Processing host: {}", key);
+            LOGGER.debug("Processing host: {}", key);
             processVirtualHost(key, value);
         });
         serversToDelete.forEach(srv -> {
-            log.debug("Removing link to virtual host: {}", srv.getName());
+            LOGGER.debug("Removing link to virtual host: {}", srv.getName());
             virtualHostManager.removeServer(srv);
         });
         nodesToDelete.forEach(node -> {
-            log.debug("Removing virtual host node: {}", node.getName());
+            LOGGER.debug("Removing virtual host node: {}", node.getName());
             virtualHostManager.removeNode(node);
         });
     }
@@ -116,7 +117,7 @@ public class VirtualHostManagerProcessor {
                 VirtualHostManagerFactory.KUBERNETES);
         if (server == null) {
             VirtualHostManagerNodeInfo nodeInfo = updateAndGetNodeInfo(hostLabel, host);
-            if (!virtualHostManager.getNodes().contains(nodeInfo)) {
+            if (isNewInPersistentSet(virtualHostManager.getNodes(), nodeInfo)) {
                 virtualHostManager.getNodes().add(nodeInfo);
             }
             else {
@@ -126,7 +127,7 @@ public class VirtualHostManagerProcessor {
             // if one doesn't already exist
             return;
         }
-        if (!virtualHostManager.getServers().contains(server)) {
+        if (isNewInPersistentSet(virtualHostManager.getServers(), server)) {
             virtualHostManager.addServer(server);
         }
         else {
@@ -166,6 +167,18 @@ public class VirtualHostManagerProcessor {
         return updateNodeInfo(info, hostLabel, host);
     }
 
+    /**
+     * For a Hibernate PersistentSet, contains() doesn't always work as expected. So it's better to lookup explicitly
+     * @param persistedElements the PersistentSet
+     * @param itemToCheck the item to check
+     * @return true if the item is not part of the set
+     * @param <T> the type of the elements in the set
+     */
+    private <T> boolean isNewInPersistentSet(Set<T> persistedElements, T itemToCheck) {
+        return persistedElements.stream()
+            .noneMatch(e -> Objects.equals(e, itemToCheck));
+    }
+
     /**
      * Extracts virtual instance type from string. Falls back to para virtualization if the
      * requested string doesn't match to any existing virtualization type.
@@ -178,8 +191,7 @@ public class VirtualHostManagerProcessor {
                 VirtualInstanceFactory.getInstance().getVirtualInstanceType(candidate);
         if (type == null) { // fallback
             type = VirtualInstanceFactory.getInstance().getFullyVirtType();
-            log.warn(String.format("Can't find virtual instance type for string '%s'. " +
-                    "Defaulting to '%s'", candidate, type));
+            LOGGER.warn("Can't find virtual instance type for string '{}'. Defaulting to '{}'", candidate, type);
         }
         return type;
     }
@@ -197,8 +209,7 @@ public class VirtualHostManagerProcessor {
     private Server updateAndGetServer(String hostId,
                                       HostJson host,
                                       String skipCreateForType) {
-        Server server = ServerFactory.lookupForeignSystemByDigitalServerId(
-                buildServerFullDigitalId(host.getHostIdentifier()));
+        Server server = getServerByHost(host.getHostIdentifier(), host.getFallbackHostIdentifier());
         if (server == null) {
             if (skipCreateForType.equalsIgnoreCase(host.getType())) {
                 return null;
@@ -219,6 +230,32 @@ public class VirtualHostManagerProcessor {
         return server;
     }
 
+    /**
+     * For VMWare we had to change the value for the host identifier. This method retrieves the server using the new
+     * value and falling back to the old one, if available. If the server is retrieve using the old identifier, the
+     * digital host id is updated to reflect the new identifier.
+     * @param hostIdentifier the host identifier
+     * @param fallback the fallback value, used by VMWare module before the change
+     * @return the server if found, null otherwise.
+     */
+    private Server getServerByHost(String hostIdentifier, String fallback) {
+        // First use the hostIdentifier field
+        String digitalId = buildServerFullDigitalId(hostIdentifier);
+        Server server = ServerFactory.lookupForeignSystemByDigitalServerId(digitalId);
+        if (server != null || StringUtils.isEmpty(fallback)) {
+            return server;
+        }
+
+        // Fallback to the old host identifier field
+        server = ServerFactory.lookupForeignSystemByDigitalServerId(buildServerFullDigitalId(fallback));
+        // Update to the digital id to use the current host identifier
+        if (server != null) {
+            server.setDigitalServerId(digitalId);
+        }
+
+        return server;
+    }
+
     /**
      * Builds full digital id from virtual host manager id and the host identifier (string
      * consisting of those two separated by '-'. For instance: 1000000001-my_host_id).
@@ -277,7 +314,7 @@ public class VirtualHostManagerProcessor {
         }
 
         cpu.setArch(ServerFactory.lookupCPUArchByName(host.getCpuArch()));
-        cpu.setMHz(Long.valueOf(Math.round(host.getCpuMhz())).toString());
+        cpu.setMHz(Long.toString(Math.round(host.getCpuMhz())));
         if (host.getTotalCpuSockets().longValue() > 0L) {
             cpu.setNrsocket(host.getTotalCpuSockets().longValue());
             cpu.setNrCore(host.getTotalCpuCores().longValue() / host.getTotalCpuSockets().longValue());
diff --git a/java/code/src/com/suse/manager/gatherer/GathererRunner.java b/java/code/src/com/suse/manager/gatherer/GathererRunner.java
index 563f5d239f5..1189365e559 100644
--- a/java/code/src/com/suse/manager/gatherer/GathererRunner.java
+++ b/java/code/src/com/suse/manager/gatherer/GathererRunner.java
@@ -50,7 +50,7 @@ public class GathererRunner {
     /**
      * Logger for this class
      */
-    private static Logger logger = LogManager.getLogger(GathererRunner.class);
+    private static final Logger LOGGER = LogManager.getLogger(GathererRunner.class);
 
     /**
      * Call gatherer --list-modules and return the result
@@ -66,7 +66,7 @@ public class GathererRunner {
 
         int exitcode = e.execute(args.toArray(new String[0]));
         if (exitcode != 0) {
-            logger.error(e.getLastCommandErrorMessage());
+            LOGGER.error(e.getLastCommandErrorMessage());
             return null;
         }
         return new GathererJsonIO().readGathererModules(e.getLastCommandOutput());
@@ -105,10 +105,10 @@ public class GathererRunner {
                 String uri = builder.build().toString();
                 env.put("http_proxy", uri);
                 env.put("https_proxy", uri);
-                logger.debug("Set http(s)_proxy to {}", uri);
+                LOGGER.debug("Set http(s)_proxy to {}", uri);
             }
             catch (URISyntaxException e) {
-                logger.error("URI syntax exception when setting Proxy: {}", e.getMessage());
+                LOGGER.error("URI syntax exception when setting Proxy: {}", e.getMessage());
             }
         }
         int debuglevel = Config.get().getInt("debug", 0);
@@ -121,7 +121,7 @@ public class GathererRunner {
         String noProxy = Config.get().getString(HttpClientAdapter.NO_PROXY);
         if (!StringUtils.isEmpty(noProxy)) {
             env.put("no_proxy", noProxy);
-            logger.debug("Set no_proxy to {}", noProxy);
+            LOGGER.debug("Set no_proxy to {}", noProxy);
         }
 
         String[] envp = new String[env.size()];
@@ -149,7 +149,7 @@ public class GathererRunner {
                     }
                 }
                 catch (Exception e) {
-                    logger.error("Error reading stderr from external process", e);
+                    LOGGER.error("Error reading stderr from external process", e);
                 }
             });
             errStreamReader.start();
@@ -167,17 +167,17 @@ public class GathererRunner {
 
             int exitCode = p.waitFor();
             if (exitCode != 0) {
-                logger.error("Error while calling the virtual-host-gatherer, exit code {}", exitCode);
-                logger.error("Please check the virtual-host-gatherer logfile.");
+                LOGGER.error("Error while calling the virtual-host-gatherer, exit code {}", exitCode);
+                LOGGER.error("Please check the virtual-host-gatherer logfile.");
                 return null;
             }
         }
         catch (IOException ioe) {
-            logger.error("execute(String[])", ioe);
+            LOGGER.error("execute(String[])", ioe);
         }
         catch (InterruptedException e) {
             Thread.currentThread().interrupt();
-            logger.error("execute(String[])", e);
+            LOGGER.error("execute(String[])", e);
         }
         return hosts;
     }
diff --git a/java/code/src/com/suse/manager/gatherer/HostJson.java b/java/code/src/com/suse/manager/gatherer/HostJson.java
index 516fa822a6d..7b3f87b75cc 100644
--- a/java/code/src/com/suse/manager/gatherer/HostJson.java
+++ b/java/code/src/com/suse/manager/gatherer/HostJson.java
@@ -31,6 +31,12 @@ public class HostJson {
     /** Host Identifier. */
     private String hostIdentifier;
 
+    /**
+     * For VMWare we had to change the value for the host identifier. This fields contains the value previously used,
+     * to allow the code to match and update the value.
+     */
+    private String fallbackHostIdentifier;
+
     /** Total CPU socket count. */
     private Integer totalCpuSockets;
 
@@ -86,6 +92,14 @@ public class HostJson {
         return hostIdentifier;
     }
 
+    /**
+     * Gets the fallback host identifier
+     * @return the fallback host identifier
+     */
+    public String getFallbackHostIdentifier() {
+        return fallbackHostIdentifier;
+    }
+
     /**
      * Gets the total CPU socket count.
      * @return the number of sockets - can be 0 if insufficient data were send
@@ -208,6 +222,14 @@ public class HostJson {
         this.hostIdentifier = hostIdentifierIn;
     }
 
+    /**
+     * Sets the fallback host identifier.
+     * @param hostUuidIn the fallback host identifier
+     */
+    public void setFallbackHostIdentifier(String hostUuidIn) {
+        this.fallbackHostIdentifier = hostUuidIn;
+    }
+
     /**
      * Sets the total CPU socket count.
      * @param totalCpuSocketsIn the number of sockets
-- 
2.35.3
