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

Serialized protobuf should have stable map order #47701

Merged
merged 3 commits into from
Jun 19, 2017
Merged

Serialized protobuf should have stable map order #47701

merged 3 commits into from
Jun 19, 2017

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jun 17, 2017 •
edited
Loading

Leverage stable_marshaller_all tag in gogo-protobuf to ensure all of our maps have stable ordering when serialized to protobuf. This preserves the behavior from JSON that we rely on to avoid writing to etcd except when the content has changed.

Will slightly increase allocations (1 slice per non-empty map in an object) during Encode, which has a minor impact on CPU. However, avoiding writes when a client issues a blind put results in significantly less CPU across the whole cluster (avoiding a new etcd version for an Endpoints object might save ~1 core/sec on large clusters).

Fixes #47678

The protobuf serialization of API objects has been updated to store maps in a predictable order to ensure that the representation of that object does not change when saved into etcd. This prevents the same object from being seen as being modified, even when no values have changed.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 17, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Jun 17, 2017
@smarterclayton
Copy link
Contributor Author

@kubernetes/sig-api-machinery-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 17, 2017
@smarterclayton smarterclayton assigned lavalamp and unassigned thockin Jun 17, 2017
@liggitt
Copy link
Member

liggitt commented Jun 17, 2017

Fuzz test that serializes multiple times and ensures output is stable would be good

@smarterclayton
Generate protobuf with stable ordered maps
cdfbeff 
The default marshalers from gogo-protobuf do not order keys, which means
successive serializations of the same data would result in same bytes.
Kubernetes leverages stable serialization to avoid excessive writes to
etcd when body content has not changed, which reduces the amount of
watch traffic generated by successive callers.

This will have a small memory impact on objects which have maps -
allocating a new string array for each map key. All maps must be keys
because we serialize those to JSON, so non-string types are not
relevant.
@smarterclayton
generated: protobuf with stable map ordering
606825e
@smarterclayton
Add test for stable encoding
c1ff759
@smarterclayton smarterclayton mentioned this pull request Jun 18, 2017
Merged
@smarterclayton
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

1 similar comment
@smarterclayton
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@liggitt
Copy link
Member

liggitt commented Jun 19, 2017

LGTM, though I'm unsure whether we want this for 1.7

@lavalamp
Copy link
Member

/lgtm
/approve

This is a small change (diff is only big because generated code). It is backwards compatible. Seems very safe as far as these things go.

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

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 47678

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 Jun 19, 2017
@smarterclayton smarterclayton added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jun 19, 2017
@smarterclayton smarterclayton added this to the v1.7 milestone Jun 19, 2017
@smarterclayton smarterclayton removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 19, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 47669, 40284, 47356, 47458, 47701)

@k8s-github-robot k8s-github-robot merged commit 6b52d8f into kubernetes:master Jun 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

@caesarxuchao caesarxuchao

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@smarterclayton @liggitt @lavalamp @k8s-github-robot @caesarxuchao @thockin @k8s-ci-robot