Items: Refactor base classes #3440
Conversation
2b8905b to
c6d4e04
Compare
c6d4e04 to
5f800e1
Compare
5f800e1 to
2be47d4
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesYou may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation |
a333f47 to
97ff169
Compare
193bdb9 to
449116d
Compare
|
@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 |
|
@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 In addition, since dependency rules are known only through |
|
@tpw56j Ahhhh thanks now I see where my logic error is. Then my assumption that |
|
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 I do believe that "parent"/"children" is different from a pure reference like Edit: And yes I do know that |
|
It seems to me that the current implementation of defining logical inheritance rules and type dependencies via
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 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. |
|
@tpw56j Please correct my thought if I am wrong but we want to speed up the I very much agree with your observation in the case of |
|
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_resultMore precisely, we cache 2 results: one with |
|
@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 |
|
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. |
|
@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. |
|
It seems to me that the easiest method to make objects thinner is to create a hierarchy of classes. Something like this: Item: only basic properties (uuid, name..). |
|
@tpw56j That is exactly what I currently have and due to the fact that I moved |
|
@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 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 I have no idea if that works in the end or not but it is worth a try in my eyes. |
|
|
|
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 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) |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesYou may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation |
The functionality described in 253f7a9 is not used and as such the remaining code for it is removed.
Even code dating back to 2.6.6 doesn't have any usage to this attribute. As such it is removed.
20dc7b3 to
a4fee18
Compare
|
I have updated the description to reflect the new plan. The last point of the numbered list will be done in this PR. This PR will contain the changelog for this undertaking. |
|
This PR (and related ones) are blocked until a stable version for the WebUI, CLI and Terraform Provider are out so we can branch those off in the related projects. |
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:
NetworkInterfacelive in its own module - Move NetworkInterface class to dedicated module #3739BaseItem- IntroduceBaseItemclass #3758InheritableItem- Items: Introduce InheritableItem #3787BootableItem- Items: IntroduceBootableItem#3805NetworkInterfacea dedicated collection (with data migration) - Feature: Dedicated network interfaces #3882BootLoadertypeVirtOptionsPowerOptionsIPv4Options&IPv6OptionsDnsOptionsTftpOptionsEach step should have a green CI so each commit can be individually validated. This PR will implement steps six and seven.
Behaviour changes
Old:
New:
Category
This is related to a:
Tests