Martin Peřina has uploaded a new change for review. Change subject: core: Refactor FenceVdsVDSCommand return value ......................................................................
core: Refactor FenceVdsVDSCommand return value Introduces FenceOperationResult which will provider better support for FenceVdsVDSCommand result management Change-Id: I4b3ada18bb36a376c5c19327461d2daa53eedc59 Bug-Url: https://bugzilla.redhat.com/1182510 Signed-off-by: Martin Perina <mper...@redhat.com> --- A backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/pm/FenceOperationResult.java A backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/pm/HostPowerStatus.java A backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/businessentities/pm/FenceOperationResultTest.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommand.java A backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommandTest.java 5 files changed, 534 insertions(+), 30 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/62/38062/1 diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/pm/FenceOperationResult.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/pm/FenceOperationResult.java new file mode 100644 index 0000000..a381ef1 --- /dev/null +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/pm/FenceOperationResult.java @@ -0,0 +1,112 @@ +package org.ovirt.engine.core.common.businessentities.pm; + +/** + * Result of action performed on single fence agent + */ +public class FenceOperationResult { + /** + * Operation result + */ + private final Status status; + + /** + * Power status + */ + private final HostPowerStatus powerStatus; + + /** + * Message received from fence agent + */ + private final String message; + + /** + * Creates instance with status {@code Status.ERROR}, power status {@code HostPowerStatus.UNKNOWN} and {@code null} + * error message + */ + public FenceOperationResult() { + this(Status.ERROR, HostPowerStatus.UNKNOWN, null); + } + + /** + * Creates instance with specified status, power status {@code HostPowerStatus.UNKNOWN} and {@code null} + * error message + */ + public FenceOperationResult(Status status) { + this(status, HostPowerStatus.UNKNOWN, null); + } + + + /** + * Creates instance with specified status, powerStatus and message + */ + public FenceOperationResult(Status status, HostPowerStatus powerStatus, String message) { + this.status = status; + this.powerStatus = powerStatus; + this.message = message; + } + + /** + * Creates instance using requested action and result of RPC call + */ + public FenceOperationResult( + FenceActionType action, + int code, + String message, + String power, + String operationStatus) { + + Status st = code == 0 ? Status.SUCCESS : Status.ERROR; + powerStatus = HostPowerStatus.forValue(power); + this.message = message; + + if (action == FenceActionType.STATUS && powerStatus == HostPowerStatus.UNKNOWN) { + st = Status.ERROR; + } + + if (action != FenceActionType.STATUS && "skipped".equalsIgnoreCase(operationStatus)) { + /* + * TODO: + * This needs to be fixed when we move the check "is host already in desired state" + * for on/off action from engine to VDSM + */ + st = Status.SKIPPED_DUE_TO_POLICY; + } + + status = st; + } + + public Status getStatus() { + return status; + } + + public HostPowerStatus getPowerStatus() { + return powerStatus; + } + + public String getMessage() { + return message; + } + + public static enum Status { + /** + * Operation finished successfully + */ + SUCCESS, + + /** + * Operation finished with error + */ + ERROR, + + /** + * Operation was skipped, because host is already in required power status + */ + SKIPPED_ALREADY_IN_STATUS, + + /** + * Operation was skipped due to fencing policy + */ + SKIPPED_DUE_TO_POLICY + } +} + diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/pm/HostPowerStatus.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/pm/HostPowerStatus.java new file mode 100644 index 0000000..03f84e2 --- /dev/null +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/pm/HostPowerStatus.java @@ -0,0 +1,35 @@ +package org.ovirt.engine.core.common.businessentities.pm; + +/** + * Power status of the host + */ +public enum HostPowerStatus { + /** + * Host is turned on + */ + ON, + + /** + * Host is turned off + */ + OFF, + + /** + * Host status is unknown + */ + UNKNOWN; + + /** + * Parses power status from string + */ + public static HostPowerStatus forValue(String value) { + if ("on".equalsIgnoreCase(value)) { + return ON; + } else if ("off".equalsIgnoreCase(value)) { + return OFF; + } else { + return UNKNOWN; + } + } +} + diff --git a/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/businessentities/pm/FenceOperationResultTest.java b/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/businessentities/pm/FenceOperationResultTest.java new file mode 100644 index 0000000..b638d40 --- /dev/null +++ b/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/businessentities/pm/FenceOperationResultTest.java @@ -0,0 +1,104 @@ +package org.ovirt.engine.core.common.businessentities.pm; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; +import org.ovirt.engine.core.common.businessentities.pm.FenceOperationResult.Status; + +public class FenceOperationResultTest { + /** + * Test result value for successful status action of host powered on + */ + @Test + public void successfulStatusOn() { + FenceOperationResult result = new FenceOperationResult( + FenceActionType.STATUS, + 0, + null, + "on", + null); + + assertEquals(Status.SUCCESS, result.getStatus()); + assertEquals(HostPowerStatus.ON, result.getPowerStatus()); + } + + /** + * Test result value for successful status action of host powered off + */ + @Test + public void successfulStatusOff() { + FenceOperationResult result = new FenceOperationResult( + FenceActionType.STATUS, + 0, + null, + "off", + null); + + assertEquals(Status.SUCCESS, result.getStatus()); + assertEquals(HostPowerStatus.OFF, result.getPowerStatus()); + } + + /** + * Test result value for failed status action + */ + @Test + public void failedStatus() { + FenceOperationResult result = new FenceOperationResult( + FenceActionType.STATUS, + 1, + null, + "unknown", + null); + + assertEquals(Status.ERROR, result.getStatus()); + assertEquals(HostPowerStatus.UNKNOWN, result.getPowerStatus()); + } + + /** + * Test result value for successful stop action + */ + @Test + public void successfulStop() { + FenceOperationResult result = new FenceOperationResult( + FenceActionType.STOP, + 0, + null, + null, + "initiated"); + + assertEquals(Status.SUCCESS, result.getStatus()); + assertEquals(HostPowerStatus.UNKNOWN, result.getPowerStatus()); + } + + /** + * Test result value for failed stop action + */ + @Test + public void failedStop() { + FenceOperationResult result = new FenceOperationResult( + FenceActionType.STOP, + 1, + null, + null, + "initiated"); + + assertEquals(Status.ERROR, result.getStatus()); + assertEquals(HostPowerStatus.UNKNOWN, result.getPowerStatus()); + } + + /** + * Test result value for skipped stop action due to fencing policy + */ + @Test + public void skippedDueToPolicyStop() { + FenceOperationResult result = new FenceOperationResult( + FenceActionType.STOP, + 0, + null, + null, + "skipped"); + + assertEquals(Status.SKIPPED_DUE_TO_POLICY, result.getStatus()); + assertEquals(HostPowerStatus.UNKNOWN, result.getPowerStatus()); + } +} diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommand.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommand.java index 0a7c635..40fcf2c 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommand.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommand.java @@ -4,11 +4,13 @@ import java.util.Map; import org.ovirt.engine.core.common.AuditLogType; -import org.ovirt.engine.core.common.businessentities.FenceStatusReturnValue; import org.ovirt.engine.core.common.businessentities.StorageDomain; import org.ovirt.engine.core.common.businessentities.StorageDomainType; import org.ovirt.engine.core.common.businessentities.VDS; import org.ovirt.engine.core.common.businessentities.pm.FenceActionType; +import org.ovirt.engine.core.common.businessentities.pm.FenceOperationResult; +import org.ovirt.engine.core.common.businessentities.pm.FenceOperationResult.Status; +import org.ovirt.engine.core.common.businessentities.pm.HostPowerStatus; import org.ovirt.engine.core.common.businessentities.vds_spm_id_map; import org.ovirt.engine.core.common.utils.FencingPolicyHelper; import org.ovirt.engine.core.common.vdscommands.FenceVdsVDSCommandParameters; @@ -58,33 +60,27 @@ getParameters().getAction(), getParameters().getAction() != FenceActionType.STATUS); - getVDSReturnValue().setSucceeded(false); - if (getParameters().getAction() == FenceActionType.STATUS && _result.power != null) { - String stat = _result.power.toLowerCase(); - String msg = _result.mStatus.mMessage; - if ("on".equals(stat) || "off".equals(stat)) { - getVDSReturnValue().setSucceeded(true); - } else { - if (!getParameters().getTargetVdsID().equals(Guid.Empty)) { - alertPowerManagementStatusFailed(msg); - } + FenceOperationResult actionResult = new FenceOperationResult( + getParameters().getAction(), + _result.mStatus.mCode, + _result.mStatus.mMessage, + _result.power, + _result.operationStatus); + setReturnValue(actionResult); + getVDSReturnValue().setSucceeded(actionResult.getStatus() != Status.ERROR); - } - FenceStatusReturnValue fenceStatusReturnValue = new FenceStatusReturnValue(stat, msg); - setReturnValue(fenceStatusReturnValue); - } else { - FenceStatusReturnValue fenceStatusReturnValue = new FenceStatusReturnValue( - _result.operationStatus, - _result.mStatus.mMessage != null ? _result.mStatus.mMessage : "" - ); - setReturnValue(fenceStatusReturnValue); - getVDSReturnValue().setSucceeded(_result.mStatus.mCode == 0); + if (getParameters().getAction() == FenceActionType.STATUS + && actionResult.getPowerStatus() == HostPowerStatus.UNKNOWN + &&!getParameters().getTargetVdsID().equals(Guid.Empty)) { + alertPowerManagementStatusFailed(actionResult.getMessage()); } + } else { // start/stop action was skipped, host is already turned on/off alertActionSkippedAlreadyInStatus(); getVDSReturnValue().setSucceeded(true); - setReturnValue(new FenceStatusReturnValue(FenceStatusReturnValue.SKIPPED, "")); + setReturnValue( + new FenceOperationResult(Status.SKIPPED_ALREADY_IN_STATUS, HostPowerStatus.UNKNOWN, "")); } } @@ -117,16 +113,18 @@ * and a Start command is issued command should do nothing. */ private boolean isAlreadyInRequestedStatus() { - boolean ret = false; FenceActionType action = getParameters().getAction(); _result = fenceNode(FenceActionType.STATUS, false); - if (_result.power != null) { - String powerStatus = _result.power.toLowerCase(); - if ((action == FenceActionType.START && powerStatus.equals("on")) || - action == FenceActionType.STOP && powerStatus.equals("off")) - ret = true; - } - return ret; + FenceOperationResult actionResult = new FenceOperationResult( + FenceActionType.STATUS, + _result.mStatus.mCode, + _result.mStatus.mMessage, + _result.power, + _result.operationStatus); + + return actionResult.getStatus() == Status.SUCCESS + && (action == FenceActionType.START && actionResult.getPowerStatus() == HostPowerStatus.ON + || action == FenceActionType.STOP && actionResult.getPowerStatus() == HostPowerStatus.OFF); } protected Map<String, Object> convertFencingPolicy() { diff --git a/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommandTest.java b/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommandTest.java new file mode 100644 index 0000000..1eb2a62 --- /dev/null +++ b/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommandTest.java @@ -0,0 +1,255 @@ +package org.ovirt.engine.core.vdsbroker.vdsbroker; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.when; + +import java.util.HashMap; +import java.util.Map; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.ovirt.engine.core.common.businessentities.FenceAgent; +import org.ovirt.engine.core.common.businessentities.VDS; +import org.ovirt.engine.core.common.businessentities.VdsStatic; +import org.ovirt.engine.core.common.businessentities.pm.FenceActionType; +import org.ovirt.engine.core.common.businessentities.pm.FenceOperationResult; +import org.ovirt.engine.core.common.businessentities.pm.FenceOperationResult.Status; +import org.ovirt.engine.core.common.businessentities.pm.HostPowerStatus; +import org.ovirt.engine.core.common.vdscommands.FenceVdsVDSCommandParameters; +import org.ovirt.engine.core.compat.Guid; +import org.ovirt.engine.core.compat.Version; +import org.ovirt.engine.core.dal.dbbroker.DbFacade; +import org.ovirt.engine.core.dao.VdsDAO; + +@RunWith(MockitoJUnitRunner.class) +public class FenceVdsVDSCommandTest { + private static Guid TARGET_HOST_ID = new Guid("11111111-1111-1111-1111-111111111111"); + private static Guid PROXY_HOST_ID = new Guid("44444444-4444-4444-4444-444444444444"); + private static Guid DC_ID = new Guid("22222222-2222-2222-2222-222222222222"); + + private FenceVdsVDSCommand<FenceVdsVDSCommandParameters> command; + + @Mock + private DbFacade dbFacade; + + @Mock + private VdsDAO vdsDAO; + + @Mock + private VDS targetHost; + + @Mock + private VdsStatic targetVdsStatic; + + @Mock + private VDS proxyHost; + + @Mock + private IVdsServer broker; + + @Before + public void setupMocks() { + when(dbFacade.getVdsDao()).thenReturn(vdsDAO); + + when(vdsDAO.get(eq(TARGET_HOST_ID))).thenReturn(targetHost); + when(targetHost.getId()).thenReturn(TARGET_HOST_ID); + + when(vdsDAO.get(eq(PROXY_HOST_ID))).thenReturn(proxyHost); + when(proxyHost.getId()).thenReturn(PROXY_HOST_ID); + when(proxyHost.getVdsGroupCompatibilityVersion()).thenReturn(Version.getLast()); + } + + private void setupCommand(FenceVdsVDSCommandParameters params) { + command = new FenceVdsVDSCommand<FenceVdsVDSCommandParameters>(params) { + + @Override + protected IVdsServer initializeVdsBroker(Guid vdsId) { + return broker; + } + + @Override + protected DbFacade getDbFacade() { + return dbFacade; + } + + @Override + protected VdsStatic getAndSetVdsStatic() { + return targetVdsStatic; + } + + @Override + protected String getVdsFenceOptions(String type, String options, String compatibilityVersion) { + return ""; + } + + @Override + protected void alertPowerManagementStatusFailed(String reason) { + } + + @Override + protected void alertActionSkippedAlreadyInStatus() { + } + }; + } + + private Map<String, Object> createBrokerResultMap( + int returnCode, + String message, + String powerStatus, + String operationStatus) { + + Map<String, Object> statusMap = new HashMap<>(); + statusMap.put("code", returnCode); + statusMap.put("message", message); + + Map<String, Object> map = new HashMap<>(); + map.put("status", statusMap); + map.put("power", powerStatus); + map.put("operationStatus", operationStatus); + return map; + } + + private void setupBrokerResult(Map<String, Object> first) { + setupBrokerResult(first, null); + } + + private void setupBrokerResult(Map<String, Object> first, Map<String, Object> second) { + when(broker.fenceNode( + any(String.class), + any(String.class), + any(String.class), + any(String.class), + any(String.class), + any(String.class), + any(String.class), + any(String.class), + any(Map.class))) + .thenReturn(new FenceStatusReturnForXmlRpc(first)) + .thenReturn(second == null ? null : new FenceStatusReturnForXmlRpc(second)); + } + + private FenceVdsVDSCommandParameters setupCommandParams(FenceActionType actionType) { + FenceAgent agent = new FenceAgent(); + agent.setIp("1.2.3.4"); + agent.setPort(1234); + agent.setType("ipmilan"); + agent.setUser("admin"); + agent.setPassword("admin"); + agent.setOptions(""); + return new FenceVdsVDSCommandParameters( + PROXY_HOST_ID, + TARGET_HOST_ID, + agent, + actionType, + null); + } + + /** + * Tests result of successful get power status of host + */ + @Test + public void successfulGetStatus() { + String agentMessage = "Test succeeded: on"; + + setupCommand(setupCommandParams(FenceActionType.STATUS)); + setupBrokerResult(createBrokerResultMap(0, agentMessage, "on", null)); + + command.execute(); + FenceOperationResult result = (FenceOperationResult) command.getVDSReturnValue().getReturnValue(); + + assertEquals(Status.SUCCESS, result.getStatus()); + assertEquals(HostPowerStatus.ON, result.getPowerStatus()); + assertEquals(agentMessage, result.getMessage()); + } + + /** + * Tests result of failed get power status of host + */ + @Test + public void failedGetStatus() { + String agentMessage = "Test failed, status unknown"; + + setupCommand(setupCommandParams(FenceActionType.STATUS)); + setupBrokerResult(createBrokerResultMap(1, agentMessage, "unknown", null)); + + command.execute(); + FenceOperationResult result = (FenceOperationResult) command.getVDSReturnValue().getReturnValue(); + + assertEquals(Status.ERROR, result.getStatus()); + assertEquals(HostPowerStatus.UNKNOWN, result.getPowerStatus()); + assertEquals(agentMessage, result.getMessage()); + } + + /** + * Test execution of stop action when host is powered down + */ + @Test + public void stopHostWhichIsPoweredDown() { + setupCommand(setupCommandParams(FenceActionType.STOP)); + setupBrokerResult( + createBrokerResultMap(0, "", "off", null)); // result of STATUS action executed prior to STOP action + + command.execute(); + FenceOperationResult result = (FenceOperationResult) command.getVDSReturnValue().getReturnValue(); + + assertEquals(Status.SKIPPED_ALREADY_IN_STATUS, result.getStatus()); + assertEquals(HostPowerStatus.UNKNOWN, result.getPowerStatus()); + } + + /** + * Test execution of stop action when host is powered up + */ + @Test + public void stopHostWhichIsPoweredUp() { + setupCommand(setupCommandParams(FenceActionType.STOP)); + setupBrokerResult( + createBrokerResultMap(0, "", "on", null), // result of STATUS action executed prior to STOP action + createBrokerResultMap(0, "", "unknown", "initiated")); // result of actual STOP action + + command.execute(); + FenceOperationResult result = (FenceOperationResult) command.getVDSReturnValue().getReturnValue(); + + assertEquals(Status.SUCCESS, result.getStatus()); + assertEquals(HostPowerStatus.UNKNOWN, result.getPowerStatus()); + } + + /** + * Test execution of stop action when host power status cannot be determined + */ + @Test + public void stopHostWithUnknownPowerStatus() { + setupCommand(setupCommandParams(FenceActionType.STOP)); + setupBrokerResult( + createBrokerResultMap(1, "", "unknown", null), // result of STATUS action executed prior to STOP action + createBrokerResultMap(0, "", "unknown", "initiated")); // result of actual STOP action + + command.execute(); + FenceOperationResult result = (FenceOperationResult) command.getVDSReturnValue().getReturnValue(); + + assertEquals(Status.SUCCESS, result.getStatus()); + assertEquals(HostPowerStatus.UNKNOWN, result.getPowerStatus()); + } + + /** + * Test execution of stop action when fencing policy forbids stopping host still connected to storage, so stopping + * the host is skipped + */ + @Test + public void stopHostSkippedDueToFencingPolicy() { + setupCommand(setupCommandParams(FenceActionType.STOP)); + setupBrokerResult( + createBrokerResultMap(0, "", "on", null), // result of STATUS action executed prior to STOP action + createBrokerResultMap(0, "", "unknown", "skipped")); // result of actual STOP action + + command.execute(); + FenceOperationResult result = (FenceOperationResult) command.getVDSReturnValue().getReturnValue(); + + assertEquals(Status.SKIPPED_DUE_TO_POLICY, result.getStatus()); + assertEquals(HostPowerStatus.UNKNOWN, result.getPowerStatus()); + } +} -- To view, visit http://gerrit.ovirt.org/38062 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I4b3ada18bb36a376c5c19327461d2daa53eedc59 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
