Change in vdsm[master]: gluster: volume snapshot list verb.

dnarayan at redhat.com dnarayan at redhat.com
Tue Feb 3 13:28:45 UTC 2015


Darshan N has posted comments on this change.

Change subject: gluster: volume snapshot list verb.
......................................................................


Patch Set 12:

(7 comments)

http://gerrit.ovirt.org/#/c/36087/12/vdsm/gluster/cli.py
File vdsm/gluster/cli.py:

Line 1232:     volume[volumeName] = {}
Line 1233:     volume[volumeName]['snapshots'] = []
Line 1234:     volume[volumeName]['snapRemaining'] = tree.find(
Line 1235:         'snapInfo/originVolume/snapRemaining'
Line 1236:     ).text
> how about 
Done
Line 1237:     volume[volumeName]['snapCount'] = tree.find(
Line 1238:         'snapInfo/originVolume/snapCount'
Line 1239:     ).text
Line 1240:     if volume[volumeName]['snapCount'] == 0:


Line 1235:         'snapInfo/originVolume/snapRemaining'
Line 1236:     ).text
Line 1237:     volume[volumeName]['snapCount'] = tree.find(
Line 1238:         'snapInfo/originVolume/snapCount'
Line 1239:     ).text
> Please remove snapCount which is a duplicate and it can be get by len(volum
Done
Line 1240:     if volume[volumeName]['snapCount'] == 0:
Line 1241:         return {}
Line 1242:     for el in tree.findall('snapInfo/snapshots/snapshot'):
Line 1243:         snapshot = {}


Line 1236:     ).text
Line 1237:     volume[volumeName]['snapCount'] = tree.find(
Line 1238:         'snapInfo/originVolume/snapCount'
Line 1239:     ).text
Line 1240:     if volume[volumeName]['snapCount'] == 0:
> Does the output have missing 'snapInfo/snapshots' or 'snapInfo/snapshots' i
Done
Line 1241:         return {}
Line 1242:     for el in tree.findall('snapInfo/snapshots/snapshot'):
Line 1243:         snapshot = {}
Line 1244:         snapshot['snapshotUuid'] = el.find('uuid').text


Line 1242:     for el in tree.findall('snapInfo/snapshots/snapshot'):
Line 1243:         snapshot = {}
Line 1244:         snapshot['snapshotUuid'] = el.find('uuid').text
Line 1245:         snapshot['snapDescription'] = "" if el.find('description') is None\
Line 1246:                                       else el.find('description').text
> cannot the below work?
if el.find('description') is None then the above statement will raise an exception
Line 1247:         snapshot['createTime'] = el.find('createTime').text
Line 1248:         snapshot['snapVolume'] = el.find('snapVolume/name').text
Line 1249:         snapshot['snapVolumeStatus'] = el.find('snapVolume/status').text
Line 1250:         snapshot['snapshotName'] = el.find('name').text


Line 1245:         snapshot['snapDescription'] = "" if el.find('description') is None\
Line 1246:                                       else el.find('description').text
Line 1247:         snapshot['createTime'] = el.find('createTime').text
Line 1248:         snapshot['snapVolume'] = el.find('snapVolume/name').text
Line 1249:         snapshot['snapVolumeStatus'] = el.find('snapVolume/status').text
> you should send status as 'ACTIVATED' or 'DEACTIVATED' than 'Started' or so
Done
Line 1250:         snapshot['snapshotName'] = el.find('name').text
Line 1251:         volume[volumeName]['snapshots'].append(snapshot)
Line 1252:     return volume
Line 1253: 


Line 1293:             volumes[volumeName]['snapRemaining'] = el.find(
Line 1294:                 'snapVolume/originVolume/snapRemaining').text
Line 1295:             volumes[volumeName]['snapCount'] = el.find(
Line 1296:                 'snapVolume/originVolume/snapCount').text
Line 1297:         volumes[volumeName]['snapshots'].append(snapshot)
> all above comments apply here
Done
Line 1298:     return volumes
Line 1299: 
Line 1300: 
Line 1301: @makePublic


Line 1311:     try:
Line 1312:         if volumeName:
Line 1313:             return _parseVolumeSnapshotList(xmltree)
Line 1314:         else:
Line 1315:             return _parseSnapshotList(xmltree)
> How about having this function name as _parseAllVolumeSnapshotList()?  If y
Done
Line 1316:     except _etreeExceptions:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06945d6781432b7fb40e417dd21c0ecf107de132
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Sahina Bose <sabose at redhat.com>
Gerrit-Reviewer: Shubhendu Tripathi <shtripat at redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list