Mark Wu has posted comments on this change.

Change subject: rest-api: Model volumes
......................................................................


Patch Set 6: I would prefer that you didn't submit this

(1 inline comment)

Sorry for not raising this problem earlier. I just notice this problem when I 
did some test yesterday.

....................................................
File vdsm/rest/Controller.py
Line 362:         role = Volume.ROLES.get(self.info['voltype'].lower(), None)
According to the definition of createVolume:
    def create(self, size, volFormat, preallocate, diskType, desc,
               srcImgUUID, srcVolUUID):
        return self._irs.createVolume(self._sdUUID, self._spUUID,
                self._imgUUID, size, volFormat, preallocate, diskType,
                self._UUID, desc, srcImgUUID, srcVolUUID)

I think the parameter 'diskType' is misused here. What the api expects is 
diskType:

# Disk type
UNKNOWN_DISK_TYPE = 0
SYSTEM_DISK_TYPE = 1
DATA_DISK_TYPE = 2
SHARED_DISK_TYPE = 3
SWAP_DISK_TYPE = 4
TEMP_DISK_TYPE = 5

, not the volume role, which is only used in copyImage() to create a template. 
In other cases, we needn't care about the volume role. vdsm can set it properly:
new created volume except template is always leaf node. If another volume 
created based on it, its role will be changed to internal. And the role of 
template is shared, which is set on creation. That's my understanding.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5a34e02c84c3668dff883b3a618d420edcf60be
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <a...@us.ibm.com>
Gerrit-Reviewer: Adam Litke <a...@us.ibm.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com>
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to