Skip to content

Commit 3ecf191

Browse files
andrijapanicsbandrijapanicsb
committed
fix(cks): preserve node offerings during nodeids scale-in
Do not infer a global resize when offering parameters are omitted. Keep node-type offerings in count-only accounting and continue processing explicit autoscaling changes without a resize. Add regression coverage for selected-worker removal, mixed and legacy offerings, and autoscaling-only requests. Fixes #14458
1 parent 1abb539 commit 3ecf191

4 files changed

Lines changed: 165 additions & 20 deletions

File tree

‎plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2283,7 +2283,7 @@ public boolean scaleKubernetesCluster(ScaleKubernetesClusterCmd cmd) throws Clou
22832283
}
22842284
validateKubernetesClusterScaleParameters(cmd);
22852285
KubernetesClusterVO kubernetesCluster = kubernetesClusterDao.findById(cmd.getId());
2286-
Map<String, ServiceOffering> nodeToOfferingMap = createNodeTypeToServiceOfferingMap(cmd.getServiceOfferingNodeTypeMap(), cmd.getServiceOfferingId(), kubernetesCluster);
2286+
Map<String, ServiceOffering> nodeToOfferingMap = createNodeTypeToServiceOfferingMap(cmd.getServiceOfferingNodeTypeMap(), cmd.getServiceOfferingId());
22872287

22882288
String[] keys = getServiceUserKeys(kubernetesCluster);
22892289
KubernetesClusterScaleWorker scaleWorker =
@@ -2302,16 +2302,15 @@ public boolean scaleKubernetesCluster(ScaleKubernetesClusterCmd cmd) throws Clou
23022302

23032303
/**
23042304
* Creates a map for the requested node type service offering
2305-
* For the node type DEFAULT: Every node is scaled to the same offering
2305+
* For the node type DEFAULT: Every node is scaled to the same explicitly requested offering.
2306+
* Omitting offering parameters preserves the existing offerings during size-only operations.
23062307
*/
2307-
protected Map<String, ServiceOffering> createNodeTypeToServiceOfferingMap(Map<String, Long> idsMapping,
2308-
Long serviceOfferingId, KubernetesClusterVO kubernetesCluster) {
2308+
protected Map<String, ServiceOffering> createNodeTypeToServiceOfferingMap(Map<String, Long> idsMapping, Long serviceOfferingId) {
23092309
Map<String, ServiceOffering> map = new HashMap<>();
23102310
if (MapUtils.isEmpty(idsMapping)) {
2311-
ServiceOfferingVO offering = serviceOfferingId != null ?
2312-
serviceOfferingDao.findById(serviceOfferingId) :
2313-
serviceOfferingDao.findById(kubernetesCluster.getServiceOfferingId());
2314-
map.put(DEFAULT.name(), offering);
2311+
if (serviceOfferingId != null) {
2312+
map.put(DEFAULT.name(), serviceOfferingDao.findById(serviceOfferingId));
2313+
}
23152314
return map;
23162315
}
23172316
for (String key : CLUSTER_NODES_TYPES_LIST) {

‎plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterScaleWorker.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ private KubernetesClusterVO updateKubernetesClusterEntryForNodeType(final Long n
212212
final ServiceOffering newServiceOffering,
213213
final boolean updateNodeOffering, boolean updateClusterOffering) throws CloudRuntimeException {
214214
final ServiceOffering serviceOffering = newServiceOffering == null ?
215-
serviceOfferingDao.findById(kubernetesCluster.getServiceOfferingId()) : newServiceOffering;
215+
getServiceOfferingForNodeTypeOnCluster(nodeType, kubernetesCluster) : newServiceOffering;
216216
final Long serviceOfferingId = newServiceOffering == null ? null : serviceOffering.getId();
217217

218218
Pair<Long, Long> clusterCountAndCapacity = calculateNewClusterCountAndCapacity(newWorkerSize, nodeType, serviceOffering);
@@ -394,7 +394,7 @@ private void scaleKubernetesClusterOffering(KubernetesClusterNodeType nodeType,
394394
kubernetesCluster = updateKubernetesClusterEntryForNodeType(null, nodeType, serviceOffering, updateNodeOffering, updateClusterOffering);
395395
}
396396

397-
private void removeNodesFromCluster(List<KubernetesClusterVmMapVO> vmMaps) throws CloudRuntimeException {
397+
protected void removeNodesFromCluster(List<KubernetesClusterVmMapVO> vmMaps) throws CloudRuntimeException {
398398
for (KubernetesClusterVmMapVO vmMapVO : vmMaps) {
399399
UserVmVO userVM = userVmDao.findById(vmMapVO.getVmId());
400400
logger.info("Removing vm {} from cluster {}", userVM, kubernetesCluster);
@@ -605,7 +605,8 @@ public boolean scaleCluster() throws CloudRuntimeException {
605605
for (KubernetesClusterNodeType nodeType : Arrays.asList(CONTROL, ETCD, WORKER)) {
606606
boolean isWorkerNode = WORKER == nodeType;
607607
final long newVMRequired = (!isWorkerNode || clusterSize == null) ? 0 : clusterSize - originalClusterSize;
608-
if (!scaleClusterDefaultOffering && !serviceOfferingNodeTypeMap.containsKey(nodeType.name()) && newVMRequired == 0) {
608+
if (!scaleClusterDefaultOffering && !serviceOfferingNodeTypeMap.containsKey(nodeType.name()) && newVMRequired == 0
609+
&& !(isWorkerNode && autoscalingChanged)) {
609610
continue;
610611
}
611612

‎plugins/integrations/kubernetes-service/src/test/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImplTest.java‎

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@
5757
import org.apache.cloudstack.api.response.KubernetesClusterResponse;
5858
import org.apache.cloudstack.context.CallContext;
5959
import org.apache.cloudstack.framework.config.ConfigKey;
60-
import org.apache.commons.collections.MapUtils;
6160
import org.junit.After;
6261
import org.junit.Assert;
6362
import org.junit.Before;
@@ -408,15 +407,35 @@ public void testIsAnyNodeOfferingEmpty() {
408407
}
409408

410409
@Test
411-
public void testCreateNodeTypeToServiceOfferingMapNullMap() {
412-
KubernetesClusterVO clusterVO = Mockito.mock(KubernetesClusterVO.class);
413-
Mockito.when(clusterVO.getServiceOfferingId()).thenReturn(1L);
410+
public void testMissingOfferingParametersDoNotRequestGlobalResize() {
411+
Assert.assertTrue(kubernetesClusterManager.createNodeTypeToServiceOfferingMap(null, null).isEmpty());
412+
Mockito.verifyNoInteractions(serviceOfferingDao);
413+
}
414+
415+
@Test
416+
public void testEmptyOfferingParametersDoNotRequestGlobalResize() {
417+
Assert.assertTrue(kubernetesClusterManager.createNodeTypeToServiceOfferingMap(new HashMap<>(), null).isEmpty());
418+
Mockito.verifyNoInteractions(serviceOfferingDao);
419+
}
420+
421+
@Test
422+
public void testExplicitGlobalOfferingStillRequestsGlobalResize() {
423+
ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class);
424+
Mockito.when(serviceOfferingDao.findById(1L)).thenReturn(offering);
425+
426+
Map<String, ServiceOffering> mapping = kubernetesClusterManager.createNodeTypeToServiceOfferingMap(null, 1L);
427+
428+
Assert.assertEquals(Collections.singletonMap(DEFAULT.name(), offering), mapping);
429+
}
430+
431+
@Test
432+
public void testExplicitGlobalOfferingWithEmptyNodeMap() {
414433
ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class);
415434
Mockito.when(serviceOfferingDao.findById(1L)).thenReturn(offering);
416-
Map<String, ServiceOffering> mapping = kubernetesClusterManager.createNodeTypeToServiceOfferingMap(new HashMap<>(), null, clusterVO);
417-
Assert.assertFalse(MapUtils.isEmpty(mapping));
418-
Assert.assertTrue(mapping.containsKey(DEFAULT.name()));
419-
Assert.assertEquals(offering, mapping.get(DEFAULT.name()));
435+
436+
Map<String, ServiceOffering> mapping = kubernetesClusterManager.createNodeTypeToServiceOfferingMap(new HashMap<>(), 1L);
437+
438+
Assert.assertEquals(Collections.singletonMap(DEFAULT.name(), offering), mapping);
420439
}
421440

422441
@Test
@@ -432,7 +451,7 @@ public void testCreateNodeTypeToServiceOfferingMap() {
432451
ServiceOfferingVO controlOffering = Mockito.mock(ServiceOfferingVO.class);
433452
Mockito.when(serviceOfferingDao.findById(controlOfferingId)).thenReturn(controlOffering);
434453

435-
Map<String, ServiceOffering> mapping = kubernetesClusterManager.createNodeTypeToServiceOfferingMap(idsMap, null, null);
454+
Map<String, ServiceOffering> mapping = kubernetesClusterManager.createNodeTypeToServiceOfferingMap(idsMap, null);
436455
Assert.assertEquals(2, mapping.size());
437456
Assert.assertTrue(mapping.containsKey(WORKER.name()) && mapping.containsKey(CONTROL.name()));
438457
Assert.assertEquals(workerOffering, mapping.get(WORKER.name()));

‎plugins/integrations/kubernetes-service/src/test/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterScaleWorkerTest.java‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,15 @@
1919
import com.cloud.kubernetes.cluster.KubernetesCluster;
2020
import com.cloud.kubernetes.cluster.KubernetesClusterVmMapVO;
2121
import com.cloud.kubernetes.cluster.KubernetesClusterManagerImpl;
22+
import com.cloud.kubernetes.cluster.KubernetesClusterVO;
2223
import com.cloud.kubernetes.cluster.dao.KubernetesClusterVmMapDao;
2324
import com.cloud.offering.ServiceOffering;
25+
import com.cloud.network.Network;
2426
import com.cloud.service.ServiceOfferingVO;
2527
import com.cloud.service.dao.ServiceOfferingDao;
2628
import com.cloud.utils.Pair;
2729
import com.cloud.vm.UserVmVO;
30+
import com.cloud.vm.UserVmManager;
2831
import com.cloud.vm.dao.UserVmDao;
2932
import org.junit.Assert;
3033
import org.junit.Before;
@@ -34,12 +37,16 @@
3437
import org.mockito.Mock;
3538
import org.mockito.Mockito;
3639
import org.mockito.junit.MockitoJUnitRunner;
40+
import org.springframework.test.util.ReflectionTestUtils;
3741

3842
import java.util.Arrays;
43+
import java.util.ArrayList;
44+
import java.util.Collections;
3945
import java.util.List;
4046

4147
import static com.cloud.kubernetes.cluster.KubernetesServiceHelper.KubernetesClusterNodeType.CONTROL;
4248
import static com.cloud.kubernetes.cluster.KubernetesServiceHelper.KubernetesClusterNodeType.DEFAULT;
49+
import static com.cloud.kubernetes.cluster.KubernetesServiceHelper.KubernetesClusterNodeType.WORKER;
4350

4451
@RunWith(MockitoJUnitRunner.class)
4552
public class KubernetesClusterScaleWorkerTest {
@@ -54,6 +61,8 @@ public class KubernetesClusterScaleWorkerTest {
5461
private KubernetesClusterVmMapDao kubernetesClusterVmMapDao;
5562
@Mock
5663
private UserVmDao userVmDao;
64+
@Mock
65+
private UserVmManager userVmManager;
5766

5867
private KubernetesClusterScaleWorker worker;
5968

@@ -67,6 +76,123 @@ public void setUp() {
6776
worker.userVmDao = userVmDao;
6877
}
6978

79+
@Test
80+
public void testNodeIdsScaleInPreservesMixedOfferingsAndRemovesSelectedWorker() {
81+
assertScaleInPreservesOfferings(true, false);
82+
}
83+
84+
@Test
85+
public void testCountOnlyScaleInPreservesMixedOfferings() {
86+
assertScaleInPreservesOfferings(false, false);
87+
}
88+
89+
@Test
90+
public void testNodeIdsScaleInPreservesLegacyGlobalOffering() {
91+
assertScaleInPreservesOfferings(true, true);
92+
}
93+
94+
private void assertScaleInPreservesOfferings(boolean selectedNode, boolean legacyOffering) {
95+
Mockito.when(kubernetesCluster.getId()).thenReturn(7L);
96+
Mockito.when(kubernetesCluster.getState()).thenReturn(KubernetesCluster.State.Running);
97+
Mockito.when(kubernetesCluster.getNodeCount()).thenReturn(2L);
98+
Mockito.when(kubernetesCluster.getTotalNodeCount()).thenReturn(3L);
99+
Mockito.when(kubernetesCluster.getServiceOfferingId()).thenReturn(defaultOfferingId);
100+
Mockito.when(kubernetesCluster.getMinSize()).thenReturn(null);
101+
Mockito.when(kubernetesCluster.getMaxSize()).thenReturn(null);
102+
Mockito.when(kubernetesCluster.getCores()).thenReturn(legacyOffering ? 6L : 9L);
103+
Mockito.when(kubernetesCluster.getMemory()).thenReturn(legacyOffering ? 6144L : 11264L);
104+
ServiceOfferingVO legacy = Mockito.mock(ServiceOfferingVO.class);
105+
if (legacyOffering) {
106+
Mockito.when(legacy.getCpu()).thenReturn(2);
107+
Mockito.when(legacy.getRamSize()).thenReturn(2048);
108+
}
109+
Mockito.when(serviceOfferingDao.findById(defaultOfferingId)).thenReturn(legacy);
110+
long workerOfferingId = legacyOffering ? defaultOfferingId : 12L;
111+
Mockito.when(kubernetesCluster.getWorkerNodeServiceOfferingId()).thenReturn(legacyOffering ? null : workerOfferingId);
112+
if (!legacyOffering) {
113+
ServiceOfferingVO workerOffering = Mockito.mock(ServiceOfferingVO.class);
114+
Mockito.when(workerOffering.getCpu()).thenReturn(3);
115+
Mockito.when(workerOffering.getRamSize()).thenReturn(4096);
116+
Mockito.when(serviceOfferingDao.findById(workerOfferingId)).thenReturn(workerOffering);
117+
}
118+
KubernetesClusterVmMapVO control = Mockito.mock(KubernetesClusterVmMapVO.class);
119+
KubernetesClusterVmMapVO retained = Mockito.mock(KubernetesClusterVmMapVO.class);
120+
KubernetesClusterVmMapVO removed = Mockito.mock(KubernetesClusterVmMapVO.class);
121+
Mockito.when(retained.getVmId()).thenReturn(100L);
122+
UserVmVO retainedVm = Mockito.mock(UserVmVO.class);
123+
Mockito.when(retainedVm.getServiceOfferingId()).thenReturn(workerOfferingId);
124+
Mockito.when(userVmDao.findById(100L)).thenReturn(retainedVm);
125+
Mockito.when(kubernetesClusterVmMapDao.listByClusterId(7L)).thenReturn(Arrays.asList(control, retained, removed));
126+
Mockito.when(kubernetesClusterVmMapDao.listByClusterIdAndVmType(7L, WORKER)).thenReturn(Arrays.asList(retained, removed));
127+
KubernetesClusterScaleWorker scaleWorker = Mockito.spy(new KubernetesClusterScaleWorker(kubernetesCluster,
128+
Collections.emptyMap(), selectedNode ? null : 1L, selectedNode ? List.of(101L) : null, null, null, null, clusterManager));
129+
scaleWorker.serviceOfferingDao = serviceOfferingDao;
130+
scaleWorker.kubernetesClusterVmMapDao = kubernetesClusterVmMapDao;
131+
scaleWorker.userVmDao = userVmDao;
132+
scaleWorker.userVmManager = userVmManager;
133+
Network network = Mockito.mock(Network.class);
134+
ReflectionTestUtils.setField(scaleWorker, "network", network);
135+
Mockito.doNothing().when(scaleWorker).init();
136+
Mockito.doReturn(new Pair<>("192.0.2.1", 2222)).when(scaleWorker).getKubernetesClusterServerIpSshPort(null);
137+
Mockito.doReturn("192.0.2.1").when(scaleWorker).resolveApiPublicIpAddress(network);
138+
Mockito.doReturn(true).when(scaleWorker).stateTransitTo(Mockito.anyLong(), Mockito.any());
139+
if (selectedNode) {
140+
Mockito.doReturn(new ArrayList<>(List.of(removed))).when(scaleWorker).getKubernetesClusterVMMapsForNodes(List.of(101L));
141+
} else {
142+
Mockito.doReturn(new ArrayList<>(List.of(removed))).when(scaleWorker).getWorkerNodesToRemove();
143+
}
144+
Mockito.doNothing().when(scaleWorker).removeNodesFromCluster(Mockito.anyList());
145+
KubernetesClusterVO updatedCluster = Mockito.mock(KubernetesClusterVO.class);
146+
Mockito.doReturn(updatedCluster).when(scaleWorker).updateKubernetesClusterEntry(Mockito.any(), Mockito.any(), Mockito.any(),
147+
Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.anyBoolean(), Mockito.anyBoolean());
148+
149+
Assert.assertTrue(scaleWorker.scaleCluster());
150+
151+
Mockito.verify(scaleWorker).removeNodesFromCluster(List.of(removed));
152+
Mockito.verify(scaleWorker).updateKubernetesClusterEntry(legacyOffering ? 4L : 6L, legacyOffering ? 4096L : 7168L,
153+
1L, null, false, null, null, WORKER, false, false);
154+
Mockito.verifyNoInteractions(userVmManager);
155+
Mockito.verify(kubernetesClusterVmMapDao, Mockito.never()).listByClusterIdAndVmType(7L, CONTROL);
156+
}
157+
158+
@Test
159+
public void testAutoscalingOnlyEnablePreservesOfferings() {
160+
assertAutoscalingPreservesOfferings(false, true, false);
161+
}
162+
163+
@Test
164+
public void testAutoscalingOnlyDisablePreservesOfferings() {
165+
assertAutoscalingPreservesOfferings(true, false, false);
166+
}
167+
168+
@Test
169+
public void testAutoscalingBoundsOnlyChangePreservesOfferings() {
170+
assertAutoscalingPreservesOfferings(true, true, true);
171+
}
172+
173+
private void assertAutoscalingPreservesOfferings(boolean existingEnabled, boolean requestedEnabled, boolean boundsChange) {
174+
Mockito.when(kubernetesCluster.getState()).thenReturn(KubernetesCluster.State.Running);
175+
Mockito.when(kubernetesCluster.getAutoscalingEnabled()).thenReturn(existingEnabled);
176+
if (boundsChange) {
177+
Mockito.when(kubernetesCluster.getMinSize()).thenReturn(1L);
178+
}
179+
KubernetesClusterScaleWorker scaleWorker = Mockito.spy(new KubernetesClusterScaleWorker(kubernetesCluster,
180+
Collections.emptyMap(), null, null, requestedEnabled, 2L, 5L, clusterManager));
181+
scaleWorker.serviceOfferingDao = serviceOfferingDao;
182+
scaleWorker.userVmManager = userVmManager;
183+
ServiceOfferingVO existingOffering = Mockito.mock(ServiceOfferingVO.class);
184+
Mockito.when(serviceOfferingDao.findById(12L)).thenReturn(existingOffering);
185+
Mockito.doNothing().when(scaleWorker).init();
186+
Mockito.doReturn(12L).when(scaleWorker).getExistingOfferingIdForNodeType(WORKER, kubernetesCluster);
187+
Mockito.doReturn(true).when(scaleWorker).autoscaleCluster(requestedEnabled, 2L, 5L);
188+
Mockito.doReturn(true).when(scaleWorker).stateTransitTo(Mockito.anyLong(), Mockito.any());
189+
190+
Assert.assertTrue(scaleWorker.scaleCluster());
191+
192+
Mockito.verify(scaleWorker).autoscaleCluster(requestedEnabled, 2L, 5L);
193+
Mockito.verifyNoInteractions(userVmManager);
194+
}
195+
70196
@Test
71197
public void testCalculateNewClusterCountAndCapacityAllNodesScaleSize() {
72198
long controlNodes = 3L;

0 commit comments

Comments
 (0)