implements StatefulSet update #46669

kow3ns · 2017-05-31T02:47:03Z

What this PR does / why we need it:

Implements rolling update for StatefulSets
Implements controller history for StatefulSets.
Makes StatefulSet status reporting consistent with DaemonSet and ReplicaSet.

Special notes for your reviewer:

Release note:

Implements rolling update for StatefulSets. Updates can be performed using the RollingUpdate, Paritioned, or OnDelete strategies. OnDelete implements the manual behavior from 1.6. status now tracks 
replicas, readyReplicas, currentReplicas, and updatedReplicas. The semantics of replicas is now consistent with DaemonSet and ReplicaSet, and readyReplicas has the semantics that replicas did prior to this release.

krmayankk · 2017-05-31T03:29:19Z

api/swagger-spec/apps_v1beta1.json

+     "updatedReplicas": {
+      "type": "integer",
+      "format": "int32",
+      "description": "updatedReplicas is the number of replicas"


nit: at updatedRevision

krmayankk · 2017-05-31T03:30:51Z

api/swagger-spec/apps_v1beta1.json

+     },
+     "updateRevision": {
+      "type": "string",
+      "description": "updateRevision is the revision the StatefulSet is attempting to roll out"


nit: comma after revision as in updateRevision is the revision, the StatefulSet is ...

krmayankk · 2017-05-31T03:31:49Z

docs/api-reference/apps/v1beta1/definitions.html

@@ -6452,6 +6535,47 @@ <h3 id="_v1_configmapenvsource">v1.ConfigMapEnvSource</h3>

 </div>
 <div class="sect2">
+<h3 id="_v1beta1_statefulsetupdatestrategy">v1beta1.StatefulSetUpdateStrategy</h3>
+<div class="paragraph">
+<p>StatefulSetUpdateStrategy indicates the strategy that the StatefulSet controller will use to perform updates. It includes any additional parameters necessary to preform the update for the indicated strategy.</p>


nit: perform

krmayankk · 2017-05-31T03:35:52Z

pkg/apis/apps/types.go

+	// PartitionStatefulSetStrategyType indicates that updates will only be
+	// applied to a partition of the StatefulSet. This is useful for canaries
+	// and phased roll outs. When a scale operation is performed with this
+	// strategy, new Pods will be created from the updated specification.


updated specification means updateRevision here, right ?

krmayankk · 2017-05-31T03:36:12Z

pkg/apis/apps/types.go

+	// tracking and ordered rolling restarts are disabled. Pods are recreated
+	// from the StatefulSetSpec when they are manually deleted. When a scale
+	// operation is performed with this strategy, new Pods will be created
+	// from the current specification.


current specification means currentRevision here , right ?

krmayankk · 2017-05-31T03:36:27Z

pkg/apis/apps/types.go

+	// from the StatefulSetSpec when they are manually deleted. When a scale
+	// operation is performed with this strategy, new Pods will be created
+	// from the current specification.
+	OnDeleteStatefulSetStrategyType = "OnDelete"


is this the default strategy ?

no. you should look at defaults.go.

krmayankk · 2017-05-31T03:37:44Z

pkg/apis/apps/types.go

+type PartitionStatefulSetStrategy struct {
+	// Ordinal indicates the ordinal at which the StatefulSet should be
+	// partitioned.
+	Ordinal int32


Can this be omitted in the Partitioned case to assume a default of one ? In which case,this should be a pointer to distringuish between the default case .

i think a default one one assumption will be helpful

RollingUpdate is default

I disagree with defaulting the partition. The user has to declare that they want to partition at a particular point.

the default partition size of 1 is the canary case. Most people would have this use case. Without it, it means specifying one extra param for partitioning in the most common case. It seems like a reasonable default with no surprises .

I disagree that canaries are the most common case. I don't think specifying the fraction of a staged roll out is an undo burden.

I think we should require partition to be specified. The default doesn't add anything for the machine operated use case which is going to be much more common

krmayankk · 2017-05-31T03:38:42Z

pkg/apis/apps/types.go

+
+	// updateStrategy indicates the StatefulSetUpdateStrategy that will be
+	// employed to update Pods in the StatefulSet when a revision is made to
+	// Template or VolumeClaimsTemplate.


Can we remove the VolumeClaimsTemplate from here, since we dont support the update to it.

krmayankk · 2017-05-31T03:41:24Z

pkg/apis/apps/types.go

+	// currentReplicas is the number of replicas at the CurrentRevision
+	CurrentReplicas int32
+
+	// updatedReplicas is the number of replicas


nit: at the updateRevision

krmayankk · 2017-05-31T03:42:40Z

pkg/apis/apps/v1beta1/conversion.go

+	}
+	if in.RevisionHistoryLimit != nil {
+		out.RevisionHistoryLimit = new(int32)
+		*out.RevisionHistoryLimit = *in.RevisionHistoryLimit


and if its nil, set it to 2 ?

defaults.go not conversions

krmayankk · 2017-05-31T03:43:18Z

pkg/apis/apps/v1beta1/conversion.go

@@ -140,8 +147,37 @@ func Convert_apps_StatefulSetSpec_To_v1beta1_StatefulSetSpec(in *apps.StatefulSe
 	} else {
 		out.VolumeClaimTemplates = nil
 	}
+	if in.RevisionHistoryLimit != nil {
+		out.RevisionHistoryLimit = new(int32)
+		*out.RevisionHistoryLimit = *in.RevisionHistoryLimit


in nil case, set it to 2

no that is done in defaults not conversions

krmayankk · 2017-05-31T03:53:03Z

pkg/apis/apps/validation/validation_test.go

+				UpdateStrategy:      apps.StatefulSetUpdateStrategy{Type: "foo"},
+			},
+		},
+		"empty parition": {


nit: partition

krmayankk · 2017-05-31T03:57:24Z

pkg/controller/statefulset/stateful_pod_control_test.go

-		t.Errorf("Expected 0 events for successful status update %d", eventCount)
-	}
-}
-


has all this content moved elsewhere ?

The status tests are in stateful_set_status_updater.go

krmayankk · 2017-05-31T04:16:47Z

pkg/controller/statefulset/stateful_set.go

@@ -305,6 +308,32 @@ func (ssc *StatefulSetController) getPodsForStatefulSet(set *apps.StatefulSet, s
 	return cm.ClaimPods(pods, filter)
 }

+// adoptOrphanRevisions adopts any orphaned ControllerRevisions matched by set's Selector.
+func (ssc *StatefulSetController) adoptOrphanRevisions(set *apps.StatefulSet) error {
+	revisions, err := ssc.control.ListRevisions(set)


is this listing of revisions based on the label on those revisions ?

Yes the revisions are constructed with the StatefulSet's selector

krmayankk · 2017-05-31T04:17:35Z

pkg/controller/statefulset/stateful_set.go

+		}
+	}
+	if hasOrphans {
+		fresh, err := ssc.kubeClient.AppsV1beta1().StatefulSets(set.Namespace).Get(set.Name, metav1.GetOptions{})


curious why cant we use the set.UID instead of querying again ?

We have to refresh the Object to ensure that it hasn't been deleted ensure that we don't patch a ControllerRevision to be owned by a cached deleted/(deleted and recreated) StatefulSet.

hmm how can you be sure of that ?What if its gets deleted after your query ?

I suppose in theory it could. In practice we have seen issues where a stale cache causes invalid adoption. This pattern is used in all controllers for orphan Pods.

krmayankk · 2017-05-31T04:20:18Z

pkg/controller/statefulset/stateful_set_utils.go

+	if pod.Labels == nil {
+		pod.Labels = make(map[string]string)
+	}
+	pod.Labels[apps.StatefulSetRevisionLabel] = revision


check if revision is empty ?

It can't be empty based on the call path. I don't see a need for a check.

krmayankk · 2017-05-31T04:29:32Z

pkg/controller/statefulset/stateful_set_utils.go

+	return patch, err
+}
+
+// newRevision creates a new ControllerRevision containing a patch that can convert a StatefulSet to an equivalent state to set.


nit: the last part of the sentence is not clear convert a StatefulSet to an equivalent state to set.

krmayankk · 2017-05-31T04:33:33Z

pkg/controller/statefulset/stateful_set_utils.go

+	return history.NewControllerRevision(set,
+		controllerKind,
+		selector,
+		runtime.RawExtension{Raw: patch},


this is confusing and at the same time interesting to me as a concept :-) I have not seen this pattern before, but it seems like what you are doing is keeping a strategic merge patch in each revision history , so that any time we want to restore the StatefulSet to that revision, you just use the ready to apply patch from the Raw field to restore it. I think this is worth some more comment

I added commentary to that effect above the getPatch and newRevision methods.

krmayankk · 2017-05-31T04:34:36Z

pkg/controller/statefulset/stateful_set_utils.go

+		return nil, err
+	}
+	clone := obj.(*apps.StatefulSet)
+	patched, err := strategicpatch.StrategicMergePatch([]byte(runtime.EncodeOrDie(patchCodec, clone)), revision.Data.Raw, clone)


check for err here ?

I added a check under strategicpatch
If you mean check that the clone casts correctly, we generally do not do that when deep copying from using Scheme.

krmayankk · 2017-05-31T04:38:50Z

pkg/controller/statefulset/stateful_set_utils.go

+	if err != nil {
+		return nil, err
+	}
+	return clone, err


return clone, nil

krmayankk · 2017-05-31T04:40:03Z

pkg/controller/statefulset/stateful_set_utils.go

+func nextRevision(revisions []*apps.ControllerRevision) int64 {
+	if count := len(revisions); count <= 0 {
+		return 1
+	} else {


remove the else

krmayankk · 2017-05-31T04:45:44Z

pkg/controller/statefulset/stateful_set_utils_test.go

+		t.Fatal(err)
+	}
+	set.Spec.Template.Spec.Containers[0].Name = "foo"
+	restored, err := applyRevision(set, revision)


nit: restoredSet ?

krmayankk · 2017-05-31T04:47:21Z

pkg/controller/statefulset/stateful_set_utils_test.go

+	if err != nil {
+		t.Fatal(err)
+	}
+	if !history.EqualRevision(revision, restoredRevision) {


do you ignore the revision number when comparing the revisions ?

Yes the revisions are only compared by hash and by contents. Two revisions with different ".Revisions" can be equal using this comparison.

krmayankk · 2017-05-31T04:55:28Z

pkg/kubectl/history.go

+	if err != nil {
+		return "", fmt.Errorf("failed to retrieve statefulset %s", err)
+	}
+	revisions, err := h.c.Apps().ControllerRevisions(namespace).List(metav1.ListOptions{LabelSelector: sts.Spec.Selector.String()})


will this sometimes also report orphaned revisions ? To avoid that, we should further validate if the ownerReferences are for this set

0xmichalis · 2017-06-06T09:16:28Z

pkg/api/testing/fuzzer.go

@@ -723,6 +723,13 @@ func appsFuncs(t apitesting.TestingCommon) []interface{} {
 			if len(s.Spec.PodManagementPolicy) == 0 {
 				s.Spec.PodManagementPolicy = apps.OrderedReadyPodManagement
 			}
+			if len(s.Spec.UpdateStrategy.Type) == 0 {
+				s.Spec.UpdateStrategy.Type = apps.RollingUpdateStatefulSetStrategyType


Should you also update this one to OnDelete?

0xmichalis · 2017-06-06T09:20:57Z

pkg/controller/statefulset/stateful_set.go

-			),
+				recorder),
+			NewRealStatefulSetStatusUpdater(kubeClient, setInformer.Lister()),
+			history.NewHistory(kubeClient, revInformer.Lister()),
 		),
 		pvcListerSynced: pvcInformer.Informer().HasSynced,


Since you are using the controllerRevision cache to list CRs, you should wait for it to be synced before starting the sts controller. Means you need a crListerSynced field which you then pass in controller.WaitForCacheSync (L150).

wojtek-t · 2017-06-06T13:47:12Z

/approve no-issue (there is issue from features repo, but bot still doesn't understand it).

wojtek-t · 2017-06-06T13:51:59Z

/approve no-issue

0xmichalis · 2017-06-06T13:55:28Z

there is issue from features repo, but bot still doesn't understand it

FWIW kubernetes/enhancements#188

Implements history utilities for ControllerRevision in the controller/history package StatefulSetStatus now has additional fields for consistency with DaemonSet and Deployment StatefulSetStatus.Replicas now represents the current number of createdPods and StatefulSetStatus.ReadyReplicas is the current number of ready Pods

k8s-ci-robot · 2017-06-06T20:22:13Z

@kow3ns: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	c560debd7c521c50516689795c1e0ca554e66008	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

janetkuo · 2017-06-06T21:42:30Z

pkg/controller/statefulset/stateful_set_utils.go

+		return nil, err
+	}
+	var raw map[string]interface{}
+	json.Unmarshal([]byte(str), &raw)


Would you want to check the returned error?

janetkuo · 2017-06-06T21:46:08Z

pkg/controller/statefulset/stateful_set_utils.go

+// PodSpecTemplate. We can modify this later to encompass more state (or less) and remain compatible with previously
+// recorded patches.
+func getPatch(set *apps.StatefulSet) ([]byte, error) {
+	str, err := runtime.Encode(patchCodec, set)


Remove patchCodec and use json.Marshal here instead, csi team wants to avoid using api.Codecs as much as possible, see #47075 (comment)

janetkuo · 2017-06-06T22:01:53Z

LGTM, comments can be fixed in follow-up PRs

janetkuo · 2017-06-06T22:02:55Z

/lgtm

k8s-github-robot · 2017-06-06T22:05:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, kargakis, kow3ns, smarterclayton, wojtek-t

Associated issue: 188

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

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

~~api/OWNERS~~ [smarterclayton,wojtek-t]
~~cmd/kube-controller-manager/OWNERS~~ [smarterclayton,wojtek-t]
~~docs/OWNERS~~ [smarterclayton,wojtek-t]
~~hack/OWNERS~~ [smarterclayton,wojtek-t]
~~pkg/api/OWNERS~~ [smarterclayton,wojtek-t]
~~pkg/controller/OWNERS~~ [janetkuo,smarterclayton,wojtek-t]
~~pkg/kubectl/OWNERS~~ [janetkuo,smarterclayton,wojtek-t]
~~pkg/registry/OWNERS~~ [smarterclayton,wojtek-t]
~~plugin/pkg/auth/OWNERS~~ [smarterclayton,wojtek-t]
~~staging/src/k8s.io/client-go/OWNERS~~ [smarterclayton,wojtek-t]
~~test/OWNERS~~ [janetkuo,kargakis,kow3ns,smarterclayton,wojtek-t]

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 · 2017-06-07T07:27:52Z

Automatic merge from submit-queue (batch tested with PRs 46235, 44786, 46833, 46756, 46669)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2017

k8s-github-robot assigned foxish and ncdc May 31, 2017

krmayankk reviewed May 31, 2017

View reviewed changes

pkg/controller/statefulset/stateful_set_utils.go Outdated

if err != nil {

return nil, err

}

return clone, err

Copy link

krmayankk May 31, 2017

Choose a reason for hiding this comment

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

return clone, nil

krmayankk reviewed May 31, 2017

View reviewed changes

0xmichalis reviewed Jun 6, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2017

kow3ns force-pushed the statefulset-update branch from 1af3cf1 to 6b41b34 Compare June 6, 2017 17:02

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 6, 2017

kow3ns force-pushed the statefulset-update branch from 6b41b34 to bec6c55 Compare June 6, 2017 18:51

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

Kenneth Owens added 2 commits June 6, 2017 12:00

Implements kubectl rollout status and history for StatefulSet

cec4171

kow3ns force-pushed the statefulset-update branch from bec6c55 to fabbcd6 Compare June 6, 2017 19:47

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

Auto generated code for StatefulSet update

1a784ef

kow3ns force-pushed the statefulset-update branch from fabbcd6 to 1a784ef Compare June 6, 2017 20:47

janetkuo reviewed Jun 6, 2017

View reviewed changes

janetkuo approved these changes Jun 6, 2017

View reviewed changes

k8s-ci-robot assigned janetkuo Jun 6, 2017

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

k8s-github-robot merged commit 0613ae5 into kubernetes:master Jun 7, 2017

kow3ns mentioned this pull request Jun 9, 2017

Allow Updating StatefulSet's entire PodTemplateSpec #41015

Closed

brancz mentioned this pull request Jun 28, 2017

Cannot add serviceAccount to Prometheus object prometheus-operator/prometheus-operator#444

Closed

wallrj mentioned this pull request Feb 1, 2018

Changing CassandraCluster persistence settings should not be allowed jetstack/navigator#233

Closed

implements StatefulSet update #46669

implements StatefulSet update #46669

Conversation

kow3ns commented May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Jun 6, 2017

wojtek-t commented Jun 6, 2017

0xmichalis commented Jun 6, 2017 • edited Loading

k8s-ci-robot commented Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

janetkuo Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

janetkuo commented Jun 6, 2017

janetkuo commented Jun 6, 2017

k8s-github-robot commented Jun 6, 2017

k8s-github-robot commented Jun 7, 2017

kow3ns commented May 31, 2017 •

edited

Loading

0xmichalis commented Jun 6, 2017 •

edited

Loading

k8s-ci-robot commented Jun 6, 2017 •

edited

Loading

janetkuo Jun 6, 2017 •

edited

Loading