Ravi Nori has posted comments on this change. Change subject: engine: Add Upgrade host command ......................................................................
Patch Set 2: (5 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))) { > I would claim that the callback might be executed prior to the command bein The doPolling is invoked after the command execute method has been completed. But I agree that it should be protected for empty list 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. I am fine with it being executed from here Line 45: CommandCoordinatorUtil.executeAsyncCommand(VdcActionType.UpgradeHostInternal, Line 46: parameters, Line 47: command.cloneContextAndDetachFromParent()); Line 48: try { 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 problem is that the commandId is already set in the parameters, so the newly executed command will have the same id as HostUpgradeCommand. Please clone the parameters with out copying commandId and pass to executeAsyncCommand 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 command is executed async. In the parameters being passed to executeAsyncCommand set parent command id and parent command parameters to HostUpgradeCommand's values. Then in the next iteration you can monitor if host upgrade has completed successfully by polling. The child command id (UpgradeHostInternal command id) should be added to childCmdIds passed to doPolling. Line 50: command.setCommandStatus(result.getSucceeded() ? CommandStatus.SUCCEEDED : CommandStatus.FAILED); Line 51: } catch (Exception e) { Line 52: command.setCommandStatus(CommandStatus.FAILED); Line 53: } 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 75: try { Line 76: result = maintenanceCmd.get(); Line 77: if (!result.getSucceeded()) { Line 78: // cancel the callback invocation Line 79: removeCommand(); > Ravi, is it an appropriate usage of this method ? it lacks documentation an removeCommand should not be invoked from here. The callback should take case of setting status to SUCCEEDED/FAILED and should be responsible form removing the command and its children Line 80: propagateFailure(result); Line 81: return; Line 82: } Line 83: } catch (InterruptedException | ExecutionException e) { -- 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: 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
