mount: ensure empty volume paths exist for copy-up #3468
Conversation
euank
force-pushed
the
rkt-mkdir-implicit-mount
branch
from
9f027c4 to
2f61bdf
Compare
|
lgtm |
|
Alternative fixes include:
|
|
I think this is the right fix for now and we can change the CopyTree behaviour separately. |
|
Well, based on the spectacular test failures, I'm wrong about something. I'll add more debug output and figure out with more confidence what was broken. |
|
breaks into profuse sweat |
Before this change, the `copy-up` operation could fail with: `stage1: could not prepare mountpoint: mkdir sharedVolumes/foo: no such file or directory` This would occur because `CopyTree` does not implicitly create the target directory, so we must create it prior to calling.
euank
force-pushed
the
rkt-mkdir-implicit-mount
branch
from
2f61bdf to
25284c0
Compare
|
Actual bug was these lines being missed when copying over to per-app-stuff: https://github.com/coreos/rkt/blob/cdb6a446c8ed064bfe96c845330cd05d3d9df0b6/stage1/init/common/pod.go#L467-L473 It was |
| @@ -342,6 +350,7 @@ func AppAddMounts(p *stage1commontypes.Pod, ra *schema.RuntimeApp, enterCmd []st | |||
| log.FatalE("Unable to setup app mounts", err) | |||
There was a problem hiding this comment.
TODO in a followup PR; make everything here return err instead of FatalE-ing
|
whoops, good find! lgtm |
| @@ -300,7 +300,15 @@ func ensureDestinationExists(source, destination string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func AppAddMounts(p *stage1commontypes.Pod, ra *schema.RuntimeApp, enterCmd []string) { | |||
| func AppAddMounts(p *stage1commontypes.Pod, ra *schema.RuntimeApp, enterCmd []string) error { | |||
| sharedVolPath := common.SharedVolumesPath(p.Root) | |||
There was a problem hiding this comment.
this is called in two places, honestly this whole thing should probably be in common. maybe as sharedVolPath, err := common.InitSharedVolumePath()
|
I did a bit more refactoring. I pushed the duplicated shared volume path creation up to If the tests are happy with this change then it should be fine to merge. |
| if err := os.MkdirAll(sharedVolPath, SharedVolumePerm); err != nil { | ||
| return "", errwrap.Wrap(errors.New("could not create shared volumes directory"), err) | ||
| } | ||
| if err := os.Chmod(sharedVolPath, SharedVolumePerm); err != nil { |
There was a problem hiding this comment.
I guess we Chmod because the directory might already be present and in this case not catched the perm parm in MkdirAll? It may seem obvious, but can we leave a small comment for the rationale?
| func AppAddMounts(p *stage1commontypes.Pod, ra *schema.RuntimeApp, enterCmd []string) { | ||
| func AppAddMounts(p *stage1commontypes.Pod, ra *schema.RuntimeApp, enterCmd []string) error { | ||
| sharedVolPath, err := common.CreateSharedVolumesPath(p.Root) | ||
|
|
There was a problem hiding this comment.
shouldn't we bail out in case err != nil?
euank
force-pushed
the
rkt-mkdir-implicit-mount
branch
from
c15f66a to
a64181f
Compare
|
Addressed your comments, @s-urbaniak. Seems very likely the chmod is overtly-defensive and could be removed since we largely control stage1, but seems outta scope for this change. |
Before this change, the
copy-upoperation could fail with:stage1: could not prepare mountpoint: mkdir sharedVolumes/foo: no such file or directoryThis would occur because
CopyTreedoes not implicitly create thetarget directory, so we must create it prior to calling.
cc @squeed
TODO: