Liron Ar has posted comments on this change.

Change subject: core: Move VDS to Maintenance only if StopSPM is successful
......................................................................


Patch Set 2:

(2 comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MaintenanceNumberOfVdssCommand.java
Line 59:         }
Line 60:     }
Line 61: 
Line 62:     private void MoveVdssToGoingToMaintenanceMode() {
Line 63:         List<VDS> nonSpms = new LinkedList<>();
Martin, perhaps you missed my point.
The previous code already changes to non spms status first and then pass the 
spms to the status change, the logic that you introduced does the same, 
therefore unless i'm missing something this refactor is unneeded.
Line 64:         List<VDS> spms = new LinkedList<>();
Line 65:         for(VDS vds : vdssToMaintenance.values()) {
Line 66:             if (vds.getSpmStatus() == VdsSpmStatus.SPM) {
Line 67:                 spms.add(0, vds);


Line 94:             if (vds.getStatus() != VDSStatus.PreparingForMaintenance 
&& vds.getStatus() != VDSStatus.NonResponsive
Line 95:                     && vds.getStatus() != VDSStatus.Down) {
Line 96:                 VDSReturnValue result = 
runVdsCommand(VDSCommandType.SetVdsStatus,
Line 97:                         new 
SetVdsStatusVDSCommandParameters(vds.getId(), 
VDSStatus.PreparingForMaintenance));
Line 98:                 if (!result.getSucceeded()) {
"Do you have any idea how to test this case without blocking the host with 
iptables?"

- I suggest to test the bug scenario, have a spm with tasks and see that it's 
status doesn't change..as it seems to me, it will change.

Logging - can't you just move this logging code to there? you don't log "this" 
(the object) but creating a new one, so u should be fine :)
Line 99:                     // remove VDS from base list, because of an error 
during SetVdsStatus
Line 100:                     vdssToMaintenance.remove(vds.getId());
Line 101: 
Line 102:                     AuditLogableBase base = new AuditLogableBase();


-- 
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: 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: 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

Reply via email to