Eli Mesika has posted comments on this change. Change subject: restapi: Host Power-Management Refactor (#977674) - WIP ......................................................................
Patch Set 18: (29 comments) Allover the code : fencing should be only fence , this includes class names , methods , variables , comments etc 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 313: Line 314: private void addFencingAgentsToDb() { Line 315: if (getParameters().getFencingAgents() != null && !getParameters().getFencingAgents().isEmpty()) { Line 316: for (FencingAgent agent : getParameters().getvds().getFencingAgents()) { Line 317: DbFacade.getInstance().getFencingAgentDao().save(agent); Liran wrote an infrastructure for batch DB operations , I think it cam be used here as well instead calling the save for each iteration Line 318: } Line 319: } Line 320: } Line 321: 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. Agree, but since this appears in original code , fix should be done in a separate patch if needed and not as part of this one 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 See my comment 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 142: if (proxyHost != null) { Line 143: break; Line 144: } Line 145: // do not retry getting proxy for Status operation. Line 146: if (!withRetries) There is a patch that adds 3 retries for temporary status failures, I see that you are still not rebased on it but when you do that, you should re-factor your code here as well and the withRetries will be very confusing .... So after rebase , I think the original code is more clear in that manner Line 147: break; Line 148: log.infoFormat("Attempt {0} to find fence proxy host failed...", ++count); Line 149: try { Line 150: Thread.sleep(delayInMs); Line 171: private void logProxySelection(String proxy, String origin) { Line 172: AuditLogableBase logable = new AuditLogableBase(); Line 173: logable.addCustomValue("Proxy", proxy); Line 174: logable.addCustomValue("Origin", origin); Line 175: logable.setVdsId(_vds.getId()); why did you remove the "command" from logging , it will say if that was a start, stop or status .... Line 176: AuditLogDirector.log(logable, AuditLogType.PROXY_HOST_SELECTION); Line 177: log.infoFormat("Using Host {0} from {1} as proxy for Host {2}", Line 178: proxy, origin, _vds.getName()); Line 179: } 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 124: addCanDoActionMessage(VdcBllMessages.VDS_NO_VDS_PROXY_FOUND); Line 125: } Line 126: } else { Line 127: addCanDoActionMessage(VdcBllMessages.VDS_FENCE_DISABLED_AT_QUIET_TIME); Line 128: addCanDoActionMessage(String.format("$seconds %1$s", secondsLeftToNextPmOp)); why did you added this format , as far as I see this is not used like that and the original was OK please do git grep addCanDoActionMessageVariable and see the correct usage Line 129: } Line 130: } else { Line 131: addCanDoActionMessage(VdcBllMessages.VDS_FENCE_DISABLED_AT_SYSTEM_STARTUP_INTERVAL); Line 132: } Line 181: AlertIfPowerManagementOperationFailed(); Line 182: } Line 183: } Line 184: // Successful fencing with reboot or shutdown op. Clear the power management policy flag Line 185: else if ((getParameters().getAction() == FenceActionType.Restart what is the change here , as far as I see this is identical to original except maybe white-spaces Line 186: || getParameters().getAction() == FenceActionType.Stop) Line 187: && getParameters().getKeepPolicyPMEnabled() == false) { Line 188: getVds().setPowerManagementControlledByPolicy(false); Line 189: getDbFacade().getVdsDynamicDao().updateVdsDynamicPowerManagementPolicyFlag( Line 306: Line 307: /** Line 308: * Attempt to 'start' the host using the provided agents. This method receives several agents which have the same Line 309: * 'order' (thus considered 'concurrent'), but actually runs them sequentially, because in the case of 'start' Line 310: * concurrency is unnecessary. In case of concurrent, bot stop and start should be performed concurrently You are mixing here the agent behavior which should be concurrent with the expected result which succeeded for stop if all agents returned OK or for start when one is enough to start the host , but this should not influence the way you call the agent which should be always concurrent exactly as in the original code Line 311: * Line 312: * @return 'true' if fencing succeeded; 'false' otherwise. Line 313: */ Line 314: private boolean handleConcurrentStart(List<FencingAgent> agents, FenceActionType action) { http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetNewVdsFenceStatusQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetNewVdsFenceStatusQuery.java: Line 36: } Line 37: Line 38: private void handleError(VDSFenceReturnValue result) { Line 39: if (!result.isProxyHostFound()) { Line 40: getQueryReturnValue().setSucceeded(false); why did you removed : final String UNKNOWN = "unknown"; better to define once and use the constant Line 41: getQueryReturnValue().setReturnValue(new FenceStatusReturnValue("unknown", Line 42: AuditLogDirector.getMessage(AuditLogType.VDS_ALERT_FENCE_NO_PROXY_HOST))); Line 43: } else { Line 44: getQueryReturnValue().setSucceeded(false); http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RegisterVdsQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RegisterVdsQuery.java: Line 251: Line 252: UpdateVdsActionParameters p = new UpdateVdsActionParameters(vdsByUniqueId.getStaticData(), "", false); Line 253: if (vdsByUniqueId.isFencingAgentsExist()) { Line 254: p.setFencingAgents(vdsByUniqueId.getFencingAgents()); Line 255: } // TODO: what about agent deletion? what about this TODO :-) Line 256: p.setTransactionScopeOption(TransactionScopeOption.RequiresNew); Line 257: VdcReturnValueBase rc = Backend.getInstance().runInternalAction(VdcActionType.UpdateVds, p); Line 258: Line 259: if (rc == null || !rc.getSucceeded()) { Line 364: parameters.setShouldBeLogged(false); Line 365: parameters.setTransactionScopeOption(TransactionScopeOption.RequiresNew); Line 366: if (vds_byHostName.isFencingAgentsExist()) { Line 367: parameters.setFencingAgents(vds_byHostName.getFencingAgents()); Line 368: } // TODO: what about agent deletion? same Line 369: Line 370: // If host exists in InstallingOs status, remove it from DB and move on Line 371: final VDS foundVds = DbFacade.getInstance().getVdsDao().getByName(parameters.getVdsStaticData().getName()); Line 372: if ((foundVds != null) && (foundVds.getDynamicData().getStatus() == VDSStatus.InstallingOS)) { Line 455: UpdateVdsActionParameters parameters = Line 456: new UpdateVdsActionParameters(hostToRegister.getStaticData(), "", false); Line 457: if (hostToRegister.isFencingAgentsExist()) { Line 458: parameters.setFencingAgents(hostToRegister.getFencingAgents()); Line 459: } // TODO: what about agent deletion? same Line 460: VdcReturnValueBase ret = Backend.getInstance().runInternalAction(VdcActionType.UpdateVds, parameters); Line 461: if (ret == null || !ret.getSucceeded()) { Line 462: error = AuditLogType.VDS_REGISTER_ERROR_UPDATING_NAME; Line 463: logable.addCustomValue("VdsName2", newName); http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java: Line 88: VdsKdumpDetection(132, QuotaDependency.NONE), Line 89: AddFencingAgent(133, ActionGroup.EDIT_HOST_CONFIGURATION, QuotaDependency.NONE), Line 90: RemoveFencingAgent(134, ActionGroup.EDIT_HOST_CONFIGURATION, QuotaDependency.NONE), Line 91: RemoveFencingAgentsByVdsId(136, ActionGroup.EDIT_HOST_CONFIGURATION, QuotaDependency.NONE), Line 92: UpdateFencingAgent(135, ActionGroup.EDIT_HOST_CONFIGURATION, QuotaDependency.NONE), please order , first 135 and after it 136 Line 93: Line 94: // Network Line 95: UpdateNetworkToVdsInterface(149, ActionGroup.CONFIGURE_HOST_NETWORK, QuotaDependency.NONE), Line 96: AttachNetworkToVdsInterface(150, ActionGroup.CONFIGURE_HOST_NETWORK, QuotaDependency.NONE), 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 Agree, there was a patch by me for 3.2 that renamed all fencing to fence ..... Line 16: Line 17: private static final long serialVersionUID = -5910560758520427911L; Line 18: private Guid id; Line 19: private Guid hostId; http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 343: VDS_CANNOT_AUTHENTICATE_TO_SERVER(ErrorType.CONFLICT), Line 344: VDS_SECURITY_CONNECTION_ERROR(ErrorType.CONFLICT), Line 345: VDS_REMOVE_FENCING_AGENT_ID_REQUIRED(ErrorType.BAD_PARAMETERS), Line 346: VDS_REMOVE_FENCING_AGENTS_VDS_ID_REQUIRED(ErrorType.BAD_PARAMETERS), Line 347: VDS_ADD_FENCING_AGENT_MANDATORY_PARAMETERS_MISSING(ErrorType.BAD_PARAMETERS), on all FENCING => FENCE Line 348: VAR__ACTION__MANUAL_FENCE, Line 349: VAR__ACTION__MAINTENANCE, Line 350: ACTION_TYPE_FAILED_PM_ENABLED_WITHOUT_AGENT(ErrorType.BAD_PARAMETERS), Line 351: ACTION_TYPE_FAILED_PM_ENABLED_WITHOUT_AGENT_CREDENTIALS(ErrorType.BAD_PARAMETERS), http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java: Line 53: GetServerSSHPublicKey, Line 54: GetServerSSHKeyFingerprint, Line 55: GetCpuStatisticsByVdsId, Line 56: GetFencingAgentById, Line 57: GetFencingAgentsByVdsId, FENCING => FENCE Line 58: Line 59: // VdsStatic Queries Line 60: GetVdsStaticByName, Line 61: http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AlertDirector.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AlertDirector.java: Line 23: public static void Alert(AuditLogableBase auditLogable, AuditLogType logType, String message) { Line 24: AuditLogDirector.log(auditLogable, logType, message); Line 25: } Line 26: Line 27: public static void AddVdsAlert(Guid vdsId, AuditLogType type) { why this is needed ??? Line 28: AddVdsAlert(vdsId, type, new AuditLogableBase()); Line 29: } Line 30: Line 31: /** Line 48: public static void RemoveVdsAlert(Guid vdsId, AuditLogType type) { Line 49: DbFacade.getInstance().getAuditLogDao().removeAllOfTypeForVds(vdsId, type.getValue()); Line 50: } Line 51: Line 52: public static void RemoveVdsAlert(Guid vdsId, AuditLogType type, AuditLogableBase alert) { same.... Line 53: // TODO: implement! //!! Line 54: } Line 55: Line 56: /** 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 b Ori, can this be filled only on demand by a separate DB call ??? Line 36: Line 37: @Override Line 38: public VDS get(Guid id) { Line 39: return get(id, null, false); Line 51: if (vdsList.size()==0) { Line 52: return null; Line 53: } else { Line 54: return uniteAgentsSingleVds(vdsList); Line 55: } please define as a separate method since this block is duplicated below Line 56: } Line 57: Line 58: @Override Line 59: public VDS getByName(String name) { http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/dal/src/main/jdbc-resources/engine-daos.properties File backend/manager/modules/dal/src/main/jdbc-resources/engine-daos.properties: Line 88: ExternalVariableDao=org.ovirt.engine.core.dao.ExternalVariableDaoDbFacadeImpl Line 89: VdsKdumpStatusDao=org.ovirt.engine.core.dao.VdsKdumpStatusDaoDbFacadeImpl Line 90: DiskProfileDao=org.ovirt.engine.core.dao.profiles.DiskProfileDaoDbFacadeImpl Line 91: CpuProfileDao=org.ovirt.engine.core.dao.profiles.CpuProfileDaoDbFacadeImpl Line 92: FencingAgentDAO=org.ovirt.engine.core.dao.FencingAgentDaoDbFacadeImpl Fencing => Fence 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 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. 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. On all fencing = > fence Line 931: AUTO_MIGRATE_DISABLED=Cannot migrate - check relevant configuration options. Line 932: AUTO_MIGRATE_VDS_NOT_FOUND=Cannot migrate - Host not found. Line 933: AUTO_MIGRATE_ALREADY_RUNNING_ON_VDS=Cannot migrate - VM already running on Host. Line 934: AUTO_MIGRATE_UNSUCCESSFUL=Cannot migrate - Previous migration was unsuccessful. http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties: Line 116: job.RemoveDiskSnapshots=Removing Disks from Snapshot(s) ${Snapshots} of VM ${VM} Line 117: job.VdsKdumpDetection=Detecting kdump flow on host ${VDS} Line 118: job.AddFencingAgent=Adding fencing agent to host ${VDS} Line 119: job.RemoveFencingAgent=Removing fencing agent from host ${VDS} Line 120: job.UpdateFencingAgent=Updating fencing agent of host ${VDS} same Fencing => Fence Line 121: Line 122: # Step types Line 123: step.VALIDATING=Validating Line 124: step.EXECUTING=Executing http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/FencingAgentResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/FencingAgentResource.java: Line 3: import org.ovirt.engine.api.model.Agent; Line 4: Line 5: public interface FencingAgentResource extends UpdatableResource<Agent> { Line 6: Line 7: } class name : Fencing => Fence http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/FencingAgentsResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/FencingAgentsResource.java: same , Fencing => Fence Line 1: package org.ovirt.engine.api.resource; Line 2: Line 3: import java.util.List; Line 4: http://gerrit.ovirt.org/#/c/27578/18/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/HostResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/HostResource.java: Line 95: @Path("permissions") Line 96: public AssignedPermissionsResource getPermissionsResource(); Line 97: Line 98: @Path("fencingagents") Line 99: public FencingAgentsResource getFencingAgentsResource(); same here http://gerrit.ovirt.org/#/c/27578/18/packaging/dbscripts/fencing_agents_sp.sql File packaging/dbscripts/fencing_agents_sp.sql: Line 36: v_vds_id UUID , Line 37: v_agent_order INTEGER , Line 38: v_ip VARCHAR(255) , Line 39: v_type VARCHAR(255) , Line 40: v_agent_user VARCHAR(255) , please remove all TWS from the code below Line 41: v_agent_password text , Line 42: v_options VARCHAR(255) , Line 43: v_port INTEGER) Line 44: RETURNS VOID http://gerrit.ovirt.org/#/c/27578/18/packaging/dbscripts/vds_sp.sql File packaging/dbscripts/vds_sp.sql: Line 395: Line 396: Create or replace FUNCTION InsertVdsStatic( Line 397: v_free_text_comment text, Line 398: v_vds_id UUID, Line 399: v_host_name VARCHAR(255), TWS Line 400: v_vds_unique_id VARCHAR(128) , Line 401: v_port INTEGER, Line 402: v_protocol SMALLINT, Line 403: v_vds_group_id UUID, Line 552: AS $procedure$ Line 553: BEGIN Line 554: RETURN QUERY SELECT vds_static.* Line 555: FROM vds_static Line 556: WHERE vds_id = v_vds_id; same Line 557: Line 558: RETURN; Line 559: END; $procedure$ Line 560: LANGUAGE plpgsql; -- 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 <ol...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
