rohityadavcloud commented on a change in pull request #5831: URL: https://github.com/apache/cloudstack/pull/5831#discussion_r810926646
########## File path: api/src/main/java/org/apache/cloudstack/api/command/admin/systemvm/PatchSystemVMCmd.java ########## @@ -0,0 +1,108 @@ +// 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.admin.systemvm; + +import com.cloud.event.EventTypes; +import com.cloud.user.Account; +import com.cloud.utils.Pair; +import com.cloud.vm.VirtualMachine; +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.BaseAsyncCmd; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.SuccessResponse; +import org.apache.cloudstack.api.response.SystemVmResponse; +import org.apache.cloudstack.context.CallContext; +import org.apache.log4j.Logger; + +@APICommand(name = PatchSystemVMCmd.APINAME, description = "Attempts to live patch systemVMs - CPVM, SSVM ", + responseObject = SuccessResponse.class, requestHasSensitiveInfo = false, + responseHasSensitiveInfo = false, authorized = { RoleType.Admin }) Review comment: Add since ########## File path: api/src/main/java/org/apache/cloudstack/api/command/admin/systemvm/PatchSystemVMCmd.java ########## @@ -0,0 +1,108 @@ +// 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.admin.systemvm; + +import com.cloud.event.EventTypes; +import com.cloud.user.Account; +import com.cloud.utils.Pair; +import com.cloud.vm.VirtualMachine; +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.BaseAsyncCmd; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.SuccessResponse; +import org.apache.cloudstack.api.response.SystemVmResponse; +import org.apache.cloudstack.context.CallContext; +import org.apache.log4j.Logger; + +@APICommand(name = PatchSystemVMCmd.APINAME, description = "Attempts to live patch systemVMs - CPVM, SSVM ", + responseObject = SuccessResponse.class, requestHasSensitiveInfo = false, + responseHasSensitiveInfo = false, authorized = { RoleType.Admin }) +public class PatchSystemVMCmd extends BaseAsyncCmd { + public static final Logger s_logger = Logger.getLogger(PatchSystemVMCmd.class.getName()); + public static final String APINAME = "PatchSystemVM"; Review comment: minor nit - the API name starts with lowercase (see other APIs) for example following convention used by other vms - `patchSystemVm` ########## File path: core/src/main/java/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java ########## @@ -173,12 +174,33 @@ private Answer execute(final SetupKeyStoreCommand cmd) { return new SetupKeystoreAnswer(result.getDetails()); } + private void scpFileToIdentifyPatching(String routerIp, String path, String filename, String content) throws InterruptedException { + String errMsg = "Failed to scp file: %s to system VM"; + for (int retries = 15; retries > 0; retries--) { + try { + _vrDeployer.createFileInVR(routerIp, path, filename, content); + } catch (Exception e) { + errMsg += ", retrying"; + s_logger.error(String.format(errMsg, filename), e); + Thread.sleep(2000); + } + } + } private Answer execute(final SetupCertificateCommand cmd) { - final String args = String.format("/usr/local/cloud/systemvm/conf/agent.properties " + + String routerName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME); Review comment: I see this isn't really router name, but name of the systemvm right? ########## File path: debian/cloudstack-common.install ########## @@ -15,7 +15,6 @@ # specific language governing permissions and limitations # under the License. -/usr/share/cloudstack-common/vms/systemvm.iso Review comment: yay! ########## File path: plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ########## @@ -226,6 +228,8 @@ public String toString() { protected static final HashMap<VmPowerState, PowerState> s_powerStatesTable; public static final String XS_TOOLS_ISO_AFTER_70 = "guest-tools.iso"; + public static final String BASEPATH = "/opt/xensource/packages/resources/"; Review comment: we might need to check/test if the path is the same across XS7.1 to xcp 8.2. ########## File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPatchSystemVmCommandWrapper.java ########## @@ -0,0 +1,113 @@ +// 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.hypervisor.kvm.resource.wrapper; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.PatchSystemVmAnswer; +import com.cloud.agent.api.PatchSystemVmCommand; +import com.cloud.agent.api.routing.NetworkElementCommand; +import com.cloud.agent.resource.virtualnetwork.VRScripts; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; +import com.cloud.utils.ExecutionResult; +import com.cloud.utils.FileUtil; +import com.cloud.utils.Pair; +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.utils.ssh.SshHelper; +import org.apache.commons.lang3.StringUtils; +import org.apache.log4j.Logger; + +import java.io.File; + +@ResourceWrapper(handles = PatchSystemVmCommand.class) +public class LibvirtPatchSystemVmCommandWrapper extends CommandWrapper<PatchSystemVmCommand, Answer, LibvirtComputingResource> { + private static final Logger s_logger = Logger.getLogger(LibvirtPatchSystemVmCommandWrapper.class); + private static int sshPort = Integer.parseInt(LibvirtComputingResource.DEFAULTDOMRSSHPORT); + private static File pemFile = new File(LibvirtComputingResource.SSHPRVKEYPATH); + + @Override + public Answer execute(PatchSystemVmCommand cmd, LibvirtComputingResource serverResource) { + final String controlIp = cmd.getAccessDetail(NetworkElementCommand.ROUTER_IP); + final String sysVMName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME); + ExecutionResult result; + try { + result = getSystemVmVersionAndChecksum(serverResource, controlIp); + } catch (CloudRuntimeException e) { + return new PatchSystemVmAnswer(cmd, e.getMessage()); + } + + final String[] lines = result.getDetails().split("&"); + // TODO: do we fail, or patch anyway?? + if (lines.length != 2) { + return new PatchSystemVmAnswer(cmd, result.getDetails()); + } + + String scriptChecksum = lines[1].trim(); + String checksum = serverResource.calculateCurrentChecksum(sysVMName, "vms/cloud-scripts.tgz").trim(); + + if (!StringUtils.isEmpty(checksum) && checksum.equals(scriptChecksum)) { + if (!cmd.isForced()) { + String msg = String.format("No change in the scripts checksum, not patching systemVM %s", sysVMName); + s_logger.info(msg); + return new PatchSystemVmAnswer(cmd, msg, lines[0], lines[1]); + } + } + + Pair<Boolean, String> patchResult = null; + try { + FileUtil.scpPatchFiles(controlIp, "/home/cloud", sshPort, pemFile, serverResource.systemVmPatchFiles, LibvirtComputingResource.BASEPATH); Review comment: nit - should we be copying inside the VR to /var/cache/cloud or /root (but not /home/cloud?) or even a random folder in /tmp? ########## File path: api/src/main/java/org/apache/cloudstack/api/command/user/network/RestartNetworkCmd.java ########## @@ -59,6 +59,9 @@ @Parameter(name = ApiConstants.MAKEREDUNDANT, type = CommandType.BOOLEAN, required = false, description = "Turn the network into a network with redundant routers.", since = "4.11.1") private Boolean makeRedundant = false; + @Parameter(name = ApiConstants.LIVE_PATCH, type = CommandType.BOOLEAN, required = false, description = "Live Patch the router before restarting it. This parameter will work only when 'cleanup' is false.", since = "4.16.1") Review comment: Can you explain it, since this will be consumed by users, for example: ``` Live patches the router software without restarting the network virtual router. This parameter will work only when 'cleanup' is false." ``` ** also check if you meant `without restarting the network VR`? Fix since version also to 4.17. ########## File path: systemvm/debian/etc/systemd/system/cloud-postinit.service ########## @@ -1,7 +1,7 @@ [Unit] Description=CloudStack post-patching init script After=cloud-early-config.service network.target local-fs.target -Before=ssh.service +#Before=ssh.service Review comment: remove commented code if not used, this may be useful if the systemvmtemplate is already say patched once and sshd is already enabled - pl check. ########## File path: systemvm/debian/opt/cloud/bin/setup/init.sh ########## @@ -0,0 +1,217 @@ +#!/bin/bash +# 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. + +set -x +PATH="/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin" +CMDLINE=/var/cache/cloud/cmdline + +hypervisor() { + if [ -d /proc/xen ]; then Review comment: ((nevermind previous comment - you've moved the code here)) ########## File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ########## @@ -2903,14 +2886,12 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { } if (vmSpec.getType() != VirtualMachine.Type.User) { - if (_sysvmISOPath != null) { - final DiskDef iso = new DiskDef(); - iso.defISODisk(_sysvmISOPath); - if (_guestCpuArch != null && _guestCpuArch.equals("aarch64")) { - iso.setBusType(DiskDef.DiskBus.SCSI); - } - vm.getDevices().addDevice(iso); + final DiskDef iso = new DiskDef(); + iso.defISODisk(_sysvmISOPath); Review comment: @Pearl1594 shouldn't this be removed as we know _sysvmISOPath is null? ########## File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ########## @@ -2903,14 +2886,12 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { } if (vmSpec.getType() != VirtualMachine.Type.User) { - if (_sysvmISOPath != null) { - final DiskDef iso = new DiskDef(); - iso.defISODisk(_sysvmISOPath); - if (_guestCpuArch != null && _guestCpuArch.equals("aarch64")) { - iso.setBusType(DiskDef.DiskBus.SCSI); - } - vm.getDevices().addDevice(iso); + final DiskDef iso = new DiskDef(); + iso.defISODisk(_sysvmISOPath); Review comment: or, not set the path? ########## File path: api/src/main/java/org/apache/cloudstack/api/command/user/vpc/RestartVPCCmd.java ########## @@ -54,6 +54,9 @@ @Parameter(name = ApiConstants.MAKEREDUNDANT, type = CommandType.BOOLEAN, required = false, description = "Turn a single VPC into a redundant one.") private Boolean makeredundant = false; + @Parameter(name = ApiConstants.LIVE_PATCH, type = CommandType.BOOLEAN, required = false, description = "Live Patch the router before restarting it", since = "4.16.1") + private Boolean livePatch = false; Review comment: Same comment as above, see comment for restart network cmd. ########## File path: core/src/main/java/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java ########## @@ -173,12 +174,33 @@ private Answer execute(final SetupKeyStoreCommand cmd) { return new SetupKeystoreAnswer(result.getDetails()); } + private void scpFileToIdentifyPatching(String routerIp, String path, String filename, String content) throws InterruptedException { + String errMsg = "Failed to scp file: %s to system VM"; + for (int retries = 15; retries > 0; retries--) { + try { + _vrDeployer.createFileInVR(routerIp, path, filename, content); Review comment: Should you try to break this, otherwise it would try to scp 15 times? ########## File path: core/src/main/java/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java ########## @@ -173,12 +174,33 @@ private Answer execute(final SetupKeyStoreCommand cmd) { return new SetupKeystoreAnswer(result.getDetails()); } + private void scpFileToIdentifyPatching(String routerIp, String path, String filename, String content) throws InterruptedException { + String errMsg = "Failed to scp file: %s to system VM"; + for (int retries = 15; retries > 0; retries--) { Review comment: nit picking - Any reason for this magic number 15? ########## File path: engine/schema/pom.xml ########## @@ -181,7 +182,8 @@ </goals> <configuration> <checkSignature>true</checkSignature> - <url>https://download.cloudstack.org/systemvm/${cs.version}/systemvmtemplate-${cs.version}.${patch.version}-kvm.qcow2.bz2</url> +<!-- <url>https://download.cloudstack.org/systemvm/${cs.version}/systemvmtemplate-${cs.version}.${patch.version}-kvm.qcow2.bz2</url>--> Review comment: same as above ########## File path: core/src/main/java/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java ########## @@ -173,12 +174,33 @@ private Answer execute(final SetupKeyStoreCommand cmd) { return new SetupKeystoreAnswer(result.getDetails()); } + private void scpFileToIdentifyPatching(String routerIp, String path, String filename, String content) throws InterruptedException { Review comment: nit - maybe rename to a general utility method name - basically you're trying to copy and not giving up on the first fail? ########## File path: core/src/main/java/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java ########## @@ -173,12 +174,33 @@ private Answer execute(final SetupKeyStoreCommand cmd) { return new SetupKeystoreAnswer(result.getDetails()); } + private void scpFileToIdentifyPatching(String routerIp, String path, String filename, String content) throws InterruptedException { + String errMsg = "Failed to scp file: %s to system VM"; + for (int retries = 15; retries > 0; retries--) { + try { + _vrDeployer.createFileInVR(routerIp, path, filename, content); + } catch (Exception e) { + errMsg += ", retrying"; + s_logger.error(String.format(errMsg, filename), e); + Thread.sleep(2000); + } + } + } private Answer execute(final SetupCertificateCommand cmd) { - final String args = String.format("/usr/local/cloud/systemvm/conf/agent.properties " + + String routerName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME); + if (!org.apache.commons.lang3.StringUtils.isEmpty(routerName) && (routerName.startsWith("s-") || routerName.startsWith("v-"))) { + try { + scpFileToIdentifyPatching(cmd.getRouterAccessIp(), "/usr/local/cloud/systemvm/conf/", KeyStoreUtils.CERT_FILENAME, cmd.getCertificate()); Review comment: tangent idea - I was thinking would it be even simpler to create the `KeyStore` in memory by the mgmt server, convert that to bytes/base64-encoded data that is then serialised/saved directly in the systemvm without the hassel of import scripts in systemvm? ########## File path: engine/schema/pom.xml ########## @@ -122,7 +122,8 @@ <goal>wget</goal> </goals> <configuration> - <url>https://download.cloudstack.org/systemvm/${cs.version}/md5sum.txt</url> +<!-- <url>https://download.cloudstack.org/systemvm/${cs.version}/md5sum.txt</url>--> Review comment: remove comment code that isn't used ########## File path: debian/rules ########## @@ -128,7 +129,7 @@ override_dh_auto_install: install -D client/target/utilities/bin/cloud-setup-management $(DESTDIR)/usr/bin/cloudstack-setup-management install -D client/target/utilities/bin/cloud-setup-encryption $(DESTDIR)/usr/bin/cloudstack-setup-encryption install -D client/target/utilities/bin/cloud-sysvmadm $(DESTDIR)/usr/bin/cloudstack-sysvmadm - install -D systemvm/dist/systemvm.iso $(DESTDIR)/usr/share/$(PACKAGE)-common/vms/systemvm.iso + install -D systemvm/dist/* $(DESTDIR)/usr/share/$(PACKAGE)-common/vms/ Review comment: @Pearl1594 do you know what's in dist/* (the systemvmtemplates?) ########## File path: engine/schema/pom.xml ########## @@ -193,7 +195,8 @@ </goals> <configuration> <checkSignature>true</checkSignature> - <url>https://download.cloudstack.org/systemvm/${cs.version}/systemvmtemplate-${cs.version}.${patch.version}-vmware.ova</url> +<!-- <url>https://download.cloudstack.org/systemvm/${cs.version}/systemvmtemplate-${cs.version}.${patch.version}-vmware.ova</url>--> Review comment: same as above ########## File path: client/pom.xml ########## @@ -878,32 +878,6 @@ </dependency> </dependencies> <build> - <plugins> - <plugin> - <artifactId>maven-antrun-plugin</artifactId> - <version>1.7</version> - <executions> - <!-- Copy the systemvm in the package phase as it is generated by console-proxy in the package - phase. --> - <execution> - <id>copy-systemvm</id> - <phase>process-resources</phase> - <goals> - <goal>run</goal> - </goals> - <configuration> - <target> - <copy todir="${basedir}/target/common/vms"> Review comment: :D so good to see old/archaic code gone ########## File path: core/src/main/java/com/cloud/resource/ServerResource.java ########## @@ -26,12 +26,21 @@ import com.cloud.agent.api.StartupCommand; import com.cloud.host.Host; import com.cloud.utils.component.Manager; +import org.apache.cloudstack.utils.security.KeyStoreUtils; + +import java.io.File; /** * ServerResource is a generic container to execute commands sent */ public interface ServerResource extends Manager { + String[] systemVmPatchFiles = new String[] { "agent.zip", "cloud-scripts.tgz", "patch-sysvms.sh" }; + String[] certificateFiles = new String[] {KeyStoreUtils.CERT_FILENAME, KeyStoreUtils.CACERT_FILENAME, KeyStoreUtils.PKEY_FILENAME}; + + String SSHKEYSPATH = "/root/.ssh"; + String SSHPRVKEYPATH = SSHKEYSPATH + File.separator + "id_rsa.cloud"; Review comment: If this is for the VR, then we can hardcode the full path (this is because File.separator is different on differnt jvm/platforms, but the target platform in linux/VR). ########## File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ########## @@ -279,6 +277,7 @@ private static final String AARCH64 = "aarch64"; public static final String RESIZE_NOTIFY_ONLY = "NOTIFYONLY"; + public static final String BASEPATH = "/usr/share/cloudstack-common/vms/"; Review comment: just wondering here - if there's a way to detect this (see agent.properties or env.properties?) ########## File path: core/src/main/java/com/cloud/resource/ServerResourceBase.java ########## @@ -306,4 +309,13 @@ public boolean start() { public boolean stop() { return true; } + + public String calculateCurrentChecksum(String name, String path) { + String cloudScriptsPath = Script.findScript("", path); Review comment: Is there a name of script/file that should be passed here? ########## File path: engine/schema/pom.xml ########## @@ -205,7 +208,8 @@ </goals> <configuration> <checkSignature>true</checkSignature> - <url>https://download.cloudstack.org/systemvm/${cs.version}/systemvmtemplate-${cs.version}.${patch.version}-xen.vhd.bz2</url> +<!-- <url>https://download.cloudstack.org/systemvm/${cs.version}/systemvmtemplate-${cs.version}.${patch.version}-xen.vhd.bz2</url>--> Review comment: same as above ########## File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartCommandWrapper.java ########## @@ -115,6 +117,16 @@ public Answer execute(final StartCommand command, final LibvirtComputingResource break; } } + + try { + File pemFile = new File(LibvirtComputingResource.SSHPRVKEYPATH); + FileUtil.scpPatchFiles(controlIp, "/home/cloud", Integer.parseInt(LibvirtComputingResource.DEFAULTDOMRSSHPORT), pemFile, LibvirtComputingResource.systemVmPatchFiles, LibvirtComputingResource.BASEPATH); Review comment: same as above - (we shouldn't assume that cloud user exists, or use /tmp or privileged /root/patch/...) ########## File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPatchSystemVmCommandWrapper.java ########## @@ -0,0 +1,113 @@ +// 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.hypervisor.kvm.resource.wrapper; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.PatchSystemVmAnswer; +import com.cloud.agent.api.PatchSystemVmCommand; +import com.cloud.agent.api.routing.NetworkElementCommand; +import com.cloud.agent.resource.virtualnetwork.VRScripts; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; +import com.cloud.utils.ExecutionResult; +import com.cloud.utils.FileUtil; +import com.cloud.utils.Pair; +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.utils.ssh.SshHelper; +import org.apache.commons.lang3.StringUtils; +import org.apache.log4j.Logger; + +import java.io.File; + +@ResourceWrapper(handles = PatchSystemVmCommand.class) +public class LibvirtPatchSystemVmCommandWrapper extends CommandWrapper<PatchSystemVmCommand, Answer, LibvirtComputingResource> { + private static final Logger s_logger = Logger.getLogger(LibvirtPatchSystemVmCommandWrapper.class); + private static int sshPort = Integer.parseInt(LibvirtComputingResource.DEFAULTDOMRSSHPORT); + private static File pemFile = new File(LibvirtComputingResource.SSHPRVKEYPATH); + + @Override + public Answer execute(PatchSystemVmCommand cmd, LibvirtComputingResource serverResource) { + final String controlIp = cmd.getAccessDetail(NetworkElementCommand.ROUTER_IP); + final String sysVMName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME); + ExecutionResult result; + try { + result = getSystemVmVersionAndChecksum(serverResource, controlIp); + } catch (CloudRuntimeException e) { + return new PatchSystemVmAnswer(cmd, e.getMessage()); + } + + final String[] lines = result.getDetails().split("&"); + // TODO: do we fail, or patch anyway?? Review comment: check/fix or remove TODO ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -2123,7 +2203,6 @@ protected StartAnswer execute(StartCommand cmd) { String msg = "secondary storage for dc " + _dcId + " is not ready yet?"; throw new Exception(msg); } - mgr.prepareSecondaryStorageStore(secStoreUrl, secStoreId); Review comment: yay! no more check/inject keys ########## File path: scripts/vm/systemvm/injectkeys.sh ########## @@ -24,14 +24,8 @@ set -e TMP=/tmp Review comment: @Pearl1594 should this file be completely removed if systemvm.iso isn't used? (unless this used for some other purposes too)? ########## File path: plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixPatchSystemVmCommandWrapper.java ########## @@ -0,0 +1,111 @@ +// 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.hypervisor.xenserver.resource.wrapper.xenbase; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.PatchSystemVmAnswer; +import com.cloud.agent.api.PatchSystemVmCommand; +import com.cloud.agent.api.routing.NetworkElementCommand; +import com.cloud.agent.resource.virtualnetwork.VRScripts; +import com.cloud.hypervisor.xenserver.resource.CitrixResourceBase; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; +import com.cloud.utils.ExecutionResult; +import com.cloud.utils.exception.CloudRuntimeException; +import com.xensource.xenapi.Connection; +import org.apache.commons.lang3.StringUtils; +import org.apache.log4j.Logger; + +import java.io.File; + +@ResourceWrapper(handles = PatchSystemVmCommand.class) +public class CitrixPatchSystemVmCommandWrapper extends CommandWrapper<PatchSystemVmCommand, Answer, CitrixResourceBase> { + private static final Logger s_logger = Logger.getLogger(CitrixPatchSystemVmCommandWrapper.class); + private static int sshPort = CitrixResourceBase.DEFAULTDOMRSSHPORT; + private static File pemFile = new File(CitrixResourceBase.SSHPRVKEYPATH); + + @Override + public Answer execute(PatchSystemVmCommand command, CitrixResourceBase serverResource) { + final String controlIp = command.getAccessDetail(NetworkElementCommand.ROUTER_IP); + final String sysVMName = command.getAccessDetail(NetworkElementCommand.ROUTER_NAME); + final Connection conn = serverResource.getConnection(); + + ExecutionResult result; + try { + result = getSystemVmVersionAndChecksum(serverResource, controlIp); + } catch (CloudRuntimeException e) { + return new PatchSystemVmAnswer(command, e.getMessage()); + } + + final String[] lines = result.getDetails().split("&"); + // TODO: do we fail, or patch anyway?? Review comment: suggestion - check/fix/remove/refactor. Suggestion - if there's some common code across all hypervisor, it might make sense to create a VR/patching framework utility/class that is reused across all of them. ########## File path: scripts/util/keystore-cert-import ########## @@ -17,19 +17,41 @@ # under the License. PROPS_FILE="$1" -KS_FILE="$2" -MODE="$3" -CERT_FILE="$4" -CERT=$(echo "$5" | tr '^' '\n' | tr '~' ' ') -CACERT_FILE="$6" -CACERT=$(echo "$7" | tr '^' '\n' | tr '~' ' ') -PRIVKEY_FILE="$8" -PRIVKEY=$(echo "$9" | tr '^' '\n' | tr '~' ' ') +KS_PASS="$2" +KS_FILE="$3" +MODE="$4" +CERT_FILE="$5" +CERT=$(echo "$6" | tr '^' '\n' | tr '~' ' ') +CACERT_FILE="$7" +CACERT=$(echo "$8" | tr '^' '\n' | tr '~' ' ') +PRIVKEY_FILE="$9" +PRIVKEY=$(echo "${10}" | tr '^' '\n' | tr '~' ' ') ALIAS="cloud" SYSTEM_FILE="/var/cache/cloud/cmdline" LIBVIRTD_FILE="/etc/libvirt/libvirtd.conf" +if [ ! -f "$LIBVIRTD_FILE" ]; then + # Re-use existing password or use the one provided + while [ ! -d /usr/local/cloud/systemvm/conf ]; do sleep 1; done + if [ -f "$PROPS_FILE" ]; then + OLD_PASS=$(sed -n '/keystore.passphrase/p' "$PROPS_FILE" 2>/dev/null | sed 's/keystore.passphrase=//g' 2>/dev/null) + if [ ! -z "${OLD_PASS// }" ]; then + KS_PASS="$OLD_PASS" + else + sed -i "/keystore.passphrase.*/d" $PROPS_FILE 2> /dev/null || true + echo "keystore.passphrase=$KS_PASS" >> $PROPS_FILE + fi + fi + +# if [ -f "$KS_FILE" ]; then Review comment: remove commented code if not used ########## File path: server/src/main/java/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ########## @@ -1273,7 +1284,8 @@ public boolean finalizeVirtualMachineProfile(VirtualMachineProfile profile, Depl if (dc.getDns2() != null) { buf.append(" dns2=").append(dc.getDns2()); } - + buf.append(" keystore_password=").append(VirtualMachineGuru.getEncodedString(PasswordGenerator.generateRandomPassword(16))); +// VirtualMachineGuru.appendCertificateDetails(buf, certificate); Review comment: remove commented code if not used ########## File path: services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java ########## @@ -1154,7 +1165,8 @@ public boolean finalizeVirtualMachineProfile(VirtualMachineProfile profile, Depl } String nfsVersion = imageStoreDetailsUtil != null ? imageStoreDetailsUtil.getNfsVersion(secStore.getId()) : null; buf.append(" nfsVersion=").append(nfsVersion); - + buf.append(" keystore_password=").append(VirtualMachineGuru.getEncodedString(PasswordGenerator.generateRandomPassword(16))); + //VirtualMachineGuru.appendCertificateDetails(buf, certificate); Review comment: remove commented code if not used ########## File path: services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java ########## @@ -109,6 +110,9 @@ private static void configProxy(Properties conf) { s_logger.info("Configure console proxy..."); for (Object key : conf.keySet()) { s_logger.info("Property " + (String)key + ": " + conf.getProperty((String)key)); +// if (!ArrayUtils.contains(skipProperties, key)) { Review comment: remove commented code if not used ########## File path: services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java ########## @@ -247,7 +251,10 @@ public static void startWithContext(Properties conf, Object context, byte[] ksBi if (conf != null) { for (Object key : conf.keySet()) { - s_logger.info("Context property " + (String)key + ": " + conf.getProperty((String)key)); +// if (!ArrayUtils.contains(skipProperties, key)) { + s_logger.info("Context property " + (String) key + ": " + conf.getProperty((String) key)); +// s_logger.info("Context property " + (String) key + ": " + conf.getProperty((String) key)); +// } Review comment: remove commented code if not used ########## File path: systemvm/debian/opt/cloud/bin/setup/bootstrap.sh ########## @@ -29,124 +29,6 @@ log_it() { log_action_msg "$@" } -hypervisor() { - if [ -d /proc/xen ]; then Review comment: great to see this removed, quick question @Pearl1594 - would that cause an issue - we don't want hypervisor specific logic to be kicked or was this moved? -- 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]
