Vered Volansky has posted comments on this change. Change subject: core: Add memory allocation check to hibernate VM ......................................................................
Patch Set 8: (5 comments) http://gerrit.ovirt.org/#/c/30883/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HibernateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HibernateVmCommand.java: Line 92: @Override Line 93: public Guid getStorageDomainId() { Line 94: if (_storageDomainId.equals(Guid.Empty) && getVm() != null) { Line 95: List<DiskImage> diskDummiesForMemSize = createDiskDummies(); Line 96: StorageDomain storageDomain = (new VmHandler()).findStorageDomainForMemory(getVm().getStoragePoolId(), diskDummiesForMemSize); > All the methods in VmHandler are static. Why not call VmHandler.findStorage Yes, residue from a not static findStorageDomainForMemory iteration (for tests). Line 97: if (storageDomain != null) { Line 98: _storageDomainId = storageDomain.getId(); Line 99: } Line 100: } http://gerrit.ovirt.org/#/c/30883/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java: Line 741: } Line 742: Line 743: private static void updateDisksStorageType(StorageType storageType, List<DiskImage> disksList) { Line 744: VolumeType volumeType = storageType.isFileDomain() ? VolumeType.Sparse : VolumeType.Preallocated; Line 745: //Only memory disk, which is added first, should be modified. > I don't understand this comment. disksList contains memory and metadata images dummies, in this order. For metadata VolumeForamat is already set to sparse. I did add disk aliases to dummies for clarity and considered using these for filtering here, but seemed like TREFA VENEVELA situation to me. Line 746: disksList.get(0).setVolumeType(volumeType); Line 747: } Line 748: Line 749: protected static boolean doesStorageDomainHaveSpaceForRequest(StorageDomain storageDomain, List<DiskImage> disksList) { Line 745: //Only memory disk, which is added first, should be modified. Line 746: disksList.get(0).setVolumeType(volumeType); Line 747: } Line 748: Line 749: protected static boolean doesStorageDomainHaveSpaceForRequest(StorageDomain storageDomain, List<DiskImage> disksList) { > Wouldn't we want to return the ValidationResult instead of just a boolean? And do what with it? public method returns storage domain, can't add this to CDA at this point. Note there is another CDA message stating there's no good domain for the request in HibernateVmCommand. Line 750: // not calling validate in order not to add the messages per domain Line 751: return (new StorageDomainValidator(storageDomain).hasSpaceForClonedDisks(disksList)).isValid(); Line 752: } Line 753: http://gerrit.ovirt.org/#/c/30883/8/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/HibernateVmCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/HibernateVmCommandTest.java: Line 98: Line 99: @Test Line 100: public void validateThreshold() { Line 101: doReturn(ValidationResult.VALID).when(storageDomainValidator).isDomainWithinThresholds(); Line 102: assertTrue(command.validateDomainWithinThresholds()); > please use the new form: Done Line 103: } Line 104: Line 105: @Test Line 106: public void validateSpaceNotWithinThreshold() throws Exception { Line 105: @Test Line 106: public void validateSpaceNotWithinThreshold() throws Exception { Line 107: doReturn((new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN))). Line 108: when(storageDomainValidator).isDomainWithinThresholds(); Line 109: assertFalse(command.validateDomainWithinThresholds()); > please use the new format: Done Line 110: } Line 111: } -- To view, visit http://gerrit.ovirt.org/30883 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1250e6ea8d67f8026d66cf06589538343d39756a Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
