[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4966: Refactor few process of VirtualMachineManagerImpl and improve logs

[GitBox <gi...@apache.org>]

GabrielBrascher commented on a change in pull request #4966:
URL: https://github.com/apache/cloudstack/pull/4966#discussion_r673423202



##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -1129,27 +1080,24 @@ public void orchestrateStart(final String vmUuid, final Map<VirtualMachineProfil
                     if (planChangedByVolume) {
                         plan = originalPlan;
                         planChangedByVolume = false;
-                        //do not enter volume reuse for next retry, since we want to look for resources outside the volume's cluster
                         reuseVolume = false;
                         continue;
                     }
                     throw new InsufficientServerCapacityException("Unable to create a deployment for " + vmProfile, DataCenter.class, plan.getDataCenterId(),
                             areAffinityGroupsAssociated(vmProfile));
                 }
 
-                if (dest != null) {

Review comment:
       This null check is not necessary indeed :+1:

##########
File path: api/src/main/java/com/cloud/vm/NicProfile.java
##########
@@ -430,16 +430,6 @@ public void deallocate() {
 
     @Override
     public String toString() {
-        return new StringBuilder("NicProfile[").append(id)
-                .append("-")
-                .append(vmId)
-                .append("-")
-                .append(reservationId)
-                .append("-")
-                .append(iPv4Address)
-                .append("-")
-                .append(broadcastUri)
-                .append("]")
-                .toString();
+        return String.format("NicProfile {\"id\": %s, \"vmId\": %s, \"reservationId\": \"%s\", \"iPv4Address\": \"%s\", \"broadcastUri\": \"%s\"}", id, vmId, reservationId, iPv4Address, broadcastUri);

Review comment:
       `NicProfile {\"id\": %s, \"vmId\": %s` will result in `NicProfile {"id": 123, "vmId": 456, ...`
   
   I would go to something as:
   `NicProfile {id: \"%s\", vmId: \"%s\"` :arrow_right: `NicProfile {id: "123", vmId: "456", ...`
   OR
   `NicProfile {id: %s, vmId: %s` :arrow_right: `NicProfile {id: 123, vmId: 456, ...`
   

##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -618,43 +607,43 @@ protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableExcepti
 
         final VirtualMachineGuru guru = getVmGuru(vm);
         guru.finalizeExpunge(vm);
-        //remove the overcommit details from the uservm details
+
         userVmDetailsDao.removeDetails(vm.getId());
 
-        // Remove deploy as-is (if any)
         userVmDeployAsIsDetailsDao.removeDetails(vm.getId());
 
-        // send hypervisor-dependent commands before removing
-        final List<Command> finalizeExpungeCommands = hvGuru.finalizeExpunge(vm);
-        if (finalizeExpungeCommands != null && finalizeExpungeCommands.size() > 0) {
-            if (hostId != null) {
-                final Commands cmds = new Commands(Command.OnError.Stop);
-                for (final Command command : finalizeExpungeCommands) {
-                    command.setBypassHostMaintenance(expungeCommandCanBypassHostMaintenance(vm));
-                    cmds.addCommand(command);
-                }
-                if (nicExpungeCommands != null) {
-                    for (final Command command : nicExpungeCommands) {
-                        command.setBypassHostMaintenance(expungeCommandCanBypassHostMaintenance(vm));
-                        cmds.addCommand(command);
-                    }
-                }
-                _agentMgr.send(hostId, cmds);
-                if (!cmds.isSuccessful()) {
-                    for (final Answer answer : cmds.getAnswers()) {
-                        if (!answer.getResult()) {
-                            s_logger.warn("Failed to expunge vm due to: " + answer.getDetails());
-                            throw new CloudRuntimeException("Unable to expunge " + vm + " due to " + answer.getDetails());
-                        }
-                    }
-                }
-            }
-        }
-
         if (s_logger.isDebugEnabled()) {
             s_logger.debug("Expunged " + vm);
         }
+    }
+
+    protected void handleUnsuccessfulCommands(Commands cmds, VMInstanceVO vm) throws CloudRuntimeException {
+        String cmdsStr = cmds.toString();
+        String vmToString = vm.toString();
+
+        if (cmds.isSuccessful()) {
+            s_logger.debug(String.format("The commands [%s] to %s were successful.", cmdsStr, vmToString));
+            return;
+        }
+
+        s_logger.info(String.format("The commands [%s] to %s were unsuccessful. Handling answers.", cmdsStr, vmToString));
 
+        Answer[] answers = cmds.getAnswers();
+        if (answers == null) {
+            s_logger.debug(String.format("There are no answers to commands [%s] to %s.", cmdsStr, vmToString));
+            return;
+        }
+
+        for (Answer answer : answers) {
+            String details = answer.getDetails();
+            if (!answer.getResult()) {
+                String message = String.format("Unable to expunge %s due to [%s].", vmToString, details);
+                s_logger.warn(message);

Review comment:
       What do you think of moving this log level from "warn" to "error"? This log look like could be an Error considering the thrown 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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org