Making the pod.alpha.kubernetes.io/initialized annotation optional in PetSet pods #35739

foxish · 2016-10-27T23:56:34Z

What this PR does / why we need it: As of now, the absence of the annotation pod.alpha.kubernetes.io/initialized in PetSets causes the PetSet controller to effectively "pause". Being a debug hook, users expect that its absence has no effect on the working of a PetSet. This PR inverts the logic so that we let the PetSet controller operate as expected in the absence of the annotation.
Letting the annotation remain alpha seems ok. Renaming it to something more meaningful needs further discussion.

Which issue this PR fixes (optional, in fixes #<issue number>(, #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #35498

Special notes for your reviewer:

Release note:

The annotation "pod.alpha.kubernetes.io/initialized" on StatefulSets (formerly PetSets) is now optional and only encouraged for debug use.

cc @erictune @smarterclayton @bprashanth @kubernetes/sig-apps
@kow3ns The examples will need to be cleaned up as well I think later on to remove them.

This change is

bprashanth · 2016-10-28T00:43:05Z

pkg/controller/petset/pet.go

 func (d *defaultPetHealthChecker) isHealthy(pod *api.Pod) bool {
 	if pod == nil || pod.Status.Phase != api.PodRunning {
 		return false
 	}
 	initialized, ok := pod.Annotations[PetSetInitAnnotation]
 	if !ok {
-		glog.Infof("PetSet pod %v in %v, waiting on annotation %v", api.PodRunning, pod.Name, PetSetInitAnnotation)
-		return false
+		// if the annotation was not found, we just perform a readiness check.


nit: we don't perform the check, we just check to see if the pod has passed the check

bprashanth · 2016-10-28T00:44:09Z

pkg/controller/petset/pet.go

-// isHealthy returns true if the pod is running and has the
-// "pod.alpha.kubernetes.io/initialized" set to "true".
+// isHealthy returns true if the pod is running and if it has the
+// "pod.alpha.kubernetes.io/initialized" annotation, it is set to "true".
 func (d *defaultPetHealthChecker) isHealthy(pod *api.Pod) bool {
 	if pod == nil || pod.Status.Phase != api.PodRunning {
 		return false
 	}
 	initialized, ok := pod.Annotations[PetSetInitAnnotation]


nit might as well rename the variable holding the annotation to StatefulSetInitAnnotation or whatever we're going to use as the suffix

bprashanth · 2016-10-28T00:46:44Z

pkg/controller/petset/pet.go

@@ -297,22 +297,23 @@ type petHealthChecker interface {
 // It doesn't update, probe or get the pod.
 type defaultPetHealthChecker struct{}

-// isHealthy returns true if the pod is running and has the
-// "pod.alpha.kubernetes.io/initialized" set to "true".
+// isHealthy returns true if the pod is running and if it has the


nit: pod is running and ready, and please reword to "if the pod is running and ready. If it has the ... set to false, pod state is ignored."

bprashanth · 2016-10-28T00:58:43Z

pkg/controller/petset/pet.go

-		glog.Infof("PetSet pod %v in %v, waiting on annotation %v", api.PodRunning, pod.Name, PetSetInitAnnotation)
-		return false
+		// if the annotation was not found, we just perform a readiness check.
+		return api.IsPodReady(pod)
 	}
 	b, err := strconv.ParseBool(initialized)


nit slightly prefer something like:

podReady := api.IsPodReady() // User might've specified a pod readiness override through a debug annotation initialized, ok := pod.Annotations[PetSetInitAnnotation] if ok { if initAnnotation, err := ParseBool(); err != nil { glog.Infof("failed to parse annotation %v", err) } else if !initAnnotation { podReady = initAnnotation } } return podReady

bprashanth · 2016-10-28T00:59:00Z

test/e2e/petset.go

@@ -889,6 +889,9 @@ func newPetSet(name, ns, governingSvcName string, replicas int32, petMounts []ap
 		ObjectMeta: api.ObjectMeta{
 			Name:      name,
 			Namespace: ns,
+			Annotations: map[string]string {
+				"pod.alpha.kubernetes.io/initialized": "false",


so we're not renaming the part after the "/"?

I think we can let it be for now and think about it when we're updating the examples?

I'm ok with letting it be inverted just for this PR.

k8s-ci-robot · 2016-10-28T01:25:20Z

Jenkins GCI GCE e2e failed for commit 0e04556. Full PR test history.

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

foxish · 2016-10-31T18:58:31Z

@bprashanth Updated. PTAL.

foxish · 2016-10-31T21:19:54Z

@k8s-bot cvm gke e2e test this

k8s-ci-robot · 2016-10-31T22:05:33Z

Jenkins GKE smoke e2e failed for commit 5c3792a. Full PR test history.

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

foxish · 2016-11-01T19:22:42Z

@k8s-bot cvm gke e2e test this

smarterclayton · 2016-11-01T19:34:38Z

/lgtm

bprashanth · 2016-11-01T21:34:49Z

LGTM thanks

k8s-github-robot · 2016-11-02T16:21:44Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-02T16:58:00Z

Automatic merge from submit-queue

janetkuo · 2016-11-02T22:37:10Z

@foxish just a reminder that statefulset docs need to be updated too

@bprashanth

Automatic merge from submit-queue Set the annotation only if the test requires it. **What this PR does / why we need it**: Fixes StatefulSet flake **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #36107 **Special notes for your reviewer**: We shouldn't be setting the debug annotation in all our tests, only the ones that bring statefulset pods up one after another. In the absence of the annotation, we have the new default behavior governed by #35739 **Release note**: ```release-note NONE ``` cc @kubernetes/sig-apps @bprashanth @calebamiles

foxish added area/stateful-apps sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Oct 27, 2016

foxish added this to the v1.5 milestone Oct 27, 2016

foxish assigned smarterclayton and bprashanth Oct 27, 2016

googlebot added the cla: yes label Oct 27, 2016

foxish force-pushed the migrating-the-annotation branch from 862cab0 to 0e04556 Compare October 27, 2016 23:59

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Oct 28, 2016

bprashanth reviewed Oct 28, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2016

foxish force-pushed the migrating-the-annotation branch from 0e04556 to ccae356 Compare October 31, 2016 18:35

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2016

foxish force-pushed the migrating-the-annotation branch 2 times, most recently from 5d2ca90 to c5ff869 Compare October 31, 2016 18:57

foxish 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 Oct 31, 2016

Flipping behavior when annotation is absent.

5c3792a

foxish force-pushed the migrating-the-annotation branch from c5ff869 to 5c3792a Compare October 31, 2016 19:49

k8s-github-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2016

k8s-github-robot merged commit 49f1aa0 into kubernetes:master Nov 2, 2016

foxish mentioned this pull request Nov 2, 2016

kubernetes-e2e-gci-gce-petset: broken test run #35768

Closed

foxish mentioned this pull request Nov 4, 2016

Set the annotation only if the test requires it. #36212

Merged

foxish deleted the migrating-the-annotation branch November 4, 2016 07:19

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making the pod.alpha.kubernetes.io/initialized annotation optional in PetSet pods #35739

Making the pod.alpha.kubernetes.io/initialized annotation optional in PetSet pods #35739

foxish commented Oct 27, 2016 •

edited

Loading

bprashanth Oct 28, 2016

bprashanth Oct 28, 2016

bprashanth Oct 28, 2016

bprashanth Oct 28, 2016

bprashanth Oct 28, 2016

foxish Oct 31, 2016

smarterclayton Oct 31, 2016

k8s-ci-robot commented Oct 28, 2016

foxish commented Oct 31, 2016

foxish commented Oct 31, 2016

k8s-ci-robot commented Oct 31, 2016

foxish commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

bprashanth commented Nov 1, 2016

k8s-github-robot commented Nov 2, 2016

k8s-github-robot commented Nov 2, 2016

janetkuo commented Nov 2, 2016

Making the pod.alpha.kubernetes.io/initialized annotation optional in PetSet pods #35739

Making the pod.alpha.kubernetes.io/initialized annotation optional in PetSet pods #35739

Conversation

foxish commented Oct 27, 2016 • edited Loading

bprashanth Oct 28, 2016

Choose a reason for hiding this comment

bprashanth Oct 28, 2016

Choose a reason for hiding this comment

bprashanth Oct 28, 2016

Choose a reason for hiding this comment

bprashanth Oct 28, 2016

Choose a reason for hiding this comment

bprashanth Oct 28, 2016

Choose a reason for hiding this comment

foxish Oct 31, 2016

Choose a reason for hiding this comment

smarterclayton Oct 31, 2016

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 28, 2016

foxish commented Oct 31, 2016

foxish commented Oct 31, 2016

k8s-ci-robot commented Oct 31, 2016

foxish commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

bprashanth commented Nov 1, 2016

k8s-github-robot commented Nov 2, 2016

k8s-github-robot commented Nov 2, 2016

janetkuo commented Nov 2, 2016

foxish commented Oct 27, 2016 •

edited

Loading