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.

Sign up for GitHub

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

Jump to bottom

Add more configuration options for helm chart #2788

Merged
merged 3 commits into from
Jun 28, 2023
Merged

Add more configuration options for helm chart #2788

merged 3 commits into from
Jun 28, 2023

Conversation

Aakcht
Copy link
Contributor

@Aakcht Aakcht commented Apr 14, 2023

This PR adds possibility to specify affinity/tolerations/securityContext/nodeSelector/labels in helm chart for main Volcano pods(scheduler/admission/controller).

resolves #2779

@volcano-sh-bot volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2023
@Aakcht
Copy link
Contributor Author

Aakcht commented Apr 14, 2023

Looks like check fail is not related to the changes of this PR. @wangyang0616 , please, take a look.

@wangyang0616
Copy link
Member

Looks like check fail is not related to the changes of this PR. @wangyang0616 , please, take a look.

See issue: #2777, this issue has proposed a repair PR (#2780), and it should be able to solve the current CI problem after being merged

@wangyang0616
Copy link
Member

/priority important-soon

@volcano-sh-bot volcano-sh-bot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 18, 2023
@Aakcht
Copy link
Contributor Author

Aakcht commented Apr 18, 2023

The repair PR (#2780) helped, after rebasing the checks are successful.

william-wang
william-wang approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@william-wang william-wang left a comment

Choose a reason for hiding this comment

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

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2023
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: william-wang

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 23, 2023
@william-wang
Copy link
Member

@Aakcht please help to handle the confliction:)

@Aakcht Aakcht force-pushed the helm_more_install_options branch from 9f5af53 to 6e847df Compare May 23, 2023 04:27
@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label May 23, 2023
@Aakcht
Copy link
Contributor Author

Aakcht commented May 23, 2023 •
edited
Loading

Hi, @william-wang . As I see, there was this PR #2836 that was merged and it added nodeSelector & affinity & tolerations(it caused the conflicts). I rebased and reverted the changes from that PR since I think it would be better to be able to set nodeSelector/affinity/tolerations per component also(as in this PR). If you think it would be better to change default nodeSelector/affinity/tolerations parameter names in accordance with #2836, please tell me.

@william-wang
Copy link
Member

@Aakcht Agree with you, it's better to configure affinity/toleration/selector per component. Please make the ci happy and we can merge this pr.

@Aakcht
Copy link
Contributor Author

Aakcht commented May 23, 2023

@william-wang looks like I'll need help with the CI.

  • It seems Code Verify / Verify codes, generated files (pull_request) check failure is not related to this PR - I can see the same error in master branch.
  • E2E Spark Integration Test / E2E about Spark Integration test (pull_request) I don't see any errors in the logs with this one, looks like it simply failed because of timeout. I suspect rerunning the check might help, but as I see this check is not required, so I think the previous check error should be fixed first.

@Aakcht
Copy link
Contributor Author

Aakcht commented May 23, 2023

It seems Code Verify / Verify codes, generated files (pull_request) check failure is not related to this PR - I can see the same error in master branch.

I also see the same error as in Code Verify / Verify codes, generated files (pull_request) check in the latest PR. So I'll rebase this PR when I see that the checks in master/latest PRs are ok.

@wangyang0616
Copy link
Member

wangyang0616 commented May 23, 2023 •
edited
Loading

@william-wang looks like I'll need help with the CI.

  • It seems Code Verify / Verify codes, generated files (pull_request) check failure is not related to this PR - I can see the same error in master branch.
  • E2E Spark Integration Test / E2E about Spark Integration test (pull_request) I don't see any errors in the logs with this one, looks like it simply failed because of timeout. I suspect rerunning the check might help, but as I see this check is not required, so I think the previous check error should be fixed first.

@Aakcht Master's code verify encountered an error and has been repaired.

refer to pr: fix goimport check error #2849

After the #2849 is merged, you can rebase the master.

@Aakcht Aakcht force-pushed the helm_more_install_options branch from 6e847df to 65c9805 Compare May 24, 2023 12:01
@Aakcht
Copy link
Contributor Author

Aakcht commented May 24, 2023

Hello, @wangyang0616 , @william-wang I rebased the PR, now all the checks are successful.

@Aakcht Aakcht force-pushed the helm_more_install_options branch 2 times, most recently from 4ece413 to 64768c9 Compare June 1, 2023 10:15
@Aakcht
Copy link
Contributor Author

Aakcht commented Jun 1, 2023

Hi, @william-wang, I rebased the PR one more time case there were new merge conflicts - now all should be good, please take a look.

@jacobsalway
Copy link

Hey all, just adding that this PR would ease the Helm chart configuration for my specific deployment circumstance. I need to add tolerations and node selectors to all deployment components, but #2836 on master is missing the configurability on the admission-init job, which this PR adds.

@Aakcht Aakcht force-pushed the helm_more_install_options branch from 64768c9 to 6f4136a Compare June 16, 2023 07:21
@Aakcht
Copy link
Contributor Author

Aakcht commented Jun 16, 2023

Hi, @hwdef @wangyang0616 @Thor-wl @william-wang , looking for review/aproval/merge.

@Aakcht Aakcht force-pushed the helm_more_install_options branch 2 times, most recently from d6fe4b3 to ae1f151 Compare June 16, 2023 12:27
@hwdef
Copy link
Member

hwdef commented Jun 17, 2023

Please close PR and reopen it to retrigger CI

@Aakcht Aakcht closed this Jun 17, 2023
@Aakcht Aakcht reopened this Jun 17, 2023
@Aakcht
Copy link
Contributor Author

Aakcht commented Jun 17, 2023

@hwdef I reopened it, but the failed check remained and I'm pretty sure that the failed check is not related to this PR (You can also see the same error in the latest opened PR #2920) .

Also it looks like E2E about Spark Integration test check is not mandatory since it does not have required tag.

@hwdef
Copy link
Member

hwdef commented Jun 17, 2023

@wangyang0616 Please check this CI failed.

@Aakcht
Add more configuration options for helm chart
c13707c 
Signed-off-by: aakcht <aakcht@gmail.com>
@Aakcht
resolve merge conflicts
cc765e8 
Signed-off-by: aakcht <aakcht@gmail.com>
@Aakcht
avoid null resources in volcano-development.yaml
eb1f1c4 
Signed-off-by: aakcht <aakcht@gmail.com>
@Aakcht Aakcht force-pushed the helm_more_install_options branch from ae1f151 to eb1f1c4 Compare June 27, 2023 13:16
@Aakcht
Copy link
Contributor Author

Aakcht commented Jun 27, 2023

Hi, @hwdef @wangyang0616 @Thor-wl @william-wang , please take a look, the checks are now successful.

hwdef
hwdef reviewed Jun 28, 2023
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023
@volcano-sh-bot volcano-sh-bot merged commit 92646d2 into volcano-sh:master Jun 28, 2023
12 checks passed
@Aakcht Aakcht deleted the helm_more_install_options branch June 28, 2023 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hwdef hwdef hwdef left review comments

@william-wang william-wang william-wang approved these changes

@jasonliu747 jasonliu747 Awaiting requested review from jasonliu747

Assignees

@william-wang william-wang

@hwdef hwdef

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow setting securityContext and resources for pods in volcano helm charts
6 participants
@Aakcht @wangyang0616 @volcano-sh-bot @william-wang @jacobsalway @hwdef