Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virt_net: idempotency of create/stop actions #53276

Open
wants to merge 14 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@goneri
Copy link
Contributor

goneri commented Mar 4, 2019

Currently, if we try to stop or start a network two time in a row, the
second call will fail. With this patch:

  • we don't recreate a network, if it exists
  • we only stop a network if it's active, and so we avoid an exception
    saying the network is not active
SUMMARY

The following call:

ansible localhost -m virt_net -a "uri=qemu:///system command=start name=default" -b

Was raising the following error if called two times in a row:

localhost | FAILED! => {
    "changed": false,
    "msg": "Requested operation is not valid: network is already active"
}
ISSUE TYPE

Fixes: #26486.

COMPONENT NAME
  • virt_net
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 4, 2019

virt_net: idempotency of create/stop actions
Currently, if we try to stop or start a network two time in a row, the
second call will fail. With this patch:

- we don't recreate a network, if it exists
- we only stop a network if it's active, and so we avoid an exception
  saying the network is not active

goneri added a commit to goneri/default-test-container that referenced this pull request Mar 4, 2019

add libvirt-dev in the base image
libvirt-python package depends on libvirt-dev. By adding this new
package, we can build the Python library and simplify the creation of
unit-tests for the virt_net[1], virt_pool and virt modules. In addition,
an effort is ongoing to build better libvirt modules[2], it may also benefit
from the availablity of the libvirt-python package in the base image.

[1]: ansible/ansible#53276
[2]: ansible/ansible#27905

@goneri goneri force-pushed the goneri:virt_net branch from c05a8f5 to 1b3861c Mar 4, 2019

@ansibot ansibot added core_review and removed needs_revision labels Mar 4, 2019

@goneri goneri force-pushed the goneri:virt_net branch 2 times, most recently from 3e2818d to 8fd012e Mar 4, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 4, 2019

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

test/units/modules/cloud/misc/conftest.py:8:0: useless-import-alias Import alias does not rename original package
test/units/modules/cloud/misc/conftest.py:48:4: no-self-argument Method should have "self" as first argument
test/units/modules/cloud/misc/conftest.py:52:4: no-method-argument Method has no argument

click here for bot help

test: mock libvirt
Do not depend on the libvirt package. Instead we mock the method that
we actually need

@goneri goneri force-pushed the goneri:virt_net branch from 8fd012e to 58a9a1a Mar 4, 2019

@@ -0,0 +1,5 @@
destructive
shippable/posix/group1

This comment has been minimized.

@mattclay

mattclay Mar 6, 2019

Member

Unless this test works under docker without privileged mode, remove this and use unsupported.

Tests that require privileged mode under docker are not run on Shippable.

This comment has been minimized.

@goneri

goneri Mar 6, 2019

Author Contributor

ok, thanks for the clarification.

This comment has been minimized.

@mattclay

mattclay Mar 6, 2019

Member

Sorry, I gave you bad instructions.

I forgot that this would run on our RHEL VMs, so having it in shippable/posix/group1 is appropriate.

This comment has been minimized.

@goneri

goneri Mar 7, 2019

Author Contributor

np, I will push an update. I understand I also need to remove unsupported.

Show resolved Hide resolved test/integration/targets/virt_net/aliases Outdated

@goneri goneri force-pushed the goneri:virt_net branch from f27be50 to 07c8903 Mar 6, 2019

goneri added a commit to goneri/ansible that referenced this pull request Mar 6, 2019

ci: use the unsupported alias
We use unsupported, unless the test can be run in docker without the
privileged mode.

See: ansible#53276 (comment)

goneri added a commit to goneri/ansible that referenced this pull request Mar 7, 2019

tests that require privileged mode are run in VM
So it's fine to use `shippable/posix/group1`.

See: ansible#53276 (comment)

@goneri goneri force-pushed the goneri:virt_net branch from 801c1b9 to a948af9 Mar 12, 2019

@Akasurde

This comment has been minimized.

Copy link
Member

Akasurde commented Mar 13, 2019

@mattclay Could you please re-review this ? Thanks

@goneri

This comment has been minimized.

Copy link
Contributor Author

goneri commented Mar 19, 2019

- name: Remove only the libvirt packages
package:
name: "{{ virt_net_packages|select('match', '.*libvirt.*')|list }}"
state: absent

This comment has been minimized.

@mattclay

mattclay Mar 19, 2019

Member

Does removing the libvirt packages also remove the network configuration created earlier? If not, it should also be removed as part of the test -- which would also cover more module functionality in the tests as well.

This comment has been minimized.

@goneri

goneri Mar 21, 2019

Author Contributor

The service is stopped automatically when the packages are removed. But yes, it's a good idea to validate the removal of the network with virt_net. I will do that.

- name: Remove libvirt packages
package:
name: "{{ virt_net_packages }}"
state: absent

This comment has been minimized.

@mattclay

mattclay Mar 19, 2019

Member

This remove is unnecessary with the always block below.

- name: Ensure the second call returns "unchanged"
assert:
that:
- "second_virt_net_start is not changed"

This comment has been minimized.

@mattclay

mattclay Mar 19, 2019

Member

You can reduce test output by combining the two asserts.

This comment has been minimized.

@goneri

goneri Mar 21, 2019

Author Contributor

ok

- "default.yml"

- block:
- include_tasks: 'prepare.yml'

This comment has been minimized.

@mattclay

mattclay Mar 19, 2019

Member

Why are these tasks in a separate file?

This comment has been minimized.

@goneri

goneri Mar 21, 2019

Author Contributor

It was to isolate the installation from the tests but Yeah, it's just two tasks. I merge the two files.

@@ -0,0 +1,69 @@
# -*- coding: utf-8 -*-

This comment has been minimized.

@mattclay

mattclay Mar 19, 2019

Member

This conftest will affect all tests in the same directory. The contents should probably be moved into the test_virt_net.py file instead, unless there is a chance of them being reused with other virt tests. If so, this file and the tests should be placed in a virt subdirectory.

This comment has been minimized.

@goneri

goneri Mar 21, 2019

Author Contributor

I move the files but none of the fixtures of the conftest.py have autouse=True. So they won't affect the behavior of the other tests.

goneri added some commits Mar 5, 2019

add integration tests for virt_net
The current test validate the idempotency of `define` and `create` actions.
test: enable virt_net test on RedHat 7 and 8
Define the dependency lists for RedHat 7 and RedHat 8.
ci: use the unsupported alias
We use unsupported, unless the test can be run in docker without the
privileged mode.

See: #53276 (comment)
tests that require privileged mode are run in VM
So it's fine to use `shippable/posix/group1`.

See: #53276 (comment)
virt_net/create raise unexpected libvirt exception
Continue to raise an exception during the `create()` if it's not
a `VIR_ERR_NETWORK_EXIST`.
virt_net: do not call create() on "active" network
If we try to create() an active network on RHEL7/RHEL8, we get the
following exception:
  code: VIR_ERR_OPERATION_INVALID.
  msg: Requested operation is not valid: network is already active

We now ensure first that the network is NOT active.
virt_net func test: only clean up the libvirt packages
To avoid any side-effect with some other tests, we only remove the libvirt
packages.
test: virt_net: don't use assert_called()
`assert_called()` is python36+, we use `assert_called_with()` instead.
virt_net: add the destructive alias
Use the destructive alias since this test installs and uninstalls packages.
move the test in virt_net dir
This to isolate the scope of the conftest.py.
test/virt_net: clean up the network at the end
Remove the network at the end of the installation and validate the
idempotency.

@goneri goneri force-pushed the goneri:virt_net branch from 05b6d0b to 3f742d8 Mar 21, 2019

Dismissing stale review.

@ansibot ansibot added core_review and removed needs_revision labels Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.