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

Avoid unnecessary requeue operations in coscheduling #700

Merged
merged 2 commits into from
Apr 15, 2024
Merged

Avoid unnecessary requeue operations in coscheduling #700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d16270f
Avoid unnecessary requeue operations in coscheduling
Huang-Wei Dec 10, 2023
e35ce33
fixup: rename preFilterState to permitState
Huang-Wei Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions pkg/coscheduling/core/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,22 @@ const (
PodGroupNotFound Status = "PodGroup not found"
Success Status = "Success"
Wait Status = "Wait"

permitStateKey = "PermitCoscheduling"
)

type PermitState struct {
Activate bool
}

func (s *PermitState) Clone() framework.StateData {
return &PermitState{Activate: s.Activate}
}

// Manager defines the interfaces for PodGroup management.
type Manager interface {
PreFilter(context.Context, *corev1.Pod) error
Permit(context.Context, *corev1.Pod) Status
Permit(context.Context, *framework.CycleState, *corev1.Pod) Status
GetPodGroup(context.Context, *corev1.Pod) (string, *v1alpha1.PodGroup)
GetCreationTimestamp(*corev1.Pod, time.Time) time.Time
DeletePermittedPodGroup(string)
Expand Down Expand Up @@ -108,6 +118,13 @@ func (pgMgr *PodGroupManager) ActivateSiblings(pod *corev1.Pod, state *framework
return
}

// Only proceed if it's explicitly requested to activate sibling pods.
if c, err := state.Read(permitStateKey); err != nil {
return
} else if s, ok := c.(*PermitState); !ok || !s.Activate {
return
}

pods, err := pgMgr.podLister.Pods(pod.Namespace).List(
labels.SelectorFromSet(labels.Set{v1alpha1.PodGroupLabel: pgName}),
)
Expand Down Expand Up @@ -193,7 +210,7 @@ func (pgMgr *PodGroupManager) PreFilter(ctx context.Context, pod *corev1.Pod) er
}

// Permit permits a pod to run, if the minMember match, it would send a signal to chan.
func (pgMgr *PodGroupManager) Permit(ctx context.Context, pod *corev1.Pod) Status {
func (pgMgr *PodGroupManager) Permit(ctx context.Context, state *framework.CycleState, pod *corev1.Pod) Status {
pgFullName, pg := pgMgr.GetPodGroup(ctx, pod)
if pgFullName == "" {
return PodGroupNotSpecified
Expand All @@ -209,6 +226,19 @@ func (pgMgr *PodGroupManager) Permit(ctx context.Context, pod *corev1.Pod) Statu
if int32(assigned)+1 >= pg.Spec.MinMember {
return Success
}

if assigned == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my understanding, when assigned == 0, we should and only should trigger the activating, then in ActivateSiblings, once Activate is Ture, we should start the trigger but now we're the opposite. And what's more, shouldn't we set the Activate to False in the follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when assigned == 0, we should and only should trigger the activating, then in ActivateSiblings, once Activate is Ture, we should start the trigger but now we're the opposite.

That's the current case. Why do you think it's the opposite.

shouldn't we set the Activate to False in the follow up.

Nope b/c every scheduling cycle comes with its own CycleState instance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense.
/lgtm
/hold cancel

// Given we've reached Permit(), it's mean all PreFilter checks (minMember & minResource)
// already pass through, so if assigned == 0, it could be due to:
// - minResource get satisfied
// - new pods added
// In either case, we should and only should use this 0-th pod to trigger activating
// its siblings.
// It'd be in-efficient if we trigger activating siblings unconditionally.
// See https://github.com/kubernetes-sigs/scheduler-plugins/issues/682
state.Write(permitStateKey, &PermitState{Activate: true})
}

return Wait
}

Expand Down
3 changes: 2 additions & 1 deletion pkg/coscheduling/core/core_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"k8s.io/client-go/informers"
clientsetfake "k8s.io/client-go/kubernetes/fake"
clicache "k8s.io/client-go/tools/cache"
"k8s.io/kubernetes/pkg/scheduler/framework"
st "k8s.io/kubernetes/pkg/scheduler/testing"
"sigs.k8s.io/scheduler-plugins/apis/scheduling/v1alpha1"
tu "sigs.k8s.io/scheduler-plugins/test/util"
Expand Down Expand Up @@ -278,7 +279,7 @@ func TestPermit(t *testing.T) {
podInformer.Informer().GetStore().Add(p)
}

if got := pgMgr.Permit(ctx, tt.pod); got != tt.want {
if got := pgMgr.Permit(ctx, &framework.CycleState{}, tt.pod); got != tt.want {
t.Errorf("Want %v, but got %v", tt.want, got)
}
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/coscheduling/coscheduling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func (cs *Coscheduling) PreFilterExtensions() framework.PreFilterExtensions {
// Permit is the functions invoked by the framework at "Permit" extension point.
func (cs *Coscheduling) Permit(ctx context.Context, state *framework.CycleState, pod *v1.Pod, nodeName string) (*framework.Status, time.Duration) {
waitTime := *cs.scheduleTimeout
s := cs.pgMgr.Permit(ctx, pod)
s := cs.pgMgr.Permit(ctx, state, pod)
var retStatus *framework.Status
switch s {
case core.PodGroupNotSpecified:
Expand Down