Skip to content

Commit a872159

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#127447 from sanposhiho/bug-topologyspread
fix(topologyspread): register UpdatePodTolerations when QHint is enabled
2 parents 221bf19 + f457777 commit a872159

File tree

2 files changed

+130
-51
lines changed

2 files changed

+130
-51
lines changed

‎pkg/scheduler/framework/plugins/podtopologyspread/plugin.go‎

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,9 @@ package podtopologyspread
1919
import (
2020
"context"
2121
"fmt"
22-
"reflect"
2322

2423
v1 "k8s.io/api/core/v1"
24+
"k8s.io/apimachinery/pkg/api/equality"
2525
"k8s.io/apimachinery/pkg/labels"
2626
"k8s.io/apimachinery/pkg/runtime"
2727
"k8s.io/client-go/informers"
@@ -69,6 +69,7 @@ type PodTopologySpread struct {
6969
statefulSets appslisters.StatefulSetLister
7070
enableNodeInclusionPolicyInPodTopologySpread bool
7171
enableMatchLabelKeysInPodTopologySpread bool
72+
enableSchedulingQueueHint bool
7273
}
7374

7475
var _ framework.PreFilterPlugin = &PodTopologySpread{}
@@ -103,6 +104,7 @@ func New(_ context.Context, plArgs runtime.Object, h framework.Handle, fts featu
103104
defaultConstraints: args.DefaultConstraints,
104105
enableNodeInclusionPolicyInPodTopologySpread: fts.EnableNodeInclusionPolicyInPodTopologySpread,
105106
enableMatchLabelKeysInPodTopologySpread: fts.EnableMatchLabelKeysInPodTopologySpread,
107+
enableSchedulingQueueHint: fts.EnableSchedulingQueueHint,
106108
}
107109
if args.DefaultingType == config.SystemDefaulting {
108110
pl.defaultConstraints = systemDefaultConstraints
@@ -135,6 +137,19 @@ func (pl *PodTopologySpread) setListers(factory informers.SharedInformerFactory)
135137
// EventsToRegister returns the possible events that may make a Pod
136138
// failed by this plugin schedulable.
137139
func (pl *PodTopologySpread) EventsToRegister(_ context.Context) ([]framework.ClusterEventWithHint, error) {
140+
podActionType := framework.Add | framework.UpdatePodLabel | framework.Delete
141+
if pl.enableSchedulingQueueHint {
142+
// When the QueueingHint feature is enabled, the scheduling queue uses Pod/Update Queueing Hint
143+
// to determine whether a Pod's update makes the Pod schedulable or not.
144+
// https://github.com/kubernetes/kubernetes/pull/122234
145+
// (If not, the scheduling queue always retries the unschedulable Pods when they're updated.)
146+
//
147+
// The Pod rejected by this plugin can be schedulable when the Pod has a spread constraint with NodeTaintsPolicy:Honor
148+
// and has got a new toleration.
149+
// So, we add UpdatePodTolerations here only when QHint is enabled.
150+
podActionType = framework.Add | framework.UpdatePodLabel | framework.UpdatePodTolerations | framework.Delete
151+
}
152+
138153
return []framework.ClusterEventWithHint{
139154
// All ActionType includes the following events:
140155
// - Add. An unschedulable Pod may fail due to violating topology spread constraints,
@@ -143,15 +158,27 @@ func (pl *PodTopologySpread) EventsToRegister(_ context.Context) ([]framework.Cl
143158
// an unschedulable Pod schedulable.
144159
// - Delete. An unschedulable Pod may fail due to violating an existing Pod's topology spread constraints,
145160
// deleting an existing Pod may make it schedulable.
146-
{Event: framework.ClusterEvent{Resource: framework.Pod, ActionType: framework.Add | framework.UpdatePodLabel | framework.Delete}, QueueingHintFn: pl.isSchedulableAfterPodChange},
161+
{Event: framework.ClusterEvent{Resource: framework.Pod, ActionType: podActionType}, QueueingHintFn: pl.isSchedulableAfterPodChange},
147162
// Node add|delete|update maybe lead an topology key changed,
148163
// and make these pod in scheduling schedulable or unschedulable.
149164
{Event: framework.ClusterEvent{Resource: framework.Node, ActionType: framework.Add | framework.Delete | framework.UpdateNodeLabel | framework.UpdateNodeTaint}, QueueingHintFn: pl.isSchedulableAfterNodeChange},
150165
}, nil
151166
}
152167

168+
// involvedInTopologySpreading returns true if the incomingPod is involved in the topology spreading of podWithSpreading.
153169
func involvedInTopologySpreading(incomingPod, podWithSpreading *v1.Pod) bool {
154-
return incomingPod.Spec.NodeName != "" && incomingPod.Namespace == podWithSpreading.Namespace
170+
return incomingPod.UID == podWithSpreading.UID ||
171+
(incomingPod.Spec.NodeName != "" && incomingPod.Namespace == podWithSpreading.Namespace)
172+
}
173+
174+
// hasConstraintWithNodeTaintsPolicyHonor returns true if any constraint has `NodeTaintsPolicy: Honor`.
175+
func hasConstraintWithNodeTaintsPolicyHonor(constraints []topologySpreadConstraint) bool {
176+
for _, c := range constraints {
177+
if c.NodeTaintsPolicy == v1.NodeInclusionPolicyHonor {
178+
return true
179+
}
180+
}
181+
return false
155182
}
156183

157184
func (pl *PodTopologySpread) isSchedulableAfterPodChange(logger klog.Logger, pod *v1.Pod, oldObj, newObj interface{}) (framework.QueueingHint, error) {
@@ -173,8 +200,15 @@ func (pl *PodTopologySpread) isSchedulableAfterPodChange(logger klog.Logger, pod
173200

174201
// Pod is modified. Return Queue when the label(s) matching topologySpread's selector is added, changed, or deleted.
175202
if modifiedPod != nil && originalPod != nil {
176-
if reflect.DeepEqual(modifiedPod.Labels, originalPod.Labels) {
177-
logger.V(5).Info("the updated pod is unscheduled or has no updated labels or has different namespace with target pod, so it doesn't make the target pod schedulable",
203+
if pod.UID == modifiedPod.UID && !equality.Semantic.DeepEqual(modifiedPod.Spec.Tolerations, originalPod.Spec.Tolerations) && hasConstraintWithNodeTaintsPolicyHonor(constraints) {
204+
// If any constraint has `NodeTaintsPolicy: Honor`, we can return Queue when the target Pod has got a new toleration.
205+
logger.V(5).Info("the unschedulable pod has got a new toleration, which could make it schedulable",
206+
"pod", klog.KObj(pod), "modifiedPod", klog.KObj(modifiedPod))
207+
return framework.Queue, nil
208+
}
209+
210+
if equality.Semantic.DeepEqual(modifiedPod.Labels, originalPod.Labels) {
211+
logger.V(5).Info("the pod's update doesn't include the label update, which doesn't make the target pod schedulable",
178212
"pod", klog.KObj(pod), "modifiedPod", klog.KObj(modifiedPod))
179213
return framework.QueueSkip, nil
180214
}

‎pkg/scheduler/framework/plugins/podtopologyspread/plugin_test.go‎

Lines changed: 91 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"k8s.io/kubernetes/pkg/scheduler/framework"
2828
plugintesting "k8s.io/kubernetes/pkg/scheduler/framework/plugins/testing"
2929
st "k8s.io/kubernetes/pkg/scheduler/testing"
30+
"k8s.io/utils/ptr"
3031
)
3132

3233
func Test_isSchedulableAfterNodeChange(t *testing.T) {
@@ -150,150 +151,192 @@ func Test_isSchedulableAfterNodeChange(t *testing.T) {
150151

151152
func Test_isSchedulableAfterPodChange(t *testing.T) {
152153
testcases := []struct {
153-
name string
154-
pod *v1.Pod
155-
oldPod, newPod *v1.Pod
156-
expectedHint framework.QueueingHint
157-
expectedErr bool
154+
name string
155+
pod *v1.Pod
156+
oldPod, newPod *v1.Pod
157+
expectedHint framework.QueueingHint
158+
expectedErr bool
159+
enableNodeInclusionPolicyInPodTopologySpread bool
158160
}{
159161
{
160162
name: "add pod with labels match topologySpreadConstraints selector",
161-
pod: st.MakePod().Name("p").Label("foo", "").
163+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
162164
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
163165
Obj(),
164-
newPod: st.MakePod().Node("fake-node").Label("foo", "").Obj(),
166+
newPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Obj(),
165167
expectedHint: framework.Queue,
166168
},
167169
{
168170
name: "add un-scheduled pod",
169-
pod: st.MakePod().Name("p").Label("foo", "").
171+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
170172
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
171173
Obj(),
172-
newPod: st.MakePod().Label("foo", "").Obj(),
174+
newPod: st.MakePod().UID("p2").Label("foo", "").Obj(),
173175
expectedHint: framework.QueueSkip,
174176
},
175177
{
176178
name: "update un-scheduled pod",
177-
pod: st.MakePod().Name("p").Label("foo", "").
179+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
178180
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
179181
Obj(),
180-
newPod: st.MakePod().Label("foo", "").Obj(),
181-
oldPod: st.MakePod().Label("bar", "").Obj(),
182+
newPod: st.MakePod().UID("p2").Label("foo", "").Obj(),
183+
oldPod: st.MakePod().UID("p2").Label("bar", "").Obj(),
182184
expectedHint: framework.QueueSkip,
183185
},
184186
{
185187
name: "delete un-scheduled pod",
186-
pod: st.MakePod().Name("p").Label("foo", "").
188+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
187189
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
188190
Obj(),
189-
oldPod: st.MakePod().Label("foo", "").Obj(),
191+
oldPod: st.MakePod().UID("p2").Label("foo", "").Obj(),
190192
expectedHint: framework.QueueSkip,
191193
},
192194
{
193195
name: "add pod with different namespace",
194-
pod: st.MakePod().Name("p").Label("foo", "").
196+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
195197
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
196198
Obj(),
197-
newPod: st.MakePod().Node("fake-node").Namespace("fake-namespace").Label("foo", "").Obj(),
199+
newPod: st.MakePod().UID("p2").Node("fake-node").Namespace("fake-namespace").Label("foo", "").Obj(),
198200
expectedHint: framework.QueueSkip,
199201
},
200202
{
201203
name: "add pod with labels don't match topologySpreadConstraints selector",
202-
pod: st.MakePod().Name("p").Label("foo", "").
204+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
203205
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
204206
Obj(),
205-
newPod: st.MakePod().Node("fake-node").Label("bar", "").Obj(),
207+
newPod: st.MakePod().UID("p2").Node("fake-node").Label("bar", "").Obj(),
206208
expectedHint: framework.QueueSkip,
207209
},
208210
{
209211
name: "delete pod with labels that match topologySpreadConstraints selector",
210-
pod: st.MakePod().Name("p").Label("foo", "").
212+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
211213
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
212214
Obj(),
213-
oldPod: st.MakePod().Node("fake-node").Label("foo", "").Obj(),
215+
oldPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Obj(),
214216
expectedHint: framework.Queue,
215217
},
216218
{
217219
name: "delete pod with labels that don't match topologySpreadConstraints selector",
218-
pod: st.MakePod().Name("p").Label("foo", "").
220+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
219221
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
220222
Obj(),
221-
oldPod: st.MakePod().Node("fake-node").Label("bar", "").Obj(),
223+
oldPod: st.MakePod().UID("p2").Node("fake-node").Label("bar", "").Obj(),
222224
expectedHint: framework.QueueSkip,
223225
},
224226
{
225227
name: "update pod's non-related label",
226-
pod: st.MakePod().Name("p").Label("foo", "").
228+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
227229
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
228230
Obj(),
229-
oldPod: st.MakePod().Node("fake-node").Label("foo", "").Label("bar", "bar1").Obj(),
230-
newPod: st.MakePod().Node("fake-node").Label("foo", "").Label("bar", "bar2").Obj(),
231+
oldPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Label("bar", "bar1").Obj(),
232+
newPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Label("bar", "bar2").Obj(),
231233
expectedHint: framework.QueueSkip,
232234
},
233235
{
234236
name: "add pod's label that matches topologySpreadConstraints selector",
235-
pod: st.MakePod().Name("p").Label("foo", "").
237+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
236238
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
237239
Obj(),
238-
oldPod: st.MakePod().Node("fake-node").Obj(),
239-
newPod: st.MakePod().Node("fake-node").Label("foo", "").Obj(),
240+
oldPod: st.MakePod().UID("p2").Node("fake-node").Obj(),
241+
newPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Obj(),
240242
expectedHint: framework.Queue,
241243
},
242244
{
243245
name: "delete pod label that matches topologySpreadConstraints selector",
244-
pod: st.MakePod().Name("p").Label("foo", "").
246+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
245247
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
246248
Obj(),
247-
oldPod: st.MakePod().Node("fake-node").Label("foo", "").Obj(),
248-
newPod: st.MakePod().Node("fake-node").Obj(),
249+
oldPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Obj(),
250+
newPod: st.MakePod().UID("p2").Node("fake-node").Obj(),
249251
expectedHint: framework.Queue,
250252
},
251253
{
252254
name: "change pod's label that matches topologySpreadConstraints selector",
253-
pod: st.MakePod().Name("p").Label("foo", "").
255+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
254256
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
255257
Obj(),
256-
oldPod: st.MakePod().Node("fake-node").Label("foo", "foo1").Obj(),
257-
newPod: st.MakePod().Node("fake-node").Label("foo", "foo2").Obj(),
258+
oldPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "foo1").Obj(),
259+
newPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "foo2").Obj(),
258260
expectedHint: framework.QueueSkip,
259261
},
260262
{
261263
name: "change pod's label that doesn't match topologySpreadConstraints selector",
262-
pod: st.MakePod().Name("p").Label("foo", "").
264+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
263265
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
264266
Obj(),
265-
oldPod: st.MakePod().Node("fake-node").Label("foo", "").Label("bar", "bar1").Obj(),
266-
newPod: st.MakePod().Node("fake-node").Label("foo", "").Label("bar", "bar2").Obj(),
267+
oldPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Label("bar", "bar1").Obj(),
268+
newPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Label("bar", "bar2").Obj(),
267269
expectedHint: framework.QueueSkip,
268270
},
269271
{
270272
name: "add pod's label that matches topologySpreadConstraints selector with multi topologySpreadConstraints",
271-
pod: st.MakePod().Name("p").Label("foo", "").
273+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
272274
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
273275
SpreadConstraint(1, "node", v1.DoNotSchedule, barSelector, nil, nil, nil, nil).
274276
Obj(),
275-
oldPod: st.MakePod().Node("fake-node").Label("foo", "").Obj(),
276-
newPod: st.MakePod().Node("fake-node").Label("foo", "").Label("bar", "bar2").Obj(),
277+
oldPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Obj(),
278+
newPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Label("bar", "bar2").Obj(),
277279
expectedHint: framework.Queue,
278280
},
279281
{
280282
name: "change pod's label that doesn't match topologySpreadConstraints selector with multi topologySpreadConstraints",
281-
pod: st.MakePod().Name("p").Label("foo", "").
283+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
282284
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
283285
SpreadConstraint(1, "node", v1.DoNotSchedule, barSelector, nil, nil, nil, nil).
284286
Obj(),
285-
oldPod: st.MakePod().Node("fake-node").Label("foo", "").Obj(),
286-
newPod: st.MakePod().Node("fake-node").Label("foo", "").Label("baz", "").Obj(),
287+
oldPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Obj(),
288+
newPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Label("baz", "").Obj(),
287289
expectedHint: framework.QueueSkip,
288290
},
289291
{
290292
name: "change pod's label that match topologySpreadConstraints selector with multi topologySpreadConstraints",
291-
pod: st.MakePod().Name("p").Label("foo", "").
293+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
292294
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, nil, nil).
293295
SpreadConstraint(1, "node", v1.DoNotSchedule, barSelector, nil, nil, nil, nil).
294296
Obj(),
295-
oldPod: st.MakePod().Node("fake-node").Label("foo", "").Label("bar", "").Obj(),
296-
newPod: st.MakePod().Node("fake-node").Label("foo", "").Label("bar", "bar2").Obj(),
297+
oldPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Label("bar", "").Obj(),
298+
newPod: st.MakePod().UID("p2").Node("fake-node").Label("foo", "").Label("bar", "bar2").Obj(),
299+
expectedHint: framework.QueueSkip,
300+
},
301+
{
302+
name: "the unschedulable Pod has topologySpreadConstraint with NodeTaintsPolicy:Honor and has got a new toleration",
303+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
304+
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, ptr.To(v1.NodeInclusionPolicyHonor), nil).
305+
SpreadConstraint(1, "node", v1.DoNotSchedule, barSelector, nil, nil, nil, nil).
306+
Obj(),
307+
oldPod: st.MakePod().UID("p").Name("p").Label("foo", "").Obj(),
308+
newPod: st.MakePod().UID("p").Name("p").Label("foo", "").Toleration(v1.TaintNodeUnschedulable).Obj(),
309+
expectedHint: framework.Queue,
310+
enableNodeInclusionPolicyInPodTopologySpread: true,
311+
},
312+
{
313+
name: "the unschedulable Pod has topologySpreadConstraint without NodeTaintsPolicy:Honor and has got a new toleration",
314+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
315+
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, ptr.To(v1.NodeInclusionPolicyIgnore), nil).
316+
SpreadConstraint(1, "node", v1.DoNotSchedule, barSelector, nil, nil, nil, nil).
317+
Obj(),
318+
oldPod: st.MakePod().UID("p").Name("p").Label("foo", "").Obj(),
319+
newPod: st.MakePod().UID("p").Name("p").Label("foo", "").Toleration(v1.TaintNodeUnschedulable).Obj(),
320+
expectedHint: framework.QueueSkip,
321+
},
322+
{
323+
name: "the unschedulable Pod has topologySpreadConstraint and has got a new label matching the selector of the constraint",
324+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
325+
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, ptr.To(v1.NodeInclusionPolicyIgnore), nil).
326+
SpreadConstraint(1, "node", v1.DoNotSchedule, barSelector, nil, nil, nil, nil).
327+
Obj(),
328+
oldPod: st.MakePod().UID("p").Name("p").Obj(),
329+
newPod: st.MakePod().UID("p").Name("p").Label("foo", "").Obj(),
330+
expectedHint: framework.Queue,
331+
},
332+
{
333+
name: "the unschedulable Pod has topologySpreadConstraint and has got a new unrelated label",
334+
pod: st.MakePod().UID("p").Name("p").Label("foo", "").
335+
SpreadConstraint(1, "zone", v1.DoNotSchedule, fooSelector, nil, nil, ptr.To(v1.NodeInclusionPolicyIgnore), nil).
336+
SpreadConstraint(1, "node", v1.DoNotSchedule, barSelector, nil, nil, nil, nil).
337+
Obj(),
338+
oldPod: st.MakePod().UID("p").Name("p").Obj(),
339+
newPod: st.MakePod().UID("p").Name("p").Label("unrelated", "").Obj(),
297340
expectedHint: framework.QueueSkip,
298341
},
299342
}
@@ -303,6 +346,8 @@ func Test_isSchedulableAfterPodChange(t *testing.T) {
303346
snapshot := cache.NewSnapshot(nil, nil)
304347
pl := plugintesting.SetupPlugin(ctx, t, topologySpreadFunc, &config.PodTopologySpreadArgs{DefaultingType: config.ListDefaulting}, snapshot)
305348
p := pl.(*PodTopologySpread)
349+
p.enableNodeInclusionPolicyInPodTopologySpread = tc.enableNodeInclusionPolicyInPodTopologySpread
350+
306351
actualHint, err := p.isSchedulableAfterPodChange(logger, tc.pod, tc.oldPod, tc.newPod)
307352
if tc.expectedErr {
308353
require.Error(t, err)

0 commit comments

Comments
 (0)