Change in vdsm[master]: Added VDSM interface for the virt-alignment-scan tool from l...

ilvovsky at redhat.com ilvovsky at redhat.com
Sun Mar 11 09:07:08 UTC 2012


Igor Lvovsky has posted comments on this change.

Change subject: Added VDSM interface for the virt-alignment-scan tool from libguestfs
......................................................................


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

(11 inline comments)

I know that you probably not familiar with vdsm internals and it's OK. 
If you want to fix all comments it's fine, but if you preferred to send only low level module and we will wrap it with other vdsm stuff it's also OK.
Anyway you did a good job and we very appreciate your contribution.

....................................................
Commit Message
Line 7: Added VDSM interface for the virt-alignment-scan tool from libguestfs
I think it will very helpful if you will explain in few word, what is a point of this tool and what is it's output.

....................................................
File vdsm/alignment.py
Line 28: 
Ooops, sorry. We can't take this,  we need it as GPL v2.

Line 33: import unittest
look at my comment below

Line 47: 
You don't need to implement it again.
We already have this functionality in misk.py.
Look at execCmd()

Line 61:         return {'status':{ 'message':'bad', 'code':1},
hmmm. 'bad' it's not a very good message. I know that you have a full  output message in 'info', but anyway ...

Line 66:            }
I think the prettier way is to return something like:
partitions1: result
partitions2: result

instead of raw output. Try to parse the output and return something more user friendly.

Line 123: 
First of all it's just great that you sent the unit tests for your commit.
Only one thing, please separate the tests from the main vdsm code.

....................................................
File vdsm/API.py
Line 1010:         return {'status': doneCode, 'info': c}
You can't just call scanImage here because of several reasons:
1. You should get disk identifier and not an image path as parameter (look at my comment  on BindingXMLRPC.py)
2. Let's assume that VM of this image is down. It means you should first call to prepareVolume before you access it.

....................................................
File vdsm/BindingXMLRPC.py
Line 283: 
virtAlignmentScan should get a disk identifiers and not a image path as parameter.
 For example:
{'poolID':UUID, 'domainID':UUID, 'imageID': UUID, 'volumeID': UUID} or
{'GUID': GUID}

....................................................
File vdsm_cli/vdsClient.py
Line 379: #                 return stats['status']['code'], stats['info']['message']
Please, remove unneeded code.

....................................................
File vdsm.spec.in
Line 452: %{_datadir}/%{vdsm_name}/alignment.py*
I think , you need to add additional requirements to the spec file
for libguestfs  in proper version

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c329e45905e6cedaeed03e095ac2a4f9ee4ca3f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić <tomic80 at gmail.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>


More information about the vdsm-patches mailing list