stage1/fly: respect runtimeApp App's MountPoints #2852

steveej · 2016-06-30T02:29:02Z

I'm not positive that all the implementation of the different App types and MountPoints really reflect the specs, but this change reproduces what stage1-coreos does.

/cc @iaguis

euank · 2016-06-30T18:33:10Z

Should this logic live in stage1/init/common/mount.go (or does it already in the form of GenerateMounts?) so that the code is shared?

steveej · 2016-06-30T19:20:31Z

Should this logic live in stage1/init/common/mount.go (or does it already in the form of GenerateMounts?) so that the code is shared?

I'll look into that after we have tests in place on all ends so I can prevent breakage.

euank · 2016-06-30T22:20:19Z

This doesn't fix all differences between mounts in coreos and fly. For example, duplicate container targets in coreos are handled (with the first one winning), while here it ends up with a rather odd error.

For example:

$ rkt run \
  --volume a,kind=host,source=/etc/resolv.conf \
  --mount volume=a,target=/etc/resolv.conf \
  --volume b,kind=host,source=/etc/hosts \
  --mount volume=b,target=/etc/resolv.conf \
  --stage1-path=.../stage1-fly.aci quay.io/coreos/alpine-sh --exec=/bin/sh  -- -c 'cat /etc/resolv.conf'
run: can't evaluate mounts: missing mount for volume "b"

The above prints out the contents of the host /etc/resolv.conf with stage1-coreos, but errors out as above with fly.

I can file a separate issue if you'd like, or we can handle that here. Switching to the common/mounts.go code does fix this.

euank · 2016-06-30T22:22:37Z

stage1_fly/run/main.go

+		imAppManifestMPs := imApp.MountPoints
+		for _, mp := range imAppManifestMPs {
+			tuple, exists := namedVolumeMounts[mp.Name]
+			switch {


I don't think using a switch over if/else-if helps readability in this case.

steveej · 2016-06-30T23:43:16Z

I can file a separate issue if you'd like, or we can handle that here. Switching to the common/mounts.go code does fix this.

Please file a separate issue since this is not a regression related to this PR or the issue it's handling.

iaguis · 2016-07-05T12:49:47Z

tests/rkt_fly_test.go

+	}
+}
+
+// }


Closes https://github.com/coreos/rkt/pull/2852/files#diff-c636300c4bc5c06ef17114ab5421a403R106

Is there a reason not to remove them both? The top comment is clear since it applies to the rest of the file anyways

iaguis · 2016-07-05T13:48:15Z

I'll look into that after we have tests in place on all ends so I can prevent breakage.

Is this something you can do now that we have tests? Maybe we can do it in a follow-up?

s-urbaniak · 2016-07-06T14:21:56Z

stage1_fly/run/main.go

@@ -110,15 +110,24 @@ func init() {
 	flag.StringVar(&discardString, "local-config", common.DefaultLocalConfigDir, "Local config path")
 }

+func checkAndAddMps(namedVolumeMounts map[types.ACName]volumeMountTuple, mountpoints []types.MountPoint) error {


can we improve the naming, i.e. simply "addMountpoints"? Since this method returns an error, checking is implied.also mountpoints []types.MountPoint stutters, i.e. name it mps instead?

Maybe even call namedVolumeMounts simply target, and mountpoints simply src.

Maybe even call namedVolumeMounts simply target, and mountpoints simply src.

Would you rename this only in the function or also in the caller? I kept the naming from there.

steveej · 2016-07-06T16:06:47Z

Is this something you can do now that we have tests? Maybe we can do it in a follow-up?

I'd tackle this with a follow-up, there is more to do than just to refactor as I talked through OOB with @euank last week.

* Fixes a bug where MountPoints from the PodManifest App's weren't processed * Refactor code and add comments

tmrts · 2016-07-07T09:24:12Z

stage1_fly/run/main.go

+func addMountPoints(namedVolumeMounts map[types.ACName]volumeMountTuple, mountpoints []types.MountPoint) error {
+	for _, mp := range mountpoints {
+		tuple, exists := namedVolumeMounts[mp.Name]
+		switch {


Minor nit for future reference, you can use switch tuple, exists := namedVolumeMounts[mp.Name]; {

tmrts · 2016-07-07T09:36:36Z

Building the PR, I investigated the #2846 and couldn't reproduce the bug, LGTM

steveej added component/fly needs/tests needs/review kind/bug labels Jun 30, 2016

steveej removed the needs/tests label Jun 30, 2016

euank reviewed Jun 30, 2016
View reviewed changes

steveej added needs/some-changes and removed needs/review labels Jun 30, 2016

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

iaguis reviewed Jul 5, 2016
View reviewed changes

s-urbaniak reviewed Jul 6, 2016
View reviewed changes

steveej added this to the v1.10.0 milestone Jul 6, 2016

steveej added 4 commits July 6, 2016 14:54

stage1/fly: respect runtimeApp App's MountPoints

d219e2e

* Fixes a bug where MountPoints from the PodManifest App's weren't processed * Refactor code and add comments

stage1/fly tests: cover CLI mounts

a1b9ca6

stage1/fly tests: cover PodManifest MountPoints

d4f5178

stage1/fly tests: cover duplicate mount target via CLI

682ef16

steveej mentioned this pull request Jul 7, 2016

add support for recursive volume/mounts #2880

Merged

15 tasks

tmrts self-assigned this Jul 7, 2016

tmrts reviewed Jul 7, 2016
View reviewed changes

tmrts added the reviewed/lgtm label Jul 7, 2016

tmrts merged commit 2d29e5f into rkt:master Jul 7, 2016

steveej deleted the rkt-fly-mounts branch July 11, 2016 19:06

lucab unassigned tmrts Apr 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stage1/fly: respect runtimeApp App's MountPoints #2852

stage1/fly: respect runtimeApp App's MountPoints #2852

steveej commented Jun 30, 2016 •

edited

euank commented Jun 30, 2016

steveej commented Jun 30, 2016

euank commented Jun 30, 2016 •

edited

euank Jun 30, 2016

steveej commented Jun 30, 2016

iaguis Jul 5, 2016

steveej Jul 6, 2016

euank Jul 6, 2016 •

edited

iaguis commented Jul 5, 2016

s-urbaniak Jul 6, 2016

s-urbaniak Jul 6, 2016

steveej Jul 6, 2016

steveej commented Jul 6, 2016

tmrts Jul 7, 2016

tmrts commented Jul 7, 2016

+              	}
+              }
+              // }

stage1/fly: respect runtimeApp App's MountPoints #2852

stage1/fly: respect runtimeApp App's MountPoints #2852

Conversation

steveej commented Jun 30, 2016 • edited

euank commented Jun 30, 2016

steveej commented Jun 30, 2016

euank commented Jun 30, 2016 • edited

euank Jun 30, 2016

Choose a reason for hiding this comment

steveej commented Jun 30, 2016

iaguis Jul 5, 2016

Choose a reason for hiding this comment

steveej Jul 6, 2016

Choose a reason for hiding this comment

euank Jul 6, 2016 • edited

Choose a reason for hiding this comment

iaguis commented Jul 5, 2016

s-urbaniak Jul 6, 2016

Choose a reason for hiding this comment

s-urbaniak Jul 6, 2016

Choose a reason for hiding this comment

steveej Jul 6, 2016

Choose a reason for hiding this comment

steveej commented Jul 6, 2016

tmrts Jul 7, 2016

Choose a reason for hiding this comment

tmrts commented Jul 7, 2016

steveej commented Jun 30, 2016 •

edited

euank commented Jun 30, 2016 •

edited

euank Jul 6, 2016 •

edited