DaanHoogland commented on code in PR #7515:
URL: https://github.com/apache/cloudstack/pull/7515#discussion_r1217616356
##########
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java:
##########
@@ -607,8 +622,58 @@ private void validateEndpointUrl() {
throw new InvalidParameterValueException(error);
}
}
+ private void validateUnmanagedKubernetesClusterCreateParameters(final
CreateKubernetesClusterCmd cmd) throws CloudRuntimeException {
Review Comment:
can these methods be disected into smaller, shared validations?
##########
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java:
##########
@@ -607,8 +622,58 @@ private void validateEndpointUrl() {
throw new InvalidParameterValueException(error);
}
}
+ private void validateUnmanagedKubernetesClusterCreateParameters(final
CreateKubernetesClusterCmd cmd) throws CloudRuntimeException {
+ final String name = cmd.getName();
+ final Long zoneId = cmd.getZoneId();
+ final Account owner =
accountService.getActiveAccountById(cmd.getEntityOwnerId());
+ final Long networkId = cmd.getNetworkId();
+ final String sshKeyPair = cmd.getSSHKeyPairName();
+ final String dockerRegistryUserName = cmd.getDockerRegistryUserName();
+ final String dockerRegistryPassword = cmd.getDockerRegistryPassword();
+ final String dockerRegistryUrl = cmd.getDockerRegistryUrl();
+ final Long nodeRootDiskSize = cmd.getNodeRootDiskSize();
+ final String externalLoadBalancerIpAddress =
cmd.getExternalLoadBalancerIpAddress();
+
+ if (name == null || name.isEmpty()) {
+ throw new InvalidParameterValueException("Invalid name for the
Kubernetes cluster name: " + name);
+ }
+
+ DataCenter zone = dataCenterDao.findById(zoneId);
+ if (zone == null) {
+ throw new InvalidParameterValueException("Unable to find zone by
ID: " + zoneId);
+ }
+
+ if (zone.getAllocationState() == Grouping.AllocationState.Disabled) {
+ throw new PermissionDeniedException(String.format("Cannot perform
this operation, zone ID: %s is currently disabled", zone.getUuid()));
+ }
- private void validateKubernetesClusterCreateParameters(final
CreateKubernetesClusterCmd cmd) throws CloudRuntimeException {
+ if (!StringUtils.isBlank(sshKeyPair)) {
+ SSHKeyPairVO sshKeyPairVO =
sshKeyPairDao.findByName(owner.getAccountId(), owner.getDomainId(), sshKeyPair);
+ if (sshKeyPairVO == null) {
+ throw new InvalidParameterValueException(String.format("Given
SSH key pair with name: %s was not found for the account %s", sshKeyPair,
owner.getAccountName()));
+ }
+ }
+
+ if (nodeRootDiskSize != null && nodeRootDiskSize <= 0) {
+ throw new InvalidParameterValueException(String.format("Invalid
value for %s", ApiConstants.NODE_ROOT_DISK_SIZE));
+ }
+
+ validateDockerRegistryParams(dockerRegistryUserName,
dockerRegistryPassword, dockerRegistryUrl);
+
+ Network network = null;
+ if (networkId != null) {
+ network = networkService.getNetwork(networkId);
+ if (network == null) {
+ throw new InvalidParameterValueException("Unable to find
network with given ID");
+ }
+ }
+
+ if (StringUtils.isNotEmpty(externalLoadBalancerIpAddress) &&
(!NetUtils.isValidIp4(externalLoadBalancerIpAddress) &&
!NetUtils.isValidIp6(externalLoadBalancerIpAddress))) {
+ throw new InvalidParameterValueException("Invalid external load
balancer IP address");
+ }
+ }
+
+ private void validateManagedKubernetesClusterCreateParameters(final
CreateKubernetesClusterCmd cmd) throws CloudRuntimeException {
Review Comment:
can these methods be dissected into smaller, shared validations?
see L625
##########
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java:
##########
@@ -1342,6 +1507,71 @@ public boolean
upgradeKubernetesCluster(UpgradeKubernetesClusterCmd cmd) throws
return upgradeWorker.upgradeCluster();
}
+ @Override
+ public boolean addVmsToCluster(AddVirtualMachinesToKubernetesClusterCmd
cmd) {
Review Comment:
I see no validation code for the actual VM, what are we trusting to the user
and what is automagically done by the system?
I understand that we require due diligence from the user. No issue there. We
should be able to document that properly though.
##########
plugins/integrations/kubernetes-service/src/main/java/org/apache/cloudstack/api/command/user/kubernetes/cluster/RemoveVirtualMachinesFromKubernetesClusterCmd.java:
##########
@@ -0,0 +1,99 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.api.command.user.kubernetes.cluster;
+
+import com.cloud.kubernetes.cluster.KubernetesClusterService;
+import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.KubernetesClusterResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.cloudstack.api.response.UserVmResponse;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import javax.inject.Inject;
+import java.util.List;
+
+@APICommand(name = "removeVirtualMachinesFromKubernetesCluster",
+ description = "Remove VMs from a kubernetes cluster",
+ responseObject = SuccessResponse.class,
+ since = "4.19.0",
+ authorized = {RoleType.Admin, RoleType.ResourceAdmin,
RoleType.DomainAdmin, RoleType.User})
+public class RemoveVirtualMachinesFromKubernetesClusterCmd extends BaseCmd {
Review Comment:
What is the practical use of this command? Only for unmanage, or also for
scaling? I'd like to see that documented here (or linked to)
##########
plugins/integrations/kubernetes-service/src/main/java/org/apache/cloudstack/api/command/user/kubernetes/cluster/StartKubernetesClusterCmd.java:
##########
@@ -99,6 +99,10 @@ public KubernetesCluster validateRequest() {
if (kubernetesCluster == null) {
throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Given
Kubernetes cluster was not found");
}
+ if (kubernetesCluster.getClusterType() ==
KubernetesCluster.ClusterType.ExternalManaged) {
+ throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
+ String.format("Start kubernetes cluster is not supported
for an externally managed cluster (%s)", kubernetesCluster.getName()));
+ }
Review Comment:
can you extract this (and maybe the other validations) for readability?
##########
test/integration/smoke/test_kubernetes_clusters.py:
##########
@@ -610,7 +615,84 @@ def test_10_vpc_tier_kubernetes_cluster(self):
k8s_cluster = None
return
- def createKubernetesCluster(self, name, version_id, size=1,
control_nodes=1):
+ @attr(tags=["advanced", "smoke"], required_hardware="false")
+ def test_11_test_unmanaged_cluster_lifecycle(self):
+ """Test all operations on unmanaged Kubernetes cluster
+
+ # Validate the following:
+ # 1. createKubernetesCluster should return valid info for new cluster
+ # 2. The Cloud Database contains the valid information
+ # 3. stopKubernetesCluster doesn't work
+ # 4. startKubernetesCluster doesn't work
+ # 5. upgradeKubernetesCluster doesn't work
+ # 6. Adding & removing vm from cluster works
+ # 7. deleteKubernetesCluster should delete an existing HA Kubernetes
cluster
+ """
+ cluster = self.createKubernetesCluster("test-unmanaged-cluster", None,
+ cluster_type="ExternalManaged")
Review Comment:
```suggestion
cluster = self.createKubernetesCluster("test-unmanaged-cluster",
None,
cluster_type="ExternalManaged")
self.cleanup.append(cluster)
```
is this needed/feasable? It seems to be the way it should be done.
Note that in `createKubernetesCluser()`,
```
if not clusterResponse:
self.cleanup.append(clusterResponse)
```
, is done. so only if no response the cluster is added???
bit scope creeping here, but the current implementation does not seem right.
##########
plugins/integrations/kubernetes-service/src/main/java/org/apache/cloudstack/api/command/user/kubernetes/cluster/CreateKubernetesClusterCmd.java:
##########
@@ -144,6 +145,9 @@ public class CreateKubernetesClusterCmd extends
BaseAsyncCreateCmd {
description = "root disk size in GB for each node")
private Long nodeRootDiskSize;
+ @Parameter(name = ApiConstants.CLUSTER_TYPE, type = CommandType.STRING,
required = true, description = "type of the cluster: CloudManaged,
ExternalManaged")
Review Comment:
we could add a since parameter here:
```suggestion
@Parameter(name = ApiConstants.CLUSTER_TYPE, type = CommandType.STRING,
required = true, description = "type of the cluster: CloudManaged,
ExternalManaged", since="4.19.0.0")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
