Refactor openvswitch_bridge (#24014)

Use common patterns from other network modules for better
maintainability.

lib/ansible/modules/network/ovs/openvswitch_bridge.py

@@ -52,7 +52,8 @@ options:
         required: false
         default: None
         description:
-            - The VLAN id of the fake bridge to manage (must be between 0 and 4095)
+            - The VLAN id of the fake bridge to manage (must be between 0 and
+              4095). This parameter is required if I(parent) parameter is set.
     state:
         required: false
         default: "present"
@@ -83,7 +84,9 @@ options:
         required: false
         default: None
         description:
-            - Set a single property on a bridge.
+            - Run set command after bridge configuration. This parameter is
+              non-idempotent, play will always return I(changed) state if
+              present
 '''
 
 EXAMPLES = '''
@@ -109,251 +112,133 @@ EXAMPLES = '''
       bridge-id: br-int
 '''
 
-def truncate_before(value, srch):
+from ansible.module_utils.basic import AnsibleModule
-    """ Return content of str before the srch parameters. """
+from ansible.module_utils.six import iteritems
+from ansible.module_utils.pycompat24 import get_exception
 
-    before_index = value.find(srch)
+def _external_ids_to_dict(text):
-    if (before_index >= 0):
+    d = {}
-        return value[:before_index]
+
+    for l in text.splitlines():
+        if l:
+            k, v = l.split('=')
+            d[k] = v
+
+    return d
+
+def map_obj_to_commands(want, have, module):
+    commands = list()
+
+    if module.params['state'] == 'absent':
+        if have:
+            templatized_command = ("%(ovs-vsctl)s -t %(timeout)s del-br"
+                                   " %(bridge)s")
+            command = templatized_command % module.params
+            commands.append(command)
     else:
-        return value
+        if have:
+            if want['fail_mode'] != have['fail_mode']:
+                templatized_command = ("%(ovs-vsctl)s -t %(timeout)s"
+                                       " set-fail-mode %(bridge)s"
+                                       " %(fail_mode)s")
+                command = templatized_command % module.params
+                commands.append(command)
 
-def _set_to_get(set_cmd, module):
+            if want['external_ids'] != have['external_ids']:
-    """ Convert set command to get command and set value.
+                templatized_command = ("%(ovs-vsctl)s -t %(timeout)s"
-    return tuple (get command, set value)
+                                       " br-set-external-id %(bridge)s")
-    """
+                command = templatized_command % module.params
-
+                for k, v in iteritems(want['external_ids']):
-    ##
+                    if (k not in have['external_ids']
-    # If set has option: then we want to truncate just before that.
+                            or want['external_ids'][k] != have['external_ids'][k]):
-    set_cmd = truncate_before(set_cmd, " option:")
+                        command += " " + k + " " + v
-    get_cmd = set_cmd.split(" ")
+                        commands.append(command)
-    (key, value) = get_cmd[-1].split("=")
-    module.log("get commands %s " % key)
-    return (["--", "get"] + get_cmd[:-1] + [key], value)
-
-
-class OVSBridge(object):
-    """ Interface to ovs-vsctl. """
-    def __init__(self, module):
-        self.module = module
-        self.bridge = module.params['bridge']
-        self.parent = module.params['parent']
-        self.vlan = module.params['vlan']
-        self.state = module.params['state']
-        self.timeout = module.params['timeout']
-        self.fail_mode = module.params['fail_mode']
-        self.set_opt = module.params.get('set', None)
-
-        if self.parent:
-            if self.vlan is None:
-                self.module.fail_json(msg='VLAN id must be set when parent is defined')
-            elif self.vlan < 0 or self.vlan > 4095:
-                self.module.fail_json(msg='Invalid VLAN ID (must be between 0 and 4095)')
-
-    def _vsctl(self, command):
-        '''Run ovs-vsctl command'''
-        return self.module.run_command(['ovs-vsctl', '-t',
-                                        str(self.timeout)] + command)
-
-    def exists(self):
-        '''Check if the bridge already exists'''
-        rtc, _, err = self._vsctl(['br-exists', self.bridge])
-        if rtc == 0:  # See ovs-vsctl(8) for status codes
-            return True
-        if rtc == 2:
-            return False
-        self.module.fail_json(msg=err)
-
-    def set(self, set_opt):
-        """ Set attributes on a bridge. """
-        self.module.log("set called %s" % set_opt)
-        if (not set_opt):
-            return False
-
-        (get_cmd, set_value) = _set_to_get(set_opt, self.module)
-        (rtc, out, err) = self._vsctl(get_cmd, False)
-        if rtc != 0:
-            ##
-            # ovs-vsctl -t 5 -- get Interface port external_ids:key
-            # returns failure if key does not exist.
-            out = None
         else:
-            out = out.strip("\n")
+            templatized_command = ("%(ovs-vsctl)s -t %(timeout)s add-br"
-            out = out.strip('"')
+                                   " %(bridge)s")
+            command = templatized_command % module.params
 
-        if (out == set_value):
+            if want['parent']:
-            return False
+                templatized_command =  "%(parent)s %(vlan)s"
+                command += " " + templatized_command % module.params
 
-        (rtc, out, err) = self._vsctl(["--", "set"] + set_opt.split(" "))
+            if want['set']:
-        if rtc != 0:
+                templatized_command = " -- set %(set)s"
-            self.module.fail_json(msg=err)
+                command += templatized_command % module.params
 
-        return True
+            commands.append(command)
 
-    def add(self):
+            if want['fail_mode']:
-        '''Create the bridge'''
+                templatized_command = ("%(ovs-vsctl)s -t %(timeout)s"
-        cmd = ['add-br', self.bridge]
+                                       " set-fail-mode %(bridge)s"
-        if self.parent and self.vlan: # Add fake bridge
+                                       " %(fail_mode)s")
-            cmd += [self.parent, str(self.vlan)]
+                command = templatized_command % module.params
+                commands.append(command)
 
-        if self.set and self.set_opt:
+            if want['external_ids']:
-            cmd += ["--", "set"]
+                for k, v in iteritems(want['external_ids']):
-            cmd += self.set_opt.split(" ")
+                    templatized_command = ("%(ovs-vsctl)s -t %(timeout)s"
+                                        " br-set-external-id %(bridge)s")
+                    command = templatized_command % module.params
+                    command += " " + k + " " + v
+                    commands.append(command)
+    return commands
 
-        rtc, _, err = self._vsctl(cmd)
-        if rtc != 0:
-            self.module.fail_json(msg=err)
-        if self.fail_mode:
-            self.set_fail_mode()
 
-    def delete(self):
+def map_config_to_obj(module):
-        '''Delete the bridge'''
+    templatized_command = "%(ovs-vsctl)s -t %(timeout)s list-br"
-        rtc, _, err = self._vsctl(['del-br', self.bridge])
+    command = templatized_command % module.params
-        if rtc != 0:
+    rc, out, err = module.run_command(command, check_rc=True)
-            self.module.fail_json(msg=err)
+    if rc != 0:
+        module.fail_json(msg=err)
 
-    def check(self):
+    obj = {}
-        '''Run check mode'''
-        changed = False
 
-        # pylint: disable=W0703
+    if module.params['bridge'] in out.splitlines():
-        try:
+        obj['bridge'] = module.params['bridge']
-            if self.state == 'present' and self.exists():
-                if (self.fail_mode and
-                   (self.fail_mode != self.get_fail_mode())):
-                    changed = True
 
-                ##
+        templatized_command = ("%(ovs-vsctl)s -t %(timeout)s br-to-parent"
-                # Check if external ids would change.
+                               " %(bridge)s")
-                current_external_ids = self.get_external_ids()
+        command = templatized_command % module.params
-                exp_external_ids = self.module.params['external_ids']
+        rc, out, err = module.run_command(command, check_rc=True)
-                if exp_external_ids is not None:
+        obj['parent'] = out.strip()
-                    for (key, value) in exp_external_ids:
-                        if ((key in current_external_ids) and
-                           (value != current_external_ids[key])):
-                            changed = True
 
-                    ##
+        templatized_command = ("%(ovs-vsctl)s -t %(timeout)s br-to-vlan"
-                    # Check if external ids would be removed.
+                               " %(bridge)s")
-                    for (key, value) in current_external_ids.items():
+        command = templatized_command % module.params
-                        if key not in exp_external_ids:
+        rc, out, err = module.run_command(command, check_rc=True)
-                            changed = True
+        obj['vlan'] = out.strip()
 
-            elif self.state == 'absent' and self.exists():
+        templatized_command = ("%(ovs-vsctl)s -t %(timeout)s get-fail-mode"
-                changed = True
+                               " %(bridge)s")
-            elif self.state == 'present' and not self.exists():
+        command = templatized_command % module.params
-                changed = True
+        rc, out, err = module.run_command(command, check_rc=True)
-        except Exception:
+        obj['fail_mode'] = out.strip()
-            earg = get_exception()
-            self.module.fail_json(msg=str(earg))
 
-        # pylint: enable=W0703
+        templatized_command = ("%(ovs-vsctl)s -t %(timeout)s br-get-external-id"
-        self.module.exit_json(changed=changed)
+                               " %(bridge)s")
+        command = templatized_command % module.params
+        rc, out, err = module.run_command(command, check_rc=True)
+        obj['external_ids'] = _external_ids_to_dict(out)
 
-    def run(self):
+    return obj
-        '''Make the necessary changes'''
-        changed = False
-        # pylint: disable=W0703
 
-        try:
-            if self.state == 'absent':
-                if self.exists():
-                    self.delete()
-                    changed = True
-            elif self.state == 'present':
 
-                if not self.exists():
+def map_params_to_obj(module):
-                    self.add()
+    obj = {
-                    changed = True
+        'bridge': module.params['bridge'],
-
+        'parent': module.params['parent'],
-                ##
+        'vlan': module.params['vlan'],
-                # If the -- set changed check here and make changes
+        'fail_mode': module.params['fail_mode'],
-                # but this only makes sense when state=present.
+        'external_ids': module.params['external_ids'],
-                if (not changed):
+        'set': module.params['set']
-                    changed = self.set(self.set_opt) or changed
+    }
-
-                current_fail_mode = self.get_fail_mode()
-                if self.fail_mode and (self.fail_mode != current_fail_mode):
-                    self.module.log( "changing fail mode %s to %s" % (current_fail_mode, self.fail_mode))
-                    self.set_fail_mode()
-                    changed = True
-
-                current_external_ids = self.get_external_ids()
-
-                ##
-                # Change and add existing external ids.
-                exp_external_ids = self.module.params['external_ids']
-                if exp_external_ids is not None:
-                    for (key, value) in exp_external_ids.items():
-                        if ((value != current_external_ids.get(key, None)) and
-                           self.set_external_id(key, value)):
-                            changed = True
-
-                    ##
-                    # Remove current external ids that are not passed in.
-                    for (key, value) in current_external_ids.items():
-                        if ((key not in exp_external_ids) and
-                           self.set_external_id(key, None)):
-                            changed = True
-
-        except Exception:
-            raise
-            earg = get_exception()
-            self.module.fail_json(msg=str(earg))
-        # pylint: enable=W0703
-        self.module.exit_json(changed=changed)
-
-    def get_external_ids(self):
-        """ Return the bridge's external ids as a dict. """
-        results = {}
-        if self.exists():
-            rtc, out, err = self._vsctl(['br-get-external-id', self.bridge])
-            if rtc != 0:
-                self.module.fail_json(msg=err)
-            lines = out.split("\n")
-            lines = [item.split("=") for item in lines if len(item) > 0]
-            for item in lines:
-                results[item[0]] = item[1]
-
-        return results
-
-    def set_external_id(self, key, value):
-        """ Set external id. """
-        if self.exists():
-            cmd = ['br-set-external-id', self.bridge, key]
-            if value:
-                cmd += [value]
-
-            (rtc, _, err) = self._vsctl(cmd)
-            if rtc != 0:
-                self.module.fail_json(msg=err)
-            return True
-        return False
-
-    def get_fail_mode(self):
-        """ Get failure mode. """
-        value = ''
-        if self.exists():
-            rtc, out, err = self._vsctl(['get-fail-mode', self.bridge])
-            if rtc != 0:
-                self.module.fail_json(msg=err)
-            value = out.strip("\n")
-        return value
-
-    def set_fail_mode(self):
-        """ Set failure mode. """
-
-        if self.exists():
-            (rtc, _, err) = self._vsctl(['set-fail-mode', self.bridge,
-                                         self.fail_mode])
-            if rtc != 0:
-                self.module.fail_json(msg=err)
 
+    return obj
 
 # pylint: disable=E0602
 def main():
     """ Entry point. """
-    module = AnsibleModule(
     argument_spec={
         'bridge': {'required': True},
         'parent': {'default': None},
@@ -363,23 +248,33 @@ def main():
         'external_ids': {'default': None, 'type': 'dict'},
         'fail_mode': {'default': None},
         'set': {'required': False, 'default': None}
-        },
+    }
-        supports_check_mode=True,
-    )
 
-    bridge = OVSBridge(module)
+    required_if = [('parent', not None, ('vlan',))]
-    if module.check_mode:
-        bridge.check()
-    else:
-        bridge.run()
 
-# pylint: disable=W0614
+    module = AnsibleModule(argument_spec=argument_spec,
-# pylint: disable=W0401
+                           required_if=required_if,
-# pylint: disable=W0622
+                           supports_check_mode=True)
+
+    result = {'changed': False}
+
+    # We add ovs-vsctl to module_params to later build up templatized commands
+    module.params["ovs-vsctl"] = module.get_bin_path("ovs-vsctl", True)
+
+    want = map_params_to_obj(module)
+    have = map_config_to_obj(module)
+
+    commands = map_obj_to_commands(want, have, module)
+    result['commands'] = commands
+
+    if commands:
+        if not module.check_mode:
+            for c in commands:
+                module.run_command(c, check_rc=True)
+        result['changed'] = True
+
+    module.exit_json(**result)
 
-# import module snippets
-from ansible.module_utils.basic import *
-from ansible.module_utils.pycompat24 import get_exception
 
 if __name__ == '__main__':
     main()

test/units/modules/network/ovs/fixtures/br_get_external_id_foo_bar.cfg

@@ -0,0 +1 @@
+foo=bar

test/units/modules/network/ovs/fixtures/br_to_parent_test_br.cfg

@@ -0,0 +1 @@
+test-br

test/units/modules/network/ovs/fixtures/br_to_vlan_zero.cfg

@@ -0,0 +1 @@
+0

test/units/modules/network/ovs/fixtures/get_fail_mode_secure.cfg

@@ -0,0 +1 @@
+secure

test/units/modules/network/ovs/fixtures/list_br_test_br.cfg

@@ -0,0 +1 @@
+test-br

test/units/modules/network/ovs/test_openvswitch_bridge.py

@@ -0,0 +1,201 @@
+#
+# (c) 2016 Red Hat Inc.
+#
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+
+# Make coding more python3-ish
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import json
+
+from ansible.compat.tests.mock import patch
+from ansible.modules.network.ovs import openvswitch_bridge
+from .ovs_module import TestOpenVSwitchModule, load_fixture, set_module_args
+
+test_name_side_effect_matrix = {
+    'test_openvswitch_bridge_absent_idempotent': [
+        (0, '', '')],
+    'test_openvswitch_bridge_absent_removes_bridge': [
+        (0, 'list_br_test_br.cfg', ''),
+        (0, '', ''),
+        (0, '', ''),
+        (0, '', ''),
+        (0, '', ''),
+        (0, '', '')],
+    'test_openvswitch_bridge_present_idempotent': [
+        (0, 'list_br_test_br.cfg', ''),
+        (0, 'br_to_parent_test_br.cfg', ''),
+        (0, 'br_to_vlan_zero.cfg', ''),
+        (0, 'get_fail_mode_secure.cfg', ''),
+        (0, 'br_get_external_id_foo_bar.cfg', '')],
+    'test_openvswitch_bridge_present_creates_bridge': [
+        (0, '', ''),
+        (0, '', ''),
+        (0, '', ''),
+        (0, '', '')],
+    'test_openvswitch_bridge_present_creates_fake_bridge': [
+        (0, '', ''),
+        (0, '', ''),
+        (0, '', ''),
+        (0, '', '')],
+    'test_openvswitch_bridge_present_adds_external_id': [
+        (0, 'list_br_test_br.cfg', ''),
+        (0, 'br_to_parent_test_br.cfg', ''),
+        (0, 'br_to_vlan_zero.cfg', ''),
+        (0, 'get_fail_mode_secure.cfg', ''),
+        (0, 'br_get_external_id_foo_bar.cfg', ''),
+        (0, '', '')],
+    'test_openvswitch_bridge_present_clears_external_id': [
+        (0, 'list_br_test_br.cfg', ''),
+        (0, 'br_to_parent_test_br.cfg', ''),
+        (0, 'br_to_vlan_zero.cfg', ''),
+        (0, 'get_fail_mode_secure.cfg', ''),
+        (0, 'br_get_external_id_foo_bar.cfg', ''),
+        (0, '', '')],
+    'test_openvswitch_bridge_present_changes_fail_mode': [
+        (0, 'list_br_test_br.cfg', ''),
+        (0, 'br_to_parent_test_br.cfg', ''),
+        (0, 'br_to_vlan_zero.cfg', ''),
+        (0, 'get_fail_mode_secure.cfg', ''),
+        (0, 'br_get_external_id_foo_bar.cfg', ''),
+        (0, '', '')],
+    'test_openvswitch_bridge_present_runs_set_mode': [
+        (0, '', ''),
+        (0, '', ''),
+        (0, '', ''),
+        (0, '', '')],
+}
+
+
+class TestOpenVSwitchBridgeModule(TestOpenVSwitchModule):
+
+    module = openvswitch_bridge
+
+    def setUp(self):
+        self.mock_run_command = (
+            patch('ansible.module_utils.basic.AnsibleModule.run_command'))
+        self.run_command = self.mock_run_command.start()
+        self.mock_get_bin_path = (
+            patch('ansible.module_utils.basic.AnsibleModule.get_bin_path'))
+        self.get_bin_path = self.mock_get_bin_path.start()
+
+    def tearDown(self):
+        self.mock_run_command.stop()
+        self.mock_get_bin_path.stop()
+
+    def load_fixtures(self, test_name):
+        test_side_effects = []
+        for s in test_name_side_effect_matrix[test_name]:
+            rc = s[0]
+            out = s[1] if s[1] == '' else str(load_fixture(s[1]))
+            err = s[2]
+            side_effect_with_fixture_loaded = (rc, out, err)
+            test_side_effects.append(side_effect_with_fixture_loaded)
+        self.run_command.side_effect = test_side_effects
+
+        self.get_bin_path.return_value = '/usr/bin/ovs-vsctl'
+
+    def test_openvswitch_bridge_absent_idempotent(self):
+        set_module_args(dict(state='absent',
+                             bridge='test-br'))
+        self.execute_module(test_name='test_openvswitch_bridge_absent_idempotent')
+
+    def test_openvswitch_bridge_absent_removes_bridge(self):
+        set_module_args(dict(state='absent',
+                             bridge='test-br'))
+        commands = ['/usr/bin/ovs-vsctl -t 5 del-br test-br']
+        self.execute_module(changed=True, commands=commands,
+                            test_name='test_openvswitch_bridge_absent_removes_bridge')
+
+    def test_openvswitch_bridge_present_idempotent(self):
+        set_module_args(dict(state='present',
+                             bridge='test-br',
+                             fail_mode='secure',
+                             external_ids={'foo': 'bar'}))
+        self.execute_module(test_name='test_openvswitch_bridge_present_idempotent')
+
+    def test_openvswitch_bridge_present_creates_bridge(self):
+        set_module_args(dict(state='present',
+                             bridge='test-br',
+                             fail_mode='secure',
+                             external_ids={'foo': 'bar'}))
+        commands = [
+            '/usr/bin/ovs-vsctl -t 5 add-br test-br',
+            '/usr/bin/ovs-vsctl -t 5 set-fail-mode test-br secure',
+            '/usr/bin/ovs-vsctl -t 5 br-set-external-id test-br foo bar'
+        ]
+        self.execute_module(changed=True, commands=commands,
+                            test_name='test_openvswitch_bridge_present_creates_bridge')
+
+    def test_openvswitch_bridge_present_creates_fake_bridge(self):
+        set_module_args(dict(state='present',
+                             bridge='test-br2',
+                             parent='test-br',
+                             vlan=10))
+        commands = [
+            '/usr/bin/ovs-vsctl -t 5 add-br test-br2 test-br 10',
+        ]
+        self.execute_module(changed=True, commands=commands,
+                            test_name='test_openvswitch_bridge_present_creates_fake_bridge')
+
+    def test_openvswitch_bridge_present_adds_external_id(self):
+        set_module_args(dict(state='present',
+                             bridge='test-br',
+                             fail_mode='secure',
+                             external_ids={'bip': 'bop'}))
+        commands = [
+            '/usr/bin/ovs-vsctl -t 5 br-set-external-id test-br bip bop'
+        ]
+        self.execute_module(changed=True, commands=commands,
+                            test_name='test_openvswitch_bridge_present_adds_external_id')
+
+    def test_openvswitch_bridge_present_clears_external_id(self):
+        set_module_args(dict(state='present',
+                             bridge='test-br',
+                             fail_mode='secure',
+                             external_ids={'foo': ''}))
+        commands = [
+            '/usr/bin/ovs-vsctl -t 5 br-set-external-id test-br foo '
+        ]
+        self.execute_module(changed=True, commands=commands,
+                            test_name='test_openvswitch_bridge_present_clears_external_id')
+
+    def test_openvswitch_bridge_present_changes_fail_mode(self):
+        set_module_args(dict(state='present',
+                             bridge='test-br',
+                             fail_mode='standalone',
+                             external_ids={'foo': 'bar'}))
+        commands = [
+            '/usr/bin/ovs-vsctl -t 5 set-fail-mode test-br standalone'
+        ]
+        self.execute_module(changed=True, commands=commands,
+                            test_name='test_openvswitch_bridge_present_changes_fail_mode')
+
+    def test_openvswitch_bridge_present_runs_set_mode(self):
+        set_module_args(dict(state='present',
+                             bridge='test-br',
+                             fail_mode='secure',
+                             external_ids={'foo': 'bar'},
+                             set="bridge test-br datapath_type=netdev"))
+        commands = [
+            '/usr/bin/ovs-vsctl -t 5 add-br test-br -- set bridge test-br'
+            ' datapath_type=netdev',
+            '/usr/bin/ovs-vsctl -t 5 set-fail-mode test-br secure',
+            '/usr/bin/ovs-vsctl -t 5 br-set-external-id test-br foo bar'
+        ]
+        self.execute_module(changed=True, commands=commands,
+                            test_name='test_openvswitch_bridge_present_runs_set_mode')