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

In webhook add match condition by scheduler name filter pod resources #3748

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

In webhook add match condition by scheduler name filter pod resources #3748

wants to merge 1 commit into from
+17 −5

Conversation

lengrongfu
Copy link
Contributor

@lengrongfu lengrongfu commented Sep 26, 2024 •
edited
Loading

Add matchConditions in webhook when rules is pod resources create operation, to validate object.spec.schedulerName whether is default volcano scheduler name volcano, current we don't consider scheduler name change.

/kind feature

fixes: #3734

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign wpeng102
You can assign the PR to them by writing /assign @wpeng102 in a comment when ready.

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

@lengrongfu
Copy link
Contributor Author

cc @Monokaix

@volcano-sh-bot volcano-sh-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 26, 2024
@volcano-sh-bot volcano-sh-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 26, 2024
@lengrongfu lengrongfu force-pushed the feat/update-webhook branch 2 times, most recently from da6069b to 8466dba Compare September 26, 2024 06:11
@volcano-sh-bot volcano-sh-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 26, 2024
@Monokaix
Copy link
Member

Monokaix commented Sep 26, 2024 •
edited
Loading

Are there other webhook also need this?

@Monokaix
Copy link
Member

Why add a new file installer/volcano-latest.yaml, you want to override the vocano-deployment.yaml?

@lengrongfu
Copy link
Contributor Author

Are there other webhook also need this?

I don't think other webhook are needed because they are all volcano resources.

@lengrongfu
Copy link
Contributor Author

Why add a new file installer/volcano-latest.yaml, you want to override the vocano-deployment.yaml?

No, because i merge pr fail for ci jobs in lint check step. so i followed the prompts generate installer/volcano-latest.yaml this file.

image

https://github.com/volcano-sh/volcano/actions/runs/11046591401/job/30686212838

@Monokaix
Copy link
Member

Why add a new file installer/volcano-latest.yaml, you want to override the vocano-deployment.yaml?

No, because i merge pr fail for ci jobs in lint check step. so i followed the prompts generate installer/volcano-latest.yaml this file.

image

https://github.com/volcano-sh/volcano/actions/runs/11046591401/job/30686212838

This file should mv to vocano-deployment.yaml and override it.

@lengrongfu
Copy link
Contributor Author

Why add a new file installer/volcano-latest.yaml, you want to override the vocano-deployment.yaml?

No, because i merge pr fail for ci jobs in lint check step. so i followed the prompts generate installer/volcano-latest.yaml this file.
image
https://github.com/volcano-sh/volcano/actions/runs/11046591401/job/30686212838

This file should mv to vocano-deployment.yaml and override it.

got it, we can change this tips? make it executable locally.

make generate-yaml TAG=latest RELEASE_DIR=installer && mv ./installer/volcano-latest.yaml  ./installer/volcano-development.yaml

@Monokaix
Copy link
Member

RELEASE_DIR

You can do that but notice that the generated file location is related with the env RELEASE_DIR.

@volcano-sh-bot volcano-sh-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 27, 2024
@lengrongfu
Copy link
Contributor Author

RELEASE_DIR

You can do that but notice that the generated file location is related with the env RELEASE_DIR.

Ok，thanks， having remove release file

@hwdef
Copy link
Member

hwdef commented Sep 30, 2024

I'm not sure if this change is necessary, or I prefer not to accept it.
volcano allows you to modify the scheduler name to implement multiple schedulers in one cluster.
Therefore it is inappropriate to fix the name of the scheduler in the webhook

@lengrongfu
Copy link
Contributor Author

I'm not sure if this change is necessary, or I prefer not to accept it.

volcano allows you to modify the scheduler name to implement multiple schedulers in one cluster.

Therefore it is inappropriate to fix the name of the scheduler in the webhook

The volcano scheduling name can be modified, but I don't see this parameter in the helm chart

@hwdef
Copy link
Member

hwdef commented Oct 8, 2024

please check this code.

https://github.com/volcano-sh/volcano/blob/0c9b846f2e871e6658122fcf2efdf653a1d99946/cmd/scheduler/app/options/options.go#L118C66-L118C86

@lengrongfu
Copy link
Contributor Author

please check this code.

https://github.com/volcano-sh/volcano/blob/0c9b846f2e871e6658122fcf2efdf653a1d99946/cmd/scheduler/app/options/options.go#L118C66-L118C86

I mean there is no place to modify in the helm chart.

@hwdef
Copy link
Member

hwdef commented Oct 9, 2024

We can add this field instead of fixing its name

@Monokaix
Copy link
Member

Monokaix commented Oct 9, 2024

We can add this field instead of fixing its name

set a separate custom flag for helm?

@hwdef
Copy link
Member

hwdef commented Oct 9, 2024

We can add this field instead of fixing its name

set a separate custom flag for helm?

Add a field in helm to modify scheduler-name

@lengrongfu
Copy link
Contributor Author

We can add this field instead of fixing its name

set a separate custom flag for helm?

Add a field in helm to modify scheduler-name

I can commit a pr to add this field.

@Monokaix
Copy link
Member

We can add this field instead of fixing its name

set a separate custom flag for helm?

Add a field in helm to modify scheduler-name

I can commit a pr to add this field.

You can do it in this pr.

@lengrongfu lengrongfu mentioned this pull request Oct 11, 2024
Merged
@Monokaix
Copy link
Member

/ok-to-test

@volcano-sh-bot volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 18, 2024
@Monokaix
Copy link
Member

Hi, any update?

@lengrongfu
Copy link
Contributor Author

Hi, any update?

no update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Monokaix Monokaix Monokaix left review comments

@hwdef hwdef hwdef left review comments

@lowang-bh lowang-bh Awaiting requested review from lowang-bh

@shinytang6 shinytang6 Awaiting requested review from shinytang6

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When volcano-admission pod not running, create other pod can faild
4 participants
@lengrongfu @volcano-sh-bot @Monokaix @hwdef