Re: [Engine-devel] [Engine-Devel] OvfDataUpdater

> > > > > > > > > > > > > 
> > > > > > > > > > > > > starting a new discussion thread.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > ----- Original Message -----
> > > > > > > > > > > > > > From: "Mike Kolesnik" <mkolesni@redhat.com>
> > > > > > > > > > > > > > To: "Liron Aravot" <laravot@redhat.com>
> > > > > > > > > > > > > > Sent: Monday, November 19, 2012 12:42:10 PM
> > > > > > > > > > > > > > Subject: Re: [Engine-devel]  OvfAutoUpdater
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I think 'version' is a more standard term
> > > > > > > > > > > > > > for
> > > > > > > > > > > > > > the
> > > > > > > > > > > > > > column
> > > > > > > > > > > > > > names.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Also in:  4. If succesfull - for each vm
> > > > > > > > > > > > > > update
> > > > > > > > > > > > > > the
> > > > > > > > > > > > > > ovf_generation
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > equal the db_generation.
> > > > > > > > > > > > > > I think you mean that the update should be
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > the
> > > > > > > > > > > > > > entity
> > > > > > > > > > > > > > version
> > > > > > > > > > > > > > you
> > > > > > > > > > > > > > initially selected.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > 1. I think should be version = version +1;
> > > > > > > > > > the version will be the db_generation that was
> > > > > > > > > > loaded
> > > > > > > > > > when
> > > > > > > > > > we
> > > > > > > > > > loaded
> > > > > > > > > > the vm/template.
> > > > > > > > > > for example - if the vm/template was updated twice
> > > > > > > > > > between
> > > > > > > > > > OvfUpdater
> > > > > > > > > > runs, we will need to increment twice - so
> > > > > > > > > > incrementing
> > > > > > > > > > by
> > > > > > > > > > one
> > > > > > > > > > will
> > > > > > > > > > cause another unneeded ovf update for that vm.
> > > > > > > > > I am talking about ovfVersion or how you called it.
> > > > > > > > me too,
> > > > > > > > if we update the vm three times between OvfAutoUpdater
> > > > > > > > runs
> > > > > > > > ,
> > > > > > > > we
> > > > > > > > will
> > > > > > > > have the following values in the loaded vm for example:
> > > > > > > > db_generation 8
> > > > > > > > ovf_generation 5
> > > > > > > > 
> > > > > > > > when we perform the ovf update , ovf_generation should
> > > > > > > > be
> > > > > > > > set
> > > > > > > > to
> > > > > > > > 8
> > > > > > > > and not to 6, as version '8' is the version that we
> > > > > > > > wrote
> > > > > > > > the
> > > > > > > > ovf
> > > > > > > > metadata of.
> > > > > > > These should be written in wiki, with remarks that values
> > > > > > > which
> > > > > > > were
> > > > > > > retrieved
> > > > > > > from DB together.
> > > > > > > > > > > > 2. Now the quartz is automatic process,
> > > > > > > > > > > > updateVmInSpm
> > > > > > > > > > > > it
> > > > > > > > > > > > is
> > > > > > > > > > > > spm
> > > > > > > > > > > > operation -it means it can trigger a
> > > > > > > > > > > > reconstruct
> > > > > > > > > > > > and spm election.
> > > > > > > > > > 2. updateVmInSpm command would be changed so it
> > > > > > > > > > won't
> > > > > > > > > > trigger
> > > > > > > > > > failover, will add that to the wiki.
> > > > > > > > > > > > 3. How your solution is handling a case of
> > > > > > > > > > > > hotPlug/hotUnplug/remove/add of vm disks.
> > > > > > > > > > > > vm_static
> > > > > > > > > > > > is
> > > > > > > > > > > > not
> > > > > > > > > > > > usually
> > > > > > > > > > > > updated.
> > > > > > > > > > Whenever will be a command that locks a
> > > > > > > > > > vm/template,
> > > > > > > > > > you
> > > > > > > > > > will
> > > > > > > > > > have
> > > > > > > > > > an
> > > > > > > > > > option during the unlock to specify if the version
> > > > > > > > > > need
> > > > > > > > > > to
> > > > > > > > > > be
> > > > > > > > > > incremented, you'll be able to increment it also
> > > > > > > > > > yourself.
> > > > > > > > > > The HotUnPlug command does lock the vm and during
> > > > > > > > > > endSuccesfully
> > > > > > > > > > attempt to update the vm in spm, so it will
> > > > > > > > > > increment
> > > > > > > > > > the
> > > > > > > > > > db_version/generation instead.
> > > > > > > > > > > 
> > > > > > > > > addDisk, removeDisk, hotPlug/unPlug not locking a vm.
> > > > > > > > > Also, as far as I know LockVm/UnLockVm it is
> > > > > > > > > operation
> > > > > > > > > on
> > > > > > > > > vm_dynamic
> > > > > > > > > and not
> > > > > > > > > vm_static.
> > > > > > > > 
> > > > > > > > hotplug acquires memory lock on the vm and has no async
> > > > > > > > tasks
> > > > > > > > if
> > > > > > > > I
> > > > > > > > recall correctly.
> > > > > > > Yes, correct and what? These what I said, vm_static is
> > > > > > > not
> > > > > > > updated.
> > > > > > you will just call the dao method for incrementing the
> > > > > > db_generation,
> > > > > > you don't have to perform vm_static update.
> > > > > > > > adddisk acquires shared lock on the vm which is
> > > > > > > > problematic
> > > > > > > > with
> > > > > > > > today flow as well - as you have a race during
> > > > > > > > updateVmInSpm,
> > > > > > > > it
> > > > > > > > should be fixed regardless of the OvfAutoUpdater.
> > > > > > > It is not answer on my question. How ovf will be updated?
> > > > > > > How
> > > > > > > you
> > > > > > > understand
> > > > > > > that ovf of vm should be updated?
> > > > > > 
> > > > > > the ovf will be updated when you increment the
> > > > > > db_generation
> > > > > > value
> > > > > > instead of calling updateVmInSpm().
> > > > > > you will be able to do that from the dao directly when you
> > > > > > want,
> > > > > > or
> > > > > > when you unlock a vm/template.
> > > > > > when you will update the db_generation, the vm/template
> > > > > > would
> > > > > > be
> > > > > > picked up by OvfUpdater as it's db_generation is different
> > > > > > than
> > > > > > its
> > > > > > ovf_generation.
> > > > > 1. Update of ovfs is not related to locks and it should not
> > > > > be,
> > > > > I
> > > > > don't understand why
> > > > > you link them.
> > > > > 2. Races - your automatic process and couple of add disk
> > > > > commands.
> > 
> Sorry, part of my response was cutted by mistake, adding it
> As of today - there is a race.
> After adding ovfautoupdater , you won't update the ovf for vm which
> is locked or has a locked disk (written in the wiki).
Vm/disk can be moved to locked after your query, at attach/hot plug 
the state of vm will not change.
Also I still don't understand why you need check a status of disk/vm.
If some process is running on vm, ovf will be update after 5 minutes.
> ovfautoupdater won't introduce nor keep that race as we should have
>  db exclusive row lock when we perform this update so even if we had
>  context switch and we later returned to the update process, we will
>  run the update again as we don't send the db_generation value from
>  the engine, but only incrementing in the db. correct
> me if i'm wrong.
Yes, you are wrong. I don't understand why you are talking about a locks at DB,
actually why you are talking about any lock, how it is related?
The race is two operation that you are doing.
1. Reset all ovf_generation to 0 because of reconstruct
2. ovf updater is updating ovf_generation to value that he kept in memory.
At that case you will miss update of ovfs after reconstruct. 

Now, why the generation field is added to vm_static table, these field is not related
to static information of vm.
Now, your implementation is to add a lot of updates to vm_static table, it will cause
to refreshes of vms view - these will stuck all flows related to vm, especially after
reconstruct flow or some mass operation on vm. Why I need these?

Now, general regards design - what you are trying to implement it is very similar to
event queue. I think these can be done in another more simple and more performance efficient
way. 

> > 
> > > > > You will miss update
> > > > > of ovf.
> > > > > 3. Also when you will add increase of generation it is not
> > > > > clear
> > > > > from
> > > > > your design
> > > > 
> > > > 1+2 The OvfAutoUpdater is not related and doesn't acquire any
> > > > locks.
> > > > 
> > > > as of today, if you call updateVmInSpm without performing the
> > > > operations with lock on the vm you'll have a race - for
> > > > example,
> > > > in
> > > Liron, updateVmInSpm is called at endSuccessfully, you can not
> > > build
> > > on locks,
> > > especially on in memory locks.
> > > > addDisk (shared calls, two updateVmInSpm might be executed in
> > > > the
> > > > same time and you can't know with with ovf you'll end).
> > > > 
> > > > therefore, when updating the ovf_generation you should have a
> > > > lock
> > > > somewhere otherwise you might miss updates because of races.
> > > > if today updatevminspm is called without locks and have races,
> > > > it
> > > > needs to be fixed unrelated to it.
> > > Liron, you should fix these. These is a new development, it
> > > should
> > > fix
> > > all known bugs and problems and not introduce a new ones
> > 
> > answered below,
> > As of today - there is a race.
> > ovfautoupdater won't introduce nor keep that race as we should have
> > automatic db exclusive row lock when we perform this update,
> > correct
> > me if i'm wrong.
> > 
> > > 
> > > > 3. you will increase the generation whenever you made
> > > > updateVmInSpm.
> > > > > > > > > 
> > > > > > > > > > > Also consider snapshots & vNICs which affect the
> > > > > > > > > > > VM
> > > > > > > > > > > state.
> > > > > > > > > > Whatever affect the vm state and triggers
> > > > > > > > > > updateVmInSpm
> > > > > > > > > > today
> > > > > > > > > > will
> > > > > > > > > > continue to, it can be added to flows which don't
> > > > > > > > > > do
> > > > > > > > > > that
> > > > > > > > > > today.
> > > > > > > > > > > 
> > > > > > > > > > > > 4. Reconstruct/Recovery - updateVmInSpm should
> > > > > > > > > > > > be
> > > > > > > > > > > > called
> > > > > > > > > > > > on
> > > > > > > > > > > > all
> > > > > > > > > > > > vms,
> > > > > > > > > > > > no matter if they were updated.
> > > > > > > > > > 
> > > > > > > > > > Ofcourse, it's being taken care of.
> > > > > > > > > How?
> > > > > > > > we don't have ovf's of the vms when we reconstruct as
> > > > > > > > we
> > > > > > > > don't
> > > > > > > > have
> > > > > > > > master domain - so we should set the ovf_generation of
> > > > > > > > the
> > > > > > > > vms
> > > > > > > > to
> > > > > > > > be
> > > > > > > > 0 - which will cause ovfautoupdater to copy the vm
> > > > > > > > metadata.
> > > > > > > I did not understand that statement and how it is
> > > > > > > related.
> > > > > > OvfAutoUpdater will get those vms for update as
> > > > > > db_generation
> > > > > > is
> > > > > > different than ovf_generation.
> > > > > > > > a performance improvement may be introduced later on in
> > > > > > > > case
> > > > > > > > of
> > > > > > > > wrong
> > > > > > > > master version, but that's not related to the
> > > > > > > > ovfautoupdater.
> > > > > > > I don't understand connection to performance.
> > > > > > > Now you are replacing a calls to
> > > > > > > VmCommand.updateVmInSpm()
> > > > > > > one
> > > > > > > of
> > > > > > > them it
> > > > > > > is at reconstruct flow, and you should handle it also and
> > > > > > > add
> > > > > > > it
> > > > > > > to
> > > > > > > wiki.
> > > > > > answered below, OvfAutoUpdater will get those vms for
> > > > > > update
> > > > > > as
> > > > > At reconstruct all vms are updated. So it is not related to
> > > > > generation.
> > > > > How you handle these case?
> > > > 
> > > > when doing reconstruct, the ovf_generation of all vms is set to
> > > > 0
> > > > as
> > > > we don't have them on the master domain.
> > > > ovfupdater will get this vms for update as ovf_generation <
> > > > db_generation, will copy the vm ovf and set db_genertion to
> > > > ovf_generation.
> > > > 
> > > > > > db_generation is different than ovf_generation.
> > > > > > > > > > > > 
> > > > > > > > > > > > > > Regards,
> > > > > > > > > > > > > > Mike
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > ----- Original Message -----
> > > > > > > > > > > > > > > http://wiki.ovirt.org/wiki/Feature/OvfAutoUpdater
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Hi all, i'll be glad if you could review
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > wiki
> > > > > > > > > > > > > > > page
> > > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > OvfAutoUpdater, if you have any
> > > > > > > > > > > > > > > suggestions
> > > > > > > > > > > > > > > or
> > > > > > > > > > > > > > > ideas
> > > > > > > > > > > > > > > please
> > > > > > > > > > > > > > > let
> > > > > > > > > > > > > > > me
> > > > > > > > > > > > > > > know.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > http://wiki.ovirt.org/wiki/Feature/OvfAutoUpdater
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > short preview from the wiki:
> > > > > > > > > > > > > > > vm/template configurations (including
> > > > > > > > > > > > > > > disks
> > > > > > > > > > > > > > > info)
> > > > > > > > > > > > > > > are
> > > > > > > > > > > > > > > stored
> > > > > > > > > > > > > > > on
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > master storage domain for backup
> > > > > > > > > > > > > > > purposes,
> > > > > > > > > > > > > > > import/export
> > > > > > > > > > > > > > > and
> > > > > > > > > > > > > > > also
> > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > provide the abillity to run VMs without
> > > > > > > > > > > > > > > having
> > > > > > > > > > > > > > > a
> > > > > > > > > > > > > > > running
> > > > > > > > > > > > > > > engine/db.
> > > > > > > > > > > > > > > Currently ovf update is done
> > > > > > > > > > > > > > > synchronously
> > > > > > > > > > > > > > > when
> > > > > > > > > > > > > > > performing
> > > > > > > > > > > > > > > various
> > > > > > > > > > > > > > > operations on vms/templates - update,
> > > > > > > > > > > > > > > save,
> > > > > > > > > > > > > > > adding/removing
> > > > > > > > > > > > > > > a
> > > > > > > > > > > > > > > disk,
> > > > > > > > > > > > > > > etc. What's more, currently updating the
> > > > > > > > > > > > > > > ovf
> > > > > > > > > > > > > > > (updateVM
> > > > > > > > > > > > > > > vdsm
> > > > > > > > > > > > > > > call)
> > > > > > > > > > > > > > > is
> > > > > > > > > > > > > > > usually done within a transcation.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > The idea behined OvfAutoUpdater is to
> > > > > > > > > > > > > > > perform
> > > > > > > > > > > > > > > batch
> > > > > > > > > > > > > > > ovf
> > > > > > > > > > > > > > > update
> > > > > > > > > > > > > > > operations that aggregate all outstanding
> > > > > > > > > > > > > > > updates
> > > > > > > > > > > > > > > per
> > > > > > > > > > > > > > > data
> > > > > > > > > > > > > > > center.
> > > > > > > > > > > > > > > These updates will be done in specifed
> > > > > > > > > > > > > > > time
> > > > > > > > > > > > > > > intervals
> > > > > > > > > > > > > > > which
> > > > > > > > > > > > > > > will
> > > > > > > > > > > > > > > reduce XML-RPC calls and will enable the
> > > > > > > > > > > > > > > removal
> > > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > this
> > > > > > > > > > > > > > > syncronous
> > > > > > > > > > > > > > > vdsm call from all over the code.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > > Liron Aravot.
> > > > > > > > > > > > > > > _______________________________________________
> > > > > > > > > > > > > > > Engine-devel mailing list
> > > > > > > > > > > > > > > Engine-devel@ovirt.org
> > > > > > > > > > > > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > _______________________________________________
> > > > > > > > > > > > > Engine-devel mailing list
> > > > > > > > > > > > > Engine-devel@ovirt.org
> > > > > > > > > > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > > > > > > > > > > 
> > > > > > > > > > > > _______________________________________________
> > > > > > > > > > > > Engine-devel mailing list
> > > > > > > > > > > > Engine-devel@ovirt.org
> > > > > > > > > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > _______________________________________________
> > Engine-devel mailing list
> > Engine-devel@ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > 
>