Skip to content

fix(remount): ensure mountpoint is a file for files #249

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

Merged
merged 6 commits into from
Jun 26, 2024

Conversation

maxbrunet
Copy link
Contributor

Fixes errors like:

error: temp remount: temp remount: bind mount /usr/bin/nvidia-smi => /.envbuilder/mnt/usr/bin/nvidia-smi: not a directory

Follow up to #183

@maxbrunet maxbrunet changed the title efix(remount): create parent directory for files fix(remount): create parent directory for files Jun 25, 2024
@maxbrunet maxbrunet marked this pull request as draft June 25, 2024 14:53
@matifali matifali requested review from johnstcn and removed request for johnstcn June 25, 2024 15:44
@maxbrunet maxbrunet force-pushed the fix/remount/mount-bind-files branch 2 times, most recently from 8df37bb to 4d94484 Compare June 25, 2024 15:50
@maxbrunet maxbrunet force-pushed the fix/remount/mount-bind-files branch from 4d94484 to 7ecee4a Compare June 25, 2024 15:51
@maxbrunet maxbrunet marked this pull request as ready for review June 25, 2024 15:55
johnstcn
johnstcn previously approved these changes Jun 25, 2024
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@maxbrunet
Copy link
Contributor Author

Apparently that's not enough, maybe files need extra flags or something, I'll try to investigate:

error: temp remount: temp remount: bind mount /usr/bin/nvidia-smi => /.envbuilder/mnt/usr/bin/nvidia-smi: no such file or directory

@maxbrunet maxbrunet marked this pull request as draft June 25, 2024 18:44
@johnstcn johnstcn dismissed their stale review June 25, 2024 18:50

research

@johnstcn
Copy link
Member

Apparently that's not enough, maybe files need extra flags or something, I'll try to investigate:

error: temp remount: temp remount: bind mount /usr/bin/nvidia-smi => /.envbuilder/mnt/usr/bin/nvidia-smi: no such file or directory

What are you trying to accomplish exactly? Can you provide steps to reproduce?

@maxbrunet maxbrunet changed the title fix(remount): create parent directory for files fix(remount): ensure mountpoint is a file for files Jun 25, 2024
@maxbrunet
Copy link
Contributor Author

maxbrunet commented Jun 25, 2024

Fixed. The file mountpoint must exist

The main use case would probably usage of pod.spec.containers.volumeMounts.subPath where the subPath is a file and readOnly is also set.

But I'm experimenting with GPUs, the NVIDIA container runtime mounts a lot of things. Now I'm back to the known issue with Kaniko

pod.yaml
apiVersion: v1
kind: Pod
metadata:
  name: envbuilder 
spec:
  containers:
  - name: envbuilder
    image: ghcr.io/coder/envbuilder-preview
    env:
    - name: FALLBACK_IMAGE
      value: busybox
    - name: INIT_SCRIPT
      value: sh 
    resources:
      limits:
        nvidia.com/gpu: "1"
    securityContext:
      privileged: true

These paths need privileges to be remounted, I am thinking ignorePrefixes should be an extension of ignorePaths since there are ignored, it should not be needed to remount them. I can try that in a separate PR

ignorePrefixes := []string{tempRemountDest, "/proc", "/sys"}

Edit: I have a workaround for the Kaniko issue, I'll open a PR for that too

@maxbrunet maxbrunet marked this pull request as ready for review June 25, 2024 21:33
@maxbrunet maxbrunet requested a review from johnstcn June 25, 2024 22:07
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

I added a couple of changes in a separate PR:

  • Modified our existing integration test to correctly reproduce the issue you mention (we weren't mounting RO) 734cefc
  • Added IgnorePaths when performing temp remount 20c0145

Let me know if this looks good to you! If you're happy, you can cherry-pick those commits into this PR or we can continue with the other one!

@johnstcn johnstcn self-assigned this Jun 26, 2024
Comment on lines +402 to +404
if options.LayerCacheDir != "" {
ignorePaths = append(ignorePaths, options.LayerCacheDir)
}
Copy link
Member

Choose a reason for hiding this comment

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

ah, nice catch 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe yes, it was ignoring everything 😅

@maxbrunet
Copy link
Contributor Author

Thank you @johnstcn, this looks good to me. There is more to consider for NVIDIA and I am not sure ignorePaths will solve everything, but I'll get into the specifics on the related issue, I don't want to sidetrack this PR too much :)

@johnstcn johnstcn merged commit 8c5c50c into coder:main Jun 26, 2024
4 checks passed
@johnstcn
Copy link
Member

Thanks for the collaboration @maxbrunet!

@maxbrunet maxbrunet deleted the fix/remount/mount-bind-files branch June 26, 2024 15:16
@guidogagl
Copy link

Apparently that's not enough, maybe files need extra flags or something, I'll try to investigate:

error: temp remount: temp remount: bind mount /usr/bin/nvidia-smi => /.envbuilder/mnt/usr/bin/nvidia-smi: no such file or directory

Hi, how did you solve this? I'm running into the same issue at the moment and I'm stuck :S

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants