Implement Daemonset history #45924

janetkuo · 2017-05-17T00:25:26Z

~~Depends on #45867 (the 1st commit, ignore it when reviewing)~~ (already merged)

Update DaemonSet API for rollback and history
- Add revisionHistoryLimit (default 10), collisionCount, and validation code
- Add daemonset-controller-hash label, and deprecate templateGeneration
Implement DaemonSet history logic in controller
- Create controllerrevisions (history) and label pods with template hash for both RollingUpdate and OnDelete update strategy
- Clean up old, non-live history based on revisionHistoryLimit
- Remove duplicate controllerrevisions (the ones with the same template) and relabel their pods
- Update RBAC to allow DaemonSet controller to manage controllerrevisions
- In DaemonSet controller unit tests, create new pods with hash labels

Ref kubernetes/community#527 and kubernetes/community#594

@kubernetes/sig-apps-api-reviews @kubernetes/sig-apps-pr-reviews @erictune @kow3ns @lukaszo @Kargakis

TODOs:

kubectl part is in #46144

Release note:

0xmichalis · 2017-06-02T12:34:19Z

pkg/controller/daemon/update.go

+			toUpdate.Labels = make(map[string]string)
+		}
+		toUpdate.Labels[extensions.DefaultDaemonSetUniqueLabelKey] = cur.Labels[extensions.DefaultDaemonSetUniqueLabelKey]
+		_, err = dsc.kubeClient.ExtensionsV1beta1().DaemonSets(ds.Namespace).UpdateStatus(toUpdate)


We are going to invalidate label updates during status updates at some point in the future and this will break.

Will follow up

0xmichalis · 2017-06-02T12:59:36Z

pkg/controller/daemon/update.go

+	if err != nil {
+		return fmt.Errorf("couldn't get node to daemon pod mapping for daemon set %q: %v", ds.Name, err)
+	}
+	_, old, err := dsc.constructHistory(ds)


I am wondering if it's going to be cleaner to construct the history once and then pass the references in manage and cleanupHistory, instead of reconstructing it for each one of them.

Sounds good, will follow up

0xmichalis · 2017-06-02T13:00:53Z

pkg/controller/daemon/update.go

+			}
+		}
+	}
+	// Label ds with current history's unique label as well


Why do we need this actually?

It's easier for us to check if pods are up-to-date without looking though all history again when calculating DS status

0xmichalis · 2017-06-02T13:02:20Z

pkg/controller/daemon/daemoncontroller.go

 		}
 		if err != nil {
 			return err
 		}
 	}

+	err = dsc.cleanupHistory(ds)


Has rollingUpdate finished the whole update at this point?

Yes, rollingUpdate calls syncNodes which creates & deletes pods and waits for those operations to complete

Does this mean that a stuck update will hold a worker forever?

Create & delete pods won't wait forever. If any creation/deletion fails, it collects the error, and returns all errors.

Successful creates / deletes doesn't mean complete rollout so rollingUpdate hasn't actually finished the whole rollout at this point in some cases.

I see. Is the concern about not killing old history when it has a pod that is terminating?

I don't see this as an issue for now but if we standarize on having cleanup being executed at the end of a rollout, we should fix this. As I said elsewhere, I am going to rollback the Deployment case now that we have collisionCount (remember that cleanup was executed at the end before and I think it's strictly correct to do so).

As an example, Jenkins is cleaning up old builds only after the new builds are complete.

0xmichalis · 2017-06-02T17:44:12Z

pkg/controller/daemon/daemoncontroller.go

+		DeleteFunc: dsc.deleteHistory,
+	})
+	dsc.historyLister = historyInformer.Lister()
+	dsc.historyStoreSynced = historyInformer.Informer().HasSynced


Don't you want to wait for the history cache to be synced?

Yes, will add in WaitForCacheSync

1. Add revisionHistoryLimit (default 10), collisionCount, and validation code 2. Add daemonset-controller-hash label, and deprecate templateGeneration

1. Create controllerrevisions (history) and label pods with template hash for both RollingUpdate and OnDelete update strategy 2. Clean up old, non-live history based on revisionHistoryLimit 3. Remove duplicate controllerrevisions (the ones with the same template) and relabel their pods 4. Update RBAC to allow DaemonSet controller to manage controllerrevisions 5. In DaemonSet controller unit tests, create new pods with hash labels

janetkuo · 2017-06-03T07:53:55Z

Rebased.

janetkuo · 2017-06-03T20:38:18Z

This blocks #46144

janetkuo · 2017-06-03T23:05:12Z

@k8s-bot pull-kubernetes-unit test this

k8s-github-robot · 2017-06-03T23:52:37Z

Automatic merge from submit-queue

@erictune

Automatic merge from submit-queue (batch tested with PRs 45871, 46498, 46729, 46144, 46804) Implement kubectl rollout undo and history for DaemonSet ~Depends on #45924, only the 2nd commit needs review~ (merged) Ref kubernetes/community#527 TODOs: - [x] kubectl rollout history - [x] sort controller history, print overview (with revision number and change cause) - [x] print detail view (content of a history) - [x] print template - [x] ~(do we need to?) print labels and annotations~ - [x] kubectl rollout undo: - [x] list controller history, figure out which revision to rollback to - if toRevision == 0, rollback to the latest revision, otherwise choose the history with matching revision - [x] update the ds using the history to rollback to - [x] replace the ds template with history's - [x] ~(do we need to?) replace the ds labels and annotations with history's~ - [x] test-cmd.sh @kubernetes/sig-apps-pr-reviews @erictune @kow3ns @lukaszo @Kargakis @kubernetes/sig-cli-maintainers --- **Release note**: ```release-note ```

janetkuo added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 17, 2017

janetkuo added this to the v1.7 milestone May 17, 2017

janetkuo self-assigned this May 17, 2017

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

janetkuo force-pushed the daemonset-history branch from 2950cc8 to 41708f9 Compare May 17, 2017 21:35

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

janetkuo force-pushed the daemonset-history branch 5 times, most recently from 425f07a to c50dbb5 Compare May 18, 2017 22:03

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

janetkuo force-pushed the daemonset-history branch 2 times, most recently from e533574 to 1709a25 Compare May 18, 2017 23:16

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

janetkuo force-pushed the daemonset-history branch 3 times, most recently from 7613856 to 43e6b50 Compare May 19, 2017 22:10

janetkuo mentioned this pull request May 19, 2017

Implement kubectl rollout undo and history for DaemonSet #46144

Merged

11 tasks

janetkuo force-pushed the daemonset-history branch from 43e6b50 to 82c3627 Compare May 19, 2017 22:15

janetkuo changed the title ~~[WIP] Implement Daemonset history~~ Implement Daemonset history May 19, 2017

janetkuo assigned lukaszo, 0xmichalis, erictune and kow3ns and unassigned janetkuo May 19, 2017

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

0xmichalis reviewed Jun 2, 2017

View reviewed changes

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

janetkuo added 5 commits June 3, 2017 00:43

Update DaemonSet API for rollback and history

8275e8f

1. Add revisionHistoryLimit (default 10), collisionCount, and validation code 2. Add daemonset-controller-hash label, and deprecate templateGeneration

Autogen: run hack/update-all.sh

4e6f70f

Test both strategies in all daemonSet controller unit tests

d2cf00f

Verify histories and pods in DaemonSet e2e test

85ec49c

janetkuo force-pushed the daemonset-history branch from 997f8b9 to 85ec49c Compare June 3, 2017 07:53

janetkuo added the queue/blocks-others label Jun 3, 2017

k8s-github-robot merged commit dbd1503 into kubernetes:master Jun 3, 2017

madhusudancs mentioned this pull request Jun 4, 2017

Flake: [Federation] Daemonset history CRUD verification failures in e2e #46925

Closed

janetkuo mentioned this pull request Jun 5, 2017

Additional tests and bug fixes for DaemonSet history #46981

Closed

6 tasks

KnVerey mentioned this pull request Jan 20, 2020

Use metadata.generation instead of deprecated attributes Shopify/krane#671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Daemonset history #45924

Implement Daemonset history #45924

janetkuo commented May 17, 2017 •

edited

Loading

0xmichalis Jun 2, 2017

0xmichalis Jun 2, 2017

janetkuo Jun 2, 2017

0xmichalis Jun 2, 2017

janetkuo Jun 2, 2017

0xmichalis Jun 2, 2017

janetkuo Jun 2, 2017

0xmichalis Jun 2, 2017

janetkuo Jun 2, 2017

0xmichalis Jun 2, 2017

janetkuo Jun 2, 2017

0xmichalis Jun 2, 2017 •

edited

Loading

janetkuo Jun 2, 2017

0xmichalis Jun 2, 2017

0xmichalis Jun 2, 2017

0xmichalis Jun 2, 2017

janetkuo Jun 2, 2017

janetkuo commented Jun 3, 2017 •

edited

Loading

janetkuo commented Jun 3, 2017

janetkuo commented Jun 3, 2017

k8s-github-robot commented Jun 3, 2017

Implement Daemonset history #45924

Implement Daemonset history #45924

Conversation

janetkuo commented May 17, 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

0xmichalis Jun 2, 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

janetkuo commented Jun 3, 2017 • edited Loading

janetkuo commented Jun 3, 2017

janetkuo commented Jun 3, 2017

k8s-github-robot commented Jun 3, 2017

janetkuo commented May 17, 2017 •

edited

Loading

0xmichalis Jun 2, 2017 •

edited

Loading

janetkuo commented Jun 3, 2017 •

edited

Loading