chore(kubelet): migrate utils to contextual logging #130480
Conversation
6e6debd to
ccaed7e
Compare
|
/retest |
ccaed7e to
5f7d0ed
Compare
|
/retest |
5f7d0ed to
c0e0f8a
Compare
|
/assign bart0sh |
| @@ -73,7 +74,7 @@ type objectStore struct { | ||
| } | ||
|
|
||
| // NewObjectStore returns a new ttl-based instance of Store interface. | ||
| func NewObjectStore(getObject GetObjectFunc, clock clock.Clock, getTTL GetObjectTTLFunc, ttl time.Duration) Store { | ||
| func NewObjectStore(getObject GetObjectFunc, clock clock.Clock, getTTL GetObjectTTLFunc, ttl time.Duration, ctx context.Context) Store { |
There was a problem hiding this comment.
ctx is not used in this function.
| @@ -168,11 +169,11 @@ type objectCache struct { | ||
| lock sync.RWMutex | ||
| items map[objectKey]*objectCacheItem | ||
| stopped bool | ||
| ctx context.Context |
There was a problem hiding this comment.
Storing context is not a good practice, please try to avoid.
There was a problem hiding this comment.
Hmm, I see. So, some functions will include context.TODO() because they are part of an interface used by many kubelet components. Are we okay with that?
There was a problem hiding this comment.
It depends on how many. In this particular case it would probably make sense to store a logger instead of context.
There was a problem hiding this comment.
I'll switch to context.TODO instead of storing the context so you can see the difference. Then, we can decide on the best approach.
|
/triage accepted |
c0e0f8a to
01ffa69
Compare
01ffa69 to
56a66f2
Compare
| @@ -343,7 +344,10 @@ func (c *objectCache) Get(namespace, name string) (runtime.Object, error) { | ||
| if c.isImmutable(object) { | ||
| item.setImmutable() | ||
| if item.stop() { | ||
| klog.V(4).InfoS("Stopped watching for changes - object is immutable", "obj", klog.KRef(namespace, name)) | ||
| // TODO: it needs to be replaced by a proper context in the future | ||
| ctx := context.TODO() |
There was a problem hiding this comment.
We should place ctx := context.TODO() at the beginning of the function.
There was a problem hiding this comment.
Are you sure? This context is only being used for the log message here, so I thought it would be better to create the context only when needed
There was a problem hiding this comment.
If it's only used here, do you think the following approach would be better?
// TODO: replace context.TODO() with proper context
klog.FromContext(context.TODO()).V(4).Info("Stopped watching for changes - object is immutable", "obj", klog.KRef(namespace, name))
bba929b to
2bff882
Compare
2bff882 to
1bb2164
Compare
| @@ -34,9 +35,11 @@ import ( | ||
| // It uses /proc/stat first, which is more accurate, and falls back to the less accurate | ||
| // unix.Sysinfo if /proc/stat failed. | ||
| func GetBootTime() (time.Time, error) { | ||
| ctx := context.TODO() |
There was a problem hiding this comment.
Please, pass context from the upper layers, thanks!
| @@ -343,7 +344,8 @@ func (c *objectCache) Get(namespace, name string) (runtime.Object, error) { | ||
| if c.isImmutable(object) { | ||
| item.setImmutable() | ||
| if item.stop() { | ||
| klog.V(4).InfoS("Stopped watching for changes - object is immutable", "obj", klog.KRef(namespace, name)) | ||
| // TODO: it needs to be replaced by a proper context in the future | ||
| klog.FromContext(context.TODO()).V(4).Info("Stopped watching for changes - object is immutable", "obj", klog.KRef(namespace, name)) |
There was a problem hiding this comment.
The same here. Please, avoid usage of context.TODO() in migrated code. Pass it from upper layers instead, thanks!
These should preferably be fixed, unless there are strong reasons not to do it:
$ git grep '\(Background()\)\|\(TODO()\)' ./pkg/kubelet/util
pkg/kubelet/util/manager/cache_based_manager_test.go: return fakeClient.CoreV1().Secrets(namespace).Get(context.TODO(), name, opts)
pkg/kubelet/util/manager/watch_based_manager.go: klog.FromContext(context.TODO()).V(4).Info("Stopped watching for changes - object is immutable", "obj", klog.KRef(namespace, name))
pkg/kubelet/util/manager/watch_based_manager.go: ctx := context.TODO()
pkg/kubelet/util/manager/watch_based_manager_test.go: return fakeClient.CoreV1().Secrets(namespace).List(context.TODO(), opts)
pkg/kubelet/util/manager/watch_based_manager_test.go: return fakeClient.CoreV1().Secrets(namespace).Watch(context.TODO(), opts)
pkg/kubelet/util/nodelease.go: if node, err := c.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{}); err == nil {
1bb2164 to
f05ed4d
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omerap12 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 |
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
f05ed4d to
73afd04
Compare
| ctx := context.TODO() | ||
| logger := klog.FromContext(ctx) | ||
|
|
||
| pod, err := m.kubeClient.CoreV1().Pods(status.podNamespace).Get(ctx, status.podName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Let's not make massive changes to other components. This PR should concentrate on migrating kubelet/util + as little as possible changes in the upper layer.
| func IsTmpfsNoswapOptionSupported(mounter mount.Interface, mountPath string) bool { | ||
| isTmpfsNoswapOptionSupportedHelper := func() bool { | ||
| func IsTmpfsNoswapOptionSupported(logger klog.Logger, mounter mount.Interface, mountPath string) bool { | ||
| // TODO: it needs to be replaced by a proper context in the future |
There was a problem hiding this comment.
this comment doesn't seem to be needed anymore.
| @@ -121,7 +122,8 @@ func isSwapOnAccordingToProcSwaps(procSwapsContent []byte) bool { | ||
| // IsSwapOn detects whether swap in enabled on the system by inspecting | ||
| // /proc/swaps. If the file does not exist, an os.NotFound error will be returned. | ||
| // If running on windows, swap is assumed to always be false. | ||
| func IsSwapOn() (bool, error) { | ||
| func IsSwapOn(logger klog.Logger) (bool, error) { | ||
| // TODO: it needs to be replaced by a proper context in the future |
|
@omerap12 This PR differs from the others in the series as it passes logger around instead of context. Other PRs mostly pass ctx to at least public interfaces and use logger at the lowest level, usually in a local functions that most probably would never use context. I'm not insisting, but it would make the series more consistent if we consider passing ctx to at least public interfaces. |
Hmm, that seems reasonable to me. I initially thought it might be better to pass only the logger since the function only uses the context for logging, but I see your point. I'll make the adjustment. |
|
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:
Which issue(s) this PR fixes:
Part of #130069
Special notes for your reviewer:
Since some functions are part of interfaces, I had to modify other areas as well (such as pkg/kubelet/config/config.go, etc.).
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: