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

Saša Tomić has uploaded a new change for review.

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

Added VDSM interface for the virt-alignment-scan tool from libguestfs

The libguestfs-tools rpm has an application called virt-alignment-scan.
http://libguestfs.org/virt-alignment-scan.1.html

Dan Kenigsberg suggested me to expose this tool via Vdsm, so that
vdsClient -s 0 virtAlignmentScan /path/to/image
returns a list of all partitions of the image file.

Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
---
A vdsm/alignmentScan.py
A vdsm/alignmentScan_test.py
2 files changed, 131 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/16/2916/1
--
To view, visit http://gerrit.ovirt.org/2916
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 1: (9 inline comments)

....................................................
Commit Message
Line 16: Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
You should have used the same Change-Id of http://gerrit.ovirt.org/2687
so we could compare this version to our comments to the former one.

Now I guess your best option is to abandon the old post, so it does not confuse us.

....................................................
File vdsm/alignmentScan.py
Line 25: def runScanArgs(args):
this should be local to the module (not that I very much like this function).

Line 31:     image_path = image_path.strip()
do not strip - trailing spaces may be significant.

Line 33:         raise ValueError("Image path does not exist")
I do not see a point in checking path existence here - let the tool fail and report its error.

Line 35:     retcode, out, err = runScanArgs(args)
please check the value of retcode, and raise an exception on error.

Line 42:         return {'status':{ 'message':out_list, 'code':1},
please do not use the awkward external API vdsm verbs have, as this is an internal function. out_list is not a message...

Instead, define a cleare API for this function: it could be a list of named tuples, each one representing a different partition.

....................................................
File vdsm/alignmentScan_test.py
Line 1: #!/usr/bin/python
there's a tests sub directory for unittests, please help us filling it up!

Line 46:     execCmd(cmd.split())
splitting on spaces is bad practice (space may appear withing imagepath). better start with a list literal.

Line 50:         img = "/tmp/aligncheck_bad.img"
please use tempdir.mktemp - who knows, two people may want to run this in conjunction.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg

[Dan Kenigsberg]

Dan Kenigsberg 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

Please do not be alarmed by the number of commit ;-)
Thanks for your code!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg

Saša Tomić has posted comments on this change.

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

Patch Set 1: (9 inline comments)

I updated the source code in my branch. Should I:
(1) submit it as another commit + git-review, OR
(2) do a git rebase, and merge the changes with the previous commit?

....................................................
Commit Message
Line 16: Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Done

....................................................
File vdsm/alignmentScan.py
Line 25: def runScanArgs(args):
It's a DRY function, it's useful for testing, and can also be useful if we have more than one calling function. For example, beside scanImage we could have scanURI.

Line 31:     image_path = image_path.strip()
Done

Line 33:         raise ValueError("Image path does not exist")
Running the tool takes several minutes on my machine, and it *must* fail if the image does not exist. This is simply an optimization.

Line 35:     retcode, out, err = runScanArgs(args)
I do some checking of the return code, but I raise an exception only if the checking failed. Please verify that this is OK.

Line 42:         return {'status':{ 'message':out_list, 'code':1},
Done

....................................................
File vdsm/alignmentScan_test.py
Line 1: #!/usr/bin/python
The problem with this unit test is that it takes too long, 2-3 (or more) minutes to finish. It's the virt-alignment-scan that is so slow. I was actually considering that, but I gave up since testing is done during every rpm build.
It would be good to have "long" and "medium" and "quick" tests in the project.

Line 46:     execCmd(cmd.split())
I agree in general. However, I control the paths here. If you believe other implementation would be better here, please tell me.

Line 50:         img = "/tmp/aligncheck_bad.img"
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Saša Tomić

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 1:

Unless you have a good reason to have two separate commits, you should squash them into one commit, and update the commit message to make sure it has the correct Change-Id.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Saša Tomić

Ewoud Kohl van Wijngaarden has posted comments on this change.

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

Patch Set 2: (3 inline comments)

....................................................
File vdsm/alignmentScan.py
Line 27:     cmd.extend(args)
these two can be joined in a single line: ['...'] + args

Line 48:     out_list = [(a[0], a[1], a[2], " ".join(a[3:])) for a in out_list]
Besides not replacing all whitespace to single spaces and a list instead of tuple this is equal, but more efficient:

out_list = [line.split(None, 3) for line in out]

....................................................
File vdsm/alignmentScan_test.py
Line 51:         img = tempfile.mkstemp()[1]
I prefer `with tempfile.NamedTemporaryFile() as f` since you don't need to worry about removing it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

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

(1 inline comment)

....................................................
File vdsm/alignmentScan_test.py
Line 1: #!/usr/bin/python
please put in the (somewhat empty) "tests" directory.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Saša Tomić has posted comments on this change.

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

Patch Set 2: (4 inline comments)

....................................................
File vdsm/alignmentScan.py
Line 27:     cmd.extend(args)
although there's no performance difference in this case, it's 1 line less, so I fully agree

Line 48:     out_list = [(a[0], a[1], a[2], " ".join(a[3:])) for a in out_list]
Nice! I didn't know that you can limit the number of splits. Thanks!

....................................................
File vdsm/alignmentScan_test.py
Line 1: #!/usr/bin/python
I moved it there. I now hope that there won't be any objections on the performance.

Line 51:         img = tempfile.mkstemp()[1]
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

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

(5 inline comments)

....................................................
File tests/alignmentScan_test.py
Line 46:     cmd = "/bin/dd if=/dev/zero of=%s bs=4K count=1K" % imagepath
I think I wrote it before: split is evil (think imagepath with a space!).
Use a list literal instead.

....................................................
File vdsm/alignmentScan.py
Line 32:     if not os.path.exists(image_path):
please do not check path existence, this may hang on blocked nfs mounts. let virt-alignment-scan explode on missing path.

Line 37:         raise ValueError("An error scanning the disk image or guest:\n%s" % err)
why is that a ValueError? I think a specific VirtAlignError is more fitting.

Line 45:     # (allows easier iteration)
else:

missing, with another raise of VirtAlignError

Line 47:     out_list = [ScanOutput(*line.split(None, 3)) for line in out]
some of these fields makes more sense if converted to int.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Saša Tomić has posted comments on this change.

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

Patch Set 3: (5 inline comments)

....................................................
File tests/alignmentScan_test.py
Line 46:     cmd = "/bin/dd if=/dev/zero of=%s bs=4K count=1K" % imagepath
I believe that the function is completely safe, since I am calling it only from this module and only with space-free filenames. Anyway, I will modify it to be a list.

....................................................
File vdsm/alignmentScan.py
Line 32:     if not os.path.exists(image_path):
Done

Line 37:         raise ValueError("An error scanning the disk image or guest:\n%s" % err)
Done

Line 45:     # (allows easier iteration)
Done

Line 47:     out_list = [ScanOutput(*line.split(None, 3)) for line in out]
Converting only a single entry of a list to int would need several lines instead of only one (this). If you *really* need it (i.e. have a particular functionality that you would like to do), I can do it. Otherwise, it would simply make the code less readable and I would avoid it. You can also convert it lazily, at the place you want to use the integer value of the partition size.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Ewoud Kohl van Wijngaarden has posted comments on this change.

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

Patch Set 4: (7 inline comments)

....................................................
File tests/alignmentScan_test.py
Line 41: class ScanImageBadPathTestCase(unittest.TestCase):
You can merge these all ScanImage test classes in a single test class. Name it something like ScanImageTestCase and the functions test_bad_path, test_bad_aligned and test_ok_aligned.

Line 45: def mkimage(imagepath):
You could add the sfdisk exec to this function as well since you always use mkimage and sfdisk together.

Line 46:     cmd = "/bin/dd if=/dev/zero".split() + ["of=%s" % imagepath] + "bs=4K count=1K".split()
What is against:

cmd = ["/bin/dd", "if=/dev/zero", "of=%s" % imagepath, "bs=4K", "count=1K"]

Line 51:         img = tempfile.NamedTemporaryFile()
I personally prefer the with variant:

with tempfile.NamedTemporaryFile() as img:
     # rest of my code

Line 56:         cmd.append(img.name)
I prefer writing it out full. So --unit instead of -u as well as no split().

cmd = ["/sbin/sfdisk", "--unit", "S", "--force", img.name]

....................................................
File vdsm/alignmentScan.py
Line 29: def runScanArgs(args):
How about *args so you can call runScanArgs('--help') or runScanArgs('--add', image_path)

Line 51:     ScanOutput = namedtuple('ScanOutput', ['partition_name', 'partition_start_bytes', 'partition_alignment', 'alignment_scan_result'])
I think you can define this outside the function definition

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Saša Tomić has posted comments on this change.

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

Patch Set 4: (7 inline comments)

....................................................
File tests/alignmentScan_test.py
Line 41: class ScanImageBadPathTestCase(unittest.TestCase):
Done

Line 45: def mkimage(imagepath):
Done

Line 46:     cmd = "/bin/dd if=/dev/zero".split() + ["of=%s" % imagepath] + "bs=4K count=1K".split()
That's quite reasonable actually. Thanks!

Line 51:         img = tempfile.NamedTemporaryFile()
Done

Line 56:         cmd.append(img.name)
Unfortunately, --unit argument is not an option on fedora16 that I'm running.

....................................................
File vdsm/alignmentScan.py
Line 29: def runScanArgs(args):
Done

Line 51:     ScanOutput = namedtuple('ScanOutput', ['partition_name', 'partition_start_bytes', 'partition_alignment', 'alignment_scan_result'])
seems reasonable.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Ewoud Kohl van Wijngaarden has posted comments on this change.

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

Patch Set 5: (5 inline comments)

Some inline comments. All mostly trivial style points, but gerrit brings out the perfectionist in me ;)

....................................................
File tests/alignmentScan_test.py
Line 29:     def runTest(self):
I think I'd prefer RunScanArgsTestCase as class name. That instantly shows what you're testing. You might name the test test_help_response.

Line 43:     assert(r == 0)
The parenthesis are not needed here

Line 48:         r, o, e = execCmd(cmd, data = "1,,\n")
I prefer a separate data variable:

data = "128,,\n" if aligned else "1,,\n"
 r, o, e = execCmd(cmd, data = "1,,\n")
 assert r == 0

Line 52:         self.assertRaises(Exception, scanImage, ("nonexistent-image-name"))
You could test for the VirtAlignError instead of Exception

Line 65:             mkimage(img.name)
This line looks like a copy paste error

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Saša Tomić has posted comments on this change.

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

Patch Set 5: (3 inline comments)

Thanks for your comments! They are highly appreciated! :)

....................................................
File tests/alignmentScan_test.py
Line 29:     def runTest(self):
Done

Line 43:     assert(r == 0)
you're right

Line 65:             mkimage(img.name)
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 6: (1 inline comment)

....................................................
File vdsm/alignmentScan.py
Line 56:     out_list = [ScanOutput(*line.split(None, 3)) for line in out]
ahhh, I see that my comment here was lost somehow, sorry! I wonder if there's anyone reading this comment now...

I was saying that defining a clear and usable binding to the command line is the whole purpose of this module. partition_start_bytes and partition_alignment are integers for every user, and alignment_scan_result should probably converted to a boolean, or dropped altogether, since it can be deduced from partition_alignment.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Saša Tomić has posted comments on this change.

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

Patch Set 6: (1 inline comment)

....................................................
File vdsm/alignmentScan.py
Line 56:     out_list = [ScanOutput(*line.split(None, 3)) for line in out]
let's give it another shot....

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Ewoud Kohl van Wijngaarden has posted comments on this change.

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

Patch Set 8: (1 inline comment)

....................................................
File vdsm/alignmentScan.py
Line 62:         scan_result = True if line[3]== "ok" else False   # True if aligned, otherwise False
line[3] == "ok" already returns a boolean, so no need for the if else construction.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Saša Tomić has posted comments on this change.

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

Patch Set 8: (1 inline comment)

....................................................
File vdsm/alignmentScan.py
Line 62:         scan_result = True if line[3]== "ok" else False   # True if aligned, otherwise False
that makes sense.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

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

(3 inline comments)

Thanks Sasha for your other shot. I'd understand it if you prefer that some else takes this from here (though you are more than welcome to go on!). lemme know.

....................................................
File tests/alignmentScan_test.py
Line 22: import unittest
let's make use of the new nose framework.

this file has to be pep8 compliant in order to pas make check-local.

Line 28: class RunScanArgsTestCase(unittest.TestCase):
since this functionality is currently optional, I think the test should be skipped if virt-align-scan is not installed.

....................................................
File vdsm/alignmentScan.py
Line 39:     #if not os.path.exists(image_path):
no need for dead code, pep8

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić

Saša Tomić has posted comments on this change.

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

Patch Set 9: (3 inline comments)

addressed the comments

....................................................
File tests/alignmentScan_test.py
Line 22: import unittest
Done

Line 28: class RunScanArgsTestCase(unittest.TestCase):
Done

....................................................
File vdsm/alignmentScan.py
Line 39:     #if not os.path.exists(image_path):
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

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

(2 inline comments)

thanks again!

....................................................
File tests/alignmentScan_test.py
Line 44: 
these test..() functions are all good.

however, the output would be better-looking if you inherit from
testrunner.VdsmTestCase

currently I get the not-very-descriptive

runTest                                                     OK
    runTest                                                     OK
    runTest                                                     OK
    runTest                                                     OK

thanks again!

Line 80:     if not virtalignscan_installed():
raising SkipTest is cooler (here and elsewhere). Currently, we may have false positives.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

Saša Tomić has posted comments on this change.

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

Patch Set 11: (2 inline comments)

....................................................
File tests/alignmentScan_test.py
Line 44: 
I'm not sure how you run tests, so I tried to replicate the configuration of other tests. I used to get quite descriptive execution by running
PYTHONPATH=$HOME/vdsm tests/alignmentScanTests.py
in vdsm base directory.

After inheriting, I found a way to run the tests from my home directory, as:
PYTHONPATH=$HOME:$HOME/vdsm/vdsm nosetests -v vdsm/tests/alignmentScanTests.py

Line 80:     if not virtalignscan_installed():
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 12: Verified; Looks good to me, but someone else must approve

sorry Sasha, I think I've run a stale version of the tests earlier.

I'm currently happy with the code and test (though the files should be added to their Makefile.am to find their way to the tarball, and probably into the .spec file, too)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

Saša Tomić has posted comments on this change.

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

Patch Set 12:

That's great! Let's see if someone else has comments... :)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

Ewoud Kohl van Wijngaarden has posted comments on this change.

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

Patch Set 12: Looks good to me, but someone else must approve

(1 inline comment)

One trivial comment, but looks good.

....................................................
File tests/alignmentScanTests.py
Line 24: #import nose
Seems unneeded.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

Saša Tomić has posted comments on this change.

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

Patch Set 12: (1 inline comment)

....................................................
File tests/alignmentScanTests.py
Line 24: #import nose
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

Ewoud Kohl van Wijngaarden has posted comments on this change.

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

Patch Set 13: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 14: Verified

I've added the new files to Makefile.am and prettified the SkipTest text.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

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

Patch Set 14: (1 inline comment)

....................................................
Commit Message
Line 14: returns a list of all partitions of the image file.
I didn't saw this change in vdsClient. Am I missing something?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

Saša Tomić has posted comments on this change.

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

Patch Set 14: (1 inline comment)

....................................................
Commit Message
Line 14: returns a list of all partitions of the image file.
You are right, this was the original intention, and the target functionality. However, the current implementation only provides an interface to the virt-alignment-scan tool.

Should I change the commit message and re-submit the patch?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 14: (1 inline comment)

....................................................
Commit Message
Line 14: returns a list of all partitions of the image file.
someone should. An if you could, it would be cool.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

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

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

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

Patch Set 16: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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

Patch Set 16: Verified; Looks good to me, approved

we may need to mark these as slowtests, but let's get over this patch!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan

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

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

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

VDSM interface for the virt-alignment-scan tool from libguestfs

The libguestfs-tools rpm has an application called virt-alignment-scan.
http://libguestfs.org/virt-alignment-scan.1.html
Virt-alignment-scan scans the partition alignment in guest VMs and reports
if a partition in guest VM is not aligned.

This is a vdsm interface to virt-alignment-scan. It is now possible
to scan the alignment of VM guest partitions directly from vdsm.
In the future, it should be possible to expose this functionality
through vdsClient.

Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Signed-off-by: Saša Tomić 
---
M tests/Makefile.am
A tests/alignmentScanTests.py
M vdsm.spec.in
M vdsm/Makefile.am
A vdsm/alignmentScan.py
5 files changed, 155 insertions(+), 0 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved
  Igor Lvovsky: Looks good to me, but someone else must approve

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saša Tomić 
Gerrit-Reviewer: Yeela Kaplan