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

izabelacg · 2024-05-24T19:06:42Z

Proposed Changes

Add new field default-affinity-type to the config-deployment configmap that allows operators to set pod anti-affinity rules to all Knative services. The following pod anti-affinity rule get then added automatically in the deployments spec:

affinity:
  podAntiAffinity:
    preferredDuringSchedulingIgnoredDuringExecution:
    - podAffinityTerm:
        topologyKey: kubernetes.io/hostname
        labelSelector:
          matchLabels:
            serving.knative.dev/revision: {{revision-name}}
      weight: 100

the affinity type is set to prefer-spread-revision-over-nodes by default

Release Note

Pod anti-affinity rules are set by default for all Knative services. The feature can be deactivated using the property `default-affinity-type` in the config-deployment configmap.

codecov · 2024-05-27T17:16:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.79%. Comparing base (ab5c1a0) to head (6c9252f).
Report is 31 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15250      +/-   ##
==========================================
+ Coverage   84.73%   84.79%   +0.06%     
==========================================
  Files         218      218              
  Lines       13479    13504      +25     
==========================================
+ Hits        11421    11451      +30     
+ Misses       1689     1687       -2     
+ Partials      369      366       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

izabelacg · 2024-05-27T20:33:06Z

/retest

izabelacg · 2024-05-27T20:56:25Z

@dprotaso can you take a look? Is there any other testing we may want to run before I take the WIP off from the MR?

izabelacg · 2024-05-27T20:56:31Z

/retest

pkg/reconciler/revision/resources/deploy.go

izabelacg · 2024-05-29T17:36:33Z

/retest all

knative-prow · 2024-05-29T17:36:35Z

@izabelacg: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test build-tests
/test contour-latest
/test contour-tls
/test gateway-api-latest
/test istio-latest-no-mesh
/test istio-latest-no-mesh-tls
/test kourier-stable
/test kourier-stable-tls
/test unit-tests
/test upgrade-tests

The following commands are available to trigger optional jobs:

/test gateway-api-latest-and-contour
/test https
/test istio-latest-mesh
/test istio-latest-mesh-short
/test istio-latest-mesh-tls
/test performance-tests

Use /test all to run the following jobs that were automatically triggered:

build-tests_serving_main
istio-latest-no-mesh-tls_serving_main
istio-latest-no-mesh_serving_main
unit-tests_serving_main
upgrade-tests_serving_main

In response to this:

/retest all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

izabelacg · 2024-05-29T17:36:50Z

/test all

izabelacg · 2024-05-29T17:39:12Z

/retest

config/core/configmaps/deployment.yaml

knative-prow · 2024-06-03T15:53:51Z

@izabelacg: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
certmanager-integration-tests_serving_main	`92f0ab5`	link	true	`/test certmanager-integration-tests`

Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

izabelacg · 2024-06-03T18:55:51Z

/retest

dprotaso · 2024-06-04T18:19:08Z

pkg/reconciler/revision/resources/deploy.go

@@ -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 {


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.

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

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.

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.

Renamed the config to defaultAffinityType

dprotaso

Everythign looks good - one minor tweak to when we apply the defaults

dprotaso · 2024-06-04T18:19:37Z

cc @skonto @ReToCode for input

skonto · 2024-06-05T08:54:59Z

config/core/configmaps/deployment.yaml

+    #         weight: 100
+    # `
+    # This may be "none" or "prefer-spread-revision-over-nodes" (default)
+    # affinity: "prefer-spread-revision-over-nodes"


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

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?

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

pkg/reconciler/revision/resources/deploy.go

ReToCode · 2024-06-06T05:33:36Z

LGTM

dprotaso · 2024-06-06T13:54:26Z

/lgtm
/approve

knative-prow · 2024-06-06T13:54:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, izabelacg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

) * add affinity-rules to config-deployment configmap * run ./hack/update-codegen.sh * change affinity rules property to be a flag * run ./hack/update-codegen.sh * add default pod anti-affinity rules to PodSpec * re-arrange imports * enable pod anti affinity by default * fix value in config-deployment * fix condition for adding pod anti-affinity based on presence of a label * run ./hack/update-codegen.sh * clean up deploy tests * change property name * enable pod anti affinity by default * update deployment.yaml * adds new default for enable pod anti affinity to existing tests * change affinity type from toggle to string * run ./hack/update-codegen.sh * fix condition to apply podspec * tweak when applying the defaults * simplify condition that apply affinity defaults * rename new field to default-affinity-type * replace usage of old name affinity * rename test cases

add affinity-rules to config-deployment configmap

ee19514

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2024

knative-prow bot requested review from ReToCode and skonto May 24, 2024 19:06

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 24, 2024

izabelacg added 3 commits May 27, 2024 10:30

run ./hack/update-codegen.sh

3b9f7aa

change affinity rules property to be a flag

dd0d599

run ./hack/update-codegen.sh

0d0bdc7

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2024

add default pod anti-affinity rules to PodSpec

4d73f10

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 27, 2024

izabelacg added 2 commits May 27, 2024 16:24

re-arrange imports

92f0ab5

enable pod anti affinity by default

a633eba

fix value in config-deployment

7d0f070

izabelacg commented May 28, 2024

View reviewed changes

pkg/reconciler/revision/resources/deploy.go Outdated Show resolved Hide resolved

izabelacg added 7 commits May 28, 2024 17:59

fix condition for adding pod anti-affinity based on presence of a label

cc096da

run ./hack/update-codegen.sh

bad694c

clean up deploy tests

e56c559

change property name

9ef849b

enable pod anti affinity by default

8daaa67

update deployment.yaml

a2610e4

adds new default for enable pod anti affinity to existing tests

905443c

izabelacg changed the title ~~[WIP] Add affinity-rules feature to configmap config-deployment~~ Add affinity-rules feature to configmap config-deployment May 29, 2024

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 29, 2024

izabelacg assigned dprotaso May 29, 2024

dprotaso reviewed May 31, 2024

View reviewed changes

config/core/configmaps/deployment.yaml Outdated Show resolved Hide resolved

izabelacg added 2 commits June 3, 2024 11:42

change affinity type from toggle to string

def1a18

run ./hack/update-codegen.sh

da265ce

fix condition to apply podspec

1e5f200

dprotaso reviewed Jun 4, 2024

View reviewed changes

dprotaso mentioned this pull request Jun 4, 2024

feat: add defaults configurable runtimeClassName #15271

Merged

skonto reviewed Jun 5, 2024

View reviewed changes

tweak when applying the defaults

3470332

dprotaso reviewed Jun 5, 2024

View reviewed changes

pkg/reconciler/revision/resources/deploy.go Outdated Show resolved Hide resolved

izabelacg added 4 commits June 5, 2024 15:45

simplify condition that apply affinity defaults

376cd87

rename new field to default-affinity-type

1f4fde2

replace usage of old name affinity

7dec43d

rename test cases

6c9252f

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2024

knative-prow bot merged commit 30a77d1 into knative:main Jun 6, 2024
67 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

izabelacg commented May 24, 2024 •

edited

Loading

codecov bot commented May 27, 2024 •

edited

Loading

izabelacg commented May 27, 2024

izabelacg commented May 27, 2024

izabelacg commented May 27, 2024

izabelacg commented May 29, 2024

knative-prow bot commented May 29, 2024

izabelacg commented May 29, 2024

izabelacg commented May 29, 2024

knative-prow bot commented Jun 3, 2024 •

edited

Loading

izabelacg commented Jun 3, 2024

dprotaso Jun 4, 2024

izabelacg Jun 4, 2024 •

edited

Loading

ReToCode Jun 5, 2024

dprotaso Jun 5, 2024

izabelacg Jun 5, 2024

dprotaso left a comment

dprotaso commented Jun 4, 2024

skonto Jun 5, 2024 •

edited

Loading

dprotaso Jun 5, 2024

skonto Jun 5, 2024

ReToCode commented Jun 6, 2024

dprotaso commented Jun 6, 2024

knative-prow bot commented Jun 6, 2024

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

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

Conversation

izabelacg commented May 24, 2024 • edited Loading

Proposed Changes

codecov bot commented May 27, 2024 • edited Loading

Codecov Report

izabelacg commented May 27, 2024

izabelacg commented May 27, 2024

izabelacg commented May 27, 2024

izabelacg commented May 29, 2024

knative-prow bot commented May 29, 2024

izabelacg commented May 29, 2024

izabelacg commented May 29, 2024

knative-prow bot commented Jun 3, 2024 • edited Loading

izabelacg commented Jun 3, 2024

dprotaso Jun 4, 2024

Choose a reason for hiding this comment

izabelacg Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

ReToCode Jun 5, 2024

Choose a reason for hiding this comment

dprotaso Jun 5, 2024

Choose a reason for hiding this comment

izabelacg Jun 5, 2024

Choose a reason for hiding this comment

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso commented Jun 4, 2024

skonto Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

dprotaso Jun 5, 2024

Choose a reason for hiding this comment

skonto Jun 5, 2024

Choose a reason for hiding this comment

ReToCode commented Jun 6, 2024

dprotaso commented Jun 6, 2024

knative-prow bot commented Jun 6, 2024

izabelacg commented May 24, 2024 •

edited

Loading

codecov bot commented May 27, 2024 •

edited

Loading

knative-prow bot commented Jun 3, 2024 •

edited

Loading

izabelacg Jun 4, 2024 •

edited

Loading

skonto Jun 5, 2024 •

edited

Loading