Add feature flags for affinity/nodeSelector/tolerations #8645

emaildanwilson · 2020-07-15T22:31:47Z

Proposed Changes

Adds a flag kubernetes.podspec-affinity in config-features with a default of disabled
Adds a flag kubernetes.podspec-nodeselector in config-features with a default of disabled
Adds a flag kubernetes.podspec-tolerations in config-features with a default of disabled
Updates FromContextOrDefaults function to supply defaults for config-default, config-features and config-autoscaler individually when the context is missing any of those sections. A code path was discovered where features is nil.
Adds unit tests for each feature flag, some test consolidation was done to remove some boilerplate.

Release Note

Feature flags are now available to enable affinity, nodeSelector and tolerations for Knative Services. See config-features for details.

knative-prow-robot

@emaildanwilson: 0 warnings.

In response to this:

Fixes #1816

Proposed Changes

Adds a flag kubernetes.podspec-affinity in config-features with a default of disabled

Adds a flag kubernetes.podspec-nodeselector in config-features with a default of disabled

Adds a flag kubernetes.podspec-tolerations in config-features with a default of disabled

Updates FromContextOrDefaults function to supply defaults for config-default, config-features and config-autoscaler individually when the context is missing any of those sections. A code path was discovered where features is nil.

Adds unit tests for each feature flag, some test consolidation was done to remove some boilerplate.

Release Note
Feature flags are now available to enable affinity, nodeSelector and tolerations for Knative Services. See config-features for details.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2020-07-15T22:32:00Z

Welcome @emaildanwilson! It looks like this is your first PR to knative/serving 🎉

knative-prow-robot · 2020-07-15T22:32:00Z

Hi @emaildanwilson. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

emaildanwilson · 2020-07-16T00:14:10Z

/assign @dprotaso

emaildanwilson · 2020-07-16T18:46:59Z

pkg/apis/config/store.go

-	if cfg := FromContext(ctx); cfg != nil {
-		return cfg
+	cfg := FromContext(ctx)
+	if cfg == nil {
+		cfg = &Config{}
 	}
-	defaults, _ := NewDefaultsConfigFromMap(map[string]string{})
-	features, _ := NewFeaturesConfigFromMap(map[string]string{})
-	autoscaler, _ := autoscalerconfig.NewConfigFromMap(map[string]string{})
-	return &Config{
-		Defaults:   defaults,
-		Features:   features,
-		Autoscaler: autoscaler,
+
+	if cfg.Defaults == nil {
+		defaults, _ := NewDefaultsConfigFromMap(map[string]string{})
+		cfg.Defaults = defaults
+	}
+
+	if cfg.Features == nil {
+		features, _ := NewFeaturesConfigFromMap(map[string]string{})
+		cfg.Features = features
+	}
+
+	if cfg.Autoscaler == nil {
+		autoscaler, _ := autoscalerconfig.NewConfigFromMap(map[string]string{})
+		cfg.Autoscaler = autoscaler
 	}
+	return cfg


This fixes a case where cfg is initialized in an autoscaler test with a context containing only autoscaling config. As a result Features is never populated and can panic when referenced. I thought it made sense to stop this from ever happening but if the preference is to add defensive code where Features is being used/or fix the autoscaler test I can make changes accordingly.

emaildanwilson · 2020-07-16T18:53:32Z

pkg/apis/serving/k8s_validation_test.go

@@ -513,6 +507,138 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
 	}
 }

+func TestPodSpecFeatureValidation(t *testing.T) {


This iterates each podSpec feature running four tests against each feature. It makes adding new tests for new podSpec features easy and reduces the amount of duplicate test code. If this is acceptable I can move the test for FieldRef into this too. If the preference is to duplicate the code for each of these for clarity I can break them out into a test function for each one. ty.

mpetason · 2020-07-17T15:52:58Z

/ok-to-test

knative-test-reporter-robot · 2020-07-17T17:54:36Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-upgrade-tests	2020-07-17 17:54:30.498 +0000 UTC	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-upgrade-tests

whaught · 2020-07-17T18:49:37Z

pkg/apis/serving/k8s_validation_test.go

@@ -34,16 +35,9 @@ import (

 type configOption func(*config.Config) *config.Config

-func withMultiContainer() configOption {
+func withFeature(feature string) configOption {


Suggested change

func withFeature(feature string) configOption {

func withFeature(feature string) configOption {

Maybe withFeatureEnabled or expand to withFeature(feature string, state Flag)

+1 - I'll go with withFeatureEnabled for now since the only use case is to enable features at this point.

mattmoor

Couple nits, but otherwise this looks like it will do the trick.

mattmoor · 2020-07-17T18:43:40Z

pkg/apis/config/store.go

+		defaults, _ := NewDefaultsConfigFromMap(map[string]string{})
+		cfg.Defaults = defaults


Suggested change

defaults, _ := NewDefaultsConfigFromMap(map[string]string{})

cfg.Defaults = defaults

cfg.Defaults, _ = NewDefaultsConfigFromMap(map[string]string{})

mattmoor · 2020-07-17T18:44:23Z

pkg/apis/config/store.go

+		features, _ := NewFeaturesConfigFromMap(map[string]string{})
+		cfg.Features = features


Suggested change

features, _ := NewFeaturesConfigFromMap(map[string]string{})

cfg.Features = features

cfg.Features, _ = NewFeaturesConfigFromMap(map[string]string{})

mattmoor · 2020-07-17T18:44:40Z

pkg/apis/config/store.go

+		autoscaler, _ := autoscalerconfig.NewConfigFromMap(map[string]string{})
+		cfg.Autoscaler = autoscaler


Suggested change

autoscaler, _ := autoscalerconfig.NewConfigFromMap(map[string]string{})

cfg.Autoscaler = autoscaler

cfg.Autoscaler, _ = autoscalerconfig.NewConfigFromMap(map[string]string{})

mattmoor · 2020-07-17T18:45:16Z

pkg/apis/serving/fieldmask.go

@@ -136,7 +137,7 @@ func KeyToPathMask(in *corev1.KeyToPath) *corev1.KeyToPath {
 // PodSpecMask performs a _shallow_ copy of the Kubernetes PodSpec object to a new
 // Kubernetes PodSpec object bringing over only the fields allowed in the Knative API. This
 // does not validate the contents or the bounds of the provided fields.
-func PodSpecMask(in *corev1.PodSpec) *corev1.PodSpec {
+func PodSpecMask(in *corev1.PodSpec, features *config.Features) *corev1.PodSpec {


How hard would it be to plumb context into here instead?

That should be easy enough. I'll give it a go.

mattmoor · 2020-07-17T18:48:49Z

pkg/apis/serving/k8s_validation_test.go

-func withFieldRef() configOption {
-	return func(cfg *config.Config) *config.Config {
-		cfg.Features.PodSpecFieldRef = config.Enabled
+		reflect.Indirect(reflect.ValueOf(&cfg.Features)).Elem().FieldByName(feature).Set(reflect.ValueOf(config.Enabled))


I feel like if folks hold this wrong (even a typo) they're gonna have a really bad time.

Agreed but it's test code only and reduces boiler plate. If you'd still rather I spell each function out then I don't mind.

let's spell it out for now.

I can imagine a simpler solution where each feature is a package constant with some helper methods to apply states on configs.

ok, will do.

evankanderson · 2020-07-17T18:51:41Z

/assign @whaught

Just to make sure that Weston's review is easily findable when he's working on his approver badge.

dprotaso · 2020-07-17T20:49:14Z

pkg/apis/config/store.go

+		defaults, _ := NewDefaultsConfigFromMap(map[string]string{})
+		cfg.Defaults = defaults


nit:

Suggested change

defaults, _ := NewDefaultsConfigFromMap(map[string]string{})

cfg.Defaults = defaults

cfg.Defaults = defaultDefaultsConfig()

dprotaso · 2020-07-17T20:49:38Z

pkg/apis/config/store.go

+		features, _ := NewFeaturesConfigFromMap(map[string]string{})
+		cfg.Features = features


nit:

Suggested change

features, _ := NewFeaturesConfigFromMap(map[string]string{})

cfg.Features = features

cfg.Features = defaultFeaturesConfig()

dprotaso · 2020-07-17T20:51:57Z

pkg/apis/config/store.go

+	}
+
+	if cfg.Autoscaler == nil {
+		autoscaler, _ := autoscalerconfig.NewConfigFromMap(map[string]string{})


We should expose these consistently - but that's a separate issue

dprotaso · 2020-07-17T21:07:38Z

pkg/apis/serving/k8s_validation_test.go

+		podSpec: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Image: "busybox",
+			}},
+		},


all the podSpecs are the same so I wonder if we can drop as an input

most likely. I was thinking there may be cases where it would be needed but clearly I didn't have one :)

dprotaso · 2020-07-17T21:40:10Z

pkg/apis/serving/fieldmask.go

@@ -136,11 +139,12 @@ func KeyToPathMask(in *corev1.KeyToPath) *corev1.KeyToPath {
 // PodSpecMask performs a _shallow_ copy of the Kubernetes PodSpec object to a new
 // Kubernetes PodSpec object bringing over only the fields allowed in the Knative API. This
 // does not validate the contents or the bounds of the provided fields.
-func PodSpecMask(in *corev1.PodSpec) *corev1.PodSpec {
+func PodSpecMask(in *corev1.PodSpec, ctx context.Context) *corev1.PodSpec {


Can you put the context first

yes, np. done.

update formatting improvements from review feedback spell out functions to enable features put context first

emaildanwilson · 2020-07-17T21:55:30Z

Thank you @mattmoor @dprotaso & @whaught for the review!
Feedback is incorporated.

dprotaso · 2020-07-17T21:55:41Z

/lgtm
/approve

thanks for the PR @emaildanwilson 🎉

knative-prow-robot · 2020-07-17T21:55:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, emaildanwilson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~config/OWNERS~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-metrics-robot · 2020-07-17T21:57:24Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/fieldmask.go	97.9%	97.9%	0.0

whaught · 2020-07-17T22:14:35Z

/lgtm

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 15, 2020

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 15, 2020

knative-prow-robot requested review from markusthoemmes and mattmoor July 15, 2020 22:31

knative-prow-robot reviewed Jul 15, 2020

View reviewed changes

knative-prow-robot added area/API API objects and controllers needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 15, 2020

mattmoor reviewed Jul 15, 2020

View reviewed changes

emaildanwilson force-pushed the nodeSelector branch from 1e59637 to 699476c Compare July 15, 2020 22:49

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2020

emaildanwilson changed the title ~~WIP: add feature flags for affinity/nodeSelector/tolerations~~ Add feature flags for affinity/nodeSelector/tolerations Jul 15, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020

knative-prow-robot assigned dprotaso Jul 16, 2020

emaildanwilson commented Jul 16, 2020

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 17, 2020

whaught reviewed Jul 17, 2020

View reviewed changes

mattmoor reviewed Jul 17, 2020

View reviewed changes

knative-prow-robot assigned whaught Jul 17, 2020

dprotaso reviewed Jul 17, 2020

View reviewed changes

emaildanwilson force-pushed the nodeSelector branch from 4c682b1 to 8c41046 Compare July 17, 2020 21:46

add feature flags for affinity/nodeSelector/tolerations

c44a38a

update formatting improvements from review feedback spell out functions to enable features put context first

emaildanwilson force-pushed the nodeSelector branch from 8c41046 to c44a38a Compare July 17, 2020 21:51

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2020

knative-prow-robot merged commit ad1aafd into knative:master Jul 17, 2020

emaildanwilson deleted the nodeSelector branch July 18, 2020 21:00

fvoznika mentioned this pull request Aug 12, 2020

Allow runtimeClassName and dnsPolicy in service template spec #5306

Closed

dprotaso mentioned this pull request May 9, 2022

Support RuntimeClass behind a feature flag #12921

Closed

	func withFeature(feature string) configOption {
	func withFeature(feature string) configOption {

		defaults, _ := NewDefaultsConfigFromMap(map[string]string{})
		cfg.Defaults = defaults

	defaults, _ := NewDefaultsConfigFromMap(map[string]string{})
	cfg.Defaults = defaults
	cfg.Defaults, _ = NewDefaultsConfigFromMap(map[string]string{})

		features, _ := NewFeaturesConfigFromMap(map[string]string{})
		cfg.Features = features

	features, _ := NewFeaturesConfigFromMap(map[string]string{})
	cfg.Features = features
	cfg.Features, _ = NewFeaturesConfigFromMap(map[string]string{})

		autoscaler, _ := autoscalerconfig.NewConfigFromMap(map[string]string{})
		cfg.Autoscaler = autoscaler

	autoscaler, _ := autoscalerconfig.NewConfigFromMap(map[string]string{})
	cfg.Autoscaler = autoscaler
	cfg.Autoscaler, _ = autoscalerconfig.NewConfigFromMap(map[string]string{})

	defaults, _ := NewDefaultsConfigFromMap(map[string]string{})
	cfg.Defaults = defaults
	cfg.Defaults = defaultDefaultsConfig()

	features, _ := NewFeaturesConfigFromMap(map[string]string{})
	cfg.Features = features
	cfg.Features = defaultFeaturesConfig()

Add feature flags for affinity/nodeSelector/tolerations #8645

Add feature flags for affinity/nodeSelector/tolerations #8645

Conversation

emaildanwilson commented Jul 15, 2020 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Jul 15, 2020

knative-prow-robot commented Jul 15, 2020

mattmoor left a comment

Choose a reason for hiding this comment

emaildanwilson commented Jul 16, 2020

Choose a reason for hiding this comment

emaildanwilson Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

mpetason commented Jul 17, 2020

knative-test-reporter-robot commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emaildanwilson commented Jul 17, 2020

dprotaso commented Jul 17, 2020

knative-prow-robot commented Jul 17, 2020

knative-metrics-robot commented Jul 17, 2020

whaught commented Jul 17, 2020

emaildanwilson commented Jul 15, 2020 •

edited

Loading

emaildanwilson Jul 16, 2020 •

edited

Loading