Allon Mureinik has posted comments on this change.
Change subject: core: Check all attached VMs when updating shared disk boot flag
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(6 inline comments)
....................................................
File backend/manager/dbscripts/all_disks_sp.sql
Line 56: Create or replace FUNCTION GetVmBootDisk(v_vm_guid UUID) RETURNS SETOF
all_disks AS $procedure$
Line 57: BEGIN
Line 58: RETURN QUERY SELECT all_disks.*
Line 59: FROM all_disks
Line 60: LEFT JOIN vm_device ON vm_device.device_id =
all_disks.image_group_id
1. The left outer join is probably wrong here, just like it is in the original
function you copied from. Not sure if this should be part of this patch or not,
though.
2. Regarding image_group_id - this is in fact the disk_id. I'm guessing it's a
misguided backwards compatibility issue, which should be fixed (the name, that
is), but definitely not in this patch.
3. IIUC, You should also filter according to plugged disks only - I don't think
we care about having 124135626 boot disks if none of them are plugged. Having
this in mind, I wonder if the way to go is to add v_only_boot to
GetDisksVmGuid, and just create a bunch of syntactic sugar (java) methods on
top of it.
Line 61: WHERE vm_device.vm_id = v_vm_guid and boot = true;
Line 62: END; $procedure$
Line 63: LANGUAGE plpgsql;
Line 64:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 129: }
Line 130:
Line 131: // Validate update boot disk.
Line 132: if (newDisk.isBoot()) {
Line 133: if (!isVMsContainingBootableDisks(newDisk,
vmsDiskPluggedTo)) {
This is a very confusing function name ("if no VM contains a boot disk fail".
Huh?!).
Line 134: return false;
Line 135: }
Line 136: }
Line 137:
Line 268: public AuditLogType getAuditLogTypeValue() {
Line 269: return getSucceeded() ? AuditLogType.USER_UPDATE_VM_DISK :
AuditLogType.USER_FAILED_UPDATE_VM_DISK;
Line 270: }
Line 271:
Line 272: private boolean isVMsContainingBootableDisks(Disk diskInfo,
List<VM> vms) {
this name makes it VERY confusing to read the CDA.
It should look something like:
if (!validate(noVmsContainBootableDisks)) {
return false;
}
Line 273: boolean vmsContainBootDisk = false;
Line 274: StringBuilder vmsContainingBootDisk = new StringBuilder();
Line 275:
Line 276: for (VM vm : vms) {
Line 275:
Line 276: for (VM vm : vms) {
Line 277: Disk bootDisk = getDiskDao().getVmBootDisk(vm.getId());
Line 278: if (bootDisk != null) {
Line 279: if (vmsContainBootDisk) { // In case this is not the
first VM to fail, add delimiter
ichs. Just save them all to a list and use StringUtils.join
Line 280: vmsContainingBootDisk.append(", ");
Line 281: }
Line 282: vmsContainingBootDisk.append(vm.getName());
Line 283: vmsContainBootDisk = true;
Line 287: if (vmsContainBootDisk) {
Line 288:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VMS_BOOT_IN_USE);
Line 289: getReturnValue().getCanDoActionMessages().add(
Line 290: String.format("$VmsName %1$s",
vmsContainingBootDisk.toString()));
Line 291: return false;
Return a ValidationResult
Line 292: }
Line 293:
Line 294: return true;
Line 295: }
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskDaoTest.java
Line 174: Disk bootDisk =
dao.getVmBootDisk(FixturesTool.VM_RHEL5_POOL_57);
Line 175: assertNotNull("VM should have a boot disk attached",
bootDisk);
Line 176: assertEquals("Wrong boot disk for VM", bootDisk.getId(),
FixturesTool.BOOTABLE_DISK_ID);
Line 177: }
Line 178:
Please add a negative test too - a VM without a boot disk
Line 179: /**
Line 180: * Asserts the result of {@link DiskImageDAO#getAllForVm(Guid)}
contains the correct disks.
Line 181: * @param disks
Line 182: * The result to check
--
To view, visit http://gerrit.ovirt.org/14709
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I82ee07e02e08d60f559017d9f8205ab7df41c5c3
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
Gerrit-Reviewer: liron aravot <liron.ara...@gmail.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
