Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

cs: provides env variables that contains the resources requested by the job #714

Merged
merged 4 commits into from Feb 5, 2018

Conversation

shamsimam
Copy link
Contributor

Changes proposed in this PR

  • Makes the job resources (cpus, gpus and mem) available as environment variables to the Mesos task

Why are we making these changes?

  • Given these environment variables, jobs can decide how much memory and threads they can allocate at runtime.

@shamsimam shamsimam self-assigned this Feb 1, 2018
@shamsimam
Copy link
Contributor Author

@wyegelwel build is now green, can you please merge?

@@ -85,6 +85,9 @@
environment (cond-> (assoc (util/job-ent->env job-ent)
"COOK_JOB_UUID" (-> job-ent :job/uuid str))
group-uuid (assoc "COOK_JOB_GROUP_UUID" (str group-uuid))
(:cpus resources) (assoc "COOK_JOB_CPUS" (-> resources :cpus int str))
(:gpus resources) (assoc "COOK_JOB_GPUS" (-> resources :gpus int str))
(:mem resources) (assoc "COOK_JOB_MEM_MB" (-> resources :mem int str))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we converting to int here? :cpus and :mem can both be floating point

Copy link
Contributor

@wyegelwel wyegelwel left a comment

Choose a reason for hiding this comment

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

I agree with @dposada, please remove the int before we merge

@shamsimam shamsimam dismissed wyegelwel’s stale review February 2, 2018 20:30

removed the int cast

@shamsimam
Copy link
Contributor Author

@wyegelwel ready for another round of review

@wyegelwel
Copy link
Contributor

Would you mind adding a note to our documentation that these environment variables are set?

@shamsimam
Copy link
Contributor Author

Added documentation in the faq section.

## Does Cook set any environment variables before running a task?

The following environment variables are set by Cook before running a task:
- COOK_JOB_UUID: represents the UUID of the Job the task belongs to.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make "job" lowercase here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


The following environment variables are set by Cook before running a task:
- COOK_JOB_UUID: represents the UUID of the Job the task belongs to.
- COOK_JOB_GROUP_UUID: represents the UUID of the Group the task belongs to.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make "group" lowercase, and maybe change to "job group"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

- COOK_JOB_MEM_MB: represents the amount of `mem` (in megabytes) configured in the Job. It can be a fractional number.

Any environment variables configured in the Job are also included.
In addition, when running an Instance using the Cook Executor, the executor configurations are passed as environment variables.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make "instance" lowercase here. It's also not so clear what is meant by "the executor configurations" - is there another area of the docs that we can link to that explains this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made lowercase. Added relative link.

- COOK_JOB_MEM_MB: represents the amount of `mem` (in megabytes) configured in the job. It can be a fractional number.

Any environment variables configured in the Job are also included.
In addition, when running an instance using the Cook Executor, the [executor configurations](configuration.adoc) are passed as environment variables.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to link to the executor section within that page, since you have to scroll quite a bit before you get to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

- COOK_JOB_GROUP_UUID: represents the UUID of the job group the task belongs to.
- COOK_JOB_CPUS: represents the amount of `cpus` configured in the job. It can be a fractional number.
- COOK_JOB_GPUS: represents the amount of `gpus` configured in the job. It can be a fractional number.
- COOK_JOB_MEM_MB: represents the amount of `mem` (in megabytes) configured in the job. It can be a fractional number.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: maybe we should put the environment variable names in backticks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@shamsimam
Copy link
Contributor Author

Build is green.

@dposada dposada merged commit 689ac99 into master Feb 5, 2018
@dposada dposada deleted the cook-resource-in-environ branch February 5, 2018 18:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants