ClusterThrottles support PodGroup aka Gang #12
Conversation
Signed-off-by: utam0k <[email protected]>
utam0k
force-pushed
the
group-pods-cluster
branch
3 times, most recently
from
f3f5a5f
to
03a069e
Compare
Signed-off-by: utam0k <[email protected]>
utam0k
force-pushed
the
group-pods-cluster
branch
from
03a069e
to
a5e2678
Compare
| ns, err := c.namespaceInformer.Lister().Get(groupPod.Namespace) | ||
| if err != nil { | ||
| return nil, nil, nil, nil, nil, err | ||
| } |
There was a problem hiding this comment.
All pods in a group have the same namespace, so these lines can be moved out of the for-loop
There was a problem hiding this comment.
Good point. I don't assume the premise that all pods in a group are in the same namespace. This is because this assumption doesn't allow us to improve its implementation and performance that much better than the current implementation in this PR.
There was a problem hiding this comment.
This is because this assumption doesn't allow us to improve its implementation and performance that much better than the current implementation in this PR.
Understood.
But, the current implementation assumes all pods in a group are in the same namespace, IIUC.
We can modify the implementation to fetch the namespace for each pod in a group in the future when we remove this assumption?
There was a problem hiding this comment.
You are right. I had missed it. Doing so would have meant exploring all the namespace pods, which would have had a huge impact on performance. I still think you are right, let's assume they are in the same namespace.
Co-authored-by: Hidehito Yabuuchi <[email protected]> Signed-off-by: utam0k <[email protected]>
| for i := 0; i < 2; i++ { | ||
| podPassedGroup = append(podPassedGroup, MustCreatePod(ctx, MakePod(DefaultNs, fmt.Sprintf("passed-pod%d", i), "100m").Annotation(groupNameAnnotation, "passed").Label(throttleKey, throttleName).Obj())) | ||
| } | ||
| for i := 0; i < 3; i++ { | ||
| podThrottledGroup = append(podThrottledGroup, MustCreatePod(ctx, MakePod(DefaultNs, fmt.Sprintf("throttled-pod%d", i), "100m").Annotation(groupNameAnnotation, "throttled").Label(throttleKey, throttleName).Obj())) | ||
| } |
There was a problem hiding this comment.
I guess we need to wait for all pods in podPassedGroup to be scheduled before creating pods in podThrottledGroup.
Otherwise, pods in podThrottledGroup can be scheduled before podPassedGroup.
| ), | ||
| func(g types.Gomega) { | ||
| for _, pod := range podPassedGroup { | ||
| PodIsScheduled(ctx, DefaultNs, pod.Name) |
There was a problem hiding this comment.
Does this line work?
Returned value from the PodIsScheduled seems not to be used 🤔
Same for MustPodFailedScheduling, PodIsScheduled below.
Scheduling methods like PodGroup and Gang require not only a scheduling pod but also all the resources of a pod group. kube-throttler needs to take this into consideration in PreFilter in order to reduce wasted scheduling cycles and preemptions.
This pull request allows us to reject a scheduling pod that is part of a pod group if the remaining resources are insufficient to schedule the entire PodGroup.