Systemd support #211

rgulewich · 2018-12-20T18:06:57Z

For all containers:

Mount /run as tmpfs (default size 128 MiB)

For systemd labeled containers (those running and image with the
com.netflix.titus.systemd label set to "true"):

Mount /run/lock as its own tmpfs mount
Tini exec's the container's init command so that it runs as pid 1
Run them using the standard apparmor and seccomp profiles: no
CAP_SYS_ADMIN requirement

Other notes:

This requires that cgroup namespaces are enabled in docker, otherwise
the systemd container will fail to come up due to not being able to
create new cgroups.
Move to Bionic for the systemd test image: the version of systemd that
ships with it is able to start without CAP_SYS_ADMIN

codecov · 2018-12-20T18:09:59Z

Codecov Report

Merging #211 into master will increase coverage by 0.16%.
The diff coverage is 67.39%.

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
+ Coverage   35.65%   35.82%   +0.16%     
==========================================
  Files          71       71              
  Lines        9031     9061      +30     
==========================================
+ Hits         3220     3246      +26     
+ Misses       5412     5409       -3     
- Partials      399      406       +7

Impacted Files	Coverage Δ
executor/runtime/types/types.go	`67.24% <ø> (ø)`	⬆️
executor/runtime/docker/capabilities.go	`82.35% <100%> (+1.1%)`	⬆️
executor/runtime/docker/seccomp/seccomp.go	`28.81% <100%> (+2.65%)`	⬆️
executor/runtime/docker/docker.go	`55.15% <57.89%> (-0.09%)`	⬇️
executor/runtime/docker/docker_linux.go	`60.68% <68.18%> (+1.73%)`	⬆️

coveralls · 2018-12-21T00:43:31Z

Pull Request Test Coverage Report for Build 3233

5 of 58 (8.62%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.07%) to 25.784%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
executor/runtime/docker/capabilities.go	3	6	50.0%
executor/runtime/docker/docker_linux.go	0	24	0.0%
executor/runtime/docker/docker.go	0	26	0.0%

Totals
Change from base Build 3218:	-0.07%
Covered Lines:	2985
Relevant Lines:	11577

💛 - Coveralls

sargun

I think the only functional bug in here is the concurrency bug with inspecting the image and modifying a shared object. The rest are stylistic.

I guess we cannot add a full end-to-end test until we get the new Docker, correct?

.circleci/config.yml

sargun · 2019-01-28T17:38:56Z

executor/runtime/docker/capabilities.go

@@ -58,9 +59,12 @@ func setupAdditionalCapabilities(c *runtimeTypes.Container, hostCfg *container.H
 	if c.TitusInfo.GetAllowNestedContainers() {
 		apparmorProfile = "docker-nested"
 		seccompProfile = "nested-container.json"
+		c.Env["TINI_UNSHARE"] = trueString


We can rip out the nested containers code if you want?

Are we never going to use it again? I wasn't sure.

We might, but for now, it's cruft that's not supported, and should never be set. Probably worth just getting rid of the code and throwing an error if we find it's set to true.

sargun · 2019-01-28T17:42:20Z

executor/runtime/docker/docker.go

@@ -468,6 +471,20 @@ func (r *DockerRuntime) dockerConfig(c *runtimeTypes.Container, binds []string,
 	// Maybe set cfs bandwidth has to be called _after_
 	maybeSetCFSBandwidth(r.dockerCfg.cfsBandwidthPeriod, c, hostCfg)

+	// Always setup tmpfs: it's needed to ensure Metatron credentials don't persist across reboots and for SystemD to work
+	tmpFsSize := int64(defaultRunTmpFsSize)
+	if hostCfg.Memory < tmpFsSize {


I don't think we need this. It should never happen, since container memory should never be so low. In fact, if system memory = tmpfs memory, aren't we effectively leaking double the RAM?

We "trust" the master to do validation on resource dimensions.

I think this would also simplify the code a little bit, because then you could just hard code the below sizes, rather than fmt.Sprintf'ing

Sure, that's fine. When you pass in tmpfs volumes to docker at create time, the used tmpfs bytes count toward the cgroup's limit, so it doesn't double count.

sargun · 2019-01-28T17:44:53Z

executor/runtime/docker/docker.go

+	if systemdBool, ok := imageInfo.Config.Labels[systemdImageLabel]; ok {
+		val, err := strconv.ParseBool(systemdBool)
+		if err != nil {
+			return err


wrap this error so we know where it's coming from (IIRC, the function is errors.wrapf

sargun · 2019-01-28T17:45:35Z

executor/runtime/docker/docker.go

+// Use image labels to determine if the container should be configured to run SystemD
+func setSystemdRunning(imageInfo types.ImageInspect, c *runtimeTypes.Container) error {
+	l := log.WithField("imageName", c.QualifiedImageName())
+


Why not log the value here, rather than below, so if the value is "corrupted" we can at least figure out what's going on?

Also, this logger should descend from the one already in Prepare

sargun · 2019-01-28T17:45:58Z

executor/runtime/docker/docker.go

+	}
+
+	l.Info("SystemD image label not set: not configuring container to run SystemD")
+	c.IsSystemD = false


Isn't this the default anyways, so this is redundant?

sargun · 2019-01-28T17:47:41Z

executor/runtime/docker/docker.go

@@ -910,7 +947,7 @@ func (r *DockerRuntime) Prepare(parentCtx context.Context, c *runtimeTypes.Conta
 		}

 		myImageInfo = imageInfo
-		return nil
+		return setSystemdRunning(*imageInfo, c)


You cannot do it in here because that's a concurrency violation.

Since this modifies "c" when other people could be using it

rgulewich · 2019-01-28T18:03:57Z

I guess we cannot add a full end-to-end test until we get the new Docker, correct?

@sargun - Correct. In the meantime, I added a separate functional test for this.

For all containers: - Mount /run as tmpfs (default size 128 MiB) For systemd labeled containers (those running and image with the `com.netflix.titus.systemd` label set to "true"): - Mount `/run/lock` as its own tmpfs mount - Tini exec's the container's init command so that it runs as pid 1 - Run them using the standard apparmor and seccomp profiles: no CAP_SYS_ADMIN requirement Other notes: - This requires that cgroup namespaces are enabled in docker, otherwise the systemd container will fail to come up due to not being able to create new cgroups. - Move to Bionic for the systemd test image: the version of systemd that ships with it is able to start without CAP_SYS_ADMIN

sargun · 2019-02-18T22:40:21Z

executor/runtime/container.go

@@ -61,6 +61,7 @@ func NewContainer(taskID string, titusInfo *titus.ContainerInfo, resources *runt
 		TitusInfo:          titusInfo,
 		Resources:          resources,
 		Env:                env,
+		IsSystemD:          false,


Isn't false the default, any reason to set it?

sargun · 2019-02-18T22:41:14Z

executor/runtime/docker/docker.go

+		l.Infof("SystemD image label set to %s", systemdBool)
+		val, err := strconv.ParseBool(systemdBool)
+		if err != nil {
+			return errors.Wrap(err, "Error parsing SystemD image label")


SystemD -> systemd

sargun · 2019-02-18T22:41:37Z

executor/runtime/docker/docker.go

+	l := log.WithField("imageName", c.QualifiedImageName())
+
+	if systemdBool, ok := imageInfo.Config.Labels[systemdImageLabel]; ok {
+		l.Infof("SystemD image label set to %s", systemdBool)


use withfield.

sargun · 2019-02-18T22:42:12Z

executor/runtime/docker/docker.go

+		return nil
+	}
+
+	l.Info("SystemD image label not set: not configuring container to run SystemD")


See above. Also, does it make sense to log on the negative?

sargun · 2019-02-18T22:44:00Z

executor/runtime/docker/docker.go

@@ -1699,6 +1728,11 @@ func (r *DockerRuntime) setupPostStartLogDirTiniHandleConnection2(parentCtx cont
 		return err
 	}

+	if err := setCgroupOwnership(parentCtx, c, cred); err != nil {
+		log.Error("Unable to setup container nesting: ", err)


Is this error message correct, given we've removed nesting (or at least begun to remove it), shouldn't it be something like "Unable to delegate cgroups?"? also, use withErr

sargun · 2019-02-18T22:44:18Z

executor/runtime/docker/docker_linux.go

@@ -13,6 +13,9 @@ import (
 	"time"
 	"unsafe"



Extra space.

sargun · 2019-02-18T22:44:46Z

executor/runtime/docker/docker_linux.go

+		return nil
+	}
+
+	cgroupPath := filepath.Join("/proc/", strconv.FormatInt(int64(cred.pid), 10), "cgroup")


stronv.itoa?

rgulewich requested a review from sargun December 20, 2018 18:06

rgulewich force-pushed the systemd branch from ac1059c to 701bd53 Compare December 21, 2018 00:41

rgulewich force-pushed the systemd branch 3 times, most recently from 99fde5e to 4bb18a7 Compare January 28, 2019 17:26

sargun suggested changes Jan 28, 2019

View reviewed changes

gabrielhartmann approved these changes Jan 28, 2019

View reviewed changes

rgulewich force-pushed the systemd branch 3 times, most recently from f742d1a to be0ef7c Compare February 13, 2019 23:33

rgulewich force-pushed the systemd branch from be0ef7c to 307744e Compare February 15, 2019 23:03

rgulewich force-pushed the systemd branch 2 times, most recently from ba1f980 to 20f14ca Compare February 18, 2019 22:26

sargun reviewed Feb 18, 2019

View reviewed changes

sargun approved these changes Feb 18, 2019

View reviewed changes

Remove remaining nested container artifacts, re-add cgroup chowning

659ca8a

rgulewich force-pushed the systemd branch from 20f14ca to 659ca8a Compare February 19, 2019 04:23

rgulewich merged commit 0234ccd into master Feb 19, 2019

rgulewich deleted the systemd branch February 19, 2019 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Systemd support #211

Systemd support #211

rgulewich commented Dec 20, 2018 •

edited

codecov bot commented Dec 20, 2018 •

edited

coveralls commented Dec 21, 2018 •

edited

sargun left a comment

sargun Jan 28, 2019

rgulewich Jan 28, 2019

sargun Jan 28, 2019

sargun Jan 28, 2019

sargun Jan 28, 2019

rgulewich Jan 28, 2019

sargun Jan 28, 2019

sargun Jan 28, 2019

sargun Jan 28, 2019

sargun Jan 28, 2019

sargun Jan 28, 2019

sargun Jan 28, 2019

rgulewich commented Jan 28, 2019

sargun Feb 18, 2019

sargun Feb 18, 2019

sargun Feb 18, 2019

sargun Feb 18, 2019

sargun Feb 18, 2019

sargun Feb 18, 2019

sargun Feb 18, 2019

Systemd support #211

Systemd support #211

Conversation

rgulewich commented Dec 20, 2018 • edited

codecov bot commented Dec 20, 2018 • edited

Codecov Report

coveralls commented Dec 21, 2018 • edited

Pull Request Test Coverage Report for Build 3233

💛 - Coveralls

sargun left a 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

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

rgulewich commented Jan 28, 2019

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

rgulewich commented Dec 20, 2018 •

edited

codecov bot commented Dec 20, 2018 •

edited

coveralls commented Dec 21, 2018 •

edited