Merge lp://staging/~soren/nova/virt-test-improvements into lp://staging/~hudson-openstack/nova/trunk

Whoa, hang on. Found a bug.

> Whoa, hang on. Found a bug.

...and fixed it, by the way. Reviews welcome.

Nice work here Soren.

Some notes:

Received this error when running the tests: http://paste.openstack.org/show/2357/

> 911 + if not auth == [[VIR_CRED_AUTHNAME, VIR_CRED_NOECHOPROMPT],
> 912 + 'root',
> 913 + None]:

Might be better as:

    if auth != [[VIR_CRED_AUTHNAME, VIR_CRED_NOECHOPROMPT],...

> 914 + raise Exception("Please extend fake libvirt module to support this "
> 915 + "auth method")

i18n.

> 1420 + try:
> 1421 + dst_fd, dst_path = tempfile.mkstemp()
> 1422 + os.close(dst_fd)
> 1423 +
> 1424 + src_fd, src_path = tempfile.mkstemp()
> 1425 + with os.fdopen(src_fd) as fp:
> 1426 + fp.write('canary')
> 1427 +
> 1428 + libvirt_utils.copy_image(src_path, dst_path)
> 1429 + with open(dst_path, 'r') as fp:
> 1430 + self.assertEquals(fp.read(), 'canary')
> 1431 + finally:
> 1432 + os.unlink(src_path)
> 1433 + os.unlink(dst_path)

Not likely to happen, but if an exception was is raised after the first
`mkstemp()` but before the second `mkstemp()`, the `finally` will attempt to
unlink `src_path` which hasn't been defined yet.

Also, in that first `mkstemp`, if that were to raise an exception, then
`dest_path` wouldn't have been bound as a local variable (since the RHS raised
before assigning to the LHS). To handle that, the `mkstemp` should go outside of the `try`.

This should solve both problems:

    dst_fd, dst_path = tempfile.mkstemp()
    try:
        os.close(dst_fd)

        src_fd, src_path = tempfile.mkstemp()
        try:
            with os.fdopen(src_fd) as fp:
                fp.write('canary')

            libvirt_utils.copy_image(src_path, dst_path)
            with open(dst_path, 'r') as fp:
                self.assertEquals(fp.read(), 'canary')
        finally:
            os.unlink(src_path)
    finally:
        os.unlink(dst_path)

> 1454 + try:
> 1455 + dst_fd, dst_path = tempfile.mkstemp()

Same deal here:

    dst_fd, dst_path = tempfile.mkstemp()
    try:
        ...

Few more places like this in the file.

> 1990 +# Copyright 2010 United States Government as represented by the
> 1991 +# Administrator of the National Aeronautics and Space Administration.
> 1992 +# All Rights Reserved.
> 1993 +# Copyright (c) 2010 Citrix Systems, Inc.
> 1994 +# Copyright (c) 2011 Piston Cloud Computing, Inc
> 1995 +# Copyright (c) 2011 OpenStack LLC

Given that this is a new file, should we just start it out with:

# Copyright (c) 2011 OpenStack LLC

No real opinion here, just tossing out the question.

> 2105 + if umask:
> 2106 + saved_umask = os.umask(umask)
> 2107 +
> 2108 + with open(path, 'w') as f:
> 2109 + f.write(contents)
> 2110 +
> 2111 + if umask:
> 2112 + os.umask(saved_umask)

Should this be inside a try/finally?

    if umask:
        saved_umask = os.umask(umask)
    try:
        with open(path, 'w') as f:
            f.write(contents)
    finally:
        if umask:
            os.umask(saved_umask)

> Nice work here Soren.
>
> Some notes:
>
>
> Received this error when running the tests:
> http://paste.openstack.org/show/2357/

Which OS are you running this on? Can you give me the outout of "uname -a" and see if its df(1) command can somehow be pursuaded to provide its output in bytes?

>> + raise Exception("Please extend fake libvirt module to support this "
>> + "auth method")
> i18n.

I added i18n here, but generally, I don't really think doing so in general for strings that only exist in the tests is super useful. Kind of like how we don't i18n enable our code comments.

> > 1420 + try:
> > 1421 + dst_fd, dst_path = tempfile.mkstemp()
> > 1422 + os.close(dst_fd)
> > 1423 +
> > 1424 + src_fd, src_path = tempfile.mkstemp()
> > 1425 + with os.fdopen(src_fd) as fp:
> > 1426 + fp.write('canary')
> > 1427 +
> > 1428 + libvirt_utils.copy_image(src_path, dst_path)
> > 1429 + with open(dst_path, 'r') as fp:
> > 1430 + self.assertEquals(fp.read(), 'canary')
> > 1431 + finally:
> > 1432 + os.unlink(src_path)
> > 1433 + os.unlink(dst_path)
> Not likely to happen, but if an exception was is raised after the first
> `mkstemp()` but before the second `mkstemp()`, the `finally` will attempt to
> unlink `src_path` which hasn't been defined yet.

Fixed.
> > 1990 +# Copyright 2010 United States Government as represented by the
> > 1991 +# Administrator of the National Aeronautics and Space
> Administration.
> > 1992 +# All Rights Reserved.
> > 1993 +# Copyright (c) 2010 Citrix Systems, Inc.
> > 1994 +# Copyright (c) 2011 Piston Cloud Computing, Inc
> > 1995 +# Copyright (c) 2011 OpenStack LLC
>
> Given that this is a new file, should we just start it out with:

Parts of its contents are based on an existing file, so the copyright carries over. This was intentional.

> > 2105 + if umask:
> > 2106 + saved_umask = os.umask(umask)
> > 2107 +
> > 2108 + with open(path, 'w') as f:
> > 2109 + f.write(contents)
> > 2110 +
> > 2111 + if umask:
> > 2112 + os.umask(saved_umask)
> Should this be inside a try/finally?

Good call. Fixed.

Thanks!

Overall I don't have a lot of code-specific comments. I will say that this does bring some much needed testing to the libvirt layer which historically has lacked some of the bigger-picture tests like the ones you've added.

However, I can't help but think about how overly complex this change is:

347 === added file 'nova/tests/fakelibvirt.py'
1112 === added file 'nova/tests/test_fakelibvirt.py'

I have always been told that if you're spending time writing tests for your tests then take a step back and think about why. In this case we're creating a 100% fake libvirt module which provides all of the functionality (we need/use) for our tests.

If a test fails, we have absolutely no way of knowing if it's an error in our driver or in our test code. Now we're debugging our test's tests which seems to be to be unneeded. Which version of the libvirt API does the fake libvirt module follow? I say leave the libvirt code up to the libvirt maintainers and the nova code, which interfaces with libvirt, to us.

We could use the libvirt 'test' driver, which provides basically what you said:

> to expose an API and behaviour identical to that of libvirt itself
> except without actually running any VM's or setting up any firewall or
> anything, but still responding correctly when asked for a domain's XML,
> a list of defined domains, running domains, etc.

However, one thing I can see is that the 'test' drive doesn't support a lot of nwfilter things that we run in our tests.

Example of 'test' driver, for anyone who hasn't used it: http://paste.openstack.org/show/2465/

So I guess long story short I don't want to hold this merge prop up if we think it's going to increase libvirt reliability, however for the future in my opinion we should shoot for:

Small Tests: Simple stubbing out of libvirt methods we call, or use of Mox to make sure we're
             passing expected arguments to libvirt. These tests are just meant to test OUR logic and
             not the interaction of our code with libvirt's code.

Medium Tests: Use the 'test' driver to test as much as it allows. Sometimes we might have to
              expect failures here when things like nwfilters are not supported by the 'test'
              driver. These tests are meant to test the interaction of our code with libvirt,
              to make sure that the functions we're calling are the correct ones and that the
              parameters passed make relative sense.

Large Tests: Have a libvirt process running on Jenkins or somewhere else that runs a full test
             suite of QEMU, LXC, and any other driver we support. These tests make sure that
             overall things are working as we intend them to work.

http://wiki.openstack.org/TestGuide

Also, perhaps this was discussed elsewhere and I missed up, but feel free to point me to doc/discussions on this topic.

Two quick code comments:

43 === added file 'nova/image/service.py'

I think this is a remnant of the past, it was moved in trunk a bit ago. I think it might have been left after a trunk merge.

1841 + try:
1842 + virt_dom = self._lookup_by_name(instance['name'])
1843 + except exception.InstanceNotFound:
1844 + raise exception.InstanceNotRunning()

Why change the exception type here? Seems like an InstanceNotFound exception makes more sense to me.

Moved to Gerrit:

    https://review.openstack.org/#change,649