Skip to content

Commit 2a3598a

Browse files
andrijapanicsbabh1sar
andrijapanicsb
and
abh1sar
committed
Fix importVM with soft-deleted dummy template
Port the production fix from apache/cloudstack PR #14195, source commit 0cb24ba (merged into 4.22 as 273b2ea). Reuse the supplied template and load its details without a findById lookup that filters removed records. Preserve template CPU-detail inheritance and explicit VM overrides. Add six regression tests for dummy and active templates, non-VO templates, CPU override precedence and missing templates. Co-authored-by: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com>
1 parent 6fa7d30 commit 2a3598a

2 files changed

Lines changed: 211 additions & 6 deletions

File tree

‎server/src/main/java/com/cloud/vm/UserVmManagerImpl.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9888,11 +9888,10 @@ public UserVm importVM(final DataCenter zone, final Host host, final VirtualMach
98889888
}
98899889

98909890
// Ensure template details are loaded so that commitUserVm can copy them into the VM's details map
9891-
VMTemplateVO vmTemplateVO = _templateDao.findById(template.getId());
9892-
if (vmTemplateVO == null) {
9893-
throw new InvalidParameterValueException("Unable to find template with id " + template.getId() + " for virtual machine import");
9891+
if (template instanceof VMTemplateVO) {
9892+
VMTemplateVO vmTemplateVO = (VMTemplateVO) template;
9893+
_templateDao.loadDetails(vmTemplateVO);
98949894
}
9895-
_templateDao.loadDetails(vmTemplateVO);
98969895

98979896
final long id = _vmDao.getNextInSequence(Long.class, "id");
98989897
String instanceName = StringUtils.isBlank(instanceNameInternal) ?
@@ -9906,8 +9905,8 @@ public UserVm importVM(final DataCenter zone, final Host host, final VirtualMach
99069905

99079906
final String uuidName = _uuidMgr.generateUuid(UserVm.class, null);
99089907
final Host lastHost = powerState != VirtualMachine.PowerState.PowerOn ? host : null;
9909-
final boolean dynamicScalingEnabled = checkIfDynamicScalingCanBeEnabled(null, serviceOffering, vmTemplateVO, zone.getId());
9910-
return commitUserVm(true, zone, host, lastHost, vmTemplateVO, hostName, displayName, owner,
9908+
final boolean dynamicScalingEnabled = checkIfDynamicScalingCanBeEnabled(null, serviceOffering, template, zone.getId());
9909+
return commitUserVm(true, zone, host, lastHost, template, hostName, displayName, owner,
99119910
null, null, userData, null, null, isDisplayVm, keyboard,
99129911
accountId, userId, serviceOffering, template.getFormat().equals(ImageFormat.ISO), guestOsId, sshPublicKeys, networkNicMap,
99139912
id, instanceName, uuidName, hypervisorType, customParameters,

‎server/src/test/java/com/cloud/vm/UserVmImportTemplateTest.java‎

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package com.cloud.vm;
18+
19+
import static org.junit.Assert.assertEquals;
20+
import static org.junit.Assert.assertNotNull;
21+
import static org.junit.Assert.assertThrows;
22+
import static org.mockito.ArgumentMatchers.any;
23+
import static org.mockito.Mockito.doAnswer;
24+
import static org.mockito.Mockito.mock;
25+
import static org.mockito.Mockito.mockStatic;
26+
import static org.mockito.Mockito.verify;
27+
import static org.mockito.Mockito.verifyNoInteractions;
28+
import static org.mockito.Mockito.verifyNoMoreInteractions;
29+
import static org.mockito.Mockito.when;
30+
31+
import java.util.Collections;
32+
import java.util.Date;
33+
import java.util.HashMap;
34+
import java.util.LinkedHashMap;
35+
import java.util.Map;
36+
37+
import org.apache.cloudstack.context.CallContext;
38+
import org.junit.Before;
39+
import org.junit.Test;
40+
import org.mockito.MockedStatic;
41+
import org.springframework.test.util.ReflectionTestUtils;
42+
43+
import com.cloud.dc.DataCenter;
44+
import com.cloud.exception.InsufficientCapacityException;
45+
import com.cloud.exception.InvalidParameterValueException;
46+
import com.cloud.hypervisor.Hypervisor.HypervisorType;
47+
import com.cloud.offering.ServiceOffering;
48+
import com.cloud.storage.GuestOSCategoryVO;
49+
import com.cloud.storage.GuestOSVO;
50+
import com.cloud.storage.Storage.ImageFormat;
51+
import com.cloud.storage.VMTemplateVO;
52+
import com.cloud.storage.dao.GuestOSCategoryDao;
53+
import com.cloud.storage.dao.GuestOSDao;
54+
import com.cloud.storage.dao.VMTemplateDao;
55+
import com.cloud.template.VirtualMachineTemplate;
56+
import com.cloud.user.Account;
57+
import com.cloud.uservm.UserVm;
58+
import com.cloud.utils.db.Transaction;
59+
import com.cloud.utils.db.TransactionCallbackWithException;
60+
import com.cloud.utils.db.UUIDManager;
61+
import com.cloud.vm.dao.UserVmDao;
62+
63+
public class UserVmImportTemplateTest {
64+
private final UserVmManagerImpl manager = new UserVmManagerImpl();
65+
private final VMTemplateDao templateDao = mock(VMTemplateDao.class);
66+
private final UserVmDao vmDao = mock(UserVmDao.class);
67+
private final DataCenter zone = mock(DataCenter.class);
68+
private final Account owner = mock(Account.class);
69+
private final ServiceOffering offering = mock(ServiceOffering.class);
70+
71+
@Before
72+
public void setUp() {
73+
UUIDManager uuidManager = mock(UUIDManager.class);
74+
GuestOSDao guestOSDao = mock(GuestOSDao.class);
75+
GuestOSCategoryDao categoryDao = mock(GuestOSCategoryDao.class);
76+
GuestOSVO guestOS = mock(GuestOSVO.class);
77+
ReflectionTestUtils.setField(manager, "_templateDao", templateDao);
78+
ReflectionTestUtils.setField(manager, "_vmDao", vmDao);
79+
ReflectionTestUtils.setField(manager, "_uuidMgr", uuidManager);
80+
ReflectionTestUtils.setField(manager, "_itMgr", mock(VirtualMachineManager.class));
81+
ReflectionTestUtils.setField(manager, "_guestOSDao", guestOSDao);
82+
ReflectionTestUtils.setField(manager, "_guestOSCategoryDao", categoryDao);
83+
when(zone.getId()).thenReturn(1L);
84+
when(owner.getId()).thenReturn(10L);
85+
when(owner.getDomainId()).thenReturn(11L);
86+
when(offering.getId()).thenReturn(12L);
87+
when(vmDao.getNextInSequence(Long.class, "id")).thenReturn(42L);
88+
when(uuidManager.generateUuid(UserVm.class, null)).thenReturn("b90b3f01-bbbd-4129-af59-d628c92d935f");
89+
when(guestOSDao.findById(5L)).thenReturn(guestOS);
90+
when(guestOS.getCategoryId()).thenReturn(6L);
91+
when(categoryDao.findById(6L)).thenReturn(mock(GuestOSCategoryVO.class));
92+
}
93+
94+
@Test
95+
public void importsSoftDeletedDummyTemplateWithoutReloadingIt() throws InsufficientCapacityException {
96+
VMTemplateVO template = createTemplate();
97+
template.setRemoved(new Date());
98+
template.setState(VirtualMachineTemplate.State.Inactive);
99+
doAnswer(invocation -> {
100+
template.setDetails(new HashMap<>());
101+
return null;
102+
}).when(templateDao).loadDetails(template);
103+
104+
UserVmVO vm = importTemplate(template);
105+
106+
assertNotNull(vm);
107+
assertEquals(203L, vm.getTemplateId());
108+
assertNotNull(template.getRemoved());
109+
verify(templateDao).loadDetails(template);
110+
verifyNoMoreInteractions(templateDao);
111+
verify(vmDao).persist(vm);
112+
}
113+
114+
@Test
115+
public void copiesLoadedCpuModeFromActiveTemplateIntoImportedVm() throws InsufficientCapacityException {
116+
VMTemplateVO template = createTemplate();
117+
template.setState(VirtualMachineTemplate.State.Active);
118+
doAnswer(invocation -> {
119+
template.setDetails(Collections.singletonMap("guest.cpu.mode", "host-passthrough"));
120+
return null;
121+
}).when(templateDao).loadDetails(template);
122+
123+
UserVmVO vm = importTemplate(template);
124+
125+
assertEquals("host-passthrough", vm.getDetail("guest.cpu.mode"));
126+
verify(templateDao).loadDetails(template);
127+
verifyNoMoreInteractions(templateDao);
128+
verify(vmDao).saveDetails(vm, Collections.emptyList());
129+
}
130+
131+
@Test
132+
public void importsNonVoTemplateUsingItsExistingDetails() throws InsufficientCapacityException {
133+
VirtualMachineTemplate template = mock(VirtualMachineTemplate.class);
134+
when(template.getId()).thenReturn(204L);
135+
when(template.getFormat()).thenReturn(ImageFormat.QCOW2);
136+
when(template.getDetails()).thenReturn(Collections.singletonMap("guest.cpu.mode", "host-model"));
137+
138+
UserVmVO vm = importTemplate(template);
139+
140+
assertEquals(204L, vm.getTemplateId());
141+
assertEquals("host-model", vm.getDetail("guest.cpu.mode"));
142+
verifyNoInteractions(templateDao);
143+
}
144+
145+
@Test
146+
public void explicitVmCpuDetailsOverrideTemplateDefaults() throws InsufficientCapacityException {
147+
VMTemplateVO template = createTemplate();
148+
doAnswer(invocation -> {
149+
template.setDetails(Map.of(VmDetailConstants.GUEST_CPU_MODE, "custom",
150+
VmDetailConstants.GUEST_CPU_MODEL, "Skylake-Client"));
151+
return null;
152+
}).when(templateDao).loadDetails(template);
153+
Map<String, String> vmDetails = Map.of(VmDetailConstants.GUEST_CPU_MODE, "custom",
154+
VmDetailConstants.GUEST_CPU_MODEL, "EPYC");
155+
156+
UserVmVO vm = importTemplate(template, vmDetails);
157+
158+
assertEquals("custom", vm.getDetail(VmDetailConstants.GUEST_CPU_MODE));
159+
assertEquals("EPYC", vm.getDetail(VmDetailConstants.GUEST_CPU_MODEL));
160+
assertEquals("Skylake-Client", template.getDetails().get(VmDetailConstants.GUEST_CPU_MODEL));
161+
}
162+
163+
@Test
164+
public void explicitVmCpuModeOverridesTemplateMode() throws InsufficientCapacityException {
165+
VMTemplateVO template = createTemplate();
166+
doAnswer(invocation -> {
167+
template.setDetails(Collections.singletonMap(VmDetailConstants.GUEST_CPU_MODE, "host-passthrough"));
168+
return null;
169+
}).when(templateDao).loadDetails(template);
170+
171+
UserVmVO vm = importTemplate(template, Collections.singletonMap(VmDetailConstants.GUEST_CPU_MODE, "host-model"));
172+
173+
assertEquals("host-model", vm.getDetail(VmDetailConstants.GUEST_CPU_MODE));
174+
}
175+
176+
@Test
177+
public void stillRejectsMissingTemplate() {
178+
InvalidParameterValueException exception = assertThrows(InvalidParameterValueException.class, () -> importTemplate(null));
179+
180+
assertEquals("Unable to import virtual machine without a template", exception.getMessage());
181+
verifyNoInteractions(templateDao, vmDao);
182+
}
183+
184+
private VMTemplateVO createTemplate() {
185+
return VMTemplateVO.createSystemIso(203L, "import-test-template", "import-test-template", false,
186+
"", true, 64, Account.ACCOUNT_ID_SYSTEM, "", "Import test template", false, 5L);
187+
}
188+
189+
private UserVmVO importTemplate(VirtualMachineTemplate template) throws InsufficientCapacityException {
190+
return importTemplate(template, new HashMap<>());
191+
}
192+
193+
private UserVmVO importTemplate(VirtualMachineTemplate template, Map<String, String> vmDetails) throws InsufficientCapacityException {
194+
try (MockedStatic<Transaction> transaction = mockStatic(Transaction.class);
195+
MockedStatic<CallContext> context = mockStatic(CallContext.class)) {
196+
transaction.when(() -> Transaction.execute(any(TransactionCallbackWithException.class))).thenAnswer(invocation -> {
197+
TransactionCallbackWithException<UserVm, InsufficientCapacityException> callback = invocation.getArgument(0);
198+
return callback.doInTransaction(null);
199+
});
200+
context.when(CallContext::current).thenReturn(mock(CallContext.class));
201+
return (UserVmVO) manager.importVM(zone, null, template, "import-test-vm", "Import test VM", owner,
202+
null, owner, true, null, 10L, 20L, offering, null, 5L, null, HypervisorType.KVM,
203+
vmDetails, VirtualMachine.PowerState.PowerOff, new LinkedHashMap<>());
204+
}
205+
}
206+
}

0 commit comments

Comments
 (0)