Liron Ar has posted comments on this change.
Change subject: core: Add a validation when deactivate ISO domain.
......................................................................
Patch Set 12:
(2 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DeactivateStorageDomainCommand.java
Line 151: if (getVmDynamicDAO().get(vmStatic.getId()).getStatus()
!= VMStatus.Down
Line 152: &&
VmDeviceUtils.isVMHasAttachedISO(vmStatic.getId())) {
Line 153: vmNames.add(vmStatic.getName());
Line 154: }
Line 155: }
1. this query can potentially huge amounts of data - most of the db is vms
obviously as it's the main focus of the system.
Loading all the VMS of the storage pool with all they dynamic information as
well in addition to all their disk devices (in the called method) will be like
50% of the db usually imo.
loading domains for the pool is less harmful, as usually we will have much less
domains then vms - not that i'm saying that other optimization can't be done.
2. generally thread safety is important here same as it's important on other
cases..only when we run vm when using iso it should be blocked on this
operation and then fail and same goes for this operation.
Line 156: return vmNames;
Line 157: }
Line 158:
Line 159: /**
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
Line 110: VmDeviceGeneralType.DISK);
Line 111: for (VmDevice device : deviceList) {
Line 112: if
(device.getDevice().equals(VmDeviceType.CDROM.toString())
Line 113: ||
device.getDevice().equals(VmDeviceType.FLOPPY.toString())) {
Line 114: String path = (String)
device.getSpecParams().get("path");
1. The user can have a hook to add a cdrom to vms that he runs
in that case the cdrom/floppy might be added as unamanaged,
we handled a similar issue with added disks.
in that case, the floppy/cdrom might be unmanaged (which means that they were
added from the info returned to VdsUpdateRuntimeInfo) and that they aren't
related to the engine.
If in that case those unamanged cdrom/floppy will be added with "path" as spec
param this fix would fail - if this is the case we might check the path (i'm
not sure what we put there) as first option, if we can't make sense out of it
alteast if the device is managed or not.
I'm saying that it should be checked if it can cause to wrong validation
results on your fix (this is just an idea).
2. About plugged, this check should return if the vm it plugged only, if it has
only attached one it's fine.
Line 115: if (!StringUtils.isEmpty(path)) {
Line 116: return true;
Line 117: }
Line 118: }
--
To view, visit http://gerrit.ovirt.org/20331
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I47c1a8155762ecd0b04bb17676151946982bb919
Gerrit-PatchSet: 12
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Cheryn Tan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
