CLOUDSTACK-8847: ListServiceOfferings is returning incompatible tagged offerings when called with VM id

Fixed the subset and superset issue.
Added unit test for the same.

Author: Nitin Kumar Maharana

engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java

@@ -2908,12 +2908,12 @@ public void checkIfCanUpgrade(final VirtualMachine vmInstance, final ServiceOffe
                     newServiceOffering.getCpu() + " cpu(s) at " + newServiceOffering.getSpeed() + " Mhz, and " + newServiceOffering.getRamSize() + " MB of memory");
         }
 
-        // Check that the service offering being upgraded to has storage tags subset of the current service offering storage tags, since volume is not migrated.
+        // Check that the service offering being upgraded to has all the tags of the current service offering.
         final List<String> currentTags = StringUtils.csvTagsToList(currentServiceOffering.getTags());
         final List<String> newTags = StringUtils.csvTagsToList(newServiceOffering.getTags());
-        if (!currentTags.containsAll(newTags)) {
-            throw new InvalidParameterValueException("Unable to upgrade virtual machine; the new service offering " + " should have tags as subset of " +
-                    "current service offering tags. Current service offering tags: " + currentTags + "; " + "new service " + "offering tags: " + newTags);
+        if (!newTags.containsAll(currentTags)) {
+            throw new InvalidParameterValueException("Unable to upgrade virtual machine; the current service offering " + " should have tags as subset of " +
+                    "the new service offering tags. Current service offering tags: " + currentTags + "; " + "new service " + "offering tags: " + newTags);
         }
     }
 

engine/orchestration/test/com/cloud/vm/VirtualMachineManagerImplTest.java

@@ -29,7 +29,9 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.ArrayList;
 
+import com.cloud.service.dao.ServiceOfferingDao;
 import junit.framework.Assert;
 
 import org.junit.Before;
@@ -135,6 +137,8 @@ public class VirtualMachineManagerImplTest {
     @Mock
     VMInstanceDao _vmInstanceDao;
     @Mock
+    ServiceOfferingDao _offeringDao;
+    @Mock
     VMTemplateDao _templateDao;
     @Mock
     VolumeDao _volsDao;
@@ -149,6 +153,8 @@ public class VirtualMachineManagerImplTest {
     @Mock
     VMInstanceVO _vmInstance;
     @Mock
+    ServiceOfferingVO _serviceOfferingMock;
+    @Mock
     HostVO _host;
     @Mock
     VMTemplateVO _templateMock;
@@ -227,6 +233,8 @@ public void setup() {
         _vmMgr._uservmDetailsDao = _vmDetailsDao;
         _vmMgr._entityMgr = _entityMgr;
         _vmMgr._configDepot = _configDepot;
+        _vmMgr._offeringDao = _offeringDao;
+        _vmMgr.hostAllocators = new ArrayList<>();
 
         when(_vmMock.getId()).thenReturn(314l);
         when(_vmInstance.getId()).thenReturn(1L);
@@ -505,4 +513,24 @@ public void testSendStopWithNullAnswer() throws Exception {
 
         Assert.assertFalse(actual);
     }
+
+    @Test
+    public void testCheckIfCanUpgrade() throws Exception {
+        when(_vmInstance.getState()).thenReturn(State.Stopped);
+        when(_serviceOfferingMock.isDynamic()).thenReturn(true);
+        when(_vmInstance.getServiceOfferingId()).thenReturn(1l);
+        when(_serviceOfferingMock.getId()).thenReturn(2l);
+
+        ServiceOfferingVO mockCurrentServiceOffering = mock(ServiceOfferingVO.class);
+
+        when(_offeringDao.findByIdIncludingRemoved(anyLong(), anyLong())).thenReturn(mockCurrentServiceOffering);
+        when(mockCurrentServiceOffering.getUseLocalStorage()).thenReturn(true);
+        when(_serviceOfferingMock.getUseLocalStorage()).thenReturn(true);
+        when(mockCurrentServiceOffering.getSystemUse()).thenReturn(true);
+        when(_serviceOfferingMock.getSystemUse()).thenReturn(true);
+        when(mockCurrentServiceOffering.getTags()).thenReturn("x,y");
+        when(_serviceOfferingMock.getTags()).thenReturn("z,x,y");
+
+        _vmMgr.checkIfCanUpgrade(_vmInstance, _serviceOfferingMock);
+    }
 }

server/src/com/cloud/api/query/QueryManagerImpl.java

@@ -2620,11 +2620,11 @@ private List<ServiceOfferingJoinVO> filterOfferingsOnCurrentTags(List<ServiceOff
         if(currentVmOffering == null) return offerings;
         List<String> currentTagsList = StringUtils.csvTagsToList(currentVmOffering.getTags());
 
-        // New offerings should be a subset of existing storage tags. Discard offerings who are not.
+        // New service offering should have all the tags of the current service offering.
         List<ServiceOfferingJoinVO> filteredOfferings = new ArrayList<>();
         for (ServiceOfferingJoinVO offering : offerings){
-            List<String> tags = StringUtils.csvTagsToList(offering.getTags());
-            if(currentTagsList.containsAll(tags)){
+            List<String> newTagsList = StringUtils.csvTagsToList(offering.getTags());
+            if(newTagsList.containsAll(currentTagsList)){
                 filteredOfferings.add(offering);
             }
         }