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

Jump to bottom

Add affinity-rules feature to configmap config-deployment #15250

Merged
merged 23 commits into from
Jun 6, 2024
Merged

Add affinity-rules feature to configmap config-deployment #15250

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
ee19514
add affinity-rules to config-deployment configmap
izabelacg May 24, 2024
3b9f7aa
run ./hack/update-codegen.sh
izabelacg May 27, 2024
dd0d599
change affinity rules property to be a flag
izabelacg May 27, 2024
0d0bdc7
run ./hack/update-codegen.sh
izabelacg May 27, 2024
4d73f10
add default pod anti-affinity rules to PodSpec
izabelacg May 27, 2024
92f0ab5
re-arrange imports
izabelacg May 27, 2024
a633eba
enable pod anti affinity by default
izabelacg May 27, 2024
7d0f070
fix value in config-deployment
izabelacg May 28, 2024
cc096da
fix condition for adding pod anti-affinity based on presence of a label
izabelacg May 28, 2024
bad694c
run ./hack/update-codegen.sh
izabelacg May 28, 2024
e56c559
clean up deploy tests
izabelacg May 28, 2024
9ef849b
change property name
izabelacg May 28, 2024
8daaa67
enable pod anti affinity by default
izabelacg May 29, 2024
a2610e4
update deployment.yaml
izabelacg May 29, 2024
905443c
adds new default for enable pod anti affinity to existing tests
izabelacg May 29, 2024
def1a18
change affinity type from toggle to string
izabelacg Jun 3, 2024
da265ce
run ./hack/update-codegen.sh
izabelacg Jun 3, 2024
1e5f200
fix condition to apply podspec
izabelacg Jun 3, 2024
3470332
tweak when applying the defaults
izabelacg Jun 5, 2024
376cd87
simplify condition that apply affinity defaults
izabelacg Jun 5, 2024
1f4fde2
rename new field to default-affinity-type
izabelacg Jun 5, 2024
7dec43d
replace usage of old name affinity
izabelacg Jun 5, 2024
6c9252f
rename test cases
izabelacg Jun 5, 2024
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
19 changes: 18 additions & 1 deletion config/core/configmaps/deployment.yaml
View file
Open in desktop
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: "ed77183a"
knative.dev/example-checksum: "593aef61"
data:
# This is the Go import path for the binary that is containerized
# and substituted here.
Expand Down Expand Up @@ -91,3 +91,20 @@ data:
# Sets rootCA for the queue proxy - used by QPOptions
# If omitted, or empty, no rootCA is added to the golang rootCAs
queue-sidecar-rootca: ""

# If set, it automatically configures pod anti-affinity requirements for all Knative services.
# It employs the `preferredDuringSchedulingIgnoredDuringExecution` weighted pod affinity term,
# aligning with the Knative revision label. It yields the configuration below in all workloads' deployments:
# `
# affinity:
# podAntiAffinity:
# preferredDuringSchedulingIgnoredDuringExecution:
# - podAffinityTerm:
# topologyKey: kubernetes.io/hostname
# labelSelector:
# matchLabels:
# serving.knative.dev/revision: {{revision-name}}
# weight: 100
# `
# This may be "none" or "prefer-spread-revision-over-nodes" (default)
# affinity: "prefer-spread-revision-over-nodes"
Copy link
Contributor

@skonto skonto Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That is fine as a starting point as this is probably most folks would use. I am wondering about our defaulting strategy though, as we don't allow the user to set up as default config whatever is needed each time. For example I can imagine folks asking "required-spread-revision-over-nodes" (assuming they dont want more than one pods on the same node) or adjust the topology key (topology.kubernetes.io/zone).

Copy link
Member

Choose a reason for hiding this comment

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

I can imagine folks asking "required-spread-revision-over-nodes" (assuming they dont want more than one pods on the same node) or adjust the topology key (topology.kubernetes.io/zone).

We can add more types in the future and even a custom option that allows the operator to specify a template maybe

I am wondering about our defaulting strategy though, as we don't allow the user to set up as default config whatever is needed each time.

I don't understand what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is about defaulting but not with all options available upfront. Anyway we can add later for sure.

29 changes: 28 additions & 1 deletion pkg/deployment/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/util/sets"

cm "knative.dev/pkg/configmap"
)

Expand Down Expand Up @@ -68,6 +67,10 @@ const (
// qpoptions
queueSidecarTokenAudiencesKey = "queue-sidecar-token-audiences"
queueSidecarRooCAKey = "queue-sidecar-rootca"

affinityKey = "affinity"

affinityDefault = PreferSpreadRevisionOverNodes
)

var (
Expand Down Expand Up @@ -103,6 +106,7 @@ func defaultConfig() *Config {
DigestResolutionTimeout: digestResolutionTimeoutDefault,
RegistriesSkippingTagResolving: sets.New("kind.local", "ko.local", "dev.local"),
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
Affinity: affinityDefault,
}
// The following code is needed for ConfigMap testing.
// defaultConfig must match the example in deployment.yaml which includes: `queue-sidecar-token-audiences: ""`
Expand Down Expand Up @@ -164,6 +168,14 @@ func NewConfigFromMap(configMap map[string]string) (*Config, error) {
return nil, fmt.Errorf("digest-resolution-timeout cannot be a non-positive duration, was %v", nc.DigestResolutionTimeout)
}

if affinity, ok := configMap[affinityKey]; ok {
switch opt := AffinityRule(affinity); opt {
case None, PreferSpreadRevisionOverNodes:
nc.Affinity = opt
default:
return nil, fmt.Errorf("unsupported `affinity` value %q", affinity)
}
}
return nc, nil
}

Expand All @@ -172,6 +184,17 @@ func NewConfigFromConfigMap(config *corev1.ConfigMap) (*Config, error) {
return NewConfigFromMap(config.Data)
}

// AffinityRule specifies which affinity requirements will be automatically applied to the PodSpec of all Knative services.
type AffinityRule string

const (
// None is used for deactivating affinity configuration for user workloads.
None AffinityRule = "none"

// PreferSpreadRevisionOverNodes is used to set pod anti-affinity requirements for user workloads.
PreferSpreadRevisionOverNodes AffinityRule = "prefer-spread-revision-over-nodes"
)

// Config includes the configurations for the controller.
type Config struct {
// QueueSidecarImage is the name of the image used for the queue sidecar
Expand Down Expand Up @@ -214,4 +237,8 @@ type Config struct {

// QueueSidecarRootCA is a root certificate to be trusted by the queue proxy sidecar qpoptions.
QueueSidecarRootCA string

// Affinity is a string that controls what affinity rules will be automatically
// applied to the PodSpec of all Knative services.
Affinity AffinityRule
}
65 changes: 65 additions & 0 deletions pkg/deployment/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,64 @@ func TestControllerConfiguration(t *testing.T) {
wantConfig *Config
data map[string]string
}{{
name: "controller configuration with no affinity rule specified",
wantConfig: &Config{
RegistriesSkippingTagResolving: sets.New("kind.local", "ko.local", "dev.local"),
DigestResolutionTimeout: digestResolutionTimeoutDefault,
QueueSidecarImage: defaultSidecarImage,
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
QueueSidecarTokenAudiences: sets.New(""),
ProgressDeadline: ProgressDeadlineDefault,
Affinity: affinityDefault,
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
},
}, {
name: "controller configuration with empty string for the affinity rule",
wantErr: true,
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
affinityKey: "",
},
}, {
name: "controller configuration with unsupported affinity value",
wantErr: true,
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
affinityKey: "coconut",
},
}, {
name: "controller configuration with the default affinity rule set",
wantConfig: &Config{
RegistriesSkippingTagResolving: sets.New("kind.local", "ko.local", "dev.local"),
DigestResolutionTimeout: digestResolutionTimeoutDefault,
QueueSidecarImage: defaultSidecarImage,
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
QueueSidecarTokenAudiences: sets.New(""),
ProgressDeadline: ProgressDeadlineDefault,
Affinity: affinityDefault,
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
affinityKey: string(PreferSpreadRevisionOverNodes),
},
}, {
name: "controller configuration with affinity deactivated",
wantConfig: &Config{
RegistriesSkippingTagResolving: sets.New("kind.local", "ko.local", "dev.local"),
DigestResolutionTimeout: digestResolutionTimeoutDefault,
QueueSidecarImage: defaultSidecarImage,
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
QueueSidecarTokenAudiences: sets.New(""),
ProgressDeadline: ProgressDeadlineDefault,
Affinity: None,
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
affinityKey: string(None),
},
}, {
name: "controller configuration with bad registries",
wantConfig: &Config{
RegistriesSkippingTagResolving: sets.New("ko.local", ""),
Expand All @@ -89,6 +147,7 @@ func TestControllerConfiguration(t *testing.T) {
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
QueueSidecarTokenAudiences: sets.New("foo", "bar", "boo-srv"),
ProgressDeadline: ProgressDeadlineDefault,
Affinity: affinityDefault,
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
Expand All @@ -104,6 +163,7 @@ func TestControllerConfiguration(t *testing.T) {
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
QueueSidecarTokenAudiences: sets.New(""),
ProgressDeadline: 444 * time.Second,
Affinity: affinityDefault,
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
Expand All @@ -118,6 +178,7 @@ func TestControllerConfiguration(t *testing.T) {
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
QueueSidecarTokenAudiences: sets.New(""),
ProgressDeadline: ProgressDeadlineDefault,
Affinity: affinityDefault,
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
Expand All @@ -132,6 +193,7 @@ func TestControllerConfiguration(t *testing.T) {
QueueSidecarCPURequest: &QueueSidecarCPURequestDefault,
QueueSidecarTokenAudiences: sets.New(""),
ProgressDeadline: ProgressDeadlineDefault,
Affinity: affinityDefault,
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
Expand All @@ -151,6 +213,7 @@ func TestControllerConfiguration(t *testing.T) {
QueueSidecarMemoryLimit: quantity("654m"),
QueueSidecarEphemeralStorageLimit: quantity("321M"),
QueueSidecarTokenAudiences: sets.New(""),
Affinity: affinityDefault,
},
data: map[string]string{
QueueSidecarImageKey: defaultSidecarImage,
Expand Down Expand Up @@ -227,6 +290,7 @@ func TestControllerConfiguration(t *testing.T) {
QueueSidecarEphemeralStorageRequest: quantity("9M"),
QueueSidecarEphemeralStorageLimit: quantity("10M"),
QueueSidecarTokenAudiences: sets.New(""),
Affinity: affinityDefault,
},
}, {
name: "newer key case takes priority",
Expand Down Expand Up @@ -268,6 +332,7 @@ func TestControllerConfiguration(t *testing.T) {
QueueSidecarEphemeralStorageRequest: quantity("20M"),
QueueSidecarEphemeralStorageLimit: quantity("21M"),
QueueSidecarTokenAudiences: sets.New("foo"),
Affinity: affinityDefault,
},
}}

Expand Down
21 changes: 21 additions & 0 deletions pkg/reconciler/revision/resources/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
"k8s.io/apimachinery/pkg/util/intstr"

apiconfig "knative.dev/serving/pkg/apis/config"
deploymentconfig "knative.dev/serving/pkg/deployment"
)

const certVolumeName = "server-certs"
Expand Down Expand Up @@ -150,6 +151,22 @@ func rewriteUserLivenessProbe(p *corev1.Probe, userPort int) {
}
}

func makePreferSpreadRevisionOverNodes(revisionLabelValue string) *corev1.PodAntiAffinity {
return &corev1.PodAntiAffinity{
PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{{
Weight: 100,
PodAffinityTerm: corev1.PodAffinityTerm{
TopologyKey: corev1.LabelHostname,
LabelSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{
serving.RevisionLabelKey: revisionLabelValue,
},
},
},
}},
}
}

func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error) {
queueContainer, err := makeQueueContainer(rev, cfg)
tokenVolume := varTokenVolume.DeepCopy()
Expand Down Expand Up @@ -210,6 +227,10 @@ func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error)
}
}

if cfg.Deployment.Affinity == deploymentconfig.PreferSpreadRevisionOverNodes && cfg.Features.PodSpecAffinity == apiconfig.Disabled {
Copy link
Member

Choose a reason for hiding this comment

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

The way I view the PodSpecAffinity feature it's the operator letting users set the affnity themselves.

I still think if that feature is enabled and the user hasn't set the affinity then we should still use the default set by the operator.

Copy link
Member Author

@izabelacg izabelacg Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, ok. I misunderstood then. I'll take a look at the code again and adjust it accordingly

Copy link
Member

Choose a reason for hiding this comment

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

Hm. IMHO, isn't the current config variation seems a bit unintuitive:

  • feature: enabled = we don't set it
  • feature: disabled + manually set to none = we don't set it
  • feature: disabled + not manually disabled = we set it

Could we rename the flag to "defaultAffinity" and apply it, regardless of the feature config is enabled/disabled (so that is clear that we apply this by default if not specified otherwise). The user could then still enable the feature flag and override it on a service basis.

Copy link
Member

Choose a reason for hiding this comment

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

Could we rename the flag to "defaultAffinity" and apply it

Changing it to defaultAffinity makes sense - maybe defaultAffinityType since we're not setting the affinity but the type we want.

regardless of the feature config is enabled/disabled (so that is clear that we apply this by default if not specified otherwise). The user could then still enable the feature flag and override it on a service basis.

Thats my intent and what I'm asking for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed the config to defaultAffinityType

podSpec.Affinity = &corev1.Affinity{PodAntiAffinity: makePreferSpreadRevisionOverNodes(rev.Name)}
}

return podSpec, nil
}

Expand Down
97 changes: 97 additions & 0 deletions pkg/reconciler/revision/resources/deploy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,20 @@ var (
EnableServiceLinks: ptr.Bool(false),
}

defaultPodAntiAffinityRules = &corev1.PodAntiAffinity{
PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{{
Weight: 100,
PodAffinityTerm: corev1.PodAffinityTerm{
TopologyKey: "kubernetes.io/hostname",
LabelSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"serving.knative.dev/revision": "bar",
},
},
},
}},
}

maxUnavailable = intstr.FromInt(0)
defaultDeployment = &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -1409,6 +1423,89 @@ func TestMakePodSpec(t *testing.T) {
withEnvVar("SERVING_READINESS_PROBE", `[{"httpGet":{"path":"/","port":8080,"host":"127.0.0.1","scheme":"HTTP"}},{"httpGet":{"path":"/","port":8090,"host":"127.0.0.1","scheme":"HTTP"}}]`),
),
}),
}, {
name: "with default affinity rules",
rev: revision("bar", "foo",
withContainers([]corev1.Container{{
Name: servingContainerName,
Image: "busybox",
ReadinessProbe: withTCPReadinessProbe(v1.DefaultUserPort),
}}),
WithContainerStatuses([]v1.ContainerStatus{{
ImageDigest: "busybox@sha256:deadbeef",
}}),
),
fc: apicfg.Features{
PodSpecAffinity: apicfg.Disabled,
},
dc: deployment.Config{
Affinity: deployment.PreferSpreadRevisionOverNodes,
},
want: podSpec(
[]corev1.Container{
servingContainer(func(container *corev1.Container) {
container.Image = "busybox@sha256:deadbeef"
}),
queueContainer(),
},
func(p *corev1.PodSpec) {
p.Affinity = &corev1.Affinity{
PodAntiAffinity: defaultPodAntiAffinityRules,
}
},
),
}, {
name: "with affinity rules deactivated",
rev: revision("bar", "foo",
withContainers([]corev1.Container{{
Name: servingContainerName,
Image: "busybox",
ReadinessProbe: withTCPReadinessProbe(v1.DefaultUserPort),
}}),
WithContainerStatuses([]v1.ContainerStatus{{
ImageDigest: "busybox@sha256:deadbeef",
}}),
),
fc: apicfg.Features{
PodSpecAffinity: apicfg.Disabled,
},
dc: deployment.Config{
Affinity: deployment.None,
},
want: podSpec(
[]corev1.Container{
servingContainer(func(container *corev1.Container) {
container.Image = "busybox@sha256:deadbeef"
}),
queueContainer(),
},
),
}, {
name: "with affinity rules on for both users and operators",
rev: revision("bar", "foo",
withContainers([]corev1.Container{{
Name: servingContainerName,
Image: "busybox",
ReadinessProbe: withTCPReadinessProbe(v1.DefaultUserPort),
}}),
WithContainerStatuses([]v1.ContainerStatus{{
ImageDigest: "busybox@sha256:deadbeef",
}}),
),
fc: apicfg.Features{
PodSpecAffinity: apicfg.Enabled,
},
dc: deployment.Config{
Affinity: deployment.PreferSpreadRevisionOverNodes,
},
want: podSpec(
[]corev1.Container{
servingContainer(func(container *corev1.Container) {
container.Image = "busybox@sha256:deadbeef"
}),
queueContainer(),
},
),
}}

for _, test := range tests {
Expand Down
Loading