Martin Peřina has posted comments on this change.
Change subject: core: Move VDS to Maintenance only if StopSPM is successful
......................................................................
Patch Set 4:
(7 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MaintenanceNumberOfVdssCommand.java
Line 59: List<VDS> spms = new ArrayList<>();
Line 60:
Line 61: // copy VDSs to new list since we will remove those on which
Stop SPM command fails
Line 62: List<VDS> vdss = new ArrayList<>(vdssToMaintenance.size());
Line 63: vdss.addAll(vdssToMaintenance.values());
Liron, please take a look at the code. Even if I add iterator over here, I will
have two choices:
1) Handle removal here, so I will have to modify return value of
setVdsStatusToPrepareForMaintenance() method. But then removal from
vdssToMaintenance would be on two places, since I will have to add removal also
on line 74.
2) Pass iterator instance into setVdsStatusToPrepareForMaintenance() method and
execute removal inside method. But using this case, I will have to solve also
execution of the method from line 74, where I don't iterate vdssToMaintenance
So IMO my solution is better.
Line 64:
Line 65: for (VDS vds : vdss) {
Line 66: // SPMs will move to Prepare For Maintenance later after
standard hosts
Line 67: if (vds.getSpmStatus() != VdsSpmStatus.SPM) {
Line 129: @Override
Line 130: protected void executeCommand() {
Line 131: MoveVdssToGoingToMaintenanceMode();
Line 132: MigrateAllVdss();
Line 133: // set network to operational / non-operational
Sorry I missed this one. I will investigate it.
Line 134: for (Guid id : _vdsGroupIds) {
Line 135: List<Network> networks =
DbFacade.getInstance().getNetworkDao().getAllForCluster(id);
Line 136: for (Network net : networks) {
Line 137: NetworkClusterHelper.setStatus(id, net);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/SetVdsStatusVDSCommandParameters.java
Line 16: public SetVdsStatusVDSCommandParameters(Guid vdsId, VDSStatus
status) {
Line 17: super(vdsId);
Line 18: _status = status;
Line 19: nonOperationalReason = NonOperationalReason.NONE;
Line 20: logStopSpmFailure = false;
Sorry, but I don't like this style. Constructor is the place, where instance
attributes should receive its initial value. I don't like relying on the
default, it's much less readable and maintainable.
Line 21: }
Line 22:
Line 23: public SetVdsStatusVDSCommandParameters(Guid vdsId, VDSStatus
status, NonOperationalReason nonOperationalReason) {
Line 24: this(vdsId, status);
Line 42: public void setNonOperationalReason(NonOperationalReason
nonOperationalReason) {
Line 43: this.nonOperationalReason = (nonOperationalReason == null ?
NonOperationalReason.NONE : nonOperationalReason);
Line 44: }
Line 45:
Line 46: public boolean shouldLogStopSpmFailure() {
Ok
Line 47: return logStopSpmFailure;
Line 48: }
Line 49:
Line 50: public void setLogStopSpmFailure(boolean logStopSpmFailure) {
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 703: DISK_ALIGNMENT_SCAN_START=Starting alignment scan of disk
'${DiskAlias}'.
Line 704: DISK_ALIGNMENT_SCAN_FAILURE=Alignment scan of disk '${DiskAlias}'
failed.
Line 705: DISK_ALIGNMENT_SCAN_SUCCESS=Alignment scan of disk '${DiskAlias}' is
complete.
Line 706: VM_MEMORY_NOT_IN_RECOMMENDED_RANGE=VM ${VmName} was configured with
${VmMemInMb}mb of memory while the recommended value range is ${VmMinMemInMb}mb
- ${VmMaxMemInMb}mb
Line 707: FAILED_TO_STOP_SPM_ON_HOST=Failed to stop SPM running on host
${VdsName}
Please suggest better message then. Because IMO we send StopSPM command, but it
failed because of running async tasks.
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/SetVdsStatusVDSCommand.java
Line 40: // log to audit log if SpmStop command failed and
SpmStop result was not marked as ignored
Line 41: if (!getVDSReturnValue().getSucceeded() &&
getParameters().shouldLogStopSpmFailure()) {
Line 42: AuditLogableBase base = new AuditLogableBase();
Line 43: base.setVds(vds);
Line 44: AuditLogDirector.log(base,
AuditLogType.FAILED_TO_STOP_SPM_ON_HOST);
Please elaborate as I don't understand your concern. Because IMO ResetIrs
failure is logged only if logging it is enabled. If ResetIrs executes
successfully, nothing will be logged.
Line 45: }
Line 46: }
Line 47:
Line 48: // set status to Prepare for Maintenance for SPM only if
ResetIrs command was successful
Line 46: }
Line 47:
Line 48: // set status to Prepare for Maintenance for SPM only if
ResetIrs command was successful
Line 49: if (getVDSReturnValue().getSucceeded()) {
Line 50: updateVdsFromParameters(parameters, vds);
Well, getVDSReturnValue().setSucceeded(true) is executed just before
executeVdsCommand(), so if ResetIrs is not executed or executed successfully ,
db update is executed. Otherwise db update is not executed, because we cannot
change status to Prepare for Maintenance since ResetIrs failure.
Line 51: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
Line 52:
Line 53: @Override
Line 54: public Void runInTransaction() {
--
To view, visit http://gerrit.ovirt.org/21231
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c58f9a9629d2e7a496f02c4dececeb842d44543
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Peřina <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Martin Peřina <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
