improve procfs.PidOf return error #89718

xigang · 2020-04-01T06:30:44Z

improve procfs.PidOf return error

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
none
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

none

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

none

k8s-ci-robot · 2020-04-01T06:30:52Z

Hi @xigang. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

xigang · 2020-04-01T06:31:47Z

/ok-to-test

k8s-ci-robot · 2020-04-01T06:31:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xigang
To complete the pull request process, please assign yujuhong
You can assign the PR to them by writing /assign @yujuhong in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/kubelet/cm/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2020-04-01T06:32:00Z

@xigang: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

xigang · 2020-04-01T06:32:46Z

/assign @yujuhon

k8s-ci-robot · 2020-04-01T06:32:47Z

@xigang: GitHub didn't allow me to assign the following users: yujuhon.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @yujuhon

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.

yuzhiquan · 2020-04-01T06:51:46Z

/ok-to-test
/lgtm

yuzhiquan · 2020-04-01T06:56:24Z

/assign @yujuhong

xigang · 2020-04-01T07:40:17Z

/test pull-kubernetes-node-e2e-containerd
/test pull-kubernetes-e2e-kind

xigang · 2020-04-01T08:35:51Z

/test pull-kubernetes-e2e-kind

mattjmcnaughton

/lgtm

derekwaynecarr · 2020-04-06T21:39:24Z

pkg/kubelet/cm/container_manager_linux.go

-	pids, err2 := procfs.PidOf(name)
-	if err2 == nil {
+	pids, pidsErr := procfs.PidOf(name)
+	if pidsErr == nil {


seems like we could change this block and the one above to be like the following:

if pids, err := foo(name); err == nil { return pids }

otherwise err and err2 in this same function are equally descriptive...

@derekwaynecarr the modification is completed, please review again.

k8s-ci-robot · 2020-04-07T03:49:32Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2020-04-07T03:52:10Z

@xigang: Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

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.

xigang · 2020-04-07T04:11:05Z

/test pull-kubernetes-conformance-kind-ga-only-parallel

xigang · 2020-04-07T04:24:12Z

/retest

xigang · 2020-04-07T04:57:39Z

/test pull-kubernetes-node-e2e-containerd

xigang · 2020-04-07T06:17:44Z

/test pull-kubernetes-node-e2e-containerd

xigang · 2020-04-07T08:45:00Z

/test pull-kubernetes-node-e2e-containerd

xigang · 2020-04-07T10:20:02Z

/test pull-kubernetes-node-e2e-containerd

xigang · 2020-04-07T11:46:17Z

/retest

k8s-ci-robot · 2020-04-07T12:54:23Z

@xigang: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e-containerd	`e81f983`	link	`/test pull-kubernetes-node-e2e-containerd`

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.

xigang · 2020-04-07T13:19:38Z

@derekwaynecarr the pull-kubernetes-node-e2e-containerd test case fails:(

mattjmcnaughton · 2020-04-07T21:16:18Z

pkg/kubelet/cm/container_manager_linux.go

-	pids, err2 := procfs.PidOf(name)
-	if err2 == nil {
+	pids, err := procfs.PidOf(name)
+	if err == nil {


This stuff is tricky :) Re the comment on line 813, don't we want to log/return the error from getPidFromFile? When we assign err here, we overwrite that value.

I think, if err is defined inline, than its online available in the scope of the if statement - see https://www.calhoun.io/one-liner-if-statements-with-errors/, particularly "This is also why we can have an existing variable named err and then redeclare it inside of an if statement. The new variable is in a new scope, so it is permitted by the compiler.".

So I think we want to use the exact code @derekwaynecarr linked :)

fejta-bot · 2020-07-06T21:35:51Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

derekwaynecarr · 2020-08-04T15:32:50Z

Thank you for the proposed change.

It appears the change is not using the idiomatic style suggested here:
#89718 (comment)

I am closing this PR given it is stale, but feel free to re-open and make the requested change at a later time.

improve procfs.PidOf return value error

958d418

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 1, 2020

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 1, 2020

k8s-ci-robot requested review from derekwaynecarr and mtaufen April 1, 2020 06:32

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 1, 2020

k8s-ci-robot assigned yuzhiquan Apr 1, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2020

k8s-ci-robot assigned yujuhong Apr 1, 2020

mattjmcnaughton approved these changes Apr 1, 2020

View reviewed changes

k8s-ci-robot assigned mattjmcnaughton Apr 1, 2020

xigang mentioned this pull request Apr 1, 2020

REQUEST: New membership for xigang kubernetes/org#1765

Closed

6 tasks

derekwaynecarr suggested changes Apr 6, 2020

View reviewed changes

xigang added 2 commits April 7, 2020 11:35

Merge remote-tracking branch 'upstream/master' into dev

2ae0573

improve procfs.PidOf return error

e81f983

k8s-ci-robot added do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 7, 2020

xigang changed the title ~~improve procfs.PidOf return value error~~ improve procfs.PidOf return error Apr 7, 2020

mattjmcnaughton suggested changes Apr 7, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2020

derekwaynecarr closed this Aug 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve procfs.PidOf return error #89718

improve procfs.PidOf return error #89718

xigang commented Apr 1, 2020 •

edited

Loading

k8s-ci-robot commented Apr 1, 2020

xigang commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

xigang commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

yuzhiquan commented Apr 1, 2020

yuzhiquan commented Apr 1, 2020

xigang commented Apr 1, 2020

xigang commented Apr 1, 2020

mattjmcnaughton left a comment

derekwaynecarr Apr 6, 2020

xigang Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

xigang commented Apr 7, 2020

mattjmcnaughton Apr 7, 2020

fejta-bot commented Jul 6, 2020

derekwaynecarr commented Aug 4, 2020

improve procfs.PidOf return error #89718

improve procfs.PidOf return error #89718

Conversation

xigang commented Apr 1, 2020 • edited Loading

k8s-ci-robot commented Apr 1, 2020

xigang commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

xigang commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

yuzhiquan commented Apr 1, 2020

yuzhiquan commented Apr 1, 2020

xigang commented Apr 1, 2020

xigang commented Apr 1, 2020

mattjmcnaughton left a comment

Choose a reason for hiding this comment

derekwaynecarr Apr 6, 2020

Choose a reason for hiding this comment

xigang Apr 7, 2020

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

xigang commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

xigang commented Apr 7, 2020

mattjmcnaughton Apr 7, 2020

Choose a reason for hiding this comment

fejta-bot commented Jul 6, 2020

derekwaynecarr commented Aug 4, 2020

xigang commented Apr 1, 2020 •

edited

Loading