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] Umount dangling mask mounts #6733

Merged
merged 1 commit into from
Nov 19, 2021
Merged

[ws-daemon] Umount dangling mask mounts #6733

merged 1 commit into from
Nov 19, 2021

Conversation

csweichel
Copy link
Contributor

@csweichel csweichel commented Nov 16, 2021

Description

This PR unmounts mask mounts used during proc and sysfs mount preparation.

Related Issue(s)

Fixes #6732

How to test

  1. Start a workspace
  2. Look at the ws-daemon filesystem and ensure there are no dangling tmpfs mounts in /tmp/staging-*
  3. Ensure that you cannot unmount any of the /proc masks, e.g. sched_debug
  4. Run docker run --rm -it alpine:latest umount /proc/sched_debug which should fail with a permission denied error
  5. Check there are no errors in ws-daemon

Release Notes

[ws-daemon] Fix resource leak during proc mounts

Documentation

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #6733 (bd1a252) into main (87e44ed) will decrease coverage by 2.37%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6733      +/-   ##
==========================================
- Coverage   19.04%   16.67%   -2.38%     
==========================================
  Files           2       25      +23     
  Lines         168     3244    +3076     
==========================================
+ Hits           32      541     +509     
- Misses        134     2641    +2507     
- Partials        2       62      +60     
Flag Coverage Δ
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.96% <ø> (?)
components-ws-daemon-lib 21.96% <ø> (?)
installer-raw-app 6.16% <ø> (?)

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

Impacted Files Coverage Δ
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go
components/ws-daemon/pkg/quota/xfs.go 45.05% <0.00%> (ø)
components/ws-daemon/pkg/internal/session/store.go 19.38% <0.00%> (ø)
components/ws-daemon/pkg/content/config.go 33.33% <0.00%> (ø)
...staller/pkg/components/ws-manager/networkpolicy.go 0.00% <0.00%> (ø)
installer/pkg/components/ws-manager/role.go 0.00% <0.00%> (ø)
installer/pkg/common/render.go 0.00% <0.00%> (ø)
...onents/ws-daemon/pkg/internal/session/workspace.go 50.73% <0.00%> (ø)
components/ws-daemon/pkg/content/archive.go 57.95% <0.00%> (ø)
... and 17 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 87e44ed...bd1a252. Read the comment docs.

@csweichel csweichel marked this pull request as ready for review November 16, 2021 16:28
@csweichel
Copy link
Contributor Author

/hold

@csweichel
Copy link
Contributor Author

We also need to umount the proc/sysfs again thanks to CLONE_TREE

@aledbf
Copy link
Member

aledbf commented Nov 16, 2021

/lgtm
/approve

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 33ee4ee75b4f304cdea362f6c73c485df8ba1e70

@csweichel
Copy link
Contributor Author

This change - as simple as it may seem - causes all sorts of problems:

  1. breaks kubectl logs ws-dameon-...
  2. breaks proc mounts for Docker container within the workspace

@csweichel csweichel force-pushed the cw/fix-6732 branch 3 times, most recently from 9c7470e to be502d4 Compare November 19, 2021 09:00
@csweichel csweichel marked this pull request as ready for review November 19, 2021 09:22
@csweichel
Copy link
Contributor Author

/hold cancel

@aledbf
Copy link
Member

aledbf commented Nov 19, 2021

/lgtm

@roboquat roboquat added the lgtm label Nov 19, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 70af2f17b56a6b5df03aec4d0987cafdf2fb6b62

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf

Associated issue: #6732

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:

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

@roboquat roboquat merged commit 5a566b9 into main Nov 19, 2021
@roboquat roboquat deleted the cw/fix-6732 branch November 19, 2021 10:28
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved 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.

[ws-daemon] proc mount leaks masking tmpfs mounts
3 participants