Change in vdsm[master]: snapshot: Start the snapshot module

michal.skrivanek at redhat.com michal.skrivanek at redhat.com
Thu Jul 2 12:01:30 UTC 2015


Michal Skrivanek has posted comments on this change.

Change subject: snapshot: Start the snapshot module
......................................................................


Patch Set 6:

(3 comments)

>> Michal, Martin: Vm class is way too big, and we cannot test it (in a sane way). We should not add new code to this class.
If I embed the code in snapshot.py into vm.py, I cannot test it. The idea is to move also the snapshot code from vm py in a similar way, so code can be easily tested.

testability is an issue of current code too. Again, I'd prefer to have a single flow (and test) for a single action, regardless underlying storage technology.

regarding separation, I think snapshot is too small thing to factor out. We moved out significant pieces already, but I think our goal is not to go all the way to 0.
I'm not opposed to refactoring, but I would ideally envision a function within vm.py, which would handle both "regular" and ceph snapshot. If it looks ugly then we can delegate the guest agent call to the current guest agent class (and that way encapsulate the fact whether we want to use our agent or qemu-ga or whatever, at least it's clear from high level that it is a guest agent "thing")

https://gerrit.ovirt.org/#/c/43018/6//COMMIT_MSG
Commit Message:

Line 10: include only the GuestAgent class, responsible for freezing and thawing
Line 11: guest file systems during a snapshot.
Line 12: 
Line 13: Freezing or thawing filesystems can fail because of many reasons;
Line 14: libvirt driver does not support freezing, guest agent is not installed
> Libvirt drivers (e.g. qemu) may implement domainFSFreese/Thaw or not. If fu
I think this assumption is so built in that we should not even mention another possibility:)
Line 15: or not running or guest agent failure. We detect the first two cases, and
Line 16: treat any other libvirt error as guest agent failure.
Line 17: 
Line 18: The errors are reported back to engine so it can handled them correctly.


https://gerrit.ovirt.org/#/c/43018/6/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 244:         return self._downtimes
Line 245: 
Line 246:     @recorded
Line 247:     def fsFreeze(self, mountpoints=None, flags=0):
Line 248:         self._failIfRequested()
> Because we want to simulate error so we can test how code handle them.
ok, I guess it's harmless as the global snapshot action succeeds
once there is a need it can be extended...
Line 249:         return 3
Line 250: 
Line 251:     @recorded
Line 252:     def fsThaw(self, mountpoints=None, flags=0):


https://gerrit.ovirt.org/#/c/43018/6/vdsm/virt/snapshot.py
File vdsm/virt/snapshot.py:

Line 51:         """
Line 52:         self._log.info("Freezing guest filesystems")
Line 53: 
Line 54:         try:
Line 55:             frozen = self._dom.fsFreeze()
> Ok, silly me just missed that ovirt-guest-agent has an hard-dep on qemu-ga,
it does. But to date we intentionally never communicated with qemu-ga, but went through ovirt-ga. There are few semi-exceptions, like current snapshot, where we pass a flag which in turn calls qemu-ga, but that's encapsulated fully.
Line 56:         except libvirt.libvirtError as e:
Line 57:             self._log.warning("Unable to freeze guest filesystems: %s", e)
Line 58:             code = e.get_error_code()
Line 59:             if code == libvirt.VIR_ERR_AGENT_UNRESPONSIVE:


-- 
To view, visit https://gerrit.ovirt.org/43018
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e4c54f461039de99823c3b80a10be0a960c4273
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list