Michael Kublin has posted comments on this change.
Change subject: core:WIP: introducing OvfAutoUpdate
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(7 inline comments)
These patch not solves problems of races.
These patch reduce significant performance of ALL system.
These patch can cause additional reconstruct or spm selection.
These patch introduce additional refresh of vm/templates views and additional
updates which was not before - again increase I/O of database.
After a reconstruct, at high load system OvfAutoUpdate will crash an engine.
....................................................
File backend/manager/dbscripts/create_views.sql
Line 325:
Line 326:
Line 327: CREATE OR REPLACE VIEW vm_templates_view
Line 328: AS
Line 329:
You understand that u added additional field which will trigger update of
views, when a field will be changed
Line 330: SELECT
Line 331: vm_templates.vm_guid as vmt_guid,
Line 332: vm_templates.vm_name as name,
Line 333: vm_templates.mem_size_mb as mem_size_mb,
....................................................
File backend/manager/dbscripts/upgrade/03_01_1490_add_vm_generation_columns.sql
Line 1: select fn_db_add_column('vm_static', 'ovf_generation', 'BIGINT');
Line 2: select fn_db_add_column('vm_static', 'db_generation', 'BIGINT default
1');
why you add your field to vm_static, vm_static contains important info which
should not be changed very often, these is not a case
Line 3: UPDATE vm_static set ovf_generation = 1;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java
Line 56: getVmsForUpdate(DbFacade.getInstance()
Line 57: .getVmDao()
Line 58:
.getAllVmsForOvfUpdateForStoragePool(pool.getId()));
Line 59: VmCommand.updateVmInSpm(pool.getId(),
vmsForUpdate);
Line 60: if (!vmsForUpdate.isEmpty()) {
How many updates the following code is performing? At worst case the number of
updates will be equal to number of vms in pool. These code will decrease
drastic performance of the system. If during run of the following update run
reconstruct, ovf updater will never update any of vm
Line 61: for (VM vm : vmsForUpdate) {
Line 62: DbFacade.getInstance()
Line 63: .getVmDao()
Line 64: .updateVmOvfGeneration(vm.getId(),
vm.getStaticData().getDb_generation());
Line 65: }
Line 66: }
Line 67: List<VmTemplate> templatesForUpdate =
getTemplatesForUpdate(DbFacade.getInstance()
Line 68: .getVmTemplateDao()
Line 69:
.getAllVmTemplatesForOvfUpdateForStoragePool(pool.getId()));
At these point all vms and alll templates and all their disks are in memory.
Line 70: if (!templatesForUpdate.isEmpty()) {
Line 71:
VmTemplateCommand.UpdateTemplateInSpm(pool.getId(), templatesForUpdate);
Line 72: for (VmTemplate template : templatesForUpdate)
{
Line 73: DbFacade.getInstance()
Line 89: * returns a list of vms that are valid for update from the given
vm list.
Line 90: * valid vm is a vm which is not locked and none of it's disks is
locked.
Line 91: * @param vms
Line 92: * @return
Line 93: */
These code is performance disaster, in worst case it will load all vms at the
system and all its disks.
Line 94: private List<VM> getVmsForUpdate(List<VM> vms) {
Line 95: List<VM> toReturn = new ArrayList<VM>();
Line 96: for (VM vm : vms) {
Line 97: if (VMStatus.ImageLocked != vm.getstatus()) {
Line 128: /**
Line 129: * returns true if none of the given disks is in status 'LOCKED',
otherwise false.
Line 130: * @param disks
Line 131: * @return
Line 132: */
These check is useless, that a vm was not locked during a query not promise
anything. During hot plug of nics or disc nothing is locked.
Line 133: private boolean verifyDisksNotLocked (List<DiskImage> disks) {
Line 134: for (DiskImage disk : disks) {
Line 135: if (disk.getimageStatus() == ImageStatus.LOCKED) {
Line 136: return false;
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/UpdateVMVDSCommand.java
Line 45: counter++;
Line 46: }
Line 47: return result;
Line 48: }
Line 49:
These will not prevent spm election or reconstruct
Line 50: @Override
Line 51: protected VDSExceptionBase createDefaultConcreteException(String
errorMessage) {
Line 52: return new IrsOperationFailedNoFailoverException(errorMessage);
Line 53: }
--
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: 3
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
