add support for recursive volume/mounts #2880

steveej · 2016-07-07T03:10:35Z

Depends on #2852
Closes #2827 (duplicate)
Fixes #2567

Follow-Up Issues

unite the tests
unite the volume/mount stage1 code
test & fix advanced cases with recursive mounts @alban to file an issue for these

euank · 2016-07-07T05:03:18Z

Documentation/subcommands/run.md

@@ -124,7 +124,8 @@ Each ACI can define a [list of mount points](https://github.com/appc/spec/blob/m
            {
                "name": "data",
                "path": "/var/data",
-                "readOnly": false
+                "readOnly": false,
+                "recursiive": true


sp: recursive

jonboulle · 2016-07-07T14:01:17Z

https://github.com/appc/spec/releases/tag/v0.8.6

euank · 2016-07-09T00:28:59Z

tests/rkt_volume_mount_kvm_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// +build kvm


There's no difference between this and the nspawn one.

Can this just be one file of rkt_volume_mount_non_fly or such, and either be !fly or coreos host kvm?

We can split out stage specific files if/when we need to.

euank · 2016-07-09T00:50:47Z

LGTM, regardless of my nit.

I would prefer if another pair of eyes looked at it. cc @alban
I didn't look as closely at the tests as I should have perhaps.

alban · 2016-07-18T13:28:06Z

tests/testutils/ctx.go

+		if child.Cmd == nil ||
+			child.Cmd.Process == nil ||
+			child.Cmd.ProcessState == nil {
+			continue


Why is it needed?

If the child isn't fully initialized there might be a panic due to nil-pointer. I ran into this while testing locally and I think the cleanup function should be able to tolerate this.

alban · 2016-07-18T13:45:03Z

@steveej Could you describe the refactoring in the patch "tests: cover recursive volume/mounts"? It is a big change.

steveej · 2016-07-19T20:09:59Z

Could you describe the refactoring in the patch "tests: cover recursive volume/mounts"? It is a big change.

Given it's a refactor I assume the motivation is obvious. What would you like to have explained?

alban · 2016-07-20T11:21:21Z

The Jenkins failure on fedora-23, flavor coreos does not seem to be related to this PR:

02:57:42 ++ git rev-parse origin/master
02:57:42 fatal: ambiguous argument 'origin/master': unknown revision or path not in the working tree.
02:57:42 Use '--' to separate paths from revisions, like this:
02:57:42 'git <command> [<revision>...] -- [<file>...]'

Given it's a refactor I assume the motivation is obvious. What would you like to have explained?

Things like:

verifyHostFile() was duplicated both in rkt_fly_test.go (build fly) and rkt_run_pod_manifest_test.go (build !fly). This patch moves it to rkt_tests.go to remove the code duplication.
tests TestFlyMountCLI and TestFlyMountPodManifest (build fly) deleted, replaced by TestVolumeMount and ... respectively.
The refactoring does not delete test cases but add new ones in ... (if it's correct)

alban · 2016-07-20T12:58:14Z

tests/rkt_volume_mount_test.go

+				if manifestFile != "" {
+					runCmd += fmt.Sprintf(" --pod-manifest=%s", manifestFile)
+				} else {
+					runCmd += fmt.Sprintf(" %s %s", tt.cmdArgs, hashesToRemove[0])


Do you mean to use hash instead of hashesToRemove[0]?

hash is defined in the previous for-loop and not available here. This code doesn't actually run the test for each image provided, but only for the first one. This can be improved and I'll add a comment.

alban · 2016-08-04T10:02:02Z

stage1_fly/run/main.go

+				for _, mnt := range mnts {
+					innerRelPath := tuple.M.Path + strings.Replace(mnt.MountPoint, tuple.V.Source, "", -1)
+					argFlyMounts = append(argFlyMounts,
+						flyMount{"", rfs, innerRelPath, "none", flags | syscall.MS_REMOUNT | syscall.MS_RDONLY},


Not tested, but I think MS_BIND is missing as well.

Also, we should test if the remount does not change the other flags such as nosuid, nodev. They are probably not important in the kvm flavor but they should not be changed in the fly flavor. See get_mount_flags.

MS_BIND should no be used in this case because it's a pure remount. Besides that, if it was necessary the current test would fail.

man mount(2) says about MS_REMOUNT:

The mountflags and data arguments should match the values used in the original mount() call, except for those parameters that are being deliberately changed.

So MS_BIND should be passed.

Also, do_remount (performing the MS_REMOUNT) behaves differently if the flag MS_BIND is not given.

In my manual test above, /tmp/data/RW was a bind mount. But in the functional test, tmpdir2path (/tmp/rkt-tests-flyxxxx/dir2) is a new mount (tmpfs). Maybe that's the difference explaining why the tests passed? I don't know.

The codepath you linked convinces me more that we don't want MS_BIND. The else case will attempt to use do_remount_sb() which IIUC is what we want.

I don't think so: the "sb" in do_remount_sb means superblock, and the superblock is the data structure which is common to all mounts of the same underlying storage.

So if you have one tmpfs, that's one superblock. If you create bind mounts of it, that creates several mounts but still all referring to the same superblock. But in our case, we want to change the RW/RO bit only for one mount, not for all mounts with the same superblock.

I think we want the change_mount_flags -> mnt_make_readonly instead.

I don't think so: the "sb" in do_remount_sb means superblock, and the superblock is the data structure which is common to all mounts of the same underlying storage.

This is beyond my knowledge about mounts and I wonder how this relates to shared mounts. Do shared mounts automatically shared the same superblock? What happens if a shared mount gets remounted private or unshared?

Besides, seems like in all our cases we need MS_BIND to do achieve what we want.

Based on Stefan Junker's work from rkt#2880

lucab · 2016-08-18T12:44:38Z

We split the vendoring part out of this into #3063.

s-urbaniak · 2016-08-29T13:53:51Z

@steveej pinging, sorry that this got so much delay. Are you still up for rebasing this one, or should we bump to the next release?

steveej · 2016-08-31T13:42:54Z

@s-urbaniak I'll rebase it again!

The volumes defined in the pod manifest now have a 'recursive' bool field. The default is recursive for nspawn containers and non-recursive for fly containers.

Refactors: - move duplicate verifyHostFile implementation from rkt_fly_test.go and rkt_run_pod_manifest_test.go to rkt_tests.go - Refactor TestFlyMount{CLI,PodManifest} to new tests using a TestVolumeMount - New Tests: - Recursive mount functionality for flavors kvm, fly, coreos - New test cases use the volumeMountTestCase struct type This practically duplicates some of the cases for kvm and coreos flavors

* remount every recursive sub-mount read-only * modify stage0 code to export GetMountsForPrefix * add tests to cover recursive/read-only mounts

s-urbaniak · 2016-09-01T13:41:12Z

A couple of passes were done, a green CI, and in the beginning of the next release cycle should give us enough headroom to let these changes cook in upcoming tests, merging.

Based on Stefan Junker's work from rkt#2880

steveej added kind/enhancement component/stage1 labels Jul 7, 2016

steveej added this to the v1.11.0 milestone Jul 7, 2016

steveej added needs/some-changes depends-on/appc labels Jul 7, 2016

steveej assigned alban and steveej Jul 7, 2016

steveej added the needs/review label Jul 7, 2016

steveej changed the title ~~add support for volume/mounts~~ add support for recursive volume/mounts Jul 7, 2016

euank reviewed Jul 7, 2016
View reviewed changes

euank reviewed Jul 9, 2016
View reviewed changes

steveej removed the needs/some-changes label Jul 9, 2016

steveej added the needs/rebase label Jul 15, 2016

alban reviewed Jul 18, 2016
View reviewed changes

alban mentioned this pull request Jul 18, 2016

[WIP] stage1: volumes: implement recursive option #2827

Closed

2 tasks

steveej added needs/some-changes and removed needs/rebase needs/review labels Jul 19, 2016

steveej removed the needs/some-changes label Jul 19, 2016

alban reviewed Jul 20, 2016
View reviewed changes

lucab removed this from the v1.12.0 milestone Aug 4, 2016

alban reviewed Aug 4, 2016
View reviewed changes

alban added a commit to kinvolk/rkt that referenced this pull request Aug 12, 2016

vendor: use appc/spec 0.8.6 and k8s.io/kubernetes v1.3.0

69ba364

Based on Stefan Junker's work from rkt#2880

alban mentioned this pull request Aug 12, 2016

vendor: use appc/spec 0.8.6 and k8s.io/kubernetes v1.3.0 #3063

Merged

alban added a commit to kinvolk/rkt that referenced this pull request Aug 12, 2016

vendor: use appc/spec 0.8.6 and k8s.io/kubernetes v1.3.0

670c2e2

Based on Stefan Junker's work from rkt#2880

alban unassigned alban and steveej Aug 12, 2016

steveej self-assigned this Aug 13, 2016

alban added a commit to kinvolk/rkt that referenced this pull request Aug 13, 2016

vendor: use appc/spec 0.8.6 and k8s.io/kubernetes v1.3.0

a50c87b

Based on Stefan Junker's work from rkt#2880

steveej added the needs/rebase label Aug 15, 2016

lucab modified the milestones: 1.14.0, v1.13.0 Aug 18, 2016

alban and others added 3 commits August 31, 2016 17:02

stage1: volumes: implement recursive option

f0b3556

The volumes defined in the pod manifest now have a 'recursive' bool field. The default is recursive for nspawn containers and non-recursive for fly containers.

stage1/kvm: volumes: implement recursive option

6801e0a

*: update --volume examples

7d37b97

lucab modified the milestones: v1.15.0, v1.14.0 Sep 1, 2016

steveej added 2 commits September 1, 2016 11:54

stage1: ensure read-only for recursive volumes

25d3021

* remount every recursive sub-mount read-only * modify stage0 code to export GetMountsForPrefix * add tests to cover recursive/read-only mounts

steveej removed the needs/rebase label Sep 1, 2016

s-urbaniak added reviewed/lgtm and removed needs/some-changes labels Sep 1, 2016

s-urbaniak merged commit 0d72cd8 into rkt:master Sep 1, 2016

steveej mentioned this pull request Sep 1, 2016

recursive mounts tests and functionality is incosistent among different stage1s #3154

Open

3 tasks

antoni pushed a commit to intelsdi-x/rkt that referenced this pull request Sep 9, 2016

vendor: use appc/spec 0.8.6 and k8s.io/kubernetes v1.3.0

77340dc

Based on Stefan Junker's work from rkt#2880

steveej deleted the recursive-mounts branch September 26, 2016 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for recursive volume/mounts #2880

add support for recursive volume/mounts #2880

steveej commented Jul 7, 2016 •

edited

Loading

euank Jul 7, 2016

jonboulle commented Jul 7, 2016

euank Jul 9, 2016

euank commented Jul 9, 2016

alban Jul 18, 2016

steveej Jul 19, 2016

alban commented Jul 18, 2016

steveej commented Jul 19, 2016

alban commented Jul 20, 2016

alban Jul 20, 2016

steveej Aug 2, 2016

alban Aug 4, 2016 •

edited

Loading

steveej Aug 4, 2016

alban Aug 4, 2016

steveej Aug 4, 2016

alban Aug 4, 2016

alban Aug 4, 2016

steveej Aug 5, 2016

lucab commented Aug 18, 2016

s-urbaniak commented Aug 29, 2016

steveej commented Aug 31, 2016

s-urbaniak commented Sep 1, 2016

add support for recursive volume/mounts #2880

add support for recursive volume/mounts #2880

Conversation

steveej commented Jul 7, 2016 • edited Loading

Choose a reason for hiding this comment

jonboulle commented Jul 7, 2016

Choose a reason for hiding this comment

euank commented Jul 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alban commented Jul 18, 2016

steveej commented Jul 19, 2016

alban commented Jul 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alban Aug 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Aug 18, 2016

s-urbaniak commented Aug 29, 2016

steveej commented Aug 31, 2016

s-urbaniak commented Sep 1, 2016

steveej commented Jul 7, 2016 •

edited

Loading

alban Aug 4, 2016 •

edited

Loading