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

Add support for topology spread constraint #12715

Merged
merged 10 commits into from
Mar 18, 2022
Merged

Add support for topology spread constraint #12715

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
eea05ab
add topology spread
stevenchen-db Mar 9, 2022
d153376
fix
stevenchen-db Mar 14, 2022
3157769
fix test
stevenchen-db Mar 14, 2022
16acca0
Merge branch 'main' into topology-spread
stevenchen-db Mar 14, 2022
253a7f0
add to schema
stevenchen-db Mar 14, 2022
987ae13
remove from schema
stevenchen-db Mar 14, 2022
b967468
fmt
stevenchen-db Mar 16, 2022
daaa81a
Merge branch 'main' into topology-spread
stevenchen-db Mar 16, 2022
3455f98
address comments
stevenchen-db Mar 17, 2022
04cebd0
Merge branch 'main' into topology-spread
stevenchen-db Mar 17, 2022
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
8 changes: 7 additions & 1 deletion config/core/configmaps/features.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ metadata:
app.kubernetes.io/version: devel
serving.knative.dev/release: devel
annotations:
knative.dev/example-checksum: "d9e300ba"
knative.dev/example-checksum: "e1c6e542"
data:
_example: |-
################################
Expand Down Expand Up @@ -53,6 +53,12 @@ data:
# See: https://knative.dev/docs/serving/feature-flags/#kubernetes-node-affinity
kubernetes.podspec-affinity: "disabled"

# Indicates whether Kubernetes topologySpreadConstraints support is enabled
#
# WARNING: Cannot safely be disabled once enabled.
# See: https://knative.dev/docs/serving/feature-flags/#kubernetes-topology-spread-constraints
kubernetes.podspec-topologyspreadconstraints: "disabled"

# Indicates whether Kubernetes hostAliases support is enabled
#
# WARNING: Cannot safely be disabled once enabled.
Expand Down
75 changes: 39 additions & 36 deletions pkg/apis/config/features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,24 +41,25 @@ const (

func defaultFeaturesConfig() *Features {
return &Features{
MultiContainer: Enabled,
PodSpecAffinity: Disabled,
PodSpecDryRun: Allowed,
PodSpecHostAliases: Disabled,
PodSpecFieldRef: Disabled,
PodSpecNodeSelector: Disabled,
PodSpecRuntimeClassName: Disabled,
PodSpecSecurityContext: Disabled,
PodSpecPriorityClassName: Disabled,
PodSpecSchedulerName: Disabled,
ContainerSpecAddCapabilities: Disabled,
PodSpecTolerations: Disabled,
PodSpecVolumesEmptyDir: Disabled,
PodSpecPersistentVolumeClaim: Disabled,
PodSpecPersistentVolumeWrite: Disabled,
PodSpecInitContainers: Disabled,
TagHeaderBasedRouting: Disabled,
AutoDetectHTTP2: Disabled,
MultiContainer: Enabled,
PodSpecAffinity: Disabled,
PodSpecTopologySpreadConstraints: Disabled,
PodSpecDryRun: Allowed,
PodSpecHostAliases: Disabled,
PodSpecFieldRef: Disabled,
PodSpecNodeSelector: Disabled,
PodSpecRuntimeClassName: Disabled,
PodSpecSecurityContext: Disabled,
PodSpecPriorityClassName: Disabled,
PodSpecSchedulerName: Disabled,
ContainerSpecAddCapabilities: Disabled,
PodSpecTolerations: Disabled,
PodSpecVolumesEmptyDir: Disabled,
PodSpecPersistentVolumeClaim: Disabled,
PodSpecPersistentVolumeWrite: Disabled,
PodSpecInitContainers: Disabled,
TagHeaderBasedRouting: Disabled,
AutoDetectHTTP2: Disabled,
}
}

Expand All @@ -69,6 +70,7 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
if err := cm.Parse(data,
asFlag("multi-container", &nc.MultiContainer),
asFlag("kubernetes.podspec-affinity", &nc.PodSpecAffinity),
asFlag("kubernetes.podspec-topologyspreadconstraints", &nc.PodSpecTopologySpreadConstraints),
asFlag("kubernetes.podspec-dryrun", &nc.PodSpecDryRun),
asFlag("kubernetes.podspec-hostaliases", &nc.PodSpecHostAliases),
asFlag("kubernetes.podspec-fieldref", &nc.PodSpecFieldRef),
Expand Down Expand Up @@ -97,24 +99,25 @@ func NewFeaturesConfigFromConfigMap(config *corev1.ConfigMap) (*Features, error)

// Features specifies which features are allowed by the webhook.
type Features struct {
MultiContainer Flag
PodSpecAffinity Flag
PodSpecDryRun Flag
PodSpecFieldRef Flag
PodSpecHostAliases Flag
PodSpecNodeSelector Flag
PodSpecRuntimeClassName Flag
PodSpecSecurityContext Flag
PodSpecPriorityClassName Flag
PodSpecSchedulerName Flag
ContainerSpecAddCapabilities Flag
PodSpecTolerations Flag
PodSpecVolumesEmptyDir Flag
PodSpecInitContainers Flag
PodSpecPersistentVolumeClaim Flag
PodSpecPersistentVolumeWrite Flag
TagHeaderBasedRouting Flag
AutoDetectHTTP2 Flag
MultiContainer Flag
PodSpecAffinity Flag
PodSpecTopologySpreadConstraints Flag
PodSpecDryRun Flag
PodSpecFieldRef Flag
PodSpecHostAliases Flag
PodSpecNodeSelector Flag
PodSpecRuntimeClassName Flag
PodSpecSecurityContext Flag
PodSpecPriorityClassName Flag
PodSpecSchedulerName Flag
ContainerSpecAddCapabilities Flag
PodSpecTolerations Flag
PodSpecVolumesEmptyDir Flag
PodSpecInitContainers Flag
PodSpecPersistentVolumeClaim Flag
PodSpecPersistentVolumeWrite Flag
TagHeaderBasedRouting Flag
AutoDetectHTTP2 Flag
}

// asFlag parses the value at key as a Flag into the target, if it exists.
Expand Down
77 changes: 53 additions & 24 deletions pkg/apis/config/features_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,30 +59,32 @@ func TestFeaturesConfiguration(t *testing.T) {
name: "features Enabled",
wantErr: false,
wantFeatures: defaultWith(&Features{
MultiContainer: Enabled,
PodSpecAffinity: Enabled,
PodSpecDryRun: Enabled,
PodSpecHostAliases: Enabled,
PodSpecNodeSelector: Enabled,
PodSpecRuntimeClassName: Enabled,
PodSpecSecurityContext: Enabled,
PodSpecTolerations: Enabled,
PodSpecPriorityClassName: Enabled,
PodSpecSchedulerName: Enabled,
TagHeaderBasedRouting: Enabled,
}),
data: map[string]string{
"multi-container": "Enabled",
"kubernetes.podspec-affinity": "Enabled",
"kubernetes.podspec-dryrun": "Enabled",
"kubernetes.podspec-hostaliases": "Enabled",
"kubernetes.podspec-nodeselector": "Enabled",
"kubernetes.podspec-runtimeclassname": "Enabled",
"kubernetes.podspec-securitycontext": "Enabled",
"kubernetes.podspec-tolerations": "Enabled",
"kubernetes.podspec-priorityclassname": "Enabled",
"kubernetes.podspec-schedulername": "Enabled",
"tag-header-based-routing": "Enabled",
MultiContainer: Enabled,
PodSpecAffinity: Enabled,
PodSpecTopologySpreadConstraints: Enabled,
PodSpecDryRun: Enabled,
PodSpecHostAliases: Enabled,
PodSpecNodeSelector: Enabled,
PodSpecRuntimeClassName: Enabled,
PodSpecSecurityContext: Enabled,
PodSpecTolerations: Enabled,
PodSpecPriorityClassName: Enabled,
PodSpecSchedulerName: Enabled,
TagHeaderBasedRouting: Enabled,
}),
data: map[string]string{
"multi-container": "Enabled",
"kubernetes.podspec-affinity": "Enabled",
"kubernetes.podspec-topologyspreadconstraints": "Enabled",
"kubernetes.podspec-dryrun": "Enabled",
"kubernetes.podspec-hostaliases": "Enabled",
"kubernetes.podspec-nodeselector": "Enabled",
"kubernetes.podspec-runtimeclassname": "Enabled",
"kubernetes.podspec-securitycontext": "Enabled",
"kubernetes.podspec-tolerations": "Enabled",
"kubernetes.podspec-priorityclassname": "Enabled",
"kubernetes.podspec-schedulername": "Enabled",
"tag-header-based-routing": "Enabled",
},
}, {
name: "multi-container Allowed",
Expand Down Expand Up @@ -129,6 +131,33 @@ func TestFeaturesConfiguration(t *testing.T) {
data: map[string]string{
"kubernetes.podspec-affinity": "Disabled",
},
}, {
name: "kubernetes.podspec-topologyspreadconstraints Allowed",
wantErr: false,
wantFeatures: defaultWith(&Features{
PodSpecTopologySpreadConstraints: Allowed,
}),
data: map[string]string{
"kubernetes.podspec-topologyspreadconstraints": "Allowed",
},
}, {
name: "kubernetes.podspec-topologyspreadconstraints Enabled",
wantErr: false,
wantFeatures: defaultWith(&Features{
PodSpecTopologySpreadConstraints: Enabled,
}),
data: map[string]string{
"kubernetes.podspec-topologyspreadconstraints": "Enabled",
},
}, {
name: "kubernetes.podspec-topologyspreadconstraints Disabled",
wantErr: false,
wantFeatures: defaultWith(&Features{
PodSpecTopologySpreadConstraints: Disabled,
}),
data: map[string]string{
"kubernetes.podspec-topologyspreadconstraints": "Disabled",
},
}, {
name: "kubernetes.podspec-fieldref Allowed",
wantErr: false,
Expand Down
3 changes: 3 additions & 0 deletions pkg/apis/serving/fieldmask.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,9 @@ func PodSpecMask(ctx context.Context, in *corev1.PodSpec) *corev1.PodSpec {
if cfg.Features.PodSpecAffinity != config.Disabled {
out.Affinity = in.Affinity
}
if cfg.Features.PodSpecTopologySpreadConstraints != config.Disabled {
out.TopologySpreadConstraints = in.TopologySpreadConstraints
}
if cfg.Features.PodSpecHostAliases != config.Disabled {
out.HostAliases = in.HostAliases
}
Expand Down
29 changes: 29 additions & 0 deletions pkg/apis/serving/k8s_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/validation"
Expand Down Expand Up @@ -65,6 +66,13 @@ func withPodSpecAffinityEnabled() configOption {
}
}

func withPodSpecTopologySpreadConstraintsEnabled() configOption {
return func(cfg *config.Config) *config.Config {
cfg.Features.PodSpecTopologySpreadConstraints = config.Enabled
return cfg
}
}

func withPodSpecHostAliasesEnabled() configOption {
return func(cfg *config.Config) *config.Config {
cfg.Features.PodSpecHostAliases = config.Enabled
Expand Down Expand Up @@ -1033,6 +1041,27 @@ func TestPodSpecFeatureValidation(t *testing.T) {
Paths: []string{"affinity"},
},
cfgOpts: []configOption{withPodSpecAffinityEnabled()},
}, {
name: "TopologySpreadConstraints",
featureSpec: corev1.PodSpec{
TopologySpreadConstraints: []corev1.TopologySpreadConstraint{{
MaxSkew: 1,
TopologyKey: "topology.kubernetes.io/zone",
WhenUnsatisfiable: "DoNotSchedule",
LabelSelector: &metav1.LabelSelector{
MatchExpressions: []metav1.LabelSelectorRequirement{{
Key: "key",
Operator: "In",
Values: []string{"value"},
}},
},
}},
},
err: &apis.FieldError{
Message: "must not set the field(s)",
Paths: []string{"topologySpreadConstraints"},
},
cfgOpts: []configOption{withPodSpecTopologySpreadConstraintsEnabled()},
}, {
name: "HostAliases",
featureSpec: corev1.PodSpec{
Expand Down
29 changes: 15 additions & 14 deletions pkg/reconciler/route/resources/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,20 +421,21 @@ func testConfig() *config.Config {
TagTemplate: network.DefaultTagTemplate,
},
Features: &apiConfig.Features{
MultiContainer: apiConfig.Disabled,
PodSpecAffinity: apiConfig.Disabled,
PodSpecFieldRef: apiConfig.Disabled,
PodSpecDryRun: apiConfig.Enabled,
PodSpecHostAliases: apiConfig.Disabled,
PodSpecNodeSelector: apiConfig.Disabled,
PodSpecTolerations: apiConfig.Disabled,
PodSpecVolumesEmptyDir: apiConfig.Disabled,
PodSpecPersistentVolumeClaim: apiConfig.Disabled,
PodSpecPersistentVolumeWrite: apiConfig.Disabled,
PodSpecInitContainers: apiConfig.Disabled,
PodSpecPriorityClassName: apiConfig.Disabled,
PodSpecSchedulerName: apiConfig.Disabled,
TagHeaderBasedRouting: apiConfig.Disabled,
MultiContainer: apiConfig.Disabled,
PodSpecAffinity: apiConfig.Disabled,
PodSpecTopologySpreadConstraints: apiConfig.Disabled,
Copy link
Member

Choose a reason for hiding this comment

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

Is this change necessary? Curious what failed when you didn't have this?

Copy link
Contributor Author

@stevenchen-db stevenchen-db Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure, I was mostly going off of this as an example PR 20be262. Is it safe to not include PodSpecTopologySpreadConstraints?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think these changes are necessary then - can you drop them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Also deleted the line you mentioned below

PodSpecFieldRef: apiConfig.Disabled,
PodSpecDryRun: apiConfig.Enabled,
PodSpecHostAliases: apiConfig.Disabled,
PodSpecNodeSelector: apiConfig.Disabled,
PodSpecTolerations: apiConfig.Disabled,
PodSpecVolumesEmptyDir: apiConfig.Disabled,
PodSpecPersistentVolumeClaim: apiConfig.Disabled,
PodSpecPersistentVolumeWrite: apiConfig.Disabled,
PodSpecInitContainers: apiConfig.Disabled,
PodSpecPriorityClassName: apiConfig.Disabled,
PodSpecSchedulerName: apiConfig.Disabled,
TagHeaderBasedRouting: apiConfig.Disabled,
},
}
}
21 changes: 11 additions & 10 deletions pkg/reconciler/route/table_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3272,16 +3272,17 @@ func reconcilerTestConfig() *config.Config {
DefaultExternalScheme: "http",
},
Features: &cfgmap.Features{
MultiContainer: cfgmap.Disabled,
PodSpecAffinity: cfgmap.Disabled,
PodSpecFieldRef: cfgmap.Disabled,
PodSpecDryRun: cfgmap.Enabled,
PodSpecHostAliases: cfgmap.Disabled,
PodSpecNodeSelector: cfgmap.Disabled,
PodSpecTolerations: cfgmap.Disabled,
PodSpecPriorityClassName: cfgmap.Disabled,
PodSpecSchedulerName: cfgmap.Disabled,
TagHeaderBasedRouting: cfgmap.Disabled,
MultiContainer: cfgmap.Disabled,
PodSpecAffinity: cfgmap.Disabled,
PodSpecTopologySpreadConstraints: cfgmap.Disabled,
Copy link
Member

Choose a reason for hiding this comment

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

same question as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same answer as above

PodSpecFieldRef: cfgmap.Disabled,
PodSpecDryRun: cfgmap.Enabled,
PodSpecHostAliases: cfgmap.Disabled,
PodSpecNodeSelector: cfgmap.Disabled,
PodSpecTolerations: cfgmap.Disabled,
PodSpecPriorityClassName: cfgmap.Disabled,
PodSpecSchedulerName: cfgmap.Disabled,
TagHeaderBasedRouting: cfgmap.Disabled,
},
}
}
Expand Down