Liron Aravot has posted comments on this change. Change subject: core: OVFs that aren't stored on any domain should be stored on all ......................................................................
Patch Set 1: (9 comments) http://gerrit.ovirt.org/#/c/34292/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessOvfUpdateForStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessOvfUpdateForStorageDomainCommand.java: Line 199: getStorageDomainDAO().getVmAndTemplatesIdsByStorageDomainId(getParameters().getStorageDomainId(), Line 200: false, Line 201: false); Line 202: Line 203: vmAndTemplatesIds.addAll(getVmStaticDAO().getVmAndTemplatesIdsWithoutAttachedImageDisks(false, false)); > Consider to re-extract line 198-204 to a different method which will be cal I'll do that in a following patch, let's keep the changes here to minimal Line 204: Line 205: byte[] bytes = buildOvfInfoFileByteArray(vmAndTemplatesIds); Line 206: Line 207: Pair<StorageDomainOvfInfo, DiskImage> lastOvfStoreForUpdate = domainOvfStoresInfoForUpdate.getLast(); http://gerrit.ovirt.org/#/c/34292/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmStaticDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmStaticDAO.java: Line 93: */ Line 94: public void remove(Guid id, boolean removePermissions); Line 95: Line 96: /** Line 97: * Retrieves all ids of vms and templates that have no attached disks matching the provided criteria. > /s/all/all the Done Line 98: * @param shareableDisks check for attached shareable disks Line 99: * @param snapshotDisks check for attached snapshotdisks Line 100: */ Line 101: public List<Guid> getVmAndTemplatesIdsWithoutAttachedImageDisks(boolean shareableDisks, boolean snapshotDisks); http://gerrit.ovirt.org/#/c/34292/1/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java: Line 252: * </ul> Line 253: */ Line 254: protected static final Guid VM_TEMPLATE_RHEL6_2 = new Guid("1b85420c-b84c-4f29-997e-0eb674b40b82"); Line 255: Line 256: protected static final Guid VM_TEMPLATE_WITHOUT_DISKS = new Guid("1b85420c-b84c-4f29-997e-0eb674b40b80"); > Please remove this constant and add this information to the java doc of VM_ weird ide bug, i searched for that id in the file. removed. Line 257: Line 258: protected static final Guid VM_WITHOUT_DISKS = new Guid("77296e00-0cad-4e5a-9299-008a7b6f4356"); Line 259: Line 260: /** Line 254: protected static final Guid VM_TEMPLATE_RHEL6_2 = new Guid("1b85420c-b84c-4f29-997e-0eb674b40b82"); Line 255: Line 256: protected static final Guid VM_TEMPLATE_WITHOUT_DISKS = new Guid("1b85420c-b84c-4f29-997e-0eb674b40b80"); Line 257: Line 258: protected static final Guid VM_WITHOUT_DISKS = new Guid("77296e00-0cad-4e5a-9299-008a7b6f4356"); > No need to add an existing guid to the fixtures tool. weird ide bug, i searched for that id in the file. removed. Line 259: Line 260: /** Line 261: * Predefined template version for testing with the following properties : Line 262: * <ul> http://gerrit.ovirt.org/#/c/34292/1/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmStaticDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmStaticDAOTest.java: Line 205: } Line 206: Line 207: Line 208: @Test Line 209: public void getVmAndTemplatesIdsWithoutAttachedImageDisks() { > Suggestion: I would also change the name to be something more shorter like I prefer to keep the current name as it's clearer imo. Line 210: List<Guid> ids = Line 211: dao.getVmAndTemplatesIdsWithoutAttachedImageDisks(false, false); Line 212: assertTrue(ids.contains(FixturesTool.VM_WITHOUT_DISKS)); Line 213: assertTrue(ids.contains(FixturesTool.VM_TEMPLATE_WITHOUT_DISKS)); Line 210: List<Guid> ids = Line 211: dao.getVmAndTemplatesIdsWithoutAttachedImageDisks(false, false); Line 212: assertTrue(ids.contains(FixturesTool.VM_WITHOUT_DISKS)); Line 213: assertTrue(ids.contains(FixturesTool.VM_TEMPLATE_WITHOUT_DISKS)); Line 214: assertFalse(ids.contains(FixturesTool.VM_TEMPLATE_RHEL5)); > Perhaps you should also add tests for shareable and snapshot disks Done Line 215: } Line 216: Line 217: @Test Line 218: public void testGetAllNamesPinnedToHostReturnsNothingForRandomHost() throws Exception { http://gerrit.ovirt.org/#/c/34292/1/packaging/dbscripts/storages_sp.sql File packaging/dbscripts/storages_sp.sql: Line 277: Line 278: Line 279: Line 280: Line 281: > will be removed on next patchset Done Line 282: Line 283: Create or replace FUNCTION Getstorage_poolsByVdsId(v_vdsId UUID) Line 284: RETURNS SETOF storage_pool STABLE Line 285: AS $procedure$ http://gerrit.ovirt.org/#/c/34292/1/packaging/dbscripts/vms_sp.sql File packaging/dbscripts/vms_sp.sql: Line 592: Line 593: Line 594: Line 595: Line 596: Create or replace FUNCTION GetVmAndTemplatesIdsWithoutAttachedImageDisks(v_shareable BOOLEAN, v_active BOOLEAN) > /s/Vm/VMs Done Line 597: RETURNS SETOF UUID STABLE Line 598: AS $procedure$ Line 599: BEGIN Line 600: RETURN QUERY SELECT vs.vm_guid Line 601: FROM vm_static vs Line 602: WHERE vs.vm_guid NOT IN (SELECT DISTINCT vd.vm_id Line 603: FROM vm_device vd INNER JOIN images_storage_domain_view i Line 604: ON i.image_group_id = vd.device_id Line 605: WHERE i.active = v_active AND i.shareable = v_shareable); > in the future we will want that shareable disks will also be included in th when it'll be needed we can change it, as of now i added it just to not have that logic hidden in the stored procedure. Line 606: END; $procedure$ Line 607: LANGUAGE plpgsql; Line 608: Line 609: -- To view, visit http://gerrit.ovirt.org/34292 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaeecc6c4526800656315842384d83830fdf0b72b Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@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
