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.

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 autoscaler ConfigMap to revision reconciler and create deployment with initial scale #8355

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions pkg/reconciler/revision/config/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type Config struct {
Network *network.Config
Observability *metrics.ObservabilityConfig
Tracing *pkgtracing.Config
Autoscaler *autoscalerconfig.Config
}

// FromContext loads the configuration from the context.
Expand Down Expand Up @@ -93,5 +94,6 @@ func (s *Store) Load() *Config {
Network: s.UntypedLoad(network.ConfigName).(*network.Config).DeepCopy(),
Observability: s.UntypedLoad(metrics.ConfigMapName()).(*metrics.ObservabilityConfig).DeepCopy(),
Tracing: s.UntypedLoad(pkgtracing.ConfigName).(*pkgtracing.Config).DeepCopy(),
Autoscaler: s.UntypedLoad(autoscalerconfig.ConfigName).(*autoscalerconfig.Config).DeepCopy(),
}
}
12 changes: 12 additions & 0 deletions pkg/reconciler/revision/config/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,13 @@ func TestStoreLoadWithContext(t *testing.T) {
t.Error("Unexpected defaults config (-want, +got):", diff)
}
})

t.Run("autoscaler", func(t *testing.T) {
expected, _ := autoscalerconfig.NewConfigFromConfigMap(autoscalerConfig)
if diff := cmp.Diff(expected, config.Autoscaler); diff != "" {
t.Error("Unexpected autoscaler config (-want, +got):", diff)
}
})
}

func TestStoreImmutableConfig(t *testing.T) {
Expand All @@ -146,6 +153,8 @@ func TestStoreImmutableConfig(t *testing.T) {
config.Logging.LoggingConfig = "mutated"
ccMutated := int64(4)
config.Defaults.ContainerConcurrency = ccMutated
scaleupMutated := float64(4)
config.Autoscaler.MaxScaleUpRate = scaleupMutated

newConfig := store.Load()

Expand All @@ -158,4 +167,7 @@ func TestStoreImmutableConfig(t *testing.T) {
if newConfig.Defaults.ContainerConcurrency == ccMutated {
t.Error("Defaults config is not immutable")
}
if newConfig.Autoscaler.MaxScaleUpRate == scaleupMutated {
t.Error("Autoscaler config is not immutable")
}
}
2 changes: 2 additions & 0 deletions pkg/reconciler/revision/cruds.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func (c *Reconciler) createDeployment(ctx context.Context, rev *v1.Revision) (*a
cfgs.Network,
cfgs.Observability,
cfgs.Deployment,
cfgs.Autoscaler,
)

if err != nil {
Expand All @@ -63,6 +64,7 @@ func (c *Reconciler) checkAndUpdateDeployment(ctx context.Context, rev *v1.Revis
cfgs.Network,
cfgs.Observability,
cfgs.Deployment,
cfgs.Autoscaler,
)

if err != nil {
Expand Down
15 changes: 13 additions & 2 deletions pkg/reconciler/revision/resources/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ import (
"knative.dev/pkg/metrics"
"knative.dev/pkg/ptr"
tracingconfig "knative.dev/pkg/tracing/config"
"knative.dev/serving/pkg/apis/autoscaling"
"knative.dev/serving/pkg/apis/serving"
v1 "knative.dev/serving/pkg/apis/serving/v1"
asconfig "knative.dev/serving/pkg/autoscaler/config"
"knative.dev/serving/pkg/deployment"
"knative.dev/serving/pkg/network"
"knative.dev/serving/pkg/queue"
Expand Down Expand Up @@ -217,7 +219,9 @@ func buildUserPortEnv(userPort string) corev1.EnvVar {

// MakeDeployment constructs a K8s Deployment resource from a revision.
func MakeDeployment(rev *v1.Revision,
loggingConfig *logging.Config, tracingConfig *tracingconfig.Config, networkConfig *network.Config, observabilityConfig *metrics.ObservabilityConfig, deploymentConfig *deployment.Config) (*appsv1.Deployment, error) {
loggingConfig *logging.Config, tracingConfig *tracingconfig.Config, networkConfig *network.Config,
observabilityConfig *metrics.ObservabilityConfig, deploymentConfig *deployment.Config,
autoscalerConfig *asconfig.Config) (*appsv1.Deployment, error) {

podTemplateAnnotations := kmeta.FilterMap(rev.GetAnnotations(), func(k string) bool {
return k == serving.RevisionLastPinnedAnnotationKey
Expand All @@ -228,6 +232,13 @@ func MakeDeployment(rev *v1.Revision,
return nil, fmt.Errorf("failed to create PodSpec: %w", err)
}

replicaCount := int(autoscalerConfig.InitialScale)
ann, found := rev.ObjectMeta.Annotations[autoscaling.InitialScaleAnnotationKey]
if found {
// Ignore errors and no error checking because already validated in webhook.
replicaCount, _ = strconv.Atoi(ann)
}

return &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: names.Deployment(rev),
Expand All @@ -240,7 +251,7 @@ func MakeDeployment(rev *v1.Revision,
OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(rev)},
},
Spec: appsv1.DeploymentSpec{
Replicas: ptr.Int32(1),
Replicas: ptr.Int32(int32(replicaCount)),
Selector: makeSelector(rev),
ProgressDeadlineSeconds: ptr.Int32(int32(deploymentConfig.ProgressDeadline.Seconds())),
Template: corev1.PodTemplateSpec{
Expand Down
63 changes: 56 additions & 7 deletions pkg/reconciler/revision/resources/deploy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ import (
"knative.dev/pkg/ptr"
"knative.dev/pkg/system"
_ "knative.dev/pkg/system/testing"
"knative.dev/serving/pkg/apis/autoscaling"
"knative.dev/serving/pkg/apis/serving"
v1 "knative.dev/serving/pkg/apis/serving/v1"
asconfig "knative.dev/serving/pkg/autoscaler/config"
"knative.dev/serving/pkg/deployment"
"knative.dev/serving/pkg/network"

Expand Down Expand Up @@ -939,17 +941,18 @@ func TestMakePodSpec(t *testing.T) {

func TestMissingProbeError(t *testing.T) {
if _, err := MakeDeployment(revision("bar", "foo"), &logConfig, &traceConfig,
&network.Config{}, &obsConfig, &deploymentConfig); err == nil {
&network.Config{}, &obsConfig, &deploymentConfig, &asConfig); err == nil {
t.Error("expected error from MakeDeployment")
}
}

func TestMakeDeployment(t *testing.T) {
tests := []struct {
name string
rev *v1.Revision
want *appsv1.Deployment
dc deployment.Config
name string
rev *v1.Revision
want *appsv1.Deployment
dc deployment.Config
acMutator func(*asconfig.Config)
}{{
name: "with concurrency=1",
rev: revision("bar", "foo",
Expand Down Expand Up @@ -1023,19 +1026,65 @@ func TestMakeDeployment(t *testing.T) {
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Spec.ProgressDeadlineSeconds = ptr.Int32(42)
}),
}, {
name: "cluster initial scale",
acMutator: func(ac *asconfig.Config) {
ac.InitialScale = 10
},
rev: revision("bar", "foo",
withoutLabels,
withContainers([]corev1.Container{{
Name: servingContainerName,
Image: "ubuntu",
ReadinessProbe: withTCPReadinessProbe(12345),
}}),
),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Spec.Replicas = ptr.Int32(int32(10))
}),
}, {
name: "cluster initial scale override by revision initial scale",
acMutator: func(ac *asconfig.Config) {
ac.InitialScale = 10
},
rev: revision("bar", "foo",
withoutLabels,
withContainers([]corev1.Container{{
Name: servingContainerName,
Image: "ubuntu",
ReadinessProbe: withTCPReadinessProbe(12345),
}}),
func(revision *v1.Revision) {
revision.ObjectMeta.Annotations = map[string]string{autoscaling.InitialScaleAnnotationKey: "20"}
},
),
want: appsv1deployment(func(deploy *appsv1.Deployment) {
deploy.Spec.Replicas = ptr.Int32(int32(20))
deploy.Spec.Template.ObjectMeta.Annotations = map[string]string{autoscaling.InitialScaleAnnotationKey: "20"}
deploy.ObjectMeta.Annotations = map[string]string{autoscaling.InitialScaleAnnotationKey: "20"}
}),
}}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
ac := &asconfig.Config{
InitialScale: 1,
AllowZeroInitialScale: false,
}
if test.acMutator != nil {
test.acMutator(ac)
}
// Tested above so that we can rely on it here for brevity.
podSpec, err := makePodSpec(test.rev, &logConfig, &traceConfig,
&obsConfig, &deploymentConfig)
if err != nil {
t.Fatal("makePodSpec returned error:", err)
}
test.want.Spec.Template.Spec = *podSpec
if test.want != nil {
test.want.Spec.Template.Spec = *podSpec
}
got, err := MakeDeployment(test.rev, &logConfig, &traceConfig,
&network.Config{}, &obsConfig, &test.dc)
&network.Config{}, &obsConfig, &test.dc, ac)
if err != nil {
t.Fatal("got unexpected error:", err)
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/reconciler/revision/resources/queue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
tracingconfig "knative.dev/pkg/tracing/config"
"knative.dev/serving/pkg/apis/serving"
v1 "knative.dev/serving/pkg/apis/serving/v1"
asconfig "knative.dev/serving/pkg/autoscaler/config"
"knative.dev/serving/pkg/deployment"
"knative.dev/serving/pkg/network"
)
Expand All @@ -66,6 +67,10 @@ var (
traceConfig tracingconfig.Config
obsConfig metrics.ObservabilityConfig
deploymentConfig deployment.Config
asConfig = asconfig.Config{
InitialScale: 1,
AllowZeroInitialScale: false,
}
)

const testProbeJSONTemplate = `{"tcpSocket":{"port":%d,"host":"127.0.0.1"}}`
Expand Down
7 changes: 6 additions & 1 deletion pkg/reconciler/revision/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
asv1a1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1"
defaultconfig "knative.dev/serving/pkg/apis/config"
v1 "knative.dev/serving/pkg/apis/serving/v1"
asconfig "knative.dev/serving/pkg/autoscaler/config"
servingclient "knative.dev/serving/pkg/client/injection/client"
revisionreconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1/revision"
"knative.dev/serving/pkg/reconciler/revision/config"
Expand Down Expand Up @@ -716,7 +717,7 @@ func deploy(t *testing.T, namespace, name string, opts ...interface{}) *appsv1.D
// before calling MakeDeployment within Reconcile.
rev.SetDefaults(context.Background())
deployment, err := resources.MakeDeployment(rev, cfg.Logging, cfg.Tracing, cfg.Network,
cfg.Observability, cfg.Deployment)
cfg.Observability, cfg.Deployment, cfg.Autoscaler)
if err != nil {
t.Fatal("failed to create deployment")
}
Expand Down Expand Up @@ -779,5 +780,9 @@ func ReconcilerTestConfig() *config.Config {
Logging: &logging.Config{},
Tracing: &tracingconfig.Config{},
Defaults: &defaultconfig.Defaults{},
Autoscaler: &asconfig.Config{
InitialScale: 1,
AllowZeroInitialScale: false,
},
}
}