diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/filtering.go b/pkg/scheduler/framework/plugins/interpodaffinity/filtering.go index 1a1bf641b93..596f1b95df3 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/filtering.go +++ b/pkg/scheduler/framework/plugins/interpodaffinity/filtering.go @@ -251,6 +251,9 @@ func (pl *InterPodAffinity) PreFilter(ctx context.Context, cycleState *framework } podInfo := framework.NewPodInfo(pod) + if podInfo.ParseError != nil { + return framework.NewStatus(framework.UnschedulableAndUnresolvable, fmt.Sprintf("parsing pod: %+v", podInfo.ParseError)) + } // existingPodAntiAffinityMap will be used later for efficient check on existing pods' anti-affinity existingPodAntiAffinityMap := getTPMapMatchingExistingAntiAffinity(pod, havePodsWithAffinityNodes) diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go b/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go index c4879d39b7c..99ecd7a84e9 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go +++ b/pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go @@ -19,6 +19,7 @@ package interpodaffinity import ( "context" "reflect" + "strings" "testing" "k8s.io/api/core/v1" @@ -772,6 +773,80 @@ func TestRequiredAffinitySingleNode(t *testing.T) { ), name: "PodAntiAffinity symmetry check b2: incoming pod and existing pod partially match each other on AffinityTerms", }, + { + name: "PodAffinity fails PreFilter with an invalid affinity label syntax", + pod: createPodWithAffinityTerms(defaultNamespace, "", podLabel, + []v1.PodAffinityTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "service", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"{{.bad-value.}}"}, + }, + }, + }, + TopologyKey: "region", + }, + }, + []v1.PodAffinityTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "service", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"antivirusscan", "value2"}, + }, + }, + }, + TopologyKey: "node", + }, + }), + node: &node1, + wantStatus: framework.NewStatus( + framework.UnschedulableAndUnresolvable, + "invalid label value", + ), + }, + { + name: "PodAntiAffinity fails PreFilter with an invalid antiaffinity label syntax", + pod: createPodWithAffinityTerms(defaultNamespace, "", podLabel, + []v1.PodAffinityTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "service", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"foo"}, + }, + }, + }, + TopologyKey: "region", + }, + }, + []v1.PodAffinityTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "service", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"{{.bad-value.}}"}, + }, + }, + }, + TopologyKey: "node", + }, + }), + node: &node1, + wantStatus: framework.NewStatus( + framework.UnschedulableAndUnresolvable, + "invalid label value", + ), + }, } for _, test := range tests { @@ -783,12 +858,15 @@ func TestRequiredAffinitySingleNode(t *testing.T) { state := framework.NewCycleState() preFilterStatus := p.PreFilter(context.Background(), state, test.pod) if !preFilterStatus.IsSuccess() { - t.Errorf("prefilter failed with status: %v", preFilterStatus) - } - nodeInfo := mustGetNodeInfo(t, snapshot, test.node.Name) - gotStatus := p.Filter(context.Background(), state, test.pod, nodeInfo) - if !reflect.DeepEqual(gotStatus, test.wantStatus) { - t.Errorf("status does not match: %v, want: %v", gotStatus, test.wantStatus) + if !strings.Contains(preFilterStatus.Message(), test.wantStatus.Message()) { + t.Errorf("prefilter failed with status: %v", preFilterStatus) + } + } else { + nodeInfo := mustGetNodeInfo(t, snapshot, test.node.Name) + gotStatus := p.Filter(context.Background(), state, test.pod, nodeInfo) + if !reflect.DeepEqual(gotStatus, test.wantStatus) { + t.Errorf("status does not match: %v, want: %v", gotStatus, test.wantStatus) + } } }) } diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/scoring.go b/pkg/scheduler/framework/plugins/interpodaffinity/scoring.go index 5848121dee6..c27ae5f41ef 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/scoring.go +++ b/pkg/scheduler/framework/plugins/interpodaffinity/scoring.go @@ -160,9 +160,15 @@ func (pl *InterPodAffinity) PreScore( } } + podInfo := framework.NewPodInfo(pod) + if podInfo.ParseError != nil { + // Ideally we never reach here, because errors will be caught by PreFilter + return framework.NewStatus(framework.Error, fmt.Sprintf("parsing pod: %+v", podInfo.ParseError)) + } + state := &preScoreState{ topologyScore: make(map[string]map[string]int64), - podInfo: framework.NewPodInfo(pod), + podInfo: podInfo, } topoScores := make([]scoreMap, len(allNodes)) diff --git a/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go b/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go index 766e4fc35f1..0defe8a2f5d 100644 --- a/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go +++ b/pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go @@ -19,6 +19,7 @@ package interpodaffinity import ( "context" "reflect" + "strings" "testing" v1 "k8s.io/api/core/v1" @@ -252,12 +253,56 @@ func TestPreferredAffinity(t *testing.T) { }, } + invalidAffinityLabels := &v1.Affinity{ + PodAffinity: &v1.PodAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []v1.WeightedPodAffinityTerm{ + { + Weight: 8, + PodAffinityTerm: v1.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "security", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"{{.bad-value.}}"}, + }, + }, + }, + TopologyKey: "region", + }, + }, + }, + }, + } + invalidAntiAffinityLabels := &v1.Affinity{ + PodAntiAffinity: &v1.PodAntiAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []v1.WeightedPodAffinityTerm{ + { + Weight: 5, + PodAffinityTerm: v1.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "security", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"{{.bad-value.}}"}, + }, + }, + }, + TopologyKey: "az", + }, + }, + }, + }, + } + tests := []struct { pod *v1.Pod pods []*v1.Pod nodes []*v1.Node expectedList framework.NodeScoreList name string + wantStatus *framework.Status }{ { pod: &v1.Pod{Spec: v1.PodSpec{NodeName: ""}, ObjectMeta: metav1.ObjectMeta{Labels: podLabelSecurityS1}}, @@ -513,6 +558,24 @@ func TestPreferredAffinity(t *testing.T) { expectedList: []framework.NodeScore{{Name: "machine1", Score: framework.MaxNodeScore}, {Name: "machine2", Score: framework.MaxNodeScore}}, name: "Avoid panic when partial nodes in a topology don't have pods with affinity", }, + { + name: "invalid Affinity fails PreScore", + pod: &v1.Pod{Spec: v1.PodSpec{NodeName: "", Affinity: invalidAffinityLabels}}, + wantStatus: framework.NewStatus(framework.Error, "invalid label value"), + nodes: []*v1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "machine1", Labels: labelRgChina}}, + {ObjectMeta: metav1.ObjectMeta{Name: "machine2", Labels: labelRgChina}}, + }, + }, + { + name: "invalid AntiAffinity fails PreScore", + pod: &v1.Pod{Spec: v1.PodSpec{NodeName: "", Affinity: invalidAntiAffinityLabels}}, + wantStatus: framework.NewStatus(framework.Error, "invalid label value"), + nodes: []*v1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "machine1", Labels: labelRgChina}}, + {ObjectMeta: metav1.ObjectMeta{Name: "machine2", Labels: labelRgChina}}, + }, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -527,25 +590,28 @@ func TestPreferredAffinity(t *testing.T) { status := p.PreScore(context.Background(), state, test.pod, test.nodes) if !status.IsSuccess() { - t.Errorf("unexpected error: %v", status) - } - var gotList framework.NodeScoreList - for _, n := range test.nodes { - nodeName := n.ObjectMeta.Name - score, status := p.Score(context.Background(), state, test.pod, nodeName) + if !strings.Contains(status.Message(), test.wantStatus.Message()) { + t.Errorf("unexpected error: %v", status) + } + } else { + var gotList framework.NodeScoreList + for _, n := range test.nodes { + nodeName := n.ObjectMeta.Name + score, status := p.Score(context.Background(), state, test.pod, nodeName) + if !status.IsSuccess() { + t.Errorf("unexpected error: %v", status) + } + gotList = append(gotList, framework.NodeScore{Name: nodeName, Score: score}) + } + + status = p.ScoreExtensions().NormalizeScore(context.Background(), state, test.pod, gotList) if !status.IsSuccess() { t.Errorf("unexpected error: %v", status) } - gotList = append(gotList, framework.NodeScore{Name: nodeName, Score: score}) - } - status = p.ScoreExtensions().NormalizeScore(context.Background(), state, test.pod, gotList) - if !status.IsSuccess() { - t.Errorf("unexpected error: %v", status) - } - - if !reflect.DeepEqual(test.expectedList, gotList) { - t.Errorf("expected:\n\t%+v,\ngot:\n\t%+v", test.expectedList, gotList) + if !reflect.DeepEqual(test.expectedList, gotList) { + t.Errorf("expected:\n\t%+v,\ngot:\n\t%+v", test.expectedList, gotList) + } } }) diff --git a/pkg/scheduler/framework/v1alpha1/types.go b/pkg/scheduler/framework/v1alpha1/types.go index 1e464bf9c97..915c5d1ec25 100644 --- a/pkg/scheduler/framework/v1alpha1/types.go +++ b/pkg/scheduler/framework/v1alpha1/types.go @@ -73,6 +73,7 @@ type PodInfo struct { RequiredAntiAffinityTerms []AffinityTerm PreferredAffinityTerms []WeightedAffinityTerm PreferredAntiAffinityTerms []WeightedAffinityTerm + ParseError error } // AffinityTerm is a processed version of v1.PodAffinityTerm. @@ -88,53 +89,50 @@ type WeightedAffinityTerm struct { Weight int32 } -func newAffinityTerm(pod *v1.Pod, term *v1.PodAffinityTerm) *AffinityTerm { +func newAffinityTerm(pod *v1.Pod, term *v1.PodAffinityTerm) (*AffinityTerm, error) { namespaces := schedutil.GetNamespacesFromPodAffinityTerm(pod, term) selector, err := metav1.LabelSelectorAsSelector(term.LabelSelector) if err != nil { - klog.Errorf("Cannot process label selector: %v", err) - return nil + return nil, err } - return &AffinityTerm{Namespaces: namespaces, Selector: selector, TopologyKey: term.TopologyKey} + return &AffinityTerm{Namespaces: namespaces, Selector: selector, TopologyKey: term.TopologyKey}, nil } // getAffinityTerms receives a Pod and affinity terms and returns the namespaces and // selectors of the terms. -func getAffinityTerms(pod *v1.Pod, v1Terms []v1.PodAffinityTerm) []AffinityTerm { +func getAffinityTerms(pod *v1.Pod, v1Terms []v1.PodAffinityTerm) ([]AffinityTerm, error) { if v1Terms == nil { - return nil + return nil, nil } var terms []AffinityTerm for _, term := range v1Terms { - t := newAffinityTerm(pod, &term) - if t == nil { - // We get here if the label selector failed to process, this is not supposed - // to happen because the pod should have been validated by the api server. - return nil + t, err := newAffinityTerm(pod, &term) + if err != nil { + // We get here if the label selector failed to process + return nil, err } terms = append(terms, *t) } - return terms + return terms, nil } // getWeightedAffinityTerms returns the list of processed affinity terms. -func getWeightedAffinityTerms(pod *v1.Pod, v1Terms []v1.WeightedPodAffinityTerm) []WeightedAffinityTerm { +func getWeightedAffinityTerms(pod *v1.Pod, v1Terms []v1.WeightedPodAffinityTerm) ([]WeightedAffinityTerm, error) { if v1Terms == nil { - return nil + return nil, nil } var terms []WeightedAffinityTerm for _, term := range v1Terms { - t := newAffinityTerm(pod, &term.PodAffinityTerm) - if t == nil { - // We get here if the label selector failed to process, this is not supposed - // to happen because the pod should have been validated by the api server. - return nil + t, err := newAffinityTerm(pod, &term.PodAffinityTerm) + if err != nil { + // We get here if the label selector failed to process + return nil, err } terms = append(terms, WeightedAffinityTerm{AffinityTerm: *t, Weight: term.Weight}) } - return terms + return terms, nil } // NewPodInfo return a new PodInfo @@ -150,12 +148,32 @@ func NewPodInfo(pod *v1.Pod) *PodInfo { } } + // Attempt to parse the affinity terms + var parseErr error + requiredAffinityTerms, err := getAffinityTerms(pod, schedutil.GetPodAffinityTerms(pod.Spec.Affinity)) + if err != nil { + parseErr = fmt.Errorf("requiredAffinityTerms: %w", err) + } + requiredAntiAffinityTerms, err := getAffinityTerms(pod, schedutil.GetPodAntiAffinityTerms(pod.Spec.Affinity)) + if err != nil { + parseErr = fmt.Errorf("requiredAntiAffinityTerms: %w", err) + } + weightedAffinityTerms, err := getWeightedAffinityTerms(pod, preferredAffinityTerms) + if err != nil { + parseErr = fmt.Errorf("preferredAffinityTerms: %w", err) + } + weightedAntiAffinityTerms, err := getWeightedAffinityTerms(pod, preferredAntiAffinityTerms) + if err != nil { + parseErr = fmt.Errorf("preferredAntiAffinityTerms: %w", err) + } + return &PodInfo{ Pod: pod, - RequiredAffinityTerms: getAffinityTerms(pod, schedutil.GetPodAffinityTerms(pod.Spec.Affinity)), - RequiredAntiAffinityTerms: getAffinityTerms(pod, schedutil.GetPodAntiAffinityTerms(pod.Spec.Affinity)), - PreferredAffinityTerms: getWeightedAffinityTerms(pod, preferredAffinityTerms), - PreferredAntiAffinityTerms: getWeightedAffinityTerms(pod, preferredAntiAffinityTerms), + RequiredAffinityTerms: requiredAffinityTerms, + RequiredAntiAffinityTerms: requiredAntiAffinityTerms, + PreferredAffinityTerms: weightedAffinityTerms, + PreferredAntiAffinityTerms: weightedAntiAffinityTerms, + ParseError: parseErr, } }