shwstppr commented on code in PR #11541:
URL: https://github.com/apache/cloudstack/pull/11541#discussion_r2321807240
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -2014,31 +2019,52 @@ public boolean unmanage(String vmUuid) {
throw new ConcurrentOperationException(msg);
}
- Boolean result = Transaction.execute(new
TransactionCallback<Boolean>() {
- @Override
- public Boolean doInTransaction(TransactionStatus status) {
+ boolean isDomainXMLPreserved = true;
+ // persist domain for kvm host
+ if (HypervisorType.KVM.equals(vm.getHypervisorType())) {
+ long hostId = vm.getHostId();
+ UnmanageInstanceCommand unmanageInstanceCommand;
+ if (State.Stopped.equals(vm.getState())) {
+ hostId = vm.getLastHostId();
+ unmanageInstanceCommand = new
UnmanageInstanceCommand(prepareVmTO(vm.getId(), hostId)); // reconstruct vmSpec
+ } else {
+ unmanageInstanceCommand = new
UnmanageInstanceCommand(vm.getName());
+ }
+ try {
+ Answer answer = _agentMgr.send(hostId,
unmanageInstanceCommand);
+ isDomainXMLPreserved = (answer instanceof
UnmanageInstanceAnswer && answer.getResult());
+ } catch (Exception ex) {
Review Comment:
do we need to use catch all?
Add some logging maybe. Answer.getDetails() generally gives the error
message in case of failure so please check if that can be used
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -4004,6 +4030,39 @@ private void checkAndSetEnterSetupMode(VirtualMachineTO
vmTo, Map<VirtualMachine
vmTo.setEnterHardwareSetup(enterSetup == null ? false : enterSetup);
}
+ protected VirtualMachineTO prepareVmTO(Long vmId, Long hostId) {
Review Comment:
maybe name the method to highlight this preparation is for specific case, to
prevent any undesired usage in the future
Check and compare how it is prepared for the StartCommand and if we can
re-use bits of the code
Check if there would be some other params for the VM that may get missed -
VM details, boot configs, extraconfig, default NICs, ROOT vs DATA volumes, etc.
##########
core/src/main/java/com/cloud/agent/api/UnmanageInstanceCommand.java:
##########
@@ -0,0 +1,56 @@
+//
+// 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.agent.api;
+
+import com.cloud.agent.api.to.VirtualMachineTO;
+
+/**
+ */
+public class UnmanageInstanceCommand extends Command {
+ String instanceName;
+ boolean executeInSequence = false;
+ VirtualMachineTO vm;
+
+ @Override
+ public boolean executeInSequence() {
+ //VR start doesn't go through queue
+ if (instanceName != null && instanceName.startsWith("r-")) {
Review Comment:
do we even allow unmanaging VRs and system VMs?
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -2014,31 +2019,52 @@ public boolean unmanage(String vmUuid) {
throw new ConcurrentOperationException(msg);
}
- Boolean result = Transaction.execute(new
TransactionCallback<Boolean>() {
- @Override
- public Boolean doInTransaction(TransactionStatus status) {
+ boolean isDomainXMLPreserved = true;
+ // persist domain for kvm host
+ if (HypervisorType.KVM.equals(vm.getHypervisorType())) {
+ long hostId = vm.getHostId();
+ UnmanageInstanceCommand unmanageInstanceCommand;
+ if (State.Stopped.equals(vm.getState())) {
+ hostId = vm.getLastHostId();
+ unmanageInstanceCommand = new
UnmanageInstanceCommand(prepareVmTO(vm.getId(), hostId)); // reconstruct vmSpec
+ } else {
+ unmanageInstanceCommand = new
UnmanageInstanceCommand(vm.getName());
+ }
+ try {
+ Answer answer = _agentMgr.send(hostId,
unmanageInstanceCommand);
+ isDomainXMLPreserved = (answer instanceof
UnmanageInstanceAnswer && answer.getResult());
+ } catch (Exception ex) {
+ isDomainXMLPreserved = false;
+ }
+ }
- logger.debug("Unmanaging VM {}", vm);
+ if (isDomainXMLPreserved) {
+ logger.debug("Unmanaging VM {}", vm);
+ Boolean result = Transaction.execute(new
TransactionCallback<Boolean>() {
+ @Override
+ public Boolean doInTransaction(TransactionStatus status) {
+ final VirtualMachineProfile profile = new
VirtualMachineProfileImpl(vm);
+ final VirtualMachineGuru guru = getVmGuru(vm);
- final VirtualMachineProfile profile = new
VirtualMachineProfileImpl(vm);
- final VirtualMachineGuru guru = getVmGuru(vm);
+ try {
+ unmanageVMSnapshots(vm);
+ unmanageVMNics(profile, vm);
+ unmanageVMVolumes(vm);
- try {
- unmanageVMSnapshots(vm);
- unmanageVMNics(profile, vm);
- unmanageVMVolumes(vm);
+ guru.finalizeUnmanage(vm);
+ } catch (Exception e) {
+ logger.error("Error while unmanaging VM {}", vm, e);
+ return false;
+ }
- guru.finalizeUnmanage(vm);
- } catch (Exception e) {
- logger.error("Error while unmanaging VM {}", vm, e);
- return false;
+ return true;
}
-
- return true;
- }
- });
-
- return BooleanUtils.isTrue(result);
+ });
+ return BooleanUtils.isTrue(result);
Review Comment:
extract these operations to a separate method for clarity
--
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]
