Recursive groups lookup

[brandomando]

Hello!

I have a case which I need to be able to see all parent groups, recursively, for a host. I know there is already the has_parent_group method, which is useful if you need to look for one specific group, but this does not satisfy my requirement.
I played around with the code a bit and here is what I came up with as a prototype for doing this:

class Host(InventoryElement):
    __slots__ = ("name", "connections", "defaults")

    ...

    def get_groups_recursive(self) -> List[Group]: 
        recursive_groups = set() 
        recursive_groups.update(set(self.groups)) 
        
        def get_parents(*groups): 
            parents = set() 
            for group in groups: 
                parents.update(set(group.groups)) 
            if parents: 
                recursive_groups.update(set(parents)) 
                get_parents(*parents) 
                
        get_parents(*self.groups)

        return list(recursive_groups)

Is this something that might be useful to others? If so, I'd clean it up and create a PR.

Thanks,

Brandon

[dbarrosop]

Yes, I think it's fine to add. However, implementation-wise I'd suggest a similar implementation to has_parent_group. I think something like this may work:

    def get_groups_recursive(self) -> List[Group]: 
           groups = set(self.groups)
           for group in self.groups:
                  groups.update(set(group.get_groups_recursive()))
           return list(groups)

You can basically handle the recursivity by calling yourself (Group is a subclass of Host), no need for another method. I haven't tested it though so I may be wrong.

Also, this made me realize I am unsure if circular dependencies might be a problem here or somewhere else (two groups having each other as a parent)

[brandomando]

I must have skipped right over the extended_groups method, which accomplishes what I was looking for. I built a test case with a circular dependency and it does throw an error, although I think it would be cryptic if someone didn't already know they had a circular dependency

In [7]: nr.inventory.hosts['test.company.com'].extended_groups()                                                                                                                                                                                         
---------------------------------------------------------------------------
RecursionError                            Traceback (most recent call last)
<ipython-input-7-f50fec3f1b51> in <module>
----> 1 nr.inventory.hosts['test.company.com'].extended_groups()

~/lib/python3.8/site-packages/nornir/core/inventory.py in extended_groups(self)
    196                 groups.append(g)
    197 
--> 198             for sg in g.extended_groups():
    199                 if sg not in groups:
    200                     groups.append(sg)

... last 1 frames repeated, from the frame below ...

~/lib/python3.8/site-packages/nornir/core/inventory.py in extended_groups(self)
    196                 groups.append(g)
    197 
--> 198             for sg in g.extended_groups():
    199                 if sg not in groups:
    200                     groups.append(sg)

RecursionError: maximum recursion depth exceeded in comparison

It might make sense to build in a validation check within the Host class so when the object is initialized or a group is added, a check runs to ensure that it is not adding a circular dependency. I'd have to think a bit on how to implement that though.

[dbarrosop]

I must have skipped right over the extended_groups method

yeah... I also forgot about it....

It might make sense to build in a validation check within the Host class so when the object is initialized or a group is added, a check runs to ensure that it is not adding a circular dependency. I'd have to think a bit on how to implement that though.

I am concerned that it might be expensive cpu-wise for an edge-case, specially when creating large inventories. Given nobody has complained about this ever I am tempted to just let it be and hope the user will search for the error in this repo and find this discussion... (because, that's what users typically do, right? right? :P)

[brandomando]

Valid point. What if within that method, we just wrap it in a `try` and raise a more descriptive error message that suggests there _might_ be circular dependencies?

…

On Thu, Mar 17, 2022, 01:20 David Barroso ***@***.***> wrote: I must have skipped right over the extended_groups method yeah... I also forgot about it.... It might make sense to build in a validation check within the Host class so when the object is initialized or a group is added, a check runs to ensure that it is not adding a circular dependency. I'd have to think a bit on how to implement that though. I am concerned that it might be expensive cpu-wise for an edge-case, specially when creating large inventories. Given nobody has complained about this ever I am tempted to just let it be and hope the user will search for the error in this repo and find this discussion... (because, that's what users typically do, right? right? :P) — Reply to this email directly, view it on GitHub <#788 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI4H3ZUHP677SVXPT4P6IPTVALTLBANCNFSM5QZLYHUA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.*** com>

[dbarrosop]

yeah, that's probably a good compromise