-
Notifications
You must be signed in to change notification settings - Fork 227
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
Reduce watch updates #4050
Reduce watch updates #4050
Conversation
Signed-off-by: John Belamaric <[email protected]>
Signed-off-by: John Belamaric <[email protected]>
Signed-off-by: John Belamaric <[email protected]>
Signed-off-by: John Belamaric <[email protected]>
Signed-off-by: John Belamaric <[email protected]>
Signed-off-by: John Belamaric <[email protected]>
Signed-off-by: John Belamaric <[email protected]>
Signed-off-by: John Belamaric <[email protected]>
FYI, here is without this PR (though also without #4049). Note the memory is stable but the CPU is climbing over the course of the day. |
Ok, getting a little closer. Looks like I am probably fixing the wrong ticker here. This ticker won't launch new go routines, but this will: https://github.com/kptdev/kpt/blob/main/porch/pkg/registry/porch/background.go#L116 because |
I will merge this now anyway, it does help the issue. But we have more work to do. |
As an FYI, as expected shortly after runs started exceeding 60 seconds, OOMKill:
I will now restart this pod with the latest build that includes this PR, and see how it does. |
Fixes #4023
This replaces the previous PR. The fixes in here are a mitigation based on the current theory of the problem, but there are some things I am not sure about yet. I am submitting this PR so we at least have a mitigation in place, and we can continue working on the problem.
Current hypothesis:
So, that all sounds great, except that for 3), I don't see a new go routine per tick, but rather one per "newRepository" call. So, maybe the issue is that somewhere we are re-creating repositories? Maybe the fix in #4049 is sufficient in that case. But we need a mitigation now, we can check that later.