Copilot commented on code in PR #13589:
URL: https://github.com/apache/cloudstack/pull/13589#discussion_r3966872253
##########
plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHAProvider.java:
##########
@@ -89,17 +90,53 @@ public boolean fence(Host r) throws HAFenceException {
try {
if (outOfBandManagementService.isOutOfBandManagementEnabled(r)){
final OutOfBandManagementResponse resp =
outOfBandManagementService.executePowerOperation(r, PowerOperation.OFF, null);
- return resp.getSuccess();
+ if (resp.getSuccess()) {
+ return true;
+ }
+ logger.warn("OOBM fence operation failed for the host {}", r);
+ return fenceHostViaStorageHeartbeat(r);
} else {
logger.warn("OOBM fence operation failed for this host {}", r);
- return false;
+ return fenceHostViaStorageHeartbeat(r);
Review Comment:
New behavior was introduced for the `isOutOfBandManagementEnabled(r) ==
false` branch (it now attempts the storage-heartbeat fallback). The added unit
tests cover OOBM success/failure/exception when OOBM is enabled, but there is
no test asserting the behavior when OOBM is disabled (both with fallback
enabled and disabled). Adding those tests would lock in the intended semantics
for this new branch.
##########
plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHAProvider.java:
##########
@@ -89,17 +90,53 @@ public boolean fence(Host r) throws HAFenceException {
try {
if (outOfBandManagementService.isOutOfBandManagementEnabled(r)){
final OutOfBandManagementResponse resp =
outOfBandManagementService.executePowerOperation(r, PowerOperation.OFF, null);
- return resp.getSuccess();
+ if (resp.getSuccess()) {
+ return true;
+ }
+ logger.warn("OOBM fence operation failed for the host {}", r);
+ return fenceHostViaStorageHeartbeat(r);
} else {
logger.warn("OOBM fence operation failed for this host {}", r);
Review Comment:
Both warn messages are misleading: in the `else` branch no OOBM fence
operation was attempted (OOBM is disabled), and in the failed-response branch
it would be useful to log that the power operation returned an unsuccessful
response. Consider updating the messages to reflect the actual condition (e.g.,
'OOBM is disabled' vs 'OOBM power-off request returned unsuccessful') to avoid
confusing operators during incidents.
##########
plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHAProvider.java:
##########
@@ -89,17 +90,53 @@ public boolean fence(Host r) throws HAFenceException {
try {
if (outOfBandManagementService.isOutOfBandManagementEnabled(r)){
final OutOfBandManagementResponse resp =
outOfBandManagementService.executePowerOperation(r, PowerOperation.OFF, null);
- return resp.getSuccess();
+ if (resp.getSuccess()) {
+ return true;
+ }
+ logger.warn("OOBM fence operation failed for the host {}", r);
+ return fenceHostViaStorageHeartbeat(r);
} else {
logger.warn("OOBM fence operation failed for this host {}", r);
- return false;
+ return fenceHostViaStorageHeartbeat(r);
}
} catch (Exception e){
logger.warn("OOBM service is not configured or enabled for this
host {} error is {}", r, e.getMessage());
+ if (fenceHostViaStorageHeartbeat(r)) {
+ return true;
+ }
throw new HAFenceException(String.format("OBM service is not
configured or enabled for this host %s", r.getName()), e);
Review Comment:
This exception message is inaccurate for cases where OOBM *is*
configured/enabled but `executePowerOperation` fails/throws (e.g., 'BMC
unreachable'). It’s also inconsistent with the rest of the method that uses
'OOBM' terminology. Consider adjusting the message to describe the actual
failure more generally (and consistently), optionally including the underlying
exception message.
##########
plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHAProvider.java:
##########
@@ -89,17 +90,53 @@ public boolean fence(Host r) throws HAFenceException {
try {
if (outOfBandManagementService.isOutOfBandManagementEnabled(r)){
final OutOfBandManagementResponse resp =
outOfBandManagementService.executePowerOperation(r, PowerOperation.OFF, null);
- return resp.getSuccess();
+ if (resp.getSuccess()) {
+ return true;
+ }
+ logger.warn("OOBM fence operation failed for the host {}", r);
+ return fenceHostViaStorageHeartbeat(r);
} else {
logger.warn("OOBM fence operation failed for this host {}", r);
- return false;
+ return fenceHostViaStorageHeartbeat(r);
}
} catch (Exception e){
logger.warn("OOBM service is not configured or enabled for this
host {} error is {}", r, e.getMessage());
Review Comment:
The catch-block log drops the stack trace by logging only `e.getMessage()`,
which makes diagnosing real OOBM failures (timeouts, auth errors, network
issues) much harder. Log the exception itself (as a Throwable parameter) so the
stack trace is captured.
--
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]
