feat: temporarily remount read-only mounts before cleaning container fs #183
Conversation
This fixes #50 by temporary bind-mounting all readonly mounts within the MagicDir to keep them out of the way for kaniko. After kaniko finished it's build, the original mountpoints are restored at their original location. Signed-off-by: Jan Losinski <[email protected]>
internal/util/remount.go
Outdated
| tgt := filepath.Join("/", dest, src) | ||
| err := m.MkdirAll(tgt, 0o750) | ||
| if err != nil { | ||
| return remount, fmt.Errorf("create temp mountpoint %s: %w", dest, err) |
There was a problem hiding this comment.
Should we return nil for remount here? And should we perhaps restore the mounts on error?
There was a problem hiding this comment.
I'd prefer to leave the responsibility to the caller. Is there a strong reason to do it ourselves?
There was a problem hiding this comment.
I'd say the main reason is meeting expectations, i.e. returning an err and a non-nil value that is actionable can lead to some bad assumptions, especially if the behavior changes at some point.
It's OK to leave as-is, though.
internal/util/remount.go
Outdated
| "github.com/prometheus/procfs" | ||
| ) | ||
|
|
||
| // TempRemount iterates through all read-only mounted filesystems, bind-mounts them at dest, |
There was a problem hiding this comment.
This is an interesting question, should we or should we not limit this to read-only mounted filesystems? If we think about how a docker container is run with -v /path:/path, the contents of the docker container can never populate the mounted /path.
There was a problem hiding this comment.
Yeah this would actually make sense given the below behaviour:
$ echo 'important stuff' >> /opt/important-stuff/tps-reports.txt
$ ls -l /opt/important-stuff/tps-reports.txt
-rw-r--r-- 1 cian 16 May 13 21:12 /opt/important-stuff/tps-reports.txt
$ docker run -it --rm \
-v /opt/important-stuff:/opt/important-stuff \
-e GIT_URL=https://github.com/coder/envbuilder-starter-devcontainer \
-e INIT_SCRIPT=bash \
envbuilder:latest
[...]
devcontainer # ls -l /opt/important-stuff/
total 0
devcontainer # exit
$ cat /opt/important-stuff/tps-reports.txt
important stuff
There was a problem hiding this comment.
I might merge this as-is and investigate rw mounts in a follow up, if that's OK?
| @@ -394,6 +396,14 @@ func Run(ctx context.Context, options Options) error { | |||
| }) | |||
| } | |||
|
|
|||
| // temp move of all ro mounts | |||
| tempRemountDest := filepath.Join("/", MagicDir) | |||
| ignorePrefixes := []string{tempRemountDest, "/proc", "/sys"} | |||
There was a problem hiding this comment.
Should these be configurable?
Will we only ever support UNIX-based builds?
There was a problem hiding this comment.
Kaniko is currently Linux-only to the best of my knowledge.
internal/util/mock_mounter_test.go
Outdated
| // | ||
| // Generated by this command: | ||
| // | ||
| // mockgen -source ./internal/util/remount.go -package ebutil mounter |
There was a problem hiding this comment.
auto mockgen in Makefile as a side issue?
internal/util/mock_mounter_test.go
Outdated
| @@ -0,0 +1,98 @@ | |||
| // Code generated by MockGen. DO NOT EDIT. | |||
There was a problem hiding this comment.
Did you use the mock to have it tested without the OS? Maybe it is more natural to write a real Linux-only test.
There was a problem hiding this comment.
Tests that modify the underlying system are extremely obnoxious and tend to be flaky. I'd prefer to just keep the mock test for now. This still gets tested by the integration tests.
There was a problem hiding this comment.
This still gets tested by the integration tests
Ok, that answers my concerns 👍
This PR is a continuation of @janLo's original PR #51 with some additional changes:
Manual verification:
(Existing behaviour): On Docker runtime, dockerd fails to start after running:
(New behaviour): Using sysbox-runc runtime*, dockerd starts succesfully:
Existing sample devcontainer builds successfully on Docker runtime:
* Lima config used: https://gist.github.com/johnstcn/b9d6854b3aedb551af1a7588e2a5af94