PR multi tags in compute offering [#4398] (#4399)

* [#4398] adapt code to handle multi tag string with commas

* [#4398] remove trailing spaces

* [#4398] add multi host tag support for ingest process

* [#4398] add test for multi tag support in offerings

* [#4398]  update multitag support for DeploymentPlanningManagerImpl

encapsulate multi tag check from Ingest Feature, DepolymentPlanningManager into
HostDaoImpl to prevent code duplicates

* [#4398] move logic to HostVO and add tests

* rename test method

* [#4398] Change string method to apaches StringUtils

* [#4398] modify test for multi tag support

* adapt sql for double tags

Co-authored-by: Dirk Klahre <Dirk.Klahre@Itelligence.de>

engine/schema/src/main/java/com/cloud/host/HostVO.java

@@ -40,10 +40,13 @@ import javax.persistence.Transient;
 
 import com.cloud.agent.api.VgpuTypesInfo;
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
+import com.cloud.offering.ServiceOffering;
 import com.cloud.resource.ResourceState;
 import com.cloud.storage.Storage.StoragePoolType;
 import com.cloud.utils.NumbersUtil;
 import com.cloud.utils.db.GenericDao;
+import java.util.Arrays;
+import org.apache.commons.lang.StringUtils;
 
 @Entity
 @Table(name = "host")
@@ -740,6 +743,18 @@ public class HostVO implements Host {
         this.uuid = uuid;
     }
 
+    public boolean checkHostServiceOfferingTags(ServiceOffering serviceOffering){
+        if (serviceOffering == null) {
+            return false;
+        }
+        if (StringUtils.isEmpty(serviceOffering.getHostTag())) {
+            return true;
+        }
+
+        List<String> serviceOfferingTags = Arrays.asList(serviceOffering.getHostTag().split(","));
+        return this.getHostTags() != null && this.getHostTags().containsAll(serviceOfferingTags);
+    }
+
     @Override
     public PartitionType partitionType() {
         return PartitionType.Host;

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

@@ -70,6 +70,7 @@ import com.cloud.utils.db.SearchCriteria.Op;
 import com.cloud.utils.db.TransactionLegacy;
 import com.cloud.utils.db.UpdateBuilder;
 import com.cloud.utils.exception.CloudRuntimeException;
+import java.util.Arrays;
 
 @DB
 @TableGenerator(name = "host_req_sq", table = "op_host", pkColumnName = "id", valueColumnName = "sequence", allocationSize = 1)
@@ -78,12 +79,19 @@ public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao
     private static final Logger status_logger = Logger.getLogger(Status.class);
     private static final Logger state_logger = Logger.getLogger(ResourceState.class);
 
+    private static final String LIST_HOST_IDS_BY_COMPUTETAGS = "SELECT filtered.host_id, COUNT(filtered.tag) AS tag_count "
+                                                             + "FROM (SELECT host_id, tag FROM host_tags GROUP BY host_id,tag) AS filtered "
+                                                             + "WHERE tag IN(%s) "
+                                                             + "GROUP BY host_id "
+                                                             + "HAVING tag_count = %s ";
+    private static final String SEPARATOR = ",";
     private static final String LIST_CLUSTERID_FOR_HOST_TAG = "select distinct cluster_id from host join host_tags on host.id = host_tags.host_id and host_tags.tag = ?";
     private static final String GET_HOSTS_OF_ACTIVE_VMS = "select h.id " +
             "from vm_instance vm " +
             "join host h on (vm.host_id=h.id) " +
             "where vm.service_offering_id= ? and vm.state not in (\"Destroyed\", \"Expunging\", \"Error\") group by h.id";
 
+
     protected SearchBuilder<HostVO> TypePodDcStatusSearch;
 
     protected SearchBuilder<HostVO> IdStatusSearch;
@@ -736,11 +744,6 @@ public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao
 
     @Override
     public List<HostVO> listByHostTag(Host.Type type, Long clusterId, Long podId, long dcId, String hostTag) {
-
-        SearchBuilder<HostTagVO> hostTagSearch = _hostTagsDao.createSearchBuilder();
-        HostTagVO tagEntity = hostTagSearch.entity();
-        hostTagSearch.and("tag", tagEntity.getTag(), SearchCriteria.Op.EQ);
-
         SearchBuilder<HostVO> hostSearch = createSearchBuilder();
         HostVO entity = hostSearch.entity();
         hostSearch.and("type", entity.getType(), SearchCriteria.Op.EQ);
@@ -749,10 +752,8 @@ public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao
         hostSearch.and("cluster", entity.getClusterId(), SearchCriteria.Op.EQ);
         hostSearch.and("status", entity.getStatus(), SearchCriteria.Op.EQ);
         hostSearch.and("resourceState", entity.getResourceState(), SearchCriteria.Op.EQ);
-        hostSearch.join("hostTagSearch", hostTagSearch, entity.getId(), tagEntity.getHostId(), JoinBuilder.JoinType.INNER);
 
         SearchCriteria<HostVO> sc = hostSearch.create();
-        sc.setJoinParameters("hostTagSearch", "tag", hostTag);
         sc.setParameters("type", type.toString());
         if (podId != null) {
             sc.setParameters("pod", podId);
@@ -764,7 +765,13 @@ public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao
         sc.setParameters("status", Status.Up.toString());
         sc.setParameters("resourceState", ResourceState.Enabled.toString());
 
-        return listBy(sc);
+        List<HostVO> tmpHosts = listBy(sc);
+        List<HostVO> correctHostsByHostTags = new ArrayList();
+        List<Long> hostIdsByComputeOffTags = findHostByComputeOfferings(hostTag);
+
+        tmpHosts.forEach((host) -> { if(hostIdsByComputeOffTags.contains(host.getId())) correctHostsByHostTags.add(host);});
+
+        return correctHostsByHostTags;
     }
 
     @Override
@@ -1179,28 +1186,69 @@ public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao
     }
 
     @Override
-    public List<Long> listClustersByHostTag(String hostTagOnOffering) {
+    public List<Long> listClustersByHostTag(String computeOfferingTags) {
         TransactionLegacy txn = TransactionLegacy.currentTxn();
+        String sql = this.LIST_CLUSTERID_FOR_HOST_TAG;
         PreparedStatement pstmt = null;
-        List<Long> result = new ArrayList<Long>();
-        StringBuilder sql = new StringBuilder(LIST_CLUSTERID_FOR_HOST_TAG);
-        // during listing the clusters that cross the threshold
-        // we need to check with disabled thresholds of each cluster if not defined at cluster consider the global value
+        List<Long> result = new ArrayList();
+        List<String> tags = Arrays.asList(computeOfferingTags.split(this.SEPARATOR));
+        String subselect = getHostIdsByComputeTags(tags);
+        sql = String.format(sql, subselect);
+
         try {
-            pstmt = txn.prepareAutoCloseStatement(sql.toString());
-            pstmt.setString(1, hostTagOnOffering);
+            pstmt = txn.prepareStatement(sql);
+
+            for(int i = 0; i < tags.size(); i++){
+                pstmt.setString(i+1, tags.get(i));
+            }
+
             ResultSet rs = pstmt.executeQuery();
             while (rs.next()) {
                 result.add(rs.getLong(1));
             }
+            pstmt.close();
+            if(result.isEmpty()){
+                throw new CloudRuntimeException("No suitable host found for follow compute offering tags: " + computeOfferingTags);
+            }
             return result;
         } catch (SQLException e) {
             throw new CloudRuntimeException("DB Exception on: " + sql, e);
-        } catch (Throwable e) {
-            throw new CloudRuntimeException("Caught: " + sql, e);
         }
     }
 
+    private List<Long> findHostByComputeOfferings(String computeOfferingTags){
+        TransactionLegacy txn = TransactionLegacy.currentTxn();
+        PreparedStatement pstmt = null;
+        List<Long> result = new ArrayList();
+        List<String> tags = Arrays.asList(computeOfferingTags.split(this.SEPARATOR));
+        String select = getHostIdsByComputeTags(tags);
+        try {
+            pstmt = txn.prepareStatement(select);
+
+            for(int i = 0; i < tags.size(); i++){
+                pstmt.setString(i+1, tags.get(i));
+            }
+
+            ResultSet rs = pstmt.executeQuery();
+            while (rs.next()) {
+                result.add(rs.getLong(1));
+            }
+            pstmt.close();
+            if(result.isEmpty()){
+                throw new CloudRuntimeException("No suitable host found for follow compute offering tags: " + computeOfferingTags);
+            }
+            return result;
+        } catch (SQLException e) {
+            throw new CloudRuntimeException("DB Exception on: " + select, e);
+        }
+    }
+
+    private String getHostIdsByComputeTags(List<String> offeringTags){
+        List<String> questionMarks = new ArrayList();
+        offeringTags.forEach((tag) -> { questionMarks.add("?"); });
+        return String.format(this.LIST_HOST_IDS_BY_COMPUTETAGS, String.join(",", questionMarks),questionMarks.size());
+    }
+
     @Override
     public List<HostVO> listHostsWithActiveVMs(long offeringId) {
         TransactionLegacy txn = TransactionLegacy.currentTxn();

engine/schema/src/test/java/com/cloud/host/HostVOTest.java

@@ -0,0 +1,61 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you 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.host;
+
+import com.cloud.service.ServiceOfferingVO;
+import com.cloud.storage.Storage.ProvisioningType;
+import com.cloud.vm.VirtualMachine;
+import java.util.Arrays;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import org.junit.Test;
+import org.junit.Before;
+
+public class HostVOTest {
+    HostVO host;
+    ServiceOfferingVO offering;
+
+    @Before
+    public void setUp() throws Exception {
+        host = new HostVO();
+        offering = new ServiceOfferingVO("TestSO", 0, 0, 0, 0, 0, false, "TestSO", ProvisioningType.THIN, true, true,"",false,VirtualMachine.Type.User,false);
+    }
+
+    @Test
+    public void testNoSO() {
+        assertFalse(host.checkHostServiceOfferingTags(null));
+    }
+
+    @Test
+    public void testNoTag() {
+        assertTrue(host.checkHostServiceOfferingTags(offering));
+    }
+
+    @Test
+    public void testRightTag() {
+        host.setHostTags(Arrays.asList("tag1","tag2"));
+        offering.setHostTag("tag2,tag1");
+        assertTrue(host.checkHostServiceOfferingTags(offering));
+    }
+
+    @Test
+    public void testWrongTag() {
+        host.setHostTags(Arrays.asList("tag1","tag2"));
+        offering.setHostTag("tag2,tag4");
+        assertFalse(host.checkHostServiceOfferingTags(offering));
+    }
+}

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java

@@ -678,7 +678,7 @@ StateListener<State, VirtualMachine.Event, VirtualMachine>, Configurable {
         ServiceOffering offering = vmProfile.getServiceOffering();
         if (offering.getHostTag() != null) {
             _hostDao.loadHostTags(host);
-            if (!(host.getHostTags() != null && host.getHostTags().contains(offering.getHostTag()))) {
+            if (!host.checkHostServiceOfferingTags(offering)) {
                 s_logger.debug("Service Offering host tag does not match the last host of this VM");
                 return false;
             }

server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java

@@ -379,17 +379,8 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager {
     }
 
     private boolean hostSupportsServiceOffering(HostVO host, ServiceOffering serviceOffering) {
-        if (host == null) {
-            return false;
-        }
-        if (serviceOffering == null) {
-            return false;
-        }
-        if (Strings.isNullOrEmpty(serviceOffering.getHostTag())) {
-            return true;
-        }
         hostDao.loadHostTags(host);
-        return host.getHostTags() != null && host.getHostTags().contains(serviceOffering.getHostTag());
+        return host.checkHostServiceOfferingTags(serviceOffering);
     }
 
     private boolean storagePoolSupportsDiskOffering(StoragePool pool, DiskOffering diskOffering) {

server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java

@@ -82,6 +82,7 @@ import com.cloud.exception.PermissionDeniedException;
 import com.cloud.host.Host;
 import com.cloud.host.HostVO;
 import com.cloud.host.Status;
+import com.cloud.host.dao.HostDao;
 import com.cloud.hypervisor.Hypervisor;
 import com.cloud.network.Network;
 import com.cloud.network.NetworkModel;
@@ -180,6 +181,8 @@ public class UnmanagedVMsManagerImplTest {
     private UserVmDao userVmDao;
     @Mock
     private NicDao nicDao;
+    @Mock
+    private HostDao hostDao;
 
     @Mock
     private VMInstanceVO virtualMachine;
@@ -235,6 +238,7 @@ public class UnmanagedVMsManagerImplTest {
         HostVO hostVO = Mockito.mock(HostVO.class);
         when(hostVO.isInMaintenanceStates()).thenReturn(false);
         hosts.add(hostVO);
+        when(hostVO.checkHostServiceOfferingTags(Mockito.any())).thenReturn(true);
         when(resourceManager.listHostsInClusterByStatus(Mockito.anyLong(), Mockito.any(Status.class))).thenReturn(hosts);
         List<VMTemplateStoragePoolVO> templates = new ArrayList<>();
         when(templatePoolDao.listAll()).thenReturn(templates);
@@ -368,6 +372,7 @@ public class UnmanagedVMsManagerImplTest {
         when(importUnmanageInstanceCmd.getName()).thenReturn("TestInstance");
         when(importUnmanageInstanceCmd.getAccountName()).thenReturn(null);
         when(importUnmanageInstanceCmd.getDomainId()).thenReturn(null);
+        doNothing().when(hostDao).loadHostTags(null);
         PowerMockito.mockStatic(UsageEventUtils.class);
         unmanagedVMsManager.importUnmanagedInstance(importUnmanageInstanceCmd);
     }

test/integration/component/test_multi_tag_support.py

@@ -0,0 +1,217 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you 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.
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.base import *
+from marvin.lib.common import get_zone, get_domain, get_template
+from marvin.lib.utils import cleanup_resources
+
+
+class Services:
+    """Test multi tag support in compute offerings
+    """
+
+    def __init__(self):
+        self.services = {
+            "account": {
+                "email": "john@doe.de",
+                "firstname": "Test",
+                "lastname": "User",
+                "username": "test",
+                # Random characters are appended for unique
+                # username
+                "password": "password",
+            },
+            "service_offering_0": {
+                "name": "NoTag",
+                "displaytext": "NoTag",
+                "cpunumber": 1,
+                "cpuspeed": 100,
+                # in MHz
+                "memory": 128,
+                # In MBs
+            },
+            "service_offering_1": {
+                "name": "OneTag",
+                "displaytext": "OneTag",
+                "cpunumber": 1,
+                "cpuspeed": 100,
+                "hosttags": "tag1",
+                # in MHz
+                "memory": 128,
+                # In MBs
+            },
+            "service_offering_2": {
+                "name": "TwoTag",
+                "displaytext": "TwoTag",
+                "cpunumber": 1,
+                "cpuspeed": 100,
+                "hosttags": "tag2,tag1",
+                # in MHz
+                "memory": 128,
+                # In MBs
+            },
+            "service_offering_not_existing_tag": {
+                "name": "NotExistingTag",
+                "displaytext": "NotExistingTag",
+                "cpunumber": 1,
+                "cpuspeed": 100,
+                "hosttags": "tagX",
+                # in MHz
+                "memory": 128,
+                # In MBs
+            },
+            "virtual_machine": {
+                "name": "TestVM",
+                "displayname": "TestVM"
+            },
+            "template": {
+                "displaytext": "Ubuntu",
+                "name": "Ubuntu16 x64",
+                "ostype": 'Ubuntu 16.04 (64-bit)',
+                "templatefilter": 'self',
+            },
+            "network": {
+                "name": "Guest",
+            },
+            "host": {
+                "name": ""
+            }
+        }
+
+
+class TestMultiTagSupport(cloudstackTestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        cls.testClient = super(TestMultiTagSupport, cls).getClsTestClient()
+        cls.hypervisor = cls.testClient.getHypervisorInfo()
+        cls.api_client = cls.testClient.getApiClient()
+        cls.services = Services().services
+        cls.zone = get_zone(cls.api_client)
+        cls.domain = get_domain(cls.api_client)
+        cls.network = cls.get_network()
+        cls.host = cls.get_host()
+        cls.host_tags = cls.host["hosttags"]
+
+        cls.template = get_template(
+            cls.api_client,
+            cls.zone.id,
+            cls.services["template"]["ostype"]
+        )
+
+        cls.service_offering_list = []
+        for x in range(0, 3):
+            cls.service_offering_list.append(ServiceOffering.create(
+                cls.api_client,
+                cls.services["service_offering_" + str(x)]
+            ))
+
+        cls.service_offering_ne_tag = ServiceOffering.create(
+            cls.api_client,
+            cls.services["service_offering_not_existing_tag"]
+        )
+
+        cls.account = Account.create(
+            cls.api_client,
+            cls.services["account"],
+            admin=True,
+        )
+
+        cls._cleanup = [
+                           cls.account,
+                           cls.service_offering_ne_tag,
+                       ] + cls.service_offering_list
+
+    @classmethod
+    def tearDownClass(cls):
+        try:
+            # Cleanup resources used
+            print("Cleanup resources used")
+            Host.update(cls.api_client, id=cls.host["id"], hosttags=cls.host_tags)
+            cleanup_resources(cls.api_client, cls._cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    def test_multi_tags(self):
+        for x in range(0, len(self.service_offering_list)):
+
+            if hasattr(self.service_offering_list[x], 'hosttags'):
+                Host.update(self.api_client, id=self.host["id"], hosttags=self.service_offering_list[x].hosttags)
+
+            vm = VirtualMachine.create(
+                self.api_client,
+                self.services["virtual_machine"],
+                accountid=self.account.name,
+                domainid=self.account.domainid,
+                zoneid=self.zone.id,
+                serviceofferingid=self.service_offering_list[x].id,
+                templateid=self.template.id,
+                networkids=self.network.id
+            )
+
+            self.assertEqual(
+                isinstance(vm, VirtualMachine),
+                True,
+                "VM %s should be created with service offering %s " %
+                (self.services["virtual_machine"]["name"], self.service_offering_list[x].name))
+
+            vm.delete(self.api_client, True)
+
+    def test_no_existing_tag(self):
+
+        with self.assertRaises(Exception):
+            VirtualMachine.create(
+                self.api_client,
+                self.services["virtual_machine"],
+                accountid=self.account.name,
+                domainid=self.account.domainid,
+                zoneid=self.zone.id,
+                serviceofferingid=self.service_offering_ne_tag.id,
+                templateid=self.template.id,
+                networkids=self.network.id
+            )
+            return
+
+    @classmethod
+    def get_network(cls):
+        networks = Network.list(cls.api_client, zoneid=cls.zone.id)
+        if len(networks) == 1:
+            return networks[0]
+
+        if len(networks) > 1:
+            for network in networks:
+                if network.name == cls.services["network"]["name"]:
+                    return network
+            raise ValueError("No suitable network found, check network name in service object")
+
+        raise ValueError("No network found for zone with id %s" % cls.zone.id)
+
+    @classmethod
+    def get_host(cls):
+        hosts = Host.list(cls.api_client, zoneid=cls.zone.id)
+
+        if len(hosts) == 1:
+            return hosts[0]
+
+        if len(hosts) > 1:
+            for host in hosts:
+                if host.name == cls.services["host"]["name"]:
+                    return host
+            raise ValueError("No suitable host found, check host name in service object")
+
+        raise ValueError("No host found for zone with id %s" % cls.zone.id)