Items: Refactor base classes

[SchoolGuy]

Linked Items

Fixes #3439

Description

The new item type "Templates" that will be introduced with #3295 doesn't need to have parents, children or things like autoinstall_meta. As such before we add this new item type we need to find a better base class that will then be used for the new item type.

Steps for this PR:

1. Make NetworkInterface live in its own module
2. Introduce BaseItem
3. Introduce Item
4. Introduce InheritableItem
5. Make NetworkInterface a dedicated collection (with data migration)
6. Introduce BootLoader type
7. Introduce VirtOptions
8. Introduce PowerOptions
9. Introduce IPv4Options & IPv6Options

Each step should have a green CI so each commit can be individually validated.

Behaviour changes

Old:

New:

Category

This is related to a:

• Bugfix
• Feature
• Packaging
• Docs
• Code Quality
• Refactoring
• Miscellaneous

Tests

• Unit-Tests were created
• System-Tests were created
• Code is already covered by Unit-Tests
• Code is already covered by System-Tests
• No tests required

[SchoolGuy]

After #3252 and #3461 are merged I can finally work on this.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.03% (target: -1.00%)	✅ 89.69% (target: 70.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (69889af)	15525	10242	65.97%
Head commit (b8be7af)	15653 (+128)	10331 (+89)	66.00% (+0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3440)	359	322	89.69%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

[SchoolGuy]

@tpw56j I am struggling a bit with the implementation of your cache for the last day. To my understanding of the history of Cobbler, the object type Repo doesn't have descendants but you tested for it in your tests. Could you please give me a bit of context for this?

[tpw56j]

@SchoolGuy In implementing caching for proper object cache invalidation, the main role is not logical inheritance, but how one type depends on another. Dependency rules are defined in the TYPE_DEPENDENCIES dict. For Repo, the directly dependent type is Profile.
Item.descendants() builds a dependency tree along which caches of dependent objects are invalidated. This tree is not a logical inheritance tree, which is built according to the LOGICAL_INHERITANCE dict.

In addition, since dependency rules are known only through TYPE_DEPENDENCIES, for reliable testing that we will not get incorrect results from the cache, a complete set of all possible Cobbler objects is required.

[SchoolGuy]

@tpw56j Ahhhh thanks now I see where my logic error is. Then my assumption that Repo has no parents and children was correct but I forgot that descendants also covers arbitrary references. I'll try to change the base class and see if the results improve.

[SchoolGuy]

The test was successful. However, that means that there never can be an item type that has a reference to another item type that is not inheritable. I would very much like to remove the "parent"/"children" logic from objects like Repo (and also other item types that are not there yet) but it seems that the current implementation makes that impossible. If you would have a spontaneous idea I would appreciate it. If not I will think about how to improve the situation.

I do believe that "parent"/"children" is different from a pure reference like Profile with Repo is already having.

Edit: And yes I do know that utils.blender() requires grab_tree() but I am working on getting rid of that too. The only thing a Repo can inherit from is the settings and this is a special case anyway.

[tpw56j]

It seems to me that the current implementation of defining logical inheritance rules and type dependencies via LOGICAL_INHERITANCE and TYPE_DEPENDENCIES is ugly. I did this because it was easier to implement the caching logic code. But as a result, this code is more difficult to maintain. Perhaps the code could be improved if, for example, decorators were somehow used instead.

TYPE_DEPENDENCIES describes formal dependencies between types, including for cascade deletion. This also allows you to narrow down the list of objects to invalidate the cache. If the InheritableProperty property of a Repo object changes, all caches of Profile objects that use them will be invalidated. I think this is correct, otherwise it can lead to subtle errors.

Much depends on what proportion of object caches need to be invalidated. If you know in advance that, for example, the dependency tree will contain 90% percent of all objects, then without building the tree you can simply reset the caches of all objects, as is done now when changing the settings. This will be faster than building a dependency tree and traversing it.

In general, I would not be afraid of excessive cache invalidation, even if this in some cases leads to a decrease in cache efficiency. It seems to me that reliability and the absence of false positive cache responses are much more important.

[SchoolGuy]

@tpw56j Please correct my thought if I am wrong but we want to speed up the to_dict() function especially for the resolved=True case. The only dependency that a Profile has on Repo is that the object has to exist. This is very different to my understanding what you are implying. Your comment would only be correct in case that we would cache the result of to_dict() as a whole. But since we are just caching individual properties we should be fine the the relationship of Profile and Repo.

I very much agree with your observation in the case of System --> Profile --> Distro since there also a change in properties affects children and parents alike. We have more constraints than the actual existence of the objects.

[tpw56j]

We cache the entire to_dict result:

    def to_dict(self, resolved: bool = False) -> Dict[Any, Any]:
        """
        This converts everything in this object to a dictionary.

        :param resolved: If this is True, Cobbler will resolve the values to its final form, rather than give you the
                     objects raw value.
        :return: A dictionary with all values present in this object.
        """
        if not self.inmemory:
            self.deserialize()
        cached_result = self.cache.get_dict_cache(resolved)
        if cached_result is not None:
            return cached_result

More precisely, we cache 2 results: one with resolved=True and one with resolved=False

[SchoolGuy]

@tpw56j Correct we can do that. However, that doesn't help us with removing the need to define the whole set of methods with the Repo object. I would like to make this object slimmer as it has no references to other items that are being serialized as such the concept of "children" doesn't make sense to maintain for it.

[tpw56j]

Yes, the concept of “children” does not apply in this case, but the concept of “dependency” remains. And this concept must be preserved at least in order to be able to recursively delete or prohibit the deletion of the repo along with all the objects that use it.

[SchoolGuy]

@tpw56j I do agree but my initial question was how to separate those two concepts in your mind. At the moment it seems we don't differentiate this.

This difference will become even more important when we begin to use real database schemas.

[tpw56j]

It seems to me that the easiest method to make objects thinner is to create a hierarchy of classes. Something like this:

        Item
        /  \
    Repo ChildItem 
         /     \
      BootItem  Menu
      /  |  \  
Distro Image Profile System

Item: only basic properties (uuid, name..).
ChildItem: the additional property children
BootItem: the properties that are needed to boot the OS (kernel_options, ..)

[SchoolGuy]

@tpw56j That is exactly what I currently have and due to the fact that I moved descendants into ChildItem/InheritableItem it doesn't work.

[SchoolGuy]

@agraul & I just a Slack Huddle about this problem. I will give his idea a shot. He mentioned that an external spectator could be responsible for signalling the items whose caches need to be updated.

Concretely this would mean that we use the __setattr__ to do a call to the CobblerAPI that contains the item type, the object uid/name and the modified attribute. The API could then use a dedicated module to populate the cache invalidation to the directly affected objects which in turn could use this API to recursively forward this signal.

The advantage that I see with this is that the logic to invalidate the caches is now outside of any abstract or specific item and as such the TYPE_DEPENDENCIES dict and its attached logic can be moved outside of the item tree as a whole.

I have no idea if that works in the end or not but it is worth a try in my eyes.

[tpw56j]

descendants and TYPE_DEPENDENCIES must remain in the Item; they also describes dependencies outside of the parent/child relationship. But it needs to be overridden in ChildItem/InheritableItem.

[tpw56j]

Item:

    def descendants(self) -> List["ITEM_UNION"]:
        results = set(self)
        for item_type in Item.TYPE_DEPENDENCIES[self.COLLECTION_TYPE]:
            dep_type_items = self.api.find_items(
                item_type[0], {item_type[1]: self.name}, return_list=True
            )
            if dep_type_items is None or not isinstance(dep_type_items, list):
                raise ValueError("Expected list to be returned by find_items")
            results.update(dep_type_items)
            for dep_item in dep_type_items:
                results.update(dep_item.descendants)
        return list(results)

ChildItem:

   def descendants(self) -> List["ITEM_UNION"]:
        childs = self.tree_walk()
        results = set(childs)
        childs.append(self)  # type: ignore
        for child in childs:
            results.update(child.descendants)
        return list(results)

Edit: We also need to add a call to the descendants method from Item

ChildItem:

   def descendants(self) -> List["ITEM_UNION"]:
        childs = self.tree_walk()
        results = set(childs)
        childs.append(self)  # type: ignore
        for child in childs:
            results.update(child.descendants)
            results.update(Item.descendants(child))
        return list(results)

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.36% (target: -1.00%)	✅ 81.54% (target: 70.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (69889af)	15525	10242	65.97%
Head commit (20dc7b3)	15683 (+158)	10403 (+161)	66.33% (+0.36%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3440)	65	53	81.54%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}