Skip to content

Commit 979767b

Browse files
shwstpprdhslove
shwstppr
authored and
dhslove
committed
api,server: allow cleaning up vm extraconfig (apache#11974)
1 parent 3973644 commit 979767b

7 files changed

Lines changed: 210 additions & 14 deletions

File tree

‎api/src/main/java/org/apache/cloudstack/api/ApiConstants.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1176,6 +1176,7 @@ public class ApiConstants {
11761176
public static final String OVM3_VIP = "ovm3vip";
11771177
public static final String CLEAN_UP_DETAILS = "cleanupdetails";
11781178
public static final String CLEAN_UP_EXTERNAL_DETAILS = "cleanupexternaldetails";
1179+
public static final String CLEAN_UP_EXTRA_CONFIG = "cleanupextraconfig";
11791180
public static final String CLEAN_UP_PARAMETERS = "cleanupparameters";
11801181
public static final String VIRTUAL_SIZE = "virtualsize";
11811182
public static final String NETSCALER_CONTROLCENTER_ID = "netscalercontrolcenterid";

‎api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,12 @@ public class UpdateVMCmd extends BaseCustomIdCmd implements SecurityGroupAction,
169169
description = "Lease expiry action, valid values are STOP and DESTROY")
170170
private String leaseExpiryAction;
171171

172+
@Parameter(name = ApiConstants.CLEAN_UP_EXTRA_CONFIG, type = CommandType.BOOLEAN, since = "4.23.0",
173+
description = "Optional boolean field, which indicates if extraconfig for the instance should be " +
174+
"cleaned up or not (If set to true, extraconfig removed for this instance, extraconfig field " +
175+
"ignored; if false or not set, no action)")
176+
private Boolean cleanupExtraConfig;
177+
172178
/////////////////////////////////////////////////////
173179
/////////////////// Accessors ///////////////////////
174180
/////////////////////////////////////////////////////
@@ -280,14 +286,18 @@ public String getExtraConfig() {
280286
return extraConfig;
281287
}
282288

283-
/////////////////////////////////////////////////////
284-
/////////////// API Implementation///////////////////
285-
/////////////////////////////////////////////////////
286-
287289
public Long getOsTypeId() {
288290
return osTypeId;
289291
}
290292

293+
public boolean isCleanupExtraConfig() {
294+
return Boolean.TRUE.equals(cleanupExtraConfig);
295+
}
296+
297+
/////////////////////////////////////////////////////
298+
/////////////// API Implementation///////////////////
299+
/////////////////////////////////////////////////////
300+
291301
@Override
292302
public String getCommandName() {
293303
return s_name;

‎engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDetailsDao.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,5 @@
2222
import com.cloud.vm.VMInstanceDetailVO;
2323

2424
public interface VMInstanceDetailsDao extends GenericDao<VMInstanceDetailVO, Long>, ResourceDetailsDao<VMInstanceDetailVO> {
25+
int removeDetailsWithPrefix(long vmId, String prefix);
2526
}

‎engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDetailsDaoImpl.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,13 @@
1717
package com.cloud.vm.dao;
1818

1919

20+
import org.apache.commons.lang3.StringUtils;
2021
import org.springframework.stereotype.Component;
2122

2223
import org.apache.cloudstack.resourcedetail.ResourceDetailsDaoBase;
2324

25+
import com.cloud.utils.db.SearchBuilder;
26+
import com.cloud.utils.db.SearchCriteria;
2427
import com.cloud.vm.VMInstanceDetailVO;
2528

2629
@Component
@@ -31,4 +34,18 @@ public void addDetail(long resourceId, String key, String value, boolean display
3134
super.addDetail(new VMInstanceDetailVO(resourceId, key, value, display));
3235
}
3336

37+
@Override
38+
public int removeDetailsWithPrefix(long vmId, String prefix) {
39+
if (StringUtils.isBlank(prefix)) {
40+
return 0;
41+
}
42+
SearchBuilder<VMInstanceDetailVO> sb = createSearchBuilder();
43+
sb.and("vmId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
44+
sb.and("prefix", sb.entity().getName(), SearchCriteria.Op.LIKE);
45+
sb.done();
46+
SearchCriteria<VMInstanceDetailVO> sc = sb.create();
47+
sc.setParameters("vmId", vmId);
48+
sc.setParameters("prefix", prefix + "%");
49+
return super.remove(sc);
50+
}
3451
}

‎engine/schema/src/test/java/com/cloud/vm/dao/VMInstanceDetailsDaoImplTest.java‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
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+
18+
package com.cloud.vm.dao;
19+
20+
import static org.mockito.ArgumentMatchers.any;
21+
import static org.mockito.ArgumentMatchers.anyString;
22+
import static org.mockito.Mockito.doReturn;
23+
import static org.mockito.Mockito.mock;
24+
import static org.mockito.Mockito.when;
25+
26+
import org.junit.Assert;
27+
import org.junit.Test;
28+
import org.junit.runner.RunWith;
29+
import org.mockito.InjectMocks;
30+
import org.mockito.Mockito;
31+
import org.mockito.Spy;
32+
import org.mockito.junit.MockitoJUnitRunner;
33+
34+
import com.cloud.utils.db.SearchBuilder;
35+
import com.cloud.utils.db.SearchCriteria;
36+
import com.cloud.vm.VMInstanceDetailVO;
37+
38+
@RunWith(MockitoJUnitRunner.class)
39+
public class VMInstanceDetailsDaoImplTest {
40+
@Spy
41+
@InjectMocks
42+
private VMInstanceDetailsDaoImpl vmInstanceDetailsDaoImpl;
43+
44+
@Test
45+
public void removeDetailsWithPrefixReturnsZeroWhenPrefixIsBlank() {
46+
Assert.assertEquals(0, vmInstanceDetailsDaoImpl.removeDetailsWithPrefix(1L, ""));
47+
Assert.assertEquals(0, vmInstanceDetailsDaoImpl.removeDetailsWithPrefix(1L, " "));
48+
Assert.assertEquals(0, vmInstanceDetailsDaoImpl.removeDetailsWithPrefix(1L, null));
49+
}
50+
51+
@Test
52+
public void removeDetailsWithPrefixRemovesMatchingDetails() {
53+
SearchBuilder<VMInstanceDetailVO> sb = mock(SearchBuilder.class);
54+
VMInstanceDetailVO entity = mock(VMInstanceDetailVO.class);
55+
when(sb.entity()).thenReturn(entity);
56+
when(sb.and(anyString(), any(), any(SearchCriteria.Op.class))).thenReturn(sb);
57+
SearchCriteria<VMInstanceDetailVO> sc = mock(SearchCriteria.class);
58+
when(sb.create()).thenReturn(sc);
59+
when(vmInstanceDetailsDaoImpl.createSearchBuilder()).thenReturn(sb);
60+
doReturn(3).when(vmInstanceDetailsDaoImpl).remove(sc);
61+
int removedCount = vmInstanceDetailsDaoImpl.removeDetailsWithPrefix(1L, "testPrefix");
62+
Assert.assertEquals(3, removedCount);
63+
Mockito.verify(sc).setParameters("vmId", 1L);
64+
Mockito.verify(sc).setParameters("prefix", "testPrefix%");
65+
Mockito.verify(vmInstanceDetailsDaoImpl, Mockito.times(1)).remove(sc);
66+
}
67+
68+
@Test
69+
public void removeDetailsWithPrefixDoesNotRemoveWhenNoMatch() {
70+
SearchBuilder<VMInstanceDetailVO> sb = mock(SearchBuilder.class);
71+
VMInstanceDetailVO entity = mock(VMInstanceDetailVO.class);
72+
when(sb.entity()).thenReturn(entity);
73+
when(sb.and(anyString(), any(), any(SearchCriteria.Op.class))).thenReturn(sb);
74+
SearchCriteria<VMInstanceDetailVO> sc = mock(SearchCriteria.class);
75+
when(sb.create()).thenReturn(sc);
76+
when(vmInstanceDetailsDaoImpl.createSearchBuilder()).thenReturn(sb);
77+
doReturn(0).when(vmInstanceDetailsDaoImpl).remove(sc);
78+
79+
int removedCount = vmInstanceDetailsDaoImpl.removeDetailsWithPrefix(1L, "nonExistentPrefix");
80+
81+
Assert.assertEquals(0, removedCount);
82+
Mockito.verify(sc).setParameters("vmId", 1L);
83+
Mockito.verify(sc).setParameters("prefix", "nonExistentPrefix%");
84+
Mockito.verify(vmInstanceDetailsDaoImpl, Mockito.times(1)).remove(sc);
85+
}
86+
}

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

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3015,6 +3015,22 @@ private void adjustVmLimits(Account owner, UserVmVO vmInstance, ServiceOfferingV
30153015
}
30163016
}
30173017

3018+
protected void updateVmExtraConfig(UserVmVO userVm, String extraConfig, boolean cleanupExtraConfig) {
3019+
if (cleanupExtraConfig) {
3020+
logger.info("Cleaning up extraconfig from user vm: {}", userVm.getUuid());
3021+
vmInstanceDetailsDao.removeDetailsWithPrefix(userVm.getId(), ApiConstants.EXTRA_CONFIG);
3022+
return;
3023+
}
3024+
if (StringUtils.isNotBlank(extraConfig)) {
3025+
if (EnableAdditionalVmConfig.valueIn(userVm.getAccountId())) {
3026+
logger.info("Adding extra configuration to user vm: {}", userVm.getUuid());
3027+
addExtraConfig(userVm, extraConfig);
3028+
} else {
3029+
throw new InvalidParameterValueException("attempted setting extraconfig but enable.additional.vm.configuration is disabled");
3030+
}
3031+
}
3032+
}
3033+
30183034
@Override
30193035
@ActionEvent(eventType = EventTypes.EVENT_VM_UPDATE, eventDescription = "updating Vm")
30203036
public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableException, InsufficientCapacityException {
@@ -3032,6 +3048,7 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
30323048
List<Long> securityGroupIdList = getSecurityGroupIdList(cmd);
30333049
boolean cleanupDetails = cmd.isCleanupDetails();
30343050
String extraConfig = cmd.getExtraConfig();
3051+
boolean cleanupExtraConfig = cmd.isCleanupExtraConfig();
30353052

30363053
UserVmVO vmInstance = _vmDao.findById(cmd.getId());
30373054
VMTemplateVO template = _templateDao.findById(vmInstance.getTemplateId());
@@ -3068,7 +3085,7 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
30683085
.map(item -> (item).trim())
30693086
.collect(Collectors.toList());
30703087
List<VMInstanceDetailVO> existingDetails = vmInstanceDetailsDao.listDetails(id);
3071-
if (cleanupDetails){
3088+
if (cleanupDetails) {
30723089
if (caller != null && caller.getType() == Account.Type.ADMIN) {
30733090
for (final VMInstanceDetailVO detail : existingDetails) {
30743091
if (detail != null && detail.isDisplay() && !isExtraConfig(detail.getName())) {
@@ -3133,15 +3150,8 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
31333150
vmInstance.setDetails(details);
31343151
_vmDao.saveDetails(vmInstance);
31353152
}
3136-
if (StringUtils.isNotBlank(extraConfig)) {
3137-
if (EnableAdditionalVmConfig.valueIn(accountId)) {
3138-
logger.info("Adding extra configuration to user vm: " + vmInstance.getUuid());
3139-
addExtraConfig(vmInstance, extraConfig);
3140-
} else {
3141-
throw new InvalidParameterValueException("attempted setting extraconfig but enable.additional.vm.configuration is disabled");
3142-
}
3143-
}
31443153
}
3154+
updateVmExtraConfig(userVm, extraConfig, cleanupExtraConfig);
31453155

31463156
if (VMLeaseManager.InstanceLeaseEnabled.value() && cmd.getLeaseDuration() != null) {
31473157
applyLeaseOnUpdateInstance(vmInstance, cmd.getLeaseDuration(), cmd.getLeaseExpiryAction());

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

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,10 @@
4040
import static org.mockito.Mockito.never;
4141
import static org.mockito.Mockito.times;
4242
import static org.mockito.Mockito.verify;
43+
import static org.mockito.Mockito.verifyNoMoreInteractions;
4344
import static org.mockito.Mockito.when;
4445

46+
import java.lang.reflect.Field;
4547
import java.text.SimpleDateFormat;
4648
import java.time.LocalDateTime;
4749
import java.time.ZoneOffset;
@@ -90,6 +92,7 @@
9092
import org.apache.cloudstack.engine.subsystem.api.storage.Scope;
9193
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
9294
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
95+
import org.apache.cloudstack.framework.config.ConfigKey;
9396
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
9497
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
9598
import org.apache.cloudstack.storage.template.VnfTemplateManager;
@@ -119,6 +122,7 @@
119122
import com.cloud.deploy.DeployDestination;
120123
import com.cloud.deploy.DeploymentPlanner;
121124
import com.cloud.deploy.DeploymentPlanningManager;
125+
import com.cloud.domain.Domain;
122126
import com.cloud.domain.DomainVO;
123127
import com.cloud.domain.dao.DomainDao;
124128
import com.cloud.event.ActionEventUtils;
@@ -144,9 +148,9 @@
144148
import com.cloud.network.dao.LoadBalancerVMMapVO;
145149
import com.cloud.network.dao.NetworkDao;
146150
import com.cloud.network.dao.NetworkVO;
147-
import com.cloud.network.element.UserDataServiceProvider;
148151
import com.cloud.network.dao.PhysicalNetworkDao;
149152
import com.cloud.network.dao.PhysicalNetworkVO;
153+
import com.cloud.network.element.UserDataServiceProvider;
150154
import com.cloud.network.guru.NetworkGuru;
151155
import com.cloud.network.rules.FirewallRuleVO;
152156
import com.cloud.network.rules.PortForwardingRule;
@@ -173,6 +177,7 @@
173177
import com.cloud.storage.dao.DiskOfferingDao;
174178
import com.cloud.storage.dao.GuestOSDao;
175179
import com.cloud.storage.dao.SnapshotDao;
180+
import com.cloud.storage.dao.SnapshotPolicyDao;
176181
import com.cloud.storage.dao.VMTemplateDao;
177182
import com.cloud.storage.dao.VolumeDao;
178183
import com.cloud.template.VirtualMachineTemplate;
@@ -472,6 +477,24 @@ public class UserVmManagerImplTest {
472477
Class<InvalidParameterValueException> expectedInvalidParameterValueException = InvalidParameterValueException.class;
473478
Class<CloudRuntimeException> expectedCloudRuntimeException = CloudRuntimeException.class;
474479

480+
private Map<ConfigKey, Object> originalConfigValues = new HashMap<>();
481+
482+
483+
private void updateDefaultConfigValue(final ConfigKey configKey, final Object o, boolean revert) {
484+
try {
485+
final String name = "_defaultValue";
486+
Field f = ConfigKey.class.getDeclaredField(name);
487+
f.setAccessible(true);
488+
String stringVal = String.valueOf(o);
489+
if (!revert) {
490+
originalConfigValues.put(configKey, f.get(configKey));
491+
}
492+
f.set(configKey, stringVal);
493+
} catch (IllegalAccessException | NoSuchFieldException e) {
494+
Assert.fail("Failed to mock config " + configKey.key() + " value due to " + e.getMessage());
495+
}
496+
}
497+
475498
@Before
476499
public void beforeTest() {
477500
userVmManagerImpl.resourceLimitService = resourceLimitMgr;
@@ -501,6 +524,9 @@ public void beforeTest() {
501524
public void afterTest() {
502525
CallContext.unregister();
503526
unmanagedVMsManagerMockedStatic.close();
527+
for (Map.Entry<ConfigKey, Object> entry : originalConfigValues.entrySet()) {
528+
updateDefaultConfigValue(entry.getKey(), entry.getValue(), true);
529+
}
504530
}
505531

506532
@Test
@@ -4183,4 +4209,49 @@ public void testUnmanageUserVMSuccess() {
41834209
verify(userVmDao, times(1)).releaseFromLockTable(vmId);
41844210
}
41854211

4212+
@Test
4213+
public void updateVmExtraConfigCleansUpWhenCleanupFlagIsTrue() {
4214+
UserVmVO userVm = mock(UserVmVO.class);
4215+
when(userVm.getUuid()).thenReturn("test-uuid");
4216+
when(userVm.getId()).thenReturn(1L);
4217+
4218+
userVmManagerImpl.updateVmExtraConfig(userVm, "someConfig", true);
4219+
4220+
verify(vmInstanceDetailsDao, times(1)).removeDetailsWithPrefix(1L, ApiConstants.EXTRA_CONFIG);
4221+
verifyNoMoreInteractions(vmInstanceDetailsDao);
4222+
}
4223+
4224+
@Test
4225+
public void updateVmExtraConfigAddsConfigWhenValidAndEnabled() {
4226+
UserVmVO userVm = mock(UserVmVO.class);
4227+
when(userVm.getUuid()).thenReturn("test-uuid");
4228+
when(userVm.getAccountId()).thenReturn(1L);
4229+
when(userVm.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.KVM);
4230+
doNothing().when(userVmManagerImpl).persistExtraConfigKvm(anyString(), eq(userVm));
4231+
updateDefaultConfigValue(UserVmManagerImpl.EnableAdditionalVmConfig, true, false);
4232+
4233+
userVmManagerImpl.updateVmExtraConfig(userVm, "validConfig", false);
4234+
4235+
verify(vmInstanceDetailsDao, never()).removeDetailsWithPrefix(anyLong(), anyString());
4236+
verify(userVmManagerImpl, times(1)).addExtraConfig(userVm, "validConfig");
4237+
}
4238+
4239+
@Test(expected = InvalidParameterValueException.class)
4240+
public void updateVmExtraConfigThrowsExceptionWhenConfigDisabled() {
4241+
UserVmVO userVm = mock(UserVmVO.class);
4242+
when(userVm.getAccountId()).thenReturn(1L);
4243+
updateDefaultConfigValue(UserVmManagerImpl.EnableAdditionalVmConfig, false, false);
4244+
4245+
userVmManagerImpl.updateVmExtraConfig(userVm, "validConfig", false);
4246+
}
4247+
4248+
@Test
4249+
public void updateVmExtraConfigDoesNothingWhenExtraConfigIsBlank() {
4250+
UserVmVO userVm = mock(UserVmVO.class);
4251+
4252+
userVmManagerImpl.updateVmExtraConfig(userVm, "", false);
4253+
4254+
verify(vmInstanceDetailsDao, never()).removeDetailsWithPrefix(anyLong(), anyString());
4255+
verify(userVmManagerImpl, never()).addExtraConfig(any(UserVmVO.class), anyString());
4256+
}
41864257
}

0 commit comments

Comments
 (0)