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.

Sign up for GitHub

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

Jump to bottom

Implement kubectl rollout undo and history for DaemonSet #46144

Merged
merged 1 commit into from
Jun 5, 2017
Merged

Implement kubectl rollout undo and history for DaemonSet #46144

merged 1 commit into from
Jun 5, 2017

Conversation

janetkuo
Copy link
Member

@janetkuo janetkuo commented May 19, 2017 •
edited
Loading

Depends on #45924, only the 2nd commit needs review (merged)

Ref kubernetes/community#527

TODOs:

  • kubectl rollout history
    • sort controller history, print overview (with revision number and change cause)
    • print detail view (content of a history)
      • print template
      • (do we need to?) print labels and annotations
  • kubectl rollout undo:
    • 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
    • update the ds using the history to rollback to
      • replace the ds template with history's
      • (do we need to?) replace the ds labels and annotations with history's
  • test-cmd.sh

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

Release note:

@janetkuo janetkuo added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 19, 2017
@janetkuo janetkuo added this to the v1.7 milestone May 19, 2017
@janetkuo janetkuo self-assigned this May 19, 2017
@janetkuo janetkuo mentioned this pull request May 19, 2017
Merged
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 19, 2017
@janetkuo janetkuo mentioned this pull request May 19, 2017
Merged
16 tasks
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2017
@lukaszo lukaszo mentioned this pull request May 22, 2017
Closed
5 tasks
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 24, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2017
@janetkuo janetkuo assigned lukaszo and 0xmichalis and unassigned janetkuo May 24, 2017
@janetkuo janetkuo force-pushed the kubectl-rollout-ds branch 4 times, most recently from b143207 to 3654c13 Compare May 25, 2017 01:01
@janetkuo
Copy link
Member Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@janetkuo janetkuo force-pushed the kubectl-rollout-ds branch 2 times, most recently from 91ded34 to 6e4a4f6 Compare May 26, 2017 07:20
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed kind/new-api labels May 26, 2017
@janetkuo
Copy link
Member Author

janetkuo commented May 30, 2017 •
edited
Loading

In controller code, annotations of a DS is copied to its history (upon history creation time). Does it make sense to copy annotations from a history to a DS when rolling back? Note that a history is reused if its template matches, which means we won't have two snapshots of a DS with the same template, even if their annotations are different.

Edit: history's labels shouldn't be copied from DS labels, but from template labels (so that it'll be selected by the DS selector)

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2017
@janetkuo janetkuo added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 1, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, mengqiy, smarterclayton

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

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

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@janetkuo
Copy link
Member Author

janetkuo commented Jun 1, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this
@k8s-bot pull-kubernetes-node-e2e test this
@k8s-bot pull-kubernetes-e2e-kops-aws test this

@janetkuo
Copy link
Member Author

janetkuo commented Jun 1, 2017

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

erictune
erictune reviewed Jun 1, 2017
View reviewed changes
pkg/kubectl/rollback.go
return nil
}
ds.Spec.Template = *toTemplate
_, err = r.c.Extensions().DaemonSets(ds.Namespace).Update(ds)
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 Update be made conditional on the resource version being the same as before, in case there is a race between Get and Update?

Copy link
Member

Choose a reason for hiding this comment

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

Unlikely, so no need to fix in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the resource version isn't the same, Update will fail (return conflict error)

Copy link
Member

Choose a reason for hiding this comment

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

Optimistic locking which is used by Update(PUT) should take care of that.

@mengqiy
Copy link
Member

mengqiy commented Jun 2, 2017

@k8s-bot pull-kubernetes-e2e-kops-aws test this

k8s-github-robot pushed a commit that referenced this pull request Jun 3, 2017
Merge pull request #45924 from janetkuo/daemonset-history
dbd1503 
Automatic merge from submit-queue

Implement Daemonset history

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

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

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

---

TODOs:
- [x] API changes
  - [x] (maybe) Remove rollback subresource if we decide to do client-side rollback 
- [x] deployment controller 
  - [x] controller revision
    - [x] owner ref (claim & adoption)
    - [x] history reconstruct (put revision number, hash collision avoidance)
    - [x] de-dup history and relabel pods
    - [x] compare ds template with history 
  - [x] hash labels (put it in controller revision, pods, and maybe deployment)
  - [x] clean up old history 
  - [x] Rename status.uniquifier when we reach consensus in #44774 
- [x] e2e tests 
- [x] unit tests 
  - [x] daemoncontroller_test.go 
  - [x] update_test.go 
  - [x] ~(maybe) storage_test.go // if we do server side rollback~

kubectl part is in #46144

--- 

**Release note**:

```release-note
```
@janetkuo
Copy link
Member Author

janetkuo commented Jun 4, 2017

Rebased

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. 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. labels Jun 4, 2017
@janetkuo janetkuo 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. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 4, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 4, 2017
@janetkuo janetkuo removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jun 4, 2017
@shashidharatd
Copy link

/retest

@janetkuo
Copy link
Member Author

janetkuo commented Jun 4, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

Unrelated federation build failure

@janetkuo
Copy link
Member Author

janetkuo commented Jun 4, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

1 similar comment
@mengqiy
Copy link
Member

mengqiy commented Jun 4, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@janetkuo
Copy link
Member Author

janetkuo commented Jun 4, 2017 •
edited
Loading

All recent pull-kubernetes-federation-e2e-gce across PRs failed: https://k8s-gubernator.appspot.com/builds/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-federation-e2e-gce 🤷‍♀️

@k8s-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce edabdac 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.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45871, 46498, 46729, 46144, 46804)

@k8s-github-robot k8s-github-robot merged commit bdf9dc1 into kubernetes:master Jun 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erictune erictune erictune left review comments

@mengqiy mengqiy mengqiy left review comments

Assignees

@lukaszo lukaszo

@mengqiy mengqiy

@0xmichalis 0xmichalis

@erictune erictune

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
9 participants
@janetkuo @smarterclayton @mengqiy @k8s-github-robot @shashidharatd @k8s-ci-robot @erictune @lukaszo @0xmichalis