[Engine-patches] Change in ovirt-engine[master]: engine: quota support for multiple storage images

[mlipchuk at redhat.com]

Maor Lipchuk has posted comments on this change.

Change subject: engine: quota support for multiple storage images
......................................................................


Patch Set 9: (8 inline comments)

Hi Michael, please see my comments inline.
I think using a list for images will gain us more flexibility advantage.
The list should be used also to pass type (raw / preallocated) when creating a VM from template, and can be used for other commands and operations as well

....................................................
File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java
Line 374:             return QuotaManager.validateMultiStorageQuota(getStoragePool().getQuotaEnforcementType(),
For now this list is only for quota, since I didn't want to get into integration problems with the GUI if I will use it also for destination storage.

For the long run we should need a list of disks that will contain all the disk parameters, such as quota id, storage id, tyep...

....................................................
File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java
Line 313:     private Map<Pair<Guid, Guid>, Double> getQuotaConsumeMap() {
I agree, although it will still not cover all the commands.
Unfourtuntily AddVmTemplate or CommonVmPoolWithVmsCommand for example don't have common inherited class with AddVmFromSnapshotCommand beside commandBase which I don't want to get it dirty with quota.

Maybe its worth to move this specific method to VmManagementCommandBase, thanks.

....................................................
File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommand.java
Line 82:     protected boolean validateQuota() {
Agreed

Line 85:                 getStoragePool()));
The disks are initialized from the GUI with destination quota id, that is why I need them from the GUI.

Agreed that the diskInfoList should be synchronized with imageToDestinationDomainMap here, will be fixed on the next patch.

....................................................
File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java
Line 103:         // Set default quota id if storage pool enforcement is disabled.
There should not be use of getParameters() here, only getQuotaId and setQuotaId. Will be changed in next patch.

....................................................
File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AddVmPoolWithVmsParameters.java
Line 22:     private ArrayList<DiskImage> diskInfoDestinationList;
This is for getting the quota destination for each image when creating a pool (same like storage domain), 

and I use a list since for  the long run we should need a list of disks that will contain all the disk parameters, such as quota id, storage id, type etc..

....................................................
File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AddVmTemplateParameters.java
Line 28:     private HashMap<Guid, DiskImage> diskInfoDestinationMap;
Since on  the long run, we should need a list of disks that will contain all the disk parameters, I think its more flexible this way.
For now quota id and storage id can be kept with pair object, but what will happen when we will need another attribute to be configured from the GUI.
We will then need to change the type to List or Map. and IMHO maintaining this will be more difficult then using simply a DiskImage map.

....................................................
File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmManagementParametersBase.java
Line 23:     private ArrayList<DiskImage> diskInfoDestinationList;
This is a parameter class, for VMs basically, there are no child classes sharing this parameter.
all the other parameter classes (such as template and pool) don't have a parameter class in common.

--
To view, visit http://gerrit.ovirt.org/3090
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4b7e28fb9dd3069e86bd8fdf41255398fd1e58
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchaplik at redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Michael Kublin <mkublin at redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofrenkel at redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs at redhat.com>