Skip to content

WIP - Tag remaining selectors as atomic#100131

Closed
jpbetz wants to merge 3 commits into
kubernetes:masterfrom
jpbetz:make-selectors-atomic
Closed

WIP - Tag remaining selectors as atomic#100131
jpbetz wants to merge 3 commits into
kubernetes:masterfrom
jpbetz:make-selectors-atomic

Conversation

@jpbetz

@jpbetz jpbetz commented Mar 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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:

/sig api-machinery
/wg api-expression

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. wg/api-expression Categorizes an issue or PR as relevant to WG API Expression. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 11, 2021
@jpbetz jpbetz changed the title WIP Tag remaining selectors as atomic WIP - Tag remaining selectors as atomic Mar 11, 2021
@k8s-ci-robot 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 jpbetz mentioned this pull request Mar 11, 2021
Merged
@liggitt liggitt self-assigned this Mar 11, 2021
liggitt
liggitt reviewed Mar 11, 2021
View reviewed changes
Comment thread 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 {

@liggitt liggitt Mar 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@jpbetz jpbetz Mar 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@jpbetz jpbetz Mar 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, it should be the default. Verifying.

@jpbetz jpbetz Mar 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@Jefftree Jefftree Mar 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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 k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Mar 11, 2021
@k8s-ci-robot

k8s-ci-robot commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

[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

@jpbetz jpbetz force-pushed the make-selectors-atomic branch from d32f817 to d257614 Compare March 11, 2021 15:04
Danil-Grigorev
Danil-Grigorev reviewed Mar 11, 2021
View reviewed changes
Comment thread staging/src/k8s.io/api/core/v1/types.go Outdated

@Danil-Grigorev Danil-Grigorev Mar 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread 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

@Danil-Grigorev Danil-Grigorev Mar 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is covered by #97989

@jpbetz jpbetz Mar 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Comment thread 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

@Danil-Grigorev Danil-Grigorev Mar 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is covered by #97989

@k8s-ci-robot

k8s-ci-robot commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

@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

caesarxuchao commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

/triage accepted

@k8s-ci-robot 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 apelisse added this to the v1.22 milestone Apr 5, 2021
@Jefftree Jefftree mentioned this pull request Apr 6, 2021
Merged
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2021
@k8s-ci-robot

k8s-ci-robot commented Apr 9, 2021

Copy link
Copy Markdown
Contributor

@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

apelisse commented Apr 9, 2021

Copy link
Copy Markdown
Member

AFAIU, we can close this one now!

@jpbetz jpbetz closed this Apr 15, 2021
@jpbetz

jpbetz commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

+2 more reviewers

@Jefftree Jefftree Jefftree left review comments

@Danil-Grigorev Danil-Grigorev Danil-Grigorev left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@liggitt liggitt

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on. wg/api-expression Categorizes an issue or PR as relevant to WG API Expression.

Projects

SIG Node: code and documentation PRs
Archived in project

Milestone

v1.22

Development

Successfully merging this pull request may close these issues.

8 participants

@jpbetz @k8s-ci-robot @caesarxuchao @apelisse @liggitt @Jefftree @Danil-Grigorev @ehashman