-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Addons auto-pause: Remove memory leak & add configurable interval #17936
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spowelljr 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 |
/ok-to-test |
ok-to-build-iso |
ok-to-build-image |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2766d66
to
560d3aa
Compare
ok-to-build-image |
ok-to-build-iso |
Hi @spowelljr, we have updated your PR with the reference to newly built kicbase image. Pull the changes locally if you want to test with them or update your PR further. |
Hi @spowelljr, we have updated your PR with the reference to newly built ISO. Pull the changes locally if you want to test with them or update your PR further. |
/ok-to-test |
kvm2 driver with docker runtime
Times for minikube start: 51.1s 51.4s 48.5s 53.6s 50.0s Times for minikube (PR 17936) ingress: 27.7s 30.1s 49.7s 28.6s 27.1s docker driver with docker runtime
Times for minikube start: 25.1s 21.6s 25.1s 22.1s 25.0s Times for minikube ingress: 20.8s 20.8s 20.8s 20.8s 21.8s docker driver with containerd runtime
Times for minikube start: 23.4s 20.6s 23.1s 23.3s 20.4s Times for minikube ingress: 31.3s 32.4s 47.3s 32.3s 32.3s |
These are the flake rates of all failed tests.
To see the flake rates of all tests by environment, click here. |
What this PR accomplishes:
Memory Leak
For details on the memory leak see the issue #10596.
A fix for the memory leak was originally implemented by @vishjain in #11909. The PR successful resolved the memory leak but introduced a bug where once the pods were paused
kubectl
commands failed withUnable to connect to the server: EOF
, the PR was reverted in #17866.I reimplemented the fix following @vishjain's main concept of using a Ticker in place of multiple Timers. Once the addon is enabled the ticker is started with a one minute interval, if any
kubectl
commands are executed before the one minute countdown concludes the ticker is reset and starts again. Once the one minute countdown concludes the ticker is indefinitely stopped and the pods are paused. If akubectl
command is executed while the pods are paused, the pods are unpaused, and then the ticker is reset and the cycle continues.Configurable Interval
A PR to implement a configurable interval for auto-pause was originally implemented by @wzslr321 in #17070. The PR had some issues related to loading configs that resulted in the auto-pause binary terminating and the addon not working, but all the logic around storing and configuring the interval value was well done and able to be reused. The PR was reverted in #17866 where more details on the config loading bug can be found.
For background, the auto-pause binary, which is where the ticker is, is its own separate binary from minikube. The binary exists inside the Kicbase/ISO itself and is ran via systemd. So in order the pass the interval value in I had to add a flag value to the auto-pause binary, and then when the auto-pause addon is enabled the interval flag is passed into the systemd unit file.
The interval can be configured in one of two ways,
--auto-pause-interval
duringminikube start
orminikube addons configure auto-pause
. Both ways have validation to ensure a valid duration string is provided, and that the duration is greater than 0s, a negative or 0s duration will cause a panic in the Ticker.Follow up work
There's an issue with running
minikube addons configure auto-pause
, the value is successfully saved, but the value is not immediately updated in the auto-pause addon itself. The systemd unit file is updated, but then asystemctl daemon-reload
&systemctl restart auto-pause.service
both have to be run for the update to be reflected in the binary. Due to the already large size of this PR I'll leave it for a follow up.