From 530788a9dc6a359d3fb4063a2f6cbfe278c74bca Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 9 May 2017 13:48:34 +0200 Subject: [PATCH 1/4] library: cleanup handling of 'wait' argument - extend the maxiumum wait time to 3600 seconds - disallow -1 values for 'wait'. It was used to mark the default value, but that marker should not be a valid value for user configuration. Instead, internally mark the missing value with None. - don't coerce a missing 'wait' parameter to 90 in _validate_post(). Instead, track it as None so we can later decide between an explicitly set paramter and a default value. Also, the default for the states 'up', 'down', and 'wait' differ. This also allows us to slience the warning about 'wait' not being implemented -- in case the user didn't configure it. - However, still allow zero. For - state: wait wait: 0 allow it for convinience, so a user can disable the statement by setting the wait value to zero. For - name: MyProfile state: up wait: 0 'wait' zero makes sense. It is what we currently do -- not wait at all. Later, when we implement waiting for complete activation, the default of wait will change (to 90 in this case). --- library/network_connections.py | 34 +++++++++++++++++------------ library/test_network_connections.py | 10 ++++----- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/library/network_connections.py b/library/network_connections.py index 49873dc..bdaaa62 100755 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -642,7 +642,7 @@ class ArgValidator_DictConnection(ArgValidatorDict): nested = [ ArgValidatorStr ('name'), ArgValidatorStr ('state', enum_values = ArgValidator_DictConnection.VALID_STATES), - ArgValidatorInt ('wait', val_min = -1, val_max = 1200), + ArgValidatorInt ('wait', val_min = 0, val_max = 3600), ArgValidatorStr ('type', enum_values = ArgValidator_DictConnection.VALID_TYPES), ArgValidatorBool('autoconnect', default_value = True), ArgValidatorStr ('slave_type', enum_values = ArgValidator_DictConnection.VALID_SLAVE_TYPES), @@ -694,14 +694,9 @@ class ArgValidator_DictConnection(ArgValidatorDict): if 'name' not in result: raise ValidationError(name, 'missing "name"') - if result['state'] == 'wait': - if result.get('wait', -1) == -1: - result['wait'] = 10 - elif result['wait'] == 0: - raise ValidationError(name + '.wait', 'the "wait" value for state "wait" must be positive') - elif result['state'] in ['up', 'down']: - if result.get('wait', -1) == -1: - result['wait'] = 90 + if result['state'] in [ 'wait', 'up', 'down' ]: + if 'wait' not in result: + result['wait'] = None else: if 'wait' in result: raise ValidationError(name + '.wait', '"wait" is not allowed for state "%s"' % (result['state'])) @@ -1571,10 +1566,13 @@ class Cmd: try: state = connection['state'] if state == 'wait': - AnsibleUtil.log_info(idx, 'wait for %s seconds' % (connection['wait'])) + w = connection['wait'] + if w is None: + w = 10 + AnsibleUtil.log_info(idx, 'wait for %s seconds' % (w)) if AnsibleUtil.check_mode == CheckMode.REAL_RUN: import time - time.sleep(connection['wait']) + time.sleep(w) elif state == 'absent': self.run_state_absent(idx) elif state == 'present': @@ -1729,7 +1727,11 @@ class Cmd_nm(Cmd): def run_state_up(self, idx): connection = AnsibleUtil.connections[idx] - if connection['wait'] != 0: + w = connection['wait'] + if w is None: + w = 90 + else: + # TODO AnsibleUtil.log_warn(idx, 'wait for activation is not yet implemented') con = Util.first(self.nmutil.connection_list(name = connection['name'], uuid = connection['nm.uuid'])) @@ -1751,7 +1753,11 @@ class Cmd_nm(Cmd): def run_state_down(self, idx): connection = AnsibleUtil.connections[idx] - if connection['wait'] != 0: + w = connection['wait'] + if w is None: + w = 10 + else: + # TODO AnsibleUtil.log_warn(idx, 'wait for activation is not yet implemented') cons = self.nmutil.connection_list(name = connection['name']) @@ -1863,7 +1869,7 @@ class Cmd_initscripts(Cmd): connection = AnsibleUtil.connections[idx] name = connection['name'] - if connection['wait'] != 0: + if connection['wait'] is not None: # initscripts don't support wait, they always block until the ifup/ifdown # command completes. Silently ignore the argument. pass diff --git a/library/test_network_connections.py b/library/test_network_connections.py index 90d075e..199772c 100755 --- a/library/test_network_connections.py +++ b/library/test_network_connections.py @@ -134,7 +134,7 @@ class TestValidator(unittest.TestCase): { 'name': '5', 'state': 'up', - 'wait': 90, + 'wait': None, 'ignore_errors': None, } ], @@ -173,7 +173,7 @@ class TestValidator(unittest.TestCase): 'interface_name': None, 'check_iface_exists': True, 'slave_type': None, - 'wait': 90, + 'wait': None, }, ], n.AnsibleUtil.ARGS_CONNECTIONS.validate([ @@ -275,7 +275,7 @@ class TestValidator(unittest.TestCase): 'interface_name': None, 'check_iface_exists': True, 'slave_type': None, - 'wait': 90, + 'wait': None, }, ], n.AnsibleUtil.ARGS_CONNECTIONS.validate([ @@ -314,7 +314,7 @@ class TestValidator(unittest.TestCase): 'interface_name': None, 'check_iface_exists': True, 'slave_type': None, - 'wait': 90, + 'wait': None, }, ], n.AnsibleUtil.ARGS_CONNECTIONS.validate([ @@ -355,7 +355,7 @@ class TestValidator(unittest.TestCase): 'interface_name': None, 'check_iface_exists': True, 'slave_type': None, - 'wait': 90, + 'wait': None, }, ], n.AnsibleUtil.ARGS_CONNECTIONS.validate([ From a5e62f27db01288b8ee1c43edf5729f9678b7110 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 9 May 2017 14:29:09 +0200 Subject: [PATCH 2/4] library: implement waiting for activation/deactivation with NetworkManager Implement what nmcli does when activating/deactivating a connection: waits until the device is fully activated or deactivate. The 'wait' option is basically like nmcli's --wait option. If omitted, wait a default duration of time. If the action does not complete within the specified timeout, it fails. Note that a failure may be suppressed by setting 'ignore_errors', either globaly or per-state. Set 'wait=0' not to wait. This was the previous behavior before this commit. --- library/network_connections.py | 170 ++++++++++++++++++++++++++++----- 1 file changed, 146 insertions(+), 24 deletions(-) diff --git a/library/network_connections.py b/library/network_connections.py index bdaaa62..8e8773f 100755 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -85,10 +85,11 @@ class Util: if n is None: import gi gi.require_version('NM', '1.0') - from gi.repository import NM, GLib, Gio + from gi.repository import NM, GLib, Gio, GObject cls._NM = NM cls._GLib = GLib cls._Gio = Gio + cls._GObject = GObject n = NM import uuid cls._uuid = uuid @@ -104,6 +105,11 @@ class Util: cls.NM() return cls._Gio + @classmethod + def GObject(cls): + cls.NM() + return cls._GObject + @classmethod def Timestamp(cls): return cls.GLib().get_monotonic_time() @@ -1090,6 +1096,16 @@ class NMUtil: nmclient = Util.NM().Client.new(None) self.nmclient = nmclient + def device_is_master_type(self, dev): + if dev: + NM = Util.NM() + GObject = Util.GObject() + if GObject.type_is_a(dev, NM.DeviceBond) \ + or GObject.type_is_a(dev, NM.DeviceBridge) \ + or GObject.type_is_a(dev, NM.DeviceTeam): + return True + return False + def active_connection_list(self, connections = None, black_list = None, mainloop_iterate = True): if mainloop_iterate: Util.GMainLoop_iterate_all() @@ -1263,7 +1279,7 @@ class NMUtil: raise MyError('created connection failed to normalize: %s' % (e)) return con - def connection_add(self, con, timeout = 5): + def connection_add(self, con, timeout = 10): def add_cb(client, result, cb_args): con = None @@ -1286,9 +1302,7 @@ class NMUtil: raise MyError('failure to add connection: %s' % (cb_args.get('error', 'unknown error'))) return cb_args['con'] - def connection_update(self, con, con_new, timeout = 5): - NM = Util.NM() - + def connection_update(self, con, con_new, timeout = 10): con.replace_settings_from_connection(con_new) def update_cb(connection, result, cb_args): @@ -1312,7 +1326,7 @@ class NMUtil: raise MyError('failure to update connection: %s' % (cb_args.get('error', 'unknown error'))) return True - def connection_delete(self, connection, timeout = 5): + def connection_delete(self, connection, timeout = 10): def delete_cb(connection, result, cb_args): success = False @@ -1334,7 +1348,7 @@ class NMUtil: if not cb_args.get('success', False): raise MyError('failure to delete connection: %s' % (cb_args.get('error', 'unknown error'))) - def connection_activate(self, connection, timeout = 5): + def connection_activate(self, connection, timeout = 15, wait_time = None): def activate_cb(client, result, cb_args): active_connection = None @@ -1355,9 +1369,85 @@ class NMUtil: raise MyError('failure to activate connection: %s' % ('timeout')) if not cb_args.get('active_connection', None): raise MyError('failure to activate connection: %s' % (cb_args.get('error', 'unknown error'))) - return cb_args['active_connection'] - def active_connection_deactivate(self, ac, timeout = 5): + ac = cb_args['active_connection'] + self.connection_activate_wait(ac, wait_time) + return ac + + def connection_activate_wait(self, ac, wait_time): + + if not wait_time: + return + + NM = Util.NM() + + state = ac.get_state() + if state == NM.ActiveConnectionState.ACTIVATED: + return + if state != NM.ActiveConnectionState.ACTIVATING: + raise MyError('activation is in unexpected state "%s"' % (state)) + + def check_activated(ac, dev): + ac_state = ac.get_state() + ac_reason = ac.get_state_reason() + if dev: + dev_state = dev.get_state() + + if ac_state == NM.ActiveConnectionState.ACTIVATING: + if self.device_is_master_type(dev) \ + and dev_state >= NM.DeviceState.IP_CONFIG \ + and dev_state <= NM.DeviceState.ACTIVATED: + # master connections qualify as activated once they reach IP-Config state. + # That is because they may wait for slave devices to attach + return True, None + # fall through + elif ac_state == NM.ActiveConnectionState.ACTIVATED: + return True, None + elif ac_state == NM.ActiveConnectionState.DEACTIVATED: + if not dev \ + or ac_reason != NM.ActiveConnectionStateReason.DEVICE_DISCONNECTED \ + or dev.get_active_connection() is not ac: + return True, (ac_reason.value_nick or 'unknown reason') + # the state of the active connection is not very helpful. + # see if the device-state is better. + if dev_state <= NM.DeviceState.DISCONNECTED or dev_state > NM.DeviceState.DEACTIVATING: + return True, (dev.get_state_reason().value_nick or ac_reason.value_nick or 'unknown reason') + # fall through, wait longer for a better state reason. + + # wait longer. + return False, None + + dev = Util.first(ac.get_devices()) + + complete, failure_reason = check_activated(ac, dev) + + if not complete: + + cb_out = [] + def check_activated_cb(): + complete, failure_reason = check_activated(ac, dev) + if complete: + cb_out.append(failure_reason) + Util.GMainLoop().quit() + + ac_id = ac.connect('state-changed', lambda source, state, reason: check_activated_cb()) + if dev: + dev_id = dev.connect('notify::state', lambda source, pspec: check_activated_cb()) + + try: + if not Util.GMainLoop_run(wait_time): + raise MyError('connection not fully activated after timeout') + finally: + if dev: + dev.handler_disconnect(dev_id) + ac.handler_disconnect(ac_id) + + failure_reason = cb_out[0] + + if failure_reason: + raise MyError('connection not activated: %s' % (failure_reason)) + + def active_connection_deactivate(self, ac, timeout = 10, wait_time = None): def deactivate_cb(client, result, cb_args): success = False @@ -1378,8 +1468,35 @@ class NMUtil: raise MyError('failure to deactivate connection: %s' % (timeout)) if not cb_args.get('success', False): raise MyError('failure to deactivate connection: %s' % (cb_args.get('error', 'unknown error'))) + + self.active_connection_deactivate_wait(ac, wait_time) return True + def active_connection_deactivate_wait(self, ac, wait_time): + + if not wait_time: + return + + NM = Util.NM() + + def check_deactivated(ac): + return ac.get_state() >= NM.ActiveConnectionState.DEACTIVATED + + if not check_deactivated(ac): + + def check_deactivated_cb(): + if check_deactivated(ac): + Util.GMainLoop().quit() + + ac_id = ac.connect('notify::state', lambda source, pspec: check_deactivated_cb()) + + try: + if not Util.GMainLoop_run(wait_time): + raise MyError('connection not fully deactivated after timeout') + finally: + ac.handler_disconnect(ac_id) + + ############################################################################### class _AnsibleUtil: @@ -1727,13 +1844,6 @@ class Cmd_nm(Cmd): def run_state_up(self, idx): connection = AnsibleUtil.connections[idx] - w = connection['wait'] - if w is None: - w = 90 - else: - # TODO - AnsibleUtil.log_warn(idx, 'wait for activation is not yet implemented') - con = Util.first(self.nmutil.connection_list(name = connection['name'], uuid = connection['nm.uuid'])) if not con: if AnsibleUtil.check_mode == CheckMode.REAL_RUN: @@ -1744,22 +1854,24 @@ class Cmd_nm(Cmd): AnsibleUtil.log_info(idx, 'up connection %s, %s' % (con.get_id(), con.get_uuid())) if AnsibleUtil.check_mode == CheckMode.REAL_RUN: try: - self.nmutil.connection_activate (con) + ac = self.nmutil.connection_activate (con) except MyError as e: AnsibleUtil.log_error(idx, 'up connection failed: %s' % (e)) + wait_time = connection['wait'] + if wait_time is None: + wait_time = 90 + + try: + self.nmutil.connection_activate_wait(ac, wait_time) + except MyError as e: + AnsibleUtil.log_error(idx, 'up connection failed while waiting: %s' % (e)) + AnsibleUtil.run_results_changed(idx) def run_state_down(self, idx): connection = AnsibleUtil.connections[idx] - w = connection['wait'] - if w is None: - w = 10 - else: - # TODO - AnsibleUtil.log_warn(idx, 'wait for activation is not yet implemented') - cons = self.nmutil.connection_list(name = connection['name']) changed = False if cons: @@ -1776,6 +1888,16 @@ class Cmd_nm(Cmd): self.nmutil.active_connection_deactivate(ac) except MyError as e: AnsibleUtil.log_error(idx, 'down connection failed: %s' % (e)) + + wait_time = connection['wait'] + if wait_time is None: + wait_time = 10 + + try: + self.nmutil.active_connection_deactivate_wait(ac, wait_time) + except MyError as e: + AnsibleUtil.log_error(idx, 'down connection failed while waiting: %s' % (e)) + cons = self.nmutil.connection_list(name = connection['name']) if not changed: From 948d36fa42d751a573706b3871a6d829430aa0d6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 May 2017 18:04:57 +0200 Subject: [PATCH 3/4] library: extend ArgValidatorInt for float and rename to ArgValidatorNum Let's handle floating point types similarly to integer types. The only difference is the "numeric_type" argument. --- library/network_connections.py | 27 +++++++++++++++++---------- library/test_network_connections.py | 20 +++++++++++++++----- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/library/network_connections.py b/library/network_connections.py index 8e8773f..c430049 100755 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -453,18 +453,25 @@ class ArgValidatorStr(ArgValidator): raise ValidationError(name, 'cannot be empty') return v -class ArgValidatorInt(ArgValidator): - def __init__(self, name, required = False, val_min = None, val_max = None, default_value = 0): - ArgValidator.__init__(self, name, required, default_value) +class ArgValidatorNum(ArgValidator): + def __init__(self, name, required = False, val_min = None, val_max = None, + default_value = ArgValidator.MISSING, + numeric_type = int): + ArgValidator.__init__(self, name, required, \ + numeric_type(0) if default_value is ArgValidator.MISSING else default_value) self.val_min = val_min self.val_max = val_max + self.numeric_type = numeric_type def _validate(self, value, name): v = None try: - if isinstance(value, int): + if isinstance(value, self.numeric_type): v = value - if isinstance(value, Util.STRING_TYPE): - v = int(value) + else: + v2 = self.numeric_type(value) + if isinstance(value, Util.STRING_TYPE) \ + or v2 == value: + v = v2 except: pass if v is None: @@ -590,10 +597,10 @@ class ArgValidator_DictIP(ArgValidatorDict): ArgValidatorBool('dhcp4', default_value = None), ArgValidatorBool('dhcp4_send_hostname', default_value = None), ArgValidatorIP ('gateway4', family = socket.AF_INET), - ArgValidatorInt ('route_metric4', val_min = -1, val_max = 0xFFFFFFFF, default_value = None), + ArgValidatorNum ('route_metric4', val_min = -1, val_max = 0xFFFFFFFF, default_value = None), ArgValidatorBool('auto6', default_value = None), ArgValidatorIP ('gateway6', family = socket.AF_INET6), - ArgValidatorInt ('route_metric6', val_min = -1, val_max = 0xFFFFFFFF, default_value = None), + ArgValidatorNum ('route_metric6', val_min = -1, val_max = 0xFFFFFFFF, default_value = None), ArgValidatorList('address', nested = ArgValidatorIPAddr('address[?]'), default_value = list, @@ -648,7 +655,7 @@ class ArgValidator_DictConnection(ArgValidatorDict): nested = [ ArgValidatorStr ('name'), ArgValidatorStr ('state', enum_values = ArgValidator_DictConnection.VALID_STATES), - ArgValidatorInt ('wait', val_min = 0, val_max = 3600), + ArgValidatorNum ('wait', val_min = 0, val_max = 3600), ArgValidatorStr ('type', enum_values = ArgValidator_DictConnection.VALID_TYPES), ArgValidatorBool('autoconnect', default_value = True), ArgValidatorStr ('slave_type', enum_values = ArgValidator_DictConnection.VALID_SLAVE_TYPES), @@ -657,7 +664,7 @@ class ArgValidator_DictConnection(ArgValidatorDict): ArgValidatorMac ('mac'), ArgValidatorBool('check_iface_exists', default_value = True), ArgValidatorStr ('parent'), - ArgValidatorInt ('vlan_id', val_min = 0, val_max = 4095, default_value = None), + ArgValidatorNum ('vlan_id', val_min = 0, val_max = 4095, default_value = None), ArgValidatorBool('ignore_errors', default_value = None), ArgValidator_DictIP(), ], diff --git a/library/test_network_connections.py b/library/test_network_connections.py index 199772c..19ef018 100755 --- a/library/test_network_connections.py +++ b/library/test_network_connections.py @@ -27,13 +27,23 @@ class TestValidator(unittest.TestCase): def test_validate_int(self): - v = n.ArgValidatorInt('state', default_value = None) + v = n.ArgValidatorNum('state', default_value = None, numeric_type = float) self.assertEqual(1, v.validate(1)) - self.assertEqual(1, v.validate("1")) + self.assertEqual(1.5, v.validate(1.5)) + self.assertEqual(1.5, v.validate("1.5")) self.assertValidationError(v, None) self.assertValidationError(v, "1a") - v = n.ArgValidatorInt('state', required = True) + v = n.ArgValidatorNum('state', default_value = None) + self.assertEqual(1, v.validate(1)) + self.assertEqual(1, v.validate(1.0)) + self.assertEqual(1, v.validate("1")) + self.assertValidationError(v, None) + self.assertValidationError(v, None) + self.assertValidationError(v, 1.5) + self.assertValidationError(v, "1.5") + + v = n.ArgValidatorNum('state', required = True) self.assertValidationError(v, None) def test_validate_bool(self): @@ -56,7 +66,7 @@ class TestValidator(unittest.TestCase): v = n.ArgValidatorDict( 'dict', nested = [ - n.ArgValidatorInt('i', required = True), + n.ArgValidatorNum('i', required = True), n.ArgValidatorStr('s', required = False, default_value = 's_default'), n.ArgValidatorStr('l', required = False, default_value = n.ArgValidator.MISSING), ]) @@ -87,7 +97,7 @@ class TestValidator(unittest.TestCase): v = n.ArgValidatorList( 'list', - nested = n.ArgValidatorInt('i') + nested = n.ArgValidatorNum('i') ) self.assertEqual( [ 1, 5 ], From 2844b3021c6b035889a14dec6024598018b5cd65 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 May 2017 18:11:31 +0200 Subject: [PATCH 4/4] library: support sub-second precision for 'wait' property For testing it is useful to be able to configure very short 'wait' times. Otherwise, the minimally configuable timeout is 1 second. --- library/network_connections.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/network_connections.py b/library/network_connections.py index c430049..9d66ca4 100755 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -655,7 +655,7 @@ class ArgValidator_DictConnection(ArgValidatorDict): nested = [ ArgValidatorStr ('name'), ArgValidatorStr ('state', enum_values = ArgValidator_DictConnection.VALID_STATES), - ArgValidatorNum ('wait', val_min = 0, val_max = 3600), + ArgValidatorNum ('wait', val_min = 0, val_max = 3600, numeric_type = float), ArgValidatorStr ('type', enum_values = ArgValidator_DictConnection.VALID_TYPES), ArgValidatorBool('autoconnect', default_value = True), ArgValidatorStr ('slave_type', enum_values = ArgValidator_DictConnection.VALID_SLAVE_TYPES),