-
Notifications
You must be signed in to change notification settings - Fork 4.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
ignore wait4 errs in observe max-err-count #18992
ignore wait4 errs in observe max-err-count #18992
Conversation
6737f17
to
7011f02
Compare
pkg/oc/cli/cmd/observe/observe.go
Outdated
@@ -716,6 +718,13 @@ func measureCommandDuration(m *prometheus.SummaryVec, fn func() error, labels .. | |||
statusCode = -1 | |||
} | |||
m.WithLabelValues(append(labels, strconv.Itoa(statusCode))...).Observe(float64(duration / time.Millisecond)) | |||
|
|||
if err != nil && err.Error() == errNoChildProc { |
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.
I'd rather not hardcode the value like that. See if you can get some reasonable information from the error, when being cast to exec.ExitError. I think you should be able to get ProcessState.Sys
which 'returns system-dependent exit information about the process. Convert it to the appropriate underlying type, such as syscall.WaitStatus on Unix, to access its contents. ' from there you should be able to get some system-wide constant.
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.
agree, this isn't a reasonable fix
7011f02
to
ff682d3
Compare
ff682d3
to
a3f8ff7
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.
One nit and you're good to go.
pkg/oc/cli/cmd/observe/observe.go
Outdated
@@ -716,9 +716,28 @@ func measureCommandDuration(m *prometheus.SummaryVec, fn func() error, labels .. | |||
statusCode = -1 | |||
} | |||
m.WithLabelValues(append(labels, strconv.Itoa(statusCode))...).Observe(float64(duration / time.Millisecond)) | |||
|
|||
errno := errnoError(err) | |||
if errno == syscall.ECHILD { |
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.
Since you've already created that method just:
if errnoError(err) == syscall.ECHILD {
...
a3f8ff7
to
f28463a
Compare
@soltysh thanks, comment addressed |
f28463a
to
0e0ee60
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.
One more nit.
pkg/oc/cli/cmd/observe/observe.go
Outdated
return err | ||
} | ||
|
||
func errnoError(err error) syscall.Errno { | ||
if err == nil { |
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.
How about:
if se, ok := err.(*os.SyscallError); ok {
if errno, ok := se.Err.(syscall.Errno); ok {
return errno
}
}
return 0
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.
thanks, done
0e0ee60
to
7565f7d
Compare
/test gcp |
1 similar comment
/test gcp |
@juanvallejo: 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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juanvallejo, soltysh 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 |
/retest |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 18953, 18992). |
Fixes #17743
These errors should not count against the --maximum-errors count
as the process has cleanly run and exited before wait4 syscall is made.
cc @smarterclayton @soltysh