Martin Peřina has posted comments on this change. Change subject: core: Fix skip fencing if host connected to storage ......................................................................
Patch Set 2: (4 comments) http://gerrit.ovirt.org/#/c/36581/2/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 207: fenceAgent.getId()); Line 208: } else if (wasSkippedDueToPolicy(fenceExecutionResult)) { Line 209: // fencing execution was skipped due to fencing policy Line 210: return fenceExecutionResult; Line 211: } else { > Agreed, this does look like a bug :) This is not a bug, we need to distinguish between two skipping statuses: 1. If StopVDS was skipped, because VDS is already OFF, then we can continue restart with StartVDS. 2. If StopVDS was skipped, because VDS is still connected to storage, then we have to abort whole NonRespondingTreatment process. I don't want to do any big refactoring here now, we know that whole code is messy and we are going to refactor it later in 3.6. But currently we broke "skip fencing if connected to storage" feature and IMHO it should be fixed ASAP. And this patch do exactly that: it just adds code that was removed by mistake during fence agents refactoring. Line 212: if (fenceExecutionResult.getSucceeded()) { Line 213: boolean requiredStatusAchieved = waitForStatus(); Line 214: int i = 0; Line 215: while (!requiredStatusAchieved && i < retries) { http://gerrit.ovirt.org/#/c/36581/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestartVdsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestartVdsCommand.java: Line 51: public class RestartVdsCommand<T extends FenceVdsActionParameters> extends VdsCommand<T> { Line 52: Line 53: private static final String INTERNAL_FENCE_USER = "Engine"; Line 54: Line 55: protected boolean skippedDueToFencingPolicy; > I think something is wrong with the use of variable. 1. VdsNotRespondingTreatmentCommand.executeCommand() calls super.executeCommand() 2. RestartVdsCommand can detect if StopVdsCommand was skipped using result and wasSkippedDueToPolicy method() 3. So if StopVds was skipped due to fencing, I will set skippedDueToFencingPolicy attribute to true, so it can be evaluated back in VdsNotRespondingTreatmentCommand instance Line 56: Line 57: /** Line 58: * Constructor for command creation when compensation is applied on startup Line 59: * Line 211: if (fenceReturnValue.getReturnValue() instanceof FenceStatusReturnValue) { Line 212: skipped = ((FenceStatusReturnValue) fenceReturnValue.getReturnValue()).getIsSkipped(); Line 213: } Line 214: } Line 215: return skipped; > Indeed, looks like a bug I don't see any bug here. Line 216: } Line 217: Line 218: @Override Line 219: public String getUserName() { http://gerrit.ovirt.org/#/c/36581/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/StopVdsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/StopVdsCommand.java: Line 160: VDSFenceReturnValue result = getMostRelevantResult(results); Line 161: Line 162: if (result != null) { Line 163: if (result.getSucceeded()) { Line 164: int numOfSkipped = countSkippedOperations(results); > I tend to agree with Oved, I don't think this situation can ever happen (on Eli, are we 100% sure that above situation will never happen? If so, I will happily remove the code :-) Line 165: if (numOfSkipped == 0) { Line 166: // no agent reported that stop operation was skipped, continue with stop operation Line 167: handleSpecificCommandActions(); Line 168: } else { -- To view, visit http://gerrit.ovirt.org/36581 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84fa56e75adf988ce36b1154269dab8974cf05bf Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@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
