Bala.FA has posted comments on this change. Change subject: gluster: volume snapshot create and delete verbs. ......................................................................
Patch Set 4: Code-Review-1 (4 comments) http://gerrit.ovirt.org/#/c/34535/4/vdsm.spec.in File vdsm.spec.in: Line 237: %endif Line 238: Line 239: # GlusterFS client-side RPMs needed for Gluster SD Line 240: %if 0%{?with_gluster} Line 241: Requires: glusterfs >= 3.6.1 could you change it to >= 3.6 to work better with downstream too? Sorry for the confusion. Line 242: Requires: glusterfs-cli Line 243: Requires: glusterfs-api Line 244: Requires: glusterfs-fuse Line 245: Requires: glusterfs-rdma http://gerrit.ovirt.org/#/c/34535/4/vdsm/gluster/cli.py File vdsm/gluster/cli.py: Line 1060: raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)]) Line 1061: Line 1062: Line 1063: @makePublic Line 1064: def snapshotCreate(volumeName, snapName, description=None, force=False): How about description to snapInfo? description looks very generic. Line 1065: command = _getGlusterSnapshotCmd() + ["create", snapName, volumeName] Line 1066: Line 1067: if description: Line 1068: command += ['description', description] Line 1079: raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)]) Line 1080: Line 1081: Line 1082: @makePublic Line 1083: def snapshotDelete(snapName=None, deleteAll=False): you could change 'deleteAll' to 'all' Line 1084: command = _getGlusterSnapshotCmd() + ["delete"] Line 1085: if deleteAll: Line 1086: command.append('all') Line 1087: elif snapName: Line 1088: command.append(snapName) Line 1089: else: Line 1090: raise ge.GlusterSnapshotDeleteFailedException( Line 1091: err="Incorrect arguments(either deleteAll should be" Line 1092: " true or snapName has to be provided)") What will happen if this case is sent to gluster cli? IMO we should just pass args to the cli and cli should react accordingly. Line 1093: Line 1094: rc, out, err = _execGluster(command) Line 1095: if rc: Line 1096: raise ge.GlusterSnapshotDeleteFailedException(rc, out, err) -- To view, visit http://gerrit.ovirt.org/34535 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia2d1f1b2f2a675c5407222cb267a7560927ae84f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Darshan N <dnara...@redhat.com> Gerrit-Reviewer: Bala.FA <barum...@redhat.com> Gerrit-Reviewer: Darshan N <dnara...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Timothy Asir <tjeya...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
