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

GuaranteedUpdate must write if stored data is not canonical #48394

Merged
merged 1 commit into from
Jul 3, 2017
Merged

GuaranteedUpdate must write if stored data is not canonical #48394

merged 1 commit into from
Jul 3, 2017

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jul 2, 2017 •
edited
Loading

An optimization added to the GuaranteedUpdate loop changed the
comparison of the current objects serialization against the stored data,
instead comparing to the in memory object, which defeated the mechanism
we use to migrate stored data (GET then PUT should update the version stored in etcd if the canonical serialization has changed)

This commit preserves that optimization but correctly verifies the in
memory serialization against the on disk serialization by fetching the
latest serialized data. Since most updates are not no-ops, this should
not regress the performance of the normal path.

Fixes #48393

When performing a GET then PUT, the kube-apiserver must write the canonical representation of the object to etcd if the current value does not match. That allows external agents to migrate content in etcd from one API version to another, across different storage types, or across varying encryption levels. This fixes a bug introduced in 1.5 where we unintentionally stopped writing the newest data.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 2, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 2, 2017
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@smarterclayton smarterclayton mentioned this pull request Jul 3, 2017
Merged
@smarterclayton smarterclayton force-pushed the must_serialize_if_data_differs branch from c92f863 to 39b18e7 Compare July 3, 2017 01:57
@smarterclayton
GuaranteedUpdate must write if stored data is not canonical
b851614 
An optimization added to the GuaranteedUpdate loop changed the
comparison of the current objects serialization against the stored data,
instead comparing to the in memory object, which defeated the mechanism
we use to migrate stored data.

This commit preserves that optimization but correctly verifies the in
memory serialization against the on disk serialization by fetching the
latest serialized data. Since most updates are not no-ops, this should
not regress the performance of the normal path.
@smarterclayton smarterclayton force-pushed the must_serialize_if_data_differs branch from 39b18e7 to b851614 Compare July 3, 2017 03:13
@smarterclayton
Copy link
Contributor Author

/retest

@deads2k
Copy link
Contributor

deads2k commented Jul 3, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, smarterclayton

Associated issue: 48393

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 3, 2017
@smarterclayton smarterclayton added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Jul 3, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 48439, 48440, 48394)

@k8s-github-robot k8s-github-robot merged commit 4ae3b03 into kubernetes:master Jul 3, 2017
@wojtek-t
Copy link
Member

wojtek-t commented Jul 4, 2017

LGTM - thanks @smarterclayton !

@liggitt liggitt added this to the v1.7 milestone Sep 17, 2017
This was referenced Sep 17, 2017
Merged
Merged
@liggitt liggitt mentioned this pull request Sep 17, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Sep 20, 2017
Merge pull request #52603 from liggitt/automated-cherry-pick-of-#48394-…
be65033 
…#43152-upstream-release-1.6

Automatic merge from submit-queue.

Automated cherry pick of #48394 #43152

Cherry pick of #48394 #43152 on release-1.6.

#48394: GuaranteedUpdate must write if stored data is not canonical
#43152: etcd3 store: retry w/live object on conflict
@wojtek-t wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 2, 2017
k8s-github-robot pushed a commit that referenced this pull request Oct 2, 2017
Merge pull request #52602 from liggitt/automated-cherry-pick-of-#48394-…
10fbd6b 
…#43152-upstream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #48394 #43152

Cherry pick of #48394 #43152 on release-1.7.

#48394: GuaranteedUpdate must write if stored data is not canonical
#43152: etcd3 store: retry w/live object on conflict
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

CaoShuFeng
CaoShuFeng reviewed Nov 5, 2017
View reviewed changes
staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go
if err != nil {
return err
}
mustCheckData = false
Copy link
Contributor

Choose a reason for hiding this comment

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

if bytes.Equal(origState.data, neworigState.data) == true here, can we return directly?

#54780

@liggitt liggitt mentioned this pull request Jan 17, 2018
Closed
@wojtek-t wojtek-t mentioned this pull request Apr 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaoShuFeng CaoShuFeng CaoShuFeng left review comments

Assignees

@hongchaodeng hongchaodeng

@liggitt liggitt

@deads2k deads2k

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

Storage version migration is broken due to an optimization in GuaranteedUpdate
9 participants
@smarterclayton @k8s-cherrypick-bot @deads2k @k8s-github-robot @wojtek-t @CaoShuFeng @hongchaodeng @liggitt @k8s-ci-robot