Change in vdsm[master]: [WIP] Add the formatConverter for Storage Domain V3

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: [WIP] Add the formatConverter for Storage Domain V3
......................................................................

[WIP] Add the formatConverter for Storage Domain V3

Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Signed-off-by: Federico Simoncelli 
---
M vdsm/storage/blockVolume.py
M vdsm/storage/imageRepository/formatConverter.py
M vdsm/storage/sp.py
3 files changed, 118 insertions(+), 8 deletions(-)

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

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

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: [WIP] Add the formatConverter for Storage Domain V3
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/imageRepository/formatConverter.py
Line 130:     # after the upgrade but let's not make assumptions about future behaviors
Missing:
 if isMsd:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: [WIP] Add the formatConverter for Storage Domain V3
......................................................................

Patch Set 2: (3 inline comments)

....................................................
File vdsm/storage/imageRepository/formatConverter.py
Line 51:         vol.setrw(True)
This could be optimized (and moved somewehere else) to run just an lvm command.

Line 58:         # advertised by the metadata, FIXME: finish implementation
To fix this we need to activate the lv and read the qcow2 header while we might have a VM running on another host. It breaks the assumption that we shouldn't be reading from a volume opened in rw by qemu-kvm.

Line 120:     log.debug("Switching the cluster lock for domain %s", domain.sdUUID)
Move this at line 130 inside the future "if" condition.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Add the formatConverter for Storage Domain V3

[Xu He Jie]

Xu He Jie has posted comments on this change.

Change subject: Add the formatConverter for Storage Domain V3
......................................................................

Patch Set 10: (1 inline comment)

Partial review

....................................................
File vdsm/storage/imageRepository/formatConverter.py
Line 34: v2LegacyConverter = partial(legacyConverter, 2)
in FormatConverter::convert will pass 4 arguments to converter implement. this will be failed

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: Add the formatConverter for Storage Domain V3

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Add the formatConverter for Storage Domain V3
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: Add the formatConverter for Storage Domain V3

Ayal Baron has posted comments on this change.

Change subject: Add the formatConverter for Storage Domain V3
......................................................................

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

(11 inline comments)

....................................................
Commit Message
Line 7: Add the formatConverter for Storage Domain V3
which supports what?

....................................................
File vdsm/storage/imageRepository/formatConverter.py
Line 41:     newMetadata._dict.update(metadata)
why use an intermediate?

Line 93:         type(vol).newVolumeLease(metaId, vol.sdUUID, vol.volUUID)
what would happen if lease already exists? (e.g. because of a partial upgrade before)
looking at the blockVolume implementation we call:
sanlock.init_resource(sdUUID, volUUID, [(leasePath, leaseOffset)])

but there is no try..except there or any check to see whether it finished successfully?

Line 97:         metaVolSize = int(vol.getMetaParam(volume.SIZE))
this part should be refactored into a separate function (resetVolSize or something)

Line 123:             # XXX: The only reason to prepare the image is to verify the volume
XXX?

Line 128:             # The analyzed scenario are:
s/scenario/scenarios/

Line 130:             #     This is safe because the prepare is superflous and the
s/superflous/superfluous/g

Line 143:             #     to read the image virtual size) and it can be restarted
why not retry in this case? it is unlikely to fail twice in a row and would improve upgrade process

Line 174:                 newClusterLock.release()
I would've preferred this to be:
with newClusterLock:
 ...

Line 183:             newClusterLock.releaseHostId(hostId)
same here (with ...)

Line 191:         domain._clusterLock.release()
this was never acquired?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: Add the formatConverter for Storage Domain V3

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Add the formatConverter for Storage Domain V3
......................................................................

Patch Set 17: (10 inline comments)

....................................................
File vdsm/storage/imageRepository/formatConverter.py
Line 41:     newMetadata._dict.update(metadata)
Because we changed the DMDK_VERSION value. Is there a different (safe) way of doing this that I don't see at the moment?

Line 93:         type(vol).newVolumeLease(metaId, vol.sdUUID, vol.volUUID)
If sanlock.init_resource fails it throws an exception: sanlock.SanlockException

Line 97:         metaVolSize = int(vol.getMetaParam(volume.SIZE))
Done

Line 123:             # XXX: The only reason to prepare the image is to verify the volume
http://stackoverflow.com/questions/1452934/what-is-the-meaning-of-xxx

Line 128:             # The analyzed scenario are:
Done

Line 130:             #     This is safe because the prepare is superflous and the
Done

Line 143:             #     to read the image virtual size) and it can be restarted
Yes I remember we discussed this. I'm not sure if I can try to do something reliable at the moment (it requires testing). I'll try to postpone this.

Line 174:                 newClusterLock.release()
No time for this.

Line 183:             newClusterLock.releaseHostId(hostId)
No time for this.

Line 191:         domain._clusterLock.release()
I'll add a comment on this.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: Add the formatConverter for Storage Domain V3

Ayal Baron has posted comments on this change.

Change subject: Add the formatConverter for Storage Domain V3
......................................................................

Patch Set 19: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: Add the formatConverter for Storage Domain V3

[Allon Mureinik]

Allon Mureinik has posted comments on this change.

Change subject: Add the formatConverter for Storage Domain V3
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: Add the formatConverter for Storage Domain V3

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Add the formatConverter for Storage Domain V3
......................................................................

Patch Set 19: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Xu He Jie

Change in vdsm[master]: Add the formatConverter for Storage Domain V3

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Add the formatConverter for Storage Domain V3
......................................................................

Add the formatConverter for Storage Domain V3

Signed-off-by: Federico Simoncelli 
Change-Id: I74185a4521fb88444c88de628a18c2210359af29
---
M vdsm/storage/blockSD.py
M vdsm/storage/domainMonitor.py
M vdsm/storage/imageRepository/formatConverter.py
M vdsm/storage/sd.py
M vdsm/storage/sp.py
5 files changed, 217 insertions(+), 71 deletions(-)

Approvals:
  Ayal Baron: Looks good to me, approved
  Federico Simoncelli: Verified
  Allon Mureinik: Looks good to me, but someone else must approve
  Dan Kenigsberg:

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I74185a4521fb88444c88de628a18c2210359af29
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Xu He Jie