Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

stage1: improve duplicate mount-volume detection #3666

Merged
merged 4 commits into from
May 8, 2017

Conversation

lucab
Copy link
Member

@lucab lucab commented Apr 28, 2017

Breaking change: volumes with duplicate names are now rejected.

This PR adds error-checking for duplicate volume names. Also, it adds path cleaning when de-duplicating mounts targets. Tests are slightly adjusted to check both cases.

Fixes #3663

@lucab lucab requested review from squeed and euank April 28, 2017 11:51
@lucab
Copy link
Member Author

lucab commented Apr 28, 2017

/cc @euank

See appc/spec#687 for the corresponding fix directly in appc/spec.

@euank
Copy link
Member

euank commented May 2, 2017

This does not fix all of #3663

The important one is that multiple mounts with the same target should work if they have different names, but that case still doesn't work with this PR.

Specifically:

sudo rkt run --stage1-name=coreos.com/rkt/stage1-fly:1.25.0 --insecure-options=image,ondisk \
   --volume y,kind=host,source=/tmp \
   --mount volume=x,target=/tmp \
   --volume x,kind=host,source=/tmp \
   --mount volume=x,target=/tmp \
   docker://busybox --exec=/bin/sh -- -c 'exit 0' && echo success

run: can't evaluate mounts: missing mount for volume "y"

@lucab
Copy link
Member Author

lucab commented May 3, 2017

@euank This is because stage0 is de-duplicating those mounts. Other stage1s work because they don't check that all volumes are covered by a mount. Looking at pod spec I think fly is being too strict in performing this check. I'll queue a commit on top of this PR to relax it.

@lucab lucab force-pushed the ups/duplicate-volname branch 2 times, most recently from 99e436b to e2b4244 Compare May 3, 2017 15:33
@lucab
Copy link
Member Author

lucab commented May 5, 2017

Rebased on master, PTAL.

@euank can you re-check if this now implements the proper behavior from wrapper flags point of view?

Copy link
Member

@euank euank left a comment

Choose a reason for hiding this comment

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

I think this allows a solution to our wrapper woes.

The other changes LGTM as well.

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

Successfully merging this pull request may close these issues.

3 participants