-
Notifications
You must be signed in to change notification settings - Fork 497
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
[noderesourcetopology] complete the contextual logging integration #725
[noderesourcetopology] complete the contextual logging integration #725
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani 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 |
✅ Deploy Preview for kubernetes-sigs-scheduler-plugins canceled.
|
/cc @PiotrProkop second and final part of the work begun in #710 |
everything but the last commit is final and ready for review. Still ironing out the last commit. |
339ec79
to
11b0b81
Compare
/hold I'm now happy with all the changes, but holding while my own final testing is ongoing |
/hold after further clarifications about the contextual logging |
6392841
to
f1d81da
Compare
finally ready. Further tunings will be deferred to future PRs. |
/hold cancel |
I'd be happy to squash commits as reviewers suggest |
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.
Great work! LGTM after squash :)
f1d81da
to
a65dba1
Compare
squashed and reformatted the commit message. |
Sorry for the delay! |
NP, need rebase anyway :) |
After more review and conversations, we have a better understanding of how integration with contextual logging should loom like. First and foremost, injecting loggers would conflict with the very goals of contextual logging. So, let's drop this code we added in kubernetes-sigs#710. The contextual logger doesn't do key/values deduplication. This is let to (some) backends. To avoid log clutter, trim down the extra key/value pairs and add only those we really need to ensure a good troubleshooting experience. Still let's make sure to add critical key/value pairs in the relevant entries, at cost of a possible duplication. When reporting the current assumed resources, the current representation is neither concise nor very human friendly. Additionally, multi-line log entries are harder to process and should be avoided. So let's move to a more concise representation, which turns out not obviously less human friendly and is no longer multiline. Review verbosiness of log entries. Move down to verbose=2 logs which are really key to understand the behavior. We should set a hard limit to log entries to minimize the log spam while keeping at least some observability without requiring v=4 or greater. The level v=4 is usually/often the highest-not-spammy log. When debug logs are needed we often set v=4, and higher verbosity levels are often used only in desperate times. Thus, promote to v=4 the debug logs we should really see. Everywhere else in the kubernetes ecosystem, and most notably in the scheduler, the pod namespace/name pair is called "pod", while we called it "logID". We do it to use the same name for all the flows, being the cache resync (which is driven by time, not by an object) the odd one. It seems better to be externally consistent (with the ecosystem) rather than internally consistent (all the flows in the same plugin), so we rename "logID" to "pod" in the log entries. Signed-off-by: Francesco Romani <fromani@redhat.com>
a65dba1
to
7098181
Compare
/lgtm |
before kubernetes-sigs#710 and kubernetes-sigs#725, we logged the container being processed alongside the pod (identified by namespace/name pair). It was dropped by mistake and not deliberately. This is useful information when troubleshooting, so let's add it back. Signed-off-by: Francesco Romani <fromani@redhat.com>
before kubernetes-sigs#710 and kubernetes-sigs#725, we logged the container being processed alongside the pod (identified by namespace/name pair). It was dropped by mistake and not deliberately. This is useful information when troubleshooting, so let's add it back. Signed-off-by: Francesco Romani <fromani@redhat.com>
before kubernetes-sigs#710 and kubernetes-sigs#725, we logged the container being processed alongside the pod (identified by namespace/name pair). It was dropped by mistake and not deliberately. This is useful information when troubleshooting, so let's add it back. Signed-off-by: Francesco Romani <fromani@redhat.com>
before kubernetes-sigs#710 and kubernetes-sigs#725, we logged the container being processed alongside the pod (identified by namespace/name pair). It was dropped by mistake and not deliberately. This is useful information when troubleshooting, so let's add it back. Signed-off-by: Francesco Romani <fromani@redhat.com> (cherry picked from commit 7a8afdf)
before kubernetes-sigs#710 and kubernetes-sigs#725, we logged the container being processed alongside the pod (identified by namespace/name pair). It was dropped by mistake and not deliberately. This is useful information when troubleshooting, so let's add it back. Signed-off-by: Francesco Romani <fromani@redhat.com>
before kubernetes-sigs#710 and kubernetes-sigs#725, we logged the container being processed alongside the pod (identified by namespace/name pair). It was dropped by mistake and not deliberately. This is useful information when troubleshooting, so let's add it back. Signed-off-by: Francesco Romani <fromani@redhat.com>
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This is the second and last part of the contextual logging/logr porting/log cleanup tracked in #709 and begun in #710 . Working on #710, addressing the review comments, and reviewing the changes in kube 1.28 and beyond (see #712) lead to the realization that some better integration is desirable. That's completed in this PR.
Which issue(s) this PR fixes:
Related to #709
Special notes for your reviewer:
Further minor tweaks of the log levels of some key messages are planned, but the vast majority of the work is considered done with this PR.
Does this PR introduce a user-facing change?