Add support for topology spread constraint #12715

stevenchen-db · 2022-03-09T17:26:35Z

Fixes #12639.
Knative serving currently does not allow specifying topologySpreadConstraints in the pod spec as noted by this issue

I tested this by locally building knative and applying it to a k8s cluster. Topology spread constraints were able to work after enabling them through the config-feature config map

Proposed Changes

Release Note

google-cla · 2022-03-09T17:26:42Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

knative-prow-robot · 2022-03-09T17:26:48Z

Welcome @stevenchen-db! It looks like this is your first PR to knative/serving 🎉

knative-prow-robot · 2022-03-09T17:26:49Z

Hi @stevenchen-db. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

dprotaso · 2022-03-10T18:24:52Z

/ok-to-test

dprotaso

You'll want to sign the CLA - see the link above.

Also can you fill out the release note block in your PR body to say we've added this feature behind a flag

dprotaso · 2022-03-10T18:27:34Z

pkg/reconciler/route/resources/service_test.go

-			TagHeaderBasedRouting:        apiConfig.Disabled,
+			MultiContainer:                   apiConfig.Disabled,
+			PodSpecAffinity:                  apiConfig.Disabled,
+			PodSpecTopologySpreadConstraints: apiConfig.Disabled,


Is this change necessary? Curious what failed when you didn't have this?

I'm not sure, I was mostly going off of this as an example PR 20be262. Is it safe to not include PodSpecTopologySpreadConstraints?

I don't think these changes are necessary then - can you drop them

Done! Also deleted the line you mentioned below

dprotaso · 2022-03-10T18:27:43Z

pkg/reconciler/route/table_test.go

-			TagHeaderBasedRouting:    cfgmap.Disabled,
+			MultiContainer:                   cfgmap.Disabled,
+			PodSpecAffinity:                  cfgmap.Disabled,
+			PodSpecTopologySpreadConstraints: cfgmap.Disabled,


same question as above

same answer as above

dprotaso · 2022-03-10T18:36:58Z

pkg/apis/config/features_test.go

-			"tag-header-based-routing":             "Enabled",
+			MultiContainer:                  Enabled,
+			PodSpecAffinity:                 Enabled,
+			PodSpecTopologySpreadConstraints Enabled,


Compile error - hence all the warnings/test failures

Suggested change

PodSpecTopologySpreadConstraints Enabled,

PodSpecTopologySpreadConstraints: Enabled,

psschwei · 2022-03-14T20:39:32Z

pkg/apis/config/features_test.go

+			PodSpecTopologySpreadConstraints: Enabled,
+		}),
+		data: map[string]string{
+			"kubernetes.podspec-fieldtopologyspreadconstraintsref": "Enabled",


Suggested change

"kubernetes.podspec-fieldtopologyspreadconstraintsref": "Enabled",

"kubernetes.podspec-topologyspreadconstraints": "Enabled",

psschwei

Since this PR is changing the PodSpec, I believe you'll need to update the schemas as well

stevenchen-db · 2022-03-14T22:56:05Z

Since this PR is changing the PodSpec, I believe you'll need to update the schemas as well

@psschwei I didn't see other things like node affinity or container runtime in hack/schemapatch-config.yaml. Is there any reason why topologySpreadConstraints need to be in this file?

psschwei · 2022-03-14T23:34:30Z

Is there any reason why topologySpreadConstraints need to be in this file?

Since you're behind a feature flag, it doesn't. And since preserveUnknownFields is already set to true, doesn't look like there's anything needed on that front for this one (sorry about that, it's been a while since I looked at the schema job...)

codecov · 2022-03-15T13:23:07Z

Codecov Report

Merging #12715 (04cebd0) into main (0753bb1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main   #12715   +/-   ##
=======================================
  Coverage   87.31%   87.31%           
=======================================
  Files         196      196           
  Lines        9750     9754    +4     
=======================================
+ Hits         8513     8517    +4     
+ Misses        952      951    -1     
- Partials      285      286    +1

Impacted Files	Coverage Δ
pkg/apis/config/features.go	`96.00% <100.00%> (+0.16%)`	⬆️
pkg/apis/serving/fieldmask.go	`95.17% <100.00%> (+0.03%)`	⬆️
pkg/reconciler/revision/background.go	`90.00% <0.00%> (-1.82%)`	⬇️
pkg/autoscaler/statserver/server.go	`79.61% <0.00%> (+1.94%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0753bb1...04cebd0. Read the comment docs.

rhuss · 2022-03-16T08:02:29Z

More a general question when changing the set of supported fields PodSpec that can be used in a RevisionTemplateSpec: What is the process to update the spec in https://github.com/knative/specs/blob/main/specs/serving/knative-api-specification-1.0.md#revisiontemplatespec ?

Is there a place where such changes could be collected for a future update of the spec (like with an OPTIONAL keyword), or can optional fields be ignored in the spec for now and it is a second step to include them in the spec? (but might be good to collect them in a separate list for reference and future updates of the spec)

I suggest updating https://github.com/knative/serving/blob/main/DEVELOPMENT.md?plain=1#L230-L236 in case we need to track those changed fields separately.

stevenchen-db · 2022-03-16T23:13:39Z

@dprotaso I just tested this manually and topology spread constraints worked! Can someone trigger the e2e and formatting tests again?

dprotaso · 2022-03-17T18:01:27Z

What is the process to update the spec in https://github.com/knative/specs/blob/main/specs/serving/knative-api-specification-1.0.md#revisiontemplatespec ?

This currently goes through the Knative Trademark Committee.

Is there a place where such changes could be collected for a future update of the spec (like with an OPTIONAL keyword), or can optional fields be ignored in the spec for now and it is a second step to include them in the spec? (but might be good to collect them in a separate list for reference and future updates of the spec)

Changes to the Knative spec apply to all distributions. Fields that are Kubernetes specific will probably never be accepted since K8s is really an implementation detail. And that's ok the OSS implementation can be a super set of the spec.

But since the OSS distribution still values a clear separation of developer/operator concerns some fields may always stay behind a feature flag (like this one).

I suggest updating https://github.com/knative/serving/blob/main/DEVELOPMENT.md?plain=1#L230-L236 in case we need to track those changed fields separately.

This field is covered by the preserveUnknownFields at the PodSpec level

rhuss · 2022-03-17T18:32:03Z

And that's ok the OSS implementation can be a super set of the spec.

I then probably don't fully understand the motivation for a spec then. I thought that one important aspect is portability between Knative installations.

So, if someone uses a field that is only supported on the Kubernetes-based reference implementation, then it should be still possible to reuse the same Knative resource definition on a different implementation, which is supposed to ignore this field. So I would expect that all fields in the reference implementation are still mentioned in the spec, but with an OPTIONAL categorization and specifying what should happen when the implementation does not support this field (usually just ignoring it).

In the 1.0 spec this might be covered via:

Additional spec and status fields MAY be provided by particular implementations, however it is expected that most extension will be accomplished via the metadata.labels and metadata.annotations fields, as Knative implementations MAY validate supplied resources against these fields and refuse resources which specify unknown fields.

but a user should really know which fields can safely be used in order to keep its resources portable, so at least a list of all fields that are supported in the Kubernetes-based reference implementation but not in the spec would be very helpful for users being able to keep the Knative resources portable.

dprotaso · 2022-03-17T18:41:34Z

The feature flag in this PR is off by default so I don't have any concerns about users being confused

dprotaso · 2022-03-18T16:50:35Z

/lgtm
/approve

knative-prow-robot · 2022-03-18T16:50:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, stevenchen-db

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:

~~config/OWNERS~~ [dprotaso]
~~pkg/apis/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 topology spread

eea05ab

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers area/networking labels Mar 9, 2022

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 9, 2022

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 10, 2022

dprotaso reviewed Mar 10, 2022

View reviewed changes

fix

d153376

psschwei reviewed Mar 14, 2022

View reviewed changes

fix test

3157769

psschwei reviewed Mar 14, 2022

View reviewed changes

Merge branch 'main' into topology-spread

16acca0

add to schema

253a7f0

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Mar 14, 2022

stevenchen-db requested review from dprotaso and psschwei March 14, 2022 23:19

remove from schema

987ae13

stevenchen-db added 2 commits March 15, 2022 21:58

fmt

b967468

Merge branch 'main' into topology-spread

daaa81a

stevenchen-db added 2 commits March 17, 2022 15:40

address comments

3455f98

Merge branch 'main' into topology-spread

04cebd0

knative-prow-robot assigned dprotaso Mar 18, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2022

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2022

knative-prow-robot merged commit b8b7480 into knative:main Mar 18, 2022

stevenchen-db mentioned this pull request Apr 28, 2022

Add support for dnsConfig and dnsPolicy in pod spec #12897

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for topology spread constraint #12715

Add support for topology spread constraint #12715

stevenchen-db commented Mar 9, 2022 •

edited

Loading

google-cla bot commented Mar 9, 2022

knative-prow-robot commented Mar 9, 2022

knative-prow-robot commented Mar 9, 2022

dprotaso commented Mar 10, 2022

dprotaso left a comment •

edited

Loading

dprotaso Mar 10, 2022

stevenchen-db Mar 14, 2022 •

edited

Loading

dprotaso Mar 17, 2022

stevenchen-db Mar 17, 2022

dprotaso Mar 10, 2022

stevenchen-db Mar 14, 2022

dprotaso Mar 10, 2022 •

edited

Loading

stevenchen-db Mar 14, 2022

psschwei Mar 14, 2022

psschwei left a comment

stevenchen-db commented Mar 14, 2022

psschwei commented Mar 14, 2022

codecov bot commented Mar 15, 2022 •

edited

Loading

rhuss commented Mar 16, 2022

stevenchen-db commented Mar 16, 2022

dprotaso commented Mar 17, 2022

rhuss commented Mar 17, 2022

dprotaso commented Mar 17, 2022 •

edited

Loading

dprotaso commented Mar 18, 2022

knative-prow-robot commented Mar 18, 2022

	PodSpecTopologySpreadConstraints Enabled,
	PodSpecTopologySpreadConstraints: Enabled,

	"kubernetes.podspec-fieldtopologyspreadconstraintsref": "Enabled",
	"kubernetes.podspec-topologyspreadconstraints": "Enabled",

Add support for topology spread constraint #12715

Add support for topology spread constraint #12715

Conversation

stevenchen-db commented Mar 9, 2022 • edited Loading

Proposed Changes

google-cla bot commented Mar 9, 2022

knative-prow-robot commented Mar 9, 2022

knative-prow-robot commented Mar 9, 2022

dprotaso commented Mar 10, 2022

dprotaso left a comment • edited Loading

Choose a reason for hiding this comment

dprotaso Mar 10, 2022

Choose a reason for hiding this comment

stevenchen-db Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

dprotaso Mar 17, 2022

Choose a reason for hiding this comment

stevenchen-db Mar 17, 2022

Choose a reason for hiding this comment

dprotaso Mar 10, 2022

Choose a reason for hiding this comment

stevenchen-db Mar 14, 2022

Choose a reason for hiding this comment

dprotaso Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

stevenchen-db Mar 14, 2022

Choose a reason for hiding this comment

psschwei Mar 14, 2022

Choose a reason for hiding this comment

psschwei left a comment

Choose a reason for hiding this comment

stevenchen-db commented Mar 14, 2022

psschwei commented Mar 14, 2022

codecov bot commented Mar 15, 2022 • edited Loading

Codecov Report

rhuss commented Mar 16, 2022

stevenchen-db commented Mar 16, 2022

dprotaso commented Mar 17, 2022

rhuss commented Mar 17, 2022

dprotaso commented Mar 17, 2022 • edited Loading

dprotaso commented Mar 18, 2022

knative-prow-robot commented Mar 18, 2022

stevenchen-db commented Mar 9, 2022 •

edited

Loading

dprotaso left a comment •

edited

Loading

stevenchen-db Mar 14, 2022 •

edited

Loading

dprotaso Mar 10, 2022 •

edited

Loading

codecov bot commented Mar 15, 2022 •

edited

Loading

dprotaso commented Mar 17, 2022 •

edited

Loading