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

[kjobctl] Move common SLURM_envs to container env vars. #3285

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mbobrovskyi
Copy link
Contributor

@mbobrovskyi mbobrovskyi commented Oct 22, 2024

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?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Oct 22, 2024
@mbobrovskyi mbobrovskyi marked this pull request as draft October 22, 2024 15:11
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 22, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mbobrovskyi
Once this PR has been reviewed and has the lgtm label, please assign trasc for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2024
Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit f1b4e3c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/671876ba1623560008b06e04

@mwielgus
Copy link
Contributor

I'm afraid this will make the already big slurm job/pod definition significantly bigger. Is why is it needed?

@mbobrovskyi
Copy link
Contributor Author

mbobrovskyi commented Oct 22, 2024

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.

@mbobrovskyi
Copy link
Contributor Author

In another way we need to create a lot of flags for go slurm-init to send all of this values.

@mimowo @trasc WDYT?

@mbobrovskyi
Copy link
Contributor Author

Another approach I also can see to get it from configmap.

@mwielgus
Copy link
Contributor

Configmap sounds ok.

@mbobrovskyi mbobrovskyi force-pushed the cleanup/move-slurm-envs-to-container-env-vars branch 3 times, most recently from 9311909 to e063352 Compare October 22, 2024 18:56
@mbobrovskyi mbobrovskyi changed the title Move common SLURM_envs to container env vars. [kjobctl] Move common SLURM_envs to container env vars. Oct 22, 2024
@mbobrovskyi mbobrovskyi force-pushed the cleanup/move-slurm-envs-to-container-env-vars branch 2 times, most recently from 4a95ab0 to 8e6cd4b Compare October 23, 2024 03:39
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2024
@mbobrovskyi mbobrovskyi force-pushed the cleanup/move-slurm-envs-to-container-env-vars branch from 8e6cd4b to f1b4e3c Compare October 23, 2024 04:08
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 23, 2024
@trasc
Copy link
Contributor

trasc commented Oct 23, 2024

In another way we need to create a lot of flags for go slurm-init to send all of this values.

@mimowo @trasc WDYT?

I don't think that loading the env vars from a = separated file is such a big challenge in a go app (split by \n then by =). So, at least for now, we should not add an extra config map just for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants