winterhazel commented on code in PR #9074: URL: https://github.com/apache/cloudstack/pull/9074#discussion_r1956993175
########## server/src/main/java/com/cloud/agent/manager/allocator/impl/BaseAllocator.java: ########## @@ -0,0 +1,90 @@ +// 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.agent.manager.allocator.impl; + +import com.cloud.agent.manager.allocator.HostAllocator; +import com.cloud.capacity.CapacityManager; +import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; +import com.cloud.offering.ServiceOffering; +import com.cloud.utils.Pair; +import com.cloud.utils.component.AdapterBase; +import org.apache.commons.collections.CollectionUtils; + +import javax.inject.Inject; +import java.util.List; + +public abstract class BaseAllocator extends AdapterBase implements HostAllocator { + + @Inject + protected HostDao hostDao; + + @Inject + protected CapacityManager capacityManager; + + protected void retainHostsMatchingServiceOfferingAndTemplateTags(List<HostVO> availableHosts, Host.Type type, long dcId, Long podId, Long clusterId, String offeringHostTag, String templateTag) { + logger.debug("Hosts {} will be checked for template and host tags compatibility.", availableHosts); + + if (offeringHostTag != null) { + logger.debug("Looking for hosts having the tag [{}] specified in the Service Offering.", offeringHostTag); + List<HostVO> hostsWithHostTag = hostDao.listByHostTag(type, clusterId, podId, dcId, offeringHostTag); + logger.debug("Retaining hosts {} because they match the offering host tag {}.", hostsWithHostTag, offeringHostTag); + availableHosts.retainAll(hostsWithHostTag); + } + + if (templateTag != null) { + logger.debug("Looking for hosts having the tag [{}] specified in the Template.", templateTag); + List<HostVO> hostsWithTemplateTag = hostDao.listByHostTag(type, clusterId, podId, dcId, templateTag); + logger.debug("Retaining hosts {} because they match the template tag {}.", hostsWithTemplateTag, templateTag); + availableHosts.retainAll(hostsWithTemplateTag); + } + + logger.debug("Remaining hosts after template tag and host tags validations are {}.", availableHosts); + } + + protected void addHostsBasedOnTagRules(String hostTagOnOffering, List<HostVO> clusterHosts) { + List<HostVO> hostsWithTagRules = hostDao.findHostsWithTagRuleThatMatchComputeOfferingTags(hostTagOnOffering); + + if (CollectionUtils.isEmpty(hostsWithTagRules)) { + logger.info("No hosts found with tag rules matching the compute offering tag [{}].", hostTagOnOffering); + return; + } + + logger.info("Found hosts %s with tag rules matching the compute offering tag [{}].", hostsWithTagRules, hostTagOnOffering); Review Comment: ```suggestion logger.info("Found hosts {} with tag rules matching the compute offering tag [{}].", hostsWithTagRules, hostTagOnOffering); ``` ########## server/src/test/java/com/cloud/agent/manager/allocator/impl/RandomAllocatorTest.java: ########## @@ -0,0 +1,332 @@ +// 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.agent.manager.allocator.impl; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import com.cloud.agent.manager.allocator.HostAllocator; +import com.cloud.deploy.DeploymentPlan; +import com.cloud.deploy.DeploymentPlanner; +import com.cloud.offering.ServiceOffering; +import com.cloud.resource.ResourceManager; +import com.cloud.service.ServiceOfferingVO; +import com.cloud.storage.VMTemplateVO; +import com.cloud.vm.VirtualMachineProfile; +import org.apache.commons.collections.CollectionUtils; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; + +@RunWith(MockitoJUnitRunner.class) +public class RandomAllocatorTest { + + @Mock + HostDao hostDao; + + @Spy + @InjectMocks + RandomAllocator randomAllocator; + + @Mock + ResourceManager resourceManagerMock; + + private final Host.Type type = Host.Type.Routing; + + private final Long clusterId = 1L; + + private final Long podId = 1L; + + private final Long zoneId = 1L; + + private final List<HostVO> emptyList = new ArrayList<>(); + + private final String hostTag = "hostTag"; + + private final HostVO host1 = Mockito.mock(HostVO.class); + + private final HostVO host2 = Mockito.mock(HostVO.class); + + private final HostVO host3 = Mockito.mock(HostVO.class); + + private final VMTemplateVO vmTemplateVO = Mockito.mock(VMTemplateVO.class); + + private final ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + + private final DeploymentPlanner.ExcludeList excludeList = Mockito.mock(DeploymentPlanner.ExcludeList.class); + + private final VirtualMachineProfile virtualMachineProfile = Mockito.mock(VirtualMachineProfile.class); + + private final DeploymentPlan deploymentPlan = Mockito.mock(DeploymentPlan.class); + + private final boolean considerReservedCapacity = true; + + + @Test + public void testListHostsByTags() { + Host.Type type = Host.Type.Routing; + Long id = 1L; + String templateTag = "tag1"; + String offeringTag = "tag2"; + HostVO host1 = Mockito.mock(HostVO.class); + HostVO host2 = Mockito.mock(HostVO.class); + Mockito.when(hostDao.listByHostTag(type, id, id, id, offeringTag)).thenReturn(List.of(host1, host2)); + + // No template tagged host + ArrayList<HostVO> noTemplateTaggedHosts = new ArrayList<>(Arrays.asList(host1, host2)); + Mockito.when(hostDao.listByHostTag(type, id, id, id, templateTag)).thenReturn(new ArrayList<>()); Review Comment: ```suggestion Mockito.when(hostDao.listByHostTag(type, clusterId, podId, zoneId, templateTag)).thenReturn(new ArrayList<>()); ``` Also need to adjust for some other calls in this file ########## server/src/test/java/com/cloud/agent/manager/allocator/impl/RandomAllocatorTest.java: ########## @@ -0,0 +1,332 @@ +// 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.agent.manager.allocator.impl; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import com.cloud.agent.manager.allocator.HostAllocator; +import com.cloud.deploy.DeploymentPlan; +import com.cloud.deploy.DeploymentPlanner; +import com.cloud.offering.ServiceOffering; +import com.cloud.resource.ResourceManager; +import com.cloud.service.ServiceOfferingVO; +import com.cloud.storage.VMTemplateVO; +import com.cloud.vm.VirtualMachineProfile; +import org.apache.commons.collections.CollectionUtils; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; + +@RunWith(MockitoJUnitRunner.class) +public class RandomAllocatorTest { + + @Mock + HostDao hostDao; + + @Spy + @InjectMocks + RandomAllocator randomAllocator; + + @Mock + ResourceManager resourceManagerMock; + + private final Host.Type type = Host.Type.Routing; + + private final Long clusterId = 1L; + + private final Long podId = 1L; + + private final Long zoneId = 1L; Review Comment: ```suggestion private final Long clusterId = 1L; private final Long podId = 2L; private final Long zoneId = 3L; ``` ########## server/src/test/java/com/cloud/agent/manager/allocator/impl/RandomAllocatorTest.java: ########## @@ -0,0 +1,332 @@ +// 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.agent.manager.allocator.impl; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import com.cloud.agent.manager.allocator.HostAllocator; +import com.cloud.deploy.DeploymentPlan; +import com.cloud.deploy.DeploymentPlanner; +import com.cloud.offering.ServiceOffering; +import com.cloud.resource.ResourceManager; +import com.cloud.service.ServiceOfferingVO; +import com.cloud.storage.VMTemplateVO; +import com.cloud.vm.VirtualMachineProfile; +import org.apache.commons.collections.CollectionUtils; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; + +@RunWith(MockitoJUnitRunner.class) +public class RandomAllocatorTest { + + @Mock + HostDao hostDao; + + @Spy + @InjectMocks + RandomAllocator randomAllocator; + + @Mock + ResourceManager resourceManagerMock; + + private final Host.Type type = Host.Type.Routing; + + private final Long clusterId = 1L; + + private final Long podId = 1L; + + private final Long zoneId = 1L; + + private final List<HostVO> emptyList = new ArrayList<>(); + + private final String hostTag = "hostTag"; + + private final HostVO host1 = Mockito.mock(HostVO.class); + + private final HostVO host2 = Mockito.mock(HostVO.class); + + private final HostVO host3 = Mockito.mock(HostVO.class); + + private final VMTemplateVO vmTemplateVO = Mockito.mock(VMTemplateVO.class); + + private final ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + + private final DeploymentPlanner.ExcludeList excludeList = Mockito.mock(DeploymentPlanner.ExcludeList.class); + + private final VirtualMachineProfile virtualMachineProfile = Mockito.mock(VirtualMachineProfile.class); + + private final DeploymentPlan deploymentPlan = Mockito.mock(DeploymentPlan.class); + + private final boolean considerReservedCapacity = true; + + + @Test + public void testListHostsByTags() { + Host.Type type = Host.Type.Routing; + Long id = 1L; + String templateTag = "tag1"; + String offeringTag = "tag2"; + HostVO host1 = Mockito.mock(HostVO.class); + HostVO host2 = Mockito.mock(HostVO.class); + Mockito.when(hostDao.listByHostTag(type, id, id, id, offeringTag)).thenReturn(List.of(host1, host2)); + + // No template tagged host + ArrayList<HostVO> noTemplateTaggedHosts = new ArrayList<>(Arrays.asList(host1, host2)); + Mockito.when(hostDao.listByHostTag(type, id, id, id, templateTag)).thenReturn(new ArrayList<>()); + randomAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(noTemplateTaggedHosts, type, id, id, id, offeringTag, templateTag); Review Comment: ```suggestion randomAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(noTemplateTaggedHosts, type, zoneId, podId, clusterId, offeringTag, templateTag); ``` Also need to adjust for some other calls in this file -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
