chore(kubelet): migrate config to contextual logging #130581
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhifei92 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cc @bart0sh |
e749fb1 to
24b2024
Compare
|
/assign |
| @@ -99,9 +99,12 @@ func (c *PodConfig) SeenAllSources(seenSources sets.Set[string]) bool { | |||
| if c.pods == nil { | |||
| return false | |||
| } | |||
| // Use klog.TODO() because we currently do not have a proper context to pass in. | |||
| // Replace this with an appropriate logger when refactoring this function to accept a logger parameter. | |||
| logger := klog.TODO() | |||
There was a problem hiding this comment.
Does this one require many changes in other components? If not, please consider passing context to this function.
There was a problem hiding this comment.
Yes, it involves more than 10 modifications, but most of them are in test files. What would be a good approach in this situation?
pkg/kubelet/kubelet.go: sourcesReady: config.NewSourcesReady(kubeDeps.PodConfig.SeenAllSources),
pkg/kubelet/volumemanager/volume_manager_test.go: sourcesReady := config.NewSourcesReady(func(_ sets.Set[string]) bool { return true })
pkg/kubelet/volumemanager/volume_manager_test.go: sourcesReady := config.NewSourcesReady(func(_ sets.Set[string]) bool { return true })
pkg/kubelet/volumemanager/volume_manager_test.go: sourcesReady := config.NewSourcesReady(func(_ sets.Set[string]) bool { return true })
pkg/kubelet/volumemanager/volume_manager_test.go: sourcesReady := config.NewSourcesReady(func(_ sets.Set[string]) bool { return true })
pkg/kubelet/volumemanager/volume_manager_test.go: sourcesReady := config.NewSourcesReady(func(_ sets.Set[string]) bool { return true })
pkg/kubelet/config/sources.go:// NewSourcesReady returns a SourcesReady with the specified function.
pkg/kubelet/config/sources.go:func NewSourcesReady(sourcesReadyFn SourcesReadyFn) SourcesReady {
pkg/kubelet/kubelet_test.go: kubelet.sourcesReady = config.NewSourcesReady(func(_ sets.Set[string]) bool { return true })
pkg/kubelet/kubelet_test.go: kubelet.sourcesReady = config.NewSourcesReady(func(_ sets.Set[string]) bool { return ready })
pkg/kubelet/pluginmanager/plugin_manager_test.go: sourcesReady := config.NewSourcesReady(func(_ sets.Set[string]) bool { return true })
pkg/kubelet/cm/devicemanager/manager_test.go: sourcesReady := config.NewSourcesReady(func(_ sets.Set[string]) bool { return true })
There was a problem hiding this comment.
Yes, it would be a good approach. Please proceed with the changes, thanks!
There was a problem hiding this comment.
However, sourcesReadyFn is called by AllReady(), and modifying AllReady() would involve significant changes.
// AllReady returns true if each configured source is ready.
func (s *sourcesImpl) AllReady() bool {
s.lock.RLock()
defer s.lock.RUnlock()
return s.sourcesReadyFn(s.sourcesSeen)
}
call AllReady()
-> % grep -rE ".AllReady()" pkg/kubelet
pkg/kubelet/kubelet.go: if !kl.sourcesReady.AllReady() {
pkg/kubelet/kubelet.go: if !kl.sourcesReady.AllReady() {
pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go: done := sourcesReady.AllReady()
pkg/kubelet/config/sources.go: // AllReady returns true if the currently configured sources have all been seen.
pkg/kubelet/config/sources.go: AllReady() bool
pkg/kubelet/config/sources.go:// AllReady returns true if each configured source is ready.
pkg/kubelet/config/sources.go:func (s *sourcesImpl) AllReady() bool {
pkg/kubelet/container/container_gc.go: // AllReady returns true if the currently configured sources have all been seen.
pkg/kubelet/container/container_gc.go: AllReady() bool
pkg/kubelet/container/container_gc.go: return cgc.runtime.GarbageCollect(ctx, cgc.policy, cgc.sourcesReadyProvider.AllReady(), false)
pkg/kubelet/container/container_gc.go: return cgc.runtime.GarbageCollect(ctx, cgc.policy, cgc.sourcesReadyProvider.AllReady(), true)
pkg/kubelet/container/testing/fake_ready_provider.go:// AllReady notifies caller that the Fake Provider is ready.
pkg/kubelet/container/testing/fake_ready_provider.go:func (frp *FakeReadyProvider) AllReady() bool {
pkg/kubelet/cm/cpumanager/cpu_manager.go:func (s *sourcesReadyStub) AllReady() bool { return true }
pkg/kubelet/cm/cpumanager/cpu_manager.go: if !m.sourcesReady.AllReady() {
pkg/kubelet/cm/devicemanager/manager.go:func (s *sourcesReadyStub) AllReady() bool { return true }
pkg/kubelet/cm/devicemanager/manager.go: if !m.sourcesReady.AllReady() {
pkg/kubelet/cm/devicemanager/manager.go: if !m.sourcesReady.AllReady() && m.isContainerAlreadyRunning(podUID, contName) {
pkg/kubelet/cm/memorymanager/memory_manager.go:func (s *sourcesReadyStub) AllReady() bool { return true }
pkg/kubelet/cm/memorymanager/memory_manager.go: if !m.sourcesReady.AllReady() {
pkg/kubelet/cm/dra/manager.go: if m.sourcesReady == nil || !m.sourcesReady.AllReady() {
There was a problem hiding this comment.
OK, let's not do that. At least we tried :)
/lgtm
|
LGTM label has been added. Git tree hash: 4ac9eda31139bf26c37b2408fb39c6d96d477783
|
|
/assign @mrunalp @SergeyKanzhelev |
|
/cc @bart0sh |
24b2024 to
e95c53b
Compare
|
New changes are detected. LGTM label has been removed. |
|
@zhifei92: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
|
PR needs rebase. 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-sigs/prow repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
migrate
kubelet/configto contextual loggingWhich issue(s) this PR fixes:
Fixes #
part of #130069
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: