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

apiextensions: fix concurrent map access copying items' ObjectMeta in UnstructuredList #65092

Merged
merged 1 commit into from
Jun 15, 2018
Merged

apiextensions: fix concurrent map access copying items' ObjectMeta in UnstructuredList #65092

merged 1 commit into from
Jun 15, 2018

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Jun 14, 2018 •
edited
Loading

The list endpoint sets the self-link. If we do not create a (shallow) copy of ObjectMeta this will mutate the cached objects.

Background: native objects like PodList have a Items []Pod field which implicitly shallow-copies ObjectMeta. UnstructuredList only shallow-copies Unstructured, but shares all the JSON data including ObjectMeta, i.e. the Object pointer points to the value in the cache.

Fixes #59478

Fixed API server panic during concurrent GET or LIST requests with non-empty `resourceVersion`.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 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. labels Jun 14, 2018
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2018
@sttts sttts added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. release-note-none Denotes a PR that doesn't merit a release note. area/custom-resources 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 Jun 14, 2018
@sttts
Copy link
Contributor Author

sttts commented Jun 14, 2018 •
edited
Loading

/cc @nikhita @ash2k

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 14, 2018
@sttts sttts mentioned this pull request Jun 14, 2018
Closed
@sttts sttts force-pushed the sttts-crd-registry-objectmeta-deepcopy branch 2 times, most recently from 59a0663 to 5e21c63 Compare June 14, 2018 10:46
@ash2k
Copy link
Member

ash2k commented Jun 14, 2018

Wow, so my educated guess was correct. Code in the List() method from the embedded type worked for 2.5 years because it was not used with unstructured before.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2018
@sttts
Copy link
Contributor Author

sttts commented Jun 14, 2018

Fixed the comment.

@sttts sttts force-pushed the sttts-crd-registry-objectmeta-deepcopy branch from 5e21c63 to 1a7308d Compare June 14, 2018 12:32
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2018
@sttts sttts added this to the v1.11 milestone Jun 14, 2018
@sttts
Copy link
Contributor Author

sttts commented Jun 14, 2018

This is also a 1.10 cherry-pick candidate.

@deads2k
Copy link
Contributor

deads2k commented Jun 15, 2018

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2018
@deads2k
Copy link
Contributor

deads2k commented Jun 15, 2018

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, deads2k, sttts

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

@sttts sttts mentioned this pull request Jun 15, 2018
Closed
@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Jun 15, 2018
@k8s-ci-robot k8s-ci-robot removed the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 15, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-ci-robot
Copy link
Contributor

@dims: Those labels are not set on the issue: priority/important-soon

In response to this:

/remove-priority important-soon

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.

@dims
Copy link
Member

dims commented Jun 15, 2018

/priority critical-urgent

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@ash2k @deads2k @liggitt @mbohlool @sttts

Pull Request Labels
  • sig/api-machinery: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/bug: Fixes a bug discovered during the current release.
Help

@liggitt
Copy link
Member

liggitt commented Jun 15, 2018

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 32003df into kubernetes:master Jun 15, 2018
This was referenced Jun 18, 2018
Merged
Merged
@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 18, 2018
@wenjiaswe
Copy link
Contributor

@sttts we have seen this on 1.8.12 as well, would you please help to cherry pick this to 1.8 release? I tried but there are some conflict that I am not sure about. I think it would be better if you could do that.

cc 1.8 patch manager @jpbetz

@sttts sttts mentioned this pull request Jul 2, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request Jul 5, 2018
Merge pull request #65169 from sttts/automated-cherry-pick-of-#65092-…
c2060cd 
…upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #65092: apiextensions: fix concurrent map access copying items'

Cherry pick of #65092 on release-1.10.

#65092: apiextensions: fix concurrent map access copying items'
jpbetz added a commit that referenced this pull request Jul 10, 2018
@jpbetz
Merge pull request #65693 from sttts/automated-cherry-pick-of-#65092-…
3450c09 
…upstream-release-1.8

Automated cherry pick of #65092: apiextensions: fix concurrent map access copying items'
k8s-github-robot pushed a commit that referenced this pull request Jul 25, 2018
Merge pull request #65170 from sttts/automated-cherry-pick-of-#65092-…
017beba 
…upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #65092: apiextensions: fix concurrent map access copying items'

Cherry pick of #65092 on release-1.9.

#65092: apiextensions: fix concurrent map access copying items'
@liggitt liggitt mentioned this pull request Sep 14, 2018
Closed
@liggitt liggitt mentioned this pull request Jan 1, 2019
Closed
@liggitt liggitt mentioned this pull request Jul 25, 2019
Closed
@ecordell ecordell mentioned this pull request Aug 29, 2019
Closed
@jfbai jfbai mentioned this pull request Mar 6, 2020
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

@deads2k deads2k Awaiting requested review from deads2k

@enisoc enisoc Awaiting requested review from enisoc

@nikhita nikhita Awaiting requested review from nikhita

Assignees

@ash2k ash2k

@liggitt liggitt

@mbohlool mbohlool

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/custom-resources cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.11
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@sttts @ash2k @deads2k @nikhita @k8s-ci-robot @k8s-github-robot @dims @liggitt @wenjiaswe @mbohlool