executor v2 #4656

nickethier · 2018-09-10T17:23:43Z

Dependent on #4655 so as to separate the vendor changes from interface/implementation.

Some cleanup is still needed as well as plumbing logging in.

nickethier · 2018-09-14T17:59:59Z

client/driver/docker.go

-		Task:           task,
-		Driver:         "docker",
-		LogDir:         ctx.TaskDir.LogDir,
+	/*executorCtx := &executor.ExecutorContext{


nickethier · 2018-09-14T18:00:13Z

client/driver/docker.go


 	// The user hasn't specified any logging options so launch our own syslog
 	// server if possible.
 	syslogAddr := ""
-	if len(d.driverConfig.Logging) == 0 {
+	/*if len(d.driverConfig.Logging) == 0 {


nickethier · 2018-09-14T18:00:21Z

client/driver/docker.go

-	if err := h.executor.UpdateTask(task); err != nil {
-		h.logger.Printf("[DEBUG] driver.docker: failed to update log config: %v", err)
-	}
+	//if err := h.executor.UpdateTask(task); err != nil {


nickethier · 2018-09-14T18:01:37Z

client/driver/exec_integration_test.go

+			"unique.cgroup.mountpoint": "/sys/fs/cgroup",
+		},
+	}
+


fix or remove

nickethier · 2018-09-14T18:02:26Z

client/driver/executor/executor.go

@@ -172,101 +188,65 @@ type UniversalExecutor struct {
 	processExited       chan interface{}
 	fsIsolationEnforced bool


I don't think we need this anymore

nickethier · 2018-09-14T18:02:36Z

client/driver/executor/executor.go

-	lre         *logRotatorWrapper
-	lro         *logRotatorWrapper
-	rotatorLock sync.Mutex
-
 	syslogServer *logging.SyslogServer


remove syslog

nickethier · 2018-09-14T18:04:32Z

client/driver/executor/executor_libcontainer.go

+	userProcExited chan struct{}
+	exitState      *ProcessState
+
+	syslogServer *logging.SyslogServer


remove syslog

nickethier · 2018-09-17T17:47:45Z

client/driver/executor/executor_libcontainer.go

+}
+
+func NewExecutorWithIsolation(logger hclog.Logger) Executor {
+	logger = logger.Named("executor")


named "isolated_executor"

nickethier · 2018-09-17T17:48:14Z

client/driver/executor/executor_libcontainer.go

+
+// Launch creates a new container in libcontainer and starts a new process with it
+func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, error) {
+	// Find the nomad executable to launch the executor process with


use logger.WithFields() to set alloc_id and task_name

So we actually don't have the task_name or alloc_id available.

nickethier · 2018-09-17T17:49:57Z

client/driver/executor/executor_libcontainer.go

+
+	cfg.Devices = lconfigs.DefaultAutoCreatedDevices
+	cfg.Mounts = []*lconfigs.Mount{
+		/*{


nickethier · 2018-09-17T17:56:10Z

client/driver/executor/executor_libcontainer.go

+			Flags:       defaultMountFlags,
+		},
+		{
+			Source:      "sysfs",


lets think about this.

nickethier · 2018-09-17T18:02:05Z

client/driver/executor/executor_libcontainer.go

+		return nil, err
+	}
+
+	pidStats, err := l.pidCollector.pidStats()


use cgroups pid tracker instead of host pid

nickethier · 2018-09-17T18:06:36Z

client/driver/executor/executor_libcontainer.go

+}
+
+// Kill
+func (l *LibcontainerExecutor) Kill() error {


Remove from interface

nickethier · 2018-09-17T18:09:56Z

client/driver/executor/executor_libcontainer.go

+
+}
+
+func (l *LibcontainerExecutor) getIsolationConfig() *dstructs.IsolationConfig {


remove from ProcessState

client/driver/java.go

nickethier · 2018-09-17T18:18:03Z

client/logmon/cmd/main.go

@@ -0,0 +1,75 @@
+package main


nsenter_linux.go

dadgar · 2018-09-17T21:27:03Z

client/driver/executor/executor.go

@@ -106,15 +71,29 @@ type ExecCommand struct {
 	// Args is the args of the command that the user wants to run.
 	Args []string

+	// Resources defined by the task
+	Resources *Resources


NoOp comment - not convinced replicating resources struct is the right idea. Lets get further into it before we decide

dadgar · 2018-09-17T22:12:42Z

nsenter_linux.go

+	_ "github.com/opencontainers/runc/libcontainer/nsenter"
+)
+
+func init() {


Leave a comment on what this does

dadgar · 2018-09-17T22:13:05Z

client/driver/executor/executor_libcontainer.go

+)
+
+const (
+	defaultCgroupParent = "nomad"


Comment on this

dadgar · 2018-09-17T22:13:17Z

client/driver/executor/executor_libcontainer.go

+	defaultCgroupParent = "nomad"
+)
+
+var allCaps []string


Comment on this and prefer to keep them all in the same var block

dadgar · 2018-09-17T22:13:32Z

client/driver/executor/executor_libcontainer.go

+var allCaps []string
+
+var (
+	// The statistics the executor exposes when using cgroups


Comments should start with the method/struct/variable name

dadgar · 2018-09-17T22:22:19Z

client/logmon/logmon.go

+	Stop() error
+}
+
+func LaunchLogMon(logger hclog.Logger) (LogMon, *plugin.Client, error) {


I think this is fine for now but we should probably add it to the plugin-launcher eventually

dadgar · 2018-09-17T22:27:20Z

client/logmon/cmd/main.go

+			HandshakeConfig: logmon.Handshake,
+			Plugins: map[string]plugin.Plugin{
+				"logmon": logmon.NewPlugin(logmon.NewLogMon(hclog.Default().Named("logmon.test"))),
+			},


Get rid of .test

dadgar · 2018-09-17T22:28:57Z

client/logmon/logmon.go

+	Stop() error
+}
+
+func LaunchLogMon(logger hclog.Logger) (LogMon, *plugin.Client, error) {


Can this be I the plugin.go file

dadgar · 2018-09-17T22:32:03Z

client/logmon/logmon.go

+}
+
+func (tl *TaskLogger) Close() {
+	tl.lro.Close()


if nil guard these?

dadgar · 2018-09-17T22:34:09Z

client/driver/executor/executor_libcontainer.go

+	l.container = container
+
+	combined := append([]string{command.Cmd}, command.Args...)
+	stdout, err := command.Stdout()


Do we have to close these?

nickethier · 2018-09-18T04:02:33Z

@dadgar comments addressed

client/driver/exec.go

dadgar · 2018-09-21T17:30:15Z

client/driver/exec.go

@@ -297,7 +298,7 @@ func (h *execHandle) run() {
 	close(h.doneCh)

 	// Destroy the executor
-	if err := h.executor.Destroy(); err != nil {
+	if err := h.executor.Shutdown(h.taskShutdownSignal, h.killTimeout); err != nil {


This should be a force as well. The main process has exited

dadgar · 2018-09-21T17:32:32Z

client/driver/java.go

@@ -437,7 +443,7 @@ func (h *javaHandle) run() {
 	}

 	// Destroy the executor
-	h.executor.Destroy()
+	h.executor.Shutdown(h.shutdownSignal, h.killTimeout)


Same comments apply to all drivers

dadgar · 2018-09-21T17:34:59Z

client/driver/executor/executor.go

@@ -45,7 +49,7 @@ var (
 type Executor interface {
 	Launch(*ExecCommand) (*ProcessState, error)
 	Wait() (*ProcessState, error)
-	Destroy() error
+	Shutdown(string, time.Duration) error


would be nice to have parameter names and comments in the interface

client/driver/executor/executor.go

dadgar · 2018-09-21T17:38:55Z

client/driver/executor/executor.go

+		return fmt.Errorf("executor failed to shutdown error: no process found")
+	}
+
+	if signal == "" {


Should 444-460 be put in an if grace != 0 block

dadgar · 2018-09-21T17:41:26Z

client/driver/executor/executor_linux.go

+		signal = "SIGINT"
+	}
+
+	sig, ok := signals.SignalLookup[signal]


Similar comment for handling a force kill

dadgar · 2018-09-21T17:42:30Z

client/driver/executor/executor_linux.go

@@ -499,3 +577,115 @@ func newLibcontainerConfig(command *ExecCommand) *lconfigs.Config {
 	configureCgroups(cfg, command)
 	return cfg
 }
+
+// configureResourceContainer


dadgar · 2018-09-21T17:45:48Z

client/driver/executor/executor_linux.go

@@ -499,3 +577,115 @@ func newLibcontainerConfig(command *ExecCommand) *lconfigs.Config {
 	configureCgroups(cfg, command)
 	return cfg
 }
+
+// configureResourceContainer
+func (e *UniversalExecutor) configureResourceContainer(pid int) error {


Hmm I don't like mixing the universal and libcontainer code in one file

Yeah I agree. executor_universal_linux.go?

dadgar · 2018-09-21T17:46:40Z

client/driver/executor/resource_container_linux.go

+	cgroupConfig "github.com/opencontainers/runc/libcontainer/configs"
+)
+
+// resourceContainerContext is a platform-specific struct for managing a


Can we make it clear that it is only used by the universal executor

jippi · 2018-09-21T20:25:24Z

This will be so good! good job so far @nickethier

schmichael

I still need to review logmon but wanted to post these comments first. Overall looks good! Feel free to ignore nitpicks!

schmichael · 2018-09-21T17:28:18Z

client/allocrunnerv2/taskrunner/task_runner.go

@@ -375,6 +384,17 @@ func (tr *TaskRunner) shouldRestart() (bool, time.Duration) {
 	}
 }

+func (tr *TaskRunner) launchLogmon() error {
+	l, c, err := logmon.LaunchLogMon(tr.logger)


[nitpick] stick to LogMon or logmon

schmichael · 2018-09-21T17:32:28Z

client/allocrunnerv2/taskrunner/task_runner.go

+	if runtime.GOOS == "windows" {
+		id := uuid.Generate()[:8]
+		stdoutFifo = fmt.Sprintf("//./pipe/%s.stdout.%s", id, tr.task.Name)
+		stderrFifo = fmt.Sprintf("//./pipe/%s.stderr.%s", id, tr.task.Name)


[nitpick] Putting the task name last seems a bit strange. I think we use this style elsewhere:

{task}-{uuid}.stdout

schmichael · 2018-09-21T17:33:07Z

client/allocrunnerv2/taskrunner/task_runner.go

+		stderrFifo = fmt.Sprintf("//./pipe/%s.stderr.%s", id, tr.task.Name)
+	} else {
+		stdoutFifo = filepath.Join(tr.taskDir.LogDir, fmt.Sprintf("%s.stdout", tr.task.Name))
+		stderrFifo = filepath.Join(tr.taskDir.LogDir, fmt.Sprintf("%s.stderr", tr.task.Name))


We should make these hidden files and add the name FIFO to them to discourage people trying to use them directly via the FS API.

schmichael · 2018-09-21T17:39:03Z

client/allocrunnerv2/taskrunner/task_runner.go

+	err = tr.logmon.Start(&logmon.LogConfig{
+		LogDir:        tr.taskDir.LogDir,
+		StdoutLogFile: fmt.Sprintf("%s.stdout", tr.task.Name),
+		StderrLogFile: fmt.Sprintf("%s.stderr", tr.task.Name),


Hm, won't this get appended to LogDir and be the same file as the FIFO? The answer must be "no" because otherwise I don't think it would work at all! 😅

Changing the FIFO filename above will make this configuration less confusing, so I think this can stay put.

client/allocrunnerv2/taskrunner/task_runner.go

schmichael · 2018-09-21T20:38:29Z

client/driver/executor/executor.go

@@ -163,110 +197,86 @@ func (v *ExecutorVersion) GoString() string {
 // and file system isolation
 type UniversalExecutor struct {
 	cmd     exec.Cmd
-	ctx     *ExecutorContext
 	command *ExecCommand


While you're in here can you come up with a saner naming scheme for these two fields that both read "command exec command" in English? 😅

I always get confused in the rest of the executor which is which.

schmichael · 2018-09-21T20:56:47Z

client/driver/executor/executor_linux.go

+	return l.userProc.Signal(s)
+}
+
+// Exec starts an additional process inside the executor


...inside the container

client/driver/executor/executor_linux.go

schmichael

logmon needs tests -- but feel free to add them in a followup PR since this once is huge so close to done!

schmichael · 2018-09-21T23:06:14Z

client/logmon/logmon.go

+	UID int
+
+	// GID is id for the desried group to write log files as
+	GID int


Could we just run the logmon process as this user and group? We wouldn't have to plumb these through or chown files on creation.

schmichael · 2018-09-21T23:13:48Z

client/logmon/logmon.go

+}
+
+// newLogRotatorWrapper takes a rotator and returns a wrapper that has the
+// processOutWriter to attach to the processes stdout or stderr.


processes -> process's

schmichael · 2018-09-21T23:14:01Z

client/logmon/logmon.go

+	return wrap, nil
+}
+
+// start starts a go-routine that copies from the pipe into the rotator. This is


client/logmon/logmon.go

nickethier · 2018-09-22T02:21:45Z

RE: tmpfs/shm

Memory does count towards the total cgroup limit and is tracked in the mapped_file field of the memory.stat file.

Do we want to add it to the limit? This would mean we're setting the limit to more than what was scheduled for.

@schmichael @dadgar

… run as desired user/group

dadgar · 2018-09-24T17:16:17Z

client/allocrunnerv2/taskrunner/logmon_hook.go

+	req *interfaces.TaskPrestartRequest, resp *interfaces.TaskPrestartResponse) error {
+
+	// Launch logmon instance for the task.
+	err := h.launchLogMon()


if err := h.launchLogMon(); err != nil {

dadgar · 2018-09-24T17:17:33Z

client/allocrunnerv2/taskrunner/logmon_hook.go

+
+func (h *logmonHook) Stop(context.Context, *interfaces.TaskStopRequest, *interfaces.TaskStopResponse) error {
+
+	h.logmon.Stop()


Nil guard both

client/allocrunnerv2/taskrunner/logmon_hook.go

dadgar · 2018-09-24T17:24:21Z

client/allocrunnerv2/taskrunner/task_runner.go

+
+	// taskLoggingFifoGetter is used to return the paths to the stdout and stderr
+	// fifos to be passed to the driver for task logging
+	taskLoggingFifoGetter func() (stdoutFifo, stderrFifo string)


See comment on hook

schmichael

Sorry! I should have given you more guidance with the hooks! Definitely need to document them somewhere...

schmichael · 2018-09-24T17:34:08Z

client/allocrunnerv2/taskrunner/logmon_hook.go

+		logger:  logger,
+	}
+
+	tr.taskLoggingFifoGetter = hook.getFifos


I think we should invert this relationship to avoid taskLoggingFifoGetter ever being a nil func. Put a setter on TaskRunner to receive the FIFOs:

nomad/client/allocrunnerv2/taskrunner/task_runner.go

Lines 689 to 697 in 22fa055

// UpdateStats updates and emits the latest stats from the driver.

func (tr *TaskRunner) UpdateStats(ru *cstructs.TaskResourceUsage) {

tr.resourceUsageLock.Lock()

tr.resourceUsage = ru

tr.resourceUsageLock.Unlock()

if ru != nil {

tr.emitStats(ru)

}

}

You can ever pass it via an interface to make hook testing easy:

nomad/client/allocrunnerv2/taskrunner/stats_hook_test.go

Lines 32 to 36 in 22fa055

func (m *mockStatsUpdater) UpdateStats(ru *cstructs.TaskResourceUsage) {

if m.Ch != nil {

m.Ch <- ru

}

}

(I'm not tied to this approach. "Polluting" our runners with a bunch of callbacks has its own downsides, so I'm open to ideas.)

schmichael · 2018-09-24T17:36:56Z

client/allocrunnerv2/taskrunner/logmon_hook.go

+	return nil
+}
+
+func (h *logmonHook) Stop(context.Context, *interfaces.TaskStopRequest, *interfaces.TaskStopResponse) error {


This needs to be Exited to destroy the logger after each run of a task.

Stop is called when the task is permanently stopped and should probably be called Destroy?

* client/executor: refactor client to remove interpolation * executor: POC libcontainer based executor * vendor: use hashicorp libcontainer fork * vendor: add libcontainer/nsenter dep * executor: updated executor interface to simplify operations * executor: implement logging pipe * logmon: new logmon plugin to manage task logs * driver/executor: use logmon for log management * executor: fix tests and windows build * executor: fix logging key names * executor: fix test failures * executor: add config field to toggle between using libcontainer and standard executors * logmon: use discover utility to discover nomad executable * executor: only call libcontainer-shim on main in linux * logmon: use seperate path configs for stdout/stderr fifos * executor: windows fixes * executor: created reusable pid stats collection utility that can be used in an executor * executor: update fifo.Open calls * executor: fix build * remove executor from docker driver * executor: Shutdown func to kill and cleanup executor and its children * executor: move linux specific universal executor funcs to seperate file * move logmon initialization to a task runner hook * client: doc fixes and renaming from code review * taskrunner: use shared config struct for logmon fifo fields * taskrunner: logmon only needs to be started once per task

github-actions · 2023-02-27T02:17:25Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

nickethier changed the base branch from master to f-vendor-libcontainer September 10, 2018 17:28

nickethier changed the base branch from f-vendor-libcontainer to r-clientv2 September 13, 2018 17:45

nickethier force-pushed the f-execv2 branch from e0afea5 to f646852 Compare September 13, 2018 18:06

nickethier force-pushed the r-clientv2 branch from ba95b44 to 2b03e8b Compare September 14, 2018 02:59

nickethier force-pushed the f-execv2 branch 3 times, most recently from 60980f3 to 000a67e Compare September 14, 2018 17:04

nickethier commented Sep 14, 2018

View reviewed changes

dadgar force-pushed the r-clientv2 branch from d3e5440 to e0114c5 Compare September 16, 2018 00:08

nickethier force-pushed the f-execv2 branch from 101ccf4 to 245f7b0 Compare September 16, 2018 01:42

nickethier commented Sep 16, 2018

View reviewed changes

nickethier force-pushed the f-execv2 branch from fa8865a to f77dc59 Compare September 17, 2018 17:23

nickethier commented Sep 17, 2018

View reviewed changes

dadgar requested changes Sep 17, 2018

View reviewed changes

dadgar force-pushed the r-clientv2 branch from 3b93ba2 to 83e91b6 Compare September 17, 2018 22:42

nickethier force-pushed the f-execv2 branch from 0cd25ba to 5472381 Compare September 18, 2018 04:01

dadgar force-pushed the r-clientv2 branch from 393ee89 to a333efb Compare September 18, 2018 17:50

nickethier force-pushed the f-execv2 branch from 5472381 to 28695b0 Compare September 18, 2018 18:22

nickethier added 10 commits September 18, 2018 14:23

client/executor: refactor client to remove interpolation

9eded0e

executor: POC libcontainer based executor

b579718

vendor: use hashicorp libcontainer fork

d64e927

vendor: add libcontainer/nsenter dep

b4197a7

executor: updated executor interface to simplify operations

4e00e62

executor: implement logging pipe

6ab8913

logmon: new logmon plugin to manage task logs

dd220bf

driver/executor: use logmon for log management

7a6b092

executor: fix tests and windows build

a77bbe0

executor: fix logging key names

4d15540

executor: Shutdown func to kill and cleanup executor and its children

35c376d

nickethier changed the title ~~WIP: exec v2~~ executor v2 Sep 21, 2018

dadgar requested changes Sep 21, 2018

View reviewed changes

nickethier added 2 commits September 21, 2018 14:54

executor: move linux specific universal executor funcs to seperate file

42152fe

fix test

11d1446

dadgar approved these changes Sep 21, 2018

View reviewed changes

schmichael reviewed Sep 21, 2018

View reviewed changes

schmichael requested changes Sep 21, 2018

View reviewed changes

move logmon initialization to a task runner hook

747f34d

nickethier added 3 commits September 21, 2018 22:28

client: doc fixes and renaming from code review

d42fc47

logmon: set user to 'nobody' when possible

fc69e6b

client/driver: remove uid/gid plumbing from rotator since logmon will…

6c354e6

… run as desired user/group

dadgar requested changes Sep 24, 2018

View reviewed changes

logmon: dont' launch as seperate user

8d7778c

schmichael requested changes Sep 24, 2018

View reviewed changes

nickethier added 2 commits September 24, 2018 14:07

taskrunner: use shared config struct for logmon fifo fields

6664b01

pr comments

73687ab

schmichael approved these changes Sep 24, 2018

View reviewed changes

taskrunner: logmon only needs to be started once per task

f7db424

dadgar approved these changes Sep 24, 2018

View reviewed changes

Merge branch 'r-clientv2' into f-execv2

55aab34

nickethier merged commit db85f9e into r-clientv2 Sep 24, 2018

nickethier deleted the f-execv2 branch September 24, 2018 18:37

tgross mentioned this pull request Mar 4, 2021

Automatically mount cgroups if missing #2249

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2023

		@@ -172,101 +188,65 @@ type UniversalExecutor struct {
		processExited chan interface{}
		fsIsolationEnforced bool


		}

		func (l LibcontainerExecutor) getIsolationConfig() dstructs.IsolationConfig {


		func (h logmonHook) Stop(context.Context, interfaces.TaskStopRequest, *interfaces.TaskStopResponse) error {

		h.logmon.Stop()

	// UpdateStats updates and emits the latest stats from the driver.
	func (tr TaskRunner) UpdateStats(ru cstructs.TaskResourceUsage) {
	tr.resourceUsageLock.Lock()
	tr.resourceUsage = ru
	tr.resourceUsageLock.Unlock()
	if ru != nil {
	tr.emitStats(ru)
	}
	}

	func (m mockStatsUpdater) UpdateStats(ru cstructs.TaskResourceUsage) {
	if m.Ch != nil {
	m.Ch <- ru
	}
	}

executor v2 #4656

executor v2 #4656

Conversation

nickethier commented Sep 10, 2018

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

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

nickethier commented Sep 18, 2018

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

jippi commented Sep 21, 2018 • edited Loading

schmichael 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

schmichael left a comment • 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

nickethier commented Sep 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael 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

github-actions bot commented Feb 27, 2023

jippi commented Sep 21, 2018 •

edited

Loading

schmichael left a comment •

edited

Loading