Copilot commented on code in PR #13789:
URL: https://github.com/apache/cloudstack/pull/13789#discussion_r3718832262
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtValidateKbossVmCommandWrapper.java:
##########
@@ -92,7 +92,7 @@ private boolean waitForBoot(ValidateKbossVmCommand cmd,
Domain vm) throws Libvir
return true;
}
} catch (LibvirtException ex) {
- if
(!ex.getMessage().contains(LibvirtComputingResource.AGENT_IS_NOT_CONNECTED)) {
+ if
(!LibvirtComputingResource.AGENT_UNRESPONSIVE_ERROR_ORDINAL.equals(ex.getError().getCode().ordinal()))
{
Review Comment:
Using `enum.ordinal()` as a stable identifier is brittle: Java enum ordinals
are not part of the public contract and can change if the enum order changes
(e.g., across libvirt-java versions). Since `getCode()` already returns an
enum, prefer comparing the enum value directly (e.g., `ex.getError().getCode()
== <VIR_ERR_AGENT_UNRESPONSIVE enum constant>`), or compare against an explicit
numeric value exposed by the API (if available) rather than `ordinal()`.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -610,7 +610,7 @@ public class LibvirtComputingResource extends
ServerResourceBase implements Serv
public static final String CGROUP_V2 = "cgroup2fs";
- public static final String AGENT_IS_NOT_CONNECTED = "QEMU guest agent is
not connected";
+ public static final Integer AGENT_UNRESPONSIVE_ERROR_ORDINAL = 86;
Review Comment:
This introduces a magic number (`86`) without any in-code provenance. Please
add a short comment referencing the upstream libvirt/libvirt-java error
identifier that maps to 86 (e.g., the corresponding `virErrorNumber`), and
consider storing the enum constant itself (or a named numeric constant from the
library) rather than a raw integer to reduce the chance of drift.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -7185,7 +7185,7 @@ public Map<String, Long>
createDiskOnlyVmSnapshotForRunningVm(List<Pair<VolumeOb
} catch (LibvirtException e) {
String errorMsg = String.format("Creation of disk-only VM snapshot
for VM [%s] failed due to %s.", vmName, e.getMessage());
boolean isVmConsistent = false;
- if (e.getMessage().contains(AGENT_IS_NOT_CONNECTED)) {
+ if
(AGENT_UNRESPONSIVE_ERROR_ORDINAL.equals(e.getError().getCode().ordinal())) {
Review Comment:
The guest-agent-unresponsive detection logic is now duplicated in multiple
places (here and in `LibvirtValidateKbossVmCommandWrapper`). To reduce
divergence over time, consider a small shared helper (e.g.,
`isGuestAgentUnresponsive(LibvirtException e)`) in `LibvirtComputingResource`
(or a utility class) that encapsulates the comparison and any future mapping
changes.
--
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]
