-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Let GC print specific message for RESTMapping failure #42862
Conversation
@caesarxuchao yes, watchable discovery is on my list for 1.7. |
@@ -143,6 +143,12 @@ func (gc *GarbageCollector) attemptToDeleteWorker() bool { | |||
} | |||
err := gc.attemptToDeleteItem(n) | |||
if err != nil { | |||
// TODO: remove this block when gc starts using dynamic RESTMapper. | |||
if restMappingError, ok := err.(*restMappingError); ok { | |||
glog.Warning(restMappingError) |
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.
What about utilruntime.HandleError
? You could (probably do?) wire that up for outside monitoring.
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.
Done.
version string | ||
} | ||
|
||
func (r *restMappingError) Error() string { |
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.
Mind making a short Error
and a long Message
or some such? The detail is great in a log, less great if it ever escaped.
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.
Done.
two optional nits. lgtm otherwise. |
5822fc9
to
d7aef0a
Compare
@deads2k comments addressed. PTAL. Thanks. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: caesarxuchao, deads2k Needs approval from an approver in each of these OWNERS Files:
We suggest the following people: |
Automatic merge from submit-queue (batch tested with PRs 38805, 42362, 42862) |
Disable garbage collector to avoid spamming the logs with repeated error messages. A mitigation is coming in k8s 1.6 (kubernetes/kubernetes#42862), and a proper fix is planned for k8s 1.7 (kubernetes/kubernetes#42615). This option can be reverted once bootkube is updated to use k8s >= 1.6.
Make the error messages reported in #39816 to be more specific, also only print the message once.
I'll also update the garbage collector's doc to clearly state we don't support tpr yet.
We'll wait for the watchable discovery feature (@sttts are you going to work on that?) to land in 1.7, and then enable the garbage collector to handle TPR.
cc @hongchaodeng @MikaelCluseau @djMax