[cgroupv2] Enable fuse device #8769

Furisto · 2022-03-11T17:34:45Z

Description

Enable fuse device on nodes that are using cgroup v2.

Related Issue(s)

How to test

Open workspace on a cgroup v2 system
Compile this program (gcc -o fuse_test fuse_test.c)

#define _GNU_SOURCE
#include <unistd.h>

#include <sys/syscall.h>
#include <linux/fs.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>


int main() {
  const char* src_path = "/dev/fuse";
  unsigned int flags = O_RDWR;
  printf("RET: %ld", syscall(SYS_openat, AT_FDCWD, src_path, flags));
}

fuse_test should return 3

Notes for reviewers

My initial implementation was using a runc facade like we discussed in #8471, but I switched to instead replacing the ebpf program that runc generates with our own.

No need to touch the host (copying the runtime facade, modifying the config of containerd or of other runtimes if we want to support them in the future)
No need to specify a runtime class when creating the workspace
Implementation uses the same approach as the cgroup v1 implementation
Less code

Release Notes

Enable the use of fuse device on cgroup v2 systems

Furisto · 2022-03-13T21:04:06Z

@csweichel Any idea why this fails in CI? I can build it with leeway in my workspace without issue.

utam0k · 2022-03-13T23:34:46Z

components/ws-daemon/pkg/daemon/cgroup_customizer.go

+func (c *CgroupCustomizerV1) WithCgroupBasePath(basePath string) {
 	c.cgroupBasePath = basePath
 }



I think we don't need this function now.

Suggested change

func (c *CgroupCustomizerV1) WithCgroupBasePath(basePath string) {

c.cgroupBasePath = basePath

}

utam0k · 2022-03-13T23:48:29Z

components/ws-daemon/pkg/daemon/cgroup_customizer.go

+
+	deviceRules := make([]*devices.Rule, 0)
+	deviceRules = append(deviceRules, &denyAll, &allowFuse)
+	for _, device := range specconv.AllowedDevices {


In our current use case, we don't specify cgroup devices. In other words, this is all we need to specify for now. However, if we want to add some devices about the runc layer(e.g. kubelet, contained) in the future, this part will arise a problem. However, it may be an option to decide that the cgroup devices are managed here. In fact, we have been used to overwrite /dev/fuse here.
@csweichel WDYT?

@Furisto Since there is currently a way to get the contents of config.json, why don't we get the original information from here? To be honest, I want to get status.json, but config.json should be sufficient.

gitpod/components/ws-daemon/pkg/container/containerd.go

Lines 460 to 470 in 60a9c55

func ExtractCGroupPathFromContainer(container containers.Container) (cgroupPath string, err error) {

var spec ocispecs.Spec

err = json.Unmarshal(container.Spec.Value, &spec)

if err != nil {

return

}

if spec.Linux == nil {

return "", xerrors.Errorf("container spec has no Linux section")

}

return spec.Linux.CgroupsPath, nil

}

Hey @utam0k , that sounds like an optimization we could potentially add later, yes? In other words, let's try to keep this PR smaller, ship the skateboard, and ship another one in the future (if we need).

Aside from this observation, do you have any other concerns which are blocking? I ask so that we can get @Furisto the feedback needed to have this approved and merged.

I did a little research and as it stands it shouldn't be a problem, because there is no API to change devices normally from k8s. However I think this should be improved. It can create some pretty strange bugs. The skateboard is a very good watchword 🛹

utam0k · 2022-03-13T23:50:23Z

Thanks for your PR. Super. I have two questions.

I wonder if the CgroupCustomizerV2 takes effect on the performance because ws-daemon loads eBPF code to kernel every 15 seconds.
I wonder if there may be a time when this override is not reflected for a few seconds since it is a dispatch.

components/ws-daemon/pkg/daemon/cgroup_customizer.go

Furisto · 2022-03-14T09:52:32Z

Thanks for your PR. Super. I have two questions.

I wonder if the CgroupCustomizerV2 takes effect on the performance because ws-daemon loads eBPF code to kernel every 15 seconds.

I wonder if there may be a time when this override is not reflected for a few seconds since it is a dispatch.

Are you thinking of the cpu limiter? The CgroupCustomizer should only be called when a new workspace is added.
It is possible but would there be any serious risk? We replace a more restrictive program with a less restrictive program. At worst the customer would not be able to use the fuse device some time after workspace start (or not at all if the event is somehow lost)

utam0k · 2022-03-14T10:15:17Z

@Furisto Thanks for your great reply.
Oh, I misunderstood 😭 There is no problem.

Are you thinking of the cpu limiter? The CgroupCustomizer should only be called when a new workspace is added.

In the worst case, I think the item of most concern is that none of the eBPF codes for the device controller is applied due to errors at load time. This can happen because the existing eBPF code is unloaded at load time. Was the eBPF code loaded in the pod cgroup?

It is possible but would there be any serious risk? We replace a more restrictive program with a less restrictive program. At worst the customer would not be able to use the fuse device some time after workspace start (or not at all if the event is somehow lost)

Furisto · 2022-03-15T13:59:17Z

@csweichel What do you think of this approach?

stale · 2022-03-26T20:24:44Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Furisto · 2022-03-29T09:25:14Z

/werft run

👍 started the job as gitpod-build-fo-fuse.9

Furisto · 2022-03-29T09:42:13Z

/werft run

👍 started the job as gitpod-build-fo-fuse.11

utam0k

Looks good

codecov · 2022-04-04T02:09:19Z

Codecov Report

Merging #8769 (a567595) into main (d469aa6) will decrease coverage by 4.97%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             main   #8769      +/-   ##
=========================================
- Coverage   12.31%   7.34%   -4.98%     
=========================================
  Files          20      32      +12     
  Lines        1161    2234    +1073     
=========================================
+ Hits          143     164      +21     
- Misses       1014    2067    +1053     
+ Partials        4       3       -1

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	`?`
install-installer-raw-app	`4.27% <ø> (?)`

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

Impacted Files	Coverage Δ
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go
...components/ws-manager/unpriviledged-rolebinding.go	`0.00% <0.00%> (ø)`
...nstall/installer/pkg/components/ws-manager/role.go	`0.00% <0.00%> (ø)`
...staller/pkg/components/ws-manager/networkpolicy.go	`0.00% <0.00%> (ø)`
...l/installer/pkg/components/ws-manager/configmap.go	`23.75% <0.00%> (ø)`
.../installer/pkg/components/ws-manager/deployment.go	`0.00% <0.00%> (ø)`
install/installer/pkg/common/objects.go	`0.00% <0.00%> (ø)`
install/installer/pkg/common/networkpolicies.go	`0.00% <0.00%> (ø)`
...installer/pkg/components/ws-manager/rolebinding.go	`0.00% <0.00%> (ø)`
... and 6 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

roboquat added do-not-merge/work-in-progress release-note size/L labels Mar 11, 2022

Enable fuse device on cgroup v2

306aacf

Furisto force-pushed the fo/fuse branch from 6636f9e to 306aacf Compare March 13, 2022 20:33

Furisto marked this pull request as ready for review March 13, 2022 21:02

Furisto requested a review from a team March 13, 2022 21:02

roboquat removed the do-not-merge/work-in-progress label Mar 13, 2022

github-actions bot added the team: workspace Issue belongs to the Workspace team label Mar 13, 2022

utam0k reviewed Mar 13, 2022

View reviewed changes

utam0k reviewed Mar 14, 2022

View reviewed changes

components/ws-daemon/pkg/daemon/cgroup_customizer.go Show resolved Hide resolved

stale bot added the meta: stale This issue/PR is stale and will be closed soon label Mar 26, 2022

kylos101 added the meta: never-stale This issue can never become stale label Mar 26, 2022

stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Mar 26, 2022

Ensure Gokart can load metadata

a567595

Furisto requested a review from a team March 29, 2022 09:38

github-actions bot added the team: delivery Issue belongs to the self-hosted team label Mar 29, 2022

mrsimonemms approved these changes Mar 29, 2022

View reviewed changes

utam0k approved these changes Apr 4, 2022

View reviewed changes

roboquat merged commit 3fb59ce into main Apr 4, 2022

roboquat deleted the fo/fuse branch April 4, 2022 02:09

roboquat added the deployed: workspace Workspace team change is running in production label Apr 4, 2022

janLo mentioned this pull request Sep 14, 2022

Draft: [bpf] respect container device cfg for allow list #12974

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cgroupv2] Enable fuse device #8769

[cgroupv2] Enable fuse device #8769

Furisto commented Mar 11, 2022 •

edited

Loading

Furisto commented Mar 13, 2022

utam0k Mar 13, 2022

utam0k Mar 13, 2022 •

edited

Loading

utam0k Mar 29, 2022

kylos101 Mar 31, 2022

utam0k Apr 4, 2022

utam0k commented Mar 13, 2022

Furisto commented Mar 14, 2022

utam0k commented Mar 14, 2022 •

edited

Loading

Furisto commented Mar 15, 2022 •

edited

Loading

stale bot commented Mar 26, 2022

Furisto commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

Furisto commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

utam0k left a comment

codecov bot commented Apr 4, 2022 •

edited by werft-gitpod-dev-com bot

Loading

	func (c *CgroupCustomizerV1) WithCgroupBasePath(basePath string) {
	c.cgroupBasePath = basePath
	}

	func ExtractCGroupPathFromContainer(container containers.Container) (cgroupPath string, err error) {
	var spec ocispecs.Spec
	err = json.Unmarshal(container.Spec.Value, &spec)
	if err != nil {
	return
	}
	if spec.Linux == nil {
	return "", xerrors.Errorf("container spec has no Linux section")
	}
	return spec.Linux.CgroupsPath, nil
	}

[cgroupv2] Enable fuse device #8769

[cgroupv2] Enable fuse device #8769

Conversation

Furisto commented Mar 11, 2022 • edited Loading

Description

Related Issue(s)

How to test

Notes for reviewers

Release Notes

Furisto commented Mar 13, 2022

utam0k Mar 13, 2022

Choose a reason for hiding this comment

utam0k Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

utam0k Mar 29, 2022

Choose a reason for hiding this comment

kylos101 Mar 31, 2022

Choose a reason for hiding this comment

utam0k Apr 4, 2022

Choose a reason for hiding this comment

utam0k commented Mar 13, 2022

Furisto commented Mar 14, 2022

utam0k commented Mar 14, 2022 • edited Loading

Furisto commented Mar 15, 2022 • edited Loading

stale bot commented Mar 26, 2022

Furisto commented Mar 29, 2022 • edited by werft-gitpod-dev-com bot Loading

Furisto commented Mar 29, 2022 • edited by werft-gitpod-dev-com bot Loading

utam0k left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 4, 2022 • edited by werft-gitpod-dev-com bot Loading

Codecov Report

Furisto commented Mar 11, 2022 •

edited

Loading

utam0k Mar 13, 2022 •

edited

Loading

utam0k commented Mar 14, 2022 •

edited

Loading

Furisto commented Mar 15, 2022 •

edited

Loading

Furisto commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

Furisto commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

codecov bot commented Apr 4, 2022 •

edited by werft-gitpod-dev-com bot

Loading