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
Migrate pkg/kubelet/cm/devicemanager to structured logging #99976
Conversation
@@ -424,13 +424,13 @@ func (m *ManagerImpl) Register(ctx context.Context, r *pluginapi.RegisterRequest | |||
} | |||
if !versionCompatible { | |||
errorString := fmt.Sprintf(errUnsupportedVersion, r.Version, pluginapi.SupportedVersions) | |||
klog.Infof("Bad registration request from device plugin with resource name %q: %s", r.ResourceName, errorString) | |||
klog.InfoS("Bad registration request from device plugin with resource.", "resource", r.ResourceName, "error", errorString) | |||
return &pluginapi.Empty{}, fmt.Errorf(errorString) |
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.
Let's pass the error from fmt.Errorf(errorString)
directly to InfoS under err
key
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.
Is it required for all ocurrences of err on InfoS?
klog.Errorf("Container device %s has conflicting mapping host devices: %s and %s", | ||
dev.ContainerPath, d, dev.HostPath) | ||
klog.ErrorS(nil, "Container device has conflicting mapping host devices.", | ||
"containerPath", dev.ContainerPath, "value1", d, "value2", dev.HostPath) |
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.
"containerPath", dev.ContainerPath, "value1", d, "value2", dev.HostPath) | |
"path", dev.ContainerPath, "got", d, "expected", dev.HostPath) |
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.
we are using "containerPath" as a standard on these files
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.
some comments, also there is inconsistent period use throughout would suggest dropping them all.
this pr is huge, thank you for going through this!
55b54a0
to
8e66485
Compare
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.
Mostly LGTM, remaining issue is the trailing periods on log lines.
(note: I didn't comment on all the log lines ending in periods, just a few---please use search to replace) /milestone v1.21 |
209bfd3
to
cf6a1b0
Compare
/retest |
/test pull-kubernetes-unit |
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.
Went through allllllll comments 😆 and found 1 thing below.
Also can you double check these two open comments since there's a difference between the expected and gots between your version and the reviewer:
#99976 (comment)
#99976 (comment)
Thank you so much for this pr @knabben It's a beast 😸
cf6a1b0
to
d0ffdc0
Compare
d0ffdc0
to
c1d24c8
Compare
/retest |
Awesome job @knabben ! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knabben, mrunalp 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 |
What type of PR is this?
/kind feature
/sig instrumentation
/sig node
What this PR does / why we need it:
Migrate
pkg/kubelet/cm/devicemanager
to structured loggingWhich issue(s) this PR fixes:
Refs #98976
Special notes for your reviewer:
Does this PR introduce a user-facing change?