Martin Peřina has posted comments on this change. Change subject: restapi: Host Power-Management Refactor (#977674) - WIP ......................................................................
Patch Set 18: (20 comments) http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java: Line 311: getCompensationContext().snapshotNewEntity(vdsStatistics); Line 312: } Line 313: Line 314: private void addFencingAgentsToDb() { Line 315: if (getParameters().getFencingAgents() != null && !getParameters().getFencingAgents().isEmpty()) { Please remove isEmpty() test, for loop will handle this Line 316: for (FencingAgent agent : getParameters().getvds().getFencingAgents()) { Line 317: DbFacade.getInstance().getFencingAgentDao().save(agent); Line 318: } Line 319: } Line 312: } Line 313: Line 314: private void addFencingAgentsToDb() { Line 315: if (getParameters().getFencingAgents() != null && !getParameters().getFencingAgents().isEmpty()) { Line 316: for (FencingAgent agent : getParameters().getvds().getFencingAgents()) { Why are testing getParameters().getFencingAgents() and after that iterating over getParameters().getvds().getFencingAgents()? Line 317: DbFacade.getInstance().getFencingAgentDao().save(agent); Line 318: } Line 319: } Line 320: } Line 393: } Line 394: return returnValue; Line 395: } Line 396: Line 397: protected boolean isPowerManagementLegal(VdsStatic vdsStatic, Please pass only boolean pmEnabled as a parameter instead of whole VdsStatic Line 398: List<FencingAgent> fencingAgents, Line 399: String clusterCompatibilityVersion) { Line 400: if (vdsStatic.isPmEnabled()) { Line 401: if (fencingAgents.isEmpty()) { http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVDSClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVDSClusterCommand.java: Line 329: // condition. Line 330: try (EngineLock lock = GlusterUtil.getInstance().acquireGlusterLockWait(sourceClusterId)) { Line 331: String hostName = Line 332: (getVds().getHostName().isEmpty()) ? getPowerManagementIp() Line 333: : getVds().getHostName(); We should discuss this condition with gluster team, it doesn't make sense. How could the host with empty hostname be installed? Line 334: VDS runningHostInSourceCluster = getClusterUtils().getUpServer(sourceClusterId); Line 335: if (runningHostInSourceCluster == null) { Line 336: log.error("Cannot remove host from source cluster, no host in Up status found in source cluster"); Line 337: handleError(-1, "No host in Up status found in source cluster"); Line 359: // condition. Line 360: try (EngineLock lock = GlusterUtil.getInstance().acquireGlusterLockWait(targetClusterId)) { Line 361: String hostName = Line 362: (getVds().getHostName().isEmpty()) ? getPowerManagementIp() Line 363: : getVds().getHostName(); Same as above Line 364: VDS runningHostInTargetCluster = getClusterUtils().getUpServer(targetClusterId); Line 365: if (runningHostInTargetCluster == null) { Line 366: log.error("Cannot add host to target cluster, no host in Up status found in target cluster"); Line 367: handleError(-1, "No host in Up status found in target cluster"); http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceExecutor.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceExecutor.java: Line 41 Line 42 Line 43 Line 44 Line 45 Please don't remove minVersionSupportingFencingPol, so we will not need to compute minimal supported version on each proxy test http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceVdsBaseCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceVdsBaseCommand.java: Line 662: Line 663: /** Line 664: * Determines according to the return status from VDSM whether the fencing-operation has been skipped due to policy. Line 665: */ Line 666: protected boolean wasSkippedDueToPolicy(VDSFenceReturnValue vdsReturnValue) { Instead of two methods wasSkippedDueToPolicy wouldn't be better only one with relevant returnValue object? protected boolean wasSkippedDueToPolicy(Object returnValue) { return returnValue instanceof FenceStatusReturnValue && ((FenceStatusReturnValue)returnvalue).getIsSkipped(); } Line 667: if (vdsReturnValue.getFenceResult().getReturnValue() instanceof FenceStatusReturnValue) { Line 668: FenceStatusReturnValue fenceStatus = Line 669: (FenceStatusReturnValue) vdsReturnValue.getFenceResult().getReturnValue(); Line 670: return fenceStatus.getIsSkipped(); http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsCommand.java: Line 256: }); Line 257: Line 258: } Line 259: Line 260: protected boolean isPowerManagementLegal(VdsStatic vdsStatic, Please pass boolean pmEnabled as a parameter instead of whole VdsStatic Line 261: List<FencingAgent> fencingAgents, Line 262: String clusterCompatibilityVersion) { Line 263: if (vdsStatic.isPmEnabled()) { Line 264: if (fencingAgents.isEmpty()) { http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/pm/PmHealthCheckManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/pm/PmHealthCheckManager.java: Line 291: public static PmHealthCheckManager getInstance() { Line 292: return instance; Line 293: } Line 294: Line 295: private class PmHealth { Please make this class static as findbugs suggest Line 296: public PmHealth(VDS host) { Line 297: super(); Line 298: this.host = host; Line 299: } http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java: Line 912 Line 913 Line 914 Line 915 Line 916 Not part of the patch http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/FencingAgent.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/FencingAgent.java: Line 11: import org.ovirt.engine.core.common.validation.annotation.HostnameOrIp; Line 12: import org.ovirt.engine.core.common.validation.group.PowerManagementCheck; Line 13: import org.ovirt.engine.core.compat.Guid; Line 14: Line 15: public class FencingAgent implements BusinessEntity<Guid> { Wouldn't FenceAgent be a better name? It's shorter and matches name of RPM package Line 16: Line 17: private static final long serialVersionUID = -5910560758520427911L; Line 18: private Guid id; Line 19: private Guid hostId; Line 16: Line 17: private static final long serialVersionUID = -5910560758520427911L; Line 18: private Guid id; Line 19: private Guid hostId; Line 20: private Integer order; Wouldn't primitive int be better? Line 21: Line 22: @EditableField Line 23: private HashMap<String, String> optionsMap; Line 24: Line 184: Line 185: public static class FencingAgentOrderComparator implements Comparator<FencingAgent> { Line 186: Line 187: @Override Line 188: public int compare(FencingAgent agent1, FencingAgent agent2) { Why not use Integer.compareTo() method? Line 189: if (agent1.getOrder() < agent2.getOrder()) { Line 190: return -1; Line 191: } else if (agent1.getOrder() == agent2.getOrder()) { Line 192: return 0; http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java: Line 1306: public void setBalloonEnabled(boolean enableBalloon) { Line 1307: this.balloonEnabled = enableBalloon; Line 1308: } Line 1309: Line 1310: public boolean isFencingAgentsExist() { Please remove this method, executing just getFencingAgents.isEmpty() is better idea Line 1311: return this.getFencingAgents().size() > 0; Line 1312: } Line 1313: http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAODbFacadeImpl.java: Line 31: /** Line 32: * <code>VdsDAODbFacadeImpl</code> provides an implementation of {@code VdsDAO} that uses previously written code from Line 33: * {@code DbFacade}. Line 34: */ Line 35: public class VdsDAODbFacadeImpl extends BaseDAODbFacade implements VdsDAO { I really don't like this mega join. I think that returning fecning agents by separate db call is a better idea. Line 36: Line 37: @Override Line 38: public VDS get(Guid id) { Line 39: return get(id, null, false); http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 924: VDS_REGISTER_UNIQUE_ID_AMBIGUOUS=Cannot add host which has the same unique ID as one of the hosts: ${HostNameList}.\nThis might occur in case this host was already added to the system. Line 925: VDS_CANNOT_CONNECT_TO_SERVER=Cannot ${action} ${type}. Connecting to host via SSH has failed, verify that the host is reachable (IP address, routable address etc.) You may refer to the engine.log file for further details. Line 926: VDS_CANNOT_AUTHENTICATE_TO_SERVER=Cannot ${action} ${type}. SSH authentication failed, verify authentication parameters are correct (Username/Password, public-key etc.) You may refer to the engine.log file for further details. Line 927: VDS_SECURITY_CONNECTION_ERROR=Cannot ${action} ${type}. SSH connection failed, ${ErrorMsg}. Line 928: VDS_REMOVE_FENCING_AGENT_ID_REQUIRED=Cannot Remove Fencing-Agent. Agent ID not provided. Please remove spaces at end Line 929: VDS_REMOVE_FENCING_AGENTS_VDS_ID_REQUIRED=Cannot Remove Fencing-Agents. Host ID not provided. Line 930: VDS_ADD_FENCING_AGENT_MANDATORY_PARAMETERS_MISSING=Cannot Add Fencing-Agent. One or more mandatory parameters were not provided. Mandatory parameters are: type, order, ip, username, address, password. Line 931: AUTO_MIGRATE_DISABLED=Cannot migrate - check relevant configuration options. Line 932: AUTO_MIGRATE_VDS_NOT_FOUND=Cannot migrate - Host not found. http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties: Line 620: VDS_ALERT_FENCE_NO_PROXY_HOST=There is no other host in the data center that can be used to test the power management settings. Line 621: VDS_ALERT_FENCE_STATUS_VERIFICATION_FAILED=Failed to verify Host ${Host} ${Status} status, Please ${Status} Host ${Host} manually. Line 622: VDS_ALERT_SECONDARY_AGENT_USED_FOR_FENCE_OPERATION=Secondary fence agent was used to ${Operation} Host ${VdsName} Line 623: VDS_ALERT_PM_HEALTH_CHECK_FENCING_AGENT_NON_RESPONSIVE=Health check on Host ${VdsName} indicates that Fencing-Agent ${AgentId} is non-responsive. Line 624: VDS_ALERT_PM_HEALTH_CHECK_START_MIGHT_FAIL=Health check on Host ${VdsName} indicates that future attempts to Start this host using Power-Management are expected to fail. Please remove spaces at end Line 625: VDS_ALERT_PM_HEALTH_CHECK_STOP_MIGHT_FAIL=Health check on Host ${VdsName} indicates that future attempts to Stop this host using Power-Management are expected to fail. Line 626: VDS_ALERT_PM_HEALTH_CHECK_RESTART_MIGHT_FAIL=Health check on Host ${VdsName} indicates that future attempts to Restart this host using Power-Management are expected to fail. Line 627: VDS_ALERT_FENCE_OPERATION_SKIPPED_BROKEN_CONNECTIVITY=Host ${VdsName} became non responsive and was not restarted due to Fencing Policy: ${Percents} percents of the Hosts in the Cluster have connectivity issues. Line 628: VDS_ALERT_NOT_RESTARTED_DUE_TO_POLICY=Host ${VdsName} became non responsive and was not restarted due to the Cluster Fencing Policy. http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Please remove spaces at ends of changed lines Line 1: <?xml version="1.0" encoding="UTF-8" standalone="yes"?> Line 2: <xs:schema version="1.0" xmlns:xs="http://www.w3.org/2001/XMLSchema"; Line 3: xmlns:jaxb="http://java.sun.com/xml/ns/jaxb"; Line 4: xmlns:xjc="http://java.sun.com/xml/ns/jaxb/xjc"; http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/utils/AgentComparator.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/utils/AgentComparator.java: Line 6: Line 7: public class AgentComparator implements Comparator<Agent> { Line 8: Line 9: @Override Line 10: public int compare(Agent agent1, Agent agent2) { Please use Integer.compareTo() Line 11: if (agent1.getOrder() < agent2.getOrder()) { Line 12: return -1; Line 13: } else if (agent1.getOrder() == agent2.getOrder()) { Line 14: return 0; http://gerrit.ovirt.org/#/c/27578/18/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostModel.java: Line 1802: getPmSecondaryUserName().setEntity(secondaryAgent.getUser()); Line 1803: getPmSecondaryPassword().setEntity(secondaryAgent.getPassword()); Line 1804: getPmSecondaryType().setSelectedItem(secondaryAgent.getType()); Line 1805: setPmSecondaryOptionsMap(secondaryAgent.getOptionsMap()); Line 1806: getPmSecondaryConcurrent().setEntity(secondaryAgent.getOrder() == primaryAgent.getOrder()); If you really need to user Integer instead of int, then you should use equals() instead of == Line 1807: } Line 1808: } Line 1809: getDisableAutomaticPowerManagement().setEntity(vds.isDisablePowerManagementPolicy()); Line 1810: getPmKdumpDetection().setEntity(vds.isPmKdumpDetection()); -- To view, visit http://gerrit.ovirt.org/27578 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id0b384347a52db8b0aa6ae684fad40a5491dd2f7 Gerrit-PatchSet: 18 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ori Liel <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
