Moti Asayag has posted comments on this change. Change subject: engine: Add Upgrade host command ......................................................................
Patch Set 2: (9 comments) 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 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))) { > The doPolling is invoked after the command execute method has been complete Done Line 26: return; Line 27: } Line 28: Line 29: CommandBase<UpdateVdsActionParameters> command = CommandCoordinatorUtil.retrieveCommand(cmdId); Line 42: */ Line 43: case Maintenance: Line 44: Future<VdcReturnValueBase> upgradeCmd = Line 45: CommandCoordinatorUtil.executeAsyncCommand(VdcActionType.UpgradeHostInternal, Line 46: parameters, > The parameters for HostUpgradeCommand is being directly passed here. The pr Done Line 47: command.cloneContextAndDetachFromParent()); Line 48: try { Line 49: VdcReturnValueBase result = upgradeCmd.get(); Line 50: command.setCommandStatus(result.getSucceeded() ? CommandStatus.SUCCEEDED : CommandStatus.FAILED); Line 45: CommandCoordinatorUtil.executeAsyncCommand(VdcActionType.UpgradeHostInternal, Line 46: parameters, Line 47: command.cloneContextAndDetachFromParent()); Line 48: try { Line 49: VdcReturnValueBase result = upgradeCmd.get(); > upgradeCmd.get() does not block for the command to complete execution. The according to the javado of Future.get: "Waits if necessary for the computation to complete, and then retrieves its result." what can be done instead is querying "isDone()" and if not to continue polling the callback. However, that would introduce more complexity that i wouldn't like to. If there a downside of leaving it as is ? Does hogging this thread can cause an issue ? Assuming in the future we'll try to upgrade several hosts at once (i.e. all hosts that are currently on maintenance). If we go with 'isDone' and release this thread, will relying on the number of child commands to be 2 will be the correct indicator that UpgradeHostInternal was invoked ? How would i know to examine the correct type of each command ? 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' Done 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 *ACTU this is a correct assumption, but wouldn't it be better to allow even the re-install and to leverage this functionality for that purpose ? Line 57: } Line 58: Line 59: @Override Line 60: protected void setActionMessageParameters() { Line 75: try { Line 76: result = maintenanceCmd.get(); Line 77: if (!result.getSucceeded()) { Line 78: // cancel the callback invocation Line 79: removeCommand(); > removeCommand should not be invoked from here. The callback should take cas Done Line 80: propagateFailure(result); Line 81: return; Line 82: } Line 83: } catch (InterruptedException | ExecutionException e) { 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 w You convinced me with Godzilla :-) I'll add compensation for this command. That will become even more handy when we move to Cluster Upgrade Manager, where several hosts might be doomed for eternity "Installing" status. 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. Done 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 Done 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: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> 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
