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

Drivers: add cwd to exec/java drivers #24249

Merged
merged 13 commits into from
Nov 1, 2024
Merged

Conversation

mismithhisler
Copy link
Member

@mismithhisler mismithhisler commented Oct 17, 2024

Adds support for setting the working directory in exec, raw_exec, and java 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?

drivers/rawexec/driver_test.go Outdated Show resolved Hide resolved
drivers/rawexec/driver.go Show resolved Hide resolved
drivers/rawexec/driver_test.go Outdated Show resolved Hide resolved
website/content/docs/drivers/exec.mdx Outdated Show resolved Hide resolved
website/content/docs/drivers/raw_exec.mdx Outdated Show resolved Hide resolved
drivers/shared/executor/executor.go Show resolved Hide resolved
@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
@mismithhisler mismithhisler force-pushed the f-add-cwd-to-exec-drivers branch from 2ae0411 to db5ee6b Compare October 23, 2024 21:42
@mismithhisler mismithhisler changed the title add cwd to exec drivers Drivers: add cwd to exec/java drivers Oct 23, 2024
Copy link
Member

@Juanadelacuesta Juanadelacuesta left a comment

Choose a reason for hiding this comment

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

This looks pretty straight forward, nice work! Remember to add the backport label.

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.

LGTM (pending my comments on the binary blobs)

@mismithhisler mismithhisler added the backport/1.9.x backport to 1.9.x release line label Oct 31, 2024
Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mismithhisler!

@mismithhisler mismithhisler merged commit 658c429 into main Nov 1, 2024
31 checks passed
@mismithhisler mismithhisler deleted the f-add-cwd-to-exec-drivers branch November 1, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.9.x backport to 1.9.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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