Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add cwd to exec drivers #24249

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

add cwd to exec drivers #24249

wants to merge 9 commits into from

Conversation

mismithhisler
Copy link
Member

Adds support for setting the working directory in exec and raw_exec drivers.

Fixes: #2224

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

This is looking great @mismithhisler!

Two big picture items:

  • It looks like the ticket didn't include the java driver but it's nearly identical to the exec driver and uses all the same underlying bits. We should probably add that as well.
  • We're referring to this as the "working directory for the task" but we already document a "task working directory" in the Filesystem docs, which is the directory where Nomad creates the local/ dir, etc. That directory is the default value of work_dir, isn't it? That leads to a few questions:
    • Should we use the work_dir as part of the binary search path we describe in the command docs?
    • Is it possible to simply replace the value of the TaskDir field with the value of work_dir if set to avoid having an extra field in our protobufs, or does the executor need to know about the fine-grained difference?
    • How do we disambiguate between the two in our docs?

Args: []string{"foo.txt"},
WorkDir: workDir,
}
require.NoError(task.EncodeConcreteDriverConfig(&tc))
Copy link
Member

Choose a reason for hiding this comment

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

For new tests we should be swapping out testify for shoenig/test.

@@ -355,6 +359,7 @@ func (d *Driver) StartTask(cfg *drivers.TaskConfig) (*drivers.TaskHandle, *drive
Env: cfg.EnvList(),
User: cfg.User,
TaskDir: cfg.TaskDir().Dir,
WorkDir: driverConfig.WorkDir,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we're not validating the work_dir is absolute the way we're doing for the exec driver.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch

@@ -598,6 +598,55 @@ func TestRawExecDriver_Exec(t *testing.T) {
require.NoError(harness.DestroyTask(task.ID, true))
}

func TestRawExecDriver_WorkDir(t *testing.T) {
ci.Parallel(t)
ctestutil.ExecCompatible(t)
Copy link
Member

Choose a reason for hiding this comment

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

ExecCompatible means we can only run this test on Linux and as root. Do we need that restriction here? If so, can we have a test that runs on Windows as well (rawexec is a popular driver for Windows users).

website/content/docs/drivers/exec.mdx Outdated Show resolved Hide resolved
@@ -53,6 +53,8 @@ the Nomad client has been hardened according to the [production][hardening] guid
- `oom_score_adj` - (Optional) A positive integer to indicate the likelihood of
the task being OOM killed (valid only for Linux). Defaults to 0.

- `work_dir` - (Optional) The working directory for the task.
Copy link
Member

Choose a reason for hiding this comment

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

Are there any restrictions on this? If it's relative, what will it be relative to?

@@ -482,7 +490,7 @@ func (e *UniversalExecutor) ExecStreaming(ctx context.Context, command []string,

cmd := exec.CommandContext(ctx, command[0], command[1:]...)

cmd.Dir = "/"
cmd.Dir = e.childCmd.Dir
Copy link
Member

Choose a reason for hiding this comment

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

This changes the working directory for alloc exec. We should probably document that in the work_dir docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is the one opinionated change I added here. But it makes work_dir function the same as setting the working directory in a container. I'll add something to the docs.

@mismithhisler
Copy link
Member Author

@tgross These are all good points.

Two big picture items:

  • It looks like the ticket didn't include the java driver but it's nearly identical to the exec driver and uses all the same underlying bits. We should probably add that as well.

I'll get this added.

  • We're referring to this as the "working directory for the task" but we already document a "task working directory" in the Filesystem docs, which is the directory where Nomad creates the local/ dir, etc. That directory is the default value of work_dir, isn't it? That leads to a few questions:

    • Should we use the work_dir as part of the binary search path we describe in the command docs?

That's a good suggestion.

  • Is it possible to simply replace the value of the TaskDir field with the value of work_dir if set to avoid having an extra field in our protobufs, or does the executor need to know about the fine-grained difference?

The executor uses TaskDir to lookup the executable, so if we overwrite it in the driver, we won't be able to search it for the executable. Unless we want to make the search exclusive?

  • How do we disambiguate between the two in our docs?

An idea here is to change task working directory to just task directory. But then there is the environment variable named NOMAD_TASK_DIR which still points to the /local directory within the task directory, and that would be confusing.

@tgross
Copy link
Member

tgross commented Oct 17, 2024

The executor uses TaskDir to lookup the executable, so if we overwrite it in the driver, we won't be able to search it for the executable. Unless we want to make the search exclusive?

Eh, come to think of it having the work_dir overwrite TaskDir is only going to make it more confusing, so let's keep that at is and just make sure we've documented all the places where the driver will treat them differently.

@mismithhisler mismithhisler changed the title F add cwd to exec drivers add cwd to exec drivers Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Configurable working directory for exec/raw_exec
4 participants