Cobbler Inventory: allow users to specify extra groups #9420

Ty9000 · 2024-12-26T23:53:08Z

SUMMARY

Fix for #9419

Adds all rolled up Cobbler variables for a system to the Ansible inventory

Allows a user to specify multiple extra groups to add to the inventory

Fixed a few typos, simplify imports for required libraries

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

inventory/cobbler.py

ADDITIONAL INFORMATION

Tested on Cobbler 3.4.0

ansibullbot · 2024-12-26T23:54:15Z

cc @opoplawski
click here for bot help

opoplawski · 2024-12-27T20:03:05Z

Thanks for the contribution. I'm not sure though from first glance how extra_groups is meant to be used. Could you give a specific example? Also, it seems to be tied to want_facts also being set, which isn't mentioned in the doc string.

I also think adding the get_system_as_rendered() call is going to add significant overhead - so I think we will want to add a second want_full_facts or some other mechanism (make want_facts accept full?) to turn it on separately. My timing of ansible-inventory --list without a cache went from 2.9s to 16.5s.

Finally, as implemented the full system info is not cached (doubly important with the added overhead) and in facts breaks the second time it is run with a local cache:

'InventoryModule' object has no attribute 'token'

I think you'll want to move the calls to get_system_as_rendered() into _get_systems so you can cache everything there.

I see how it could be useful for some people though, so it definitely is worth adding.

felixfontein · 2024-12-28T12:43:17Z

so I think we will want to add a second want_full_facts or some other mechanism (make want_facts accept full?) to turn it on separately.

want_facts is currently a boolean parameter, changing it to a string with choices is dangerous since it's not fully backwards compatible. (Ansible has some magic that will make it mostly backwards compatible, but probably not fully.)

How about adding a new option that determines the level of facts (maybe facts_level with choices minimal, normal, and as_rendered or something like that?) and that will eventually replace want_facts (whose values would map to two specific choices for the new option)? That would also allow to later add even more levels.

Ty9000 · 2024-12-28T13:08:13Z

For extra_groups, the environment I'm in using Cobbler to declare autoinstall_meta key:value pairs in the distros, profiles, and systems for things like branch, environment (test/ops/etc), system_role, profile_role. Declaring the extra_groups allows ansible-inventory to pull those values of as group membership. We have so many systems (like 10,000+) and independent Cobbler installations that using branch, environment, profile_role really helps organize machines into groups, especially since we truly have no other way to categorize machines

Yeah, adding a new option like facts_level would work! Sorry about that! It's my first foray into this kind of thing.

As for the massive increase in timing, just curious how many systems did you have in the inventory and what version of Cobbler? We're kinda tied down a specific version of Cobbler for now and was curious if updates are increasing/decreasing overhead on API calls

Yeah, that's my bad - I didn't test the caching. The environment I'm in doesn't really work with caching of anything. We have so many systems, profiles, and distros that may change in between ansible-inventory runs...

I'll test some stuff out at work on Monday/Tuesday with the caching, fixes, et cetera and bring those home to merge in

russoz

hi @Ty9000 thanks for your contribution.

The new feature seems to be fine, but I left some comments on some of the other changes made.

russoz · 2024-12-28T23:49:01Z

plugins/inventory/cobbler.py

-                    data = c.get_profiles()
-            except (socket.gaierror, socket.error, xmlrpc_client.ProtocolError):
+                    data = self.c.get_profiles()
+            except (gaierror, error, xmlrpc_client.ProtocolError):


As a personal preference, I think it is clearer what those errors are about when prefixed with socket.. One year from now if I read this code, I would have to go back and forth to the imports to understand what's going on. In this case they are close and it would be very quick, but I think it would be a good practice to be more explicit about it. Just my $0.02

russoz · 2024-12-28T23:50:30Z

changelogs/fragments/9419-inventory-cobbler-extra-vars.yml

@@ -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)


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

russoz · 2024-12-28T23:54:29Z

plugins/inventory/cobbler.py

@@ -180,13 +185,13 @@ 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.c = self._get_connection()


Not sure I like calling the attribute c. I am fine using these shortcuts in very localised contexts, specially for short-lived vars or comprehensions, but when it comes to actual attributes it is a good practice to have meaningful names.

Someone not familiar with the code, one year from now, should be able to read one of the methods below without having to go back and forth within the code to understand what c means. connection? cache? cobbler? compute?

felixfontein · 2025-01-25T09:37:37Z

Ping @Ty9000

needs_info

Ty9000 · 2025-01-26T01:04:06Z

Ping @Ty9000

needs_info

Sorry! I'm on a work trip and won't be back home for a couple more weeks... This is still on my radar and I'll get some updates pushed out mid-February, if that's okay

felixfontein · 2025-01-26T11:42:51Z

No worries, it's mainly to make sure that PRs aren't completely abandoned :) (The bot will auto-close if nobody reacts in 2-3 months if the needs_info label is there.)

ansibullbot · 2025-02-27T12:14:23Z

@Ty9000 This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

plugins/inventory/cobbler.py

opoplawski

There is still more that I don't understand and have questions about but will have to wait until later.

opoplawski · 2025-03-13T20:50:36Z

plugins/inventory/cobbler.py

            interface DNS names to IP addresses.
        type: boolean
        default: true
        version_added: 7.1.0
+      facts_level:


We will want version_added for these as well.

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

opoplawski · 2025-03-13T20:51:36Z

plugins/inventory/cobbler.py

+        default: normal
+      extra_groups:
+        description:
+          - Comma or space-separated string containing extra groups to be added to the Cobbler inventory


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

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

opoplawski · 2025-03-13T20:55:36Z

plugins/inventory/cobbler.py

@@ -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


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

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

opoplawski · 2025-03-13T20:58:34Z

plugins/inventory/cobbler.py

@@ -326,8 +341,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:


Can group really be false here?

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

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

felixfontein · 2025-03-15T06:44:49Z

Plesae note that the PR has a conflict with the latest main brnach.

ansibullbot · 2025-03-15T20:51:16Z

@Ty9000 this PR contains the following merge commits:

3adab6f

Please rebase your branch to remove these commits.

click here for bot help

felixfontein · 2025-03-23T13:44:42Z

changelogs/fragments/9419-inventory-cobbler-extra-vars.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - cobbler inventory plugin - fix typos, 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).


Suggested change

- cobbler inventory plugin - fix typos, 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).

- cobbler inventory plugin - 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, https://github.com/ansible-collections/community.general/pull/9420).

felixfontein · 2025-03-23T13:45:02Z

plugins/inventory/cobbler.py

            interface DNS names to IP addresses.
        type: boolean
        default: true
        version_added: 7.1.0
+      facts_level:


felixfontein · 2025-03-23T13:45:45Z

plugins/inventory/cobbler.py

+          - "normal: Gather only system-level variables"
+          - "as_rendered: Gather all variables as rolled up by Cobbler"


Please use full sentences in descriptions.

Suggested change

- "normal: Gather only system-level variables"

- "as_rendered: Gather all variables as rolled up by Cobbler"

- "Set to V(normal) to gather only system-level variables."

- "Set to V(as_rendered) to gather all variables as rolled up by Cobbler."

felixfontein · 2025-03-23T13:46:18Z

plugins/inventory/cobbler.py

+        description:
+          - Comma or space-separated string containing extra groups to be added to the Cobbler inventory
+          - Requires O(want_facts) to be set to V(true)
+        type: string


Suggested change

type: string

type: list

elements: string

felixfontein · 2025-03-23T13:46:25Z

plugins/inventory/cobbler.py

+          - Comma or space-separated string containing extra groups to be added to the Cobbler inventory
+          - Requires O(want_facts) to be set to V(true)
+        type: string
+        default: ""


Suggested change

default: ""

default: []

ansibullbot · 2025-03-31T13:50:49Z

@Ty9000 You have not responded to information requests in this pullrequest so we will assume it no longer affects you. If you are still interested in this, please create a new pullrequest with the requested information.

click here for bot help

Ty9000 · 2025-04-06T13:42:19Z

Hey all, I added those changes that Felix suggested. I'm going to test this at work tomorrow and then it should be good to go if everyone else if okay with it

felixfontein · 2025-04-06T14:41:26Z

Please note that you need to update version_added to 10.6.0 by now.

Ty9000 · 2025-04-07T19:51:30Z

Okay, I tested this at work and had no issues and had to make no changes to the code

opoplawski · 2025-04-07T23:41:50Z

I have an updated version in #9975 . This should handle caching and cleans up the connection code.

I still don't like the way that extra_groups is described and implemented. It currently creates groups based on items in mgmt_parameters but that isn't clear from the description of the parameter or changeable (though not sure if that would be desired or not).

Ty9000 · 2025-04-10T23:27:21Z

I have an updated version in #9975 . This should handle caching and cleans up the connection code.

Okay, cool. No worries! Should this PR be closed then? Thanks!!

felixfontein · 2025-04-15T18:43:06Z

The other PR is currently marked as WIP; I guess once it's ready and can get merged we can close this one.

opoplawski · 2025-04-24T16:14:41Z

I changed #9975 to just contain your fixes and the as_rendered addition. Thanks for those. If you want to rework this to just add the extra_groups that's fine. But it needs to be documented much better - and if it is going to be limited to sourcing from mgmt_parameters perhaps it should be called mgmt_parameters_extra_group or generalized to be able to create extra groups from multiple keys.

Ty9000 · 2025-05-07T22:23:16Z

Okay, cool. I copied some notes from work today:

Using key = “mgmt_parameters”

This is because Cobbler get_system_as_rendered spits out all vars under this key in a dictionary

Using key = “autoinstall_meta” spits out a str object that isn’t readily parsable, but could be used

Using key = “ks_meta” is a dictionary too, but Cobbler is moving away from that since it sounds like they are trying to become more generic and not use Kickstart terminology

--
Each extra_group in extra_groups contains a list of roles assigned to a distro, profile, or system. We also use it to gather systems by other metavariables, like their locality

i.e. in inventory/cobbler.yml -- extra_groups: [‘profile_role’, 'locality', 'env']
We currently use a large YAML file to create profiles and we add the below to a profile
autoinstall_meta:
profile_role: ‘abc’
This can also be added to a system, too using a different metavariable name
autoinstall_meta:
locality: 'def'
env: 'test'
So when the system is fully rendered, the extra_groups metavariables can be parsed and organize a system by its variables, roles, groups, et cetera

Does that make more sense?

ansibullbot added feature This issue/PR relates to a feature request inventory inventory plugin new_contributor Help guide this first time contributor plugins plugin (any type) labels Dec 26, 2024

Ty9000 changed the title ~~Updated to include extra groups~~ Cobbler Inventory: Pull request to merge changes to allow users to specify extra groups Dec 26, 2024

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Dec 27, 2024

russoz changed the title ~~Cobbler Inventory: Pull request to merge changes to allow users to specify extra groups~~ Cobbler Inventory: allow users to specify extra groups Dec 28, 2024

russoz reviewed Dec 29, 2024

View reviewed changes

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jan 6, 2025

ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Jan 25, 2025

ansibullbot removed the needs_info This issue requires further information. Please answer any outstanding questions label Jan 26, 2025

ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Jan 26, 2025

ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Mar 8, 2025

This comment was marked as outdated.

Sign in to view

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Mar 8, 2025

felixfontein reviewed Mar 8, 2025

View reviewed changes

plugins/inventory/cobbler.py Outdated Show resolved Hide resolved

opoplawski reviewed Mar 13, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

felixfontein reviewed Mar 23, 2025

View reviewed changes

ansibullbot closed this Mar 31, 2025

felixfontein reopened this Apr 6, 2025

ansibullbot removed the needs_info This issue requires further information. Please answer any outstanding questions label Apr 6, 2025

opoplawski mentioned this pull request Apr 7, 2025

cobbler.inventory: Allow collecting fully rendered facts #9975

Merged

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Apr 15, 2025

patchback bot mentioned this pull request Apr 26, 2025

[PR #9975/7524e74b backport][stable-10] cobbler.inventory: Allow collecting fully rendered facts #10073

Merged

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed new_contributor Help guide this first time contributor labels May 2, 2025

Ty9000 closed this May 7, 2025

Ty9000 force-pushed the main branch from bc9aa8a to fcd2d8b Compare May 7, 2025 22:11

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label May 10, 2025

		@@ -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)

	- 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).

	- cobbler inventory plugin - fix typos, 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).
	- cobbler inventory plugin - 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, https://github.com/ansible-collections/community.general/pull/9420).

		- "normal: Gather only system-level variables"
		- "as_rendered: Gather all variables as rolled up by Cobbler"

Cobbler Inventory: allow users to specify extra groups #9420

Cobbler Inventory: allow users to specify extra groups #9420

Uh oh!

Conversation

Ty9000 commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

ansibullbot commented Dec 26, 2024

Uh oh!

opoplawski commented Dec 27, 2024

Uh oh!

felixfontein commented Dec 28, 2024

Uh oh!

Ty9000 commented Dec 28, 2024

Uh oh!

russoz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfontein commented Jan 25, 2025

Uh oh!

Ty9000 commented Jan 26, 2025

Uh oh!

felixfontein commented Jan 26, 2025

Uh oh!

ansibullbot commented Feb 27, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

opoplawski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ty9000 Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfontein commented Mar 15, 2025

Uh oh!

ansibullbot commented Mar 15, 2025

Uh oh!

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ansibullbot commented Mar 31, 2025

Uh oh!

Ty9000 commented Dec 26, 2024 •

edited

Loading

Ty9000 Mar 13, 2025 •

edited

Loading