Change in vdsm[master]: [WIP] Implement live snapshots

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: [WIP] Implement live snapshots
......................................................................

[WIP] Implement live snapshots

Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
---
M vdsm/clientIF.py
M vdsm/libvirtvm.py
2 files changed, 68 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/69/469/1
--
To view, visit http://gerrit.ovirt.org/469
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(2 inline comments)

Why you need <vmDisk1>? what is it?

....................................................
File vdsm/libvirtvm.py
Line 1224:         except:
should it be libvirt specific exception?

Line 1233:                 vmDev = drive.libvirtName()
use drive.name I already fixed it my hotplug patchset.
It unrelated to whole hotplug feature, so you can push it in :)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 2: (2 inline comments)
> Why you need <vmDisk1>? what is it?

vmDisk1 is the disk name, I'll change that to vmDiskName1.

....................................................
File vdsm/libvirtvm.py
Line 1224:         except:
for any reason/exception we must abort the snapshot

Line 1233:                 vmDev = drive.libvirtName()
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 6: I would prefer that you didn't submit this

(4 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 1351:             errStatus = errCode['changeDisk']['status']
Do you think it's good enough to use 'changeDisk'  error here?
We use it for change CD flow. Maybe we need new dedicated error for shapshots?   I don't know ...

Line 1357:             for drive in self._drives:
The is no such thing _drives anymore. 
Use self._vm._devices[vm.DISK_DEVICES]   instead

Line 1361:                     drive.volumeID = snapParam[vmDev]["volumeID"]
you should update same fields in self.conf["devices"] too.
In additional you need  self.saveState() at the end

Line 1364: 
Do we need to update self._preparedDrives ?
I know that we want to drop it at some moment, but it still here

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 8: (3 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 1355:                         return device.name
why you are looking for the name in Drive object and not just in self.conf["devices"]? 
Did you filled it in _getUnderlyingDriveInfo? 
It's not very important, it just looks me a little odd to compare objects with dictionaries

Line 1378:                     self.log.error("Unable to teardown drive: %s", vmDev)
What about traceback here? Not must, but could be nicer

Line 1393:                 if device.get("name") == drive["name"]:
self.conf["devices"] consist all devices not only drives. 
Maybe compare only drives devices could be nicer:
   for dev in self.conf['devices']:
       if dev['type'] == vm.DISK_DEVICES and dev.get("name") == drive["name"]

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 8: (5 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 1350:                 for device in self._devices[vm.DISK_DEVICES]:
should it be self._devices[vm.DISK_DEVICES][:]  ?
Don't forget that we can hotplug/hotunplug disk.
Hmmm, now when we talking about it. What do you think about interleving snapshot and hotUnplug?  Maybe we need some lock here?

Line 1358:                 for device in self._devices[vm.DISK_DEVICES]:
self._devices[vm.DISK_DEVICES] [:]?

Line 1365:                 for device in self._devices[vm.DISK_DEVICES]:
self._devices[vm.DISK_DEVICES] [:]?

Line 1382:             for device in self._devices[vm.DISK_DEVICES]:
self._devices[vm.DISK_DEVICES][:] ?

Line 1392:             for device in self.conf["devices"]:
self.conf["devices"][:]?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 8: I would prefer that you didn't submit this

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 8: (2 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 1350:                 for device in self._devices[vm.DISK_DEVICES]:
Let's postpone the lock in a future patch that fixes it for all the invalid combinations (other commands included).

Line 1355:                         return device.name
Why should I go through all the devices when I'm looking for a drive?
It's the beauty of duplicating the information (grrr!). You can pick what you want according to your personal taste :-)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 9: I would prefer that you didn't submit this

(5 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 1359:             """Find a drive name given its definition"""
Find drive itself not a name

Line 1391:             for vmDevName, drive in newDrives.items():
As Dan like to comment :), why copy things? use iteritems.

Line 1404:                     for k, v in drive.items():
As above, consider iteritems()

Line 1412:             for device in self.conf["devices"]:
self.conf["devices"][:]  ?
Ah Federico, sorry. it's my fault (I gave you this line in the comment), but I wrote the right thing in comment before  :)

Line 1421:             self.saveState()
One general question here. What do you think, should we continue/success snapshot if we didn't found the proper drive to update? I mean, you try to update self.conf and _devices and you just log  if you can't find the proper device to update. Is it OK?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 9: (1 inline comment)

....................................................
File vdsm/libvirtvm.py
Line 1421:             self.saveState()
What else can we do here? The change already happened and we're trying to update our information, if this fails we can't try to rollback. Moreover this "fails" only if we can't find the drives (which means we have a bug somewhere else).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 10: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 10: (1 inline comment)

....................................................
File vdsm/libvirtvm.py
Line 1319:     def snapshot(self, snapParam):
snapParam is a list of drive specifications. I'd like the name to  capture this (drives? snapDrives?)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 10: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 1826:             target = x.getElementsByTagName('target')
Sorry missed it before.
we should initialize 'name', like name = ""
I know that for regular disk, target always exists, but I  am not sure what happens for example with empty cdrom

Line 1849:                     dev['name'] = name
maybe we need:
   if name:
      dev['name'] = name

Line 1858:                                              'name': name,
same as above, first build dictionary with or without name and append it after this

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 10: (1 inline comment)

....................................................
File vdsm/libvirtvm.py
Line 1826:             target = x.getElementsByTagName('target')
Actually, the right way is
 if target:
     name = target[0].getAttribute('dev')
 else:
         name = ''

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 10: (1 inline comment)

....................................................
File vdsm/libvirtvm.py
Line 1826:             target = x.getElementsByTagName('target')
1. Aren't we retrieving only the "disk" devices in line 1816? (the "cdrom" devices shouldn't be present in disksxml)

2. The "cdrom" devices always have a target, and it doesn't matter if the cd is present or not (just like any regular/real machine, the "hdc" device doesn't appear only when you insert a cd)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 10: Looks good to me, but someone else must approve

(1 inline comment)

....................................................
File vdsm/libvirtvm.py
Line 1826:             target = x.getElementsByTagName('target')
1. No, we are looking on all disks including cdrom:
   <disk type='file' device='cdrom'>
      <driver name='qemu' type='raw'/>
      <source file='xxx.iso'/>
      <target dev='hdc' bus='ide'/>
      <readonly/>
      <alias name='ide0-1-0'/>
      <address type='drive' controller='0' bus='1' unit='0'/>
    </disk>
the differences in 'device' attribute that we gather bellow.

2. OK, if you sure about it you have my ACK back.
    Sorry about the noise

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 11: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Implement VMs live snapshots
......................................................................

Patch Set 11: Verified; Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Implement VMs live snapshots

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Implement VMs live snapshots
......................................................................

Implement VMs live snapshots

Add a new command to take live snapshots of VMs:

clientIF.snapshot(vmId, snapDrives)

The parameter snapDrives is in the format:

snapDrives = [
    { "domainID": "<sdUUID>",
      "imageID": "<imgUUID>",
      "baseVolumeID": "<baseVolUUID>",
      "volumeID": "<volUUID>" },
    ...
  ]

Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
---
M vdsm.spec.in
M vdsm/clientIF.py
M vdsm/define.py
M vdsm/libvirtvm.py
4 files changed, 190 insertions(+), 4 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved
  Igor Lvovsky: Looks good to me, but someone else must approve

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id48a905938037eca2b1de966f4f83d801fdb9970
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky