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

Make selectors atomic #97989

Merged
merged 1 commit into from
Apr 8, 2021
Merged

Make selectors atomic #97989

merged 1 commit into from
Apr 8, 2021

Conversation

Danil-Grigorev
Copy link
Member

@Danil-Grigorev Danil-Grigorev commented Jan 12, 2021 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

Ensure that all label selectors are treated as atomic values, to exclude situations when selectors are being corrupted by different actors attempting to apply their overlapping definition for this field with server-side-apply.

Which issue(s) this PR fixes:

Fixes #97970

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Server Side Apply now treats all <Some>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.

/sig api-machinery
/wg api-expression

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 12, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @Danil-Grigorev. Thanks for your PR.

I'm waiting for a kubernetes 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.

@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. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 12, 2021
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 12, 2021
@fejta-bot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@apelisse
Copy link
Member

@jpbetz Can you think of a good way to test whether this is going to break?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 13, 2021
@Danil-Grigorev
Copy link
Member Author

@jpbetz @apelisse I added some tests covering the change. I also observed a couple of left-overs still needed to be fixed with defaulted protocol field - https://github.com/kubernetes/kubernetes/pull/97989/files#diff-364a0d27e9a9e7a6f1d2e26ca7509443be3ef59461db6125f982e6c1f19fa2c2R613. Also, so far it is not possible to test ReplicationController apply where the changes were also made, as it seems it does not genereate openapi schema.

@jpbetz
Copy link
Contributor

jpbetz commented Jan 13, 2021

Joe Betz Can you think of a good way to test whether this is going to break?

The underlying mechanism was tested in kubernetes-sigs/structured-merge-diff#170 so I'd expect tests of the specific cases, like @Danil-Grigorev has added, to be sufficient for this PR.

@jpbetz
Copy link
Contributor

jpbetz commented Jan 13, 2021

I've reviewed the tests. The approach of checking that adding a field of to the selector results in a conflict looks right to me.

Use of +mapType=atomic also looks right.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2021
@Danil-Grigorev
Copy link
Member Author

Just realized I committed unwanted tests. PTAL again 😄

@jpbetz jpbetz mentioned this pull request Mar 11, 2021
Closed
1 task
@jpbetz
Copy link
Contributor

jpbetz commented Mar 11, 2021 •
edited
Loading

I don't know if this was already covered. Here's a list of structs that need to be marked as atomic. I opened #100131 with a bunch of tag changes. It has the struct tags and overlaps with this PR for some of the map type tags.

structs that need structType=atomic:

  • corev1.NodeSelector (3 usages)
  • corev1.TopologySelectorTerm (2 usages)
  • corev1.ObjectFieldSelector (2 usages)
  • corev1.ResourceFieldSelector (4 usages)
  • corev1.ConfigMapKeySelector (1 usages)
  • corev1.SecretKeySelector (1 usages)
  • corev1.ScopeSelector (2 usages)

@Danil-Grigorev
Copy link
Member Author

This PR only included common selectors which are user faced. Some of them were not covered, as I guess there was no generate tasks for those openapi types. The plan was to include those later. Let me have a look and update missing.

@Danil-Grigorev Danil-Grigorev force-pushed the atomic-label-selectors branch from 8953bdc to 66869b0 Compare March 11, 2021 16:35
@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 11, 2021
@Danil-Grigorev Danil-Grigorev force-pushed the atomic-label-selectors branch from 66869b0 to 2e12d3d Compare March 12, 2021 09:11
@Danil-Grigorev
Copy link
Member Author

@jpbetz @liggitt Added other selectors of type map which should be atomic. Other api fields of same type are not required to be atomic, as they are similar in function to a map of labels or are part of example api packages. #100131 could take care of structured fields with similar properties.

@ehashman
Copy link
Member

/remove-sig node

@k8s-ci-robot k8s-ci-robot removed the sig/node Categorizes an issue or PR as relevant to SIG Node. label Mar 12, 2021
@apelisse apelisse added this to the v1.22 milestone Apr 5, 2021
@Jefftree Jefftree mentioned this pull request Apr 6, 2021
Merged
@apelisse
Copy link
Member

apelisse commented Apr 7, 2021

I'm pretty sure this is still lgtm, can we get the label re-applied?

@apelisse
Copy link
Member

apelisse commented Apr 7, 2021

Maybe squash some of these commits together?

@Danil-Grigorev
Make selectors atomic
d8a7764 
Ensure that all label selectors are treated as atomic values,
to exclude situations when selectors are being corrupted by
different actors attempting to apply their overlapping definition
for this field with server-side-apply.
@Danil-Grigorev Danil-Grigorev force-pushed the atomic-label-selectors branch from 52c1a75 to d8a7764 Compare April 8, 2021 08:38
@Danil-Grigorev
Copy link
Member Author

@apelisse squashed.

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Apr 8, 2021
@Danil-Grigorev
Copy link
Member Author

/remove-sig node

@k8s-ci-robot k8s-ci-robot removed the sig/node Categorizes an issue or PR as relevant to SIG Node. label Apr 8, 2021
@liggitt
Copy link
Member

liggitt commented Apr 8, 2021

/lgtm
/approve
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danil-Grigorev, liggitt

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1e05d25 into kubernetes:master Apr 8, 2021
@github-actions github-actions bot mentioned this pull request Apr 21, 2021
Open
@jan--f jan--f mentioned this pull request Apr 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

Assignees

@jpbetz jpbetz

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
API Reviews
Status: API review completed, 1.22
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

Server Side Apply - Selectors are not atomic
10 participants
@Danil-Grigorev @k8s-ci-robot @fejta-bot @apelisse @jpbetz @kwiesmueller @fedebongio @ehashman @cbandy @liggitt