use replicas when initializing pg minResources #4000

sceneryback · 2025-02-07T09:43:20Z

When creating pod using replicaSet, the minMember of podgroup created is always 1, this is not reasonable. Consider this case:

we created a deployment with 2 replicas, totally requesting 16 gpus
the podgroup created has 1 minMember and 8 gpus
the capability of queue has 8 gpus and the scheduler configmap disables AllocatableFn of proportion plugin
the podgroup will be enqueued and running since minResources(8 gpus) is not bigger than queue capability
finally the queue allocated 16 gpus, more than its deserved

As can be seen from this picture:

In the case that pod has replicas from its owners, the podgroup should consider relicas, just as training-operator does, which creates pod group based on resources from all replicas

sceneryback · 2025-02-07T09:47:16Z

/assign @hzxuzhonghu

hwdef · 2025-02-07T10:09:05Z

Same situation as mentioned in this issue:
#3970

Monokaix · 2025-02-07T10:14:54Z

proportion plugin controls the queue related capabilities, if you disable it, the capacity check will not take effect.

Monokaix · 2025-02-07T10:16:40Z

Just as @hwdef said, please set an annotation to control the minmember to set, and also other workload type like statefulset should also has this capability: )

sceneryback · 2025-02-08T02:54:35Z

hi @hwdef @Monokaix thanks for all your suggestions, I have updated the feature using new annotation, so it won't break current behaviors. Please have a look.
apis change: volcano-sh/apis#154
I will update go.mod to delete local references of apis package after volcano-sh/apis#154 is merged

pkg/controllers/podgroup/pg_controller_handler.go

JesseStutler · 2025-02-10T12:27:11Z

pkg/controllers/podgroup/pg_controller_handler.go

+
+	for _, reference := range pod.OwnerReferences {
+		if reference.Kind != "" && reference.Name != "" {
+			tmp := pg.getAnnotationsFromUpperRes(reference.Kind, reference.Name, pod.Namespace)


Usually we write annotation on the deployment to specify minMember, but here the ownerReferences of the Pod can only be traced back to the replicaset.

From k8s source code https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/deployment/util/deployment_util.go#L235, the ReplicaSet will inherit annotations from Deployment, so I think it's enough to get annotations from RS

JesseStutler · 2025-02-10T12:30:04Z

pkg/controllers/podgroup/pg_controller_handler.go

+	for _, reference := range pod.OwnerReferences {
+		if reference.Kind != "" && reference.Name != "" {
+			tmp := pg.getAnnotationsFromUpperRes(reference.Kind, reference.Name, pod.Namespace)
+			if err := mergo.Merge(&annotations, &tmp); err != nil {


Do we have scenarios that Pod has multiple OwnerReferences? If there are trully multiple annotations, we won't specify twice the minMember annotations, do we need to use the merge.Merge?

Normally there won't be multiple OwnerReferences with this annotation, updated. Thanks

JesseStutler · 2025-02-10T12:31:58Z

pkg/controllers/podgroup/pg_controller_handler.go

@@ -177,6 +181,37 @@ func (pg *pgcontroller) getAnnotationsFromUpperRes(kind string, name string, nam
 	}
 }

+func (pg *pgcontroller) getMinMemberFromUpperRes(pod *v1.Pod) *int32 {
+	defaultMinMember := int32(1)


This variable name is very strange. You can set 1 as a constant (as defaultMinMember), and then the variable here is initialized to defaultMinMember. minMember here is ok

JesseStutler · 2025-02-10T12:33:15Z

pkg/controllers/podgroup/pg_controller_handler.go

@@ -193,7 +228,7 @@ func (pg *pgcontroller) inheritUpperAnnotations(pod *v1.Pod, obj *scheduling.Pod
 	}
 }

-func (pg *pgcontroller) createNormalPodPGIfNotExist(pod *v1.Pod) error {
+func (pg *pgcontroller) createNormalPodPGIfNotExist(pod *v1.Pod, minAvailable *int32) error {


Why are the variable names here not unified as minMember?

JesseStutler · 2025-02-10T12:34:13Z

pkg/controllers/podgroup/pg_controller_handler.go

@@ -203,6 +238,11 @@ func (pg *pgcontroller) createNormalPodPGIfNotExist(pod *v1.Pod) error {
 			return err
 		}

+		minMember := int32(1)


There is no need to initialize again here, you already have the initialization value in getMinMemberFromUpperRes.

JesseStutler · 2025-02-10T12:41:27Z

pkg/controllers/job/job_controller_util.go

@@ -327,10 +327,10 @@ func (p TasksPriority) CalcFirstCountResources(count int32) v1.ResourceList {

 	for _, task := range p {
 		if count <= task.Replicas {
-			minReq = quotav1.Add(minReq, calTaskRequests(&v1.Pod{Spec: task.Template.Spec}, count))
+			minReq = quotav1.Add(minReq, *util.CalTaskRequests(&v1.Pod{Spec: task.Template.Spec}, count))


The return value of the function here is a pointer, and then you use * directly to get the value pointed to by the pointer, the way it's written looks a bit strange and doesn't quite fit in with go's clean code. The return value of the func CalTaskRequests does not need to be refactored, keeping the original is better. In fact, when the API was designed, minResources should not be a pointer, ResourceList is a map.

simply copied from an existing util function:

I will update GetPodQuotaUsage too
Also notice that the minResources field of PodGroup is designed as pointer originally, so currently won't break this

sceneryback · 2025-02-11T03:19:51Z

hi @JesseStutler Thanks for your reviewing! Have updated, please have a look

Monokaix · 2025-02-11T11:37:58Z

API related pr is merged, you can move on: )

sceneryback · 2025-02-12T01:40:53Z

Updated go.mod, please have a look @Monokaix @hwdef @JesseStutler

hwdef

/lgtm

hwdef · 2025-02-12T03:18:04Z

/ok-to-test

Monokaix · 2025-02-12T07:08:46Z

pkg/controllers/util/util.go

 	"k8s.io/kubernetes/pkg/apis/core/v1/helper"
 	quotacore "k8s.io/kubernetes/pkg/quota/v1/evaluator/core"
 	"k8s.io/utils/clock"
 )

-func GetPodQuotaUsage(pod *v1.Pod) *v1.ResourceList {
+func GetPodQuotaUsage(pod *v1.Pod) v1.ResourceList {


Why changed to non-poiner?

Just follow this comment from @JesseStutler for better style #4000 (comment)

Monokaix · 2025-02-12T07:30:41Z

pkg/controllers/podgroup/pg_controller.go

@@ -172,7 +172,8 @@ func (pg *pgcontroller) processNextReq() bool {

 	// normal pod use volcano
 	klog.V(4).Infof("Try to create podgroup for pod %s/%s", pod.Namespace, pod.Name)
-	if err := pg.createNormalPodPGIfNotExist(pod); err != nil {
+	minMember := pg.getMinMemberFromUpperRes(pod)


This logic should put in createNormalPodPGIfNotExist, because when the podgroup is already existed, there is no need to calculate the min member, it will request APIServer which is a heavy operation.
And there is also a qustion mentioned here #3970 (comment)

very good point 👍 so it's better to keep rs/sts etc informers in pgcontroller and read cache from these informers, right?

Yeah, DaemonSet and Job should also be considered.

Also this should also be considered #3970 (comment)

Also this should also be considered #3970 (comment)

I think it's not necessary to check pod annotation? The annotation is designed for workload, it will be strange to see this in pod

volcano-sh-bot · 2025-02-12T10:47:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign hwdef
You can assign the PR to them by writing /assign @hwdef 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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sceneryback · 2025-02-12T12:17:24Z

Refactored code using informers, please have a look again @hwdef @Monokaix @JesseStutler

hwdef · 2025-02-14T02:16:28Z

Hi, please make the CI happy

Signed-off-by: sceneryback <[email protected]>

sceneryback · 2025-02-19T10:36:17Z

hi @hwdef fixed the failed e2e tests, please have a look again

hwdef

/lgtm
Thanks!

hwdef · 2025-02-20T03:22:50Z

PTAL
@Monokaix @JesseStutler @lowang-bh

Monokaix · 2025-02-21T08:49:16Z

go.mod

@@ -46,7 +46,7 @@ require (
 	sigs.k8s.io/controller-runtime v0.13.0
 	sigs.k8s.io/yaml v1.4.0
 	stathat.com/c/consistent v1.0.0
-	volcano.sh/apis v1.10.0-alpha.0.0.20241210014034-bf27f4e986d0
+	volcano.sh/apis v1.11.1-0.20250211082520-7f8222e881d9


Why 1.11.1?

Monokaix · 2025-02-21T09:46:11Z

pkg/controllers/podgroup/pg_controller.go

@@ -99,15 +118,29 @@ func (pg *pgcontroller) Initialize(opt *framework.ControllerOption) error {
 		AddFunc: pg.addPod,
 	})

+	pg.rsInformer = pg.informerFactory.Apps().V1().ReplicaSets()


These should be under statement if utilfeature.DefaultFeatureGate.Enabled(features.WorkLoadSupport) {.

rsLister is also used in 'inheritUpperAnnotations()' when WorkLoadSupport is not enabled, so only add these codes under the statement:

pg.rsInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: pg.addReplicaSet, UpdateFunc: pg.updateReplicaSet, })

WorkLoadSupport feature means whether Volcano supports deployment/rs/ds/sts,etc., so the informers should be initiated when featuregate is not true.

Monokaix · 2025-02-21T09:47:58Z

pkg/controllers/podgroup/pg_controller.go

@@ -99,15 +118,29 @@ func (pg *pgcontroller) Initialize(opt *framework.ControllerOption) error {
 		AddFunc: pg.addPod,
 	})

+	pg.rsInformer = pg.informerFactory.Apps().V1().ReplicaSets()


I think we should add a param feature switch to users whether enable non-vcjob supports minMember, becasue it's a little heavy.

hi @Monokaix you mean a feature gate like features.NonVcjobMinMemberSupport which only applies logic in this PR when enabled?

a param flag is ok.

Monokaix · 2025-02-21T09:57:07Z

pkg/controllers/podgroup/pg_controller_handler.go

+					klog.Infof("Failed to convert minMemberAnnotation of Pod owners <%s/%s> into number, skipping",
+						pod.Namespace, pod.Name)
+					return minMember
+				}


should be positive, will fix

Monokaix · 2025-02-21T09:58:21Z

pkg/controllers/podgroup/pg_controller_handler.go

+			if minMemberAnno, ok := annotations[scheduling.VolcanoGroupMinMemberAnnotationKey]; ok {
+				minMemberFromAnno, err := strconv.Atoi(minMemberAnno)
+				if err != nil {
+					klog.Infof("Failed to convert minMemberAnnotation of Pod owners <%s/%s> into number, skipping",


This klog info is not so clear, we should mention that remain minmember =1, or users will don't know the value finally set.

Monokaix · 2025-02-21T09:58:32Z

pkg/controllers/podgroup/pg_controller_handler.go

+					return minMember
+				}
+				if minMemberFromAnno < math.MinInt32 || minMemberFromAnno > math.MaxInt32 {
+					klog.Infof("minMember %d exceeds bounds of int32, skipping", minMember)


same above.

JesseStutler · 2025-02-21T13:18:40Z

pkg/controllers/podgroup/pg_controller.go

@@ -99,15 +118,29 @@ func (pg *pgcontroller) Initialize(opt *framework.ControllerOption) error {
 		AddFunc: pg.addPod,
 	})

+	pg.rsInformer = pg.informerFactory.Apps().V1().ReplicaSets()
+	pg.rsLister = pg.rsInformer.Lister()
+	pg.rsSynced = pg.rsInformer.Informer().HasSynced


There is no need to add these rsSynced funcs, these codes:

for informerType, ok := range pg.informerFactory.WaitForCacheSync(stopCh) { if !ok { klog.Errorf("caches failed to sync: %v", informerType) } } for informerType, ok := range pg.vcInformerFactory.WaitForCacheSync(stopCh) { if !ok { klog.Errorf("caches failed to sync: %v", informerType) return } }

has already done this

JesseStutler · 2025-02-21T13:21:51Z

installer/helm/chart/volcano/templates/controllers.yaml

@@ -74,13 +74,13 @@ rules:
    verbs: ["get", "create", "delete"]
  - apiGroups: ["apps"]
    resources: ["daemonsets", "statefulsets"]
-    verbs: ["get"]
+    verbs: ["get", "list"]


If we change all these APIs to list/watch them, I think the verbs here should be ["list","watch"]?

JesseStutler · 2025-02-21T13:51:07Z

Better to post a screenshot to verify your codes work in actual environment thanks

volcano-sh-bot · 2025-02-22T15:07:55Z

New changes are detected. LGTM label has been removed.

volcano-sh-bot requested review from hwdef and hzxuzhonghu February 7, 2025 09:43

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 7, 2025

volcano-sh-bot assigned hzxuzhonghu Feb 7, 2025

hwdef mentioned this pull request Feb 7, 2025

Support setting minMember when creating workloads #3970

Open

sceneryback mentioned this pull request Feb 8, 2025

add minAvailable anno key volcano-sh/apis#154

Merged

github-advanced-security bot found potential problems Feb 8, 2025

View reviewed changes

pkg/controllers/podgroup/pg_controller_handler.go Fixed Show fixed Hide fixed

JesseStutler reviewed Feb 10, 2025

View reviewed changes

sceneryback force-pushed the pgcontroller_create_use_replicas branch from 4241911 to 05dd3e0 Compare February 12, 2025 01:00

hwdef reviewed Feb 12, 2025

View reviewed changes

volcano-sh-bot assigned hwdef Feb 12, 2025

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2025

volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 12, 2025

Monokaix reviewed Feb 12, 2025

View reviewed changes

volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2025

sceneryback force-pushed the pgcontroller_create_use_replicas branch from 2495285 to 27e95ed Compare February 12, 2025 12:09

volcano-sh-bot 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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 12, 2025

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 12, 2025

sceneryback force-pushed the pgcontroller_create_use_replicas branch from 7c46da3 to 2922e6d Compare February 19, 2025 09:21

use group-min-member annotation to initialize PodGroup

be21116

Signed-off-by: sceneryback <[email protected]>

sceneryback force-pushed the pgcontroller_create_use_replicas branch from ad05f15 to be21116 Compare February 19, 2025 09:27

Merge branch 'master' into pgcontroller_create_use_replicas

b94a929

hwdef reviewed Feb 19, 2025

View reviewed changes

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2025

Monokaix reviewed Feb 21, 2025

View reviewed changes

JesseStutler reviewed Feb 21, 2025

View reviewed changes

Merge branch 'master' into pgcontroller_create_use_replicas

a7394fa

volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2025

use replicas when initializing pg minResources #4000

Are you sure you want to change the base?

use replicas when initializing pg minResources #4000

Conversation

sceneryback commented Feb 7, 2025

sceneryback commented Feb 7, 2025

hwdef commented Feb 7, 2025

Monokaix commented Feb 7, 2025

Monokaix commented Feb 7, 2025

sceneryback commented Feb 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sceneryback commented Feb 11, 2025

Monokaix commented Feb 11, 2025

sceneryback commented Feb 12, 2025

hwdef left a comment

Choose a reason for hiding this comment

hwdef commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volcano-sh-bot commented Feb 12, 2025

sceneryback commented Feb 12, 2025

hwdef commented Feb 14, 2025

sceneryback commented Feb 19, 2025

hwdef left a comment

Choose a reason for hiding this comment

hwdef commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseStutler commented Feb 21, 2025

volcano-sh-bot commented Feb 22, 2025