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

Jump to bottom

Tests for empty constraints array when DefaultPodTopologySpread is enabled #94864

Merged
merged 1 commit into from Sep 22, 2020
Merged

Tests for empty constraints array when DefaultPodTopologySpread is enabled #94864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/scheduler/apis/config/scheme/BUILD
View file
Open in desktop
Expand Up @@ -34,10 +34,14 @@ go_test(
srcs = ["scheme_test.go"],
embed = [":go_default_library"],
deps = [
"//pkg/features:go_default_library",
"//pkg/scheduler/apis/config:go_default_library",
"//staging/src/k8s.io/api/core/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//staging/src/k8s.io/component-base/featuregate:go_default_library",
"//staging/src/k8s.io/component-base/featuregate/testing:go_default_library",
"//staging/src/k8s.io/kube-scheduler/config/v1beta1:go_default_library",
"//vendor/github.com/google/go-cmp/cmp:go_default_library",
"//vendor/k8s.io/utils/pointer:go_default_library",
Expand Down
98 changes: 98 additions & 0 deletions pkg/scheduler/apis/config/scheme/scheme_test.go
View file
Open in desktop
Expand Up @@ -24,7 +24,11 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apiserver/pkg/util/feature"
"k8s.io/component-base/featuregate"
featuregatetesting "k8s.io/component-base/featuregate/testing"
"k8s.io/kube-scheduler/config/v1beta1"
"k8s.io/kubernetes/pkg/features"
"k8s.io/kubernetes/pkg/scheduler/apis/config"
"k8s.io/utils/pointer"
"sigs.k8s.io/yaml"
Expand All @@ -34,6 +38,7 @@ func TestCodecsDecodePluginConfig(t *testing.T) {
testCases := []struct {
name string
data []byte
feature featuregate.Feature
wantErr string
wantProfiles []config.KubeSchedulerProfile
}{
Expand Down Expand Up @@ -257,6 +262,7 @@ profiles:
args:
- name: VolumeBinding
args:
- name: PodTopologySpread
`),
wantProfiles: []config.KubeSchedulerProfile{
{
Expand Down Expand Up @@ -291,6 +297,73 @@ profiles:
BindTimeoutSeconds: 600,
},
},
{
Name: "PodTopologySpread",
Args: &config.PodTopologySpreadArgs{},
},
},
},
},
},
{
name: "empty PodTopologySpread, feature DefaultPodTopologySpread enabled",
data: []byte(`
apiVersion: kubescheduler.config.k8s.io/v1beta1
kind: KubeSchedulerConfiguration
profiles:
- pluginConfig:
- name: PodTopologySpread
args:
defaultConstraints:
Comment on lines +316 to +317
Copy link
Member

Choose a reason for hiding this comment

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

This works the same with:

- name: PodTopologySpread

correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, but I wanted to be more explicit

`),
feature: features.DefaultPodTopologySpread,
wantProfiles: []config.KubeSchedulerProfile{
{
SchedulerName: "default-scheduler",
PluginConfig: []config.PluginConfig{
{
Name: "PodTopologySpread",
Args: &config.PodTopologySpreadArgs{
DefaultConstraints: []corev1.TopologySpreadConstraint{
{
MaxSkew: 3,
TopologyKey: corev1.LabelHostname,
WhenUnsatisfiable: corev1.ScheduleAnyway,
},
{
MaxSkew: 5,
TopologyKey: corev1.LabelZoneFailureDomainStable,
WhenUnsatisfiable: corev1.ScheduleAnyway,
},
Comment on lines +328 to +337
Copy link
Member

Choose a reason for hiding this comment

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

nit: reuse the default value defined in pkg/scheduler/apis/config/v1beta1/defaults.go?

Copy link
Member Author

Choose a reason for hiding this comment

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

for the whole array? It's not exported, and I think it's best to keep it like that.

},
},
},
},
},
},
},
{
name: "empty array PodTopologySpread, feature DefaultPodTopologySpread enabled",
data: []byte(`
apiVersion: kubescheduler.config.k8s.io/v1beta1
kind: KubeSchedulerConfiguration
profiles:
- pluginConfig:
- name: PodTopologySpread
args:
defaultConstraints: []
`),
feature: features.DefaultPodTopologySpread,
wantProfiles: []config.KubeSchedulerProfile{
{
SchedulerName: "default-scheduler",
PluginConfig: []config.PluginConfig{
{
Name: "PodTopologySpread",
Args: &config.PodTopologySpreadArgs{
DefaultConstraints: []corev1.TopologySpreadConstraint{},
},
},
},
},
},
Expand All @@ -299,6 +372,9 @@ profiles:
decoder := Codecs.UniversalDecoder()
for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
if tt.feature != "" {
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, tt.feature, true)()
}
obj, gvk, err := decoder.Decode(tt.data, nil, nil)
if err != nil {
if tt.wantErr != err.Error() {
Expand Down Expand Up @@ -373,6 +449,14 @@ func TestCodecsEncodePluginConfig(t *testing.T) {
},
},
},
{
Name: "PodTopologySpread",
Args: runtime.RawExtension{
Object: &v1beta1.PodTopologySpreadArgs{
DefaultConstraints: []corev1.TopologySpreadConstraint{},
},
},
},
{
Name: "OutOfTreePlugin",
Args: runtime.RawExtension{
Expand Down Expand Up @@ -428,6 +512,11 @@ profiles:
- name: mem
weight: 2
name: NodeResourcesLeastAllocated
- args:
apiVersion: kubescheduler.config.k8s.io/v1beta1
defaultConstraints: []
kind: PodTopologySpreadArgs
name: PodTopologySpread
- args:
foo: bar
name: OutOfTreePlugin
Expand Down Expand Up @@ -458,6 +547,10 @@ profiles:
BindTimeoutSeconds: 300,
},
},
{
Name: "PodTopologySpread",
Args: &config.PodTopologySpreadArgs{},
},
{
Name: "OutOfTreePlugin",
Args: &runtime.Unknown{
Expand Down Expand Up @@ -510,6 +603,11 @@ profiles:
bindTimeoutSeconds: 300
kind: VolumeBindingArgs
name: VolumeBinding
- args:
apiVersion: kubescheduler.config.k8s.io/v1beta1
defaultConstraints: null
kind: PodTopologySpreadArgs
name: PodTopologySpread
- args:
foo: bar
name: OutOfTreePlugin
Expand Down
16 changes: 13 additions & 3 deletions pkg/scheduler/apis/config/v1beta1/defaults_test.go
View file
Open in desktop
Expand Up @@ -25,7 +25,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/apiserver/pkg/util/feature"
componentbaseconfig "k8s.io/component-base/config/v1alpha1"
"k8s.io/component-base/featuregate"
featuregatetesting "k8s.io/component-base/featuregate/testing"
Expand Down Expand Up @@ -391,7 +391,7 @@ func TestPluginArgsDefaults(t *testing.T) {
},
},
{
name: "PodTopologySpreadArgs resources empty, NewPodTopologySpread feature enabled",
name: "PodTopologySpreadArgs resources empty, DefaultPodTopologySpread feature enabled",
feature: features.DefaultPodTopologySpread,
in: &v1beta1.PodTopologySpreadArgs{},
want: &v1beta1.PodTopologySpreadArgs{
Expand All @@ -409,13 +409,23 @@ func TestPluginArgsDefaults(t *testing.T) {
},
},
},
{
name: "PodTopologySpreadArgs empty array, DefaultPodTopologySpread feature enabled",
feature: features.DefaultPodTopologySpread,
in: &v1beta1.PodTopologySpreadArgs{
DefaultConstraints: []v1.TopologySpreadConstraint{},
},
want: &v1beta1.PodTopologySpreadArgs{
DefaultConstraints: []v1.TopologySpreadConstraint{},
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

There can add a test for the DefaultPodTopologySpread feature disabled.

A question, why we add those default test to encode/decode tests at scheme_test.go, seems the tests in defaults_test.go should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

There can add a test for the DefaultPodTopologySpread feature disabled.

That's the default, so it's already tested above.

A question, why we add those default test to encode/decode tests at scheme_test.go, seems the tests in defaults_test.go should be enough.

Because tests in scheme_test.go are more e2e-like, ensuring that we didn't mistype function names and that our assumptions of what the api-machinery does align with what we expect (nil vs empty array).

}
for _, tc := range tests {
scheme := runtime.NewScheme()
utilruntime.Must(AddToScheme(scheme))
t.Run(tc.name, func(t *testing.T) {
if tc.feature != "" {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, tc.feature, true)()
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, tc.feature, true)()
}
scheme.Default(tc.in)
if diff := cmp.Diff(tc.in, tc.want); diff != "" {
Expand Down