Skip to content
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

Deal with when cpu.cfs_quota_us is negative #8428

Merged
merged 7 commits into from
Feb 28, 2022
Merged

Deal with when cpu.cfs_quota_us is negative #8428

merged 7 commits into from
Feb 28, 2022

Conversation

utam0k
Copy link
Contributor

@utam0k utam0k commented Feb 24, 2022

Description

Deal with when cpu.cfs_quota_us is negative

Related Issue(s)

None

How to test

$ cd /workspace/gitpod/components/ws-daemon
$ go test -v ./...

Release Notes

Deal with when cpu.cfs_quota_us is negative

Documentation

No

@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #8428 (78720fc) into main (343ae26) will decrease coverage by 17.91%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #8428       +/-   ##
===========================================
- Coverage   32.85%   14.94%   -17.92%     
===========================================
  Files          33       49       +16     
  Lines        4745     4732       -13     
===========================================
- Hits         1559      707      -852     
- Misses       3068     3955      +887     
+ Partials      118       70       -48     
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
components-ws-daemon-app 21.17% <50.00%> (?)
components-ws-daemon-lib 21.17% <50.00%> (?)
components-ws-manager-app ?
install-installer-raw-app 4.58% <ø> (?)

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

Impacted Files Coverage Δ
components/ws-daemon/pkg/cpulimit/cfs.go 30.52% <50.00%> (ø)
components/ws-manager/pkg/manager/manager_ee.go
...s/ws-manager/pkg/manager/internal/grpcpool/pool.go
...-manager/pkg/manager/internal/workpool/workpool.go
components/ws-manager/pkg/clock/clock.go
components/ws-manager/pkg/manager/metrics.go
...omponents/ws-manager/pkg/manager/pod_controller.go
components/ws-manager/pkg/manager/probe.go
components/ws-manager/pkg/manager/monitor.go
components/ws-manager/pkg/manager/annotations.go
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 343ae26...78720fc. Read the comment docs.

@roboquat roboquat added size/L and removed size/M labels Feb 25, 2022
}

s := strings.TrimSpace(string(fc))
if s == "max" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@utam0k utam0k marked this pull request as ready for review February 25, 2022 08:19
@utam0k utam0k requested a review from a team February 25, 2022 08:19
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label Feb 25, 2022
@kylos101
Copy link
Contributor

Hey @utam0k ,

Thank you for your considerate PR! You rock. 😄

Is this happening in prod now? If yes, can you share a link to a log in GCP so I can inspect frequency?

What is the user experience like when it happens? I'm just curious.

PS: It's the weekend, no need to answer now, can do on Monday.

@kylos101
Copy link
Contributor

As this has no issue, I added it to our project, and assigned you, @utam0k .

@utam0k
Copy link
Contributor Author

utam0k commented Feb 26, 2022

Hey @utam0k ,

Thank you for your considerate PR! You rock. 😄

Is this happening in prod now? If yes, can you share a link to a log in GCP so I can inspect frequency?

What is the user experience like when it happens? I'm just curious.

PS: It's the weekend, no need to answer now, can do on Monday.

Hi @kylos101 I didn't create an issue because I thought it would be a quick fix, sorry.
Currently this is not an error, it is simply handled as an overflow. I have written a test and confirmed the situation with the debugging tools. It doesn't affect the user, because the overflow values can be compared with each other. I checked the kernel code, and it seems that all negative values represent infinity, but well, there is only -1 in effect. However, it's not very good and I want to fix it in this PR.

@utam0k
Copy link
Contributor Author

utam0k commented Feb 28, 2022

@sagor999 Thanks for your review. I made some comments and a commitment to addressing your feedback. PTAL.

@roboquat roboquat merged commit dade734 into main Feb 28, 2022
@roboquat roboquat deleted the to/cgroup-minus branch February 28, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note size/L team: workspace Issue belongs to the Workspace team
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants