Copilot commented on code in PR #12331:
URL: https://github.com/apache/cloudstack/pull/12331#discussion_r2693366085
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -378,6 +380,28 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
+ private DomainState getDestDomainState(Domain destDomain, String vmName) {
+ DomainState dmState = null;
+ try {
+ dmState = destDomain.getInfo().state;
+ } catch (final LibvirtException e) {
+ logger.info("Failed to get domain state for VM: " + vmName + " due
to: " + e.getMessage());
Review Comment:
The error message uses logger.info() for an exception, which is inconsistent
with the existing error handling pattern in this file. Line 274 uses
logger.info() for a similar case, but line 388 should use String.format() for
consistency with other error messages in the file (e.g., lines 285, 324, 338).
Consider changing to: `logger.info(String.format("Failed to get domain state
for VM [%s] due to: [%s].", vmName, e.getMessage()));`
```suggestion
logger.info(String.format("Failed to get domain state for VM
[%s] due to: [%s].", vmName, e.getMessage()));
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -378,6 +380,28 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
+ private DomainState getDestDomainState(Domain destDomain, String vmName) {
+ DomainState dmState = null;
+ try {
+ dmState = destDomain.getInfo().state;
+ } catch (final LibvirtException e) {
+ logger.info("Failed to get domain state for VM: " + vmName + " due
to: " + e.getMessage());
+ }
+ return dmState;
+ }
+
+ private void resumeDomainIfPaused(Domain destDomain, String vmName) {
+ DomainState dmState = getDestDomainState(destDomain, vmName);
+ if (dmState == DomainState.VIR_DOMAIN_PAUSED) {
+ logger.info("Resuming VM " + vmName + " on destination after
migration");
Review Comment:
The log message uses String concatenation instead of String.format() which
is the established pattern in this file (e.g., lines 279, 316). For consistency
with other log messages, consider: `logger.info(String.format("Resuming VM [%s]
on destination after migration.", vmName));`
```suggestion
logger.info(String.format("Resuming VM %s on destination after
migration", vmName));
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -378,6 +380,28 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
+ private DomainState getDestDomainState(Domain destDomain, String vmName) {
+ DomainState dmState = null;
+ try {
+ dmState = destDomain.getInfo().state;
+ } catch (final LibvirtException e) {
+ logger.info("Failed to get domain state for VM: " + vmName + " due
to: " + e.getMessage());
+ }
+ return dmState;
+ }
+
+ private void resumeDomainIfPaused(Domain destDomain, String vmName) {
+ DomainState dmState = getDestDomainState(destDomain, vmName);
+ if (dmState == DomainState.VIR_DOMAIN_PAUSED) {
+ logger.info("Resuming VM " + vmName + " on destination after
migration");
+ try {
+ destDomain.resume();
+ } catch (final Exception e) {
+ logger.error("Failed to resume vm " + vmName + " on
destination after migration due to : " + e.getMessage());
Review Comment:
The error message uses String concatenation instead of String.format() which
is the pattern used consistently throughout this file for similar log messages
(e.g., lines 279, 285, 324). Consider changing to:
`logger.error(String.format("Failed to resume VM [%s] on destination after
migration due to: [%s].", vmName, e.getMessage()));` for consistency.
```suggestion
logger.error(String.format("Failed to resume VM [%s] on
destination after migration due to: [%s].", vmName, e.getMessage()));
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -378,6 +380,28 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
+ private DomainState getDestDomainState(Domain destDomain, String vmName) {
+ DomainState dmState = null;
+ try {
+ dmState = destDomain.getInfo().state;
+ } catch (final LibvirtException e) {
+ logger.info("Failed to get domain state for VM: " + vmName + " due
to: " + e.getMessage());
+ }
+ return dmState;
+ }
+
+ private void resumeDomainIfPaused(Domain destDomain, String vmName) {
Review Comment:
The new methods `getDestDomainState` and `resumeDomainIfPaused` lack test
coverage. Given that the test file LibvirtMigrateCommandWrapperTest.java has
comprehensive test coverage for this class, these new methods should have
corresponding unit tests to verify the behavior when the domain is paused, when
it's running, and when LibvirtException is thrown.
--
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]
