-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Fix init order during starup for dynamic kubelet config #83184
Fix init order during starup for dynamic kubelet config #83184
Conversation
As you see here, the node e2e tests (running with Non of them pass in master: https://testgrid.k8s.io/sig-node-kubelet#node-kubelet-serial /cc @alejandrox1 |
34b159f
to
4182612
Compare
/assign @logicalhan |
If I understand this correctly, then the metrics do show up eventually, right? Once the kubelet is created, then then metrics should reflect the syncs. |
I think #83184 (comment) answers this question. |
|
If the metrics ain't created, the values will not be registered, and the metrics will not be visible in the metric endpoint. Therefore move init of dynamic kubelet config below the startup of the kubelet server (and the init of metrics).
4182612
to
2292873
Compare
Thanks for looking at it @logicalhan!
Yes, I think that is a good solution. I just need confirmation from someone that it will work, and that there is no dependencies that require it to stay where it is. I have updated the PR now,
Agree that force-registering the metrics is a bad solution, and goes against the new metric stuff; so I don't think that is a viable solution to the issue.
The test has a 1min timeout when looking for the metrics. And as I said in the other comments; from what I see and understand, if the first metric write is lost, the metrics will not be reapplied (Sync will not run) unless some of the data is changed. |
cc @kubernetes/sig-node-bugs |
Ping @logicalhan. Any new thoughts? |
/cc mtaufen derekwaynecarr |
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.
/lgtm
/assign dchen1107 derekwaynecarr |
all synchronous setup is done in Controller#Bootstrap(), everything done by this method (other than programming error sanity checks that exit the process) is done in goroutines /lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, odinuge 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 |
/test pull-kubernetes-integration |
…4-upstream-release-1.16 Automated cherry pick of #83184: Fix dynamic kubelet config init order
Fix init order during starup for dynamic kubelet config
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #83183
Special notes for your reviewer:
This is just a "proof of concept" fix in order to make it easier to understand the problem. From what I understand it does work, and all the tests pass, but I don't think the is the correct solution long term. If this makes sense and works, I guesswe can use this fix on the v1.16 branch to mitigate the problem.
The probelm is essentially that the sync function changed in the PR is executed before the kubelet is started and the metrics are "Created" with the way #81534 works.
If the metrics ain't created, the values will not be registered, and the
metrics will not be visible in the metric endpoint.
Feel free to change the priority; but since this breaks all the related tests, and the metrics are missing, I guess this is ok
/priority critical-urgent
/sig node
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: