OpenStack Compute (nova)

Merge lp://qastaging/~blamar/nova/libvirt-cleanup-branch into lp://qastaging/~hudson-openstack/nova/trunk

  1. libvirt-cleanup-branch
  2. Merge into trunk
Proposed by Brian Lamar
Status: Work in progress
Proposed branch: lp://qastaging/~blamar/nova/libvirt-cleanup-branch
Merge into: lp://qastaging/~hudson-openstack/nova/trunk
Diff against target: 433 lines (+123/-139)
6 files modified
nova/tests/test_libvirt.py (+8/-52)
nova/utils.py (+15/-0)
nova/virt/libvirt/__init__.py (+85/-0)
nova/virt/libvirt/connection.py (+13/-82)
nova/virt/libvirt/firewall.py (+1/-5)
tools/pip-requires (+1/-0)
To merge this branch: bzr merge lp://qastaging/~blamar/nova/libvirt-cleanup-branch
Reviewer Review Type Date Requested Status
Monty Taylor (community) Needs Fixing
Devin Carlen (community) Approve
Ed Leafe (community) Approve
William Wolf (community) Approve
Alex Meade (community) Approve
Review via email: mp+66064@code.qastaging.launchpad.net

Commit message

Moved libvirt flag defines to libvirt/__init__.py, and modified lazy loading of libvirt/libxml2/Cheetah to use nova.utils.optional_import.

Description of the change

Two small cleanups in libvirt:

  1) Moved flag defines to __init__.py
  2) Replaced lazy-loading of imports with utils.optional_import (new)

  One unrelated change is the addition of unittest2 to pip-requires. I'm
  using the unittest.skipIf method in test_libvirt.py and unittest2 has
  backported all Python 2.7 unittest improvements.

To post a comment you must log in.
Revision history for this message
Alex Meade (alex-meade) wrote :

I like it

review: Approve
Reply
Revision history for this message
Brian Waldon (bcwaldon) wrote :

Good refactoring, Brian. one minor thing:

164: Can you rephrase this log line to not repeat the word 'import'? And should it be info or debug?

Reply
Revision history for this message
Ed Leafe (ed-leafe) wrote :

358 + """Retrieve a libvirt connection object.
359 +
360 + :param read_only: If True, the libvirt connection will be read only.
361 + :returns: `nova.virt.libvirt.connection:LibvirtConnection`
362 +
363 + """

Can this be reduced to a single line docstring? The param name makes its use obvious, and the return value type is what the docstring already stated.

Also, is there a reason not to change all uses of unittest to unittest2? Or is that planned as part of another merge?

review: Needs Information
Reply
lp://qastaging/~blamar/nova/libvirt-cleanup-branch updated
1219. By Brian Lamar

review feedback from bcwaldon and ed leafe

Revision history for this message
Brian Lamar (blamar) wrote :

Shortened comment to single-line (you're very right, it was unnecessarily redundant!) and re-worded the log line for bcwaldon.

Ed, there is not a blueprint I'm aware of, so I made one. Great suggestion. I'm not going to actively pursue it now, but it will be something to point to when I do get the time or someone else feels like taking it on.

https://blueprints.launchpad.net/nova/+spec/nova-unittest2

Reply
Revision history for this message
William Wolf (throughnothing) wrote :

This looks good

review: Approve
Reply
Revision history for this message
Ed Leafe (ed-leafe) wrote :

ok, lgtm.

review: Approve
Reply
Revision history for this message
Devin Carlen (devcamcar) wrote :

lgtm

review: Approve
Reply
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :
Download full text (71.0 KiB)

The attempt to merge lp:~blamar/nova/libvirt-cleanup-branch into lp:nova failed. Below is the output from the failed tests.

FloatingIpTest
    test_floating_ip_allocate OK 0.14
    test_floating_ip_associate OK 0.21
    test_floating_ip_disassociate OK 0.09
    test_floating_ip_release OK 0.09
    test_floating_ip_show OK 0.09
    test_floating_ips_list OK 0.09
    test_translate_floating_ip_view OK 0.04
FlavorsExtraSpecsTest
    test_create OK 0.04
    test_create_empty_body OK 0.04
    test_delete OK 0.04
    test_index OK 0.05
    test_index_no_data OK 0.04
    test_show OK 0.05
    test_show_spec_not_found OK 0.25
    test_update_item OK 0.05
    test_update_item_body_uri_mismatch OK 0.04
    test_update_item_empty_body OK 0.04
    test_update_item_too_many_keys OK 0.04
AccountsTest
    test_account_create OK 0.17
    test_account_delete OK 0.17
    test_account_update OK 0.18
    test_get_account OK 0.40
AdminAPITest
    test_admin_disabled OK 0.13
    test_admin_enabled OK 0.16
APITest
    test_exceptions_are_converted_to_faults OK 0.01
    test_malformed_json OK 0.05
    test_malformed_xml OK 0.06
Test
    test_authorize_project OK 0.09
    test_authorize_token OK 0.34
    test_authorize_user OK 0.05
    test_bad_project OK 0.09
    test_bad_token OK 0.05
    test_bad_user_bad_key OK 0.05
    test_bad_user_good_key OK 0.05
    test_no_user OK 0.06
    test_not_existing_project OK 0.08
    test_token_expiry OK 0.27
TestFunctional
    test_token_doesnotexist OK 0.06
    test_token_expiry OK 0.08
TestLimiter
    test_authorize_token OK 0.10
LimiterTest
    test_limiter_custom_max_limit ...

Reply
lp://qastaging/~blamar/nova/libvirt-cleanup-branch updated
1220. By Brian Lamar

Merged trunk.

1221. By Brian Lamar

Merged trunk

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :
Download full text (170.5 KiB)

The attempt to merge lp:~blamar/nova/libvirt-cleanup-branch into lp:nova failed. Below is the output from the failed tests.

FloatingIpTest
    test_floating_ip_allocate OK 0.29
    test_floating_ip_associate OK 0.10
    test_floating_ip_disassociate OK 0.09
    test_floating_ip_release OK 0.10
    test_floating_ip_show OK 0.10
    test_floating_ips_list OK 0.10
    test_translate_floating_ip_view OK 0.04
FixedIpTest
    test_add_fixed_ip OK 0.08
    test_add_fixed_ip_no_network OK 0.08
    test_remove_fixed_ip OK 0.08
    test_remove_fixed_ip_no_address OK 0.27
FlavorsExtraSpecsTest
    test_create OK 0.04
    test_create_empty_body OK 0.05
    test_delete OK 0.05
    test_index OK 0.05
    test_index_no_data OK 0.05
    test_show OK 0.05
    test_show_spec_not_found OK 0.04
    test_update_item OK 0.05
    test_update_item_body_uri_mismatch OK 0.05
    test_update_item_empty_body OK 0.05
    test_update_item_too_many_keys OK 0.06
AccountsTest
    test_account_create OK 0.36
    test_account_delete OK 0.17
    test_account_update OK 0.18
    test_get_account OK 0.18
AdminAPITest
    test_admin_disabled OK 0.15
    test_admin_enabled OK 0.42
APITest
    test_exceptions_are_converted_to_faults OK 0.03
    test_malformed_json OK 0.05
    test_malformed_xml OK 0.08
Test
    test_authorize_project OK 0.10
    test_authorize_token OK 0.10
    test_authorize_user OK 0.06
    test_bad_project OK 0.34
    test_bad_token OK 0.06
    test_bad_user_bad_key OK 0.06
    test_bad_user_good_key OK 0.06
    test_no_user OK 0.05
    test_not_existing_project OK 0.10
    test_token_expiry OK ...

Reply
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :
Download full text (182.4 KiB)

The attempt to merge lp:~blamar/nova/libvirt-cleanup-branch into lp:nova failed. Below is the output from the failed tests.

FloatingIpTest
    test_floating_ip_allocate OK 0.29
    test_floating_ip_associate OK 0.10
    test_floating_ip_disassociate OK 0.10
    test_floating_ip_release OK 0.10
    test_floating_ip_show OK 0.10
    test_floating_ips_list OK 0.09
    test_translate_floating_ip_view OK 0.04
FixedIpTest
    test_add_fixed_ip OK 0.08
    test_add_fixed_ip_no_network OK 0.08
    test_remove_fixed_ip OK 0.28
    test_remove_fixed_ip_no_address OK 0.07
FlavorsExtraSpecsTest
    test_create OK 0.05
    test_create_empty_body OK 0.05
    test_delete OK 0.05
    test_index OK 0.05
    test_index_no_data OK 0.05
    test_show OK 0.05
    test_show_spec_not_found OK 0.05
    test_update_item OK 0.05
    test_update_item_body_uri_mismatch OK 0.05
    test_update_item_empty_body OK 0.05
    test_update_item_too_many_keys OK 0.05
AccountsTest
    test_account_create OK 0.39
    test_account_delete OK 0.17
    test_account_update OK 0.18
    test_get_account OK 0.18
AdminAPITest
    test_admin_disabled OK 0.14
    test_admin_enabled OK 0.47
APITest
    test_exceptions_are_converted_to_faults OK 0.01
    test_malformed_json OK 0.06
    test_malformed_xml OK 0.06
Test
    test_authorize_project OK 0.10
    test_authorize_token OK 0.10
    test_authorize_user OK 0.06
    test_bad_project OK 0.35
    test_bad_token OK 0.06
    test_bad_user_bad_key OK 0.06
    test_bad_user_good_key OK 0.06
    test_no_user OK 0.06
    test_not_existing_project OK 0.10
    test_token_expiry OK ...

Reply
Revision history for this message
Monty Taylor (mordred) wrote :

skipIf seems like an excellent way to describe conditional skipping... except that now it's throwing a SkipTest error.

The other tests which skip things use skip_test ... could you look in to fixing things so that skipIf does not throw a SkipTest error (or that the test harness understands that we skipped that on purpose)

OR

If unittest2 was added to get skipIf and skipIf is causing these problems - perhaps just use skip_test and remove the unittest2 need? (I mean, unless there is some grand scheme for migrating to unittest2 underway)

review: Needs Fixing
Reply

Unmerged revisions

1221. By Brian Lamar

Merged trunk

1220. By Brian Lamar

Merged trunk.

1219. By Brian Lamar

review feedback from bcwaldon and ed leafe

1218. By Brian Lamar

Two small cleanups in libvirt:

1) Moved flag defines to __init__.py
2) Replaced lazy-loading of imports with utils.optional_import (new)

One unrelated change is the addition of unittest2 to pip-requires. I'm
using the unittest.skipIf method in test_libvirt.py and unittest2 has
backported all Python 2.7 unittest improvements.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.