DaemonSet updates - take 2 #41116

lukaszo · 2017-02-08T02:19:10Z

It implements https://github.com/kubernetes/community/blob/master/contributors/design-proposals/daemonset-update.md
Feature kubernetes/enhancements#124
#15310

Implement the update feature for DaemonSet.

cc @Kargakis @janetkuo @mikedanese

k8s-reviewable · 2017-02-08T02:19:17Z

This change is

0xmichalis · 2017-02-08T11:58:14Z

@kubernetes/sig-apps-pr-reviews

0xmichalis · 2017-02-08T12:03:26Z

pkg/apis/extensions/types.go

+	// available (ready for at least minReadySeconds)
+	NumberAvailable int32
+
+	// NumberUnavailable is the number of nodes that should be running the


FWIW, for Deployments, this is the number of pods running but not considered ready/available.

kubernetes/pkg/controller/deployment/sync.go

Line 586 in d953402

unavailableReplicas := totalReplicas - availableReplicas

FWIW, for Deployments, this is the number of pods running but not considered ready/available.

kubernetes/pkg/controller/deployment/sync.go

Line 586 in d953402

unavailableReplicas := totalReplicas - availableReplicas

That means the same thing as !(running && available) as this comment described, right?

0xmichalis · 2017-02-08T12:03:47Z

pkg/apis/extensions/v1beta1/conversion.go

@@ -219,6 +225,73 @@ func Convert_v1beta1_RollingUpdateDeployment_To_extensions_RollingUpdateDeployme
 	return nil
 }

+func Convert_extensions_DaemonSetSpec_To_v1beta1_DaemonSetSpec(in *extensions.DaemonSetSpec, out *DaemonSetSpec, s conversion.Scope) error {


Why do we need custom conversions?

Hmm, I had some errors with validation earlier and thought that it will help. It turns out that only
Convert_extensions_RollingUpdateDaemonSet_To_v1beta1_RollingUpdateDaemonSet Convert_v1beta1_RollingUpdateDaemonSet_To_extensions_RollingUpdateDaemonSet,
are required

0xmichalis · 2017-02-08T12:06:07Z

pkg/apis/extensions/validation/validation.go

+		}
+		allErrs = append(allErrs, ValidateRollingUpdateDaemonSet(strategy.RollingUpdate, fldPath.Child("rollingUpdate"))...)
+	} else {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), strategy.Type, "RollingUpdate and OnDelete are only supported types"))


use field.NotSupported

0xmichalis · 2017-02-08T12:06:55Z

pkg/apis/extensions/validation/validation.go

+
+func ValidateDaemonSetUpdateStrategy(strategy *extensions.DaemonSetUpdateStrategy, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if strategy.Type == extensions.OnDeleteDaemonSetStrategyType {


nit: make this a switch

0xmichalis · 2017-02-08T12:17:53Z

pkg/controller/daemon/daemoncontroller.go

@@ -548,10 +560,12 @@ func (dsc *DaemonSetsController) manage(ds *extensions.DaemonSet) error {
 	glog.V(4).Infof("Nodes needing daemon pods for daemon set %s: %+v, creating %d", ds.Name, nodesNeedingDaemonPods, createDiff)
 	createWait := sync.WaitGroup{}
 	createWait.Add(createDiff)
+	dsPodTemplateSpecHash := podutil.GetPodTemplateSpecHashFnv(ds.Spec.Template)


Add a TODO to this helper to be moved in a more generic location.

0xmichalis · 2017-02-08T12:27:51Z

pkg/controller/daemon/util/daemonset_util.go

+			return true
+		}
+	} else {
+		// XXX: Hash does not exist. It's old pod. For now returning true.


In such a case I think you should compare (apimachinery.Semantic.DeepEqual) the ds pod template spec with the pod spec AND the labels and annotations found in the metadata of the ds podtemplate vs pod metadata.

From @lukaszo's earlier comment it seems hard to compare pod spec with DS template because a lot of default values are applied while a pod is created. @lukaszo does this mean we won't be able to rolling update a DS created prior to 1.6 (unless users manually kill those pods without hash)?

Why can't we have a helper that will do the Pod->PodTemplateSpec conversion?

0xmichalis · 2017-02-08T12:30:57Z

pkg/controller/daemon/util/daemonset_util.go

+	badPods := []*v1.Pod{}
+	goodPods := []*v1.Pod{}
+	for _, pod := range pods {
+		if pod.Status.Phase == v1.PodRunning {


Terminating pods are still running pods, right?

Also we should probably use IsPodReady (uses Conditions) in favor of Phase.

0xmichalis · 2017-02-08T12:36:43Z

pkg/controller/daemon/util/daemonset_util.go

+func GetNumUnavailable(ds *extensions.DaemonSet, newPods, oldPods []*v1.Pod) int {
+	unavailable := int(ds.Status.DesiredNumberScheduled) - len(newPods) - len(oldPods)
+	for _, pod := range newPods {
+		if pod.Status.Phase != v1.PodRunning {


You need to use IsPodAvailable and provide MinReadySeconds from the ds spec

Also pods that are marked for deletion should count as unavailable.

0xmichalis · 2017-02-08T12:38:35Z

pkg/controller/daemon/update.go

+	glog.V(4).Infof("Marking all bad old pods for deletion")
+	for _, pod := range badPods {
+		glog.V(4).Infof("Marking pod %s/%s for deletion", ds.Name, pod.Name)
+		podsToDelete = append(podsToDelete, pod.Name)


We shouldn't retry the deletion if the pod is already marked for deletion?

I'm not sure. manage doesn't check it. Also RollingUpdate shouldn't be executed until all expected deletions are complete.

Pods won't be deleted immediately after Delete returns; it may be terminating depending on its termination grace period

0xmichalis · 2017-02-08T12:41:56Z

pkg/controller/daemon/daemoncontroller.go

@@ -716,6 +744,17 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error {
 		}
 	}

+	dsNeedsSync = dsc.expectations.SatisfiedExpectations(dsKey)


Already called above. Although expectations never worked as expected and we should probably get rid of them from this controller too.

manage can set new expectations. I want to run updates only if manage didn't change anything.

0xmichalis · 2017-02-08T12:54:55Z

pkg/controller/daemon/util/daemonset_util.go

+}
+
+func GetNumUnavailable(ds *extensions.DaemonSet, newPods, oldPods []*v1.Pod) int {
+	unavailable := int(ds.Status.DesiredNumberScheduled) - len(newPods) - len(oldPods)


DesiredNumberScheduled may not be synced when you call GetNumUnavailable. Generally, utilities for constructing the status of an object should not treat fields from the status object as correct. Instead try to identify how much DesiredNumberScheduled really is.

0xmichalis · 2017-02-08T12:56:16Z

pkg/controller/daemon/update.go

+	if err != nil {
+		return fmt.Errorf("couldn't get list of nodes during rolling update of daemon set %#v: %v", ds, err)
+	}
+	maxUnavailable, err := intstrutil.GetValueFromIntOrPercent(ds.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable, len(nodeList), true)


There is a helper for this in the deployment utilities.

This one https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/deployment/util/deployment_util.go#L417 ? I can not use it.

0xmichalis · 2017-02-08T12:57:27Z

pkg/controller/daemon/daemoncontroller.go

+	return utilerrors.NewAggregate(errors)
+}
+
+func (dsc *DaemonSetsController) syncNodes(ds *extensions.DaemonSet, podsToDelete, nodesNeedingDaemonPods []string) []error {


godoc what syncNodes is supposed to do.

0xmichalis · 2017-02-08T13:00:23Z

pkg/controller/daemon/update.go

+	podutil "k8s.io/kubernetes/pkg/controller/deployment/util"
+)
+
+func (dsc *DaemonSetsController) rollingUpdate(ds *extensions.DaemonSet) error {


godoc what rollingUpdate is doing exactly. It seems that it simply deletes pods if possible and then manage will make sure the new pods will spin up?

Yes, manage will create new pods but I'm not sure if this is a correct approach. I need to run more tests.

Add a comment to describe that? // rollingUpdate does such and such...

0xmichalis · 2017-02-08T13:01:59Z

pkg/controller/daemon/update.go

+}
+
+func (dsc *DaemonSetsController) getAllDaemonSetPods(ds *extensions.DaemonSet) ([]*v1.Pod, []*v1.Pod, error) {
+	newPods := []*v1.Pod{}


Use var newPods []*v1.Pod instead - consistent with what's the conventional way of defining stuff in the codebase (avoids an extra allocation)

0xmichalis · 2017-02-08T13:04:03Z

pkg/controller/daemon/daemoncontroller.go

+					updated = true
+				}
+				if ready && updated {
+					numberAvailable++


This is not what available should be. Available is the # of pods (old or new) that are ready for at least minReadySeconds.

Yes, ref https://github.com/kubernetes/community/blob/master/contributors/design-proposals/daemonset-update.md#daemonset-controller:

.status.numberAvailable = the total number of DaemonSet pods that have become Ready for MinReadySeconds

.status.numberUnavailable = .status.desiredNumberScheduled - .status.numberAvailable

idvoretskyi · 2017-02-08T14:26:02Z

cc @kubernetes/mirantis

0xmichalis · 2017-02-08T16:18:58Z

Can you paste the errors you are getting? We should be good with generated conversions for DS afaik

…

On Wed, Feb 8, 2017 at 4:19 PM, Łukasz Oleś ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/apis/extensions/v1beta1/conversion.go <#41116>: > @@ -219,6 +225,73 @@ func Convert_v1beta1_RollingUpdateDeployment_To_extensions_RollingUpdateDeployme return nil } +func Convert_extensions_DaemonSetSpec_To_v1beta1_DaemonSetSpec(in *extensions.DaemonSetSpec, out *DaemonSetSpec, s conversion.Scope) error { Hmm, I had some errors with validation earlier and thought that it will help. It turns out that only Convert_extensions_RollingUpdateDaemonSet_To_v1beta1_RollingUpdateDaemonSet Convert_v1beta1_RollingUpdateDaemonSet_To_extensions_ RollingUpdateDaemonSet, are required — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#41116>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADuFf8XNL153xm7TX44rft-Fa7XnBV0mks5radz-gaJpZM4L6TEV> .

0xmichalis · 2017-02-24T22:48:14Z

We need more e2e tests and kubectl rollout status should probably respect strategy params and quit as soon as the minimum available pods are updated. Fine with a follow-up

lgtm

lukaszo · 2017-02-26T23:36:56Z

@k8s-bot non-cri e2e test this

lukaszo · 2017-02-26T23:48:47Z

@k8s-bot gce etcd3 e2e test this

It implements https://github.com/kubernetes/community/blob/master/contributors/design-proposals/daemonset-update.md Feature kubernetes/enhancements#124

lukaszo · 2017-02-27T08:19:28Z

Nothing changed, just rebased and updated generated part.

k8s-github-robot · 2017-02-27T08:30:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: janetkuo, k8s-merge-robot, lukaszo

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

OWNERS
~~pkg/controller/daemon/OWNERS~~ [janetkuo]
~~pkg/controller/deployment/OWNERS~~ [janetkuo]
~~pkg/kubectl/OWNERS~~ [janetkuo]
~~test/OWNERS~~ [janetkuo]

We suggest the following people:
cc @smarterclayton
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

0xmichalis · 2017-02-27T08:35:44Z

pkg/apis/extensions/v1beta1/types.go

@@ -508,8 +507,20 @@ type DaemonSet struct {
 	// More info: http://releases.k8s.io/HEAD/docs/devel/api-conventions.md#spec-and-status
 	// +optional
 	Status DaemonSetStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"`
+
+	// A sequence number representing a specific generation of the template.
+	// Populated by the system. Read-only.


This is not entirely true since users can populate it at creation time, right?

I will prepare a follow up PR with a fix

0xmichalis · 2017-02-27T08:50:22Z

pkg/controller/daemon/update.go

+		}
+		available := false
+		for _, pod := range daemonPods {
+			if v1.IsPodAvailable(pod, ds.Spec.MinReadySeconds, metav1.Now()) {


getNodesToDaemonPods does not filter out terminating pods and I don't think IsPodAvailable filter them out either. You should consider them as unavailable so before this check you probably want to continue on deletiontimestamp != nil.

0xmichalis · 2017-02-27T08:53:18Z

@k8s-bot unit test this issue #39471

0xmichalis · 2017-02-27T08:54:34Z

P2 because it has been rebased a couple of times already and needs to land sooner than most of the PRs in the queue.

lukaszo · 2017-02-27T11:51:34Z

@k8s-bot unit test this

k8s-github-robot · 2017-02-27T12:10:22Z

Automatic merge from submit-queue (batch tested with PRs 41116, 41804, 42104, 42111, 42120)

@Kargakis

Automatic merge from submit-queue (batch tested with PRs 42024, 42780, 42808, 42640) kubectl: respect DaemonSet strategy parameters for rollout status It handles "after-merge" comments from kubernetes#41116 cc @Kargakis @janetkuo I will add one more e2e test later. I need to handle some in company stuff.

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

k8s-github-robot assigned soltysh Feb 8, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Feb 8, 2017

lukaszo mentioned this pull request Feb 8, 2017

[WiP] DaemonSet updates #31693

Closed

0xmichalis 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 Feb 8, 2017

0xmichalis reviewed Feb 8, 2017

View reviewed changes

0xmichalis self-assigned this Feb 8, 2017

janetkuo self-assigned this Feb 8, 2017

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

lukaszo added 5 commits February 27, 2017 09:15

DaemonSet updates

b27308c

It implements https://github.com/kubernetes/community/blob/master/contributors/design-proposals/daemonset-update.md Feature kubernetes/enhancements#124

Implement kubectl rollout status for daemon sets

b59ecde

Move TemplateGeneration to the Spec

620310c

Moved tests and fixed typs

b2d9f99

Generated part for DaemonSet updates

7d9c817

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 Feb 27, 2017

0xmichalis reviewed Feb 27, 2017

View reviewed changes

0xmichalis added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/P2 labels Feb 27, 2017

k8s-github-robot merged commit 73c9fd8 into kubernetes:master Feb 27, 2017

spxtr mentioned this pull request Feb 27, 2017

ci-kubernetes-e2e-gci-gce-proto: broken test run #42100

Closed

idvoretskyi mentioned this pull request Feb 27, 2017

DaemonSet updates kubernetes/enhancements#124

Closed

23 tasks

lukaszo mentioned this pull request Mar 7, 2017

kubectl: respect DaemonSet strategy parameters for rollout status #42640

Merged

errordeveloper mentioned this pull request Apr 4, 2017

DaemonSet updates landed in 1.6 weaveworks/weave#2879

Closed

wking mentioned this pull request Apr 21, 2021

Fix RollingUpdateDaemonSet godoc regarding rounding #101296

Merged

DaemonSet updates - take 2 #41116

DaemonSet updates - take 2 #41116

Conversation

lukaszo commented Feb 8, 2017 • edited

k8s-reviewable commented Feb 8, 2017

0xmichalis commented Feb 8, 2017

Choose a reason for hiding this comment

janetkuo Feb 9, 2017 • edited

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

janetkuo Feb 10, 2017 • edited

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

idvoretskyi commented Feb 8, 2017

0xmichalis commented Feb 8, 2017 via email

0xmichalis commented Feb 24, 2017

lukaszo commented Feb 26, 2017

lukaszo commented Feb 26, 2017

lukaszo commented Feb 27, 2017

k8s-github-robot commented Feb 27, 2017

Choose a reason for hiding this comment

lukaszo Feb 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Feb 27, 2017

0xmichalis commented Feb 27, 2017 • edited

lukaszo commented Feb 27, 2017

k8s-github-robot commented Feb 27, 2017

lukaszo commented Feb 8, 2017 •

edited

janetkuo Feb 9, 2017 •

edited

janetkuo Feb 10, 2017 •

edited

lukaszo Feb 27, 2017 •

edited

0xmichalis commented Feb 27, 2017 •

edited