Hi @krzysztof-jastrzebski. 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.

/sig autoscaling

/assign mwielgus

How was this PR tested?

I've created K8s build with debugs showing when HPAs are processed, created cluster and HPA. I observed how often HPA is updated. Next I removed HPA just after HPA was processed and verified that HPA is not processed anymore.

/lgtm

/approve

/ok-to-test

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krzysztof-jastrzebski, mwielgus

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

The pull request process is described here

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

/hold

Unconditional reinsertion seems very likely to cause problems. Doesn't this cause HPAs to be processed much more frequently than intended?

HPA controller uses rate limiter which always returns ResyncPeriod (currently 15 seconds).
Rate Limiter:

I see this is not the best way of using RateLimitingQueue but I would like to do small fix so I can backport it to 1.12. If you think we should use queue in other way then I'm happy to fix it next PR. Do you have any suggestion how to do that?

This is a strange use of the rate limiting queue trying to achieve tight guarantees about retry interval.

I'm concerned about doubling the number of queue insertion attempts for systems with large numbers of HPAs. Attempting to enqueue all HPAs in the system every 15 seconds is already very aggressive... this change would attempt to enqueue them all twice every 15 seconds.

Until the structure of this controller can be revisited, can we do something simpler and less likely to impact performance like one of the following:

• set up the HPA AddEventHandlerWithResyncPeriod with resyncPeriod+time.Second
• set up NewDefaultHPARateLimiter(resyncPeriod-time.Second)

Either of those approaches would give delayingType#waitingLoop a second to move ready HPA items from waitingForQueue into the queue. If that takes longer than a second (for example, under load), HPA processing could still skip a resync interval, but that actually seems like a better outcome to me than doubling queue attempts in pursuit of exact retry intervals.

I was thinking about similar solution. The problem with it is why to add/subtract 1 second. resyncPeriod is defined by a flag. Default is 15 seconds but someone can use 1 second resuncPeriod. If we added 1 second then HPAs would be refreshed every 2 seconds instead of 1, we would also process HPA on every change of the objects (rate limiter set to 0) which can be in some cases very often.
I don't think that adding HPA to queue after processing it is a problem as current implementation of HPA(it is single-threaded) can process less than 10 HPAs per second. Adding 10 request every second to local queue shouldn't be a big deal.

Default is 15 seconds but someone can use 1 second resyncPeriod. If we added 1 second then HPAs would be refreshed every 2 seconds instead of 1, we would also process HPA on every change of the objects (rate limiter set to 0) which can be in some cases very often.

Setting a 1 second resyncPeriod isn't likely to work well in general, but I agree we wouldn't want to drop that to 0. Setting to a percentage or capping at a minimum value would prevent that.

Unconditional requeuing is confusing to anyone who is familiar with all the other controllers. I'd prefer the smallest change we can make that doesn't take this controller even further away from standard usage of the queue components, so I'm not in favor of the current PR, but will defer to @kubernetes/sig-autoscaling-pr-reviews

/hold cancel

please also fix the PR description to remove the unused flags.

I'm inclined to go with @liggitt's suggestion short-term, and think up a better long-term fix (e.g. don't watch HPA, and instead just reconcile every 15 seconds, staggered, or something).

Using solution suggested by @liggitt will take the same amount of time (implementing and testing) as implementing final solution. I prefer submitting this solution (unless you see any bugs in it), backporting it and implementing final solution immediately.

All of the proposed solutions are ugly one way or another. Probably the cleanest solution would be to write the right queue instead of trying to bend the existing one to match the needs. Given the amount of time needed to implement, it I'm rather in favour of merging the proposed/existing/tested fix unless someone proves it doesn't work.

@krzysztof-jastrzebski please expand the comments in the code so that the possible confusion caused by non-standard use is minimal.

@mwielgus done

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-kubemark-e2e-gce-big

/test pull-kubernetes-e2e-gce-100-performance

/test pull-kubernetes-kubemark-e2e-gce-big

/test pull-kubernetes-e2e-gce-100-performance

/lgtm

/test pull-kubernetes-integration