From 596d0be5d80c8ff3ff0ec8e253184fae22a79385 Mon Sep 17 00:00:00 2001
From: Mahmood Ali <mahmood@hashicorp.com>
Date: Wed, 11 Dec 2019 11:28:09 -0500
Subject: [PATCH] executor: stop joining executor to container cgroup

Stop joining libcontainer executor process into the newly created task
container cgroup, to ensure that the cgroups are fully destroyed on
shutdown, and to make it consistent with other plugin processes.

Previously, executor process is added to the container cgroup so the
executor process resources get aggregated along with user processes in
our metric aggregation.

However, adding executor process to container cgroup adds some
complications with much benefits:

First, it complicates cleanup.  We must ensure that the executor is
removed from container cgroup on shutdown.  Though, we had a bug where
we missed removing it from the systemd cgroup.  Because executor uses
`containerState.CgroupPaths` on launch, which includes systemd, but
`cgroups.GetAllSubsystems` which doesn't.

Second, it may have advese side-effects.  When a user process is cpu
bound or uses too much memory, executor should remain functioning
without risk of being killed (by OOM killer) or throttled.

Third, it is inconsistent with other drivers and plugins.  Logmon and
DockerLogger processes aren't in the task cgroups.  Neither are
containerd processes, though it is equivalent to executor in
responsibility.

Fourth, in my experience when executor process moves cgroup while it's
running, the cgroup aggregation is odd.  The cgroup
`memory.usage_in_bytes` doesn't seem to capture the full memory usage of
the executor process and becomes a red-harring when investigating memory
issues.

For all the reasons above, I opted to have executor remain in nomad
agent cgroup and we can revisit this when we have a better story for
plugin process cgroup management.
---
 drivers/shared/executor/executor_linux.go | 50 -----------------------
 1 file changed, 50 deletions(-)

diff --git a/drivers/shared/executor/executor_linux.go b/drivers/shared/executor/executor_linux.go
index 379bab13a..7cf06c057 100644
--- a/drivers/shared/executor/executor_linux.go
+++ b/drivers/shared/executor/executor_linux.go
@@ -17,7 +17,6 @@ import (
 	"github.com/armon/circbuf"
 	"github.com/hashicorp/consul-template/signals"
 	hclog "github.com/hashicorp/go-hclog"
-	multierror "github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/nomad/client/allocdir"
 	"github.com/hashicorp/nomad/client/stats"
 	cstructs "github.com/hashicorp/nomad/client/structs"
@@ -92,15 +91,6 @@ func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, erro
 
 	l.command = command
 
-	// Move to the root cgroup until process is started
-	subsystems, err := cgroups.GetAllSubsystems()
-	if err != nil {
-		return nil, err
-	}
-	if err := JoinRootCgroup(subsystems); err != nil {
-		return nil, err
-	}
-
 	// create a new factory which will store the container state in the allocDir
 	factory, err := libcontainer.New(
 		path.Join(command.TaskDir, "../alloc/container"),
@@ -194,15 +184,6 @@ func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, erro
 		return nil, err
 	}
 
-	// Join process cgroups
-	containerState, err := container.State()
-	if err != nil {
-		l.logger.Error("error entering user process cgroups", "executor_pid", os.Getpid(), "error", err)
-	}
-	if err := cgroups.EnterPid(containerState.CgroupPaths, os.Getpid()); err != nil {
-		l.logger.Error("error entering user process cgroups", "executor_pid", os.Getpid(), "error", err)
-	}
-
 	// start a goroutine to wait on the process to complete, so Wait calls can
 	// be multiplexed
 	l.userProcExited = make(chan interface{})
@@ -287,15 +268,6 @@ func (l *LibcontainerExecutor) Shutdown(signal string, grace time.Duration) erro
 		return nil
 	}
 
-	// move executor to root cgroup
-	subsystems, err := cgroups.GetAllSubsystems()
-	if err != nil {
-		return err
-	}
-	if err := JoinRootCgroup(subsystems); err != nil {
-		return err
-	}
-
 	status, err := l.container.Status()
 	if err != nil {
 		return err
@@ -787,28 +759,6 @@ func newLibcontainerConfig(command *ExecCommand) (*lconfigs.Config, error) {
 	return cfg, nil
 }
 
-// JoinRootCgroup moves the current process to the cgroups of the init process
-func JoinRootCgroup(subsystems []string) error {
-	mErrs := new(multierror.Error)
-	paths := map[string]string{}
-	for _, s := range subsystems {
-		mnt, _, err := cgroups.FindCgroupMountpointAndRoot("", s)
-		if err != nil {
-			multierror.Append(mErrs, fmt.Errorf("error getting cgroup path for subsystem: %s", s))
-			continue
-		}
-
-		paths[s] = mnt
-	}
-
-	err := cgroups.EnterPid(paths, os.Getpid())
-	if err != nil {
-		multierror.Append(mErrs, err)
-	}
-
-	return mErrs.ErrorOrNil()
-}
-
 // cmdDevices converts a list of driver.DeviceConfigs into excutor.Devices.
 func cmdDevices(devices []*drivers.DeviceConfig) ([]*lconfigs.Device, error) {
 	if len(devices) == 0 {