virt_net: idempotency of create/stop actions (#53276)

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

* test: mock libvirt

* add integration tests for virt_net

* test: enable virt_net test on RedHat 7 and 8

* ci: use the unsupported alias

* tests that require privileged mode are run in VM

* virt_net/create raise unexpected libvirt exception

* import mock from units.compat

* virt_net: do not call create() on "active" network

* virt_net func test: only clean up the libvirt packages

* test: virt_net: don't use assert_called()

* virt_net: add the destructive alias

* move the test in virt_net dir

* test/virt_net: clean up the network at the end

lib/ansible/modules/cloud/misc/virt_net.py

@@ -201,28 +201,16 @@ class LibvirtConnection(object):
         self.conn = conn
 
     def find_entry(self, entryid):
-        # entryid = -1 returns a list of everything
+        if entryid == -1:  # Get active entries
-
+            names = self.conn.listNetworks() + self.conn.listDefinedNetworks()
-        results = []
+            return [self.conn.networkLookupByName(n) for n in names]
-
-        # Get active entries
-        for name in self.conn.listNetworks():
-            entry = self.conn.networkLookupByName(name)
-            results.append(entry)
-
-        # Get inactive entries
-        for name in self.conn.listDefinedNetworks():
-            entry = self.conn.networkLookupByName(name)
-            results.append(entry)
-
-        if entryid == -1:
-            return results
-
-        for entry in results:
-            if entry.name() == entryid:
-                return entry
 
+        try:
+            return self.conn.networkLookupByName(entryid)
+        except libvirt.libvirtError as e:
+            if e.get_error_code() == libvirt.VIR_ERR_NO_NETWORK:
                 raise EntryNotFound("network %s not found" % entryid)
+            raise
 
     def create(self, entryid):
         if not self.module.check_mode:
@@ -285,11 +273,18 @@ class LibvirtConnection(object):
                 return self.module.exit_json(changed=True)
 
     def undefine(self, entryid):
-        if not self.module.check_mode:
+        entry = None
+        try:
+            entry = self.find_entry(entryid)
+            found = True
+        except EntryNotFound:
+            found = False
+
+        if found:
             return self.find_entry(entryid).undefine()
-        else:
+
-            if not self.find_entry(entryid):
+        if self.module.check_mode:
-                return self.module.exit_json(changed=True)
+            return self.module.exit_json(changed=found)
 
     def get_status2(self, entry):
         state = entry.isActive()
@@ -418,19 +413,27 @@ class VirtNetwork(object):
         return self.conn.set_autostart(entryid, state)
 
     def create(self, entryid):
+        if self.conn.get_status(entryid) == "active":
+            return
+        try:
             return self.conn.create(entryid)
+        except libvirt.libvirtError as e:
+            if e.get_error_code() == libvirt.VIR_ERR_NETWORK_EXIST:
+                return None
+            raise
 
     def modify(self, entryid, xml):
         return self.conn.modify(entryid, xml)
 
     def start(self, entryid):
-        return self.conn.create(entryid)
+        return self.create(entryid)
 
     def stop(self, entryid):
+        if self.conn.get_status(entryid) == "active":
             return self.conn.destroy(entryid)
 
     def destroy(self, entryid):
-        return self.conn.destroy(entryid)
+        return self.stop(entryid)
 
     def undefine(self, entryid):
         return self.conn.undefine(entryid)

test/integration/targets/virt_net/aliases

@@ -0,0 +1,5 @@
+shippable/posix/group1
+skip/freebsd
+skip/osx
+needs/privileged
+destructive

test/integration/targets/virt_net/files/foobar.xml

@@ -0,0 +1,9 @@
+<network>
+    <name>foobar</name>
+    <forward mode='nat'/>
+    <ip address='192.168.125.1' netmask='255.255.255.0'>
+        <dhcp>
+            <range start='192.168.125.2' end='192.168.125.254'/>
+        </dhcp>
+    </ip>
+</network>

test/integration/targets/virt_net/tasks/main.yml

@@ -0,0 +1,78 @@
+---
+- include_vars: '{{ item }}'
+  with_first_found:
+    - "{{ ansible_distribution }}-{{ ansible_distribution_version}}.yml"
+    - "{{ ansible_distribution }}-{{ ansible_distribution_major_version}}.yml"
+    - "{{ ansible_distribution }}.yml"
+    - "default.yml"
+
+- block:
+    - name: Install libvirt packages
+      package:
+        name: "{{ virt_net_packages }}"
+
+    - name: Start libvirt
+      service:
+        name: libvirtd
+        state: started
+
+    - name: Define the foobar network
+      virt_net:
+        command: define
+        name: foobar
+        xml: '{{ lookup("file", "foobar.xml") }}'
+    
+    - name: Define the foobar network (again)
+      virt_net:
+        command: define
+        name: foobar
+        xml: '{{ lookup("file", "foobar.xml") }}'
+      register: second_virt_net_define
+    
+    - name: Start the default network
+      virt_net:
+        uri: qemu:///system
+        command: start
+        name: foobar
+    
+    - name: Start the default network (again)
+      virt_net:
+        uri: qemu:///system
+        command: start
+        name: foobar
+      register: second_virt_net_start
+
+    - name: Destroy the foobar network
+      virt_net:
+        command: destroy
+        name: foobar
+
+    - name: Undefine the foobar network
+      virt_net:
+        command: undefine
+        name: foobar
+      register: second_virt_net_define
+
+    - name: Undefine the foobar network (again)
+      virt_net:
+        command: undefine
+        name: foobar
+      register: second_virt_net_undefine
+
+    - name: Ensure the second calls return "unchanged"
+      assert:
+        that:
+          - "second_virt_net_start is not changed"
+          - "second_virt_net_define is not changed"
+          - "second_virt_net_undefine is not changed"
+    
+  always:
+    - name: Stop libvirt
+      service:
+        name: libvirtd
+        state: stopped
+
+    - name: Remove only the libvirt packages
+      package:
+        name: "{{ virt_net_packages|select('match', '.*libvirt.*')|list }}"
+        state: absent

test/integration/targets/virt_net/vars/Debian.yml

@@ -0,0 +1,6 @@
+---
+virt_net_packages:
+  - libvirt-daemon
+  - libvirt-daemon-system
+  - python-libvirt
+  - python-lxml

test/integration/targets/virt_net/vars/Fedora-29.yml

@@ -0,0 +1,6 @@
+---
+virt_net_packages:
+  - libvirt
+  - libvirt-daemon
+  - python3-libvirt
+  - python3-lxml

test/integration/targets/virt_net/vars/RedHat-7.yml

@@ -0,0 +1,6 @@
+---
+virt_net_packages:
+  - libvirt
+  - libvirt-daemon
+  - libvirt-python
+  - python-lxml

test/integration/targets/virt_net/vars/RedHat-8.yml

@@ -0,0 +1,6 @@
+---
+virt_net_packages:
+  - libvirt
+  - libvirt-daemon
+  - python3-libvirt
+  - python3-lxml

test/integration/targets/virt_net/vars/Ubuntu-16.04.yml

@@ -0,0 +1,5 @@
+---
+virt_net_packages:
+  - libvirt-daemon
+  - python-libvirt
+  - python-lxml

test/integration/targets/virt_net/vars/Ubuntu-18.04.yml

@@ -0,0 +1,5 @@
+---
+virt_net_packages:
+  - libvirt-daemon
+  - python-libvirt
+  - python-lxml

test/integration/targets/virt_net/vars/Ubuntu-18.10.yml

@@ -0,0 +1,6 @@
+---
+virt_net_packages:
+  - libvirt-daemon
+  - libvirt-daemon-system
+  - python-libvirt
+  - python-lxml

test/integration/targets/virt_net/vars/default.yml

@@ -0,0 +1,5 @@
+---
+virt_net_packages:
+  - libvirt-daemon
+  - python-libvirt
+  - python-lxml

test/units/modules/cloud/misc/virt_net/conftest.py

@@ -0,0 +1,69 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright: (c) 2019, Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+import pytest
+
+from ansible.modules.cloud.misc import virt_net
+
+from units.compat import mock
+
+
+virt_net.libvirt = None
+virt_net.HAS_VIRT = True
+
+
+class DummyNetwork():
+    def __init__(self, name, isActive=True):
+        self._name = name
+        self._isActive = isActive
+
+    def name(self):
+        return self._name
+
+    def isActive(self):
+        return self._isActive
+
+
+class DummyLibvirtConn():
+    def __init__(self):
+        self._network = [
+            DummyNetwork("inactive_net", isActive=False),
+            DummyNetwork("active_net", isActive=True)]
+
+    def listNetworks(self):
+        return [i.name() for i in self._network]
+
+    def networkLookupByName(self, name):
+        for i in self._network:
+            if i.name() == name:
+                return i
+
+    def listDefinedNetworks(self):
+        return []
+
+
+class DummyLibvirt():
+    VIR_ERR_NETWORK_EXIST = 'VIR_ERR_NETWORK_EXIST'
+
+    @classmethod
+    def open(cls, uri):
+        return DummyLibvirtConn()
+
+    class libvirtError(Exception):
+        def __init__(self):
+            self.error_code
+
+        def get_error_code(self):
+            return self.error_code
+
+
+@pytest.fixture
+def dummy_libvirt(monkeypatch):
+    monkeypatch.setattr(virt_net, 'libvirt', DummyLibvirt)
+    return DummyLibvirt
+
+
+@pytest.fixture
+def virt_net_obj(dummy_libvirt):
+    return virt_net.VirtNetwork('qemu:///nowhere', mock.MagicMock())

test/units/modules/cloud/misc/virt_net/test_virt_net.py

@@ -0,0 +1,30 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright: (c) 2019, Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+from units.compat import mock
+
+
+def test_virt_net_create_already_active(virt_net_obj, dummy_libvirt):
+    virt_net_obj.conn.create = mock.Mock()
+    assert virt_net_obj.create("active_net") is None
+    virt_net_obj.conn.create.assert_not_called()
+
+
+def test_virt_net_recreate(virt_net_obj, dummy_libvirt):
+    virt_net_obj.conn.create = mock.Mock()
+    dummy_libvirt.libvirtError.error_code = 'VIR_ERR_NETWORK_EXIST'
+    virt_net_obj.conn.create.side_effect = dummy_libvirt.libvirtError
+    assert virt_net_obj.create("active_net") is None
+
+
+def test_virt_stop_ignore_inactive(virt_net_obj):
+    virt_net_obj.conn.destroy = mock.Mock()
+    virt_net_obj.stop('inactive_net')
+    virt_net_obj.conn.destroy.assert_not_called()
+
+
+def test_virt_stop_active(virt_net_obj, monkeypatch):
+    virt_net_obj.conn.destroy = mock.Mock()
+    virt_net_obj.stop('active_net')
+    virt_net_obj.conn.destroy.assert_called_with('active_net')