mirror of
https://github.com/linux-system-roles/network.git
synced 2026-01-23 02:15:17 +00:00
library: don't do anything for "up" and "down" states if already in state
The states "up" and "down" previously would always change state. That is, specifying them in the playbook will always invoke `ifup` (or `ifdown`) or the corresponding of `nmcli connection up` (or `nmcli connection down`). That was intentional behavior, because the role doesn't really know which profile is currently active. That is certainly the case for "initscripts", where the role has almost no information about the current runtime state. For "nm" provider, the role knows whether the connection is already active. However, that alone also does not guarantee that the current runtime state is idential to what would be the result of an explicit `nmcli connection up`. Hence, to be sure that the current state is always as expected, the role would always explicitly issue the commands and report "changed=1". That is quite harmful, because running the same role multiple times should not report changes every time. Also, issuing `ifup` may behave badly, if the interface is already configured. Now, try to determine whether the desire "up" or "down" state is already reached and do nothing. For "nm" provider that is easy and quite safe. There is still the possibility to trick the role into thinking that the right configuration is active, when it actually is not. For example via `nmcli device modify` on the host. But in general, it should work just fine. Especially, if the admin manually modifies the runtime state, it may be just desired for "state: up" not to change anything. For "initscripts" this is much more fragile. There isn't really much that can be done about it, because the role doesn't know what is currently configured on the system. There is also a new option "force_state_change" to restore the previous behavior. https://bugzilla.redhat.com/show_bug.cgi?id=1476053
This commit is contained in:
Thomas Haller
parent
2eaf5a9a61
commit
5deb2afd46
3 changed files with 153 additions and 17 deletions
|
|
@ -699,6 +699,7 @@ class ArgValidator_DictConnection(ArgValidatorDict):
|
|||
nested = [
|
||||
ArgValidatorStr ('name'),
|
||||
ArgValidatorStr ('state', enum_values = ArgValidator_DictConnection.VALID_STATES),
|
||||
ArgValidatorBool('force_state_change', default_value = None),
|
||||
ArgValidatorNum ('wait', val_min = 0, val_max = 3600, numeric_type = float),
|
||||
ArgValidatorStr ('type', enum_values = ArgValidator_DictConnection.VALID_TYPES),
|
||||
ArgValidatorBool('autoconnect', default_value = True),
|
||||
|
|
@ -733,8 +734,9 @@ class ArgValidator_DictConnection(ArgValidatorDict):
|
|||
VALID_FIELDS = list(self.nested.keys())
|
||||
if result['state'] == 'present':
|
||||
VALID_FIELDS.remove('wait')
|
||||
VALID_FIELDS.remove('force_state_change')
|
||||
elif result['state'] in ['up', 'down']:
|
||||
VALID_FIELDS = ['name', 'state', 'wait', 'ignore_errors']
|
||||
VALID_FIELDS = ['name', 'state', 'wait', 'ignore_errors', 'force_state_change']
|
||||
elif result['state'] == 'absent':
|
||||
VALID_FIELDS = ['name', 'state', 'ignore_errors']
|
||||
elif result['state'] == 'wait':
|
||||
|
|
@ -1183,6 +1185,35 @@ class IfcfgUtil:
|
|||
with open(path, 'w') as text_file:
|
||||
text_file.write(h)
|
||||
|
||||
@classmethod
|
||||
def connection_seems_active(cls, name):
|
||||
# we don't know whether a ifcfg file is currently active,
|
||||
# and we also don't know which.
|
||||
#
|
||||
# Do a very basic guess based on whether the interface
|
||||
# is in operstate "up".
|
||||
#
|
||||
# But first we need to find the interface name. Do
|
||||
# some naive parsing and check for DEVICE setting.
|
||||
content = cls.content_from_file(name, 'ifcfg')
|
||||
if content['ifcfg'] is not None:
|
||||
content = cls.ifcfg_parse(content['ifcfg'])
|
||||
else:
|
||||
content = {}
|
||||
if 'DEVICE' not in content:
|
||||
return None
|
||||
path = '/sys/class/net/' + content['DEVICE'] + '/operstate'
|
||||
try:
|
||||
with open(path, 'r') as content_file:
|
||||
i_content = str(content_file.read())
|
||||
except Exception as e:
|
||||
return None
|
||||
|
||||
if i_content.strip() != 'up':
|
||||
return False
|
||||
|
||||
return True
|
||||
|
||||
###############################################################################
|
||||
|
||||
class NMUtil:
|
||||
|
|
@ -1216,8 +1247,7 @@ class NMUtil:
|
|||
active_cons = [ac for ac in active_cons if ac.get_connection() in connections]
|
||||
if black_list:
|
||||
active_cons = [ac for ac in active_cons if ac not in black_list]
|
||||
active_cons = list(active_cons)
|
||||
return active_cons;
|
||||
return list(active_cons)
|
||||
|
||||
def connection_list(self, name = None, uuid = None, black_list = None, black_list_names = None, black_list_uuids = None):
|
||||
cons = self.nmclient.get_connections()
|
||||
|
|
@ -1275,6 +1305,14 @@ class NMUtil:
|
|||
|
||||
return not(not(con_a.compare (con_b, compare_flags)))
|
||||
|
||||
def connection_is_active(self, con):
|
||||
NM = Util.NM()
|
||||
for ac in self.active_connection_list(connections=[con]):
|
||||
if ac.get_state() >= NM.ActiveConnectionState.ACTIVATING \
|
||||
and ac.get_state() <= NM.ActiveConnectionState.ACTIVATED:
|
||||
return True
|
||||
return False
|
||||
|
||||
def connection_create(self, connections, idx):
|
||||
NM = Util.NM()
|
||||
|
||||
|
|
@ -1655,6 +1693,7 @@ class _AnsibleUtil:
|
|||
|
||||
ARGS = {
|
||||
'ignore_errors': { 'required': False, 'default': False, 'type': 'str' },
|
||||
'force_state_change': { 'required': False, 'default': False, 'type': 'bool' },
|
||||
'provider': { 'required': True, 'default': None, 'type': 'str' },
|
||||
'connections': { 'required': False, 'default': None, 'type': 'list' },
|
||||
}
|
||||
|
|
@ -1716,6 +1755,15 @@ class _AnsibleUtil:
|
|||
v = default_value
|
||||
return v
|
||||
|
||||
def params_force_state_change(self, connection, default_value = None):
|
||||
v = connection['force_state_change']
|
||||
if v is None:
|
||||
if 'force_state_change' in self.params:
|
||||
v = Util.boolean(self.params['force_state_change'])
|
||||
if v is None:
|
||||
v = default_value
|
||||
return v
|
||||
|
||||
@property
|
||||
def connections(self):
|
||||
c = self._connections
|
||||
|
|
@ -1728,6 +1776,40 @@ class _AnsibleUtil:
|
|||
self._connections = c
|
||||
return c
|
||||
|
||||
def connection_modified_earlier(self, idx):
|
||||
# for index @idx, check if any of the previous profiles [0..idx[
|
||||
# modify the connection.
|
||||
|
||||
con = self.connections[idx]
|
||||
assert(con['state'] in ['up', 'down'])
|
||||
|
||||
# also check, if the current profile is 'up' with a 'type' (which
|
||||
# possibly modifies the connection as well)
|
||||
if con['state'] == 'up' \
|
||||
and 'type' in con \
|
||||
and self.run_results[idx]['changed']:
|
||||
return True
|
||||
|
||||
for i in reversed(range(idx)):
|
||||
c = self.connections[i]
|
||||
if 'name' not in c:
|
||||
continue
|
||||
if c['name'] != con['name']:
|
||||
continue
|
||||
|
||||
c_state = c['state']
|
||||
if c_state == 'up' and 'type' not in c:
|
||||
pass
|
||||
elif c_state == 'down':
|
||||
return True
|
||||
elif c_state == 'absent':
|
||||
return True
|
||||
elif c_state in ['present', 'up']:
|
||||
if self.run_results[i]['changed']:
|
||||
return True
|
||||
|
||||
return False
|
||||
|
||||
@property
|
||||
def run_results(self):
|
||||
c = self._run_results
|
||||
|
|
@ -2021,7 +2103,21 @@ class Cmd_nm(Cmd):
|
|||
else:
|
||||
AnsibleUtil.log_info(idx, 'up connection %s, %s' % (connection['name'], connection['nm.uuid']))
|
||||
return
|
||||
AnsibleUtil.log_info(idx, 'up connection %s, %s' % (con.get_id(), con.get_uuid()))
|
||||
|
||||
is_active = self.nmutil.connection_is_active(con)
|
||||
is_modified = AnsibleUtil.connection_modified_earlier(idx)
|
||||
force_state_change = AnsibleUtil.params_force_state_change(connection, False)
|
||||
|
||||
if is_active and not force_state_change and not is_modified:
|
||||
AnsibleUtil.log_info(idx, 'up connection %s, %s skipped because already active' %
|
||||
(con.get_id(), con.get_uuid()))
|
||||
return
|
||||
|
||||
AnsibleUtil.log_info(idx, 'up connection %s, %s (%s)' %
|
||||
(con.get_id(), con.get_uuid(),
|
||||
'not-active' if not is_active else \
|
||||
'is-modified' if is_modified else \
|
||||
'force-state-change'))
|
||||
if AnsibleUtil.check_mode == CheckMode.REAL_RUN:
|
||||
try:
|
||||
ac = self.nmutil.connection_activate (con)
|
||||
|
|
@ -2154,7 +2250,7 @@ class Cmd_initscripts(Cmd):
|
|||
|
||||
AnsibleUtil.run_results_changed(idx)
|
||||
|
||||
def _run_state_updown(self, idx, cmd):
|
||||
def _run_state_updown(self, idx, do_up):
|
||||
if not self.check_name(idx):
|
||||
return
|
||||
|
||||
|
|
@ -2166,29 +2262,56 @@ class Cmd_initscripts(Cmd):
|
|||
# command completes. Silently ignore the argument.
|
||||
pass
|
||||
|
||||
AnsibleUtil.log_info(idx, 'call `%s %s`' % (cmd, name))
|
||||
|
||||
path = IfcfgUtil.ifcfg_path(name)
|
||||
if not os.path.isfile(path):
|
||||
if AnsibleUtil.check_mode == CheckMode.REAL_RUN:
|
||||
AnsibleUtil.log_error(idx, 'ifcfg file "%s" does not exist' % (path))
|
||||
else:
|
||||
AnsibleUtil.log_info(idx, 'ifcfg file "%s" does not exist in check mode' % (path))
|
||||
return
|
||||
|
||||
is_active = IfcfgUtil.connection_seems_active(name)
|
||||
is_modified = AnsibleUtil.connection_modified_earlier(idx)
|
||||
force_state_change = AnsibleUtil.params_force_state_change(connection, False)
|
||||
|
||||
if do_up:
|
||||
if is_active is True and not force_state_change and not is_modified:
|
||||
AnsibleUtil.log_info(idx, 'up connection %s skipped because already active' %
|
||||
(name))
|
||||
return
|
||||
|
||||
AnsibleUtil.log_info(idx, 'up connection %s (%s)' %
|
||||
(name,
|
||||
'not-active' if is_active is not True else \
|
||||
'is-modified' if is_modified else \
|
||||
'force-state-change'))
|
||||
cmd = 'ifup'
|
||||
else:
|
||||
if AnsibleUtil.check_mode == CheckMode.REAL_RUN:
|
||||
rc, out, err = AnsibleUtil.module.run_command([cmd, name], encoding=None)
|
||||
AnsibleUtil.log_info(idx, 'call `%s %s`: rc=%d, out="%s", err="%s"' % (cmd, name, rc, out, err))
|
||||
if rc != 0:
|
||||
AnsibleUtil.log_error(idx, 'call `%s %s` failed with exit status %d' % (cmd, name, rc))
|
||||
if is_active is False and not force_state_change:
|
||||
AnsibleUtil.log_info(idx, 'down connection %s skipped because not active' %
|
||||
(name))
|
||||
return
|
||||
|
||||
AnsibleUtil.log_info(idx, 'up connection %s (%s)' %
|
||||
(name,
|
||||
'active' if is_active is not False else \
|
||||
'force-state-change'))
|
||||
cmd = 'ifdown'
|
||||
|
||||
if AnsibleUtil.check_mode == CheckMode.REAL_RUN:
|
||||
rc, out, err = AnsibleUtil.module.run_command([cmd, name], encoding=None)
|
||||
AnsibleUtil.log_info(idx, 'call `%s %s`: rc=%d, out="%s", err="%s"' % (cmd, name, rc, out, err))
|
||||
if rc != 0:
|
||||
AnsibleUtil.log_error(idx, 'call `%s %s` failed with exit status %d' % (cmd, name, rc))
|
||||
|
||||
AnsibleUtil.run_results_changed(idx)
|
||||
|
||||
|
||||
def run_state_up(self, idx):
|
||||
self._run_state_updown(idx, 'ifup')
|
||||
self._run_state_updown(idx, True)
|
||||
|
||||
def run_state_down(self, idx):
|
||||
self._run_state_updown(idx, 'ifdown')
|
||||
self._run_state_updown(idx, False)
|
||||
|
||||
###############################################################################
|
||||
|
||||
|
|
|
|||
|
|
@ -165,6 +165,7 @@ class TestValidator(unittest.TestCase):
|
|||
{
|
||||
'name': '5',
|
||||
'state': 'up',
|
||||
'force_state_change': None,
|
||||
'wait': None,
|
||||
'ignore_errors': None,
|
||||
}
|
||||
|
|
@ -203,6 +204,7 @@ class TestValidator(unittest.TestCase):
|
|||
'ignore_errors': None,
|
||||
'interface_name': None,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'slave_type': None,
|
||||
'wait': None,
|
||||
'infiniband_p_key': None,
|
||||
|
|
@ -264,6 +266,7 @@ class TestValidator(unittest.TestCase):
|
|||
'state': 'up',
|
||||
'mtu': 1450,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'mac': '52:54:00:44:9f:ba',
|
||||
'master': None,
|
||||
'vlan_id': None,
|
||||
|
|
@ -312,6 +315,7 @@ class TestValidator(unittest.TestCase):
|
|||
'state': 'up',
|
||||
'mtu': 1450,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'mac': '52:54:00:44:9f:ba',
|
||||
'master': None,
|
||||
'vlan_id': None,
|
||||
|
|
@ -349,6 +353,7 @@ class TestValidator(unittest.TestCase):
|
|||
'mac': None,
|
||||
'mtu': None,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'state': 'up',
|
||||
'master': None,
|
||||
'slave_type': None,
|
||||
|
|
@ -404,6 +409,7 @@ class TestValidator(unittest.TestCase):
|
|||
'mac': None,
|
||||
'mtu': None,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'state': 'up',
|
||||
'master': None,
|
||||
'vlan_id': None,
|
||||
|
|
@ -434,6 +440,7 @@ class TestValidator(unittest.TestCase):
|
|||
'mac': None,
|
||||
'mtu': None,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'state': 'up',
|
||||
'master': 'prod2',
|
||||
'vlan_id': None,
|
||||
|
|
@ -488,6 +495,7 @@ class TestValidator(unittest.TestCase):
|
|||
'mac': None,
|
||||
'mtu': None,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'state': 'up',
|
||||
'master': None,
|
||||
'vlan_id': None,
|
||||
|
|
@ -534,6 +542,7 @@ class TestValidator(unittest.TestCase):
|
|||
'mac': None,
|
||||
'mtu': None,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'state': 'up',
|
||||
'master': None,
|
||||
'vlan_id': None,
|
||||
|
|
@ -635,6 +644,7 @@ class TestValidator(unittest.TestCase):
|
|||
'ignore_errors': None,
|
||||
'interface_name': None,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'slave_type': None,
|
||||
'wait': None,
|
||||
'infiniband_p_key': None,
|
||||
|
|
@ -676,6 +686,7 @@ class TestValidator(unittest.TestCase):
|
|||
'ignore_errors': None,
|
||||
'interface_name': None,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'slave_type': None,
|
||||
'wait': None,
|
||||
'infiniband_p_key': None,
|
||||
|
|
@ -719,6 +730,7 @@ class TestValidator(unittest.TestCase):
|
|||
'ignore_errors': None,
|
||||
'interface_name': None,
|
||||
'check_iface_exists': True,
|
||||
'force_state_change': None,
|
||||
'slave_type': None,
|
||||
'wait': None,
|
||||
'infiniband_p_key': None,
|
||||
|
|
|
|||
|
|
@ -19,9 +19,10 @@
|
|||
|
||||
- name: Configure networking connection profiles
|
||||
network_connections:
|
||||
provider: "{{ network_provider | mandatory }}"
|
||||
ignore_errors: "{{ network_ignore_errors | default(omit) }}"
|
||||
connections: "{{ network_connections | default([]) }}"
|
||||
provider: "{{ network_provider | mandatory }}"
|
||||
ignore_errors: "{{ network_ignore_errors | default(omit) }}"
|
||||
force_state_change: "{{ network_force_state_change | default(omit) }}"
|
||||
connections: "{{ network_connections | default([]) }}"
|
||||
|
||||
- name: Re-test connectivity
|
||||
ping:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue