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

Add allow-container-concurrency-zero configmap property #7932

Merged
merged 3 commits into from
May 14, 2020
Merged

Add allow-container-concurrency-zero configmap property #7932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36eece6
Add allow-container-concurrency-zero configmap property
julz May 12, 2020
ac08d04
Use nice bool fields loop
julz May 13, 2020
94a4803
nit-dent
julz May 14, 2020
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
9 changes: 9 additions & 0 deletions config/core/configmaps/defaults.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,17 @@ data:
# the individual revisions cannot have arbitrary large concurrency
# values, or autoscaling targets. `container-concurrency` default setting
# must be at or below this value.
#
# Must be greater than 1.
#
# Note: even with this set, a user can choose a containerConcurrency
# of 0 (i.e. unbounded) unless allow-container-concurrency-zero is
# set to "false".
container-concurrency-max-limit: "1000"

# allow-container-concurrency-zero controls whether users can
# specify 0 (i.e. unbounded) for containerConcurrency.
allow-container-concurrency-zero: "true"

# feature flag indicates whether to enable multi container support or not
enable-multi-container: "false"
44 changes: 33 additions & 11 deletions pkg/apis/config/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,26 +53,44 @@ const (
// DefaultMaxRevisionContainerConcurrency is the maximum configurable
// container concurrency.
DefaultMaxRevisionContainerConcurrency int64 = 1000

// DefaultAllowContainerConcurrencyZero is whether, by default,
// containerConcurrency can be set to zero (i.e. unbounded) by users.
DefaultAllowContainerConcurrencyZero bool = true
Comment on lines +57 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit torn on necessity for named constant here, but 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha yeah funny you should say that, I spent five or so minutes I’m not getting back umming and ahhing on it too, but in the end it just looks so much nicer with all the constants lined up consistently in defaultConfig I think 🤷‍♂️

)

func defaultConfig() *Defaults {
return &Defaults{
RevisionTimeoutSeconds: DefaultRevisionTimeoutSeconds,
MaxRevisionTimeoutSeconds: DefaultMaxRevisionTimeoutSeconds,
UserContainerNameTemplate: DefaultUserContainerName,
ContainerConcurrency: DefaultContainerConcurrency,
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
RevisionTimeoutSeconds: DefaultRevisionTimeoutSeconds,
MaxRevisionTimeoutSeconds: DefaultMaxRevisionTimeoutSeconds,
UserContainerNameTemplate: DefaultUserContainerName,
ContainerConcurrency: DefaultContainerConcurrency,
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
AllowContainerConcurrencyZero: DefaultAllowContainerConcurrencyZero,
}
}

// NewDefaultsConfigFromMap creates a Defaults from the supplied Map
// NewDefaultsConfigFromMap creates a Defaults from the supplied Map.
func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) {
nc := defaultConfig()

// Process bool field.
nc.EnableMultiContainer = strings.EqualFold(data["enable-multi-container"], "true")
// Process bool fields.
for _, b := range []struct {
key string
field *bool
}{{
key: "enable-multi-container",
field: &nc.EnableMultiContainer,
}, {
key: "allow-container-concurrency-zero",
field: &nc.AllowContainerConcurrencyZero,
}} {
if raw, ok := data[b.key]; ok {
*b.field = strings.EqualFold(raw, "true")
}
}

// Process int64 fields
// Process int64 fields.
for _, i64 := range []struct {
key string
field *int64
Expand Down Expand Up @@ -153,14 +171,14 @@ func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) {
return nc, nil
}

// NewDefaultsConfigFromConfigMap creates a Defaults from the supplied configMap
// NewDefaultsConfigFromConfigMap creates a Defaults from the supplied configMap.
func NewDefaultsConfigFromConfigMap(config *corev1.ConfigMap) (*Defaults, error) {
return NewDefaultsConfigFromMap(config.Data)
}

// Defaults includes the default values to be populated by the webhook.
type Defaults struct {
// Feature flag to enable multi container support
// Feature flag to enable multi container support.
EnableMultiContainer bool

RevisionTimeoutSeconds int64
Expand All @@ -176,6 +194,10 @@ type Defaults struct {
// or target value in the system.
ContainerConcurrencyMaxLimit int64

// AllowContainerConcurrencyZero determines whether users are permitted to specify
// a containerConcurrency of 0 (i.e. unbounded).
AllowContainerConcurrencyZero bool

RevisionCPURequest *resource.Quantity
RevisionCPULimit *resource.Quantity
RevisionMemoryRequest *resource.Quantity
Expand Down
51 changes: 34 additions & 17 deletions pkg/apis/config/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,34 +70,51 @@ func TestDefaultsConfiguration(t *testing.T) {
name: "specified values",
wantErr: false,
wantDefaults: &Defaults{
EnableMultiContainer: true,
RevisionTimeoutSeconds: 123,
MaxRevisionTimeoutSeconds: 456,
ContainerConcurrencyMaxLimit: 1984,
RevisionCPURequest: &oneTwoThree,
UserContainerNameTemplate: "{{.Name}}",
EnableMultiContainer: true,
RevisionTimeoutSeconds: 123,
MaxRevisionTimeoutSeconds: 456,
ContainerConcurrencyMaxLimit: 1984,
RevisionCPURequest: &oneTwoThree,
UserContainerNameTemplate: "{{.Name}}",
AllowContainerConcurrencyZero: false,
},
data: map[string]string{
"enable-multi-container": "true",
"revision-timeout-seconds": "123",
"max-revision-timeout-seconds": "456",
"revision-cpu-request": "123m",
"container-concurrency-max-limit": "1984",
"container-name-template": "{{.Name}}",
"enable-multi-container": "true",
"revision-timeout-seconds": "123",
"max-revision-timeout-seconds": "456",
"revision-cpu-request": "123m",
"container-concurrency-max-limit": "1984",
"container-name-template": "{{.Name}}",
"allow-container-concurrency-zero": "false",
},
}, {
name: "invalid multi container flag value",
wantErr: false,
wantDefaults: &Defaults{
EnableMultiContainer: false,
RevisionTimeoutSeconds: DefaultRevisionTimeoutSeconds,
MaxRevisionTimeoutSeconds: DefaultMaxRevisionTimeoutSeconds,
UserContainerNameTemplate: DefaultUserContainerName,
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
EnableMultiContainer: false,
RevisionTimeoutSeconds: DefaultRevisionTimeoutSeconds,
MaxRevisionTimeoutSeconds: DefaultMaxRevisionTimeoutSeconds,
UserContainerNameTemplate: DefaultUserContainerName,
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
AllowContainerConcurrencyZero: DefaultAllowContainerConcurrencyZero,
},
data: map[string]string{
"enable-multi-container": "invalid",
},
}, {
name: "invalid allow container concurrency zero flag value",
wantErr: false,
wantDefaults: &Defaults{
EnableMultiContainer: false,
RevisionTimeoutSeconds: DefaultRevisionTimeoutSeconds,
MaxRevisionTimeoutSeconds: DefaultMaxRevisionTimeoutSeconds,
UserContainerNameTemplate: DefaultUserContainerName,
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
AllowContainerConcurrencyZero: false,
},
data: map[string]string{
"allow-container-concurrency-zero": "invalid",
},
}, {
name: "bad revision timeout",
wantErr: true,
Expand Down
10 changes: 8 additions & 2 deletions pkg/apis/serving/metadata_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,15 @@ func ValidateTimeoutSeconds(ctx context.Context, timeoutSeconds int64) *apis.Fie
func ValidateContainerConcurrency(ctx context.Context, containerConcurrency *int64) *apis.FieldError {
if containerConcurrency != nil {
cfg := config.FromContextOrDefaults(ctx).Defaults
if *containerConcurrency < 0 || *containerConcurrency > cfg.ContainerConcurrencyMaxLimit {

var minContainerConcurrency int64 = 0
if !cfg.AllowContainerConcurrencyZero {
minContainerConcurrency = 1
}

if *containerConcurrency < minContainerConcurrency || *containerConcurrency > cfg.ContainerConcurrencyMaxLimit {
return apis.ErrOutOfBoundsValue(
*containerConcurrency, 0, cfg.ContainerConcurrencyMaxLimit, apis.CurrentField)
*containerConcurrency, minContainerConcurrency, cfg.ContainerConcurrencyMaxLimit, apis.CurrentField)
}
}
return nil
Expand Down
11 changes: 11 additions & 0 deletions pkg/apis/serving/metadata_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,20 @@ func TestValidateContainerConcurrency(t *testing.T) {
})),
expectErr: apis.ErrOutOfBoundsValue(
2000, 0, 1950, apis.CurrentField),
}, {
name: "invalid containerConcurrency value, zero but allow-container-concurrency-zero is false",
containerConcurrency: ptr.Int64(0),
ctx: config.ToContext(context.Background(), cfg(map[string]string{
"allow-container-concurrency-zero": "false",
})),
expectErr: apis.ErrOutOfBoundsValue(
0, 1, config.DefaultMaxRevisionContainerConcurrency, apis.CurrentField),
}, {
name: "valid containerConcurrency value",
containerConcurrency: ptr.Int64(10),
}, {
name: "valid containerConcurrency value zero",
containerConcurrency: ptr.Int64(0),
}, {
name: "valid containerConcurrency value huge",
containerConcurrency: ptr.Int64(2019),
Expand Down