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ć <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/vdsm-patches
