The approach copied from node taint eviction was to fire off one goroutine per
pod the intended time. This leads to the "thundering herd" problem: when a
single taint causes eviction of several pods and those all have no or the same
toleration grace period, then they all get deleted concurrently at the same
time.
For node taint eviction that is limited by the number of pods per node, which
is typically ~100. In an integration test, that already led to problems with
watchers:
cacher.go:855] cacher (pods): 100 objects queued in incoming channel.
cache_watcher.go:203] Forcing pods watcher close due to unresponsiveness: key: "/pods/", labels: "", fields: "". len(c.input) = 10, len(c.result) = 10, graceful = false
It also causes spikes in memory consumption (mostly the 2KB stack per goroutine
plus closure) with no upper limit.
Using a workqueue makes concurrency more deterministic because there is an
upper limit. In the integration test, 10 workers kept the watch active.
Another advantage is that failures to evict the pod get retried with
exponential backoff per affected pod forever. Previously, evicting was tried a
few times with a fixed rate and then the controller gave up. If the apiserver
was down long enough, pods didn't get evicted.
When the ResourceSlice no longer exists, the ResourceSlice tracker didn't and
couldn't report the tainted devices even if they are allocated and in use. The
controller must keep track of DeviceTaintRules itself and handle this scenario.
In this scenario it is impossible to evaluation CEL expressions because the
necessary device attributes aren't available. We could:
- Copy them in the allocation result: too large, big change.
- Limit usage of CEL expressions to rules with no eviction: inconsistent.
- Remove the fields which cannot be supported well.
The last option is chosen.
The tracker is now no longer needed by the eviction controller. Reading
directly from the informer means that we cannot assume that pointers are
consistent. We have to track ResourceSlices by their name, not their pointer.
This raises the number of allowed taints per device to 16 by lowering
the number of allowed devices to 64 per ResourceSlice if (and only if!)
taints are used.
"effect: None" and DeviceTaintRule status with conditions get added
to support giving feedback to admins.
Instead of merely adding the new effect value, this also changes validation of
the enum so that unknown values are valid if they were already stored. This
will simplify adding new effects in the future because validation won't fail
for them after a downgrade. Consumers must treat them like this new None
effect, i.e. ignore them.
The immediate benefit is that the time required for running the package's unit
test goes down from ~10 seconds (because of required real-world delays) to ~0.5
seconds (depending on the CPU performance of the host). It can also make
writing tests easier because after a `Wait` there is no need for locking before
accessing internal state (all background goroutines are known to be blocked
waiting for the main goroutine).
What somewhat ruins the perfect determinism is the polling for informer cache
syncs: that can take an unknown number of loop iterations. Probably could be
fixed by making the waiting block on channels (requires work in client-go).
The only change required in the implementation is avoiding the sleep when
deleting a pod failed for the last time in the loop (a useful, albeit minor
improvement by itself): the test proceeds after having blocked that last Delete
call, in which case synctest expects the background goroutine to exit without
delay.
They now live in a separate file, use ktesting+Gomega consistently, and
follow (mostly) best practices:
- strict validation during create
- wait for completion of delete
The latter is not complete yet because it was found that some binding condition
test keeps a ResourceCLaim in an undeletable state. That's okayish (namespace
stays around until apiserver gets restarted), but is worth looking into later.
This change completes the contextual logging migration for the memory
manager by updating the Manager interface and all implementations to
accept logr.Logger parameters instead of context.Context.
Key changes:
- Update Manager interface methods to accept logr.Logger:
* AddContainer, RemoveContainer, GetMemoryNUMANodes
* GetAllocatableMemory, GetMemory (removed context entirely)
- Update Policy interface methods to accept logr.Logger instead of context.Context
- Pass logger to NewManager() and policy constructors (NewPolicyStatic, NewPolicyNone, NewPolicyBestEffort)
- Update internal_container_lifecycle to use klog.TODO() when calling memory manager methods
- Update fake manager to accept and use logger parameter
- Update all test code to pass logger instead of context
This aligns with the contextual logging migration pattern where:
- Functions that need a logger accept logr.Logger parameter directly
- Logger is passed from the boundary (e.g., Start()) down to implementation
- klog.TODO() is used temporarily in call sites where logger is not yet available
- Context is only used where truly needed (e.g., Start() method)
This follows the same pattern as recent migrations in:
- pkg/kubelet/cm/topologymanager (#134174)
- pkg/kubelet/cm/devicemanager (#134293)
- pkg/kubelet/cm/cpumanager (#125912)
Related to the initial memory manager contextual logging work in #130727.
Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
