nvazquez commented on a change in pull request #5831: URL: https://github.com/apache/cloudstack/pull/5831#discussion_r828718161
########## 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 }, since = "4.17.0") +public class PatchSystemVMCmd extends BaseAsyncCmd { + public static final Logger s_logger = Logger.getLogger(PatchSystemVMCmd.class.getName()); + public static final String APINAME = "patchSystemVm"; + + ///////////////////////////////////////////////////// + //////////////// API parameters ///////////////////// + ///////////////////////////////////////////////////// + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = SystemVmResponse.class, + description = "patches systemVM - CPVM/SSVM/Router with the specified ID") + private Long id; + + @Parameter(name = ApiConstants.FORCED, type = CommandType.BOOLEAN, + description = "If true, initiates copy of scripts and restart of the agent, even if the scripts version matches." + + "To be used with ID parameter only") + private Boolean force; + + ///////////////////////////////////////////////////// + /////////////////// Accessors /////////////////////// + ///////////////////////////////////////////////////// + + + public Long getId() { + return id; + } + + public boolean isForced() { + return force != null ? force : false; Review comment: ```suggestion return force != null && force; ``` ########## File path: core/src/main/java/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java ########## @@ -582,4 +584,23 @@ private Answer execute(AggregationControlCommand cmd) { } return new Answer(cmd, false, "Fail to recognize aggregation action " + action.toString()); } + + public boolean isSystemVMSetup(String vmName, String controlIp) throws InterruptedException { + if (vmName.startsWith("s-") || vmName.startsWith("v-")) { + ScriptConfigItem scriptConfigItem = new ScriptConfigItem(VRScripts.SYSTEM_VM_PATCHED, "/opt/cloud/bin/keystore*"); + ExecutionResult result = new ExecutionResult(false, ""); + int retries = 0; + while (!result.isSuccess() && retries < 600) { Review comment: Not saying its wrong but what's the logic behind the value 600? Should it be parametrized? ########## 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: Just a reminder on this, to be removed when ready ########## File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java ########## @@ -1192,8 +1192,12 @@ public void orchestrateStart(final String vmUuid, final Map<VirtualMachineProfil handlePath(vmTO.getDisks(), vm.getHypervisorType()); Commands cmds = new Commands(Command.OnError.Stop); + final Map<String, String> sshAccessDetails = _networkMgr.getSystemVMAccessDetails(vm); Review comment: Why not wrapping these on an if block? (when VM type = systemVM) ########## 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: +1 to this idea if could be implemented, but not mandatory ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -630,6 +637,79 @@ public Answer executeRequest(Command cmd) { return answer; } + private ExecutionResult getSystemVmVersionAndChecksum(String controlIp) { + ExecutionResult result; + try { + result = executeInVR(controlIp, VRScripts.VERSION, null); + if (!result.isSuccess()) { + String errMsg = String.format("GetSystemVMVersionCmd on %s failed, message %s", controlIp, result.getDetails()); + s_logger.error(errMsg); + throw new CloudRuntimeException(errMsg); + } + } catch (final Exception e) { + final String msg = "GetSystemVMVersionCmd failed due to " + e; + s_logger.error(msg, e); + throw new CloudRuntimeException(msg, e); + } + return result; + } + + private Answer execute(PatchSystemVmCommand cmd) { + String controlIp = cmd.getAccessDetail((NetworkElementCommand.ROUTER_IP)); + String sysVMName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME); + String homeDir = System.getProperty("user.home"); + File pemFile = new File(homeDir + "/.ssh/id_rsa"); + + ExecutionResult result = getSystemVmVersionAndChecksum(controlIp); + try { + FileUtil.scpPatchFiles(controlIp, "/tmp/", DefaultDomRSshPort, pemFile, systemVmPatchFiles, BASEPATH); + } 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 = calculateCurrentChecksum(sysVMName, "vms/cloud-scripts.tgz").trim(); + + if (!org.apache.commons.lang3.StringUtils.isEmpty(checksum) && checksum.equals(scriptChecksum)) { Review comment: ```suggestion if (!org.apache.commons.lang3.StringUtils.isEmpty(checksum) && checksum.equals(scriptChecksum) && !cmd.isForced()) { ``` ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -630,6 +637,79 @@ public Answer executeRequest(Command cmd) { return answer; } + private ExecutionResult getSystemVmVersionAndChecksum(String controlIp) { + ExecutionResult result; + try { + result = executeInVR(controlIp, VRScripts.VERSION, null); + if (!result.isSuccess()) { + String errMsg = String.format("GetSystemVMVersionCmd on %s failed, message %s", controlIp, result.getDetails()); + s_logger.error(errMsg); + throw new CloudRuntimeException(errMsg); + } + } catch (final Exception e) { + final String msg = "GetSystemVMVersionCmd failed due to " + e; + s_logger.error(msg, e); + throw new CloudRuntimeException(msg, e); + } + return result; + } + + private Answer execute(PatchSystemVmCommand cmd) { + String controlIp = cmd.getAccessDetail((NetworkElementCommand.ROUTER_IP)); + String sysVMName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME); + String homeDir = System.getProperty("user.home"); + File pemFile = new File(homeDir + "/.ssh/id_rsa"); + + ExecutionResult result = getSystemVmVersionAndChecksum(controlIp); + try { + FileUtil.scpPatchFiles(controlIp, "/tmp/", DefaultDomRSshPort, pemFile, systemVmPatchFiles, BASEPATH); + } 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 = calculateCurrentChecksum(sysVMName, "vms/cloud-scripts.tgz").trim(); + + if (!org.apache.commons.lang3.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 { + patchResult = SshHelper.sshExecute(controlIp, DefaultDomRSshPort, "root", + pemFile, null, "/tmp/patch-sysvms.sh", 10000, 10000, 600000); + } catch (Exception e) { + return new PatchSystemVmAnswer(cmd, e.getMessage()); + } + + String scriptVersion = lines[1]; + if (patchResult.second() != null) { Review comment: isNotEmpty() instead? ########## 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: +1 ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -630,6 +637,79 @@ public Answer executeRequest(Command cmd) { return answer; } + private ExecutionResult getSystemVmVersionAndChecksum(String controlIp) { + ExecutionResult result; + try { + result = executeInVR(controlIp, VRScripts.VERSION, null); + if (!result.isSuccess()) { + String errMsg = String.format("GetSystemVMVersionCmd on %s failed, message %s", controlIp, result.getDetails()); + s_logger.error(errMsg); + throw new CloudRuntimeException(errMsg); + } + } catch (final Exception e) { + final String msg = "GetSystemVMVersionCmd failed due to " + e; + s_logger.error(msg, e); + throw new CloudRuntimeException(msg, e); + } + return result; + } + + private Answer execute(PatchSystemVmCommand cmd) { + String controlIp = cmd.getAccessDetail((NetworkElementCommand.ROUTER_IP)); + String sysVMName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME); + String homeDir = System.getProperty("user.home"); + File pemFile = new File(homeDir + "/.ssh/id_rsa"); + + ExecutionResult result = getSystemVmVersionAndChecksum(controlIp); + try { + FileUtil.scpPatchFiles(controlIp, "/tmp/", DefaultDomRSshPort, pemFile, systemVmPatchFiles, BASEPATH); + } 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 = calculateCurrentChecksum(sysVMName, "vms/cloud-scripts.tgz").trim(); + + if (!org.apache.commons.lang3.StringUtils.isEmpty(checksum) && checksum.equals(scriptChecksum)) { + if (!cmd.isForced()) { Review comment: Then can remove this check ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -630,6 +637,79 @@ public Answer executeRequest(Command cmd) { return answer; } + private ExecutionResult getSystemVmVersionAndChecksum(String controlIp) { + ExecutionResult result; + try { + result = executeInVR(controlIp, VRScripts.VERSION, null); + if (!result.isSuccess()) { + String errMsg = String.format("GetSystemVMVersionCmd on %s failed, message %s", controlIp, result.getDetails()); + s_logger.error(errMsg); + throw new CloudRuntimeException(errMsg); + } + } catch (final Exception e) { + final String msg = "GetSystemVMVersionCmd failed due to " + e; + s_logger.error(msg, e); + throw new CloudRuntimeException(msg, e); + } + return result; + } + + private Answer execute(PatchSystemVmCommand cmd) { + String controlIp = cmd.getAccessDetail((NetworkElementCommand.ROUTER_IP)); + String sysVMName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME); + String homeDir = System.getProperty("user.home"); + File pemFile = new File(homeDir + "/.ssh/id_rsa"); + + ExecutionResult result = getSystemVmVersionAndChecksum(controlIp); Review comment: I think its worth wrapping on try/catch here as well and return an answer in case of catching an exception -- 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]
