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

Support setting minMember when creating workloads #3970

Open
hwdef opened this issue Jan 14, 2025 · 16 comments
Open

Support setting minMember when creating workloads #3970

hwdef opened this issue Jan 14, 2025 · 16 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Milestone

Comments

@hwdef
Copy link
Member

hwdef commented Jan 14, 2025

What is the problem you're trying to solve

Currently, the default value of minMember is 1, which is inappropriate in some scenarios.

I hope that minMember can be set when creating a deployment or other non-vcjob workloads.

Describe the solution you'd like

When creating a podgroup, we can read an annotation, such as
scheduling.volcano.sh/minMember: 10
Set minMember based on this value

Additional context

No response

@hwdef hwdef added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 14, 2025
@noobzzw
Copy link

noobzzw commented Jan 19, 2025

Hi, this is a great idea. Besides setting the minMember manually, can we also read the StatefulSet or ReplicaSet in the ownerReferences to obtain the minMember automatically?

@hwdef
Copy link
Member Author

hwdef commented Jan 20, 2025

This is probably not a good idea since many deployment functions don't want to be affected by the gang.

@noobzzw
Copy link

noobzzw commented Jan 20, 2025

got it.

@Monokaix
Copy link
Member

Monokaix commented Feb 5, 2025

I think we can support read an annotation key from the workload, and users can set an expected value, or else the minmember is still 1.

@Monokaix Monokaix added this to the v1.12 milestone Feb 5, 2025
@JesseStutler
Copy link
Member

scheduling.volcano.sh/minMember or scheduling.volcano.sh/minAvailable? From the perspective of vcjob, is minAvailable, deployment/statefulset/daemonset/job and vcjob are at the same level, so should it be minAvailable?

@Monokaix
Copy link
Member

Monokaix commented Feb 5, 2025

minAvailable is the semantics of vcjob, and for other workloads users may just need to concern the podgroup.
But the user experience will be more unified usingminAvailable.

@Monokaix
Copy link
Member

Monokaix commented Feb 5, 2025

Another question is: The annotation should be set at pod level or workload level, though its more reasonable setting at workload level, the current queue and podgroup annotations are both set at pod level, so it's better to keep consistent.
How do you guys think?

@hwdef
Copy link
Member Author

hwdef commented Feb 6, 2025

I think it's more reasonable to set at the workload level.
But we can do compatibility. priority reading the workload annotation. If not exist, then read the pod annotation.

@Monokaix
Copy link
Member

Monokaix commented Feb 6, 2025

I think it's more reasonable to set at the workload level. But we can do compatibility. priority reading the workload annotation. If not exist, then read the pod annotation.

It seems a little confused...

@lowang-bh
Copy link
Member

It is more reasonable setting at workload level.

@hwdef
Copy link
Member Author

hwdef commented Feb 7, 2025

We also should consider minResource

Ref: #4000

@Monokaix
Copy link
Member

Monokaix commented Feb 7, 2025

/assign @sceneryback

@volcano-sh-bot
Copy link
Contributor

@Monokaix: GitHub didn't allow me to assign the following users: sceneryback.

Note that only volcano-sh members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @sceneryback

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/test-infra repository.

@sceneryback
Copy link
Contributor

@hwdef @Monokaix @lowang-bh @JesseStutler Thanks for all your suggestions, have implemented this feature in #4000 and volcano-sh/apis#154, please have a look

@Monokaix
Copy link
Member

It is more reasonable setting at workload level.

There is also a question when setting at workload level, there will be an extra API request to get the workload's annotations, which may cause performance at scale.

@Monokaix
Copy link
Member

It is more reasonable setting at workload level.

There is also a question when setting at workload level, there will be an extra API request to get the workload's annotations, which may cause performance at scale.

A workaround is that we cache annotations of rs/sts, etc. and get annotations from cache, which not only solve the current problem but also inheritUpperAnnotations can avoid to request APIServer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

7 participants