Change in vdsm[master]: Deprecate volume mtime

[Dan Kenigsberg]

Dan Kenigsberg has uploaded a new change for review.

Change subject: Deprecate volume mtime
......................................................................

Deprecate volume mtime

In pre-historic 2.y days, Engine used volume's "mtime" attribute to
deduces the ancestry of a volume. See for example BZ#513396.
In case of an error reading the metadata, "0" or "" was returned.
This was a bad heuristic that was fixed to using proper pointer to
parents and children.

Engine 3.y continues to use the mtime field only when importing ancient
images that lack proper creation time in their OVF.

Maintaining this attribure requires costly readings of volume metadata.

This patch drops the mtime-related code. For backward compatibility with
old Vdsms and Engines that may expect the existance of this attribute,
we keep writing "0" to the metadata and reporting "0" to Engine.

Change-Id: I38b0a636222fa74125d25f0c3c9ea5b3e5701565
Signed-off-by: Dan Kenigsberg 
---
M vdsm/storage/blockVolume.py
M vdsm/storage/fileVolume.py
M vdsm/storage/volume.py
M vdsm_api/vdsmapi-schema.json
4 files changed, 3 insertions(+), 29 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/04/20504/1

diff --git a/vdsm/storage/blockVolume.py b/vdsm/storage/blockVolume.py
index 923f93a..20c6f75 100644
--- a/vdsm/storage/blockVolume.py
+++ b/vdsm/storage/blockVolume.py
@@ -623,17 +623,6 @@

getVolumeTrueSize = getVolumeSize

-    def getVolumeMtime(self):
-        """
-        Return the volume mtime in msec epoch
-        """
-        try:
-            mtime = self.getMetaParam(volume.MTIME)
-        except se.MetaDataKeyNotFoundError:
-            mtime = 0
-
-        return mtime
-
     def _extendSizeRaw(self, newSize):
         # Since this method relies on lvm.extendLV (lvextend) when the
         # requested size is equal or smaller than the current size, the
diff --git a/vdsm/storage/fileVolume.py b/vdsm/storage/fileVolume.py
index 1ae90de..c9ec5a1 100644
--- a/vdsm/storage/fileVolume.py
+++ b/vdsm/storage/fileVolume.py
@@ -568,16 +568,6 @@
         volPath = self.getVolumePath()
         return int(int(self.oop.os.stat(volPath).st_blocks) * BLOCK_SIZE / bs)

-    def getVolumeMtime(self):
-        """
-        Return the volume mtime in msec epoch
-        """
-        volPath = self.getVolumePath()
-        try:
-            return self.getMetaParam(volume.MTIME)
-        except se.MetaDataKeyNotFoundError:
-            return self.oop.os.stat(volPath).st_mtime
-
     def _extendSizeRaw(self, newSize):
         volPath = self.getVolumePath()
         curSizeBytes = self.oop.os.stat(volPath).st_size
diff --git a/vdsm/storage/volume.py b/vdsm/storage/volume.py
index fc7a3e5..341a54d 100644
--- a/vdsm/storage/volume.py
+++ b/vdsm/storage/volume.py
@@ -807,9 +807,6 @@
         self.updateInvalidatedSize()

try:
-            # Mtime is the time of the last prepare for RW
-            if rw:
-                self.setMetaParam(MTIME, int(time.time()))
             if justme:
                 return True
             pvol = self.getParentVolume()
@@ -845,7 +842,7 @@
             "domain": meta.get(DOMAIN, ""),
             "image": self.getImage(),
             "ctime": meta.get(CTIME, ""),
-            "mtime": meta.get(MTIME, ""),
+            "mtime": "0",
             "legality": meta.get(LEGALITY, ""),
         }

@@ -864,7 +861,7 @@
             IMAGE: str(imgUUID),
             DESCRIPTION: str(desc),
             PUUID: str(puuid),
-            MTIME: int(time.time()),
+            MTIME: 0,
             LEGALITY: str(legality),
         }

@@ -888,13 +885,11 @@
             avsize = self.getVolumeTrueSize(bs=1)
             info['apparentsize'] = str(vsize)
             info['truesize'] = str(avsize)
-            info['mtime'] = self.getVolumeMtime()
             info['status'] = "OK"
         except se.StorageException as e:
             self.log.debug("exception: %s:%s" % (str(e.message), str(e.value)))
             info['apparentsize'] = "0"
             info['truesize'] = "0"
-            info['mtime'] = "0"
             info['status'] = "INVALID"

# Both engine and dumpStorageTable don't use this option so
diff --git a/vdsm_api/vdsmapi-schema.json b/vdsm_api/vdsmapi-schema.json
index 73889d1..19a9191 100644
--- a/vdsm_api/vdsmapi-schema.json
+++ b/vdsm_api/vdsmapi-schema.json
@@ -6465,7 +6465,7 @@
 #
 # @ctime:         The Volume creation time in seconds since the epoch
 #
-# @mtime:         The Volume modification time in seconds since the epoch
+# @mtime:         Deprecated
 #
 # @legality:      Indicates whether the volume is legal to use
 #

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Deprecate volume mtime
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5058/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4254/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5132/ : SUCCESS

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Deprecate volume mtime
......................................................................

Patch Set 1:

(2 comments)

....................................................
File vdsm_api/vdsmapi-schema.json
Line 6464: # @image:         The Image associated with the Volume
Line 6465: #
Line 6466: # @ctime:         The Volume creation time in seconds since the epoch
Line 6467: #
Line 6468: # @mtime:         Deprecated
Maybe keeping the description is better (mentioning that it's deprecated).
Line 6469: #
Line 6470: # @legality:      Indicates whether the volume is legal to use
Line 6471: #
Line 6472: # @apparentsize:  The size of the Volume (in bytes)

Line 6483:  'data': {'uuid': 'UUID', 'allocType': 'VolumeAllocation',
Line 6484:           'format': 'VolumeFormat', 'disktype': 'DiskType',
Line 6485:           'voltype': 'VolumeRole', 'capacity': 'uint', 'parent': 'UUID',
Line 6486:           'description': 'str', 'pool': 'UUID', 'domain': 'UUID',
Line 6487:           'image': 'UUID', 'ctime': 'int', 'mtime': 'uint',
I suppose now mtime should be optional (*mtime). (You're not reporting it anymore if I understand correctly).
Line 6488:           'legality': 'VolumeLegality', 'apparentsize': 'uint',
Line 6489:           'truesize': 'uint', 'status': 'VolumeStatus', 'children': ['UUID']}}
Line 6490: 
Line 6491: ##

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Deprecate volume mtime
......................................................................

Patch Set 1:

(2 comments)

....................................................
File vdsm_api/vdsmapi-schema.json
Line 6464: # @image:         The Image associated with the Volume
Line 6465: #
Line 6466: # @ctime:         The Volume creation time in seconds since the epoch
Line 6467: #
Line 6468: # @mtime:         Deprecated
Done
Line 6469: #
Line 6470: # @legality:      Indicates whether the volume is legal to use
Line 6471: #
Line 6472: # @apparentsize:  The size of the Volume (in bytes)

Line 6483:  'data': {'uuid': 'UUID', 'allocType': 'VolumeAllocation',
Line 6484:           'format': 'VolumeFormat', 'disktype': 'DiskType',
Line 6485:           'voltype': 'VolumeRole', 'capacity': 'uint', 'parent': 'UUID',
Line 6486:           'description': 'str', 'pool': 'UUID', 'domain': 'UUID',
Line 6487:           'image': 'UUID', 'ctime': 'int', 'mtime': 'uint',
Actually, as stated in the commit message, I keep reporting it (as "0") just in case someone out there expects its existence.
Line 6488:           'legality': 'VolumeLegality', 'apparentsize': 'uint',
Line 6489:           'truesize': 'uint', 'status': 'VolumeStatus', 'children': ['UUID']}}
Line 6490: 
Line 6491: ##

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Deprecate volume mtime
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4363/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5167/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5243/ : SUCCESS

Saggi Mizrahi has posted comments on this change.

Change subject: Deprecate volume mtime
......................................................................

Patch Set 2:

One of SaggiMizrahi's automated scripts discovered this patch might require his approval.
Please wait until he had time to check it out.

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Deprecate volume mtime
......................................................................

Patch Set 2: Code-Review+1

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Deprecate volume mtime
......................................................................

Patch Set 2: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Deprecate volume mtime
......................................................................

Patch Set 2: Verified+1

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Deprecate volume mtime
......................................................................

Deprecate volume mtime

In pre-historic 2.y days, Engine used volume's "mtime" attribute to
deduces the ancestry of a volume. See for example BZ#513396.
In case of an error reading the metadata, "0" or "" was returned.
This was a bad heuristic that was fixed to using proper pointer to
parents and children.

Engine 3.y continues to use the mtime field only when importing ancient
images that lack proper creation time in their OVF.

Maintaining this attribure requires costly readings of volume metadata.

This patch drops the mtime-related code. For backward compatibility with
old Vdsms and Engines that may expect the existance of this attribute,
we keep writing "0" to the metadata and reporting "0" to Engine.

Change-Id: I38b0a636222fa74125d25f0c3c9ea5b3e5701565
Signed-off-by: Dan Kenigsberg 
Reviewed-on: http://gerrit.ovirt.org/20504
Reviewed-by: Federico Simoncelli 
---
M vdsm/storage/blockVolume.py
M vdsm/storage/fileVolume.py
M vdsm/storage/volume.py
M vdsm_api/vdsmapi-schema.json
4 files changed, 4 insertions(+), 30 deletions(-)

Approvals:
  Federico Simoncelli: Looks good to me, approved
  Dan Kenigsberg: Verified