Skip to content

Commit 18972ca

Browse files
shwstpprshwstppr
authored
api,server: allow cleaning up vm extraconfig (#11974)
1 parent 9d523cb commit 18972ca

File tree

8 files changed

+212
-16
lines changed

8 files changed

+212
-16
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1168,6 +1168,7 @@ public class ApiConstants {
11681168
public static final String OVM3_VIP = "ovm3vip";
11691169
public static final String CLEAN_UP_DETAILS = "cleanupdetails";
11701170
public static final String CLEAN_UP_EXTERNAL_DETAILS = "cleanupexternaldetails";
1171+
public static final String CLEAN_UP_EXTRA_CONFIG = "cleanupextraconfig";
11711172
public static final String CLEAN_UP_PARAMETERS = "cleanupparameters";
11721173
public static final String VIRTUAL_SIZE = "virtualsize";
11731174
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
@@ -163,6 +163,12 @@ public class UpdateVMCmd extends BaseCustomIdCmd implements SecurityGroupAction,
163163
description = "Lease expiry action, valid values are STOP and DESTROY")
164164
private String leaseExpiryAction;
165165

166+
@Parameter(name = ApiConstants.CLEAN_UP_EXTRA_CONFIG, type = CommandType.BOOLEAN, since = "4.23.0",
167+
description = "Optional boolean field, which indicates if extraconfig for the instance should be " +
168+
"cleaned up or not (If set to true, extraconfig removed for this instance, extraconfig field " +
169+
"ignored; if false or not set, no action)")
170+
private Boolean cleanupExtraConfig;
171+
166172
/////////////////////////////////////////////////////
167173
/////////////////// Accessors ///////////////////////
168174
/////////////////////////////////////////////////////
@@ -271,14 +277,18 @@ public String getExtraConfig() {
271277
return extraConfig;
272278
}
273279

274-
/////////////////////////////////////////////////////
275-
/////////////// API Implementation///////////////////
276-
/////////////////////////////////////////////////////
277-
278280
public Long getOsTypeId() {
279281
return osTypeId;
280282
}
281283

284+
public boolean isCleanupExtraConfig() {
285+
return Boolean.TRUE.equals(cleanupExtraConfig);
286+
}
287+
288+
/////////////////////////////////////////////////////
289+
/////////////// API Implementation///////////////////
290+
/////////////////////////////////////////////////////
291+
282292
@Override
283293
public String getCommandName() {
284294
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
@@ -2866,6 +2866,22 @@ private void adjustVmLimits(Account owner, UserVmVO vmInstance, ServiceOfferingV
28662866
}
28672867
}
28682868

2869+
protected void updateVmExtraConfig(UserVmVO userVm, String extraConfig, boolean cleanupExtraConfig) {
2870+
if (cleanupExtraConfig) {
2871+
logger.info("Cleaning up extraconfig from user vm: {}", userVm.getUuid());
2872+
vmInstanceDetailsDao.removeDetailsWithPrefix(userVm.getId(), ApiConstants.EXTRA_CONFIG);
2873+
return;
2874+
}
2875+
if (StringUtils.isNotBlank(extraConfig)) {
2876+
if (EnableAdditionalVmConfig.valueIn(userVm.getAccountId())) {
2877+
logger.info("Adding extra configuration to user vm: {}", userVm.getUuid());
2878+
addExtraConfig(userVm, extraConfig);
2879+
} else {
2880+
throw new InvalidParameterValueException("attempted setting extraconfig but enable.additional.vm.configuration is disabled");
2881+
}
2882+
}
2883+
}
2884+
28692885
@Override
28702886
@ActionEvent(eventType = EventTypes.EVENT_VM_UPDATE, eventDescription = "updating Vm")
28712887
public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableException, InsufficientCapacityException {
@@ -2883,6 +2899,7 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
28832899
List<Long> securityGroupIdList = getSecurityGroupIdList(cmd);
28842900
boolean cleanupDetails = cmd.isCleanupDetails();
28852901
String extraConfig = cmd.getExtraConfig();
2902+
boolean cleanupExtraConfig = cmd.isCleanupExtraConfig();
28862903

28872904
UserVmVO vmInstance = _vmDao.findById(cmd.getId());
28882905
VMTemplateVO template = _templateDao.findById(vmInstance.getTemplateId());
@@ -2919,7 +2936,7 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
29192936
.map(item -> (item).trim())
29202937
.collect(Collectors.toList());
29212938
List<VMInstanceDetailVO> existingDetails = vmInstanceDetailsDao.listDetails(id);
2922-
if (cleanupDetails){
2939+
if (cleanupDetails) {
29232940
if (caller != null && caller.getType() == Account.Type.ADMIN) {
29242941
for (final VMInstanceDetailVO detail : existingDetails) {
29252942
if (detail != null && detail.isDisplay() && !isExtraConfig(detail.getName())) {
@@ -2982,15 +2999,8 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
29822999
vmInstance.setDetails(details);
29833000
_vmDao.saveDetails(vmInstance);
29843001
}
2985-
if (StringUtils.isNotBlank(extraConfig)) {
2986-
if (EnableAdditionalVmConfig.valueIn(accountId)) {
2987-
logger.info("Adding extra configuration to user vm: " + vmInstance.getUuid());
2988-
addExtraConfig(vmInstance, extraConfig);
2989-
} else {
2990-
throw new InvalidParameterValueException("attempted setting extraconfig but enable.additional.vm.configuration is disabled");
2991-
}
2992-
}
29933002
}
3003+
updateVmExtraConfig(userVm, extraConfig, cleanupExtraConfig);
29943004

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

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

Lines changed: 72 additions & 3 deletions
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;
@@ -59,8 +61,6 @@
5961
import java.util.TimeZone;
6062
import java.util.UUID;
6163

62-
import com.cloud.domain.Domain;
63-
import com.cloud.storage.dao.SnapshotPolicyDao;
6464
import org.apache.cloudstack.acl.ControlledEntity;
6565
import org.apache.cloudstack.acl.SecurityChecker;
6666
import org.apache.cloudstack.api.ApiCommandResourceType;
@@ -88,6 +88,7 @@
8888
import org.apache.cloudstack.engine.subsystem.api.storage.Scope;
8989
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
9090
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
91+
import org.apache.cloudstack.framework.config.ConfigKey;
9192
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
9293
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
9394
import org.apache.cloudstack.storage.template.VnfTemplateManager;
@@ -117,6 +118,7 @@
117118
import com.cloud.deploy.DeployDestination;
118119
import com.cloud.deploy.DeploymentPlanner;
119120
import com.cloud.deploy.DeploymentPlanningManager;
121+
import com.cloud.domain.Domain;
120122
import com.cloud.domain.DomainVO;
121123
import com.cloud.domain.dao.DomainDao;
122124
import com.cloud.event.ActionEventUtils;
@@ -142,9 +144,9 @@
142144
import com.cloud.network.dao.LoadBalancerVMMapVO;
143145
import com.cloud.network.dao.NetworkDao;
144146
import com.cloud.network.dao.NetworkVO;
145-
import com.cloud.network.element.UserDataServiceProvider;
146147
import com.cloud.network.dao.PhysicalNetworkDao;
147148
import com.cloud.network.dao.PhysicalNetworkVO;
149+
import com.cloud.network.element.UserDataServiceProvider;
148150
import com.cloud.network.guru.NetworkGuru;
149151
import com.cloud.network.rules.FirewallRuleVO;
150152
import com.cloud.network.rules.PortForwardingRule;
@@ -172,6 +174,7 @@
172174
import com.cloud.storage.dao.DiskOfferingDao;
173175
import com.cloud.storage.dao.GuestOSDao;
174176
import com.cloud.storage.dao.SnapshotDao;
177+
import com.cloud.storage.dao.SnapshotPolicyDao;
175178
import com.cloud.storage.dao.VMTemplateDao;
176179
import com.cloud.storage.dao.VolumeDao;
177180
import com.cloud.template.VirtualMachineTemplate;
@@ -467,6 +470,24 @@ public class UserVmManagerImplTest {
467470
Class<InvalidParameterValueException> expectedInvalidParameterValueException = InvalidParameterValueException.class;
468471
Class<CloudRuntimeException> expectedCloudRuntimeException = CloudRuntimeException.class;
469472

473+
private Map<ConfigKey, Object> originalConfigValues = new HashMap<>();
474+
475+
476+
private void updateDefaultConfigValue(final ConfigKey configKey, final Object o, boolean revert) {
477+
try {
478+
final String name = "_defaultValue";
479+
Field f = ConfigKey.class.getDeclaredField(name);
480+
f.setAccessible(true);
481+
String stringVal = String.valueOf(o);
482+
if (!revert) {
483+
originalConfigValues.put(configKey, f.get(configKey));
484+
}
485+
f.set(configKey, stringVal);
486+
} catch (IllegalAccessException | NoSuchFieldException e) {
487+
Assert.fail("Failed to mock config " + configKey.key() + " value due to " + e.getMessage());
488+
}
489+
}
490+
470491
@Before
471492
public void beforeTest() {
472493
userVmManagerImpl.resourceLimitService = resourceLimitMgr;
@@ -496,6 +517,9 @@ public void beforeTest() {
496517
public void afterTest() {
497518
CallContext.unregister();
498519
unmanagedVMsManagerMockedStatic.close();
520+
for (Map.Entry<ConfigKey, Object> entry : originalConfigValues.entrySet()) {
521+
updateDefaultConfigValue(entry.getKey(), entry.getValue(), true);
522+
}
499523
}
500524

501525
@Test
@@ -4178,4 +4202,49 @@ public void testUnmanageUserVMSuccess() {
41784202
verify(userVmDao, times(1)).releaseFromLockTable(vmId);
41794203
}
41804204

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

‎ui/src/views/compute/EditVM.vue‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -425,6 +425,8 @@ export default {
425425
}
426426
if (values.extraconfig && values.extraconfig.length > 0) {
427427
params.extraconfig = encodeURIComponent(values.extraconfig)
428+
} else if (this.combinedExtraConfig) {
429+
params.cleanupextraconfig = true
428430
}
429431
this.loading = true
430432

0 commit comments

Comments
 (0)