shwstppr commented on a change in pull request #4329:
URL: https://github.com/apache/cloudstack/pull/4329#discussion_r693855982
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -453,6 +457,12 @@ public void allocate(final String vmInstanceName, final
VirtualMachineTemplate t
final VirtualMachineProfileImpl vmProfile = new
VirtualMachineProfileImpl(vmFinal, template, serviceOffering, null, null);
+ Long rootDiskSize = rootDiskOfferingInfo.getSize();
+ if (vm.getType().isUsedBySystem() && SystemVmRootDiskSize.value() !=
null && SystemVmRootDiskSize.value() > 0L) {
Review comment:
I think `SystemVmRootDiskSize.value()` will always give a non-null
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -405,6 +405,10 @@
static final ConfigKey<Boolean> HaVmRestartHostUp = new
ConfigKey<Boolean>("Advanced", Boolean.class, "ha.vm.restart.hostup", "true",
"If an out-of-band stop of a VM is detected and its host is up,
then power on the VM", true);
+ static final ConfigKey<Long> SystemVmRootDiskSize = new
ConfigKey<Long>("Advanced",
+ Long.class, "systemvm.root.disk.size", "-1",
+ "root size (in GB) of systemvm and virtual routers", true);
Review comment:
```suggestion
"Size of root volume (in GB) of system VMs and virtual routers",
true);
```
##########
File path:
engine/schema/src/main/java/com/cloud/upgrade/SystemVmTemplateRegistration.java
##########
@@ -0,0 +1,759 @@
+// 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 com.cloud.upgrade;
+
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.storage.Storage.ImageFormat;
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.Pair;
+import com.cloud.utils.UriUtils;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.script.Script;
+import org.apache.log4j.Logger;
+import org.ini4j.Ini;
+
+import javax.naming.ConfigurationException;
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileReader;
+import java.io.IOException;
+import java.net.URI;
+import java.security.MessageDigest;
+import java.sql.Connection;
+import java.sql.Date;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.UUID;
+import java.util.stream.Collectors;
+
+public class SystemVmTemplateRegistration {
+ private static final Logger LOGGER =
Logger.getLogger(SystemVmTemplateRegistration.class);
+ private static final String mountCommand = "sudo mount -t nfs %s %s";
+ private static final String umountCommand = "sudo umount %s";
+ private static final String hashAlgorithm = "MD5";
+ private static final String relativeTemplatePath =
"./engine/schema/dist/systemvm-templates/";
+ private static final String AbsolutetemplatesPath =
"/usr/share/cloudstack-management/templates/";
+ private static final String templatesPath = fetchTemplatesPath();
+ private static final String metadataFileName = "metadata.ini";
+ private static final String metadataFile = templatesPath +
metadataFileName;
+ private static final String TEMPORARY_SECONDARY_STORE =
"/tmp/tmpSecStorage";
+ private static final String PARENT_TEMPLATE_FOLDER =
TEMPORARY_SECONDARY_STORE;
+ private static final String PARTIAL_TEMPLATE_FOLDER = "/template/tmpl/1/";
Review comment:
Why different naming scheme for different variables with same access?
##########
File path:
engine/schema/src/main/java/com/cloud/upgrade/SystemVmTemplateRegistration.java
##########
@@ -0,0 +1,759 @@
+// 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 com.cloud.upgrade;
+
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.storage.Storage.ImageFormat;
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.Pair;
+import com.cloud.utils.UriUtils;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.script.Script;
+import org.apache.log4j.Logger;
+import org.ini4j.Ini;
+
+import javax.naming.ConfigurationException;
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileReader;
+import java.io.IOException;
+import java.net.URI;
+import java.security.MessageDigest;
+import java.sql.Connection;
+import java.sql.Date;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.UUID;
+import java.util.stream.Collectors;
+
+public class SystemVmTemplateRegistration {
+ private static final Logger LOGGER =
Logger.getLogger(SystemVmTemplateRegistration.class);
+ private static final String mountCommand = "sudo mount -t nfs %s %s";
+ private static final String umountCommand = "sudo umount %s";
+ private static final String hashAlgorithm = "MD5";
Review comment:
Not good for people with OCD :-D
```suggestion
private static final String hashAlgorithm = "MD5";
```
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -1667,6 +1667,7 @@ public boolean passCmdLine(final String vmName, final
String cmdLine) throws Int
command.add("-n", vmName);
command.add("-c", cmdLine);
result = command.execute();
+
Review comment:
probably not needed
##########
File path:
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterActionWorker.java
##########
@@ -310,6 +333,7 @@ protected String getControlVmPrivateIp() {
}
protected void attachIsoKubernetesVMs(List<UserVm> clusterVMs, final
KubernetesSupportedVersion kubernetesSupportedVersion) throws
CloudRuntimeException {
+ //final long startTimeoutTime = System.currentTimeMillis() +
KubernetesClusterService.KubernetesClusterStartTimeout.value() * 1000;
Review comment:
needed?
##########
File path:
engine/schema/src/main/java/com/cloud/upgrade/SystemVmTemplateRegistration.java
##########
@@ -0,0 +1,759 @@
+// 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 com.cloud.upgrade;
+
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.storage.Storage.ImageFormat;
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.Pair;
+import com.cloud.utils.UriUtils;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.script.Script;
+import org.apache.log4j.Logger;
+import org.ini4j.Ini;
+
+import javax.naming.ConfigurationException;
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileReader;
+import java.io.IOException;
+import java.net.URI;
+import java.security.MessageDigest;
+import java.sql.Connection;
+import java.sql.Date;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.UUID;
+import java.util.stream.Collectors;
+
+public class SystemVmTemplateRegistration {
+ private static final Logger LOGGER =
Logger.getLogger(SystemVmTemplateRegistration.class);
+ private static final String mountCommand = "sudo mount -t nfs %s %s";
+ private static final String umountCommand = "sudo umount %s";
+ private static final String hashAlgorithm = "MD5";
+ private static final String relativeTemplatePath =
"./engine/schema/dist/systemvm-templates/";
+ private static final String AbsolutetemplatesPath =
"/usr/share/cloudstack-management/templates/";
+ private static final String templatesPath = fetchTemplatesPath();
+ private static final String metadataFileName = "metadata.ini";
+ private static final String metadataFile = templatesPath +
metadataFileName;
+ private static final String TEMPORARY_SECONDARY_STORE =
"/tmp/tmpSecStorage";
+ private static final String PARENT_TEMPLATE_FOLDER =
TEMPORARY_SECONDARY_STORE;
+ private static final String PARTIAL_TEMPLATE_FOLDER = "/template/tmpl/1/";
+ private static final String FETCH_DISTINCT_ELIGIBLE_ZONES = "SELECT
DISTINCT(data_center_id) FROM `cloud`.`image_store` WHERE protocol = \"nfs\"
AND role = \"Image\" AND removed is null";
+ private static final String FETCH_DISTINCT_HYPERVISORS_IN_ZONE = "SELECT
DISTINCT(hypervisor_type) FROM `cloud`.`cluster` where removed is null AND
data_center_id=?";
+ private static final String FETCH_IMAGE_STORE_PER_ZONE = "SELECT url,id
FROM `cloud`.`image_store` WHERE data_center_id=? AND role = \"Image\" AND
image_provider_name = \"NFS\" AND removed IS NULL LIMIT 1";
+ private static final String INSERT_VM_TEMPLATE_TABLE = "INSERT INTO
`cloud`.`vm_template` (uuid, unique_name, name, public, featured, created,
type, hvm, bits, account_id, url, checksum, enable_password, display_text,
format, guest_os_id, cross_zones, hypervisor_type, state, deploy_as_is)" +
+ "VALUES (?, ?, ?, 0, 0, ?, 'SYSTEM', 0, 64, 1, ?, ?, 0, ?, ?, ?, 1, ?,
'Inactive', ?)";
+ private static final String INSERT_TEMPLATE_STORE_REF_TABLE = "INSERT INTO
`cloud`.`template_store_ref` (store_id, template_id, created, last_updated,
job_id, download_pct, download_state, error_str, local_path, install_path, url,
state, destroyed, is_copy," +
+ " update_count, ref_cnt, store_role) VALUES (?, ?, ?, ?, NULL, 0,
'NOT_DOWNLOADED', NULL, NULL, ?, ?, 'Allocated', 0, 0, 0, 0, 'Image')";
+ private static final String UPDATE_TEMPLATE_STORE_REF_TABLE = "UPDATE
`cloud`.`template_store_ref` SET download_pct=100, download_state='DOWNLOADED',
" +
+ "state='Ready', size=?, physical_size=?, last_updated=?, updated=?
where template_id=?";
+ private static final String UPDATE_VM_TEMPLATE_ENTRY = "UPDATE
`cloud`.`vm_template` set size = ?, state = 'Active' where id = ?";
+ private static final String UPDATE_CONFIGURATION_TABLE = "UPDATE
`cloud`.`configuration` SET value = ? WHERE name = ?";
+ private static final String UPDATE_TEMPLATE_TABLE_ON_FAILURE = "UPDATE
`cloud`.`vm_template` set removed = ?, state = 'Inactive' where id = ?";
+ private static final String DELETE_TEMPLATE_REF_RECORD_ON_FAILURE =
"DELETE from `cloud`.`template_store_ref` where template_id = ?";
+ private static final Integer SCRIPT_TIMEOUT = 1800000;
+ private static final Integer LOCK_WAIT_TIMEOUT = 1200;
+ public static String CS_MAJOR_VERSION = "4.16";
Review comment:
We will update this default value for every version in future?
##########
File path:
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterVO.java
##########
@@ -333,6 +369,16 @@ public KubernetesClusterVO(String name, String
description, long zoneId, long ku
this.checkForGc = false;
}
+ public KubernetesClusterVO(String name, String description, long zoneId,
long kubernetesVersionId, long serviceOfferingId, long templateId,
Review comment:
Is this used anywhere? Can't find its usage in my IDE
##########
File path:
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java
##########
@@ -864,30 +807,92 @@ private void
validateKubernetesClusterScaleParameters(ScaleKubernetesClusterCmd
final Long kubernetesClusterId = cmd.getId();
final Long serviceOfferingId = cmd.getServiceOfferingId();
final Long clusterSize = cmd.getClusterSize();
+ final List<Long> nodeIds = cmd.getNodeIds();
+ final Boolean isAutoscalingEnabled = cmd.isAutoscalingEnabled();
+ final Long minSize = cmd.getMinSize();
+ final Long maxSize = cmd.getMaxSize();
+
if (kubernetesClusterId == null || kubernetesClusterId < 1L) {
throw new InvalidParameterValueException("Invalid Kubernetes
cluster ID");
}
+
KubernetesClusterVO kubernetesCluster =
kubernetesClusterDao.findById(kubernetesClusterId);
if (kubernetesCluster == null || kubernetesCluster.getRemoved() !=
null) {
throw new InvalidParameterValueException("Invalid Kubernetes
cluster ID");
}
+
final DataCenter zone =
dataCenterDao.findById(kubernetesCluster.getZoneId());
if (zone == null) {
logAndThrow(Level.WARN, String.format("Unable to find zone for
Kubernetes cluster : %s", kubernetesCluster.getName()));
}
+ if (serviceOfferingId == null && clusterSize == null && nodeIds ==
null && isAutoscalingEnabled == null) {
+ throw new InvalidParameterValueException(String.format("Kubernetes
cluster %s cannot be scaled, either service offering or cluster size or nodeids
to be removed or autoscaling must be passed", kubernetesCluster.getName()));
+ }
+
Account caller = CallContext.current().getCallingAccount();
accountManager.checkAccess(caller,
SecurityChecker.AccessType.OperateEntry, false, kubernetesCluster);
- if (serviceOfferingId == null && clusterSize == null) {
- throw new InvalidParameterValueException(String.format("Kubernetes
cluster : %s cannot be scaled, either a new service offering or a new cluster
size must be passed", kubernetesCluster.getName()));
- }
-
final KubernetesSupportedVersion clusterVersion =
kubernetesSupportedVersionDao.findById(kubernetesCluster.getKubernetesVersionId());
if (clusterVersion == null) {
throw new CloudRuntimeException(String.format("Invalid Kubernetes
version associated with Kubernetes cluster : %s", kubernetesCluster.getName()));
}
+ if
(!(kubernetesCluster.getState().equals(KubernetesCluster.State.Created) ||
+
kubernetesCluster.getState().equals(KubernetesCluster.State.Running) ||
+
kubernetesCluster.getState().equals(KubernetesCluster.State.Stopped))) {
Review comment:
Use list `'contains`?
##########
File path: api/src/main/java/com/cloud/vm/UserVmService.java
##########
@@ -379,7 +379,7 @@ UserVm createAdvancedVirtualMachine(DataCenter zone,
ServiceOffering serviceOffe
String hostName, String displayName, Long diskOfferingId, Long
diskSize, String group, HypervisorType hypervisor, HTTPMethod httpmethod,
String userData,
String sshKeyPair, Map<Long, IpAddresses> requestedIps, IpAddresses
defaultIps, Boolean displayVm, String keyboard, List<Long> affinityGroupIdList,
Map<String, String> customParameters, String customId, Map<String,
Map<Integer, String>> dhcpOptionMap, Map<Long, DiskOffering>
dataDiskTemplateToDiskOfferingMap,
- Map<String, String> templateOvfPropertiesMap, boolean
dynamicScalingEnabled)
+ Map<String, String> templateOvfPropertiesMap, boolean
dynamicScalingEnabled, String type)
Review comment:
Can `type` be an enum value?
##########
File path:
plugins/integrations/kubernetes-service/src/main/java/org/apache/cloudstack/api/command/user/kubernetes/cluster/CreateKubernetesClusterCmd.java
##########
@@ -55,6 +57,7 @@
public class CreateKubernetesClusterCmd extends BaseAsyncCreateCmd {
public static final Logger LOGGER =
Logger.getLogger(CreateKubernetesClusterCmd.class.getName());
public static final String APINAME = "createKubernetesCluster";
+ private static final Long DEFAULT_NODE_ROOT_DISK_SIZE = 8L;
Review comment:
Is this some tested value?
##########
File path:
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java
##########
@@ -261,60 +269,6 @@ private void logAndThrow(final Level logLevel, final
String message, final Excep
logTransitStateAndThrow(logLevel, message, null, null, ex);
}
- private boolean isKubernetesServiceTemplateConfigured(DataCenter zone) {
- // Check Kubernetes VM template for zone
- boolean isHyperVAvailable = false;
- boolean isKVMAvailable = false;
- boolean isVMwareAvailable = false;
- boolean isXenserverAvailable = false;
- List<ClusterVO> clusters = clusterDao.listByZoneId(zone.getId());
- for (ClusterVO clusterVO : clusters) {
- if
(Hypervisor.HypervisorType.Hyperv.equals(clusterVO.getHypervisorType())) {
- isHyperVAvailable = true;
- }
- if
(Hypervisor.HypervisorType.KVM.equals(clusterVO.getHypervisorType())) {
- isKVMAvailable = true;
- }
- if
(Hypervisor.HypervisorType.VMware.equals(clusterVO.getHypervisorType())) {
- isVMwareAvailable = true;
- }
- if
(Hypervisor.HypervisorType.XenServer.equals(clusterVO.getHypervisorType())) {
- isXenserverAvailable = true;
- }
- }
- List<Pair<String, String>> templatePairs = new ArrayList<>();
- if (isHyperVAvailable) {
- templatePairs.add(new
Pair<>(KubernetesClusterHyperVTemplateName.key(),
KubernetesClusterHyperVTemplateName.value()));
- }
- if (isKVMAvailable) {
Review comment:
@Pearl1594 a follow up question on this - I've a running k8s cluster. I
upgrade ACS to 4.16. I scale-up my k8s cluster. Now there will be mixed node
VMs (with coreos and systemvm templates). Now when I try to upgrade k8s version
on my k8s cluster will there be an issue? As user on these VMs may be different
`core` and `root`.
##########
File path:
engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/ImageStoreVO.java
##########
@@ -135,6 +135,10 @@ public Long getDataCenterId() {
return this.dcId;
}
+ public Long getDcId() {
+ return this.dcId;
+ }
+
Review comment:
Using `getDataCenterId` won't work?
##########
File path:
plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterActionWorker.java
##########
@@ -203,11 +218,19 @@ protected void logTransitStateDetachIsoAndThrow(final
Level logLevel, final Stri
throw new CloudRuntimeException(message, e);
}
+ protected void deleteTemplateLaunchPermission() {
+ if (clusterTemplate != null && owner != null) {
+ LOGGER.info("Revoking launch permission for systemVM template");
+ launchPermissionDao.removePermissions(clusterTemplate.getId(),
Collections.singletonList(owner.getId()));
+ }
+ }
+
protected void logTransitStateAndThrow(final Level logLevel, final String
message, final Long kubernetesClusterId, final KubernetesCluster.Event event,
final Exception e) throws CloudRuntimeException {
logMessage(logLevel, message, e);
if (kubernetesClusterId != null && event != null) {
stateTransitTo(kubernetesClusterId, event);
}
+ deleteTemplateLaunchPermission();
Review comment:
Is it possible to call it when needed? Not sure but as I can understand
this might be called everytime there is an issue the k8s cluster such as
upgrade/start (from stopped state) as well. Revoking permission log can be
confusing in those cases. Though not a major issue
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
