Skip to content
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

Cobbler Inventory: allow users to specify extra groups #9420

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
2 changes: 2 additions & 0 deletions changelogs/fragments/9419-inventory-cobbler-extra-vars.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
minor_changes:
- inventory/cobbler.py - fix a few typos, simplify required library imports, request full rendered variables for Cobbler system, allow end user to specify extra groups to add systems to in Ansible inventory, should not change currently expected behavior (https://github.com/ansible-collections/community.general/issues/9419)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some required formatting changes, but also a bit of a style suggestion - we could/should be more concise in here.

Suggested change
- inventory/cobbler.py - fix a few typos, simplify required library imports, request full rendered variables for Cobbler system, allow end user to specify extra groups to add systems to in Ansible inventory, should not change currently expected behavior (https://github.com/ansible-collections/community.general/issues/9419)
- cobbler inventory plugin - fix typos, simplify required library imports, request full rendered variables for Cobbler system, allow end user to specify extra groups, should not change currently expected behavior (https://github.com/ansible-collections/community.general/issues/9419).

Please refer to the standard formatting for the changelog fragments' entries:
https://docs.ansible.com/ansible/devel/community/development_process.html#changelogs-how-to-format

The outcome of this changelog fragment can be seen in the collection changelog:
https://docs.ansible.com/ansible/latest/collections/community/general/changelog.html

73 changes: 55 additions & 18 deletions plugins/inventory/cobbler.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
- inventory_cache
options:
plugin:
description: The name of this plugin, it should always be set to V(community.general.cobbler) for this plugin to recognize it as it's own.
description: The name of this plugin, it should always be set to V(community.general.cobbler) for this plugin to recognize it as its own.
type: string
required: true
choices: [ 'cobbler', 'community.general.cobbler' ]
Expand Down Expand Up @@ -95,16 +95,29 @@
description: Prefix to apply to cobbler groups.
default: cobbler_
want_facts:
description: Toggle, if V(true) the plugin will retrieve host facts from the server.
description: Toggle, if V(true) the plugin will retrieve all host facts from the server.
type: boolean
default: true
want_ip_addresses:
description:
- Toggle, if V(true) the plugin will add a C(cobbler_ipv4_addresses) and C(cobbleer_ipv6_addresses) dictionary to the defined O(group) mapping
- Toggle, if V(true) the plugin will add a C(cobbler_ipv4_addresses) and C(cobbler_ipv6_addresses) dictionary to the defined O(group) mapping
interface DNS names to IP addresses.
type: boolean
default: true
version_added: 7.1.0
facts_level:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will want version_added for these as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I can do that - what version number should I put?

description:
- normal - Gather only system-level variables
- as_rendered - Gather all variables as rolled up by Cobbler
type: string
choices: [ 'normal', 'as_rendered' ]
default: normal
extra_groups:
description:
- Comma or space-separated string containing extra groups to be added to the Cobbler inventory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a comma or space-separated string? Why not a list of strings?

Copy link
Author

@Ty9000 Ty9000 Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it can be a list of strings, I'm not tied to anything specific and that portion of the code would need to be modified to read over the list

- Requires O(want_facts) to be set to V(true)
type: string
default: ""
'''

EXAMPLES = '''
Expand All @@ -115,6 +128,7 @@
password: secure
'''

from re import split
import socket

from ansible.errors import AnsibleError
Expand Down Expand Up @@ -144,7 +158,7 @@ class InventoryModule(BaseInventoryPlugin, Cacheable):
def __init__(self):
super(InventoryModule, self).__init__()
self.cache_key = None
self.connection = None
self.cobbler_connection = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get what's going on with cobbler_connection vs local_connection.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was because @russoz wanted the self.c variable renamed to something more sensible, and that is just what I came up with. It can always be changed


def verify_file(self, path):
valid = False
Expand All @@ -159,13 +173,13 @@ def _get_connection(self):
if not HAS_XMLRPC_CLIENT:
raise AnsibleError('Could not import xmlrpc client library')

if self.connection is None:
if self.cobbler_connection is None:
self.display.vvvv(f'Connecting to {self.cobbler_url}\n')
self.connection = xmlrpc_client.Server(self.cobbler_url, allow_none=True)
self.cobbler_connection = xmlrpc_client.Server(self.cobbler_url, allow_none=True)
self.token = None
if self.get_option('user') is not None:
self.token = self.connection.login(text_type(self.get_option('user')), text_type(self.get_option('password')))
return self.connection
self.token = self.cobbler_connection.login(text_type(self.get_option('user')), text_type(self.get_option('password')))
return self.cobbler_connection

def _init_cache(self):
if self.cache_key not in self._cache:
Expand All @@ -180,12 +194,12 @@ def _reload_cache(self):

def _get_profiles(self):
if not self.use_cache or 'profiles' not in self._cache.get(self.cache_key, {}):
c = self._get_connection()
self.local_connection = self._get_connection()
try:
if self.token is not None:
data = c.get_profiles(self.token)
data = self.local_connection.get_profiles(self.token)
else:
data = c.get_profiles()
data = self.local_connection.get_profiles()
except (socket.gaierror, socket.error, xmlrpc_client.ProtocolError):
self._reload_cache()
else:
Expand All @@ -196,12 +210,11 @@ def _get_profiles(self):

def _get_systems(self):
if not self.use_cache or 'systems' not in self._cache.get(self.cache_key, {}):
c = self._get_connection()
try:
if self.token is not None:
data = c.get_systems(self.token)
data = self.local_connection.get_systems(self.token)
else:
data = c.get_systems()
data = self.local_connection.get_systems()
except (socket.gaierror, socket.error, xmlrpc_client.ProtocolError):
self._reload_cache()
else:
Expand Down Expand Up @@ -240,6 +253,7 @@ def parse(self, inventory, loader, path, cache=True):
self.include_profiles = self.get_option('include_profiles')
self.group_by = self.get_option('group_by')
self.inventory_hostname = self.get_option('inventory_hostname')
self.extra_groups = self.get_option('extra_groups')

for profile in self._get_profiles():
if profile['parent']:
Expand Down Expand Up @@ -326,8 +340,9 @@ def parse(self, inventory, loader, path, cache=True):
else:
groups = [host[group_by]] if isinstance(host[group_by], str) else host[group_by]
for group in groups:
group_name = self._add_safe_group_name(group, child=hostname)
self.display.vvvv(f'Added host {hostname} to group_by {group_by} group {group_name}\n')
if group:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can group really be false here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to check on that one at work tomorrow - not sure why I put that in there originally

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I fixed this up - it was because host[group_by] could return a blank/null value or it can return the blank string ''. I added in another check against '' instead of if group

group_name = self._add_safe_group_name(group, child=hostname)
self.display.vvvv(f'Added host {hostname} to group_by {group_by} group {group_name}\n')

# Add to group for this inventory
if self.group is not None:
Expand Down Expand Up @@ -373,9 +388,31 @@ def parse(self, inventory, loader, path, cache=True):
if ipv6_address is not None:
self.inventory.set_variable(hostname, 'cobbler_ipv6_address', make_unsafe(ipv6_address))

if self.get_option('want_facts'):
if self.get_option('facts_level') == "as_rendered":
if self.token is not None:
all_data = self.local_connection.get_system_as_rendered(host['name'], self.token)
else:
all_data = self.local_connection.get_system_as_rendered(host['name'])
key = "mgmt_parameters"
elif self.get_option('facts_level') == "normal":
all_data = host
key = "autoinstall_meta"

# If user requests extra groups to be added to the Cobbler inventory,
if self.extra_groups:

# Gather each extra group, split by ',' or ' '
for extra_group in split(',| ', self.extra_groups):
try:
# Gather each value of the extra group, split by ',' or ' '
# e.g. profile_role: 'test1,test2'
for entry in split(',| ', all_data[key][extra_group]):
self._add_safe_group_name(entry, child=hostname)
except KeyError as e:
self.display.vvvv(f"Could not find {e} value for {hostname}")

try:
self.inventory.set_variable(hostname, 'cobbler', make_unsafe(host))
self.inventory.set_variable(hostname, 'cobbler', make_unsafe(all_data))
except ValueError as e:
self.display.warning(f"Could not set host info for {hostname}: {to_text(e)}")

Expand Down