Backport lazy collection init #3634
Conversation
SchoolGuy
changed the title
SchoolGuy
force-pushed
the
backport/lazy-collection-init
branch
from
51e67ed
to
4b6f2f2
Compare
This is a backport of a25eb46. The items can be loaded when accessing the properties of the elements.
SchoolGuy
force-pushed
the
backport/lazy-collection-init
branch
from
4b6f2f2
to
a8e8da9
Compare
|
I will try to test the PR as soon as possible regarding the performance gains that it potentially presents. (The number of target items is 500000). |
|
@tpw56j Do you have an idea for the backport of how to prevent the recursion error for the IP addresses? I don't want to introduce breaking changes for this one and as such I am not sure where to start debugging/fixing at the moment... |
|
What does an IP addresses recursion error look like? |
|
@tpw56j Something like this: Edit: The issue is that since not everything is deserialized it tries to deserialize during deserializing. Do we just introduce a check for if the object is inmemory? |
|
One possible error could be here: Line 973 in a8e8da9 It should be like this: |
|
@tpw56j Yes that is certainly a bug but this is the inverse direction that is shown in the stacktrace. Any more ideas? |
|
The recursion occurs here: cobbler/cobbler/items/system.py Line 254 in b8a7f7a The interface as a whole must have |
|
Ahhhh! Okay I see that was my "search and replace" moment when I was bulk replacing decorators. Thanks for catching that! |
|
@tpw56j Sorry to say but it wasn't enough to fix the issue. We still have recursion during adding a new system since it tries to deserialize during deserializing ( |
|
This bug is also present on This bug is present for Edit: If |
|
@tpw56j I have pushed an experimental commit, it solves the chicken-egg problem that I described. I would love it if you would play the devil's advocate for my change and propose a better solution if you can offer one! |
|
Your commit breaks the logic of def deserialize(self) -> None:
"""
Deserializes the object itself and, if necessary, recursively all the objects it depends on.
"""
def deserialize_ancestor(ancestor_item_type: str, ancestor_name: str):
if ancestor_name not in {"", enums.VALUE_INHERITED}:
ancestor = self.api.get_items(ancestor_item_type).get(ancestor_name)
if ancestor is not None and not ancestor.inmemory:
ancestor.deserialize()
+ if not self._has_initialized:
+ return
+
item_dict = self.api.deserialize_item(self)
if item_dict["inmemory"]:
for ancestor_item_type, ancestor_deps in Item.TYPE_DEPENDENCIES.items():This solves the chicken-egg problem. Along the way, I found another bug in Fix: else:
raise CX("virt mac assignment not yet supported")
result = ":".join([f"{x:02x}" for x in mac])
systems = api_handle.systems()
- while systems.find(mac_address=mac):
+ while systems.find(mac_address=result):
result = get_random_mac(api_handle)
return result |
|
@tpw56j Thanks for the help! I will try it out after I slept. Recovering my k8s cluster atm. 😴 |
|
I was haunted last night by your idea of a lazy start with 500,000 target items. Therefore I suggest:
|
|
@tpw56j Thanks a lot :) |
|
I tested on my laptop the effect of In the current implementation, Here are my results for 1000 target systems. Timing of serializers.file deserialize() based on the results of 3 measurements
serializers.sqlite
serializers.mongodb
|
SchoolGuy
force-pushed
the
backport/lazy-collection-init
branch
from
419fb3a
to
f420775
Compare
|
@SchoolGuy I think this fix is not enough to solve the problem. To fix the bug in the main branch #3654, I had to additionally monitor the deserialization state of the entire collection to avoid recursion. The problem arises if, when deserializing an item, it is necessary to check the attributes of this item for validity among other items of the same or another collection. For example, check for duplicate mac addresses. In this case, |
|
@tpw56j Thanks for your take on this. I will cherry pick the required changes from your PR tomorrow. |
|
This PR isn't actually closed. We still want the backport. |
Linked Items
Fixes #3596
Description
This PR attempts to backport the lazy collection init. This will be off per default.
Behaviour changes
Old: Cobbler wasn't available until all items are fully loaded.
New: Cobbler will startup up with only the item names in memory until the item is first accessed.
Category
This is related to a:
Tests