VPA: prune stale container aggregates, split recommendations over true number of containers

[jkyros]

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:

• VPAs showing recommendations for containers which no longer exist
• Resources being split across containers which no longer exist (resulting in some containers ending up with resource limits too small for them to effectively live)
• There was also a corner case where during a rollout after a container was renamed/removed from a deployment, we were counting the number of unique container names and not the actual number of containers in each pod, so we were splitting resources that shouldn't have been split.

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:

• I went with explicitly marking and sweeping them because it saved us an additional loop through all the pods and containers
• We could also just as easily just have a PruneAggregates() that runs after LoadPods() that goes through everything and removes them (or do this work as part of LoadPods() but that seems...deceptive?)
• We could probably also tweak the existing garbageCollectAggregateCollectionStates and run it immediately after LoadPods() 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.

• If I am being ignorant, and there are corner cases I'm missing, absolutely let me know
• it probably need some tests/cleanup and I'll change the names of things to...whatever you want them to be. 😄

Does this PR introduce a user-facing change?

Added pruning of container aggregates and changed container math so resources will no longer be split across the wrong number of containers

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[kwiesmueller]

Is it a lot of extra work to do that? Do you see any risks doing it here?

[jkyros]

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...)

[kwiesmueller]

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.

[jkyros]

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:

1. update the VPA as the pods roll through (which requires me to find the VPA for each pod like I did here) or
2. 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)
3. 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?

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[sreber84]

/remove-lifecycle rotten

[adrianmoisey]

/ok-to-test
/assign

I want to see if I can help get this merged

[maxcao13]

Hi everyone, so John has take a hiatus and has left me with this PR, so after catching up, I guess we are still waiting for those conversations to resolve on which way do we want to go with those design decisions. The two commits I just put up are just a improvement on the existing implementation (assuming we will go with that, we don't have to), and some e2e tests to prove this works.

[adrianmoisey]

I thought of a scenario, which may be something that needs to be catered for.

if someone has a VPA on a CronJob with successfulJobsHistoryLimit set to 0, then the VPA could cleanup any recommendation that it has of that CronJob, since there are moments in time when no Pods exist.

Sorry for the noise, and forgive me for not understanding, but what would be the problem here in terms of removing stale recommendations? Is it because the VPA is supposed to use identical recommendations for every container in a Job that gets created by a CronJob and we want to reuse the recommendations? If so, then apparently #6745 (comment) should solve this issue since the CronJob still exists. But I'm probably misunderstanding the question...

I think this is a possible scenario prior to this PR merging:

1. A CronJob and VPA are configured
2. On first run of the CronJob, VPA's recommendation is calculated and written to the VPA object
3. On second run of the CronJob, the admission-controller has a recommendation that it can apply to the new Pod
4. Step 3 repeats, and as the workload changes over time, so does the recommendation

But, I think this PR would change that scenario to the following:

1. A CronJob and VPA are configured
2. On first run of the CronJob, VPA's recommendation is calculated and written to the VPA object
3. VPA recommender cleans up the recommendation, as no Pods currently exist
4. On second run of the CronJob, no recommendation is applied, since no recommendation exists. But the resulting Pod will cause a recommendation to be created
5. VPA recommender cleans up the recommendation, as no Pods currently exist
6. On third run of the CronJob, no recommendation is applied, since no recommendation exists. But the resulting Pod will cause a recommendation to be created
7. Steps 5-6 repeat, effectively causing the CronJob to not have a VPA

This scenario is assuming that the CronJob is configured with successfulJobsHistoryLimit set to 0, and that the frequency of the recommendation cleanup is greater than the frequency of the CronJob.

[maxcao13]

This scenario is assuming that the CronJob is configured with successfulJobsHistoryLimit set to 0, and that the frequency of the recommendation cleanup is greater than the frequency of the CronJob.

Got it, thanks for the detailed explanation!

This does happen and the recommendations are cleaned up before the next job gets scheduled, assuming the recommendation happens in time and the job schedule is long enough for the recommendation to prune the stale recommendations.

Thinking about your solution, I think it makes sense to add some sort of timeout feature for recommendations that refresh when they are used again -- pretty much what you mean by your cleanup feature. Maybe as a per VPA configuration in the VPA CR spec? Currently this is the current implemenation of whether recommendations will get recorded in the object:

autoscaler/vertical-pod-autoscaler/pkg/recommender/routines/recommender.go

Line 107 in c44d4f0

hasMatchingPods := vpa.PodCount > 0

So instead, the "matching pods" filtered pods should still include pods that have removed aggregates for a certain period of time based on a per VPA configuration, and maybe a global timeout as well (similar to how minReplicas works).

Although thinking about it, there is a small window between the VPA deciding "we should remove this container recommendation, it doesn't exist anymore and we should stop splitting recommendations over it because it was a rename" vs "we shouldn't remove this container recommendation because its targetRef's top most controller is a cronjob so we should not remove the recommendation as jobs will quickly use it again." I think there would need to be a lot of care in setting this staleness timeout feature, and users would have to know how exactly how long they want the recommendation to linger for depending on if it's a cronjob, or a regular workload, for them to scale as they intended.

[maxcao13]

So maybe it is best to just look at if the topmost controller is CronJob and decide based on that to remove this extra complexity?

[adrianmoisey]

So maybe it is best to just look at if the topmost controller is CronJob and decide based on that to remove this extra complexity?

I was actually wondering about this option too.
It would also be done for all well known types. Rather than looking at the Pods to determine if a VPA recommendation, needs cleaning, look at the top most well known type.

[maxcao13]

It would also be done for all well known types. Rather than looking at the Pods to determine if a VPA recommendation, needs cleaning, look at the top most well known type.

In this case if we do this, if we rename a container, and the deployment exists, wouldn't there still be stale recommendations for the old container? Maybe the aggregate would get deleted and we wouldn't split resources anymore, but there just might be a stale recommendation in the vpa object that will never get deleted (but maybe that's okay?), if I am understanding this right.

That's why I am wondering if we have to specially handle CronJobs since those containers that get created are supposed to be deleted.

EDIT: I think the grace period idea might be the way to go, because the renaming situation vs the CronJob situation seems to me like the VPA would not be able to tell the difference, and I'm not sure we want to implement the top most controller checking in the recommender since the caches and informers don't exist there (exists for updater and admission I think?).

[maxcao13]

Okay, in b6e435b, I added pruningGracePeriod to per VPA containerPolicies so that people can specify graces periods for stale recommendations. Here's an example:

apiVersion: batch/v1
kind: CronJob
metadata:
  name: hello
spec:
  schedule: "*/3 * * * *"
  successfulJobsHistoryLimit: 0
  jobTemplate:
    spec:
      template:
        metadata:
          labels:
            app: hello
        spec:
          containers:
          - name: hello
            image: busybox:1.28
            imagePullPolicy: IfNotPresent
            command:
            - /bin/sh
            - -c
            - |
              tries=0
              while [ $tries -lt 30 ]; do
                echo "Hello, World $tries!"
                tries=$((tries+1))
                sleep 5
              done
          restartPolicy: OnFailure
---
apiVersion: autoscaling.k8s.io/v1
kind: VerticalPodAutoscaler
metadata:
  name: hello
spec:
  targetRef:
      apiVersion: "apps/v1"
      kind:       "CronJob"
      name:       "hello"
  updatePolicy:
    updateMode: "Initial"
    minReplicas: 1
  resourcePolicy:
    containerPolicies:
    - containerName: "*"
      pruningGracePeriod: 3m

The grace period is long enough so that aggregates don't get pruned before the next job gets created and refreshes the last update time of the container, and so the recommendations still get preserved. If you specify a long grace period or the targetRef is CronJob, it will be pruned in 24 hours. If you don't specify the field and the targetRef is not CronJob, then the pruningGracePeriod is 0 and aggregates will be pruned immediately as soon as the next recommendation loop occurs, which will then remove the recommendations from the api object.

IDK how people feel about this, so I didn't write any tests. But using it locally, it seems to work as intended.

[adrianmoisey]

It would also be done for all well known types. Rather than looking at the Pods to determine if a VPA recommendation, needs cleaning, look at the top most well known type.

In this case if we do this, if we rename a container, and the deployment exists, wouldn't there still be stale recommendations for the old container? Maybe the aggregate would get deleted and we wouldn't split resources anymore, but there just might be a stale recommendation in the vpa object that will never get deleted (but maybe that's okay?), if I am understanding this right.

My idea was to check the targetRef to see what it contains, in order to determine if a VPA's recommendation needs cleaning.
Ie: for a Deployment targetRef, specifically looking at the list of Deployment.spec.template.spec.containers[*].name to figure out which containers recommendations to keep.

This will only work on well-known types

[adrianmoisey]

Okay, in b6e435b, I added pruningGracePeriod to per VPA containerPolicies so that people can specify graces periods for stale recommendations.

My opinion is that this is an adequate solution.

@voelzmo @raywainman @kwiesmueller @omerap12 thoughts on this one?

[omerap12]

Okay, in b6e435b, I added pruningGracePeriod to per VPA containerPolicies so that people can specify graces periods for stale recommendations.

My opinion is that this is an adequate solution.

@voelzmo @raywainman @kwiesmueller @omerap12 thoughts on this one?

I’m not sure I understand the use case for allowing users to specify grace periods for stale recommendations.
For deployments where container names have changed, couldn’t we use a default value or immediately discard stale recommendations?
For CronJobs, why not calculate the value dynamically based on the schedule? For example, if the schedule is 1 hour, we could set it to 1 hour + 10 minutes as a safeguard.

What do you think?

[omerap12]

IMHO It does make sense.

[adrianmoisey]

If you don't specify the field and the targetRef is not CronJob, then the pruningGracePeriod is 0 and aggregates will be pruned immediately as soon as the next recommendation loop occurs, which will then remove the recommendations from the api object.

I think I figured out another edge-case that needs to be handled.

Sometimes in my day-job, when we're firefighting issues, we may scale a deployment down to zero to alleviate pressure on other parts of the system. I think this use-case is possibly common, tools such as https://keda.sh are built to allow users to scale workloads down to zero, to save costs.

During these times the recommendation would be removed. I think the grace period needs to apply to all resource types.

[maxcao13]

I’m not sure I understand the use case for allowing users to specify grace periods for stale recommendations. For deployments where container names have changed, couldn’t we use a default value or immediately discard stale recommendations? For CronJobs, why not calculate the value dynamically based on the schedule? For example, if the schedule is 1 hour, we could set it to 1 hour + 10 minutes as a safeguard.

What do you think?

I think allowing the users to set it will be good in a case like @adrianmoisey said, where you may scale a deployment down to 0 to alleviate pressure on the system but don't want the recommendations to get lost. Then you can specifically set the pruningGracePeriod on the deployment's containers to be something high like 10d, so that the recommendations don't get removed while you are troubleshooting your system. But maybe you don't want that for some other workload, so you can set the gracePeriod to something like 10 minutes, so we don't have a memory leak in the recommender.

I think I figured out another edge-case that needs to be handled.

Sometimes in my day-job, when we're firefighting issues, we may scale a deployment down to zero to alleviate pressure on other parts of the system. I think this use-case is possibly common, tools such as https://keda.sh/ are built to allow users to scale workloads down to zero, to save costs.

During these times the recommendation would be removed. I think the grace period needs to apply to all resource types.

This should be able to work on all types as long as you set the field (unless I am misunderstanding you). Do you mean that we should default the gracePeriod to be very high (never expire) instead of 0? Maybe we also allow a global default gracePeriod set with a command line flag?

[adrianmoisey]

I think I figured out another edge-case that needs to be handled.

Sometimes in my day-job, when we're firefighting issues, we may scale a deployment down to zero to alleviate pressure on other parts of the system. I think this use-case is possibly common, tools such as https://keda.sh/ are built to allow users to scale workloads down to zero, to save costs.

During these times the recommendation would be removed. I think the grace period needs to apply to all resource types.

This should be able to work on all types as long as you set the field (unless I am misunderstanding you). Do you mean that we should default the gracePeriod to be very high (never expire) instead of 0? Maybe we also allow a global default gracePeriod set with a command line flag?

Yeah, I guess it makes sense to have a long default or keep the feature disabled and allow it to be opt-in.

[raywainman]

Could the default be "don't clean them up" to match behavior today? I worry that this is verging on a breaking change.

Then a user can override the pruning grace period for the problematic VPAs that are churning containers?

[omerap12]

I think I figured out another edge-case that needs to be handled.

Sometimes in my day-job, when we're firefighting issues, we may scale a deployment down to zero to alleviate pressure on other parts of the system. I think this use-case is possibly common, tools such as https://keda.sh/ are built to allow users to scale workloads down to zero, to save costs.

During these times the recommendation would be removed. I think the grace period needs to apply to all resource types.

This should be able to work on all types as long as you set the field (unless I am misunderstanding you). Do you mean that we should default the gracePeriod to be very high (never expire) instead of 0? Maybe we also allow a global default gracePeriod set with a command line flag?

Yeah, I guess it makes sense to have a long default or keep the feature disabled and allow it to be opt-in.

I agree

[maxcao13]

Okay, I've added a --pruning-grace-period-duration flag which defaults to 100 years meaning that aggregates will not expire which is the previous implementation before this PR (hopefully someone is not keeping a vpa-recommender container up for 100 years).

I removed any special handling of CronJob since the default pruning functionality should now opt-in and not be a breaking change.

EDIT: To make it clear for reviewers, this pruning of aggregate collection states/recommendations only applies to the state that is kept by each VPA. The state map that exists for the clusterState as a whole is unaffected by this change because that state gets GC'd on an hourly basis

autoscaler/vertical-pod-autoscaler/pkg/recommender/model/cluster.go

Line 364 in 7df4f84

func (cluster *ClusterState) garbageCollectAggregateCollectionStates(ctx context.Context, now time.Time, controllerFetcher controllerfetcher.ControllerFetcher) {

.

So if the top-most-controller still exists, the gc will never prune the clusterState aggregates, but the per-vpa aggregates will not be pruned as long as the pruning-grace-period-duration for the container is long enough. However, if the top-most-controller no longer exists, the pod is in a non-active state, and an hour passes, the GC will prune both maps ignoring the grace period. This bug fix is so that renames/removals are caught early, and doesn't require the top-most-controller to not exist + we don't have to wait an entire hour -> as long as the user specifies the new per container pruningGracePeriod field.

Eventually if the top-most-controller not longer exists, the per-vpa aggregates will eventually get pruned anyways as mentioned in an earlier review comment.

[adrianmoisey]

I'm unsure what others think, but I'm not super excited by having the default be 100 years. My preference would be that setting the global to zero would disable the pruning feature (except for VPAs with pruningGracePeriod set)

[maxcao13]

Yeah, I agree it's not ideal. I was trying to figure out how I could enable/disable it with a single flag, and without specially handling a zero value for a duration. I guess I could switch to flag.String and specially handle a 0 as off. Or if there's some cleaner way to do it I'm happy for feedback.