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

stage1: prepare-app: don't mount /sys if path already used #2888

Merged
merged 2 commits into from
Jul 15, 2016

Conversation

alban
Copy link
Member

@alban alban commented Jul 8, 2016

When users mount /sys or a sub-directory of /sys as a volume,
prepare-app should not mount /sys: that would mask the volume provided
by users. This patch detects if there is a volume mounted in /sys or a
subdirectory and skips the mount.

Fixes #2874


/cc @tjdett

TODO:

  • add tests

@@ -65,7 +65,7 @@ func ServiceWantPath(root string, appName types.ACName) string {
func InstantiatedPrepareAppUnitName(appName types.ACName) string {
// Naming respecting escaping rules, see systemd.unit(5) and systemd-escape(1)
escapedRoot := unit.UnitNamePathEscape(common.RelAppRootfsPath(appName))
return "prepare-app@" + escapedRoot + ".service"
return "prepare-app@-" + escapedRoot + ".service"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for this except being more readable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes: the string after the @ is not part of the filename written on disk but the parameter to the parametrized systemd unit. In this case, it is a directory, for example /opt/stage2/busybox/rootfs. But the string is escaped: / are converted to -, following systemd escaping rules. And the variable escapedRoot is not an absolute path: it does not have the leading /. It was not a problem before because prepare-app was accepting a relative path as parameter. But I made changes below that requires an absolute path. Hence this fix.

@iaguis
Copy link
Member

iaguis commented Jul 13, 2016

LGTM modulo tests

When users mount /sys or a sub-directory of /sys as a volume,
prepare-app should not mount /sys: that would mask the volume provided
by users. This patch detects if there is a volume mounted in /sys or a
subdirectory and skips the mount.

Fixes rkt#2874
@alban
Copy link
Member Author

alban commented Jul 15, 2016

@iaguis Updated.

@@ -288,3 +288,60 @@ func TestDockerVolumeSemanticsPodManifest(t *testing.T) {
runRktAndCheckOutput(t, cmd, expected, false)
}
}

var volSysfsTests = []struct {
name string
Copy link
Member

@iaguis iaguis Jul 15, 2016

Choose a reason for hiding this comment

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

Nit: description?

@iaguis
Copy link
Member

iaguis commented Jul 15, 2016

LGTM.

Semaphore failure seems to be unrelated, the test works on my machine but we're getting failures in Semaphore pretty regularly, we should investigate it a bit more.

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.

2 participants