fix: indexer cache error when default evictor is re-initialized

[a7i]

It appears that DefaultEvictor is re-initialized, which causes a conflict issue when adding the same indexer again.

closes #1440

[a7i]

/cc @ingvagabund

any ideas on how to make this better? to be honest, I'm not sure why DefaultEvictor is getting re-initialized. But perhaps this is possible in case of multiple profiles

[ingvagabund]

I'm not sure why DefaultEvictor is getting re-initialized. But perhaps this is possible in case of multiple profiles

Yeah. Each profile gets initialized separately. The same shared informer, a new instance of the default evictor.

[ingvagabund]

Some thoughts:

• what if two plugins register the same indexer? We should prefix each indexer with a plugin name (some time in the future)
• should we register generic indexer so the default plugins do not need to register it on their own? This will not resolve the current issue. Yet, it will get rid of the error
• should we create a separate shared informer for each profile? We don't limit the number of profiles. We probably should, e.g. to 100 or 1000 and extend later when needed?

[ingvagabund]

The current implementation creates a new profile in each descheduling cycle. Yet, the created indexer never gets removed from the shared informer. Making every other descheduling cycle to error when minReplicas is set to a non-zero number.

[ingvagabund]

Worth checking if the code can be cleanly refactored to initialize each plugin only once per the whole descheduler run. The primary motivation for re-initializing each plugin was to start clean. If we can guarantee no plugin will keep a state then this will be a nice improvement to the code base.

[a7i]

what if two plugins register the same indexer? We should prefix each indexer with a plugin name (some time in the future)

Why is that necessarily? This is no plugin logic inside the shared informer. And ideally we share the shared informer.

should we register generic indexer so the default plugins do not need to register it on their own? This will not resolve the current issue. Yet, it will get rid of the error

we certainly could, but we then need to compare it against the current implementation. I personally prefer this PR but open to feedback from others. For example, no need to initialize the shared informer for policies that do not need minReplicas check.

should we create a separate shared informer for each profile? We don't limit the number of profiles. We probably should, e.g. to 100 or 1000 and extend later when needed?

is there a reason for this? ideally we share it to reduce memory footprint.

[a7i]

Worth checking if the code can be cleanly refactored to initialize each plugin only once per the whole descheduler run. The primary motivation for re-initializing each plugin was to start clean. If we can guarantee no plugin will keep a state then this will be a nice improvement to the code base.

I think the issue is that the params/config for each can be different. Correct?

[ingvagabund]

We could inject the params/config as "data" (alongside nodes and pods). Assuming the args do not interfere with a plugin initialization. Though, that will change the plugin API.

[ingvagabund]

I think the issue is that the params/config for each can be different. Correct?

You are right. That was another motivation for it.

[ingvagabund]

Note: We share a single shared informer through all the plugins. As long as the plugins are part of this repository we have a full control over how each indexer gets created. We don't have such control over custom plugins. Thus, we need to rely on good citizenship principles and assume anyone assembling their own deschedulers from the descheduling framework will take the extra step of making sure all the extra plugins create their indexes properly. Plugin owners can already define as many indexers as they like without following any good practices. The same holds for updating items in either of the informer caches without making a deep copy.

[ingvagabund]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ingvagabund]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment