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

Group membership filter #149

Closed
wants to merge 5 commits into from

Conversation

ktbyers
Copy link
Collaborator

@ktbyers ktbyers commented May 24, 2018

Here is the code I used for initial testing against the changes specified in the PR.

from nornir.core import InitNornir
from nornir.plugins.tasks.networking import netmiko_send_command
from nornir.plugins.functions.text import print_result
from nornir.core.inventory import Inventory

norn = InitNornir()
junos_hosts = norn.filter(group_member='juniper')

result = junos_hosts.run(
     netmiko_send_command,
     num_workers=60,
     command_string="show interfaces terse"
)
 
print_result(result)

The hosts.yaml entry intentionally has multiple groups so you can't just do:

junos_hosts = norn.filter(groups=['juniper'])

I think this is probably a common enough pattern (host belonging to multiple groups) and wanting to filter by group that we probably should have some pretty simple solution for it.

@coveralls
Copy link

coveralls commented May 25, 2018

Pull Request Test Coverage Report for Build 685

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 92.809%

Totals Coverage Status
Change from base Build 683: 0.03%
Covered Lines: 955
Relevant Lines: 1029

💛 - Coveralls

@coveralls
Copy link

coveralls commented May 25, 2018

Pull Request Test Coverage Report for Build 684

  • 2 of 4 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 92.614%

Changes Missing Coverage Covered Lines Changed/Added Lines %
nornir/core/inventory.py 2 4 50.0%
Totals Coverage Status
Change from base Build 683: -0.2%
Covered Lines: 953
Relevant Lines: 1029

💛 - Coveralls

@ktbyers ktbyers changed the title Do Not Merge: Group membership (for discussion) Group membership filter May 31, 2018
@dbarrosop
Copy link
Contributor

dbarrosop commented Jun 3, 2018

So my only concern with this method is that we are using a "special keyword" as an argument and that might mask a legit attribute in someone's inventory. We do that already with filter_func (although chances of anyone naming anything filter_func are probably low) and I'd rather try avoiding this.

So the Host class already has has_parent_group (function) and groups (property) to help figuring out the hierarchy. So you can do:

my_host.has_parent_group("routers")

What I was thinking is adding to the Group class the functions get_hosts and filter that returns the hosts that belong to group a properly filtered nornir object. Something like:

my_group.get_hosts(_recursive=True) # returns lists of hosts
my_group.filter(_recursive=True) # returns nr object with filtered hosts, might receive **kwargs

# or

nr.inventory.groups["my_group"].get_hosts(recursive=True)
nr.inventory.groups["my_group"].filter(_recursive=True)

_recursive could allow us to return only directly "attached" hosts (False) or also resolve from subgroups (True).

Alternatively we could implement a more generic approach and have some syntax for allowing "queries" (we could get inspiration from django for this). For instance:

nr.inventory.filter(__group__has__="blah")  # or similar

That advantage of this method is that it is a generic solution that will work for user data as well and that could be expanded to check dictionary values. For instance:

# inventory_data: mydict: {"mykey": "bleh", "nestedlist": [1, 2, 3]}
nr.inventory.filter(__mydict__mykey__="bleh") # check value of mydict["mykey"]
nr.inventory.filter(__mydict__has__="mykey") # check mydict has key "mykey" regardless of value
nr.inventory.filter(__mydict__nestedlist__has__=1) # is 1 inside mydict["nestedlist"]?
nr.inventory.filter(__mydict__nestedlist__=[1, 2 3]) # exact match on the nested list

Thoughts?

@ogenstad
Copy link
Collaborator

ogenstad commented Jun 4, 2018

I had the same thought about introducing more reserved keys. Another one is what would be default filtering methods.

If I want to have all hosts that are a member of group A but not of B. Is that something that should be possible by default, or do we require a user-defined function for that?

An option could be that someone would have to run several filters and that you can also negate matches.

my_group.get_hosts(_recursive=True, _match=False) # returns lists of hosts

I think that the examples from Django might be confusing for people...

@dbarrosop
Copy link
Contributor

dbarrosop commented Jun 4, 2018

I think that the examples from Django might be confusing for people...

But it's the only way of fixing it in a generic way for nested structures. It also has the advantage you can combine them:

nr.inventory.filter(__mydict__mykey__="bleh", __group__has__="myrouters")

And you could even easily add a __not__ keyword:

nr.inventory.filter(__mydict__mykey__="bleh", __group__not__has__="myrouters")

I think it's not as complex to be honest and with proper documentation and plenty of examples it should be easy. We could also add allow the + operator between nornir objects to implement the "OR" functionality:

bleh_and_not_myrouters = nr.inventory.filter(__mydict__mykey__="bleh", __group__not__has__="myrouters")
bleh_or_not_myrouters = nr.inventory.filter(__mydict__mykey__="bleh") + nr.inventory.filter(__group__not__has__="myrouters")

@ktbyers
Copy link
Collaborator Author

ktbyers commented Jun 4, 2018

I vote for the Django pattern. I like that better and think it is simpler/easier to understand than the other solution.

The fundamental question for the Django pattern is are the enough filtering use cases to justify this or is group_membership just a special one off. I am pretty sure there are enough use cases to justify it (given what @dbarrosop demonstrated and also Patricks multiple group operation case--member group A, but not group B).

@dbarrosop
Copy link
Contributor

I think there is, mostly because we don't control user's data. For instance, when I implemented nsot plugin I had to do this:

https://github.com/nornir-automation/nornir/blob/develop/nornir/plugins/inventory/nsot.py#L73

So I could use nsot data to filter the objects. I had to do something similar at $dayjob because my data there is also heavily nested.

@ktbyers
Copy link
Collaborator Author

ktbyers commented Jun 14, 2018

Closing as we will be using a different, more generic solution probably Django QuerySet style queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants