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

[WIP] remove inter-object references from Host and Group objects #58289

Open
wants to merge 7 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@s-hertel
Copy link
Contributor

commented Jun 24, 2019

SUMMARY

Fixes #42594

WIP as it needs better compatibility + deprecation warnings for 3rd party strategy plugins

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

Inventory

@ansibot

This comment was marked as resolved.

Copy link
Contributor

commented Jun 24, 2019

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

lib/ansible/inventory/data.py:187:112: undefined-variable Undefined variable 'to_native'
lib/ansible/inventory/data.py:187:130: undefined-variable Undefined variable 'to_native'
lib/ansible/inventory/data.py:228:93: undefined-variable Undefined variable 'to_native'
lib/ansible/inventory/manager.py:267:23: singleton-comparison Comparison to None should be 'expr is None'

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/inventory_data/aliases:0:0: missing alias `shippable/posix/group[1-4]` or `unsupported`

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/cli/inventory.py:359:38: E261 at least two spaces before inline comment
lib/ansible/inventory/manager.py:267:39: E711 comparison to None should be 'if cond is None:'

The test ansible-test sanity --test yamllint [explain] failed with 10 errors:

test/integration/targets/inventory_data/groups_of_groups/a.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'
test/integration/targets/inventory_data/groups_of_groups/b.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'
test/integration/targets/inventory_data/one_valid_and_invalid_source/a_parses.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'
test/integration/targets/inventory_data/one_valid_and_invalid_source/b_fails_before_partial_parsing.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'
test/integration/targets/inventory_data/partially_bad_source/a_parses_first_and_fails.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'
test/integration/targets/inventory_data/partially_bad_source/b_parses_second_and_succeeds.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'
test/integration/targets/inventory_data/partially_bad_source_test_remove_var/a_succeeds.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'
test/integration/targets/inventory_data/partially_bad_source_test_remove_var/b_adds_var_then_fails.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'
test/integration/targets/inventory_data/source_with_duplication/a.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'
test/integration/targets/inventory_data/source_with_duplication/b.yml:2:1: error syntax error: expected '<document start>', but found '<scalar>'

click here for bot help

@s-hertel s-hertel force-pushed the s-hertel:restorable_inventory branch from c5abb79 to acf8f67 Jun 24, 2019

s-hertel added some commits Jun 24, 2019

@s-hertel s-hertel force-pushed the s-hertel:restorable_inventory branch from acf8f67 to e678db2 Jun 24, 2019

host = self.hosts[host_name]
if additions is None:
for group_name in host.groups:
for ancestor_name in self.group_get_ancestors(group_name):

This comment has been minimized.

Copy link
@AlanCoding

AlanCoding Jun 24, 2019

Member

It looks like group_get_ancestors isn't defined in this class but in the group manager class? I'm not sure what the more elegant way to reference one from the other is.

:type host_name: str
:type group_name: str
"""
self.host_populate_ancestors(host_name)

This comment has been minimized.

Copy link
@AlanCoding

AlanCoding Jun 24, 2019

Member

I have always thought that rebuilding ancestors when adding an object seemed like something that could be easily avoided. Additions happen by the inventory plugin for the most part. Between adding one host and the next host, a correct list of ancestors is almost never needed. Inventory plugins add parents, but they don't generally read ancestors. So my preferred solution would be to have a lazily evaluated groups property on hosts.

I also have a hard time understanding the function of this method. It I, as an inventory plugin author, call this, I would be doing it wrong. It seems that this is only ever used correctly if called while in the process of adding the host to the group via the add_host method or something like that. So this isn't really adding the host to the group, but it is maintaining synchronization between links from groups to hosts and from hosts to ancestors (which are called "groups").

This comment has been minimized.

Copy link
@s-hertel

s-hertel Jun 24, 2019

Author Contributor

You may be right, I'll look into it. For the time being I just tried to keep parity of behavior and ancestor evaluation. I do feel like InventoryData is overly complicated and that there are probably simplifications I'm overlooking.

Regarding the naming, the methods I took from the host file I prefixed with host_, those from group.py group_. This was just to help me remember which were from where. Maybe I should give these clearer names (something along the lines of add_group_rel_to_host?) and move them into InventoryData? I'm not sure where they make the most sense aesthetically (I feel like these are overloading InventoryData if I move them there directly but I think this now is inelegant), but you're right that they shouldn't be advertised or at least be more clear what their purpose is for plugin authors.

This comment has been minimized.

Copy link
@AlanCoding

AlanCoding Jun 24, 2019

Member

(I see now where I was wrong, these are both base classes of InventoryData, so that's how it has access to both methods)

In terms of big picture, when @bcoca spoke of this idea previously, my take-away was that many things that used to live in the host or group model would move to the Inventory or InventoryData model.

I like that idea, because keeping track of relationships in inventory-level objects makes it a lot easier to follow the save-point logic you're doing.

You're doing some of that here, by migrating the methods so far. The thing that strikes me is that the data are still maintained on the Host and Group objects. My prior reading of this proposal was that the relationships themselves (of what is a child of what) would move to the InventoryData as well. Then if someone referenced host.groups, it would have to obtain this from a backlink to the InventoryData object.

I found that to be a compelling idea because the plugins tend to operate on the inventory-scope objects, but that's still very wishy washy. For something more concrete, introducing an InventoryDataCache class or something to differentiate computed data versus the original data from the loading process could be very productive (computed data being the indirect ancestors in specific). Along the lines of what you said, reducing the methods available on InventoryData would be the most maintainable IMHO because that's what the plugin authors touch.

@@ -260,6 +263,10 @@ def parse_source(self, source, cache=False):
plugin_wants = False

if plugin_wants:
# No need to deepcopy inventory unless it has changed
if inventory_copy is None:
inventory_copy = deepcopy(self._inventory)

This comment has been minimized.

Copy link
@AlanCoding

AlanCoding Jun 24, 2019

Member

As a note to myself and @chrismeyersfsu, this is a motivator for me want to check running this with multi-10s of thousands of hosts.

This comment has been minimized.

Copy link
@s-hertel

s-hertel Jun 24, 2019

Author Contributor

Yeah, this line was the cause of decoupling the objects because it was not scaling well. I generated a mock inventory directory (20 files, 10,000 hosts a file, failing from invalid syntax at the very end of every other file to test correct inventory restoration) and added a cgroup profiler to the inventory manager. I'm still looking for optimizations, so let me know your findings/ideas when you test.

@s-hertel s-hertel removed the needs_triage label Jun 25, 2019

@s-hertel s-hertel force-pushed the s-hertel:restorable_inventory branch 3 times, most recently from 13e7b95 to e1beac4 Jun 25, 2019

@s-hertel s-hertel force-pushed the s-hertel:restorable_inventory branch from e1beac4 to 0209933 Jun 25, 2019

@ansibot ansibot added the stale_ci label Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.