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.

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

Controller history #45867

Merged
merged 1 commit into from
May 26, 2017
Merged

Conversation

kow3ns
Copy link
Member

@kow3ns kow3ns commented May 16, 2017

What this PR does / why we need it:
Implements the ControllerRevision API object and clientset to allow for the implementation of StatefulSet update and DaemonSet history

ControllerRevision type added for StatefulSet and DaemonSet history.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 16, 2017
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 16, 2017
@piosz piosz assigned erictune and unassigned piosz May 16, 2017
@piosz
Copy link
Member

piosz commented May 16, 2017

@erictune to review or delegate.

@0xmichalis
Copy link
Contributor

@kubernetes/sig-apps-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label May 16, 2017
@@ -132,3 +132,39 @@ func ValidateStatefulSetStatusUpdate(statefulSet, oldStatefulSet *apps.StatefulS
// TODO: Validate status.
return allErrs
}

// ValidateControllerRevisionName can be used to check whether the given ConfigMap name is valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ConfigMap/ControllerRevision/


// ShortNames implements the ShortNamesProvider interface. Returns a list of short names for a resource.
func (r *REST) ShortNames() []string {
return []string{"cr"}
Copy link
Contributor

Choose a reason for hiding this comment

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

@kubernetes/sig-cli-api-reviews

Copy link
Contributor

Choose a reason for hiding this comment

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

@deads2k now that short names are not consolidated I wonder how can we avoid collisions with other short names. What happens if someone checks in a short name that already exists? Do we have a test to prevent such a scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

btw do we have any documentation about how they work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kargakis sorry I wasn't clear, I meant documentation about short names server-side, how they work and how to add them.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fabianofranz oh, I thought you already knew that one:) +1 on such a doc

cc: @kubernetes/sig-api-machinery-misc

Copy link
Contributor

Choose a reason for hiding this comment

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

No short name for now, we can always add later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with the short name, @smarterclayton do you want it removed after all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, yes, I want it removed. Short names are reserved for really important "accessed all the time by end users" resources. That's not controllerrevision

@kow3ns kow3ns force-pushed the controller-history branch from 4fd98f2 to c661109 Compare May 16, 2017 14:26
@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 16, 2017
@kow3ns
Copy link
Member Author

kow3ns commented May 16, 2017

@janetkuo
@smarterclayton

"k8s.io/kubernetes/pkg/registry/cachesize"
)

// REST implements a RESTStorage for AnyState
Copy link
Member

Choose a reason for hiding this comment

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

AnyState -> ControllerRevision

*genericregistry.Store
}

// NewREST returns a RESTStorage object that will work with AnyState objects.
Copy link
Member

Choose a reason for hiding this comment

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

AnyState -> ControllerRevision

@kow3ns kow3ns force-pushed the controller-history branch 2 times, most recently from d1a2d6d to 5db06a9 Compare May 16, 2017 22:43
@janetkuo janetkuo mentioned this pull request May 17, 2017
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 17, 2017
@kow3ns kow3ns force-pushed the controller-history branch from 2fad6b1 to a5b8901 Compare May 17, 2017 04:31
@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 17, 2017
@@ -0,0 +1,205 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: strategy_test

@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 17, 2017
@@ -57,6 +57,8 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&Scale{},
&StatefulSet{},
&StatefulSetList{},
&ControllerRevision{},
Copy link
Contributor

Choose a reason for hiding this comment

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

These APIs will technically be alpha, we should probably put them in an alpha group (@lavalamp)

Copy link
Member Author

Choose a reason for hiding this comment

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

The only issue with doing that is that StatefulSet and DaemonSet update will break if the API's aren't enabled by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

As per decision on call, these are fine to leave in v1beta1, but we'll want to ensure the documentation reflects that we reserve the right to change the beta api.

@kow3ns kow3ns force-pushed the controller-history branch from 1e30a0f to 7fb5c87 Compare May 17, 2017 23:37
@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 17, 2017
@smarterclayton
Copy link
Contributor

smarterclayton commented May 18, 2017 via email

if revision.Data == nil {
errs = append(errs, field.Required(field.NewPath("data"), "data is mandatory"))
}
errs = append(errs, apivalidation.ValidateNonnegativeField(revision.Revision, field.NewPath("revision"))...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be zero?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

// Validates that given value is not negative.
func ValidateNonnegativeField(value int64, fldPath *field.Path) field.ErrorList {
	allErrs := field.ErrorList{}
	if value < 0 {
		allErrs = append(allErrs, field.Invalid(fldPath, value, IsNegativeErrorMsg))
	}
	return allErrs
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I am asking if it should be valid for a revision to equal 0. Should all controllers start setting from zero? Do we care if some users will start from one?

Copy link
Member

@janetkuo janetkuo May 24, 2017

Choose a reason for hiding this comment

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

Please consider not making it 0. In kubectl rollout undo, --to-revision=0 means rolling back to last revision without specifying a specific revision number. Deployment rollback (server side) also uses "rollback to revision 0" to roll back to the last revision.

Copy link
Member Author

@kow3ns kow3ns May 24, 2017

Choose a reason for hiding this comment

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

I think that a ControllerRevision with a .Revision of 0 is a valid object. It doesn't enforce that the end user ever creates such an object, but implementing validation based on a convention for a kubectl flag default seems to be mixing concerns imo. What if the flag is defaulted to -1 when non-present in the future, or if a nil pointer is used to indicate its absence. I don't see a strong reason to say that a ControllerRevision with .Revision == 0, is not a valid object.

Copy link
Member

Choose a reason for hiding this comment

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

Use -1 would work. Need to maintain backward compatibility when we 1) change --to-revision default value and 2) change what --to-revision=0 means.

We can make both --to-revision=-1 and =0 work for Deployment (compatible with 1.6 server). But can we change the --to-revision=0 behavior in 1.7?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not saying we should change it, I'm just saying we shouldn't constrain validation of the API Object based on it. StatefulSet and DaemonSet are both going to use 1 for the first revision, but I don't think that necessitates constraining validation of the object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that kubectl flags and their defaults are API and we are treating them like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also use 0 as "rollback to the last revision" in the Deployment API and not just kubectl:

// The revision to rollback to. If set to 0, rollbck to the last revision.

// If rollback revision is 0, rollback to the last revision

By validating zero here, we enforce the convention that rolling back to 0 means rolling back to the previous revision. I don't feel strong about it but then again I see we can have a solution that works the same across all controllers.

@kow3ns
Copy link
Member Author

kow3ns commented May 24, 2017

@k8s-bot pull-kubernetes-unit test this

1 similar comment
@kow3ns
Copy link
Member Author

kow3ns commented May 24, 2017

@k8s-bot pull-kubernetes-unit test this

@kow3ns kow3ns force-pushed the controller-history branch from 9a7d008 to ffd4d3e Compare May 24, 2017 22:56
@janetkuo
Copy link
Member

pull-kubernetes-federation-e2e-gce is flaky: #45978

@smarterclayton
Copy link
Contributor

Remove the short name and this LGTM. Other reviewer comments?

@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 25, 2017
@janetkuo
Copy link
Member

janetkuo commented May 25, 2017

LGTM as well. We can merge this first and resolve #45867 (comment) in follow-up PRs if we need to

@kow3ns kow3ns force-pushed the controller-history branch from ffd4d3e to 1457966 Compare May 25, 2017 17:29
@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 25, 2017
@smarterclayton
Copy link
Contributor

I don't understand how the follow up PR is easier for removing short names.

@kow3ns kow3ns force-pushed the controller-history branch from 1457966 to ba128e6 Compare May 25, 2017 18:39
@kow3ns
Copy link
Member Author

kow3ns commented May 25, 2017

@smarterclayton @janetkuo was talking about potentially making the validation enforce a strictly positive revision vs a non-negative.
This rebase contains a commit to remove the short name

@kow3ns
Copy link
Member Author

kow3ns commented May 25, 2017

@k8s-bot pull-kubernetes-node-e2e test this

2 similar comments
@kow3ns
Copy link
Member Author

kow3ns commented May 25, 2017

@k8s-bot pull-kubernetes-node-e2e test this

@kow3ns
Copy link
Member Author

kow3ns commented May 25, 2017

@k8s-bot pull-kubernetes-node-e2e test this

@janetkuo
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 25, 2017

@kow3ns: The following test(s) failed:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce ba128e6 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
Copy link
Member

@k8s-bot pull-kubernetes-node-e2e test this

@smarterclayton
Copy link
Contributor

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46429, 46308, 46395, 45867, 45492)

@k8s-github-robot k8s-github-robot merged commit 7d37a26 into kubernetes:master May 26, 2017
k8s-github-robot pushed a commit that referenced this pull request Jun 3, 2017
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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.