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

[pvc] add init container to workspace to chown pvc mount folder #14096

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

sagor999
Copy link
Contributor

@sagor999 sagor999 commented Oct 21, 2022

Description

Adds init container when using PVC to ensure that PVC mounted folder has proper permissions (k8s mounts folder as root)

Related Issue(s)

Fixes #14003

How to test

Enable PVC
Open workspace.
ls -la / and verify that /workspace is owned by gitpod:gitpod now.

Release Notes

none

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=workspace
    Valid options are all, workspace, webapp, ide

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-14003.1 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-14003.2 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-14003.3 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-14003.4 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-14003.5 because the annotations in the pull request description changed
(with .werft/ from main)

@utam0k
Copy link
Contributor

utam0k commented Oct 24, 2022

/werft run with-preview

👍 started the job as gitpod-build-pavel-14003.9
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-14003.16 because the annotations in the pull request description changed
(with .werft/ from main)

@sagor999 sagor999 marked this pull request as ready for review October 25, 2022 18:48
@sagor999 sagor999 requested a review from a team October 25, 2022 18:48
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label Oct 25, 2022
@csweichel
Copy link
Contributor

What does that do to workspace startup time?
What alternatives to an init container exist?
How would that work in an airgapped environment?

@sagor999
Copy link
Contributor Author

What does that do to workspace startup time?

nothing. It executes almost instantenous (<1s), with only exception is a first image pull, which is fast since image is small and I believe we pack it into image for k3s as well.

What alternatives to an init container exist?

none that I can think of. :(
I guess it might be possible to do the same from workspace container itself? But when I tried to do same from even ring0, I would get permission denied, since workspace container does not run with actual root (AFAIK), yet k8s mounts that folder as root.

How would that work in an airgapped environment?

I presume we need to ensure we pack busybox image for airgapped envs. Do you have a better image that I can use? 🤔 I was looking for anything similar that we already use and could not find anything.

@csweichel

@utam0k
Copy link
Contributor

utam0k commented Oct 25, 2022

What alternatives to an init container exist?

How about using the IWS from ring0?

@sagor999
Copy link
Contributor Author

How about using the IWS from ring0?

IWS runs in ws-daemon right? ws-daemon does not have access to PVC. Only workspace container mounts it.

@utam0k
Copy link
Contributor

utam0k commented Oct 26, 2022

IWS runs in ws-daemon right? ws-daemon does not have access to PVC. Only workspace container mounts it.

I completely forgot about it 😭

@jenting
Copy link
Contributor

jenting commented Oct 26, 2022

I presume we need to ensure we pack busybox image for airgapped envs. Do you have a better image that I can use? 🤔 I was looking for anything similar that we already use and could not find anything.

We could build our own container image with chown.

Question: does ws-manager container image contains the chown binary?

@sagor999
Copy link
Contributor Author

Question: does ws-manager container image contains the chown binary?

I would be hesitant on trying to add wsmanager image into workspace pod. It feels potentially unsafe somehow. 🤔

@sagor999 sagor999 requested a review from a team October 27, 2022 21:42
@csweichel
Copy link
Contributor

csweichel commented Oct 27, 2022

IWS could indeed be a way to do this. Although it runs on the node, we call IWS from ring0 (e.g. PrepareForUserNS) where we could also correct the path.

Also, we assemble a new filesystem layout in ring1 which might give us the opportunity to fix permissions.

@csweichel
Copy link
Contributor

Depending on what this issue is blocking, we could also consider merging the change and look for a simpler solution immediately afterwards to remove the init container.

@sagor999
Copy link
Contributor Author

@csweichel it blocks roll out of PVC to end users. (as without this fix some workspaces might become broken due to permission issue on /workspace folder).

@kylos101
Copy link
Contributor

Depending on what this issue is blocking, we could also consider merging the change and look for a simpler solution immediately afterwards to remove the init container.

@csweichel leaving this PR unmerged blocks our ability to continue the rollout for PVC with regular workspaces. The feedback in #14003 indicates without this fix, it could break user workflows (there's a workaround, but we'd prefer to avoid needing it).

My preference would be to ship this as is, assuming init-container failures result in a failed workspace start that:

  1. we clearly log
  2. which the user can see as a failed workspace start

Why? That'll give us a chance to focus on learning from a gradual rollout, which could include alternate designs (removing the init container, for example).

@sagor999 if the init-container fails, what is the user experience like when starting the workspace? Do we log the error?

@sagor999
Copy link
Contributor Author

I am not sure how that init container can fail...
but if it would, it would result in workspace pod failing, which would show an error to user, something like "failed to start workspace".

Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

Depending on what this issue is blocking, we could also consider merging the change and look for a simpler solution immediately afterwards to remove the init container.

My preference is not to block this, LGTM.

@roboquat roboquat merged commit 8c36a94 into main Nov 2, 2022
@roboquat roboquat deleted the pavel/14003 branch November 2, 2022 05:20
@utam0k
Copy link
Contributor

utam0k commented Nov 2, 2022

@sagor999 @csweichel @jenting
chown is costly in terms of performance. I am concerned that this aspect has been verified.
Can we consider mounting to shiftfs instead of chown? It is just an idea.

@sagor999
Copy link
Contributor Author

sagor999 commented Nov 2, 2022

@utam0k it only chowns folder, it is not recursive. so it should be very cheap. or am I wrong?

@utam0k
Copy link
Contributor

utam0k commented Nov 2, 2022

@sagor999 Oh, I missed. You are right

it is not recursive.

@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Nov 9, 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-none size/S team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pvc] /workspace directory is owned by nobody
6 participants