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

include interface info in ansible_interfaces #34395

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
6 participants
@dbckz
Contributor

dbckz commented Jan 3, 2018

SUMMARY

Fixes #30246 by implementing the changes @n-st suggested there

gather_facts currently creates a list ansible_interfaces of all interfaces on a host, and for each interface creates a dict ansible_<interface_name> containing the details of that interface. This PR changes ansible_interfaces to a dict, with interface names for keys, and the corresponding interface details for the value.

The rationale for this change is copied verbatim from #30246:

I propose making the network interface facts available as a dictionary (similar to what's done for disk devices) in the existing ansible_interfaces (which is currently only a list of names).
The dictionary approach is preferable, as it:

  • reduces clutter in the hierarchy root,
  • avoids name collisions (imagine someone named their network interface "dns", for example), and
  • makes it much easier and "cleaner" to access those facts dynamically (see example playbook).

This change can be implemented in a backwards-compatible way due to the sematics of Python iterators:
for x in ["lo", "eth0"] iterates over list items, i.e. lo and eth0
for x in {"lo": {…}, "eth0": {…}} iterates over dictionary keys, i.e. lo and eth0

NOTE: of course the ansible_<interface_name> facts are left untouched for backward compatibility.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

setup module

ANSIBLE VERSION
ansible 2.5.0 (ansible_interfaces 27e058953c) last updated 2018/01/03 13:26:48 (GMT +100)
  config file = None
  configured module search path = [u'/Users/buckleyd/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/buckleyd/projects/github/ansible/lib/ansible
  executable location = /Users/buckleyd/projects/github/ansible/bin/ansible
  python version = 2.7.10 (default, Oct 23 2015, 19:19:21) [GCC 4.2.1 Compatible Apple LLVM 7.0.0 (clang-700.0.59.5)]
ADDITIONAL INFORMATION

Current implementation makes it non-trivial to iterate through the network interfaces and grab e.g. IP information. We can get around this by building an object in-play such as:

- name: Create interfaces dictionary
  set_fact:
    interfaces: "{{ interfaces | default({}) | combine( {item: hostvars[inventory_hostname]['ansible_' + item ]} ) }}"
  with_items: "{{ ansible_interfaces | replace('-', '_') }}"

but this is ugly, and can cost significant time/memory if there are many interfaces.

Before change:

"ansible_interfaces": [
            "awdl0",
            "bridge0",
            ...
        ]

After change:

"ansible_interfaces": {
        "awdl0": {
            "device": "awdl0",
            "flags": [
                "BROADCAST",
                "PROMISC",
                "SIMPLEX",
                "MULTICAST"
            ],
            "ipv4": [],
            "ipv6": [],
            ...
        },
        "bridge0": {
            "device": "bridge0",
            "flags": [
                "UP",
                "BROADCAST",
                "SMART",
                "RUNNING",
                "SIMPLEX",
                "MULTICAST"
            ],
            "ipv4": [],
            "ipv6": [],
            ...
        },
        ...
    }
@ansibot

This comment has been minimized.

Contributor

ansibot commented Jan 3, 2018

The test ansible-test sanity --test pylint [?] failed with the following error:

lib/ansible/module_utils/facts/network/hpux.py:45:12: undefined-variable Undefined variable 'network_fact'

click here for bot help

@dbckz

This comment has been minimized.

Contributor

dbckz commented Jan 3, 2018

There is a concern that this isn't strictly backwards compatible, as using the bare ansible_interfaces fact (i.e. not looping though it) gives different results. Maybe a better approach is to create a new fact e.g. ansible_interfaces_detailed?

@mattclay

This comment has been minimized.

Member

mattclay commented Jan 3, 2018

@mattclay mattclay added the ci_verified label Jan 3, 2018

@jborean93

This comment has been minimized.

Contributor

jborean93 commented Jan 4, 2018

@dbckz you might be best off adding this PR to the agenda ansible/community#291 for the next meeting on IRC to have it discussed further. Considering facts are a core part of Ansible it would require some input from multiple core team members.

@jborean93 jborean93 removed the needs_triage label Jan 4, 2018

@sivel

This comment has been minimized.

Member

sivel commented Jan 9, 2018

@dbckz we discussed in todays core dev meeting, that this will need to be placed in a new key and cannot replace the current key.

However, we won't be able to accept this until we add deprecation functionality for old keys. We will deprecate the old ansible_eth0 style keys, when adding this new fact. @jimi-c will be working on that functionality, as well as performing some fact cleanups for the 2.6 release.

@dbckz

This comment has been minimized.

Contributor

dbckz commented Jan 9, 2018

@sivel great, that makes perfect sense. thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment