A comprehensive solution for #CLOUDSTACK-9025.

The first PR(apache#1176) intended to solve #CLOUDSTACK-9025 was only tackling the problem for CloudStack deployments that use single hypervisor types (restricted to XenServer). Additionally, the lack of information regarding that solution (poor documentation, test cases and description in PRs and Jira ticket) led the code to be removed in apache#1124 after a long discussion and analysis in apache#1056. That piece of code seemed logicless (and it was!).  It would receive a hostId and then change that hostId for other hostId of the zone without doing any check; it was not even checking the hypervisor and storage in which the host was plugged into.

The problem reported in #CLOUDSTACK-9025 is caused by partial snapshots that are taken in XenServer. This means, we do not take a complete snapshot, but a partial one that contains only the modified data. This requires rebuilding the VHD hierarchy when creating a template out of the snapshot. The point is that the first hostId received is not a hostId, but a system VM ID(SSVM). That is why the code in apache#1176 fixed the problem for some deployment scenarios, but would cause problems for scenarios where we have multiple hypervisors in the same zone. We need to execute the creation of the VHD that represents the template in the hypervisor, so the VHD chain can be built using the parent links.

This commit changes the method com.cloud.hypervisor.XenServerGuru.getCommandHostDelegation(long, Command). From now on we replace the hostId that is intended to execute the “copy command” that will create the VHD of the template according to some conditions that were already in place. The idea is that starting with XenServer 6.2.0 hotFix ESP1004 we need to execute the command in the hypervisor host and not from the SSVM. Moreover, the method was improved making it readable and understandable; it was also created test cases assuring that from XenServer 6.2.0 hotFix ESP1004 and upward versions we change the hostId that will be used to execute the “copy command”.

Furthermore, we are not selecting a random host from a zone anymore. A new method was introduced in the HostDao called “findHostConnectedToSnapshotStoragePoolToExecuteCommand”, using this method we look for a host that is in the cluster that is using the storage pool where the volume from which the Snaphost is taken of. By doing this, we guarantee that the host that is connected to the primary storage where all of the snapshots parent VHDs are stored is used to create the template.

Author: rafaelweingartner

api/src/com/cloud/hypervisor/HypervisorGuru.java

@@ -33,7 +33,7 @@
 
 public interface HypervisorGuru extends Adapter {
     static final ConfigKey<Boolean> VmwareFullClone = new ConfigKey<Boolean>("Advanced", Boolean.class, "vmware.create.full.clone", "true",
-                        "If set to true, creates guest VMs as full clones on ESX", false);
+            "If set to true, creates guest VMs as full clones on ESX", false);
     HypervisorType getHypervisorType();
 
     /**
@@ -45,7 +45,7 @@ public interface HypervisorGuru extends Adapter {
     VirtualMachineTO implement(VirtualMachineProfile vm);
 
     /**
-     * Give hypervisor guru opportunity to decide if certain command needs to be delegated to other host, mainly to secondary storage VM host
+     * Give hypervisor guru opportunity to decide if certain command needs to be delegated to other host, mainly from the secondary storage VM host to a real hypervisor host
      *
      * @param hostId original hypervisor host
      * @param cmd command that is going to be sent, hypervisor guru usually needs to register various context objects into the command object

core/src/org/apache/cloudstack/storage/command/CopyCommand.java

@@ -24,7 +24,7 @@
 
 import com.cloud.agent.api.to.DataTO;
 
-public final class CopyCommand extends StorageSubSystemCommand {
+public class CopyCommand extends StorageSubSystemCommand {
     private DataTO srcTO;
     private DataTO destTO;
     private DataTO cacheTO;

engine/schema/src/com/cloud/host/dao/HostDao.java

@@ -72,14 +72,6 @@ public interface HostDao extends GenericDao<HostVO, Long>, StateDao<Status, Stat
 
     List<HostVO> findHypervisorHostInCluster(long clusterId);
 
-    /**
-     * @param type
-     * @param clusterId
-     * @param podId
-     * @param dcId
-     * @param haTag TODO
-     * @return
-     */
     List<HostVO> listAllUpAndEnabledNonHAHosts(Type type, Long clusterId, Long podId, long dcId, String haTag);
 
     List<HostVO> findByDataCenterId(Long zoneId);
@@ -103,4 +95,13 @@ public interface HostDao extends GenericDao<HostVO, Long>, StateDao<Status, Stat
     List<HostVO> listByType(Type type);
 
     HostVO findByIp(String ip);
+
+    /**
+     * This method will look for a host that is connected to the storage pool where the volume of the Snapshot is stored.
+     * <ul>
+     * <li>If the storage pool found for the volume of the snapshotId has more than a host, we will choose one randomly;
+     * <li>If no host is found, we throw a runtime exception
+     * </ul>
+     */
+    HostVO findHostToOperateOnSnapshot(long snapshotId);
 }

engine/schema/src/com/cloud/host/dao/HostDaoImpl.java

@@ -27,11 +27,9 @@
 import java.util.TimeZone;
 
 import javax.annotation.PostConstruct;
-import javax.ejb.Local;
 import javax.inject.Inject;
 import javax.persistence.TableGenerator;
 
-import com.cloud.utils.NumbersUtil;
 import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
 import org.apache.log4j.Logger;
 import org.springframework.stereotype.Component;
@@ -55,6 +53,7 @@
 import com.cloud.org.Managed;
 import com.cloud.resource.ResourceState;
 import com.cloud.utils.DateUtil;
+import com.cloud.utils.NumbersUtil;
 import com.cloud.utils.db.Attribute;
 import com.cloud.utils.db.DB;
 import com.cloud.utils.db.Filter;
@@ -70,9 +69,8 @@
 import com.cloud.utils.db.UpdateBuilder;
 import com.cloud.utils.exception.CloudRuntimeException;
 
-@Component
-@Local(value = {HostDao.class})
 @DB
+@Component
 @TableGenerator(name = "host_req_sq", table = "op_host", pkColumnName = "id", valueColumnName = "sequence", allocationSize = 1)
 public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao { //FIXME: , ExternalIdDao {
     private static final Logger s_logger = Logger.getLogger(HostDaoImpl.class);
@@ -307,11 +305,11 @@ public void init() {
         }
         HostTransferSearch.and("id", HostTransferSearch.entity().getId(), SearchCriteria.Op.NULL);
         UnmanagedDirectConnectSearch.join("hostTransferSearch", HostTransferSearch, HostTransferSearch.entity().getId(), UnmanagedDirectConnectSearch.entity().getId(),
-            JoinType.LEFTOUTER);
+                JoinType.LEFTOUTER);
         ClusterManagedSearch = _clusterDao.createSearchBuilder();
         ClusterManagedSearch.and("managed", ClusterManagedSearch.entity().getManagedState(), SearchCriteria.Op.EQ);
-        UnmanagedDirectConnectSearch.join("ClusterManagedSearch", ClusterManagedSearch, ClusterManagedSearch.entity().getId(), UnmanagedDirectConnectSearch.entity()
-            .getClusterId(), JoinType.INNER);
+        UnmanagedDirectConnectSearch.join("ClusterManagedSearch", ClusterManagedSearch, ClusterManagedSearch.entity().getId(), UnmanagedDirectConnectSearch.entity().getClusterId(),
+                JoinType.INNER);
         UnmanagedDirectConnectSearch.done();
 
         DirectConnectSearch = createSearchBuilder();
@@ -390,7 +388,8 @@ public void init() {
 
         ClusterManagedSearch = _clusterDao.createSearchBuilder();
         ClusterManagedSearch.and("managed", ClusterManagedSearch.entity().getManagedState(), SearchCriteria.Op.EQ);
-        ClustersForHostsNotOwnedByAnyMSSearch.join("ClusterManagedSearch", ClusterManagedSearch, ClusterManagedSearch.entity().getId(), ClustersForHostsNotOwnedByAnyMSSearch.entity().getClusterId(), JoinType.INNER);
+        ClustersForHostsNotOwnedByAnyMSSearch.join("ClusterManagedSearch", ClusterManagedSearch, ClusterManagedSearch.entity().getId(),
+                ClustersForHostsNotOwnedByAnyMSSearch.entity().getClusterId(), JoinType.INNER);
 
         ClustersForHostsNotOwnedByAnyMSSearch.done();
 
@@ -658,7 +657,7 @@ public List<HostVO> findAndUpdateApplianceToLoad(long lastPingSecondsAfter, long
         SearchCriteria<HostVO> sc = UnmanagedApplianceSearch.create();
         sc.setParameters("lastPinged", lastPingSecondsAfter);
         sc.setParameters("types", Type.ExternalDhcp, Type.ExternalFirewall, Type.ExternalLoadBalancer, Type.BaremetalDhcp, Type.BaremetalPxe, Type.TrafficMonitor,
-            Type.L2Networking, Type.NetScalerControlCenter);
+                Type.L2Networking, Type.NetScalerControlCenter);
         List<HostVO> hosts = lockRows(sc, null, true);
 
         for (HostVO host : hosts) {
@@ -792,11 +791,8 @@ public void loadHostTags(HostVO host) {
     @Override
     public List<HostVO> findLostHosts(long timeout) {
         List<HostVO> result = new ArrayList<HostVO>();
-        String sql =
-                "select h.id from host h left join  cluster c on h.cluster_id=c.id where h.mgmt_server_id is not null and h.last_ping < ? and h.status in ('Up', 'Updating', 'Disconnected', 'Connecting') and h.type not in ('ExternalFirewall', 'ExternalLoadBalancer', 'TrafficMonitor', 'SecondaryStorage', 'LocalSecondaryStorage', 'L2Networking') and (h.cluster_id is null or c.managed_state = 'Managed') ;";
-        try (
-                TransactionLegacy txn = TransactionLegacy.currentTxn();
-                PreparedStatement pstmt = txn.prepareStatement(sql);) {
+        String sql = "select h.id from host h left join  cluster c on h.cluster_id=c.id where h.mgmt_server_id is not null and h.last_ping < ? and h.status in ('Up', 'Updating', 'Disconnected', 'Connecting') and h.type not in ('ExternalFirewall', 'ExternalLoadBalancer', 'TrafficMonitor', 'SecondaryStorage', 'LocalSecondaryStorage', 'L2Networking') and (h.cluster_id is null or c.managed_state = 'Managed') ;";
+        try (TransactionLegacy txn = TransactionLegacy.currentTxn(); PreparedStatement pstmt = txn.prepareStatement(sql);) {
             pstmt.setLong(1, timeout);
             try (ResultSet rs = pstmt.executeQuery();) {
                 while (rs.next()) {
@@ -889,8 +885,7 @@ public boolean update(Long hostId, HostVO host) {
     @Override
     @DB
     public List<RunningHostCountInfo> getRunningHostCounts(Date cutTime) {
-        String sql =
-            "select * from (" + "select h.data_center_id, h.type, count(*) as count from host as h INNER JOIN mshost as m ON h.mgmt_server_id=m.msid "
+        String sql = "select * from (" + "select h.data_center_id, h.type, count(*) as count from host as h INNER JOIN mshost as m ON h.mgmt_server_id=m.msid "
                 + "where h.status='Up' and h.type='SecondaryStorage' and m.last_update > ? " + "group by h.data_center_id, h.type " + "UNION ALL "
                 + "select h.data_center_id, h.type, count(*) as count from host as h INNER JOIN mshost as m ON h.mgmt_server_id=m.msid "
                 + "where h.status='Up' and h.type='Routing' and m.last_update > ? " + "group by h.data_center_id, h.type) as t " + "ORDER by t.data_center_id, t.type";
@@ -1010,24 +1005,12 @@ public boolean updateState(Status oldStatus, Event event, Status newStatus, Host
 
                 StringBuilder str = new StringBuilder("Unable to update host for event:").append(event.toString());
                 str.append(". Name=").append(host.getName());
-                str.append("; New=[status=")
-                        .append(newStatus.toString())
-                        .append(":msid=")
-                        .append(newStatus.lostConnection() ? "null" : host.getManagementServerId())
-                        .append(":lastpinged=")
-                        .append(host.getLastPinged())
-                        .append("]");
+                str.append("; New=[status=").append(newStatus.toString()).append(":msid=").append(newStatus.lostConnection() ? "null" : host.getManagementServerId())
+                .append(":lastpinged=").append(host.getLastPinged()).append("]");
                 str.append("; Old=[status=").append(oldStatus.toString()).append(":msid=").append(host.getManagementServerId()).append(":lastpinged=").append(oldPingTime)
-                        .append("]");
-                str.append("; DB=[status=")
-                        .append(vo.getStatus().toString())
-                        .append(":msid=")
-                        .append(vo.getManagementServerId())
-                        .append(":lastpinged=")
-                        .append(vo.getLastPinged())
-                        .append(":old update count=")
-                        .append(oldUpdateCount)
-                        .append("]");
+                .append("]");
+                str.append("; DB=[status=").append(vo.getStatus().toString()).append(":msid=").append(vo.getManagementServerId()).append(":lastpinged=").append(vo.getLastPinged())
+                .append(":old update count=").append(oldUpdateCount).append("]");
                 status_logger.debug(str.toString());
             } else {
                 StringBuilder msg = new StringBuilder("Agent status update: [");
@@ -1194,4 +1177,26 @@ public List<HostVO> listByType(Host.Type type) {
         sc.setParameters("type", type);
         return listBy(sc);
     }
-}
+
+    String sqlFindHostConnectedToSnapshotStoragePoolToExecuteCommand = "select h.id from snapshots s join volumes v on v.id = s.volume_id "
+            + "join storage_pool pool on pool.id = v.pool_id join cluster c on pool.cluster_id = c.id join host h on h.cluster_id = c.id "
+            + "where s.id = ? and h.status = 'Up' and h.type = 'Routing' ORDER by rand() limit 1";
+
+    @Override
+    public HostVO findHostToOperateOnSnapshot(long snapshotId) {
+        TransactionLegacy tx = TransactionLegacy.currentTxn();
+        try {
+            PreparedStatement pstmt = tx.prepareAutoCloseStatement(sqlFindHostConnectedToSnapshotStoragePoolToExecuteCommand);
+            pstmt.setLong(1, snapshotId);
+            ResultSet rs = pstmt.executeQuery();
+            if (!rs.next()) {
+                throw new CloudRuntimeException(
+                        String.format("Could not find a host connected to a storage pool that is used to store the informed snapshot [snapshotId=%d]. ", snapshotId));
+            }
+            long hostId = rs.getLong("id");
+            return findById(hostId);
+        } catch (SQLException e) {
+            throw new CloudRuntimeException(e);
+        }
+    }
+}

plugins/hypervisors/xenserver/src/com/cloud/hypervisor/XenServerGuru.java

@@ -32,6 +32,7 @@
 import org.apache.cloudstack.storage.command.StorageSubSystemCommand;
 import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
+import org.apache.commons.lang.StringUtils;
 import org.apache.log4j.Logger;
 
 import com.cloud.agent.api.Command;
@@ -58,7 +59,9 @@
 import com.cloud.vm.dao.UserVmDao;
 
 public class XenServerGuru extends HypervisorGuruBase implements HypervisorGuru, Configurable {
-    private final Logger LOGGER = Logger.getLogger(XenServerGuru.class);
+
+    private Logger logger = Logger.getLogger(getClass());
+
     @Inject
     private GuestOSDao _guestOsDao;
     @Inject
@@ -74,7 +77,7 @@ public class XenServerGuru extends HypervisorGuruBase implements HypervisorGuru,
     @Inject
     private UserVmDao _userVmDao;
     @Inject
-    GuestOsDetailsDao _guestOsDetailsDao;
+    private GuestOsDetailsDao _guestOsDetailsDao;
 
     private static final ConfigKey<Integer> MaxNumberOfVCPUSPerVM = new ConfigKey<Integer>("Advanced", Integer.class, "xen.vm.vcpu.max", "16",
             "Maximum number of VCPUs that VM can get in XenServer.", true, ConfigKey.Scope.Cluster);
@@ -167,35 +170,61 @@ public List<Command> finalizeExpungeVolumes(VirtualMachine vm) {
 
     @Override
     public Pair<Boolean, Long> getCommandHostDelegation(long hostId, Command cmd) {
-        LOGGER.debug("getCommandHostDelegation: " + cmd.getClass());
         if (cmd instanceof StorageSubSystemCommand) {
             StorageSubSystemCommand c = (StorageSubSystemCommand)cmd;
             c.setExecuteInSequence(true);
         }
-        if (cmd instanceof CopyCommand) {
-            CopyCommand cpyCommand = (CopyCommand)cmd;
-            DataTO srcData = cpyCommand.getSrcTO();
-            DataTO destData = cpyCommand.getDestTO();
-
-            if (srcData.getHypervisorType() == HypervisorType.XenServer && srcData.getObjectType() == DataObjectType.SNAPSHOT &&
-                    destData.getObjectType() == DataObjectType.TEMPLATE) {
-                DataStoreTO srcStore = srcData.getDataStore();
-                DataStoreTO destStore = destData.getDataStore();
-                if (srcStore instanceof NfsTO && destStore instanceof NfsTO) {
-                    HostVO host = hostDao.findById(hostId);
-                    hostDao.loadDetails(host);
-                    String hypervisorVersion = host.getHypervisorVersion();
-                    String snapshotHotFixVersion = host.getDetail(XenserverConfigs.XS620HotFix);
-                    if (hypervisorVersion != null && !hypervisorVersion.equalsIgnoreCase("6.1.0")) {
-                        if (!(hypervisorVersion.equalsIgnoreCase("6.2.0") &&
-                                !(snapshotHotFixVersion != null && snapshotHotFixVersion.equalsIgnoreCase(XenserverConfigs.XSHotFix62ESP1004)))) {
-                            return new Pair<Boolean, Long>(Boolean.TRUE, new Long(host.getId()));
-                        }
-                    }
-                }
-            }
+        boolean isCopyCommand = cmd instanceof CopyCommand;
+        Pair<Boolean, Long> defaultHostToExecuteCommands = super.getCommandHostDelegation(hostId, cmd);
+        if (!isCopyCommand) {
+            logger.debug("We are returning the default host to execute commands because the command is not of Copy type.");
+            return defaultHostToExecuteCommands;
+        }
+        CopyCommand copyCommand = (CopyCommand)cmd;
+        DataTO srcData = copyCommand.getSrcTO();
+        DataTO destData = copyCommand.getDestTO();
+
+        boolean isSourceDataHypervisorXenServer = srcData.getHypervisorType() == HypervisorType.XenServer;
+        if (!isSourceDataHypervisorXenServer) {
+            logger.debug("We are returning the default host to execute commands because the target hypervisor of the source data is not XenServer.");
+            return defaultHostToExecuteCommands;
+        }
+        DataStoreTO srcStore = srcData.getDataStore();
+        DataStoreTO destStore = destData.getDataStore();
+        boolean isSourceAndDestinationNfsObjects = srcStore instanceof NfsTO && destStore instanceof NfsTO;
+        if (!isSourceAndDestinationNfsObjects) {
+            logger.debug("We are returning the default host to execute commands because the source and destination objects are not NFS type.");
+            return defaultHostToExecuteCommands;
+        }
+        boolean isSourceObjectSnapshotTypeAndDestinationObjectTemplateType = srcData.getObjectType() == DataObjectType.SNAPSHOT
+                && destData.getObjectType() == DataObjectType.TEMPLATE;
+        if (!isSourceObjectSnapshotTypeAndDestinationObjectTemplateType) {
+            logger.debug("We are returning the default host to execute commands because the source and destination objects are not snapshot and template respectively.");
+            return defaultHostToExecuteCommands;
+        }
+        long snapshotId = srcData.getId();
+        HostVO hostCandidateToExecutedCommand = hostDao.findHostToOperateOnSnapshot(snapshotId);
+        hostDao.loadDetails(hostCandidateToExecutedCommand);
+        String hypervisorVersion = hostCandidateToExecutedCommand.getHypervisorVersion();
+        if (StringUtils.isBlank(hypervisorVersion)) {
+            logger.debug("We are returning the default host to execute commands because the hypervisor version is blank.");
+            return defaultHostToExecuteCommands;
+        }
+        boolean isXenServer610 = StringUtils.equals(hypervisorVersion, "6.1.0");
+        if (isXenServer610) {
+            logger.debug("We are returning the default host to execute commands because the hypervisor version is 6.1.0.");
+            return defaultHostToExecuteCommands;
+        }
+        String snapshotHotFixVersion = hostCandidateToExecutedCommand.getDetail(XenserverConfigs.XS620HotFix);
+        boolean isXenServer620 = StringUtils.equals(hypervisorVersion, "6.2.0");
+        if (isXenServer620 && !StringUtils.equalsIgnoreCase(XenserverConfigs.XSHotFix62ESP1004, snapshotHotFixVersion)) {
+            logger.debug(String.format(
+                    "We are returning the default host to execute commands because the hypervisor version is not 6.2.0 with hotfix ESP1004 [hypervisorVersion=%s, hotfixVersion=%s]",
+                    hypervisorVersion, snapshotHotFixVersion));
+            return defaultHostToExecuteCommands;
         }
-        return new Pair<Boolean, Long>(Boolean.FALSE, new Long(hostId));
+        logger.debug(String.format("We are changing the hostId to executed command from %d to %d.", hostId, hostCandidateToExecutedCommand.getId()));
+        return new Pair<Boolean, Long>(Boolean.TRUE, new Long(hostCandidateToExecutedCommand.getId()));
     }
 
     @Override