Skip to content

Commit

Permalink
fix incorrect pod equivalency check for pod specs with volume
Browse files Browse the repository at this point in the history
  • Loading branch information
marwanad committed Nov 9, 2021
1 parent 821d677 commit 0e66339
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 4 deletions.
4 changes: 2 additions & 2 deletions cluster-autoscaler/core/equivalence_groups.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ limitations under the License.
package core

import (
"k8s.io/autoscaler/cluster-autoscaler/core/utils"
"reflect"

apiv1 "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/types"
"k8s.io/autoscaler/cluster-autoscaler/processors/status"
"k8s.io/autoscaler/cluster-autoscaler/utils/drain"
Expand Down Expand Up @@ -67,7 +67,7 @@ func groupPodsBySchedulingProperties(pods []*apiv1.Pod) map[equivalenceGroupId][

matchingFound := false
for _, g := range equivalenceGroupsByController[controllerRef.UID] {
if reflect.DeepEqual(pod.Labels, g.representant.Labels) && apiequality.Semantic.DeepEqual(pod.Spec, g.representant.Spec) {
if reflect.DeepEqual(pod.Labels, g.representant.Labels) && utils.PodSpecSemanticallyEqual(pod.Spec, g.representant.Spec) {
matchingFound = true
podEquivalenceGroups[g.id] = append(podEquivalenceGroups[g.id], pod)
break
Expand Down
21 changes: 19 additions & 2 deletions cluster-autoscaler/core/equivalence_groups_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ func TestGroupSchedulablePodsForNode(t *testing.T) {
},
}

rc3 := apiv1.ReplicationController{
ObjectMeta: metav1.ObjectMeta{
Name: "rc3",
Namespace: "default",
SelfLink: "api/v1/namespaces/default/replicationcontrollers/rc3",
UID: "12345678-1234-1234-1234-12345678901e",
},
}

projectedSAVol := BuildServiceTokenProjectedVolumeSource("path")
p1 := BuildTestPod("p1", 1500, 200000)
p2_1 := BuildTestPod("p2_1", 3000, 200000)
p2_1.OwnerReferences = GenerateOwnerReferences(rc1.Name, "ReplicationController", "extensions/v1beta1", rc1.UID)
Expand All @@ -56,10 +66,16 @@ func TestGroupSchedulablePodsForNode(t *testing.T) {
p3_1.OwnerReferences = GenerateOwnerReferences(rc2.Name, "ReplicationController", "extensions/v1beta1", rc2.UID)
p3_2 := BuildTestPod("p3_2", 100, 200000)
p3_2.OwnerReferences = GenerateOwnerReferences(rc2.Name, "ReplicationController", "extensions/v1beta1", rc2.UID)
unschedulablePods := []*apiv1.Pod{p1, p2_1, p2_2, p3_1, p3_2}
p4_1 := BuildTestPod("p4_1", 100, 200000)
p4_1.OwnerReferences = GenerateOwnerReferences(rc2.Name, "ReplicationController", "extensions/v1beta1", rc3.UID)
p4_1.Spec.Volumes = []apiv1.Volume{{Name: "kube-api-access-nz94b", VolumeSource: apiv1.VolumeSource{Projected: projectedSAVol}}}
p4_2 := BuildTestPod("p4_2", 100, 200000)
p4_2.OwnerReferences = GenerateOwnerReferences(rc2.Name, "ReplicationController", "extensions/v1beta1", rc3.UID)
p4_2.Spec.Volumes = []apiv1.Volume{{Name: "kube-api-access-mo25i", VolumeSource: apiv1.VolumeSource{Projected: projectedSAVol}}}
unschedulablePods := []*apiv1.Pod{p1, p2_1, p2_2, p3_1, p3_2, p4_1, p4_2}

podGroups := groupPodsBySchedulingProperties(unschedulablePods)
assert.Equal(t, 3, len(podGroups))
assert.Equal(t, 4, len(podGroups))

wantedGroups := []struct {
pods []*apiv1.Pod
Expand All @@ -68,6 +84,7 @@ func TestGroupSchedulablePodsForNode(t *testing.T) {
{pods: []*apiv1.Pod{p1}},
{pods: []*apiv1.Pod{p2_1, p2_2}},
{pods: []*apiv1.Pod{p3_1, p3_2}},
{pods: []*apiv1.Pod{p4_1, p4_2}},
}

equal := func(a, b []*apiv1.Pod) bool {
Expand Down
14 changes: 14 additions & 0 deletions cluster-autoscaler/utils/test/test_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,20 @@ func BuildTestPod(name string, cpu int64, mem int64) *apiv1.Pod {
return pod
}

// BuildServiceTokenProjectedVolumeSource returns a ProjectedVolumeSource with SA token
// projection
func BuildServiceTokenProjectedVolumeSource(path string) *apiv1.ProjectedVolumeSource {
return &apiv1.ProjectedVolumeSource{
Sources: []apiv1.VolumeProjection{
{
ServiceAccountToken: &apiv1.ServiceAccountTokenProjection{
Path: path,
},
},
},
}
}

const (
// cannot use constants from gpu module due to cyclic package import
resourceNvidiaGPU = "nvidia.com/gpu"
Expand Down
36 changes: 36 additions & 0 deletions cluster-autoscaler/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package utils

import (
apiv1 "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
klog "k8s.io/klog/v2"

"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
Expand Down Expand Up @@ -54,3 +55,38 @@ func FilterOutNodes(nodes []*apiv1.Node, nodesToFilterOut []*apiv1.Node) []*apiv

return filtered
}

// PodSpecSemanticallyEqual returns if two pod specs are similar after dropping
// the fields we don't care about
func PodSpecSemanticallyEqual(p1 apiv1.PodSpec, p2 apiv1.PodSpec) bool {
p1Spec := sanitizedProjectedVolumesAndMounts(p1)
p2Spec := sanitizedProjectedVolumesAndMounts(p2)
return apiequality.Semantic.DeepEqual(p1Spec, p2Spec)
}

// sanitizedProjectedVolumesAndMounts returns a pod spec with projected volumes
// and their mounts removed
// Due to the generated suffixes, a strict DeepEquals check will fail and generate
// an equivalence group per pod which is undesirable.
// Projected volumes do not impact scheduling so we should ignore them
func sanitizedProjectedVolumesAndMounts(podSpec apiv1.PodSpec) apiv1.PodSpec {
var projectedVolumeNames []string
for i, v := range podSpec.Volumes {
if v.Projected != nil {
projectedVolumeNames = append(projectedVolumeNames, v.Name)
podSpec.Volumes[i] = apiv1.Volume{}
}
}

for _, pv := range projectedVolumeNames {
for c := range podSpec.Containers {
for v, mount := range podSpec.Containers[c].VolumeMounts {
if mount.Name == pv {
podSpec.Containers[c].VolumeMounts[v] = apiv1.VolumeMount{}
}
}
}
}

return podSpec
}

0 comments on commit 0e66339

Please sign in to comment.