Daniel Erez has posted comments on this change. Change subject: engine: Add Upgrade host command ......................................................................
Patch Set 2: (10 comments) Ravi, your input would be highly appreciated for this patch. https://gerrit.ovirt.org/#/c/40462/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/host/HostUpgradeCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/host/HostUpgradeCallback.java: Line 1: package org.ovirt.engine.core.bll.host; please add logs where needed Line 2: Line 3: import java.util.List; Line 4: import java.util.concurrent.Future; Line 5: Line 21: * The callback is being polling till the host move to maintenance or failed to do so. Line 22: */ Line 23: public void doPolling(Guid cmdId, List<Guid> childCmdIds) { Line 24: Line 25: if (!isMaintenanceCommandExecuted(childCmdIds.get(0))) { I would claim that the callback might be executed prior to the command being completed. meaning: 1. the command is persisted 2. some business in the command 2.1. some business in the command that persist a child command 3. the callback is called (since the command was persisted and polled) 4. some more business in the command between 1 and 3 there is a potential devastating NPE due to none existence of 2.1. You should protect the childCmdIds from being empty. Ravi, could you affirm the above ? Line 26: return; Line 27: } Line 28: Line 29: CommandBase<UpdateVdsActionParameters> command = CommandCoordinatorUtil.retrieveCommand(cmdId); Line 40: /** Line 41: * Invoke the upgrade action Line 42: */ Line 43: case Maintenance: Line 44: Future<VdcReturnValueBase> upgradeCmd = i wonder if the onSucceeded is more appropriated for this action. Ravi, what is your opinion ? Line 45: CommandCoordinatorUtil.executeAsyncCommand(VdcActionType.UpgradeHostInternal, Line 46: parameters, Line 47: command.cloneContextAndDetachFromParent()); Line 48: try { Line 45: CommandCoordinatorUtil.executeAsyncCommand(VdcActionType.UpgradeHostInternal, Line 46: parameters, Line 47: command.cloneContextAndDetachFromParent()); Line 48: try { Line 49: VdcReturnValueBase result = upgradeCmd.get(); what would happen if the engine crashes at this point ? the installation process will be terminated and will be reattempted in the next engine restart. IINM, it would be better executing this from the onSuccessful so it will guarantee to execute once and only once. Line 50: command.setCommandStatus(result.getSucceeded() ? CommandStatus.SUCCEEDED : CommandStatus.FAILED); Line 51: } catch (Exception e) { Line 52: command.setCommandStatus(CommandStatus.FAILED); Line 53: } Line 52: command.setCommandStatus(CommandStatus.FAILED); Line 53: } Line 54: break; Line 55: Line 56: default: please add explanation for the 'default' Line 57: command.setCommandStatus(CommandStatus.FAILED); Line 58: break; Line 59: Line 60: } https://gerrit.ovirt.org/#/c/40462/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdeploy/UpgradeHostCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdeploy/UpgradeHostCommand.java: Line 52: Line 53: return validate(validator.hostExists()) Line 54: && validate(validator.statusSupportedForHostUpgrade()) Line 55: && validate(validator.updatesAvailable()) Line 56: && validate(validator.imageProvidedForOvirtNode(getParameters().getoVirtIsoFile())); i would claim that upgrade of ovirt node should verify that the image *ACTUALLY* upgrades the existing version. Else, the user should use the reinstall action. Line 57: } Line 58: Line 59: @Override Line 60: protected void setActionMessageParameters() { Line 79: removeCommand(); Ravi, is it an appropriate usage of this method ? it lacks documentation and I'm not sure if it is the responsibility of the command to remove itself and its children. https://gerrit.ovirt.org/#/c/40462/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdeploy/UpgradeHostInternalCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdeploy/UpgradeHostInternalCommand.java: Line 33: private VdsDynamicDAO hostDao; Line 34: Line 35: private VDS host; Line 36: Line 37: public UpgradeHostInternalCommand(T parameters) { IMHO2: you must have here compensation for the command since you enter it with "Maintenance" status and you might end it unexpectedly (i.e. engine crash/earthquake/Godzilla eating the network cable) and the host will remain in 'Installing' status which isn't recoverable by the monitoring thread. Line 38: super(parameters); Line 39: } Line 40: Line 41: public UpgradeHostInternalCommand(T parameters, CommandContext cmdContext) { https://gerrit.ovirt.org/#/c/40462/2/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 129: VAR__ACTION__SYNC, Line 130: VAR__ACTION__GET_PUB_KEY, Line 131: VAR__ACTION__WRITE_PUB_KEYS, Line 132: VAR__ACTION__REGISTER, Line 133: VAR__ACTION__UPGRADE, please add the description in the relevant resources files. Line 134: Line 135: // Host statuses replacements Line 136: VAR__HOST_STATUS__UP, Line 137: VAR__HOST_STATUS__UP_MAINTENANCE_OR_NON_OPERATIONAL, Line 380: VDS_SECURITY_CONNECTION_ERROR(ErrorType.CONFLICT), Line 381: VDS_REMOVE_FENCE_AGENT_ID_REQUIRED(ErrorType.BAD_PARAMETERS), Line 382: VDS_REMOVE_FENCE_AGENTS_VDS_ID_REQUIRED(ErrorType.BAD_PARAMETERS), Line 383: VDS_ADD_FENCE_AGENT_MANDATORY_PARAMETERS_MISSING(ErrorType.BAD_PARAMETERS), Line 384: NO_AVAILABLE_UPDATES_FOR_HOST(ErrorType.CONFLICT), as above same below Line 385: IMAGE_NOT_PROVIDED_FOR_OVIRT_NODE_UPGRADE(ErrorType.BAD_PARAMETERS), Line 386: VAR__ACTION__MANUAL_FENCE, Line 387: VAR__ACTION__MAINTENANCE, Line 388: ACTION_TYPE_FAILED_PM_ENABLED_WITHOUT_AGENT(ErrorType.BAD_PARAMETERS), -- To view, visit https://gerrit.ovirt.org/40462 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a1447f3ebb5a7e3a67a8c68d449ae61e29f97fc Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
