Michael Kublin has posted comments on this change.
Change subject: core: ImagesHandler: Remove SD status validation
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(14 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
Line 291: &&
validate(snapshotValidator.vmNotDuringSnapshot(getVmId()))
Line 292: &&
validate(snapshotValidator.vmNotInPreview(getVmId()))
Line 293: && validate(vmValidator.vmNotDuringMigration())
Line 294: && validate(vmValidator.vmNotRunningStateless())
Line 295: && validate(vmValidator.vmNotIlegal())
Like I wrote a comment inside a some next class, the following change will
increase a number of queries for storage domains
Line 296: && validateStorageDoaminsActive(disksList)
Line 297: && ImagesHandler.PerformImagesChecks(
Line 298: getReturnValue().getCanDoActionMessages(),
Line 299: getVm().getStoragePoolId(),
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java
Line 151:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW);
Line 152: return false;
Line 153: }
Line 154:
Line 155: // Check the source storage domains exist and are up
These defiantly can be a method or method inside some util
Line 156: for (Guid sdId :
ImagesHandler.getAllDomainsIds(getDisksBasedOnImage())) {
Line 157: StorageDomainValidator sdValidator = new
StorageDomainValidator(getStorageDomain(sdId));
Line 158: if (validate(sdValidator.isDomainExistAndActive())) {
Line 159: return false;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java
Line 38: public class RemoveSnapshotCommand<T extends RemoveSnapshotParameters>
extends VmCommand<T>
Line 39: implements QuotaStorageDependent {
Line 40:
Line 41: private static final long serialVersionUID = 3162100352844371734L;
Line 42: private List<DiskImage> _sourceImages = null;
name can be by java convention
Line 43: private List<DiskImage> _imagesForValidation = null;
Line 44:
Line 45: public RemoveSnapshotCommand(T parameters) {
Line 46: super(parameters);
Line 83: * In the future, this should be revisited and checked if it can
be united with {@link #getSourceImages()}
Line 84: */
Line 85: protected List<DiskImage> getImagesForValidation() {
Line 86: if (_imagesForValidation == null) {
Line 87: _imagesForValidation =
getDiskDao() can be used
Line 88:
ImagesHandler.filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(getVmId()),
Line 89: true,
Line 90: false);
Line 91: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java
Line 68: public NGuid getStorageDomainId() {
Line 69: return getParameters() != null ?
!Guid.Empty.equals(getParameters().getStorageDomainId()) ? getParameters()
Line 70: .getStorageDomainId() : super.getStorageDomainId() :
super.getStorageDomainId();
Line 71: }
Line 72:
Why public?
Line 73: public storage_domains getStorageDomain(Guid domainId) {
Line 74: return getStorageDomainDAO().getForStoragePool(domainId,
getStoragePoolId());
Line 75: }
Line 76:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/TryBackToAllSnapshotsOfVmCommand.java
Line 214: updateVmDisksFromDb();
Line 215: Collection<DiskImage> diskImages =
Line 216:
ImagesHandler.filterImageDisks(getVm().getDiskMap().values(), false, true);
Line 217: if (!diskImages.isEmpty()) {
Line 218: if (!validate(new
StoragePoolValidator(getStoragePool()).isUp())
The following change is increasing number of queries performed to DB. Actually
the number of queries will be increased at all places where status of domain
and space are checked together
Line 219: || !validateStorageDoaminsActive(diskImages)
Line 220: || !ImagesHandler.PerformImagesChecks(
Line 221:
getReturnValue().getCanDoActionMessages(),
Line 222: getVm().getStoragePoolId(),
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java
Line 408: }
Line 409: }
Line 410: return true;
Line 411: }
Line 412:
I think I saw that piece of code couple of times
Line 413: protected boolean
validateStorageDoaminsActive(Collection<DiskImage> disksList) {
Line 414: for (Guid sdId : ImagesHandler.getAllDomainsIds(disksList)) {
Line 415: StorageDomainValidator sdValidator = new
StorageDomainValidator(getStorageDomain(sdId));
Line 416: if (validate(sdValidator.isDomainExistAndActive())) {
Line 418: }
Line 419: }
Line 420: return true;
Line 421: }
Line 422:
Maybe that method can be moved to some parent class?
Line 423: protected storage_domains getStorageDomain(Guid domainId) {
Line 424: return getStorageDomainDAO().getForStoragePool(domainId,
getStoragePoolId());
Line 425: }
Line 426:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java
Line 212: if (!spUpResult.isValid()) {
Line 213: messages.add(spUpResult.getMessage().name());
Line 214: returnValue = false;
Line 215: }
Line 216:
By the way here is a bug, imgaes of vm can be located on different storages and
not only on the same one .
Guid storageDomainId = vmImages.size() > 0 ?
vmImages.get(0).getstorage_ids().get(0)
: Guid.Empty
Line 217: if (!Guid.Empty.equals(storageDomainId)) {
Line 218: storage_domains domain =
DbFacade.getInstance().getStorageDomainDao().getForStoragePool(
Line 219: storageDomainId, vm.getStoragePoolId());
Line 220: ValidationResult sdUpResult = new
StorageDomainValidator(domain).isDomainExistAndActive();
Line 213: messages.add(spUpResult.getMessage().name());
Line 214: returnValue = false;
Line 215: }
Line 216:
Line 217: if (!Guid.Empty.equals(storageDomainId)) {
Can be inside a method
Line 218: storage_domains domain =
DbFacade.getInstance().getStorageDomainDao().getForStoragePool(
Line 219: storageDomainId, vm.getStoragePoolId());
Line 220: ValidationResult sdUpResult = new
StorageDomainValidator(domain).isDomainExistAndActive();
Line 221: if (!sdUpResult.isValid()) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java
Line 118:
message.add(spUpResult.getMessage().name());
Line 119: retValue = false;
Line 120: }
Line 121: }
Line 122:
These can be inside some method
Line 123: List<DiskImage> vmDiskImages =
ImagesHandler.filterImageDisks(vmDisks, true, false);
Line 124: if (retValue && isRunInvokedByUser(vm,
runParams)) {
Line 125: for (Guid sdId :
ImagesHandler.getAllDomainsIds(vmDiskImages)) {
Line 126: storage_domains sd =
Line 248: * Check isValid, storageDomain and diskSpace only if VM is not
HA VM
Line 249: */
Line 250: protected boolean performImageChecksForRunningVm
Line 251: (VM vm, List<String> message, RunVmParams runParams,
List<? extends Disk> vmDisks) {
Line 252: return ImagesHandler.PerformImagesChecks(message,
same here, we will retrieve a storage domain twice. The scenario of multiple vm
runs already had suffered from too big number of queries to DB, the number of
queries should be removed and not increased
Line 253: vm.getStoragePoolId(), Guid.Empty,
!vm.isAutoStartup(),
Line 254: true, false, false,
Line 255: isRunInvokedByUser(vm, runParams),
Line 256: vmDisks);
Line 256: vmDisks);
Line 257: }
Line 258:
Line 259: /** @return Is the VM run due to a user request (i.e., not due to
the HA mechanism */
Line 260: private static boolean isRunInvokedByUser(VM vm, RunVmParams
runParams) {
Why it is static?
Line 261: return !vm.isAutoStartup() || !runParams.getIsInternal() &&
vm.isAutoStartup();
Line 262: }
Line 263:
Line 264: /**
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java
Line 206:
jobProperties.put(VdcObjectType.VmTemplate.name().toLowerCase(),
getVmTemplateName());
Line 207: }
Line 208: return jobProperties;
Line 209: }
Line 210:
where it is used?
Line 211: protected storage_domains getStorageDomain(Guid domainId) {
Line 212: return getStorageDomainDAO().getForStoragePool(domainId,
getStoragePoolId());
Line 213: }
--
To view, visit http://gerrit.ovirt.org/11487
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id632168da64832a9c9393b81e552112240e974c0
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
