WIP - Tag remaining selectors as atomic #100131

jpbetz · 2021-03-11T12:36:24Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Without this change using server-side apply to change selectors can result in objects with unexpected selectors.

Based on a review of all occurrences of the term "selector" in types.go files.

TODO:

Add tests

Which issue(s) this PR fixes:

Fixes ##97970

Special notes for your reviewer:

Related to #97989 but for 17 selectors found after reviewing all occurrences of the term "selector" in types.go files.

Does this PR introduce a user-facing change?

Server Side Apply now treats all <*>Selector fields as atomic (meaning the entire selector is managed by a single writer and updated together), since they contain interrelated and inseparable fields that do not merge in intuitive ways.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Please use the following format for linking documentation:

KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/555-server-side-apply
Feature issue: Server-side Apply enhancements#555
List of "selector" occurrences in types.go files: https://docs.google.com/document/d/1jA39N3uvyDHheE-g8nLKbbnKkB3zhe4plAyl8qSdk_k/edit

/sig api-machinery
/wg api-expression

liggitt · 2021-03-11T14:43:13Z

staging/src/k8s.io/api/core/v1/types.go

 // It provides a subset of functionality as NodeSelectorTerm.
 // This is an alpha feature and may change in the future.
+// +structType=atomic
 type TopologySelectorTerm struct {


most uses of this are in a list... should those lists be marked listType=atomic or is that the default?

Good catch, those lists need to be marked as atomic. I'll fix.

Actually, it should be the default. Verifying.

From the docs: "atomic and set apply to lists with scalar elements only" (https://kubernetes.io/docs/reference/using-api/server-side-apply/).

@apelisse do you know if this is accurate?

@jpbetz: Discussed this with @apelisse today, lists are atomic by default and we shouldn't need to do anything extra for them. The documentation is around set lists which aren't applicable in our case (I'll make a todo to update our docs)

k8s-ci-robot · 2021-03-11T14:58:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jpbetz
To complete the pull request process, please assign liggitt after the PR has been reviewed.
You can assign the PR to them by writing /assign @liggitt in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

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

Danil-Grigorev · 2021-03-11T15:03:51Z

staging/src/k8s.io/api/core/v1/types.go

This is covered by https://github.com/kubernetes/kubernetes/pull/97989/files#diff-5ffbebb40a29f8e491f4ff1e07c618eb3edcc0d5f00a1a71445272d6767d7883R2985 and has included tests. No need to include this in that PR.

Danil-Grigorev · 2021-03-11T15:07:21Z

staging/src/k8s.io/api/core/v1/types.go

 	// controller, if empty defaulted to labels on Pod template.
 	// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors
 	// +optional
+	// +mapType=atomic


This is covered by #97989

Yes, I'll drop everything already in #97989 from this PR. Probably makes sense for #97989 to cover all the map typed fields.

Danil-Grigorev · 2021-03-11T15:07:31Z

staging/src/k8s.io/api/core/v1/types.go

 	// Ignored if type is ExternalName.
 	// More info: https://kubernetes.io/docs/concepts/services-networking/service/
 	// +optional
+	// +mapType=atomic


This is covered by #97989

k8s-ci-robot · 2021-03-11T17:28:20Z

@jpbetz: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	`43b1355`	link	`/test pull-kubernetes-unit`
pull-kubernetes-integration	`43b1355`	link	`/test pull-kubernetes-integration`
pull-kubernetes-e2e-gce-100-performance	`43b1355`	link	`/test pull-kubernetes-e2e-gce-100-performance`
pull-kubernetes-e2e-gce-ubuntu-containerd	`43b1355`	link	`/test pull-kubernetes-e2e-gce-ubuntu-containerd`
pull-kubernetes-conformance-kind-ga-only-parallel	`43b1355`	link	`/test pull-kubernetes-conformance-kind-ga-only-parallel`
pull-kubernetes-e2e-kind-ipv6	`43b1355`	link	`/test pull-kubernetes-e2e-kind-ipv6`
pull-kubernetes-e2e-kind	`43b1355`	link	`/test pull-kubernetes-e2e-kind`
pull-kubernetes-node-e2e	`43b1355`	link	`/test pull-kubernetes-node-e2e`
pull-kubernetes-verify-govet-levee	`43b1355`	link	`/test pull-kubernetes-verify-govet-levee`
pull-kubernetes-typecheck	`43b1355`	link	`/test pull-kubernetes-typecheck`
pull-kubernetes-verify	`43b1355`	link	`/test pull-kubernetes-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

caesarxuchao · 2021-03-11T21:08:33Z

/triage accepted

k8s-ci-robot · 2021-04-09T16:22:31Z

@jpbetz: PR needs rebase.

Details

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.

apelisse · 2021-04-09T16:23:54Z

AFAIU, we can close this one now!

jpbetz · 2021-04-15T18:09:03Z

Yep, this is superseded by the work @Jefftree is doing. Closed.

jpbetz added 2 commits March 10, 2021 20:51

Add mapType=atomic and structType=atomic to all selectors

a5f6302

Generate openapi and proto files

d257614

jpbetz changed the title ~~WIP Tag remaining selectors as atomic~~ WIP - Tag remaining selectors as atomic Mar 11, 2021

k8s-ci-robot requested review from dchen1107 and derekwaynecarr March 11, 2021 12:37

k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/node Categorizes an issue or PR as relevant to SIG Node. labels Mar 11, 2021

jpbetz mentioned this pull request Mar 11, 2021

Make selectors atomic #97989

Merged

liggitt self-assigned this Mar 11, 2021

liggitt reviewed Mar 11, 2021

View reviewed changes

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Mar 11, 2021

jpbetz force-pushed the make-selectors-atomic branch from d32f817 to d257614 Compare March 11, 2021 15:04

fix usages of TopologySelectorTerm and clarify expected usage

43b1355

Danil-Grigorev reviewed Mar 11, 2021

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 11, 2021

apelisse added this to the v1.22 milestone Apr 5, 2021

Jefftree mentioned this pull request Apr 6, 2021

Tag structs as atomic #100684

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2021

jpbetz closed this Apr 15, 2021

WIP - Tag remaining selectors as atomic #100131

WIP - Tag remaining selectors as atomic #100131

Uh oh!

Conversation

jpbetz commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 11, 2021

Uh oh!

caesarxuchao commented Mar 11, 2021

Uh oh!

k8s-ci-robot commented Apr 9, 2021

Uh oh!

apelisse commented Apr 9, 2021

Uh oh!

jpbetz commented Apr 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jpbetz commented Mar 11, 2021 •

edited

Loading