Repository: nornir-automation/nornir

Issue:
Inheritance Issue
There appears to be an inheritance issue as found by Netigo @ https://nornir.discourse.group/t/inheritance-problem/645

If you recreate the inventory files at https://github.com/nornir-automation/nornir/blob/develop/docs/tutorial/inventory.ipynb

Then try

```
leaf01_bma = nr.inventory.hosts["leaf01.bma"]
print(leaf01_bma["domain"])  
```
The data is supposed to come from the group `global`, but instead comes from defaults.yaml, ie acme.local.  It should be global.local as mentioned in the documentation.

I've attempted a possible patch but not sure how correct it is within the Nornir framework.

In `nornir/core/inventory.py`

Modify the group loop in `_resolve_data` 

    for g in self.groups:
        for k, v in g.data.items():
            if k not in processed:
                processed.append(k)
                result[k] = v

        for pg in g.groups:
            for k, v in pg.data.items():
                if k not in processed:
                    processed.append(k)
                    result[k] = v


Modify the group loop in `__getitem__`

        for g in self.groups:
            try:
                r = g.data[item]
                return r
            except KeyError:
                pass

            for pg in g.groups:
                try:
                    r = pg.data[item]
                    return r
                except KeyError:
                    pass






Discussion:
dbarrosop: @no-such-anthony would you mind taking a look?
no-such-anthony: @dbarrosop  Perfect, and the scenarios I had are all returning the correct inherited values.  

Patch:
diff --git a/docs/tutorial/inventory.ipynb b/docs/tutorial/inventory.ipynb
index b1ec6a8..2721aad 100644
--- a/docs/tutorial/inventory.ipynb
+++ b/docs/tutorial/inventory.ipynb
@@ -877,7 +877,7 @@
     {
      "data": {
       "text/plain": [
-       "'acme.local'"
+       "'global.local'"
       ]
      },
      "execution_count": 13,
diff --git a/nornir/core/inventory.py b/nornir/core/inventory.py
index 9da6cef..0d09b15 100644
--- a/nornir/core/inventory.py
+++ b/nornir/core/inventory.py
@@ -55,11 +55,11 @@ class BaseAttributes(object):
 
     def dict(self) -> Dict[str, Any]:
         return {
-            "hostname": self.hostname,
-            "port": self.port,
-            "username": self.username,
-            "password": self.password,
-            "platform": self.platform,
+            "hostname": object.__getattribute__(self, "hostname"),
+            "port": object.__getattribute__(self, "port"),
+            "username": object.__getattribute__(self, "username"),
+            "password": object.__getattribute__(self, "password"),
+            "platform": object.__getattribute__(self, "platform"),
         }
 
 
@@ -162,6 +162,45 @@ class InventoryElement(BaseAttributes):
             **super().dict(),
         }
 
+    def extended_groups(self) -> List["Group"]:
+        """
+        Returns the groups this host belongs to by virtue of inheritance.
+
+        This list is ordered based on the inheritance rules and groups are not
+        duplicated. For instance, given a host with the following groups:
+
+        hostA:
+            groups:
+                - group_a
+                - group_b
+
+        group_a:
+            groups:
+                - group_1
+                - group_2
+        group_b:
+            groups:
+                - group_2
+                - group_3
+
+        group_1:
+            groups:
+                - group_X
+
+        this will return [group_a, group_1, group_X, group_2, group_b, group_3]
+        """
+        groups: List["Group"] = []
+
+        for g in self.groups:
+            if g not in groups:
+                groups.append(g)
+
+            for sg in g.extended_groups():
+                if sg not in groups:
+                    groups.append(sg)
+
+        return groups
+
 
 class Defaults(BaseAttributes):
     __slots__ = ("data", "connection_options")
@@ -234,14 +273,17 @@ class Host(InventoryElement):
             connection_options=connection_options,
         )
 
-    def _resolve_data(self) -> Dict[str, Any]:
+    def extended_data(self) -> Dict[str, Any]:
+        """
+        Returns the data associated with the object including inherited data
+        """
         processed = []
         result = {}
         for k, v in self.data.items():
             processed.append(k)
             result[k] = v
-        for g in self.groups:
-            for k, v in g.items():
+        for g in self.extended_groups():
+            for k, v in g.data.items():
                 if k not in processed:
                     processed.append(k)
                     result[k] = v
@@ -270,18 +312,18 @@ class Host(InventoryElement):
 
     def keys(self) -> KeysView[str]:
         """Returns the keys of the attribute ``data`` and of the parent(s) groups."""
-        return self._resolve_data().keys()
+        return self.extended_data().keys()
 
     def values(self) -> ValuesView[Any]:
         """Returns the values of the attribute ``data`` and of the parent(s) groups."""
-        return self._resolve_data().values()
+        return self.extended_data().values()
 
     def items(self) -> ItemsView[str, Any]:
         """
         Returns all the data accessible from a device, including
         the one inherited from parent groups
         """
-        return self._resolve_data().items()
+        return self.extended_data().items()
 
     def has_parent_group(self, group: Union[str, "Group"]) -> bool:
         """Returns whether the object is a child of the :obj:`Group` ``group``"""
@@ -308,9 +350,9 @@ class Host(InventoryElement):
             return self.data[item]
 
         except KeyError:
-            for g in self.groups:
+            for g in self.extended_groups():
                 try:
-                    r = g[item]
+                    r = g.data[item]
                     return r
                 except KeyError:
                     continue
@@ -326,8 +368,8 @@ class Host(InventoryElement):
             return object.__getattribute__(self, name)
         v = object.__getattribute__(self, name)
         if v is None:
-            for g in self.groups:
-                r = getattr(g, name)
+            for g in self.extended_groups():
+                r = object.__getattribute__(g, name)
                 if r is not None:
                     return r
 
@@ -342,10 +384,10 @@ class Host(InventoryElement):
         self.data[item] = value
 
     def __len__(self) -> int:
-        return len(self._resolve_data().keys())
+        return len(self.extended_data().keys())
 
     def __iter__(self) -> Iterator[str]:
-        return self.data.__iter__()
+        return self.extended_data().__iter__()
 
     def __str__(self) -> str:
         return self.name


Test patch:
diff --git a/tests/core/test_filter.py b/tests/core/test_filter.py
index 946a697..6851354 100644
--- a/tests/core/test_filter.py
+++ b/tests/core/test_filter.py
@@ -24,7 +24,12 @@ class Test(object):
         f = F(site="site2") | (F(role="www") & F(my_var="comes_from_dev1.group_1"))
         filtered = sorted(list((nornir.inventory.filter(f).hosts.keys())))
 
-        assert filtered == ["dev1.group_1", "dev3.group_2", "dev4.group_2"]
+        assert filtered == [
+            "dev1.group_1",
+            "dev3.group_2",
+            "dev4.group_2",
+            "dev6.group_3",
+        ]
 
         f = (F(site="site2") | F(role="www")) & F(my_var="comes_from_dev1.group_1")
         filtered = sorted(list((nornir.inventory.filter(f).hosts.keys())))
@@ -41,7 +46,12 @@ class Test(object):
         f = ~F(groups__contains="group_1")
         filtered = sorted(list((nornir.inventory.filter(f).hosts.keys())))
 
-        assert filtered == ["dev3.group_2", "dev4.group_2", "dev5.no_group"]
+        assert filtered == [
+            "dev3.group_2",
+            "dev4.group_2",
+            "dev5.no_group",
+            "dev6.group_3",
+        ]
 
     def test_negate_and_second_negate(self, nornir):
         f = F(site="site1") & ~F(role="www")
@@ -58,6 +68,7 @@ class Test(object):
             "dev3.group_2",
             "dev4.group_2",
             "dev5.no_group",
+            "dev6.group_3",
         ]
 
     def test_nested_data_a_string(self, nornir):
@@ -93,6 +104,7 @@ class Test(object):
             "dev3.group_2",
             "dev4.group_2",
             "dev5.no_group",
+            "dev6.group_3",
         ]
 
     def test_nested_data_a_list_contains(self, nornir):
@@ -105,7 +117,12 @@ class Test(object):
         f = F(has_parent_group="parent_group")
         filtered = sorted(list((nornir.inventory.filter(f).hosts.keys())))
 
-        assert filtered == ["dev1.group_1", "dev2.group_1", "dev4.group_2"]
+        assert filtered == [
+            "dev1.group_1",
+            "dev2.group_1",
+            "dev4.group_2",
+            "dev6.group_3",
+        ]
 
     def test_filtering_by_attribute_name(self, nornir):
         f = F(name="dev1.group_1")
@@ -117,7 +134,12 @@ class Test(object):
         f = F(platform__in=["linux", "mock"])
         filtered = sorted(list((nornir.inventory.filter(f).hosts.keys())))
 
-        assert filtered == ["dev3.group_2", "dev4.group_2", "dev5.no_group"]
+        assert filtered == [
+            "dev3.group_2",
+            "dev4.group_2",
+            "dev5.no_group",
+            "dev6.group_3",
+        ]
 
     def test_filtering_list_any(self, nornir):
         f = F(nested_data__a_list__any=[1, 3])
diff --git a/tests/core/test_inventory.py b/tests/core/test_inventory.py
index 2f89dcd..e360edb 100644
--- a/tests/core/test_inventory.py
+++ b/tests/core/test_inventory.py
@@ -100,9 +100,9 @@ class Test(object):
                     "hostname": None,
                     "name": "group_1",
                     "password": "from_group1",
-                    "platform": "linux",
+                    "platform": None,
                     "port": None,
-                    "username": "root",
+                    "username": None,
                 },
                 "group_2": {
                     "connection_options": {},
@@ -110,21 +110,21 @@ class Test(object):
                     "groups": [],
                     "hostname": None,
                     "name": "group_2",
-                    "password": "docker",
-                    "platform": "linux",
+                    "password": None,
+                    "platform": None,
                     "port": None,
-                    "username": "root",
+                    "username": None,
                 },
                 "group_3": {
                     "connection_options": {},
                     "data": {"site": "site2"},
-                    "groups": [],
+                    "groups": ["dummy_group", "parent_group"],
                     "hostname": None,
                     "name": "group_3",
-                    "password": "docker",
-                    "platform": "linux",
+                    "password": None,
+                    "platform": None,
                     "port": None,
-                    "username": "root",
+                    "username": None,
                 },
                 "parent_group": {
                     "connection_options": {
@@ -145,14 +145,29 @@ class Test(object):
                             "username": None,
                         },
                     },
-                    "data": {"a_false_var": False, "a_var": "blah"},
+                    "data": {
+                        "a_false_var": False,
+                        "a_var": "blah",
+                        "my_var": "comes_from_parent_group",
+                    },
                     "groups": [],
                     "hostname": None,
                     "name": "parent_group",
                     "password": "from_parent_group",
-                    "platform": "linux",
+                    "platform": None,
                     "port": None,
-                    "username": "root",
+                    "username": None,
+                },
+                "dummy_group": {
+                    "connection_options": {},
+                    "data": {},
+                    "groups": [],
+                    "hostname": None,
+                    "name": "dummy_group",
+                    "password": None,
+                    "platform": None,
+                    "port": None,
+                    "username": None,
                 },
             },
             "hosts": {
@@ -191,7 +206,7 @@ class Test(object):
                     "password": "a_password",
                     "platform": "eos",
                     "port": 65020,
-                    "username": "root",
+                    "username": None,
                 },
                 "dev2.group_1": {
                     "connection_options": {
@@ -223,10 +238,10 @@ class Test(object):
                     "groups": ["group_1"],
                     "hostname": "localhost",
                     "name": "dev2.group_1",
-                    "password": "from_group1",
+                    "password": None,
                     "platform": "junos",
                     "port": 65021,
-                    "username": "root",
+                    "username": None,
                 },
                 "dev3.group_2": {
                     "connection_options": {
@@ -243,10 +258,10 @@ class Test(object):
                     "groups": ["group_2"],
                     "hostname": "localhost",
                     "name": "dev3.group_2",
-                    "password": "docker",
+                    "password": None,
                     "platform": "linux",
                     "port": 65022,
-                    "username": "root",
+                    "username": None,
                 },
                 "dev4.group_2": {
                     "connection_options": {
@@ -271,10 +286,10 @@ class Test(object):
                     "groups": ["parent_group", "group_2"],
                     "hostname": "localhost",
                     "name": "dev4.group_2",
-                    "password": "from_parent_group",
+                    "password": None,
                     "platform": "linux",
                     "port": 65023,
-                    "username": "root",
+                    "username": None,
                 },
                 "dev5.no_group": {
                     "connection_options": {},
@@ -282,13 +297,74 @@ class Test(object):
                     "groups": [],
                     "hostname": "localhost",
                     "name": "dev5.no_group",
-                    "password": "docker",
+                    "password": None,
                     "platform": "linux",
                     "port": 65024,
-                    "username": "root",
+                    "username": None,
                 },
+                "dev6.group_3": {
+                    "connection_options": {},
+                    "data": {"asd": 1},
+                    "groups": ["group_3"],
+                    "hostname": "localhost",
+                    "name": "dev6.group_3",
+                    "password": None,
+                    "platform": "linux",
+                    "port": 65025,
+                    "username": None,
+                },
+            },
+        }
+
+    def test_extended_data(self, inv):
+        assert inv.hosts["dev1.group_1"].extended_data() == {
+            "a_false_var": False,
+            "a_var": "blah",
+            "my_var": "comes_from_dev1.group_1",
+            "nested_data": {
+                "a_dict": {"a": 1, "b": 2},
+                "a_list": [1, 2],
+                "a_string": "asdasd",
             },
+            "only_default": "only_defined_in_default",
+            "role": "www",
+            "site": "site1",
+            "www_server": "nginx",
         }
+        assert inv.hosts["dev3.group_2"].extended_data() == {
+            "my_var": "comes_from_defaults",
+            "only_default": "only_defined_in_default",
+            "role": "www",
+            "site": "site2",
+            "www_server": "apache",
+        }
+        assert inv.hosts["dev5.no_group"].extended_data() == {
+            "my_var": "comes_from_defaults",
+            "only_default": "only_defined_in_default",
+        }
+        assert inv.hosts["dev6.group_3"].extended_data() == {
+            "a_false_var": False,
+            "a_var": "blah",
+            "asd": 1,
+            "my_var": "comes_from_parent_group",
+            "only_default": "only_defined_in_default",
+            "site": "site2",
+        }
+
+    def test_parent_groups_extended(self, inv):
+        assert inv.hosts["dev1.group_1"].extended_groups() == [
+            inv.groups["group_1"],
+            inv.groups["parent_group"],
+        ]
+        assert inv.hosts["dev3.group_2"].extended_groups() == [
+            inv.groups["group_2"],
+        ]
+        assert inv.hosts["dev5.no_group"].extended_groups() == []
+        assert inv.hosts["dev6.group_3"].extended_groups() == [
+            inv.groups["group_3"],
+            inv.groups["dummy_group"],
+            inv.groups["parent_group"],
+        ]
 
     def test_filtering(self, inv):
         unfiltered = sorted(list(inv.hosts.keys()))
@@ -298,6 +374,7 @@ class Test(object):
             "dev3.group_2",
             "dev4.group_2",
             "dev5.no_group",
+            "dev6.group_3",
         ]
 
         www = sorted(list(inv.filter(role="www").hosts.keys()))
@@ -315,7 +392,7 @@ class Test(object):
         long_names = sorted(
             list(inv.filter(filter_func=lambda x: len(x["my_var"]) > 20).hosts.keys())
         )
-        assert long_names == ["dev1.group_1", "dev4.group_2"]
+        assert long_names == ["dev1.group_1", "dev4.group_2", "dev6.group_3"]
 
         def longer_than(dev, length):
             return len(dev["my_var"]) > length
@@ -323,7 +400,7 @@ class Test(object):
         long_names = sorted(
             list(inv.filter(filter_func=longer_than, length=20).hosts.keys())
         )
-        assert long_names == ["dev1.group_1", "dev4.group_2"]
+        assert long_names == ["dev1.group_1", "dev4.group_2", "dev6.group_3"]
 
     def test_filter_unique_keys(self, inv):
         filtered = sorted(list(inv.filter(www_server="nginx").hosts.keys()))
@@ -334,6 +411,8 @@ class Test(object):
         assert inv.hosts["dev2.group_1"]["my_var"] == "comes_from_group_1"
         assert inv.hosts["dev3.group_2"]["my_var"] == "comes_from_defaults"
         assert inv.hosts["dev4.group_2"]["my_var"] == "comes_from_dev4.group_2"
+        assert inv.hosts["dev5.no_group"]["my_var"] == "comes_from_defaults"
+        assert inv.hosts["dev6.group_3"]["my_var"] == "comes_from_parent_group"
         assert inv.hosts["dev1.group_1"]["a_false_var"] is False
 
         assert inv.hosts["dev1.group_1"].data["my_var"] == "comes_from_dev1.group_1"
@@ -349,6 +428,7 @@ class Test(object):
         assert inv.hosts["dev3.group_2"].password == "docker"
         assert inv.hosts["dev4.group_2"].password == "from_parent_group"
         assert inv.hosts["dev5.no_group"].password == "docker"
+        assert inv.hosts["dev6.group_3"].password == "from_parent_group"
 
     def test_has_parents(self, inv):
         assert inv.hosts["dev1.group_1"].has_parent_group(inv.groups["group_1"])
@@ -399,6 +479,7 @@ class Test(object):
             inv.hosts["dev1.group_1"],
             inv.hosts["dev2.group_1"],
             inv.hosts["dev4.group_2"],
+            inv.hosts["dev6.group_3"],
         }
 
         assert inv.children_of_group("group_1") == {
@@ -418,6 +499,7 @@ class Test(object):
             inv.hosts["dev1.group_1"],
             inv.hosts["dev2.group_1"],
             inv.hosts["dev4.group_2"],
+            inv.hosts["dev6.group_3"],
         }
 
         assert inv.children_of_group(inv.groups["group_1"]) == {
diff --git a/tests/core/test_processors.py b/tests/core/test_processors.py
index bb82082..5bede97 100644
--- a/tests/core/test_processors.py
+++ b/tests/core/test_processors.py
@@ -104,6 +104,12 @@ class Test:
                     "completed": True,
                     "failed": False,
                 },
+                "dev6.group_3": {
+                    "completed": True,
+                    "failed": False,
+                    "started": True,
+                    "subtasks": {},
+                },
                 "completed": True,
             }
         }
@@ -231,6 +237,32 @@ class Test:
                     "completed": True,
                     "failed": False,
                 },
+                "dev6.group_3": {
+                    "started": True,
+                    "subtasks": {
+                        "mock_task": {
+                            "started": True,
+                            "subtasks": {},
+                            "completed": True,
+                            "failed": False,
+                        },
+                        "mock_subsubtask": {
+                            "started": True,
+                            "subtasks": {
+                                "mock_task": {
+                                    "started": True,
+                                    "subtasks": {},
+                                    "completed": True,
+                                    "failed": False,
+                                }
+                            },
+                            "completed": True,
+                            "failed": False,
+                        },
+                    },
+                    "completed": True,
+                    "failed": False,
+                },
                 "completed": True,
             }
         }
diff --git a/tests/inventory_data/groups.yaml b/tests/inventory_data/groups.yaml
index bf996b1..e3b3e86 100644
--- a/tests/inventory_data/groups.yaml
+++ b/tests/inventory_data/groups.yaml
@@ -1,4 +1,5 @@
 ---
+dummy_group: {}
 parent_group:
     port:
     hostname:
@@ -6,6 +7,7 @@ parent_group:
     password: from_parent_group
     platform:
     data:
+        my_var: comes_from_parent_group
         a_var: blah
         a_false_var: false
     groups: []
@@ -54,7 +56,9 @@ group_3:
     username:
     password:
     platform:
-    groups: []
+    groups:
+    - dummy_group
+    - parent_group
     data:
         site: site2
     connection_options: {}
diff --git a/tests/inventory_data/hosts.yaml b/tests/inventory_data/hosts.yaml
index 8f11e47..1e2129d 100644
--- a/tests/inventory_data/hosts.yaml
+++ b/tests/inventory_data/hosts.yaml
@@ -119,3 +119,14 @@ dev5.no_group:
     data: {}
     groups: []
     connection_options: {}
+dev6.group_3:
+    port: 65025
+    hostname: localhost
+    username:
+    password:
+    platform: linux
+    data:
+        asd: 1
+    groups:
+    - group_3
+    connection_options: {}
diff --git a/tests/plugins/inventory/test_simple_inventory.py b/tests/plugins/inventory/test_simple_inventory.py
index cbb6cf1..27040dd 100644
--- a/tests/plugins/inventory/test_simple_inventory.py
+++ b/tests/plugins/inventory/test_simple_inventory.py
@@ -42,9 +42,9 @@ class Test:
                     "hostname": None,
                     "name": "group_1",
                     "password": "from_group1",
-                    "platform": "linux",
+                    "platform": None,
                     "port": None,
-                    "username": "root",
+                    "username": None,
                 },
                 "group_2": {
                     "connection_options": {},
@@ -52,10 +52,10 @@ class Test:
                     "groups": [],
                     "hostname": None,
                     "name": "group_2",
-                    "password": "docker",
-                    "platform": "linux",
+                    "password": None,
+                    "platform": None,
                     "port": None,
-                    "username": "root",
+                    "username": None,
                 },
                 "group_3": {
                     "connection_options": {},
@@ -63,10 +63,10 @@ class Test:
                     "groups": [],
                     "hostname": None,
                     "name": "group_3",
-                    "password": "docker",
-                    "platform": "linux",
+                    "password": None,
+                    "platform": None,
                     "port": None,
-                    "username": "root",
+                    "username": None,
                 },
                 "parent_group": {
                     "connection_options": {
@@ -92,9 +92,9 @@ class Test:
                     "hostname": None,
                     "name": "parent_group",
                     "password": "from_parent_group",
-                    "platform": "linux",
+                    "platform": None,
                     "port": None,
-                    "username": "root",
+                    "username": None,
                 },
             },
             "hosts": {
@@ -133,7 +133,7 @@ class Test:
                     "password": "a_password",
                     "platform": "eos",
                     "port": 65020,
-                    "username": "root",
+                    "username": None,
                 },
                 "dev2.group_1": {
                     "connection_options": {
@@ -165,10 +165,10 @@ class Test:
                     "groups": ["group_1"],
                     "hostname": "localhost",
                     "name": "dev2.group_1",
-                    "password": "from_group1",
+                    "password": None,
                     "platform": "junos",
                     "port": 65021,
-                    "username": "root",
+                    "username": None,
                 },
                 "dev3.group_2": {
                     "connection_options": {
@@ -185,10 +185,10 @@ class Test:
                     "groups": ["group_2"],
                     "hostname": "localhost",
                     "name": "dev3.group_2",
-                    "password": "docker",
+                    "password": None,
                     "platform": "linux",
                     "port": 65022,
-                    "username": "root",
+                    "username": None,
                 },
                 "dev4.group_2": {
                     "connection_options": {
@@ -213,10 +213,10 @@ class Test:
                     "groups": ["parent_group", "group_2"],
                     "hostname": "localhost",
                     "name": "dev4.group_2",
-                    "password": "from_parent_group",
+                    "password": None,
                     "platform": "linux",
                     "port": 65023,
-                    "username": "root",
+                    "username": None,
                 },
                 "dev5.no_group": {
                     "connection_options": {},
@@ -224,10 +224,10 @@ class Test:
                     "groups": [],
                     "hostname": "localhost",
                     "name": "dev5.no_group",
-                    "password": "docker",
+                    "password": None,
                     "platform": "linux",
                     "port": 65024,
-                    "username": "root",
+                    "username": None,
                 },
             },
         }
diff --git a/tests/plugins/processors/test_serial.py b/tests/plugins/processors/test_serial.py
index 5b00687..c21a9da 100644
--- a/tests/plugins/processors/test_serial.py
+++ b/tests/plugins/processors/test_serial.py
@@ -31,7 +31,7 @@ class TestSerialRunner(object):
         nornir.with_runner(SerialRunner()).run(blocking_task, wait=0.5)
         t2 = datetime.datetime.now()
         delta = t2 - t1
-        assert delta.seconds == 2, delta
+        assert delta.seconds == 3, delta
 
     def test_failing_task_simple_singlethread(self, nornir):
         result = nornir.with_runner(SerialRunner()).run(failing_task_simple)