use replicas when initializing pg minResources #4000
Conversation
volcano-sh-bot
added
the
size/L
|
/assign @hzxuzhonghu |
|
Same situation as mentioned in this issue: |
|
proportion plugin controls the queue related capabilities, if you disable it, the capacity check will not take effect. |
|
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: ) |
|
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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| for _, reference := range pod.OwnerReferences { | ||
| if reference.Kind != "" && reference.Name != "" { | ||
| tmp := pg.getAnnotationsFromUpperRes(reference.Kind, reference.Name, pod.Namespace) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Normally there won't be multiple OwnerReferences with this annotation, updated. Thanks
| @@ -177,6 +181,37 @@ func (pg *pgcontroller) getAnnotationsFromUpperRes(kind string, name string, nam | |||
| } | |||
| } | |||
|
|
|||
| func (pg *pgcontroller) getMinMemberFromUpperRes(pod *v1.Pod) *int32 { | |||
| defaultMinMember := int32(1) | |||
There was a problem hiding this comment.
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
| @@ -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 { | |||
There was a problem hiding this comment.
Why are the variable names here not unified as minMember?
| @@ -203,6 +238,11 @@ func (pg *pgcontroller) createNormalPodPGIfNotExist(pod *v1.Pod) error { | |||
| return err | |||
| } | |||
|
|
|||
| minMember := int32(1) | |||
There was a problem hiding this comment.
There is no need to initialize again here, you already have the initialization value in getMinMemberFromUpperRes.
| @@ -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)) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
hi @JesseStutler Thanks for your reviewing! Have updated, please have a look |
|
API related pr is merged, you can move on: ) |
sceneryback
force-pushed
the
pgcontroller_create_use_replicas
branch
from
4241911 to
05dd3e0
Compare
|
Updated go.mod, please have a look @Monokaix @hwdef @JesseStutler |
|
/ok-to-test |
volcano-sh-bot
added
the
ok-to-test
| "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 { |
There was a problem hiding this comment.
Just follow this comment from @JesseStutler for better style #4000 (comment)
| @@ -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) | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
very good point 👍 so it's better to keep rs/sts etc informers in pgcontroller and read cache from these informers, right?
There was a problem hiding this comment.
Yeah, DaemonSet and Job should also be considered.
There was a problem hiding this comment.
Also this should also be considered #3970 (comment)
There was a problem hiding this 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
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Signed-off-by: sceneryback <[email protected]>
sceneryback
force-pushed
the
pgcontroller_create_use_replicas
branch
from
2495285 to
27e95ed
Compare
volcano-sh-bot
removed
the
size/L
volcano-sh-bot
added
the
size/XL
volcano-sh-bot
added
size/L
|
Refactored code using informers, please have a look again @hwdef @Monokaix @JesseStutler |
|
Hi, please make the CI happy |
When creating pod using replicaSet, the minMember of podgroup created is always 1, this is not reasonable. Consider this case:
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