Arik Hadas has posted comments on this change.
Change subject: RAM snapshots feature
......................................................................
Patch Set 7: (8 inline comments)
....................................................
File vdsm/BindingXMLRPC.py
Line 279: def vmUpdateDevice(self, vmId, params):
Line 280: vm = API.VM(vmId)
Line 281: return vm.vmUpdateDevice(params)
Line 282:
Line 283: def vmSnapshot(self, vmId, snapDrives, snapMemory=''):
ok, I'll rename. but why to initialize it to None? I think it is better to
initialize it to the value the engine will send when there's no memory volume
in the snapshot, which is an empty string, in order to stay with two possible
input types (empty string and memory volume representation). it wouldn't change
much anyway since in both cases the 'if snapMemory:' in API#snapshot will fail..
Line 284: """
Line 285: Take snapshot of VM
Line 286:
Line 287: vmId and snapDrives are always set by the caller.
Line 289: It is not set by the caller if memory snapshots are not
supported for
Line 290: the given VM. in that case the default value is used.
Line 291: Otherwise, it is set to string:
Line 292: If the snapshot should contain memory, the string is
structured as a
Line 293: comma-separated string similar to hibernation volume
representation
Done
Line 294: (domain,pool,image1,volume1,image2,volume2). if the snapshot
should
Line 295: not contain memory, the field is set to empty string.
Line 296: """
Line 297: vm = API.VM(vmId)
....................................................
File vdsm/vdsmd.8.in
Line 89:
Line 90: The environment of before_vm_set_ticket and after_vm_set_ticket hooks
is augmented
Line 91: with a set of params passed by the caller of setVmTicket.
Line 92:
Line 93: The environment of before_vm_dehibernate and after_vm_dehibernate
hooks is augmented
Done
Line 94: with a parameter called 'backToSnapshot' which indicates whether the
restored state
Line 95: is taken from a snapshot the domain was reverted to, if it is set to
true, or it is
Line 96: the state the domain had before it was hibernated, if it is set to
false.
Line 97:
Line 90: The environment of before_vm_set_ticket and after_vm_set_ticket hooks
is augmented
Line 91: with a set of params passed by the caller of setVmTicket.
Line 92:
Line 93: The environment of before_vm_dehibernate and after_vm_dehibernate
hooks is augmented
Line 94: with a parameter called 'backToSnapshot' which indicates whether the
restored state
Done
Line 95: is taken from a snapshot the domain was reverted to, if it is set to
true, or it is
Line 96: the state the domain had before it was hibernated, if it is set to
false.
Line 97:
Line 98: The environment of hooks specific to devices:
Line 92:
Line 93: The environment of before_vm_dehibernate and after_vm_dehibernate
hooks is augmented
Line 94: with a parameter called 'backToSnapshot' which indicates whether the
restored state
Line 95: is taken from a snapshot the domain was reverted to, if it is set to
true, or it is
Line 96: the state the domain had before it was hibernated, if it is set to
false.
2 questions:
1. since 'FROM_SNAPSHOT' is a flag, why not using True/False instead of number?
2. would it mean that 'backToSnapshot' and 'restoreFromSnapshot' should be
renamed to 'FROM_SNAPSHOT' ?
Line 97:
Line 98: The environment of hooks specific to devices:
Line 99: before_nic_hotplug, after_nic_hotplug, after_nic_hotplug_fail,
Line 100: before_nic_hotunplug, after_nic_hotunplug,
after_nic_hotunplug_fail,
....................................................
File vdsm/vm.py
Line 2843: elif 'restoreState' in self.conf:
Line 2844: restoreFromSnap = self.conf.get('restoreFromSnapshot',
False)
Line 2845: srcDomXML = self.conf.pop('_srcDomXML')
Line 2846: hooks.before_vm_dehibernate(srcDomXML, self.conf,
Line 2847: {'backToSnapshot':
restoreFromSnap})
so should I rename 'backToSnapshot' to 'restoreFromSnapshot' as well or use
'FROM_SNAPSHOT' as you wrote in previous comment?
Line 2848:
Line 2849: fname =
self.cif.prepareVolumePath(self.conf['restoreState'])
Line 2850: try:
Line 2851: if restoreFromSnap:
Line 2881: self._domDependentInit()
Line 2882:
Line 2883: def _updateDiskVolumes(self, srcDomXML):
Line 2884: """
Line 2885: Update each image in the given XML to point to the right
volume.
I rephrased the description. regarding the function's name - it was called
"_correctDiskVolumes" before, and you asked me in the previous patch to rename
it to this one. I would go with the previous name, do you think of a better
name?
Line 2886: Each image should have a newer volume than the one that was
the
Line 2887: newer when the snapshot was taken, since we create new volume
Line 2888: for each image on 'preview' operation.
Line 2889: """
Line 3534: vmConfVolPath = self.cif.prepareVolumePath(vmConfVol)
Line 3535: vmConf = _vmConfForSnapshot()
Line 3536: try:
Line 3537: with file(vmConfVolPath, "w") as f:
Line 3538: pickle.dump(vmConf, f)
I think this vmConfVol should disappear, but not as part of this patch. my
reference was the way it is done as part of the hibernation process, can you
give me a reference to a place where it works as you expect it to work?
Line 3539: finally:
Line 3540: self.cif.teardownVolumePath(vmConfVol)
Line 3541: else:
Line 3542: snapFlags |= libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY
--
To view, visit http://gerrit.ovirt.org/15072
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I62401940afb0228cbd9dd3611b6ed8e0ff67c82c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Eduardo <ewars...@redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvov...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Peter V. Saveliev <p...@redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
