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.

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

Rename PetSet to StatefulSet in docs and examples. #35776

Merged
merged 1 commit into from Nov 6, 2016
Merged

Rename PetSet to StatefulSet in docs and examples. #35776

merged 1 commit into from Nov 6, 2016

Conversation

jimmycuadra
Copy link
Contributor

@jimmycuadra jimmycuadra commented Oct 28, 2016

What this PR does / why we need it: Addresses some of the pre-code-freeze changes for implementing the PetSet --> StatefulSet rename. (#35534)

Special notes for your reviewer: This PR only changes docs and examples, as #35731 hasn't been merged yet and I don't want to create merge conflicts. I'll open another PR for any remaining code changes needed after that PR is merged. /cc @erictune @janetkuo @chrislovecnm


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot 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 will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Oct 28, 2016
@bgrant0607 bgrant0607 assigned janetkuo and unassigned bgrant0607 Oct 28, 2016
# each other's IP addresses. It does not create a load-balanced ClusterIP and should not be used
# directly by clients in most circumstances.
# This service only exists to create DNS entries for each pod in the stateful set such that they
# can resolve # each other's IP addresses. It does not create a load-balanced ClusterIP and
Copy link
Member

Choose a reason for hiding this comment

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

remove redundant #

# directly by clients in most circumstances.
# This service only exists to create DNS entries for each pod in the stateful set such that they
# can resolve # each other's IP addresses. It does not create a load-balanced ClusterIP and
# should not be used # directly by clients in most circumstances.
Copy link
Member

Choose a reason for hiding this comment

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

remove #

- [Step 2: Use a Pet Set to create Cassandra Ring](#step-2-create-a-cassandra-petset)
- [Step 3: Validate and Modify The Cassandra Pet Set](#step-3-validate-and-modify-the-cassandra-pet-set)
- [Step 4: Delete Cassandra Pet Set](#step-4-delete-cassandra-pet-set)
- [Step 2: Use a StatefulSet to create Cassandra Ring](#step-2-create-a-cassandra-statefulset)
Copy link
Member

Choose a reason for hiding this comment

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

The link should be #step-2-use-a-statefulset-to-create-cassandra-ring

@@ -30,7 +30,7 @@ This example also uses some of the core components of Kubernetes:
- [_Pods_](../../../docs/user-guide/pods.md)
- [ _Services_](../../../docs/user-guide/services.md)
- [_Replication Controllers_](../../../docs/user-guide/replication-controller.md)
- [_Pet Sets_](http://kubernetes.io/docs/user-guide/petset/)
- [_StatefulSets_](http://kubernetes.io/docs/user-guide/statefulset/)
Copy link
Member

@janetkuo janetkuo Oct 28, 2016

Choose a reason for hiding this comment

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

We don't have http://kubernetes.io/docs/user-guide/statefulset/ yet. To do this we need to rename the file in the docs repo and redirect http://kubernetes.io/docs/user-guide/petset/ to http://kubernetes.io/docs/user-guide/statefulset/ (so that we don't break others)

We can leave this link as-is for now

scope of this documentation. [Please refer to the Pet Set documentation.](http://kubernetes.io/docs/user-guide/petset/)
environment can be challenging. We implemented StatefulSet to greatly simplify this
process. Multiple StatefulSet features are used within this example, but is out of
scope of this documentation. [Please refer to the StatefulSet documentation.](http://kubernetes.io/docs/user-guide/statefulset/)
Copy link
Member

Choose a reason for hiding this comment

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

same here wrt the url


There are some limitations with the Alpha release of Pet Set in 1.3. From the [documentation](http://kubernetes.io/docs/user-guide/petset/):
There are some limitations with the Alpha release of StatefulSet in 1.3. From the [documentation](http://kubernetes.io/docs/user-guide/statefulset/):
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -2406,7 +2406,7 @@ __EOF__
# Pre-condition: no statefulset exists
kube::test::get_object_assert statefulset "{{range.items}}{{$id_field}}:{{end}}" ''
# Command: create statefulset
kubectl create -f hack/testdata/nginx-petset.yaml "${kube_flags[@]}"
kubectl create -f hack/testdata/nginx-statefulset.yaml "${kube_flags[@]}"
Copy link
Member

Choose a reason for hiding this comment

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

I guess you forgot to commit hack/testdata/nginx-statefulset.yaml?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope he removed it. Please add it back in.

@janetkuo janetkuo added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 28, 2016
@janetkuo
Copy link
Member

@jimmycuadra thanks for the work! I have a few comments. Please address them in a separate commit so that it's easier to review changes. Then you can squash commits after it's ready to be merged.

@janetkuo janetkuo mentioned this pull request Oct 28, 2016
11 tasks
@janetkuo
Copy link
Member

cc @kow3ns

@janetkuo
Copy link
Member

@k8s-bot ok to test

@janetkuo janetkuo added this to the v1.5 milestone Oct 28, 2016

- PetSet is for ones to tens of instances. Indexed job should work with tens of
- StatefulSet is for ones to tens of instances. Indexed job should work with tens of
thousands of instances.
- When you have few instances, you may want to given them pet names. When you
Copy link
Member

Choose a reason for hiding this comment

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

nit - Perhaps "When you have few instances, you may want to given them pet names. When you have many instances, you that many instances, integer indexes make more sense." should be something like
"When you have few instances, you may want to give them names. When you have many instances, integer indexes make more sense."

@@ -393,21 +393,21 @@ NAME DESIRED CURRENT AGE
cassandra 4 4 36m
```

For the Alpha release of Kubernetes 1.3 the Pet Set resource does not have `kubectl scale`
For the Alpha release of Kubernetes 1.3 the StatefulSet resource does not have `kubectl scale`
Copy link
Member

Choose a reason for hiding this comment

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

Should this updated to reflect that StatefulSet will be a beta feature in 1.5?
The scale limitation remains true.

Copy link
Member

@kow3ns kow3ns left a comment

Choose a reason for hiding this comment

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

Aside from 1 nit and comment LGTM

@jimmycuadra
Copy link
Contributor Author

Thanks for the review! I'm gonna be on a plane most of today but will push up some fixes later tonight or tomorrow morning.

@@ -246,7 +246,7 @@ spec:
timeoutSeconds: 5
# These volume mounts are persistent. They are like inline claims,
# but not exactly because the names need to match exactly one of
# the pet volumes.
# the pod volumes.
Copy link
Member

Choose a reason for hiding this comment

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

This part (from <!-- BEGIN MUNGE --> to <!-- END MUNGE -->) is auto-generated, so you'll need to modify cassandra-statefulset.yaml and then run hack/update-munge-docs.sh to update it

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Thanks, please go through my changes. Also what is the status of the e2e tests?

@@ -874,24 +874,24 @@ admission time; it will need to understand indexes.
previous container failures.
- modify the job template, affecting all indexes.

#### Comparison to PetSets
#### Comparison to StatefulSets
Copy link
Contributor

Choose a reason for hiding this comment

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

We should reference that this feature was called PetSet. Otherwise, people are lacking context and may see this as a new feature.

@@ -47,7 +47,7 @@ spec:
app: cockroachdb
---
apiVersion: apps/v1alpha1
Copy link
Contributor

Choose a reason for hiding this comment

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

@janetkuo is this alpha or beta?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't moved it to beta but the PR #35731 is to-be-merged in 1.5. If we use this file in any test it'd fail until the beta PR gets merged. Maybe do this in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intend for this PR to be merged only after #35731 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to mark the PR as WIP so someone will not accidentally merge it.

Copy link
Member

Choose a reason for hiding this comment

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

I put do-not-merge label :)

@@ -165,18 +165,18 @@ cassandra None <none> 9042/TCP 45s

If an error is returned the service create failed.

## Step 2: Use a Pet Set to create Cassandra Ring
## Step 2: Use a StatefulSet to create Cassandra Ring
Copy link
Contributor

Choose a reason for hiding this comment

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

Also is it StatefulSet or Stateful Set? The object is called StatefulSet, but in English how are we defining it? Again please mention that this feature was previously named PetSet.

Copy link
Member

Choose a reason for hiding this comment

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

I vote for Stateful Set. I saw similar use in the docs of Replica Set, Scheduled Job, etc

scope of this documentation. [Please refer to the Pet Set documentation.](http://kubernetes.io/docs/user-guide/petset/)
environment can be challenging. We implemented StatefulSet to greatly simplify this
process. Multiple StatefulSet features are used within this example, but is out of
scope of this documentation. [Please refer to the StatefulSet documentation.](http://kubernetes.io/docs/user-guide/statefulset/)
Copy link
Contributor

Choose a reason for hiding this comment

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

So we need to link the Pet Set docs and add file an issue to update. Please explain why we are linking the Pet Set docs.

@@ -358,7 +358,7 @@ the last line of the example below is the replicas line that you want to change.
# reopened with the relevant failures.
#
apiVersion: apps/v1alpha1
Copy link
Contributor

Choose a reason for hiding this comment

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

Again is this beta?

@@ -367,7 +367,7 @@ metadata:
name: cassandra
namespace: default
resourceVersion: "323"
selfLink: /apis/apps/v1alpha1/namespaces/default/petsets/cassandra
selfLink: /apis/apps/v1alpha1/namespaces/default/statefulsets/cassandra
Copy link
Contributor

Choose a reason for hiding this comment

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

Again what does this selfLink look like off of a 1.5 cluster?

Copy link
Member

Choose a reason for hiding this comment

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

The selfLink will look like this in 1.5, except that version should be v1beta1

@@ -380,10 +380,10 @@ spec:
replicas: 4
```

The Pet Set will now contain four Pets.
The StatefulSet will now contain four pods.
Copy link
Contributor

Choose a reason for hiding this comment

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

These are not pods. What are we calling them? We have replicas in a deployment. But each replica contains a pod, also you can have multiple pods in a single Pet.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if I understand this. Aren't those "pets" also pods? StatefulSet's status.replicas is calculated from the number of pods the controller sees, and the type of pets in the code is api.Pod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not clear on this either. I understood "pet" to be stateful pod, but it does seem they are not equivalent: From the user guide:

Relationship between Pets and Pods: PetSet requires there be {0..N-1} Pets. Each Pet has a deterministic name - PetSetName-Ordinal, and a unique identity. Each Pet has at most one pod, and each PetSet has at most one Pet with a given identity.

Yet the example shown seems to only contain one pod template, so I'm not sure what a multi-pod pet would look like. The code in the apps API group's types.go also shows only a single pod template for a StatefulSet, which suggests that my original understanding was correct. Chris, do you have an example that explains this better?

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake, I am incorrect ;)


"Deleting the Pet Set will not delete any pets. You will either have to manually scale it down to 0 pets first, or delete the pets yourself.
Deleting and/or scaling a Pet Set down will not delete the volumes associated with the Pet Set. This is done to ensure safety first, your data is more valuable than an auto purge of all related Pet Set resources. Deleting the Persistent Volume Claims will result in a deletion of the associated volumes."
"Deleting the StatefulSet will not delete any pods. You will either have to manually scale it down to 0 pods first, or delete the pods yourself.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again not a pod.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore this mistake, carry on ;)

@@ -2406,7 +2406,7 @@ __EOF__
# Pre-condition: no statefulset exists
kube::test::get_object_assert statefulset "{{range.items}}{{$id_field}}:{{end}}" ''
# Command: create statefulset
kubectl create -f hack/testdata/nginx-petset.yaml "${kube_flags[@]}"
kubectl create -f hack/testdata/nginx-statefulset.yaml "${kube_flags[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nope he removed it. Please add it back in.

@chrislovecnm
Copy link
Contributor

There is a release-note-none label on this. @janetkuo should we mention that all of the examples have changed in the release notes? Pretty big change, not just single line saying hey we changed PetSet -> StatefulSet. But list out the areas of change.

@jimmycuadra
Copy link
Contributor Author

How do we plan to manage the migration of http://kubernetes.io/docs/user-guide/petset/ to http://kubernetes.io/docs/user-guide/statefulset/? Do we want to change the URL and text of the guide in the docs repo before changing references to in the main repo? How will these changes be coordinated with the release of 1.5?

Regarding mentioning the name change in individual examples and guides, I don't think we want to do that. Having an item in the changelog that the feature was renamed is enough. Saying "this used to be called PetSet" everywhere somewhat defeats the purpose of having renamed it. The point is to not use that name anymore. It's only been in alpha—people that used it will learn quickly that it changed. And of course, people that weren't aware of it before won't be confused that it's a different feature if they didn't know about PetSet to begin with.

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Oct 29, 2016

Ah I disagree. People are expecting this feature to be called PetSet. A change log cannot be the only place that references the name change. We need one mention per doc. We are going to confuse people without a simple mention to the previous name. Hell it would confuse me and I maintain the Cassandra example. The only been in alpha argument is an opinion that inferers that people have not been using this feature or it had not been popular. At lest 10-15 engineers at my company use it, and that is just one company. This is a popular feature, even an amazing one. You took great care in trying to get this renamed, now let's take great care in trying to educate people that the name has been changed.

Please open another issue in the website repo about transitioning the name and the url. We are going to need to keep the old page, without the content, with a reference to the new page.

@chrislovecnm
Copy link
Contributor

Also in 3-6 mo then we can start removing the reference to the previous name. Once people have been educated that the name has been changed.

@jimmycuadra
Copy link
Contributor Author

You're right, that seems reasonable. I've updated the two docs to mention the name change.

Regarding using "StatefulSet" as opposed to "stateful set" or "Stateful Set" when writing about the feature in English, I don't have a preference either way. Happy to change it if more people thing it should be written differently when talking about the feature in general rather than referring to the actual resource.

I opted not to change any code in this PR (including e2e tests) to make it easier to merge the non-code stuff in while the alpha to beta PR is pending. We can deal with additional code changes later.

@janetkuo
Copy link
Member

There is a release-note-none label on this. @janetkuo should we mention that all of the examples have changed in the release notes? Pretty big change, not just single line saying hey we changed PetSet -> StatefulSet. But list out the areas of change.

I think release note for this PR is optional, since we already have release note for "Rename PetSet to StatefulSet" in the API change PR (merged). Is that enough?

@janetkuo janetkuo added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Oct 31, 2016
@@ -30,7 +30,7 @@ This example also uses some of the core components of Kubernetes:
- [_Pods_](../../../docs/user-guide/pods.md)
- [ _Services_](../../../docs/user-guide/services.md)
- [_Replication Controllers_](../../../docs/user-guide/replication-controller.md)
- [_Pet Sets_](http://kubernetes.io/docs/user-guide/petset/)
- [_PetSets_](http://kubernetes.io/docs/user-guide/petset/)
Copy link
Member

Choose a reason for hiding this comment

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

I'd say we change the text to StatefulSets, and update the url / filename later

@@ -367,7 +367,7 @@ metadata:
name: cassandra
namespace: default
resourceVersion: "323"
selfLink: /apis/apps/v1alpha1/namespaces/default/petsets/cassandra
selfLink: /apis/apps/v1alpha1/namespaces/default/statefulsets/cassandra
Copy link
Member

Choose a reason for hiding this comment

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

The selfLink will look like this in 1.5, except that version should be v1beta1

@janetkuo
Copy link
Member

janetkuo commented Oct 31, 2016

Since this depends on #35731, we can safely change apps/v1alpha1 to apps/v1beta1 (including apiVersion and selfLink in config files). I prefer Stateful Set over StatefulSet, but the change is optional.

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 31, 2016
@erictune erictune mentioned this pull request Nov 1, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 2, 2016
@jimmycuadra
Copy link
Contributor Author

Now that #35731 is merged, this is ready for final review and merge.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2016
@janetkuo janetkuo removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 3, 2016
@janetkuo
Copy link
Member

janetkuo commented Nov 3, 2016

Mostly LGTM. Would you address these two comments also? #35776 (review)

You may squash commits now.

@jimmycuadra
Copy link
Contributor Author

Done!

@janetkuo janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2016
@janetkuo
Copy link
Member

janetkuo commented Nov 4, 2016

LGTM. Thanks much!

@janetkuo
Copy link
Member

janetkuo commented Nov 4, 2016

@k8s-bot unit test this #36262

---
apiVersion: apps/v1beta1
apiVersion: apps/v1alpha1
Copy link
Member

Choose a reason for hiding this comment

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

Unit test failed. This needs to be apps/v1beta1 as well

@janetkuo janetkuo removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2016
@janetkuo
Copy link
Member

janetkuo commented Nov 4, 2016

@jimmycuadra would you update hack/testdata/nginx-statefulset.yaml to pass the unit test. I'll apply the tag afterwards

@jimmycuadra
Copy link
Contributor Author

Done.

@janetkuo janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit d42eabd. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@janetkuo
Copy link
Member

janetkuo commented Nov 6, 2016

@k8s-bot unit test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b75c3a4 into kubernetes:master Nov 6, 2016
@jimmycuadra jimmycuadra deleted the petset-rename-docs-examples branch November 7, 2016 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/design Categorizes issue or PR as related to design. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants