-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
⚠ logging: align to Kubernetes structured logging, add reconcileID #1827
⚠ logging: align to Kubernetes structured logging, add reconcileID #1827
Conversation
2266c83
to
cffcc49
Compare
cffcc49
to
21fcf2e
Compare
/test pull-controller-runtime-test-master I'm seeing similar failed tests on the periodic job: https://prow.k8s.io/job-history/gs/kubernetes-jenkins/logs/periodic-controller-runtime-test-master |
21fcf2e
to
1de701a
Compare
a027dd4
to
e177e92
Compare
e177e92
to
e34c179
Compare
e34c179
to
b8b84e7
Compare
@alvaroaleman Findings should be addressed or answered, PTAL. Thank you very much! |
Signed-off-by: Stefan Büringer [email protected]
b8b84e7
to
598978c
Compare
@sbueringer: 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/test-infra repository. I understand the commands that are listed here. |
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.
looks good to me, thanks for the work!
/hold
@joelanford @vincepri any concerns or objections?
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.
/approve
/lgtm
+1 from me to merge |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, sbueringer, vincepri 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 |
/hold cancel |
Thanks everyone for the quick feedback and good discussions! :) |
Signed-off-by: Stefan Büringer [email protected]
For more details, please see: #1826
This PR introduces the
LogConstructor
controller option which replaces the previousLog
option.LogConstructor
creates aLogger
on-demand either within the context of a reconciliation based on a *reconcile.Request or outside of it (then nil is passed in).Through this PR the logs change as follows:
When the controller is created with the builder:
When the controller is created with
controller.New
orcontroller.NewUnmanaged
:The output of the text format changed similarly. Although the objects are just formatted as
<namespace>/<name>
in this case (when using klog).Please note by providing a LogConstructor via controller options everything except the
reconcileID
can be overwritten.Fixes #1826