-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GCE: Implement kube-env caching #6531
Conversation
7ea55df
to
de085c9
Compare
de085c9
to
1990b4e
Compare
1990b4e
to
e81c27b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In General, LGTM
However I might be messing some stuff so it'd be safer if someone else took a quick look
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: atwamahmoud, BigDarkClown 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:
Approvers can indicate their approval by writing |
e81c27b
to
760b2b5
Compare
/lgtm It would be better to omit the Edit: Ahh, I forgot I'm not in the OWNERS file passing to @jayantjain93 |
@atwamahmoud: changing LGTM is restricted to collaborators In response to this:
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. |
} | ||
|
||
// InvalidateAllMigKubeEnvs clears the kube-env cache | ||
func (gc *GceCache) InvalidateAllMigKubeEnvs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we ever need either Invalidate
? Since instance templates are immutable, we probably won't ever call this. What we might want instead though is some expiration mechanism - to avoid memleaks in clusters with high MIG churn. This is a nice to have though, I don't expect single CA instance to observe enough instance templates over entire process lifetime to visibly inflate memory usage.
I left a comment, but that is something to follow up on later, not in this PR, so let's merge as is. /lgtm Btw, @atwamahmoud - you don't need to be an OWNER to /lgtm, you just need to be a Kubernetes org member. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Caching of kube-env for instance templates. The change greatly improves the CPU usage for idle clusters, as ~80% of the main loop time is spend on repetitive unmarshalling of kube-env.
The improvement is drastic for idle clusters even with only 3 MIGs. In my testing the loop time decreased from 1.25s to 0.25s.