liron aravot has posted comments on this change.
Change subject: core:WIP: introducing OvfAutoUpdate
......................................................................
Patch Set 6: (19 inline comments)
....................................................
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:
In OvfDataUpdater we perform batch update on vms regulary.
having OvfDataUpdater performing that batch update on the same table used by
all commands for db_generation can cause performance issues as the batch
update takes ROW EXCLUSIVE lock on all updates vm row, it will block other
commands trying to update that rows and basically halt operation on vms - if we
have 10,000 vms and managed to take 9999 locks and we wait for the last one, no
operation could be done on all of those 9999 vms as we hold the row lock.
this doesn't happen on regular flows as we basically lock the object we operate
on, in this flow we don't have any locks and i don't want to hold other
commands from completing their execution.
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');
that's for testing, will send as 5/10.
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:
no need to load the whole vm when we just need it's db_generation.
Line 23: Create or replace FUNCTION GetDbGeneration(v_vm_guid UUID)
Line 24: RETURNS BIGINT
Line 25: AS $procedure$
Line 26: BEGIN
....................................................
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;
correct, left here by mistake.
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:
Done
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;
Done
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 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.
will change the comment.
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:
the concept of force is that you avoid performing uneeded vdsm calls, take a
look in line 126 - if we have only 10 vms for update there's no need to run if
we can get to 100 per the vdsm command , when we are in the end of execution
(line 101) we run the vds command with whatever we 'collected' so far to avoid
multiple calls.
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);
that's a final member of the class that is used in order to avoid multiple list
creations when we need only one basically.
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);
next comment expalins, clear on sublist clears on the list itself.
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();
clear on sublist (actually 'view' of the original list) removes the items from
the original list. no reason to have processed guids in memory.
what do need to be added here is creation of LinkedList as the dao returns
arraylist, will add that.
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) {
agreed.
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);
agreed
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());
I don't agree, will discuss.
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: */
I don't want to load all vms at once , but limit it to ITEMS_COUNT_PER_UPDATE -
so we load vms only when we don't have enough yet to perform vds command. Will
change.
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:
used by export command.
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:
that's a cut from VmTeplateCommand.updateVmInSpm() - haven't changed this code.
might be unnecessary .
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:
haven't changed back to LinkedList, will change.
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:
agreed.
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 <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: liron aravot <liron.ara...@gmail.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
