Change in vdsm[master]: [WIP] Separate the Volume.share implementation

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: [WIP] Separate the Volume.share implementation
......................................................................

[WIP] Separate the Volume.share implementation

In this patch:
* Separate the Volume.share implementation for block and file domains
* Link also the lease file on file domains (when required)
* Block domains are enforcing the use of the symlinks
* The rollback must be specific for the domain type

Signed-off-by: Federico Simoncelli 
Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
---
M vdsm/storage/blockVolume.py
M vdsm/storage/fileUtils.py
M vdsm/storage/fileVolume.py
M vdsm/storage/volume.py
4 files changed, 81 insertions(+), 28 deletions(-)

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

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

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: [WIP] Separate the Volume.share implementation
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/volume.py
Line 268:                 os.symlink(srcPath, dstPath)
Hmmm, it's run for both block and NFS.
Should we do link/unlink   with oop ?

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

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

Change in vdsm[master]: Separate the Volume.share implementation

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

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

(4 inline comments)

....................................................
File vdsm/storage/fileVolume.py
Line 212:         lease_path = self._getMetaVolumePath(vol_path)
meta or lease ?

....................................................
File vdsm/storage/volume.py
Line 265:             # FIXME: specific rollback per class
Is it still relevant ?

Line 271:             fileUtils.safeUnlink(dstPath)
it's run for both block and NFS. Should we do this with oop ?

Line 276:                 os.symlink(srcPath, dstPath)
As I wrote in previous patchset,
> it's run for both block and NFS. Should we do link/unlink with oop ?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
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]: Separate the Volume.share implementation

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

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

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

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

Change in vdsm[master]: Separate the Volume.share implementation

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

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

(1 inline comment)

....................................................
Commit Message
Line 14: 
sorry, I've lost you. I'd prefer to see a clear statement of the motivation for this refactoring. What connects the 4 bullets above?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 4
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]: Separate the Volume.share implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

Patch Set 4: (1 inline comment)

....................................................
Commit Message
Line 14: 
Volume.share is used in Volume.create. Since Volume.create is transparent across domain types Volume.share must be transparent too (implementing the hard/symbolic link as a detail of the domain type).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 4
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]: Separate the Volume.share implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

Patch Set 4:

The three points to clarify about this patch are:

1. Do we want to be able to specify the type of link (hard, symbolic) when we call Volume.share? (hardLink kwarg)

2. Anyway we need Volume.share to be transparent and being able to do the correct thing (overriding the link type where they don't make sense, eg: hardlinks are not supported on block domains)

3. Are the two implementation so different (eg: using oop) that we don't gain any benefit from having a common Volume.share code to be used by the inheriting classes?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 4
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]: Separate the Volume.share implementation

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

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

(1 inline comment)

Only minor comment

....................................................
File vdsm/storage/blockVolume.py
Line 399:         self.oop.os.symlink(srcPath, dstPath)
It's harmless but it's a local link.  
do we realy need oop here?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 5
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]: Separate the Volume.share implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

Patch Set 5: (1 inline comment)

....................................................
File vdsm/storage/blockVolume.py
Line 399:         self.oop.os.symlink(srcPath, dstPath)
sorry my mistake

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 5
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]: Separate the Volume.share implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

Ayal Baron has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

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

(3 inline comments)

Some minor comments inline.
Commit message should explain motivation for patch, not just what was changed.
(it is not clear that the only really required change is the linking of the lease file)

....................................................
Commit Message
Line 7: Separate the Volume.share implementation
why?

....................................................
File vdsm/storage/fileVolume.py
Line 594:         return type(self).__metaVolumePath(vol_path)
why bother with 'type', self should be sufficient?

Line 602:         return type(self).__leaseVolumePath(vol_path)
same

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

Patch Set 20: (1 inline comment)

....................................................
File vdsm/storage/fileVolume.py
Line 594:         return type(self).__metaVolumePath(vol_path)
It's actually because __metaVolumePath is a classmethod. In practice using self shouldn't make any difference but still the correct way to invocate such method is from the class.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

Ayal Baron has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 21
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

[Allon Mureinik]

Allon Mureinik has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

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

(1 inline comment)

see inline suggestion

....................................................
File vdsm/storage/fileVolume.py
Line 298:         self.oop.os.link(self.getVolumePath(), dstVolPath)
You have this pattern several times:
self.oop.fileUtils.safeUnlink(destPath)
self.oop.os.link(src, destPath)

how about wrapping it in a method? e.g., self.forceLink(src, dst)?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 22
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: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

Ayal Baron has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

Patch Set 22: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 22
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: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

Patch Set 22: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 22
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: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

[Allon Mureinik]

Allon Mureinik has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 24
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: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

Ayal Baron has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

Patch Set 24: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 24
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: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Separate the Volume.share implementation
......................................................................

Patch Set 24: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 24
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: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Separate the Volume.share implementation

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Separate the Volume.share implementation
......................................................................

Separate the Volume.share implementation

The current Volume.share method is exposing the hardlink/symlink option
which is an implementation detail of the domain type (hardlink for file
domains and symlinks for block domains). This makes any external (or
generic) use of such call domain-type dependant.
This patch separates part of the Volume.share implementation to use the
correct link type transparently.

In this patch:
* Separate the Volume.share implementation for block and file domains
* Separate the shareVolumeRollback implementation
* Link also the lease file on file domains (when required)

Signed-off-by: Federico Simoncelli 
Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
---
M vdsm/storage/blockVolume.py
M vdsm/storage/fileUtils.py
M vdsm/storage/fileVolume.py
M vdsm/storage/volume.py
4 files changed, 101 insertions(+), 50 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/3737
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I965b56dc98fe2d6db3ef6f3d7efe2a7b9791b555
Gerrit-PatchSet: 24
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: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi