-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Controller history #45867
Conversation
@erictune to review or delegate. |
@kubernetes/sig-apps-pr-reviews |
@@ -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. |
There was a problem hiding this comment.
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"} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
4fd98f2
to
c661109
Compare
"k8s.io/kubernetes/pkg/registry/cachesize" | ||
) | ||
|
||
// REST implements a RESTStorage for AnyState |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AnyState -> ControllerRevision
d1a2d6d
to
5db06a9
Compare
2fad6b1
to
a5b8901
Compare
@@ -0,0 +1,205 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: strategy_test
@@ -57,6 +57,8 @@ func addKnownTypes(scheme *runtime.Scheme) error { | |||
&Scale{}, | |||
&StatefulSet{}, | |||
&StatefulSetList{}, | |||
&ControllerRevision{}, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
1e30a0f
to
7fb5c87
Compare
I'm ok with having these be in beta AS LONG AS we indicate that controller
revisions may change. this is halfway between alpha and beta - the right
thing to do is ensure users understand we may change how updates work. we
should think a bit before we ship about how we'll deal with a breaking
field change in 1.8 on controller history
…On Thu, May 18, 2017 at 10:02 AM, Kenneth Owens ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/apis/apps/v1beta1/register.go
<#45867 (comment)>
:
> @@ -57,6 +57,8 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&Scale{},
&StatefulSet{},
&StatefulSetList{},
+ &ControllerRevision{},
The only issue with doing that is that StatefulSet and DaemonSet update
will break if the API's aren't enabled by default.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45867 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pyZC6YNqhtDBys_3bCS7H8DMMAAQks5r7E-SgaJpZM4Nb-Wp>
.
|
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"))...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be zero?
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
kubernetes/pkg/apis/apps/v1beta1/types.go
Line 262 in 4def5ad
// 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.
@k8s-bot pull-kubernetes-unit test this |
1 similar comment
@k8s-bot pull-kubernetes-unit test this |
9a7d008
to
ffd4d3e
Compare
pull-kubernetes-federation-e2e-gce is flaky: #45978 |
Remove the short name and this LGTM. Other reviewer comments? |
LGTM as well. We can merge this first and resolve #45867 (comment) in follow-up PRs if we need to |
ffd4d3e
to
1457966
Compare
I don't understand how the follow up PR is easier for removing short names. |
1457966
to
ba128e6
Compare
@smarterclayton @janetkuo was talking about potentially making the validation enforce a strictly positive revision vs a non-negative. |
@k8s-bot pull-kubernetes-node-e2e test this |
2 similar comments
@k8s-bot pull-kubernetes-node-e2e test this |
@k8s-bot pull-kubernetes-node-e2e test this |
/lgtm |
@kow3ns: The following test(s) failed:
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-bot pull-kubernetes-node-e2e test this |
/approve |
[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 |
Automatic merge from submit-queue (batch tested with PRs 46429, 46308, 46395, 45867, 45492) |
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 ```
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