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

ws-daemon: align to decide if cgroup v2. #9390

Merged
merged 2 commits into from
Apr 19, 2022
Merged

ws-daemon: align to decide if cgroup v2. #9390

merged 2 commits into from
Apr 19, 2022

Conversation

utam0k
Copy link
Contributor

@utam0k utam0k commented Apr 19, 2022

Description

For some reason, the existing decision process was not working properly to determine using cgroup v2.

Related Issue(s)

Fixes #9389

How to test

Create the cgroup v2 with fusefs environment and run a workspace.
https://github.com/gitpod-io/workspace-preview/tree/to/fusefs

Release Notes

ws-daemon: align to decide if cgroup v2 to fix cgroup v2 with fuse

Documentation

No

@utam0k utam0k requested a review from a team April 19, 2022 03:28
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label Apr 19, 2022
if cgroups.Mode() == cgroups.Unified {
unified, err := cgroups.IsUnifiedCgroupSetup()
if err != nil {
return nil, status.Errorf(codes.FailedPrecondition, "could not determine cgroup setup")
Copy link
Contributor

Choose a reason for hiding this comment

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

should we include the error err in the message?

Copy link
Contributor

Choose a reason for hiding this comment

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

This error might be exposed to the user to whom it would be confusing.
We should log this instead.

@princerachit
Copy link
Contributor

@utam0k can you attach a screenshot where it shows that the issue is resolved, or maybe include a loom video so all reviewers don't have to explicitly test?

@roboquat roboquat added size/M and removed size/S labels Apr 19, 2022
@Furisto
Copy link
Member

Furisto commented Apr 19, 2022

@utam0k While discussing the cgroup issues Pavel had with his PVC work, we discovered that the method to evacuate the cgroup does not get called when using fuse. I have fixed that and added it to this PR.

Copy link
Contributor

@sagor999 sagor999 left a comment

Choose a reason for hiding this comment

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

lgtm
added to workspace cluster changelog

@roboquat roboquat merged commit d1ca55d into main Apr 19, 2022
@roboquat roboquat deleted the to/cgv2-fuse branch April 19, 2022 21:24
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note size/M team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cgroup v2 with fusefs doesn't work
6 participants