Liron Ar has posted comments on this change.
Change subject: core: Move VDS to Maintenance only if StopSPM is successful
......................................................................
Patch Set 4:
(5 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());
I'm in favour of 1 - I don't see whats the problem with having the remove in
two places rather then create another arraylist just for the iternation..IMO,
that's the reason that you can iterate and remove.
actually IMO - it even makes more sense to change it here and not within
setVdsStatusToPrepareForMaintenance which as it names states, should take care
of changing the status, please reconsider it.
Regardless, if you do insist - you can create the arraylist with the values
passed to the constructor rather then have 2 calls (create with size and then
addAll).
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) {
....................................................
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;
I don't think that it's matter of style, that's part of the language
definition, there's no need to add it to each c'tor.
Line 21: }
Line 22:
Line 23: public SetVdsStatusVDSCommandParameters(Guid vdsId, VDSStatus
status, NonOperationalReason nonOperationalReason) {
Line 24: this(vdsId, status);
....................................................
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}
we have 2 relevant cases
1. spmStop was executed on VDSM and failed
2. spmStop wasn't executed because there were async tasks.
we can't distinguish between the 2 cases with that message.
....................................................
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);
elaborated on previous file.
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);
Martin, you didn't understand my comment.
currently the update here updates other things but the status -
all other things should be still updated to avoid regressions unless you are
sure that it's 100% safe to remove it.
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 <mper...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Martin Peřina <mper...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
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
