Change in vdsm[master]: Live Merge: Resolve unknown merge status in vdsm after host ...

nsoffer at redhat.com nsoffer at redhat.com
Sun Aug 17 15:28:25 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: Live Merge: Resolve unknown merge status in vdsm after host crash
......................................................................


Patch Set 5:

(5 comments)

http://gerrit.ovirt.org/#/c/31267/5//COMMIT_MSG
Commit Message:

Line 17: regular cleanup.
Line 18: 
Line 19: 2. Allow a live merge to be resubmitted on a new host using blank volume
Line 20: uuids.  This will tell vdsm to skip the libvirt part of the merge but to
Line 21: rerun synchronization operations.
Using empty uuids is little ugly. How does engine knows that the libvirt part is done and it could be skipped?

Can you explain where this special call is used in the different flows?

It looks like this verb is trying to do two different actions, and we use magic arguments values to use the second action. Why not use another verb for this case (synchronization)?
Line 22: 
Line 23: Change-Id: I1ca5e4ad67216d30c3149a53c2ea65cc97601bfc
Line 24: Signed-off-by: Adam Litke <alitke at redhat.com>


http://gerrit.ovirt.org/#/c/31267/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 5321:         with self._jobsLock:
Line 5322:             for storedJob in self.conf['_blockJobs'].values():
Line 5323:                 jobID = storedJob['jobID']
Line 5324:                 cleanThread = self._liveMergeCleanupThreads.get(jobID)
Line 5325:                 if cleanThread and cleanThread.isFinalized():
Previously we handled only successful jobs, and now we handle only successful cleanup jobs - is this our intention?
Line 5326:                     # Handle successfully cleaned jobs early because the job
Line 5327:                     # just needs to be untracked and the stored disk info might
Line 5328:                     # be stale anyway (ie. after active layer commit).
Line 5329:                     self.untrackBlockJob(jobID)


Line 5322:             for storedJob in self.conf['_blockJobs'].values():
Line 5323:                 jobID = storedJob['jobID']
Line 5324:                 cleanThread = self._liveMergeCleanupThreads.get(jobID)
Line 5325:                 if cleanThread and cleanThread.isFinalized():
Line 5326:                     # Handle successfully cleaned jobs early because the job
The comment needs update - it was about "successful" jobs", and it is about "finalized" jobs.
Line 5327:                     # just needs to be untracked and the stored disk info might
Line 5328:                     # be stale anyway (ie. after active layer commit).
Line 5329:                     self.untrackBlockJob(jobID)
Line 5330:                     continue


Line 5569:             # properly recover the VM in case we crash.  At this point the
Line 5570:             # active layer contains the same data as its parent so the ILLEGAL
Line 5571:             # flag indicates that the VM should be restarted using the parent.
Line 5572:             newVols = [x['volumeID'] for x in drive.volumeChain
Line 5573:                        if x['volumeID'] != drive.volumeID]
Please use clear name instead of x, like vol.
Line 5574:             self.cif.irs.imageSyncVolumeChain(drive.domainID, drive.imageID,
Line 5575:                                               drive['volumeID'], newVols)
Line 5576: 
Line 5577:             self.log.info("Requesting pivot to complete active layer commit "


Line 5655:         """
Line 5656:         Returns True if the last cleanup phase succeeded and the job can be
Line 5657:         removed.
Line 5658:         """
Line 5659:         return self.success and self.mode == self.MODE_CLEANUP
This new interface and its handling is very confusing. I don't see here any steps, so it is not clear why this should work in the way it does.

We need to put the logic in one place instead of spreading it in both this class and handleBlockJobEvent.

Can you explain the different possible flows?
Line 5660: 
Line 5661: 
Line 5662: def _devicesWithAlias(domXML):
Line 5663:     return vmxml.filter_devices_with_alias(vmxml.all_devices(domXML))


-- 
To view, visit http://gerrit.ovirt.org/31267
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ca5e4ad67216d30c3149a53c2ea65cc97601bfc
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list