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

Allow setting Queue Proxy resources in ConfigMap #8195

Merged
merged 1 commit into from
Jun 9, 2020
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
28 changes: 27 additions & 1 deletion config/core/configmaps/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ metadata:
labels:
serving.knative.dev/release: devel
annotations:
knative.dev/example-checksum: 3d5b261a
knative.dev/example-checksum: 1de882eb
data:
# This is the Go import path for the binary that is containerized
# and substituted here.
Expand All @@ -47,3 +47,29 @@ data:
# ProgressDeadline is the duration we wait for the deployment to
# be ready before considering it failed.
progressDeadline: "120s"

# queueSidecarCPURequest is the requests.cpu to set for the queue proxy sidecar container.
# If omitted, a default value (currently "25m"), is used.
queueSidecarCPURequest: "25m"

# queueSidecarCPULimit is the limits.cpu to set for the queue proxy sidecar container.
# If omitted, no value is specified and the system default is used.
queueSidecarCPULimit: "1000m"

# queueSidecarMemoryRequest is the requests.memory to set for the queue proxy container.
# If omitted, no value is specified and the system default is used.
queueSidecarMemoryRequest: "400m"

# queueSidecarMemoryLimit is the limits.memory to set for the queue proxy container.
# If omitted, no value is specified and the system default is used.
queueSidecarMemoryLimit: "800m"

# queueSidecarEphemeralStorageRequest is the requests.ephemeral-storage to
# set for the queue proxy sidecar container.
# If omitted, no value is specified and the system default is used.
queueSidecarEphemeralStorageRequest: "512m"

# queueSidecarEphemeralStorageLimit is the limits.ephemeral-storage to set
# for the queue proxy sidecar container.
# If omitted, no value is specified and the system default is used.
queueSidecarEphemeralStorageLimit: "1024m"
52 changes: 50 additions & 2 deletions pkg/deployment/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"time"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/util/sets"

cm "knative.dev/pkg/configmap"
Expand All @@ -38,16 +39,36 @@ const (
// ProgressDeadlineSeconds. This does not match the K8s default value of 600s.
ProgressDeadlineDefault = 120 * time.Second

registriesSkippingTagResolvingKey = "registriesSkippingTagResolving"

// ProgressDeadlineKey is the key to configure deployment progress deadline.
ProgressDeadlineKey = "progressDeadline"

// registriesSkippingTagResolvingKey is the config map key for the set of registries
// (e.g. ko.local) where tags should not be resolved to digests.
registriesSkippingTagResolvingKey = "registriesSkippingTagResolving"

// queueSidecar resource request keys.
queueSidecarCPURequestKey = "queueSidecarCPURequest"
queueSidecarMemoryRequestKey = "queueSidecarMemoryRequest"
queueSidecarEphemeralStorageRequestKey = "queueSidecarEphemeralStorageRequest"

// queueSidecar resource limit keys.
queueSidecarCPULimitKey = "queueSidecarCPULimit"
queueSidecarMemoryLimitKey = "queueSidecarMemoryLimit"
queueSidecarEphemeralStorageLimitKey = "queueSidecarEphemeralStorageLimit"
)

var (
// QueueSidecarCPURequestDefault is the default request.cpu to set for the
// queue sidecar. It is set at 25m for backwards-compatibility since this was
// the historic default before the field was operator-settable.
QueueSidecarCPURequestDefault = resource.MustParse("25m")
julz marked this conversation as resolved.
Show resolved Hide resolved
)

func defaultConfig() *Config {
return &Config{
ProgressDeadline: ProgressDeadlineDefault,
RegistriesSkippingTagResolving: sets.NewString("ko.local", "dev.local"),
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to set all of them. Since that's what the CM above implies.

Copy link
Member Author

@julz julz Jun 5, 2020

Choose a reason for hiding this comment

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

The CM example doc explicitly says that all fields except this one (to avoid changing current defaults) default to unset (ie no limit). This is same behaviour - again other than cpu to avoid changing current default - as defaults.yaml has for user container resource defaults: if you don’t explicitly set, we default to no request/limit, but we still have examples of what valid values look like.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I thought for QP we do set them, vs what we do for UserContainer.

Copy link
Member Author

Choose a reason for hiding this comment

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

yah I think eventually it'd make sense to, but tbh I don't know what the correct values to default to would be for all workloads yet so I limited the initial feature proposal to letting operators set, but maintaining existing behaviour (i.e unset/inherit/infinite) if they don't. Hopefully we can experiment a bit and determine good defaults once this lands.

Copy link
Contributor

Choose a reason for hiding this comment

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

SG

}
}

Expand All @@ -59,6 +80,13 @@ func NewConfigFromMap(configMap map[string]string) (*Config, error) {
cm.AsString(QueueSidecarImageKey, &nc.QueueSidecarImage),
cm.AsDuration(ProgressDeadlineKey, &nc.ProgressDeadline),
cm.AsStringSet(registriesSkippingTagResolvingKey, &nc.RegistriesSkippingTagResolving),

cm.AsQuantity(queueSidecarCPURequestKey, &nc.QueueSidecarCPURequest),
cm.AsQuantity(queueSidecarMemoryRequestKey, &nc.QueueSidecarMemoryRequest),
cm.AsQuantity(queueSidecarEphemeralStorageRequestKey, &nc.QueueSidecarEphemeralStorageRequest),
cm.AsQuantity(queueSidecarCPULimitKey, &nc.QueueSidecarCPULimit),
cm.AsQuantity(queueSidecarMemoryLimitKey, &nc.QueueSidecarMemoryLimit),
cm.AsQuantity(queueSidecarEphemeralStorageLimitKey, &nc.QueueSidecarEphemeralStorageLimit),
); err != nil {
return nil, err
}
Expand Down Expand Up @@ -91,4 +119,24 @@ type Config struct {
// ProgressDeadline is the time in seconds we wait for the deployment to
// be ready before considering it failed.
ProgressDeadline time.Duration

// QueueSidecarCPURequest is the CPU Request to set for the queue proxy sidecar container
QueueSidecarCPURequest *resource.Quantity

// QueueSidecarCPULimit is the CPU Limit to set for the queue proxy sidecar container
QueueSidecarCPULimit *resource.Quantity

// QueueSidecarMemoryRequest is the Memory Request to set for the queue proxy sidecar container
QueueSidecarMemoryRequest *resource.Quantity

// QueueSidecarMemoryLimit is the Memory Limit to set for the queue proxy sidecar container
QueueSidecarMemoryLimit *resource.Quantity

// QueueSidecarEphemeralStorageRequest is the Ephemeral Storage Request to
// set for the queue proxy sidecar container
QueueSidecarEphemeralStorageRequest *resource.Quantity

// QueueSidecarEphemeralStorageLimit is the Ephemeral Storage Limit to set
// for the queue proxy sidecar container
QueueSidecarEphemeralStorageLimit *resource.Quantity
}
37 changes: 37 additions & 0 deletions pkg/deployment/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/google/go-cmp/cmp"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"

Expand All @@ -46,6 +47,13 @@ func TestControllerConfigurationFromFile(t *testing.T) {
want := defaultConfig()
// We require QSI to be explicitly set. So do it here.
want.QueueSidecarImage = "ko://knative.dev/serving/cmd/queue"

// The following are in the example yaml, to show usage,
// but default is nil, i.e. inheriting k8s.
// So for this test we ignore those, but verify the other fields.
got.QueueSidecarCPULimit = nil
got.QueueSidecarMemoryRequest, got.QueueSidecarMemoryLimit = nil, nil
got.QueueSidecarEphemeralStorageRequest, got.QueueSidecarEphemeralStorageLimit = nil, nil
Comment on lines +54 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we actually set those as defaults anyway?

Copy link
Member Author

@julz julz Jun 5, 2020

Choose a reason for hiding this comment

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

Maybe eventually, but for this PR the feature proposal specifically says we won’t change current defaults (which are empty ie inherit k8s/no limit), we’ll just let operators override that if they want.

if !cmp.Equal(got, want) {
t.Error("Example stanza does not match default, diff(-want,+got):", cmp.Diff(want, got))
}
Expand All @@ -63,6 +71,7 @@ func TestControllerConfiguration(t *testing.T) {
wantConfig: &Config{
RegistriesSkippingTagResolving: sets.NewString("ko.local", ""),
QueueSidecarImage: defaultSidecarImage,
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
ProgressDeadline: ProgressDeadlineDefault,
},
data: map[string]string{
Expand All @@ -74,6 +83,7 @@ func TestControllerConfiguration(t *testing.T) {
wantConfig: &Config{
RegistriesSkippingTagResolving: sets.NewString("ko.local", "dev.local"),
QueueSidecarImage: defaultSidecarImage,
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
ProgressDeadline: 444 * time.Second,
},
data: map[string]string{
Expand All @@ -85,12 +95,35 @@ func TestControllerConfiguration(t *testing.T) {
wantConfig: &Config{
RegistriesSkippingTagResolving: sets.NewString("ko.local", "ko.dev"),
QueueSidecarImage: defaultSidecarImage,
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
ProgressDeadline: ProgressDeadlineDefault,
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
registriesSkippingTagResolvingKey: "ko.local,ko.dev",
},
}, {
name: "controller configuration with custom queue sidecar resource request/limits",
wantConfig: &Config{
RegistriesSkippingTagResolving: sets.NewString("ko.local", "dev.local"),
QueueSidecarImage: defaultSidecarImage,
ProgressDeadline: ProgressDeadlineDefault,
QueueSidecarCPURequest: resourcePtr(resource.MustParse("123m")),
QueueSidecarMemoryRequest: resourcePtr(resource.MustParse("456M")),
QueueSidecarEphemeralStorageRequest: resourcePtr(resource.MustParse("789m")),
QueueSidecarCPULimit: resourcePtr(resource.MustParse("987M")),
QueueSidecarMemoryLimit: resourcePtr(resource.MustParse("654m")),
QueueSidecarEphemeralStorageLimit: resourcePtr(resource.MustParse("321M")),
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
queueSidecarCPURequestKey: "123m",
queueSidecarMemoryRequestKey: "456M",
queueSidecarEphemeralStorageRequestKey: "789m",
queueSidecarCPULimitKey: "987M",
queueSidecarMemoryLimitKey: "654m",
queueSidecarEphemeralStorageLimitKey: "321M",
},
}, {
name: "controller with no side car image",
wantErr: true,
Expand Down Expand Up @@ -146,3 +179,7 @@ func TestControllerConfiguration(t *testing.T) {
})
}
}

func resourcePtr(q resource.Quantity) *resource.Quantity {
return &q
}
30 changes: 30 additions & 0 deletions pkg/deployment/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/reconciler/revision/resources/deploy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ var (

defaultQueueContainer = &corev1.Container{
Name: QueueContainerName,
Resources: createQueueResources(make(map[string]string), &corev1.Container{}),
Resources: createQueueResources(&deploymentConfig, make(map[string]string), &corev1.Container{}),
Ports: append(queueNonServingPorts, queueHTTPPort),
ReadinessProbe: &corev1.Probe{
Handler: corev1.Handler{
Expand Down
32 changes: 29 additions & 3 deletions pkg/reconciler/revision/resources/queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,35 @@ var (
}
)

func createQueueResources(annotations map[string]string, userContainer *corev1.Container) corev1.ResourceRequirements {
resourceRequests := corev1.ResourceList{corev1.ResourceCPU: queueContainerCPU}
func createQueueResources(cfg *deployment.Config, annotations map[string]string, userContainer *corev1.Container) corev1.ResourceRequirements {
resourceRequests := corev1.ResourceList{}
resourceLimits := corev1.ResourceList{}

for _, r := range []struct {
Name corev1.ResourceName
Request *resource.Quantity
Limit *resource.Quantity
}{{
Name: corev1.ResourceCPU,
Request: cfg.QueueSidecarCPURequest,
Limit: cfg.QueueSidecarCPULimit,
}, {
Name: corev1.ResourceMemory,
Request: cfg.QueueSidecarMemoryRequest,
Limit: cfg.QueueSidecarMemoryLimit,
}, {
Name: corev1.ResourceEphemeralStorage,
Request: cfg.QueueSidecarEphemeralStorageRequest,
Limit: cfg.QueueSidecarEphemeralStorageLimit,
}} {
if r.Request != nil {
resourceRequests[r.Name] = *r.Request
}
if r.Limit != nil {
resourceLimits[r.Name] = *r.Limit
}
}

var requestCPU, limitCPU, requestMemory, limitMemory resource.Quantity

if resourceFraction, ok := fractionFromPercentage(annotations, serving.QueueSideCarResourcePercentageAnnotation); ok {
Expand Down Expand Up @@ -246,7 +272,7 @@ func makeQueueContainer(rev *v1.Revision, loggingConfig *logging.Config, tracing
return &corev1.Container{
Name: QueueContainerName,
Image: deploymentConfig.QueueSidecarImage,
Resources: createQueueResources(rev.GetAnnotations(), container),
Resources: createQueueResources(deploymentConfig, rev.GetAnnotations(), container),
Ports: ports,
ReadinessProbe: makeQueueProbe(rp),
VolumeMounts: volumeMounts,
Expand Down
Loading