Liron Ar has posted comments on this change. Change subject: core: Add snapshot to the OVF generation ......................................................................
Patch Set 4: (6 comments) partially reviewed. http://gerrit.ovirt.org/#/c/25947/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java: Line 370: List<VM> vms = getVmDao().getVmsByIds(idsToProcess); Line 371: for (VM vm : vms) { Line 372: if (VMStatus.ImageLocked != vm.getStatus()) { Line 373: updateVmDisksFromDb(vm); Line 374: ArrayList<DiskImage> vmDisks = updateSnapshotsForVM(vm); 1. the name isn't good enough, it's vmImages. 2. let's not make unneeded db calls, we can load the disks, check it and only if they are fine load the snapshots and volumes. it can be changed into: updateVmDisksFromDb(vm); if (!verifyDisksNotLocked(vm.getDiskList() { continue; } updateSnapshotsForVM(vm); if (!verifySnapshotsStatus) { continue; } OVF GENERATION HERE etc.. Line 375: if (verifyDisksNotLocked(vm.getDiskList()) && verifySnapshotsStatus(vm.getSnapshots())) { Line 376: loadVmData(vm); Line 377: Long currentDbGeneration = getVmStaticDao().getDbGeneration(vm.getId()); Line 378: // currentDbGeneration can be null in case that the vm was deleted during the run of OvfDataUpdater. Line 387: } Line 388: return vmsAndTemplateMetadata; Line 389: } Line 390: Line 391: private ArrayList<DiskImage> updateSnapshotsForVM(VM vm) { please separate the image loading/snapshot loading into different functions. Line 392: vm.setSnapshots(getSnapshotDao().getAllWithConfiguration(vm.getId())); Line 393: ArrayList<DiskImage> vmDisks = new ArrayList<>(); Line 394: List<DiskImage> filteredDisks = ImagesHandler.filterImageDisks(vm.getDiskList(), false, true, true); Line 395: Line 394: List<DiskImage> filteredDisks = ImagesHandler.filterImageDisks(vm.getDiskList(), false, true, true); Line 395: Line 396: for (DiskImage diskImage : filteredDisks) { Line 397: vmDisks.addAll(getAllImageSnapshots(diskImage)); Line 398: } please export this logic in lines 392-298 to a method and reuse it in lines 494-498 Line 399: return vmDisks; Line 400: } Line 401: Line 402: protected void proccessDisksDomains(List<DiskImage> disks) { Line 419: Line 420: /** Line 421: * Returns true if all snapshots have a valid status to use in the OVF. Line 422: */ Line 423: protected boolean verifySnapshotsStatus(List<Snapshot> snapshots) { IMO you should export this check to a new validation in SnapshotValidator - you can take example from there snapshotTypeSupported() method there (perhaps add snapshotStatusSupported()) Line 424: for (Snapshot snapshot : snapshots) { Line 425: if (snapshot.getStatus() != SnapshotStatus.OK && snapshot.getStatus() != SnapshotStatus.BROKEN) { Line 426: return false; Line 427: } Line 489: /** Line 490: * Adds the given vm metadata to the given map Line 491: */ Line 492: protected String buildMetadataDictionaryForVm(VM vm, Line 493: Map<Guid, KeyValuePairCompat<String, List<Guid>>> metaDictionary) { if this logic is relevant only for export vm, please move this logic there and leave here only the relevant code - IMO it's not readable. Line 494: ArrayList<DiskImage> AllVmImages = new ArrayList<DiskImage>(); Line 495: List<DiskImage> filteredDisks = ImagesHandler.filterImageDisks(vm.getDiskList(), false, true, true); Line 496: Line 497: for (DiskImage diskImage : filteredDisks) { http://gerrit.ovirt.org/#/c/25947/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/OvfDataUpdaterTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/OvfDataUpdaterTest.java: Line 151: Line 152: // dao related mocks. Line 153: doReturn(1L).when(vmStaticDAO).getDbGeneration(any(Guid.class)); Line 154: List<Snapshot> snapshots = new ArrayList<Snapshot>(); Line 155: doReturn(snapshots).when(snapshotDAO).getAllWithConfiguration(any(Guid.class)); please add tests that actually test what you added (for example, results for different snapshot status) Line 156: mockAnswers(); Line 157: } Line 158: Line 159: private void initMembers() { -- To view, visit http://gerrit.ovirt.org/25947 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ba05c2e362fb9b22d6b4fb5a14698ad99d7bd1e Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: [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
