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

feat: add defaults configurable runtimeClassName #15271

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
17 changes: 16 additions & 1 deletion config/core/configmaps/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ metadata:
app.kubernetes.io/component: controller
app.kubernetes.io/version: devel
annotations:
knative.dev/example-checksum: "e2f637c6"
knative.dev/example-checksum: "720ddb97"
data:
# This is the Go import path for the binary that is containerized
# and substituted here.
Expand Down Expand Up @@ -108,3 +108,18 @@ data:
# `
# This may be "none" or "prefer-spread-revision-over-nodes" (default)
# default-affinity-type: "prefer-spread-revision-over-nodes"

# runtime-class-name contains the selector for which runtimeClassName
# is selected to put in a revision.
# By default, it is not set by Knative.
#
# Example:
# runtime-class-name: |
# "":
# selector:
# use-default-runc: "yes"
# kata: {}
# gvisor:
# selector:
# use-gvisor: "please"
runtime-class-name: ""
69 changes: 69 additions & 0 deletions pkg/deployment/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,18 @@ package deployment
import (
"errors"
"fmt"
"strings"
"time"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/sets"
"sigs.k8s.io/yaml"

cm "knative.dev/pkg/configmap"
"knative.dev/pkg/ptr"
)

const (
Expand Down Expand Up @@ -70,6 +76,8 @@ const (

defaultAffinityTypeKey = "default-affinity-type"
defaultAffinityTypeValue = PreferSpreadRevisionOverNodes

RuntimeClassNameKey = "runtime-class-name"
)

var (
Expand Down Expand Up @@ -116,10 +124,53 @@ func defaultConfig() *Config {
return cfg
}

func (d Config) PodRuntimeClassName(lbs map[string]string) *string {
runtimeClassName := ""
specificity := -1
for k, v := range d.RuntimeClassNames {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create a func for calculating specificity and also unify types: LabelSelector (domain) and RuntimeClassNameLabelSelector to avoid duplication?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say do it in a follow up

Copy link
Member Author

Choose a reason for hiding this comment

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

@skonto, apologies since I missed this message. Agree, that would be helpful!

if !v.Matches(lbs) || v.specificity() < specificity {
continue
}
if v.specificity() > specificity || strings.Compare(k, runtimeClassName) < 0 {
Copy link
Contributor

@skonto skonto Jun 14, 2024

Choose a reason for hiding this comment

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

Is string comparison a rule to select a class when they have equal specificity?
Looks a bit ad hoc imho. Btw could you add a test for this and some scenario with multiple runtime classes?

Copy link
Member Author

Choose a reason for hiding this comment

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

This implementation comes from the existing domain selection code for a Service

func NewDomainFromConfigMap(configMap *corev1.ConfigMap) (*Domain, error) {
c := Domain{Domains: map[string]DomainConfig{}}
hasDefault := false
for k, v := range configMap.Data {
if k == configmap.ExampleKey {
continue
}
internalConfig := domainInternalConfig{}
err := yaml.Unmarshal([]byte(v), &internalConfig)
if err != nil {
return nil, err
}
if len(internalConfig.Selector) == 0 {
hasDefault = true
internalConfig.Type = DomainTypeWildcard
}
c.Domains[k] = DomainConfig{
Selector: &LabelSelector{Selector: internalConfig.Selector},
Type: internalConfig.Type,
}
}
if !hasDefault {
c.Domains[DefaultDomain] = DomainConfig{Selector: &LabelSelector{}, Type: DomainTypeWildcard}
}
return &c, nil
}
// LookupDomainForLabels returns a domain given a set of labels.
// Since we reject configuration without a default domain, this should
// always return a value.
func (c *Domain) LookupDomainForLabels(labels map[string]string) string {
domain := ""
specificity := -1
// If we see VisibilityLabelKey sets with VisibilityClusterLocal, that
// will take precedence and the route will get a Cluster's Domain Name.
if labels[networking.VisibilityLabelKey] == serving.VisibilityClusterLocal {
return "svc." + network.GetClusterDomainName()
}
for k, v := range c.Domains {
// Ignore if selector doesn't match, or decrease the specificity.
if !v.Selector.Matches(labels) || v.Selector.specificity() < specificity {
continue
}
if v.Selector.specificity() > specificity || strings.Compare(k, domain) < 0 {
domain = k
specificity = v.Selector.specificity()
}
}
return domain
}
.

TLDR; the one with most matching labels wins.

Good thinking. I've added two tests for this behaviour in 4adbced#diff-2a14f9c49eb40c3a55958c8e2d57523cd618232821a9d310c1ddd7cc64d91ef9R476-R517

Copy link
Contributor

Choose a reason for hiding this comment

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

TLDR; the one with most matching labels wins.

Also a classname can't have a label (key, value) that does not exist in the service labels.

On top of that, if you have two class names that have the same specificity, lexicographic order in class names is what matters:
("three" < "two"):

{
		name: "priority with multiple label selectors and three labels set",
		serviceLabels: map[string]string{
			"needs-two":   "yes",
			"needs-three": "yes",
			"needs-four":  "yes",
		},
		runtimeClassNames: map[string]RuntimeClassNameLabelSelector{
			"one": {},
			"two": {
				Selector: map[string]string{
					"needs-two":   "yes",
					"needs-three": "yes",
				},
			},
			"three": {
				Selector: map[string]string{
					"needs-two":   "yes",
					"needs-three": "yes",
				},
			},
		},
		want: ptr.String("three"),
	},

Copy link
Member Author

Choose a reason for hiding this comment

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

Also a classname can't have a label (key, value) that does not exist in the service labels.

I'm not sure I understand. Are you meaning to validate a selector against the labels of Services that exist?
If so, that wouldn't make a lot of sense to me.

On top of that, if you have two class names that have the same specificity, lexicographic order in class names is what matters:
("three" < "two"):

Yes that is true, but I would think an uncommon practical issue.

Do you have any alternative patterns for selected which I may consider?

Copy link
Contributor

@skonto skonto Jun 18, 2024

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 understand.

		serviceLabels: map[string]string{
			"needs-two":   "yes",
			"needs-three": "yes",
			"needs-four":  "yes",
		},
		runtimeClassNames: map[string]RuntimeClassNameLabelSelector{
			"one": {},
			"two": {
				Selector: map[string]string{
					"needs-two":   "yes",
					"needs-five": "yes",
				},
			},

two is ignored due to the v.MatchLabels condition. Matches has this loop that checks against the service labels:

	for label, expectedValue := range s.Selector {
		value, ok := labels[label]
		if !ok || expectedValue != value {
			return false
		}
	}

I am trying to summarize the key points for the logic. It is not just "the one with most matching labels wins."

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any alternative patterns for selected which I may consider?

I am ok as long as we document it.

Copy link
Member Author

Choose a reason for hiding this comment

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

That appears to be correct behaviour, the service labels in the above sample don't include "need-five".

Copy link
Member Author

Choose a reason for hiding this comment

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

runtimeClassName = k
specificity = v.specificity()
}
}
if runtimeClassName == "" {
return nil
}
return ptr.String(runtimeClassName)
Copy link
Contributor

@skonto skonto Jun 14, 2024

Choose a reason for hiding this comment

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

Do we want to validate with https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L314 and fail fast?
For example right now random names pass eg. "runtime-class-name": "_ : {}", although they should print:

error : [a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')]

Tested with

{
		name:    "runtime class name should fail",
		wantErr: false,
		data: map[string]string{
			QueueSidecarImageKey: defaultSidecarImage,
			"runtime-class-name": "_ : {}",
		},
		wantConfig: &Config{
			DigestResolutionTimeout:        digestResolutionTimeoutDefault,
			ProgressDeadline:               ProgressDeadlineDefault,
			QueueSidecarCPURequest:         &QueueSidecarCPURequestDefault,
			QueueSidecarImage:              defaultSidecarImage,
			QueueSidecarTokenAudiences:     sets.New(""),
			RegistriesSkippingTagResolving: sets.New("kind.local", "ko.local", "dev.local"),
			RuntimeClassNames:              map[string]RuntimeClassNameLabelSelector{"_": RuntimeClassNameLabelSelector{}},
			DefaultAffinityType:            defaultAffinityTypeValue,
		},
	},

in config_test.

Copy link
Member Author

@BobyMCbobs BobyMCbobs Jun 15, 2024

Choose a reason for hiding this comment

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

@skonto, thank you for your thoughts. More validation is essential and will make it more robust.

I've tried adding in

+			if err := apivalidation.ValidateRuntimeClassName(class, field.NewPath(".data."+RuntimeClassNameKey)); err != nil {
+				return nil, fmt.Errorf("%v invalid: %v", RuntimeClassNameKey, err)
+			}

but now have an import failure. Haven't gotten one of these errors in a bit

$ go mod tidy
go: finding module for package k8s.io/kubernetes/pkg/apis/core/validation
go: found k8s.io/kubernetes/pkg/apis/core/validation in k8s.io/kubernetes v1.30.2
go: downloading k8s.io/component-helpers v0.0.0
go: downloading k8s.io/kubelet v0.0.0
go: downloading k8s.io/cloud-provider v0.0.0
go: finding module for package k8s.io/client-go/features
go: found k8s.io/client-go/features in k8s.io/client-go v0.30.2
go: knative.dev/serving/pkg/deployment imports
        k8s.io/kubernetes/pkg/apis/core/validation imports
        k8s.io/component-helpers/node/util/sysctl: reading k8s.io/component-helpers/go.mod at revision v0.0.0: unknown revision v0.0.0
go: knative.dev/serving/pkg/deployment imports
        k8s.io/kubernetes/pkg/apis/core/validation imports
        k8s.io/component-helpers/scheduling/corev1: reading k8s.io/component-helpers/go.mod at revision v0.0.0: unknown revision v0.0.0
go: knative.dev/serving/pkg/deployment imports
        k8s.io/kubernetes/pkg/apis/core/validation imports
        k8s.io/kubelet/pkg/apis: reading k8s.io/kubelet/go.mod at revision v0.0.0: unknown revision v0.0.0
go: knative.dev/serving/pkg/deployment imports
        k8s.io/kubernetes/pkg/apis/core/validation imports
        k8s.io/kubernetes/pkg/cluster/ports imports
        k8s.io/cloud-provider/options: reading k8s.io/cloud-provider/go.mod at revision v0.0.0: unknown revision v0.0.0

from adding the dependencies

	"k8s.io/apimachinery/pkg/util/validation/field"
	apivalidation "k8s.io/kubernetes/pkg/apis/core/validation"

I suspect it is something with version pinning and will continue investigating further.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can just import apimachineryvalidation.NameIsDNSSubdomain(name, false) and it should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated in 4adbced

}

type RuntimeClassNameLabelSelector struct {
Selector map[string]string `json:"selector,omitempty"`
}

func (s *RuntimeClassNameLabelSelector) specificity() int {
if s.Selector == nil {
return 0
}
return len(s.Selector)
}

func (s *RuntimeClassNameLabelSelector) Matches(labels map[string]string) bool {
if s.Selector == nil {
return true
}
for label, expectedValue := range s.Selector {
value, ok := labels[label]
if !ok || expectedValue != value {
return false
}
}
return true
}

// NewConfigFromMap creates a DeploymentConfig from the supplied Map.
func NewConfigFromMap(configMap map[string]string) (*Config, error) {
nc := defaultConfig()

var runtimeClassNames string
if err := cm.Parse(configMap,
// Legacy keys for backwards compatibility
cm.AsString(DeprecatedQueueSidecarImageKey, &nc.QueueSidecarImage),
Expand Down Expand Up @@ -147,6 +198,8 @@ func NewConfigFromMap(configMap map[string]string) (*Config, error) {

cm.AsStringSet(queueSidecarTokenAudiencesKey, &nc.QueueSidecarTokenAudiences),
cm.AsString(queueSidecarRooCAKey, &nc.QueueSidecarRootCA),

cm.AsString(RuntimeClassNameKey, &runtimeClassNames),
); err != nil {
return nil, err
}
Expand Down Expand Up @@ -175,6 +228,19 @@ func NewConfigFromMap(configMap map[string]string) (*Config, error) {
return nil, fmt.Errorf("unsupported %s value: %q", defaultAffinityTypeKey, affinity)
}
}
if err := yaml.Unmarshal([]byte(runtimeClassNames), &nc.RuntimeClassNames); err != nil {
return nil, fmt.Errorf("%v cannot be parsed, please check the format: %w", RuntimeClassNameKey, err)
}
for class, rcn := range nc.RuntimeClassNames {
if warns := apimachineryvalidation.NameIsDNSSubdomain(class, false); len(warns) > 0 {
return nil, fmt.Errorf("%v %v selector not valid DNSSubdomain: %v", RuntimeClassNameKey, class, warns)
}
if len(rcn.Selector) > 0 {
if _, err := labels.ValidatedSelectorFromSet(rcn.Selector); err != nil {
return nil, fmt.Errorf("%v %v selector invalid: %w", RuntimeClassNameKey, class, err)
}
}
}
return nc, nil
}

Expand Down Expand Up @@ -240,4 +306,7 @@ type Config struct {
// DefaultAffinityType is a string that controls what affinity rules will be automatically
// applied to the PodSpec of all Knative services.
DefaultAffinityType AffinityType

// RuntimeClassNames specifies which runtime the Pod will use
RuntimeClassNames map[string]RuntimeClassNameLabelSelector
}
Loading
Loading