DaanHoogland commented on a change in pull request #4955:
URL: https://github.com/apache/cloudstack/pull/4955#discussion_r623847956
##########
File path:
services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
##########
@@ -1251,13 +1194,12 @@ public boolean finalizeCommandsOnStart(Commands cmds,
VirtualMachineProfile prof
if (controlNic == null) {
if (managementNic == null) {
- s_logger.error("Management network doesn't exist for the
secondaryStorageVm " + profile.getVirtualMachine());
+ s_logger.warn(String.format("Management network does not exist
for the secondary storage %s.", profile. toString()));
return false;
}
controlNic = managementNic;
}
- // verify ssh access on management nic for system vm running on HyperV
Review comment:
how about making this a private method name
```suggestion
verifySshAccessOnManagementNicForSystemvmRunningOnHyperV(profile,
controlNic, managementNic, cmds);
return true;
}
private void
verifySshAccessOnManagementNicForSystemvmRunningOnHyperV(VirtualMachineProfile
profile,
NicProfile controlNic,
NicProfile managementNic,
Commands cmds) {
if(profile.getHypervisorType() == HypervisorType.Hyperv) {
controlNic = managementNic;
}
CheckSshCommand check = new
CheckSshCommand(profile.getInstanceName(),controlNic.getIPv4Address(), 3922);
cmds.addCommand("checkSsh", check);
}
```
context: see comment removal below.
##########
File path:
services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
##########
@@ -1272,22 +1216,20 @@ public boolean finalizeCommandsOnStart(Commands cmds,
VirtualMachineProfile prof
public boolean finalizeStart(VirtualMachineProfile profile, long hostId,
Commands cmds, ReservationContext context) {
CheckSshAnswer answer = (CheckSshAnswer)cmds.getAnswer("checkSsh");
if (!answer.getResult()) {
- s_logger.warn("Unable to ssh to the VM: " + answer.getDetails());
+ s_logger.warn(String.format("Unable to connect via SSH to the VM
[%s] due to [%s] ", profile.toString(), answer.getDetails()));
return false;
}
try {
- //get system ip and create static nat rule for the vm in case of
basic networking with EIP/ELB
Review comment:
I agree;
1. comment last resort
2. (replace by) method name first resort
in this case I only wonder about the `EIP/ELB` part of the comment, so for
me it was only adding confusion.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
