-
Notifications
You must be signed in to change notification settings - Fork 268
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
[kjobctl] Move common SLURM_envs to container env vars. #3285
[kjobctl] Move common SLURM_envs to container env vars. #3285
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mbobrovskyi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
I'm afraid this will make the already big slurm job/pod definition significantly bigger. Is why is it needed? |
We discussed on the previous weekly meeting to create init container on go. Before that trying to optimize init script. |
Another approach I also can see to get it from configmap. |
Configmap sounds ok. |
9311909
to
e063352
Compare
4a95ab0
to
8e6cd4b
Compare
8e6cd4b
to
f1b4e3c
Compare
I don't think that loading the env vars from a |
/close In the case where we are using the current ConfigMap, it will be addressed in #3293. |
@mbobrovskyi: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Move common SLURM_ envs to container env vars on kjobctl.
Which issue(s) this PR fixes:
Prepare #3269
Special notes for your reviewer:
We have many variables that are the same in each container, so there's no need to generate them in the init container. Instead of that we can set it as a container env vars on builder.
Does this PR introduce a user-facing change?