fix: Walkfunc should check the err

[j4ckstraw]

Signed-off-by: j4ckstraw [email protected]

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

fix https://github.com/koordinator-sh/koordinator/issues/459

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

• [ ] I have written necessary docs and comments
• [ ] I have added necessary unit tests and integration tests
• [x] All checks passed in make test

[j4ckstraw]

fix #459

[codecov[bot]]

Codecov Report

Merging #460 (420dc65) into main (aaead0f) will decrease coverage by 0.02%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #460      +/-   ##
==========================================
- Coverage   67.97%   67.95%   -0.03%     
==========================================
  Files         155      155              
  Lines       16576    16579       +3     
==========================================
- Hits        11268    11266       -2     
- Misses       4474     4478       +4     
- Partials      834      835       +1     

Flag	Coverage Δ
unittests	67.95% <0.00%> (-0.03%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/koordlet/resmanager/cpu_suppress.go	72.34% <0.00%> (-0.59%)	:arrow_down:
pkg/util/httputil/reverseproxy.go	84.84% <0.00%> (-0.54%)	:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[saintube]

@j4ckstraw Could you please add a unit test for this fix?

[j4ckstraw]

@j4ckstraw Could you please add a unit test for this fix?

Let me try

[jasonliu747]

@j4ckstraw any progress with this PR?

[j4ckstraw]

@j4ckstraw any progress with this PR?

sorry, I am busy recently.

BYW, Is it really need a test?

[jasonliu747]

BYW, Is it really need a test?

Actually no, but we would love to see the code coverage getting higher. We can still merge this PR if you want, it's your call, please let me know.

[saintube]

@j4ckstraw any progress with this PR?

sorry, I am busy recently.

BYW, Is it really need a test?

We will appreciate it if you can make a testable patch and help us avoid regression.

[j4ckstraw]

I have a problem when try to write a test for this function.

I know that if the path which stat call is not found, the Walk function will return error. BUT rootCgroupPath is checked before walk, so I can't reproduce this error conveniently. https://github.com/koordinator-sh/koordinator/blob/main/pkg/koordlet/resmanager/cpu_suppress.go#L78

Now I can reproduce only in race condition, I have no idea to write a test for it. help!

[jasonliu747]

Hi @j4ckstraw, please rebase to the latest code. Codecov won't be a problem anymore, we can merge this PR then.

[jasonliu747]

@j4ckstraw have you joined our DingTalk or WeChat group? I would like to discuss further cooperation with you~

[j4ckstraw]

@j4ckstraw have you joined our DingTalk or WeChat group? I would like to discuss further cooperation with you~

Not yet, It would be my pleasure.

[jasonliu747]

Not yet, It would be my pleasure.

Nice! You can find our DingTalk QR Code in README

[j4ckstraw]

Not yet, It would be my pleasure.

Nice! You can find our DingTalk QR Code in README Got it, I have joined in it.

[hormes]

/approve

[koordinator-bot[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hormes

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:

• ~~OWNERS~~ [hormes]

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