Liron Aravot has posted comments on this change.
Change subject: webadmin: Allow remove of a VM without removing its disks
......................................................................
Patch Set 1: (9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java
Line 71: VM vm = getVm();
Line 72: Guid vmId = getVmId();
Line 73: hasImages = vm.getDiskList().size() > 0;
Line 74:
Line 75: removeVmInSpm(vm.getStoragePoolId(), vmId);
you can wrap the whole block with if (hasImages &&
getParameters().isRemoveDisks())) or export it to a boolean...
Line 76: if (getParameters().isRemoveDisks() && hasImages &&
!removeVmImages(null)) {
Line 77: return false;
Line 78: }
Line 79:
Line 105:
Line 106: // TODO: Why is this needed? Legacy? Workaround?
Line 107: setDescription(getVmName());
Line 108:
Line 109: if (!getParameters().isRemoveDisks() &&
!canRemoveVmWithDetachDisks()) {
it should be done before canRemoveVm() that will issue vdsm call..please
remember to also load the vm disks before.. VmHandler.updateVmDisksFromDb and
to remove the load from canRemoveVm()
Line 110: return false;
Line 111: }
Line 112:
Line 113: return true;
Line 175:
Line 176: return true;
Line 177: }
Line 178:
Line 179: private boolean canRemoveVmWithDetachDisks() {
test to this method?
Line 180: if (!NGuid.Empty.equals(getVm().getVmtGuid())) {
Line 181: // return failCanDoAction(message)
Line 182: }
Line 183:
Line 176: return true;
Line 177: }
Line 178:
Line 179: private boolean canRemoveVmWithDetachDisks() {
Line 180: if (!NGuid.Empty.equals(getVm().getVmtGuid())) {
we use Guid.Empty as convention..please change to it..it'll save you an import
as well :)
Line 181: // return failCanDoAction(message)
Line 182: }
Line 183:
Line 184: boolean isSnapshotsPresent = false;
Line 181: // return failCanDoAction(message)
Line 182: }
Line 183:
Line 184: boolean isSnapshotsPresent = false;
Line 185: for (Disk disk : getVm().getDiskList()) {
getVm().getDiskList() returns list of DiskImage...no need to check and cast.
Line 186: if (disk.getDiskStorageType() == DiskStorageType.IMAGE) {
Line 187: if (((DiskImage) disk).getSnapshots().size() > 1) {
Line 188: isSnapshotsPresent = true;
Line 189: break;
Line 183:
Line 184: boolean isSnapshotsPresent = false;
Line 185: for (Disk disk : getVm().getDiskList()) {
Line 186: if (disk.getDiskStorageType() == DiskStorageType.IMAGE) {
Line 187: if (((DiskImage) disk).getSnapshots().size() > 1) {
are you sure that the disks snapshots were loaded before we got here?
Line 188: isSnapshotsPresent = true;
Line 189: break;
Line 190: }
Line 191: }
Line 184: boolean isSnapshotsPresent = false;
Line 185: for (Disk disk : getVm().getDiskList()) {
Line 186: if (disk.getDiskStorageType() == DiskStorageType.IMAGE) {
Line 187: if (((DiskImage) disk).getSnapshots().size() > 1) {
Line 188: isSnapshotsPresent = true;
how about return true here and avoid if on line 194? (just add the message
there)
Line 189: break;
Line 190: }
Line 191: }
Line 192: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveVmParameters.java
Line 2:
Line 3: import org.ovirt.engine.core.compat.Guid;
Line 4:
Line 5: public class RemoveVmParameters extends VmOperationParameterBase
implements java.io.Serializable {
Line 6: private static final long serialVersionUID = -6256468461166321723L;
need to be generated again...
Line 7: private boolean force;
Line 8: private boolean removeDisks = true;
Line 9:
Line 10:
Line 29: setForce(force);
Line 30: }
Line 31:
Line 32: public RemoveVmParameters(Guid vmId, boolean force, boolean
removeDisks) {
Line 33: super(vmId);
how about call the other ctor and then set removeDisks
Line 34: setForce(force);
Line 35: setRemoveDisks(removeDisks);
Line 36: }
Line 37:
--
To view, visit http://gerrit.ovirt.org/10376
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba531f7a87295564e20bd99763d185f78e483a44
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
