Define PodDisruptionBudget API type.#24697
Define PodDisruptionBudget API type.#24697k8s-github-robot merged 2 commits intokubernetes:masterfrom
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
|
@mqliang can you please confirm as the bot requests? |
|
Also, I guess the way I squashed the history here has obscured who wrote what. Should I separate my part of the change somehow to clarify that? Not sure who to ask, so I pick @davidopp |
|
@davidopp Never mind that last question. git user error. |
|
@mml Actually, you can squash the commits for easier review, I am fine with that. |
pkg/api/types.go
Outdated
There was a problem hiding this comment.
I like this representation for "exactly one of these" but Deployment uses intstr.IntOrString so for consistency I would suggest you do the same (i.e. replace these two fields with a single IntOrString field).
There was a problem hiding this comment.
My reason here was that I didn't want to constantly be checking if it's an int or string, decoding the string into a number and multiplying. I preferred a back-end representation that already had the percentage decoded, at which point it made more sense to have the public representation match the back-end.
If I switch to IntOrString, I guess all consumers of this info will just do this work over and over again. It's not horrible inefficiency, but it feels wrong to be wasteful merely for consistency. Perhaps if we want consistency we should revise Deployment to use this "exactly one" approach in v2?
There was a problem hiding this comment.
It occurs to me that it's going to be tricky to infer the correct number of expected pods in order to do the multiplication when this is specified as a percentage. If the RC (or other pod controller) is lagging, we could be off by one or a lot more than one. To do a better job than that, we'd need knowledge of the controller's specs and its semantics.
Given the distributed-responsibility design of k8s, I'm not sure of a cleaner path forward than just trusting that the count of all the pods we see as the correct number of "expected" pods.
This approach would not work with DaemonSets, though, since the DS controller doesn't create pods when it doesn't believe they'll fit.
There was a problem hiding this comment.
Regarding your last comment: That's a really great point. As you alluded to, to compute the denominator for the percentage, you'd need to iterate through all controllers in the system, and add up the number of replicas in the "intended state" of all controllers' specs whose pods use this DisruptionBudget. But in fact that would still be wrong, because in theory you can have pods with no controller, that point back to a DisruptionBudget. The denominator is fundamentally not well-defined.
I don't think it works to use the number of pods you see as the "expected" number of pods, because then by definition 100% will always be available (or at least existing, if not available in the sense of "up"). (Though maybe I misunderstood that part of your comment.)
I guess we should just do MinAvailable (an int), and expect that the config system would have templating that would allow you to say "MinAvailable = 0.8 * X" (e.g. you use X in your PodTemplate to set number of replicas) and the value would get filled in when the config is evaluated...
|
I think you should rename the type to PodDisruptionBudget. You could imagine having something similar for nodes (i.e. NodeDisruptionBudget) to control how many can be taken down for maintenance at a time, or the rate of taking them down. |
|
CLAs look good, thanks! |
|
@davidopp PTAL |
0719062 to
4c9fc3d
Compare
|
@davidopp We should be good to go. Keeping the actual edits separate from the codegen in the commit history requires a lot of git acrobatics, especially as the PR progresses. |
pkg/apis/policy/types.go
Outdated
There was a problem hiding this comment.
You can omit "Selector is a", but it's fine to fix this in a followup PR.
|
LGTM |
|
cc/ @bgrant0607 @lavalamp |
|
Sigh. This passes on my workstation. |
|
@kubernetes/sig-api-machinery and @kubernetes/test-infra-admins any idea what's going on here? I verified that jenkins and I are testing at the same SHA-1. |
| import "k8s.io/kubernetes/pkg/api/resource/generated.proto"; | ||
| import "k8s.io/kubernetes/pkg/api/unversioned/generated.proto"; | ||
| import "k8s.io/kubernetes/pkg/api/v1/generated.proto"; | ||
| import "k8s.io/kubernetes/pkg/apis/policy/v1alpha1/generated.proto"; |
There was a problem hiding this comment.
I think you've somehow picked up this spurious line. If you remove it and rerun update-codegen.sh, does the proto generator put it back? @smarterclayton might have more insight.
There was a problem hiding this comment.
Yes it does.
% git log -n1 -u
commit 5b5ba10a9673baa88e7a93b3527d05e444bca58b
Author: Matt Liggett <mml@google.com>
Date: Fri May 6 15:24:00 2016 -0700
wtf
diff --git a/pkg/apis/apps/v1alpha1/generated.proto b/pkg/apis/apps/v1alpha1/generated.proto
index de4d560..6cb15bf 100644
--- a/pkg/apis/apps/v1alpha1/generated.proto
+++ b/pkg/apis/apps/v1alpha1/generated.proto
@@ -24,7 +24,6 @@ package k8s.io.kubernetes.pkg.apis.apps.v1alpha1;
import "k8s.io/kubernetes/pkg/api/resource/generated.proto";
import "k8s.io/kubernetes/pkg/api/unversioned/generated.proto";
import "k8s.io/kubernetes/pkg/api/v1/generated.proto";
-import "k8s.io/kubernetes/pkg/apis/policy/v1alpha1/generated.proto";
import "k8s.io/kubernetes/pkg/util/intstr/generated.proto";
// Package-wide variables from generator "generated".
[..after re-running u-g-p..]
% git di
diff --git a/pkg/apis/apps/v1alpha1/generated.proto b/pkg/apis/apps/v1alpha1/generated.proto
index 6cb15bf..de4d560 100644
--- a/pkg/apis/apps/v1alpha1/generated.proto
+++ b/pkg/apis/apps/v1alpha1/generated.proto
@@ -24,6 +24,7 @@ package k8s.io.kubernetes.pkg.apis.apps.v1alpha1;
import "k8s.io/kubernetes/pkg/api/resource/generated.proto";
import "k8s.io/kubernetes/pkg/api/unversioned/generated.proto";
import "k8s.io/kubernetes/pkg/api/v1/generated.proto";
+import "k8s.io/kubernetes/pkg/apis/policy/v1alpha1/generated.proto";
import "k8s.io/kubernetes/pkg/util/intstr/generated.proto";
// Package-wide variables from generator "generated".
|
Go version?
|
|
|
Ok, so my second theory is that there is an unstable sort order in Crazy idea - try running "git clean -fd" on your directory and trying I'll look into the ordering in the protobufs generation again. At worst, check in the version expected and wojciech and I will look into On May 6, 2016, at 7:02 PM, Matt Liggett notifications@github.com wrote: % hack ugp [...] — |
|
OK. Same result after |
|
GCE e2e build/test passed for commit e1fa2a0. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit e1fa2a0. |
|
Automatic merge from submit-queue |
Automatic merge from submit-queue A few followups from #24697
No description provided.