Arik Hadas has posted comments on this change. Change subject: core: fix removal of attached disks to stateless vm ......................................................................
Patch Set 3: (3 comments) Maor, this command is called automatically when the VM went down so we can't really ask the user to detach the disks. We also prefer not to postpone it - it's better to remove the stateless snapshot when we detect the VM went down instead of when running it (for pre-started VMs in pool for example). It also makes sense because we promise the user to restore the previous state of stateless VM when it went down. since it didn't have those disks attached, we detach them. we still keep our promise when doing it automatically :) http://gerrit.ovirt.org/#/c/36035/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreStatelessVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreStatelessVmCommand.java: Line 71: Line 72: Set<Guid> disksWithStatelessSnapshot = new HashSet<>(); Line 73: for (DiskImage statelessDiskSnapshot : statelessDiskSnapshots) { Line 74: disksWithStatelessSnapshot.add(statelessDiskSnapshot.getId()); Line 75: } > Please use Entities.getIds(...) Done Line 76: Line 77: for (DiskImage activeDiskSnapshot : activeDiskSnapshots) { Line 78: if (!disksWithStatelessSnapshot.contains(activeDiskSnapshot.getId())) { Line 79: VdcReturnValueBase returnValue = runInternalAction ( Line 78: if (!disksWithStatelessSnapshot.contains(activeDiskSnapshot.getId())) { Line 79: VdcReturnValueBase returnValue = runInternalAction ( Line 80: VdcActionType.DetachDiskFromVm, Line 81: new AttachDetachVmDiskParameters( Line 82: getVmId(), activeDiskSnapshot.getId(), false, false)); > I assume that we want that when a failure occurs the disks which were alrea actually I don't care about detached disks being attached again as part of the rollback. note that the VM is down and no operation is made in VDSM, so it's not an expensive operation Line 83: Line 84: if (!returnValue.getSucceeded()) { Line 85: return false; Line 86: } Line 81: new AttachDetachVmDiskParameters( Line 82: getVmId(), activeDiskSnapshot.getId(), false, false)); Line 83: Line 84: if (!returnValue.getSucceeded()) { Line 85: return false; > I think we don't have an audit log indicating that we failed to detach a Di I see that we do have audit log in DetachDiskFromVmCommand, I don't see a reason why wouldn't it be logged, is there a reason? Anyway, note that it's just a DB operation, no real reason for it to fail. Regarding the log - Done Line 86: } Line 87: } Line 88: } Line 89: -- To view, visit http://gerrit.ovirt.org/36035 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3a1d6eb8d2f4606622c7ed5c73370792406bb9b3 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@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
