Michael Kublin has posted comments on this change.
Change subject: core:WIP: introducing OvfAutoUpdate
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(22 inline comments)
Needs to be removed numbers of lines of code, remove one table. Increase
default update time out. Fix some queries, start to use JOIN
....................................................
File backend/manager/dbscripts/upgrade/03_01_1520_add_vm_generation_columns.sql
Line 10: vm_guid UUID PRIMARY KEY references vm_static(vm_guid),
Line 11: ovf_generation BIGINT default 1
Line 12: );
Line 13:
Line 14:
Why I need two tables. Just for example, I saw implementation of cluster wise
cache , it also managed some kind of version , and one table was enough for it,
I don't understand why u need two tables.
Line 15: INSERT into vm_db_generations (select vm_guid from vm_static);
Line 16:
Line 17: INSERT into vm_ovf_generations (select vm_guid from vm_static);
Line 18:
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 338: select
fn_db_add_config_value('OvirtIsoPrefix','ovirt-node','general');
Line 339: select
fn_db_add_config_value('oVirtISOsRepositoryPath','/usr/share/ovirt-node-iso','general');
Line 340: select
fn_db_add_config_value('oVirtUpgradeScriptName','/usr/share/vdsm-reg/vdsm-upgrade','general');
Line 341: select
fn_db_add_config_value('oVirtUploadPath','/data/updates/ovirt-node-image.iso','general');
Line 342: select
fn_db_add_config_value('OvfUpdateIntervalInMinutes','1','general');
1 minutes? Really ! What is a reason for such timeout?
Line 343: select
fn_db_add_config_value('OvfItemsCountPerUpdate','100','general');
Line 344: select fn_db_add_config_value('PayloadSize','8192','general');
Line 345: select fn_db_add_config_value('PosixStorageEnabled','false','2.2');
Line 346: select fn_db_add_config_value('PosixStorageEnabled','false','3.0');
....................................................
File backend/manager/dbscripts/vms_sp.sql
Line 18:
Line 19:
Line 20:
Line 21:
Line 22:
why I need these query, why it is not retrieved on object from view?
Line 23: Create or replace FUNCTION GetDbGeneration(v_vm_guid UUID)
Line 24: RETURNS BIGINT
Line 25: AS $procedure$
Line 26: BEGIN
Line 40: RETURNS VOID
Line 41: AS $procedure$
Line 42: BEGIN
Line 43: UPDATE db_generations
Line 44: SET db_generation = db_generation + 1
you not familiar with join? you can not join by vm_guid with vms_static ?
Line 45: WHERE vm_guid IN (SELECT vm_guid from vm_static WHERE
vds_group_id = v_storage_pool_id);
Line 46: END; $procedure$
Line 47: LANGUAGE plpgsql;
Line 48:
....................................................
File backend/manager/dbscripts/vm_templates_sp.sql
Line 222: DELETE FROM vm_static
Line 223: WHERE vm_guid = v_vmt_guid
Line 224: AND entity_type = 'TEMPLATE';
Line 225: -- delete Template permissions --
Line 226: DELETE FROM permissions where object_id = v_vmt_guid;
How after that you will retrieve ids of template that were removed?
Line 227: DELETE from vm_db_generations where vm_guid=v_vmt_guid;
Line 228: DELETE from vm_ovf_generations where vm_guid=v_vmt_guid;
Line 229: END; $procedure$
Line 230: LANGUAGE plpgsql;
Line 262:
Line 263:
Line 264:
Line 265:
Line 266:
Template should be already deleted from vm_static table, u should retrieve ids
from vm_db_... which is not in vm_static
Line 267: Create or replace FUNCTION
GetVmTemplatesIdsForOvfDeletion(v_storage_pool_id UUID) RETURNS SETOF UUID
Line 268: AS $procedure$
Line 269: BEGIN
Line 270: RETURN QUERY SELECT vm.vm_guid as vm_guid
Line 284: AS $procedure$
Line 285: BEGIN
Line 286: RETURN QUERY SELECT vm_templates.vm_guid as vm_guid
Line 287: FROM vm_static vm_templates, vm_db_ovf_generations generations
Line 288: WHERE generations.vm_guid = vm_templates.vmt_guid AND
generations.db_generation > generations.ovf_generation AND vds_group_id =
v_storage_pool_id;
where is entity type?
Line 289: END; $procedure$
Line 290: LANGUAGE plpgsql;
Line 291:
Line 292:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java
Line 111: log.debug(ex.getStackTrace());
Line 112: }
Line 113: }
Line 114: }
Line 115:
These should be written only with simple for.
Line 116: private void updateOvfForVmsOfStoragePool(Guid poolId) {
Line 117: // get vm ids that needs to be updated.
Line 118: List<Guid> vmsIdsForUpdate =
getVmDao().getVmsIdsForOvfUpdate(poolId);
Line 119:
Line 116: private void updateOvfForVmsOfStoragePool(Guid poolId) {
Line 117: // get vm ids that needs to be updated.
Line 118: List<Guid> vmsIdsForUpdate =
getVmDao().getVmsIdsForOvfUpdate(poolId);
Line 119:
Line 120: // get actual vms for update.
Regards comments, you should write a javadoc for method, what it suppose to do,
and comment every line. Also if you think that you should comment every line it
is means that code is unreadable and not understandable
Line 121: List<VM> vms = getVmsForUpdate(vmsIdsForUpdate);
Line 122: while (!vms.isEmpty() || !vmsIdsForUpdate.isEmpty()) {
Line 123: // build vms ovf
Line 124: getVmsMetadataDictionary(poolId, vms);
Line 123: // build vms ovf
Line 124: getVmsMetadataDictionary(poolId, vms);
Line 125: // update vms metadata in storage
Line 126: performOvfUpdateIfNeeded(poolId, false);
Line 127: // check if there are any other vms for update.
These is not a check, these a bad code. Check is if condition.
Line 128: vms.addAll(getVmsForUpdate(vmsIdsForUpdate));
Line 129: }
Line 130: }
Line 131:
Line 155:
//getVmAndTemplatesGenerationsDAO().DeleteOvfGenerations(idsForCurrentOperation);
Line 156: }
Line 157: }
Line 158:
Line 159:
I don't understand concept of force.
Line 160: private void performOvfUpdateIfNeeded(Guid poolId, boolean force)
{
Line 161: if (proccessedIdsInfo.size() == ITEMS_COUNT_PER_UPDATE ||
force) {
Line 162: // update ovf metadata in vdsm
Line 163: executeUpdateVmInSpmCommand(poolId,
vmsAndTemplateMetadata, Guid.Empty);
Line 162: // update ovf metadata in vdsm
Line 163: executeUpdateVmInSpmCommand(poolId,
vmsAndTemplateMetadata, Guid.Empty);
Line 164:
Line 165: // update vm/templates ovf generation to the db
generation that was updated in the storage.
Line 166:
getVmAndTemplatesGenerationsDAO().updateOvfGenerations(proccessedIdsInfo,
proccessedOvfGenerationsInfo);
If you want to clear a memory you can just do proccessedIdsInfo = null;
Line 167: proccessedIdsInfo.clear();
Line 168: proccessedOvfGenerationsInfo.clear();
Line 169: vmsAndTemplateMetadata.clear();
Line 170: }
Line 176: * @return
Line 177: */
Line 178: private List<Guid> getIdsToProcess(List<Guid> ids) {
Line 179: int size = (ids.size() >= ITEMS_COUNT_PER_UPDATE ?
ITEMS_COUNT_PER_UPDATE : ids.size());
Line 180: List<Guid> guidsToProcess = ids.subList(0, size);
These I really don't understand.
As I got you need to go throw array and work only on ITEMS_COUNT_PER_UPDATE
number of items, such code can be written by 5 lines of code.
Line 181: List<Guid> idsToProcess = new ArrayList<Guid>(guidsToProcess);
Line 182: guidsToProcess.clear();
Line 183: return idsToProcess;
Line 184: }
Line 178: private List<Guid> getIdsToProcess(List<Guid> ids) {
Line 179: int size = (ids.size() >= ITEMS_COUNT_PER_UPDATE ?
ITEMS_COUNT_PER_UPDATE : ids.size());
Line 180: List<Guid> guidsToProcess = ids.subList(0, size);
Line 181: List<Guid> idsToProcess = new ArrayList<Guid>(guidsToProcess);
Line 182: guidsToProcess.clear();
why clear here, the internal variables inside method is cleaned by JVM. Please
read how JVM works
Line 183: return idsToProcess;
Line 184: }
Line 185:
Line 186: /**
Line 201: for (VmTemplate template : templates) {
Line 202: if (VmTemplateStatus.Locked != template.getstatus()) {
Line 203: VmTemplateHandler.UpdateDisksFromDb(template);
Line 204: long currentDbGeneration =
getVmAndTemplatesGenerationsDAO().getDbGeneration(template.getId());
Line 205: if (template.getDb_generation() ==
currentDbGeneration) {
maybe before try to check if some disk is locked and after that run a DB query?
Line 206: boolean verifyDisksNotLocked =
verifyDisksNotLocked(template.getDiskList());
Line 207: if (verifyDisksNotLocked) {
Line 208: toReturn.add(template);
Line 209: }
Line 231:
Line 232: List<VM> vms = getVmDao().getVmsByIds(idsToProcess);
Line 233: for (VM vm : vms) {
Line 234: if (VMStatus.ImageLocked != vm.getstatus()) {
Line 235: VmHandler.updateDisksFromDb(vm);
same here, make a query after check
Line 236: Long currentDbGeneration =
getVmAndTemplatesGenerationsDAO().getDbGeneration(vm.getId());
Line 237: if (vm.getStaticData().getDb_generation() ==
currentDbGeneration) {
Line 238: boolean verifyDisksNotLocked =
verifyDisksNotLocked(vm.getDiskList());
Line 239: if (verifyDisksNotLocked) {
Line 232: List<VM> vms = getVmDao().getVmsByIds(idsToProcess);
Line 233: for (VM vm : vms) {
Line 234: if (VMStatus.ImageLocked != vm.getstatus()) {
Line 235: VmHandler.updateDisksFromDb(vm);
Line 236: Long currentDbGeneration =
getVmAndTemplatesGenerationsDAO().getDbGeneration(vm.getId());
also check for version should be done before send, I think it is more logical?
Especially when vm has snaphots and nics which can be changed
Line 237: if (vm.getStaticData().getDb_generation() ==
currentDbGeneration) {
Line 238: boolean verifyDisksNotLocked =
verifyDisksNotLocked(vm.getDiskList());
Line 239: if (verifyDisksNotLocked) {
Line 240: toReturn.add(vm);
Line 264: *
Line 265: * @param storagePoolId
Line 266: * @param templatesList
Line 267: * @return Returns true if updateVm succeeded.
Line 268: */
Performance problem same as getVmsMetadataDictionary.
Also I don't understand why so many lines of code needed in order to go throw
array.
Line 269: public void getTemplatesMetadataDictionary(Guid storagePoolId,
List<VmTemplate> templatesList) {
Line 270: Iterator<VmTemplate> iterator = templatesList.iterator();
Line 271: while (iterator.hasNext() && proccessedIdsInfo.size() <
ITEMS_COUNT_PER_UPDATE) {
Line 272: VmTemplate template = iterator.next();
Line 276: iterator.remove();
Line 277: }
Line 278: }
Line 279:
Line 280:
Why that method is public, it contains internal logic.
Line 281: public void buildMetadataDictionaryForTemplate(VmTemplate
template , Map<Guid, KeyValuePairCompat<String, List<Guid>>> metaDictionary){
Line 282: List<DiskImage> allTemplateImages = template.getDiskList();
Line 283:
Line 284: template.getInterfaces().clear();
Line 279:
Line 280:
Line 281: public void buildMetadataDictionaryForTemplate(VmTemplate
template , Map<Guid, KeyValuePairCompat<String, List<Guid>>> metaDictionary){
Line 282: List<DiskImage> allTemplateImages = template.getDiskList();
Line 283:
why clear here?
Line 284: template.getInterfaces().clear();
Line 285: for (VmNetworkInterface iface :
DbFacade.getInstance().getVmNetworkInterfaceDao()
Line 286: .getAllForTemplate(template.getId())) {
Line 287: template.getInterfaces().add(iface);
Line 324: }
Line 325: })));
Line 326: }
Line 327:
Line 328:
These method has awful performance.
iterator.remove() == System.arraycopy().
Code has O(n^2) memory copy operation.
No reason for that
Line 329: private void getVmsMetadataDictionary(Guid storagePoolId,
List<VM> vmsList) {
Line 330: Iterator<VM> iterator = vmsList.iterator();
Line 331: while (vmsList.iterator().hasNext() &&
proccessedIdsInfo.size() < ITEMS_COUNT_PER_UPDATE) {
Line 332: VM vm = iterator.next();
Line 360: tempVar.setStorageDomainId(storageDomainId);
Line 361: return
Backend.getInstance().getResourceManager().RunVdsCommand(VDSCommandType.UpdateVM,
tempVar)
Line 362: .getSucceeded();
Line 363: }
Line 364:
The following method is wrong. If RunVdsCommand() will fail it will throw an
exception. The same also at executeUpdateVmInSpmCommand
Line 365: public boolean executeRemoveVmInSpm(Guid storagePoolId, Guid
vmID, Guid storageDomainId) {
Line 366: return
Backend.getInstance().getResourceManager().RunVdsCommand(VDSCommandType.RemoveVM,
Line 367: new RemoveVMVDSCommandParameters(storagePoolId, vmID,
storageDomainId)).getSucceeded();
Line 368: }
--
To view, visit http://gerrit.ovirt.org/9328
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b5132300fb1f1fd94f771cab15efe5246dbeca8
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[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]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: liron aravot <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
