Change in vdsm[master]: StorageDomainManifest: move getLeasesFilePath

Adam Litke has uploaded a new change for review.

Change subject: StorageDomainManifest: move getLeasesFilePath
......................................................................

StorageDomainManifest: move getLeasesFilePath

getLeasesFilePath is needed by VolumeMetadata to implement the
newVolumeLease function on block storage.  Move it into the Manifest
class which can be safely instantiated.

Change-Id: I2bb49591357e0ae77268789da78e8504864a8057
Signed-off-by: Adam Litke 
---
M vdsm/storage/blockSD.py
M vdsm/storage/sd.py
2 files changed, 14 insertions(+), 8 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/95/41995/1

diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py
index 362ee28..7ccb7e5 100644
--- a/vdsm/storage/blockSD.py
+++ b/vdsm/storage/blockSD.py
@@ -419,6 +419,10 @@

return int(size)

+    def getLeasesFilePath(self):
+        lvm.activateLVs(self.sdUUID, [sd.LEASES])
+        return lvm.lvPath(self.sdUUID, sd.LEASES)
+

class BlockStorageDomain(sd.StorageDomain):
     ManifestClass = BlockStorageDomainManifest
@@ -817,10 +821,6 @@
     def getIdsFilePath(self):
         lvm.activateLVs(self.sdUUID, [sd.IDS])
         return lvm.lvPath(self.sdUUID, sd.IDS)
-
-    def getLeasesFilePath(self):
-        lvm.activateLVs(self.sdUUID, [sd.LEASES])
-        return lvm.lvPath(self.sdUUID, sd.LEASES)

def getLeasesFileSize(self):
         lv = lvm.getLV(self.sdUUID, sd.LEASES)
diff --git a/vdsm/storage/sd.py b/vdsm/storage/sd.py
index ab86987..fd4cecf 100644
--- a/vdsm/storage/sd.py
+++ b/vdsm/storage/sd.py
@@ -299,6 +299,14 @@
         self.domaindir = domaindir
         self._metadata = metadata

+    def getMDPath(self):
+        if self.domaindir:
+            return os.path.join(self.domaindir, DOMAIN_META_DATA)
+        return None
+
+    def getLeasesFilePath(self):
+        return os.path.join(self.getMDPath(), LEASES)
+

class StorageDomain(object):
     log = logging.getLogger("Storage.StorageDomain")
@@ -459,9 +467,7 @@
             preallocate, diskType, volUUID, desc, srcImgUUID, srcVolUUID)

def getMDPath(self):
-        if self.domaindir:
-            return os.path.join(self.domaindir, DOMAIN_META_DATA)
-        return None
+        return self.manifest.getMDPath()

def initSPMlease(self):
         """
@@ -497,7 +503,7 @@
         return os.path.join(self.getMDPath(), IDS)

def getLeasesFilePath(self):
-        return os.path.join(self.getMDPath(), LEASES)
+        return self.manifest.getLeasesFilePath()

def getReservedId(self):
         return self._clusterLock.getReservedId()

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move getLeasesFilePath
......................................................................

Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomainManifest: move getLeasesFilePath
......................................................................

Patch Set 1: Code-Review+1

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomainManifest: move getLeasesFilePath
......................................................................

Patch Set 1:

The ci failures are fixed by https://gerrit.ovirt.org/42006

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move getLeasesFilePath
......................................................................

Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomainManifest: move getLeasesFilePath
......................................................................

Patch Set 2:

(4 comments)

https://gerrit.ovirt.org/#/c/41995/2/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 423: 
Line 424:         return int(size)
Line 425: 
Line 426:     def getLeasesFilePath(self):
Line 427:         lvm.activateLVs(self.sdUUID, [sd.LEASES])
Activating lvs as a side offfect of getting the path is very wrong. Please add a TODO so we do not forget to fix this.
Line 428:         return lvm.lvPath(self.sdUUID, sd.LEASES)
Line 429: 
Line 430: 
Line 431: class BlockStorageDomain(sd.StorageDomain):

https://gerrit.ovirt.org/#/c/41995/2/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 290
Line 291
Line 292
Line 293
Line 294
I'm not sure we need this super class, better if we can eliminate it.

Line 305: 
Line 306:     def replaceMetadata(self, md):
Line 307:         self._metadata = md
Line 308: 
Line 309:     def getLeasesFilePath(self):
This is also can be in FileStorageDomainManifest
Line 310:         return os.path.join(self.getMDPath(), LEASES)
Line 311: 
Line 312:     def getMDPath(self):
Line 313:         if self.domaindir:

Line 308: 
Line 309:     def getLeasesFilePath(self):
Line 310:         return os.path.join(self.getMDPath(), LEASES)
Line 311: 
Line 312:     def getMDPath(self):
This looks like file storage domain method that sneaked into StorageDomain. While we move stuff, I would be nice to eliminate such errors and move this the the file based manifest.
Line 313:         if self.domaindir:
Line 314:             return os.path.join(self.domaindir, DOMAIN_META_DATA)
Line 315:         return None
Line 316:

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move getLeasesFilePath
......................................................................

Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Adam Litke has posted comments on this change.

Change subject: StorageDomainManifest: move getLeasesFilePath
......................................................................

Patch Set 2:

(4 comments)

https://gerrit.ovirt.org/#/c/41995/2/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 423: 
Line 424:         return int(size)
Line 425: 
Line 426:     def getLeasesFilePath(self):
Line 427:         lvm.activateLVs(self.sdUUID, [sd.LEASES])
> Activating lvs as a side offfect of getting the path is very wrong. Please

Done
Line 428:         return lvm.lvPath(self.sdUUID, sd.LEASES)
Line 429: 
Line 430: 
Line 431: class BlockStorageDomain(sd.StorageDomain):

https://gerrit.ovirt.org/#/c/41995/2/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 290
Line 291
Line 292
Line 293
Line 294
> I'm not sure we need this super class, better if we can eliminate it.

There are plenty of methods common to both types that will need to be here.  I don't think we can remove it.  The biggest example are metadata getters/setters (ie. getDescription / getInfo) which rely on the low-level metadata operations implemented in the child Manifest classes.

Line 305: 
Line 306:     def replaceMetadata(self, md):
Line 307:         self._metadata = md
Line 308: 
Line 309:     def getLeasesFilePath(self):
> This is also can be in FileStorageDomainManifest

Done
Line 310:         return os.path.join(self.getMDPath(), LEASES)
Line 311: 
Line 312:     def getMDPath(self):
Line 313:         if self.domaindir:

Line 308: 
Line 309:     def getLeasesFilePath(self):
Line 310:         return os.path.join(self.getMDPath(), LEASES)
Line 311: 
Line 312:     def getMDPath(self):
> This looks like file storage domain method that sneaked into StorageDomain.

Unfortunately it's an interface of StorageDomain that is being used in sp.py so we cannot remove it.
Line 313:         if self.domaindir:
Line 314:             return os.path.join(self.domaindir, DOMAIN_META_DATA)
Line 315:         return None
Line 316:

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 5:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

Adam Litke has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 5: Verified+1

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 6:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 7:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

Adam Litke has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 7: Verified+1

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 8:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 9:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 10:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 11:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

Adam Litke has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 11: Verified+1

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 12:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 12:

(1 comment)

https://gerrit.ovirt.org/#/c/41995/12/tests/manifest_tests.py
File tests/manifest_tests.py:

Line 45:             imguuid, voluuid = make_volume(manifest.domaindir, VOLSIZE)
Line 46:             self.assertEqual(VOLSIZE, manifest.getVSize(imguuid, voluuid))
Line 47: 
Line 48:     def test_getisodomainimagesdir(self):
Line 49:         with namedTemporaryDir() as path:
Do we need always temporary directory? Can use unbound manifest for testing?
Line 50:             manifest = self.make_manifest(path)
Line 51:             isopath = os.path.join(manifest.domaindir, sd.DOMAIN_IMAGES,
Line 52:                                    sd.ISO_IMAGE_UUID)
Line 53:             self.assertEquals(isopath, manifest.getIsoDomainImagesDir())

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 13:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

Adam Litke has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 13: Verified+1

Change in vdsm[master]: StorageDomainManifest: move path getters

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 13: Code-Review+1

(1 comment)

https://gerrit.ovirt.org/#/c/41995/13/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 320:         """
Line 321:         return os.path.join(self.domaindir, DOMAIN_IMAGES, ISO_IMAGE_UUID)
Line 322: 
Line 323:     def getMDPath(self):
Line 324:         if self.domaindir:
Is this possible? I would like to eliminate the possible None return value.
Line 325:             return os.path.join(self.domaindir, DOMAIN_META_DATA)
Line 326:         return None
Line 327: 
Line 328:

Change in vdsm[master]: StorageDomainManifest: move path getters

Adam Litke has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 13:

(1 comment)

https://gerrit.ovirt.org/#/c/41995/13/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 320:         """
Line 321:         return os.path.join(self.domaindir, DOMAIN_IMAGES, ISO_IMAGE_UUID)
Line 322: 
Line 323:     def getMDPath(self):
Line 324:         if self.domaindir:
> Is this possible? I would like to eliminate the possible None return value.

sp.py checks for None and raises an exception.  It must be needed for some reason.  Anyway, changing this should be done as a separate patch.  This refactoring series is not the appropriate place to make such a change.
Line 325:             return os.path.join(self.domaindir, DOMAIN_META_DATA)
Line 326:         return None
Line 327: 
Line 328:

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 14:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

Adam Litke has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 14: Verified+1

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 15:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: StorageDomainManifest: move path getters

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 13:

(1 comment)

https://gerrit.ovirt.org/#/c/41995/13/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 320:         """
Line 321:         return os.path.join(self.domaindir, DOMAIN_IMAGES, ISO_IMAGE_UUID)
Line 322: 
Line 323:     def getMDPath(self):
Line 324:         if self.domaindir:
> sp.py checks for None and raises an exception.  It must be needed for some

Right, lets add a TODO here.
Line 325:             return os.path.join(self.domaindir, DOMAIN_META_DATA)
Line 326:         return None
Line 327: 
Line 328:

Change in vdsm[master]: StorageDomainManifest: move path getters

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 15: Code-Review+1

Looks fine, would be nice to add a TODO so we don't forget to check the None return value in getMDPath().

Change in vdsm[master]: StorageDomainManifest: move path getters

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 15: Code-Review+2

Change in vdsm[master]: StorageDomainManifest: move path getters

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: StorageDomainManifest: move path getters
......................................................................

StorageDomainManifest: move path getters

getLeasesFilePath is needed by VolumeMetadata to implement the
newVolumeLease function on block storage.  Move it into the Manifest
class which can be safely instantiated.  Also move topically similar
getIdsFilePath and getIsoDomainImagesDir.

Tests:
 - file: get the metadata path
 - file: get the iso domain images path

Change-Id: I2bb49591357e0ae77268789da78e8504864a8057
Signed-off-by: Adam Litke 
Reviewed-on: https://gerrit.ovirt.org/41995
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
Reviewed-by: Federico Simoncelli 
---
M tests/manifest_tests.py
M vdsm/storage/blockSD.py
M vdsm/storage/fileSD.py
M vdsm/storage/sd.py
4 files changed, 44 insertions(+), 17 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Adam Litke: Verified
  Federico Simoncelli: Looks good to me, approved
  Jenkins CI: Passed CI tests

Change in vdsm[master]: StorageDomainManifest: move path getters

[e-mail] has posted comments on this change.

Change subject: StorageDomainManifest: move path getters
......................................................................

Patch Set 16:

* Update tracker::IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.