Change in vdsm[master]: vm: small refactor for _normalizeVdsmImg

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: vm: small refactor for _normalizeVdsmImg
......................................................................

vm: small refactor for _normalizeVdsmImg

Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Signed-off-by: Federico Simoncelli <fsimonce(a)redhat.com&gt;
---
M vdsm/vm.py
1 file changed, 18 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/57/19157/1

diff --git a/vdsm/vm.py b/vdsm/vm.py
index 92d274e..2605f24 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -1779,25 +1779,26 @@
                 break
         return str(idx)
 
-    def _normalizeVdsmImg(self, drv):
-        drv['reqsize'] = drv.get('reqsize', '0')  # Backward
compatible
-        if 'device' not in drv:
-            drv['device'] = 'disk'
+    def _normalizeVdsmImg(self, drive):
+        drive['device'] = drive.get('device', 'disk')  # Disk by
default
+        drive['reqsize'] = drive.get('reqsize', '0')  # Backward
compatible
 
-        if drv['device'] == 'disk':
-            res = self.cif.irs.getVolumeSize(drv['domainID'],
drv['poolID'],
-                                             drv['imageID'],
drv['volumeID'])
-            try:
-                drv['truesize'] = res['truesize']
-                drv['apparentsize'] = res['apparentsize']
-            except KeyError:
-                self.log.error("Unable to get volume size for %s",
-                               drv['volumeID'], exc_info=True)
-                raise RuntimeError("Volume %s is corrupted or missing" %
-                                   drv['volumeID'])
+        if drive['device'] == 'disk':
+            volInfo = self.cif.irs.getVolumeInfo(
+                drive['domainID'], drive['poolID'],
drive['imageID'],
+                drive['volumeID'])
+
+            if volInfo.get('status', {}).get('code', -1):
+                self.log.error(
+                    "Unable to get volume info for %s",
drive['volumeID'])
+                raise RuntimeError(
+                    "Volume %s is corrupted or missing" %
drive['volumeID'])
+
+            drive['truesize'] = volInfo['info']['truesize']
+            drive['apparentsize'] =
volInfo['info']['apparentsize']
         else:
-            drv['truesize'] = 0
-            drv['apparentsize'] = 0
+            drive['truesize'] = 0
+            drive['apparentsize'] = 0
 
     @classmethod
     def _normalizeDriveSharedAttribute(self, drive):


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;

Change in vdsm[master]: vm: small refactoring for _normalizeVdsmImg

Ayal Baron has posted comments on this change.

Change subject: vm: small refactoring for _normalizeVdsmImg
......................................................................


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: small refactor for _normalizeVdsmImg
......................................................................


Patch Set 1:

Build Successful 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: vm: _normalizeVdsmImg refactoring

Itamar Heim has posted comments on this change.

Change subject: vm: _normalizeVdsmImg refactoring
......................................................................


Patch Set 4:

ping

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: vm: small refactoring for _normalizeVdsmImg

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: small refactoring for _normalizeVdsmImg
......................................................................


Patch Set 2:

Build Successful 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: vm: _normalizeVdsmImg refactoring

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: _normalizeVdsmImg refactoring
......................................................................


Patch Set 4:

(3 comments)

The refactoring is nice (name changes, use dict.get with defualt instead of using
if-else.

I don't see why getVolumeInfo is better, and the commit mesage should explain it.

I'm not sure about the way the code is tring to check the status code, this will make
it harder to fix bugs in irs code.

The behavior changes (using getVolumeInfo instead of getVolumeSize and way of checking
status code) should be split into separate patch.

http://gerrit.ovirt.org/#/c/19157/4/vdsm/vm.py
File vdsm/vm.py:

Line 2037:         return str(idx)
Line 2038: 
Line 2039:     def _normalizeVdsmImg(self, drive):
Line 2040:         drive['device'] = drive.get('device', 'disk') 
# Disk by default
Line 2041:         drive['reqsize'] = drive.get('reqsize', '0')  #
Backward compatible
Nice
Line 2042: 
Line 2043:         if drive['device'] == 'disk':
Line 2044:             volInfo = self.cif.irs.getVolumeInfo(
Line 2045:                 drive['domainID'], drive['poolID'],
drive['imageID'],


Line 2040:         drive['device'] = drive.get('device', 'disk') 
# Disk by default
Line 2041:         drive['reqsize'] = drive.get('reqsize', '0')  #
Backward compatible
Line 2042: 
Line 2043:         if drive['device'] == 'disk':
Line 2044:             volInfo = self.cif.irs.getVolumeInfo(
>  I'll split this change out. 
Can you explain in the commit
message why getVolumeInfo is better then getVolumeSize?
Line 2045:                 drive['domainID'], drive['poolID'],
drive['imageID'],
Line 2046:                 drive['volumeID'])
Line 2047: 
Line 2048:             if volInfo.get('status', {}).get('code', -1):


Line 2044:             volInfo = self.cif.irs.getVolumeInfo(
Line 2045:                 drive['domainID'], drive['poolID'],
drive['imageID'],
Line 2046:                 drive['volumeID'])
Line 2047: 
Line 2048:             if volInfo.get('status', {}).get('code', -1):
I'm not sure about this - since it will hide a bad response from irs. We expect to get
a dict with status dict containing a code key. If we don't get this, I prefer to get a
KeyError  so we can fix irs.
Line 2049:                 self.log.error(
Line 2050:                     'unable to get volume info for %s (response: %s)',
Line 2051:                     drive['volumeID'], volInfo)
Line 2052:                 raise StorageUnavailableError(


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vm: _normalizeVdsmImg refactoring

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: _normalizeVdsmImg refactoring
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.ovirt.org/#/c/19157/4//COMMIT_MSG
Commit Message:

Line 4: Commit:     Federico Simoncelli <fsimonce(a)redhat.com&gt;
Line 5: CommitDate: 2014-02-21 05:56:20 -0500
Line 6: 
Line 7: vm: _normalizeVdsmImg refactoring
Line 8: 
Commit message to be improved once we decide to keep this
Line 9: Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa


http://gerrit.ovirt.org/#/c/19157/4/vdsm/vm.py
File vdsm/vm.py:

Line 2040:         drive['device'] = drive.get('device', 'disk') 
# Disk by default
Line 2041:         drive['reqsize'] = drive.get('reqsize', '0')  #
Backward compatible
Line 2042: 
Line 2043:         if drive['device'] == 'disk':
Line 2044:             volInfo = self.cif.irs.getVolumeInfo(
I'll split this change out.
Line 2045:                 drive['domainID'], drive['poolID'],
drive['imageID'],
Line 2046:                 drive['volumeID'])
Line 2047: 
Line 2048:             if volInfo.get('status', {}).get('code', -1):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <majopela(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vm: _normalizeVdsmImg refactoring

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: _normalizeVdsmImg refactoring
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/19157/3/vdsm/vm.py
File vdsm/vm.py:

Line 2040:         drive['device'] = drive.get('device', 'disk') 
# Disk by default
Line 2041:         drive['reqsize'] = drive.get('reqsize', '0')  #
Backward compatible
Line 2042: 
Line 2043:         if drive['device'] == 'disk':
Line 2044:             volInfo = self.cif.irs.getVolumeInfo(
I'll split this change out.
Line 2045:                 drive['domainID'], drive['poolID'],
drive['imageID'],
Line 2046:                 drive['volumeID'])
Line 2047: 
Line 2048:             if volInfo['status']['code'] != 0:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <majopela(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vm: _normalizeVdsmImg refactoring

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: _normalizeVdsmImg refactoring
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/19157/3//COMMIT_MSG
Commit Message:

Line 4: Commit:     Federico Simoncelli <fsimonce(a)redhat.com&gt;
Line 5: CommitDate: 2014-02-21 05:43:42 -0500
Line 6: 
Line 7: vm: _normalizeVdsmImg refactoring
Line 8: 
Commit message to be improved once we decide to keep this.
Line 9: Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <majopela(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: vm: _normalizeVdsmImg refactoring

Itamar Heim has abandoned this change.

Change subject: vm: _normalizeVdsmImg refactoring
......................................................................


Abandoned

no activity. please restore if relevant.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ie68292eee4b82fbe8527e3960739979cfe117dfa
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server