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

Document pod-template-hash label #3876

Merged
merged 1 commit into from
Jun 26, 2017
Merged

Document pod-template-hash label #3876

merged 1 commit into from
Jun 26, 2017

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented May 23, 2017 •
edited by k8s-reviewable
Loading

Fixes #3569

@kubernetes/sig-apps-misc

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added sig/apps cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 23, 2017
@chenopis
Copy link
Contributor

chenopis commented May 27, 2017 •
edited
Loading

@Kargakis Since you mention 1.7, is this for the 1.7 release? If so, I'm going to tag it w/ the 1.7 Milestone and move it to the release-1.7 branch.

@chenopis chenopis added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. Docs LGTM labels May 27, 2017
@janetkuo
Copy link
Member

pod-template-hash label is implementation detail. It should be documented in developer docs, not user docs IMO. @chenopis WDYT?

@chenopis
Copy link
Contributor

chenopis commented Jun 1, 2017

Do users need to know about the pod-template-hash label at all? If it's an important concept for them to know in order to perform common tasks, then it's fine to be here. Otherwise, if they can get by without knowing this at all, then I would agree that it should go in the developer docs instead.

@janetkuo
Copy link
Member

janetkuo commented Jun 1, 2017

pod-template-hash label is a mechanism for Deployment to separate its children (ReplicaSets) so that they won't fight with each others over the control of the grandchildren (Pods). Users only need to know how to update Deployments and how to interpret Deployment statuses, without knowing anything about this label.

@chenopis
Copy link
Contributor

chenopis commented Jun 1, 2017

Ok, then I would be inclined to put this in the developer docs. @janetkuo Can you suggest a location? Thx

@janetkuo
Copy link
Member

janetkuo commented Jun 1, 2017

https://github.com/kubernetes/community/blob/master/contributors/design-proposals/deployment.md

@chenopis
Copy link
Contributor

chenopis commented Jun 1, 2017

@Kargakis If you agree, then I will close this PR.

@0xmichalis
Copy link
Contributor Author

pod-template-hash label is implementation detail. It should be documented in developer docs, not user docs IMO. @chenopis WDYT?

The fact that users are confused with this label and ask questions about it, warrants adding a section in user docs.

@chenopis
Copy link
Contributor

chenopis commented Jun 2, 2017

Well, if users are asking questions about it, I don't see the harm in having it in the user docs as well.

@0xmichalis
Copy link
Contributor Author

@chenopis can we move forward with this one?

@0xmichalis
Copy link
Contributor Author

@chenopis ping

@0xmichalis 0xmichalis mentioned this pull request Jun 16, 2017
Closed
@janetkuo
Copy link
Member

Sorry for the delay, taking a look now

janetkuo
janetkuo reviewed Jun 16, 2017
View reviewed changes
docs/concepts/workloads/controllers/deployment.md Outdated
ReplicaSets of a Deployment will not overlap among them. It is computed by hashing the PodTemplate of the ReplicaSet
and using the resulting hash as the label value that will be added in the ReplicaSet selector, pod template labels,
and in any existing Pods that the ReplicaSet may have. Up until 1.6, the hashing algorithm that is used is adler.
Starting from 1.7, we switch to a more stable algorithm, fnv, and also introduce a hashing collision avoidance
Copy link
Member

Choose a reason for hiding this comment

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

Remove the hash algorithm part, since it's implementation detail.

docs/concepts/workloads/controllers/deployment.md Outdated
Starting from 1.7, we switch to a more stable algorithm, fnv, and also introduce a hashing collision avoidance
mechanism which will help in ensuring that Deployments are not going to be plagued by hashing collisions anymore.

**Note:** This label is not meant to be updated or removed by users!
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this to be the first sentence of this section? The first thing users need to know is that this label is generated by the server, is essential, and should not be mutated by the users. Then we can talk about details of the label later.

@chenopis chenopis added Tech Review: Open Issues and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Jun 23, 2017
@chenopis
Copy link
Contributor

@Kargakis Sorry for the delay. I was out of town last week. If you can address @janetkuo comments, I can merge this.

@0xmichalis
Document pod-template-hash label
ca2969b 
Signed-off-by: Michail Kargakis <mkargaki@redhat.com>
@0xmichalis
Copy link
Contributor Author

@chenopis @janetkuo comments addressed, ptal

@chenopis
Copy link
Contributor

I'm good w/ this. Once @janetkuo gives the tech lgtm, I will merge it.

@chenopis chenopis merged commit 8c41480 into kubernetes:master Jun 26, 2017
@0xmichalis 0xmichalis deleted the pod-template-hash branch June 26, 2017 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janetkuo janetkuo janetkuo approved these changes

Assignees

@janetkuo janetkuo

@chenopis chenopis

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0xmichalis @chenopis @janetkuo @k8s-ci-robot