VPA: prune stale container aggregates, split recommendations over true number of containers #6745
Conversation
Previously we were dividing the resources per pod by the number of container aggregates, but in a situation where we're doing a rollout and the container names are changing (either a rename, or a removal) we're splitting resources across the wrong number of containers, resulting in smaller values than we should actually have. This collects a count of containers in the model when the pods are loaded, and uses the "high water mark value", so in the event we are doing something like adding a container during a rollout, we favor the pod that has the additional container. There are probably better ways to do this plumbing, but this was my initial attempt, and it does fix the issue.
Previously we were only cleaning checkpoints after something happened to the VPA or the targetRef, and so when a container got renamed the checkpoint would stick around forever. Since we're trying to clean up the aggregates immediately now, we need to force the checkpoint garbage collection to clean up any checkpoints that don't have matching aggregates. If the checkpoints did get loaded back in after a restart, PruneContainers() would take the aggregates back out, but we probably shouldn't leave the checkpoints out there.
Previously we were letting the rate limited garbage collector clean up the aggregate states, and that works really well in most cases, but when the list of containers in a pod changes, either due to the removal or rename of a container, the aggregates for the old containers stick around forever and cause problems. To get around this, this marks all existing aggregates/initial aggregates in the list for each VPA as "not under a VPA" every time before we LoadPods(), and then LoadPods() will re-mark the aggregates as "under a VPA" for all the ones that are still there, which lets us easily prune the stale container aggregates that are still marked as "not under a VPA" but are still wrongly in the VPA's list. This does leave the ultimate garbage collection to the rate limited garbage collector, which should be fine, we just needed the stale entries to get removed from the per-VPA lists so they didn't affect VPA behavior.
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jkyros The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
Hi @jkyros. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/L
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| // TODO(jkyros): This only removes the container state from the VPA's aggregate states, there | ||
| // is still a reference to them in feeder.clusterState.aggregateStateMap, and those get | ||
| // garbage collected eventually by the rate limited aggregate garbage collector later. | ||
| // Maybe we should clean those up here too since we know which ones are stale? |
There was a problem hiding this comment.
Is it a lot of extra work to do that? Do you see any risks doing it here?
There was a problem hiding this comment.
No, I don't think it's a lot of extra work, it should be reasonably cheap to clean them up here since it's just deletions from the other maps if the keys exist, I just didn't know all the history.
It seemed possible at least that we were intentionally waiting to clean up the aggregates so if there was an unexpected hiccup we didn't just immediately blow away all that aggregate history we worked so hard to get? (Like maybe someone oopses, deletes their deployment, then puts it back? Right now we don't have to start over -- the pods come back in, find their container aggregates, and resume ? But if I clean them up here, we have to start over...)
| // the correct number and not just the number of aggregates that have *ever* been present. (We don't want minimum resources | ||
| // to erroneously shrink, either) | ||
| func (cluster *ClusterState) setVPAContainersPerPod(pod *PodState) { | ||
| for _, vpa := range cluster.Vpas { |
There was a problem hiding this comment.
I'm wondering if there is already a place where this logic could go so we don't have to loop over all VPAs for every pod again here.
In large clusters with a VPA to Pod ratio that's closer to 1 this could be a little wasteful.
There was a problem hiding this comment.
Hmm, yeah, I struggled with finding a less expensive way without making too much of a mess. Unless I'm missing something (and I might be) we don't seem to have a VPA <--> Pod map -- probably because we didn't need one until now? At the very least I think I should gate this to only run if the number of containers in the pod is > 1.
Like, I think our options are:
- update the VPA as the pods roll through (which requires me to find the VPA for each pod like I did here) or
- count the containers as we load the VPAs (but we load the VPAs before we load the pods, so we'd have to go through the pods again, so that doesn't help us)
- have the VPA actually track the pods it's managing, something like this: jkyros@6ddc208 (could also just be an array of
PodIDs and we could look up the state so we could save the memory cost of the PodState pointer, but you know what I mean)
I put it where I did (option 1) because at least LoadPods() was already looping through all the pods so we could freeload off the "outer" pod loop and I figured we didn't want to spend the memory on option 3. If we'd entertain option 3 and are okay with the memory usage, I can totally do that?
What type of PR is this?
/kind bug
What this PR does / why we need it:
Previously we weren't cleaning up "stale" aggregates when container names changed (because of renames, removals) and that was resulting in:
This PR is an attempt to clean up those stale aggregates without incurring too much overhead, and make sure that the resources get spread across the correct number of containers during a rollout.
Which issue(s) this PR fixes:
Fixes #6744
Special notes for your reviewer:
There are probably a lot of different ways we can do the pruning of stale aggregates for missing containers:
PruneAggregates()that runs afterLoadPods()that goes through everything and removes them (or do this work as part ofLoadPods()but that seems...deceptive?)garbageCollectAggregateCollectionStatesand run it immediately afterLoadPods()every time but that might be expensive.I'm not super-attached to any particular approach, I'd just like to fix this, so I can retool it if necessary.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: