Copilot commented on code in PR #13492:
URL: https://github.com/apache/cloudstack/pull/13492#discussion_r3474538170
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -1825,10 +1825,29 @@ public Host cancelHostAsDegraded(final
CancelHostAsDegradedCmd cmd) throws NoTra
protected void setKVMVncAccess(long hostId, List<VMInstanceVO> vms) {
for (VMInstanceVO vm : vms) {
GetVncPortAnswer vmVncPortAnswer = (GetVncPortAnswer)
_agentMgr.easySend(hostId, new GetVncPortCommand(vm.getId(),
vm.getInstanceName()));
- if (vmVncPortAnswer != null) {
- vmInstanceDetailsDao.addDetail(vm.getId(),
VmDetailConstants.KVM_VNC_ADDRESS, vmVncPortAnswer.getAddress(), true);
- vmInstanceDetailsDao.addDetail(vm.getId(),
VmDetailConstants.KVM_VNC_PORT, String.valueOf(vmVncPortAnswer.getPort()),
true);
- }
+ updateVncAccessDetailForVM(vm, vmVncPortAnswer, hostId);
+ }
+ }
+
+ private void updateVncAccessDetailForVM(VMInstanceVO vm, GetVncPortAnswer
vmVncPortAnswer, long hostId) {
+ if (vm == null || vmVncPortAnswer == null) {
+ logger.warn("VM or VNC port answer is null. Cannot update VNC
access details.");
+ return;
+ }
+
+ if (!vmVncPortAnswer.getResult()) {
+ logger.warn("Failed to get VNC port for VM {} on host {}. Details:
{}", vm, hostId, vmVncPortAnswer.getDetails());
+ return;
+ }
+
+ String vncAddress = vmVncPortAnswer.getAddress();
+ String vncPort = String.valueOf(vmVncPortAnswer.getPort());
+ if
(org.apache.commons.lang3.StringUtils.isNotBlank(vmVncPortAnswer.getAddress())
&& org.apache.commons.lang3.StringUtils.isNotBlank(vncPort)) {
+ logger.info("Setting VNC access details for VM {} on host {} to
address: {}, port: {}", vm, hostId, vncAddress, vncPort);
+ vmInstanceDetailsDao.addDetail(vm.getId(),
VmDetailConstants.KVM_VNC_ADDRESS, vncAddress, true);
+ vmInstanceDetailsDao.addDetail(vm.getId(),
VmDetailConstants.KVM_VNC_PORT, vncPort, true);
+ } else {
+ logger.warn("Unable to set VNC access details for VM {} on host {}
as the address or port is blank. Address: {}, Port: {}", vm, hostId,
vncAddress, vncPort);
}
Review Comment:
When the agent reply is unsuccessful or missing VNC address/port, the method
returns without changing existing VM details. If a VM already has stale
kvm.vnc.address/port from a prior maintenance event, ConsoleAccessManagerImpl
will keep using them (it only falls back when the values are missing), which
can misdirect console connections. Consider removing the details on
failure/invalid data so the fallback path works and stale data is not retained.
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -1825,10 +1825,29 @@ public Host cancelHostAsDegraded(final
CancelHostAsDegradedCmd cmd) throws NoTra
protected void setKVMVncAccess(long hostId, List<VMInstanceVO> vms) {
for (VMInstanceVO vm : vms) {
GetVncPortAnswer vmVncPortAnswer = (GetVncPortAnswer)
_agentMgr.easySend(hostId, new GetVncPortCommand(vm.getId(),
vm.getInstanceName()));
- if (vmVncPortAnswer != null) {
- vmInstanceDetailsDao.addDetail(vm.getId(),
VmDetailConstants.KVM_VNC_ADDRESS, vmVncPortAnswer.getAddress(), true);
- vmInstanceDetailsDao.addDetail(vm.getId(),
VmDetailConstants.KVM_VNC_PORT, String.valueOf(vmVncPortAnswer.getPort()),
true);
- }
+ updateVncAccessDetailForVM(vm, vmVncPortAnswer, hostId);
+ }
+ }
+
+ private void updateVncAccessDetailForVM(VMInstanceVO vm, GetVncPortAnswer
vmVncPortAnswer, long hostId) {
+ if (vm == null || vmVncPortAnswer == null) {
+ logger.warn("VM or VNC port answer is null. Cannot update VNC
access details.");
+ return;
+ }
+
+ if (!vmVncPortAnswer.getResult()) {
+ logger.warn("Failed to get VNC port for VM {} on host {}. Details:
{}", vm, hostId, vmVncPortAnswer.getDetails());
+ return;
+ }
Review Comment:
This change introduces new branches that skip updating VNC details when
GetVncPortAnswer is unsuccessful or incomplete. There is currently no unit test
covering these failure paths (e.g., getResult=false or address=null), which are
the scenarios this PR is fixing; adding tests would prevent regressions (and
should verify that no DB write occurs and/or details are cleared to allow
console fallback).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVncPortCommandWrapper.java:
##########
@@ -41,6 +41,7 @@ public Answer execute(final GetVncPortCommand command, final
LibvirtComputingRes
final Integer vncPort = libvirtComputingResource.getVncPort(conn,
command.getName());
return new GetVncPortAnswer(command,
libvirtComputingResource.getPrivateIp(), 5900 + vncPort);
} catch (final LibvirtException e) {
+ logger.error("Failed to get vnc port for the vm: {} due to {}",
command.getName(), e.getMessage());
return new GetVncPortAnswer(command, e.toString());
Review Comment:
The new log line drops the exception stack trace (only logs e.getMessage()).
Including the throwable will preserve useful debugging context; also consider
using WARN (similar to other GetVncPort implementations) to avoid noisy ERROR
logs for expected cases like “domain not found”.
--
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]
