mirror of
https://github.com/k3s-io/kubernetes.git
synced 2025-07-25 12:43:23 +00:00
Merge pull request #54411 from frodenas/kubectl-pdb-fix
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Fixes kubectl Pod Disruption Budget and adds tests **What this PR does / why we need it**: This PR fixes several kubectl Pod Disruption Budget issues: * The `min-available` parameter in Pod Disruption Budget V1 is not required (otherwise it will never use the default value) * Removes the deprecated `min-available` default in Pod Disruption Budget V2 * The `selector` parameter in Pod Disruption Budget V2 is required * Fixes (typo) the `max-unavailable` parameter check in Pod Disruption Budget V2 * Fixes some assertion error messages where the value printed was always the zero value of the parameter type instead of the parameter provided * Updated kubectl Pod Disruption Budget to use policy V1Beta1 instead of unversioned API (see https://github.com/kubernetes/kubectl/issues/90) * Add missing tests **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes https://github.com/kubernetes/kubectl/issues/101 **Special notes for your reviewer**: Spllited in several commits to make the review process easier. Please let me know if you prefer to squash some commits or if you prefer to split them in different PRs. **Release note**: ```release-note NONE ``` /cc @kubernetes/sig-cli-pr-reviews
This commit is contained in:
Kubernetes Submit Queue
GitHub
commit
ad750b60b2
@ -38,7 +38,7 @@ var _ StructuredGenerator = &PodDisruptionBudgetV1Generator{}
|
||||
func (PodDisruptionBudgetV1Generator) ParamNames() []GeneratorParam {
|
||||
return []GeneratorParam{
|
||||
{"name", true},
|
||||
{"min-available", true},
|
||||
{"min-available", false},
|
||||
{"selector", true},
|
||||
}
|
||||
}
|
||||
@ -50,15 +50,15 @@ func (s PodDisruptionBudgetV1Generator) Generate(params map[string]interface{})
|
||||
}
|
||||
name, isString := params["name"].(string)
|
||||
if !isString {
|
||||
return nil, fmt.Errorf("expected string, saw %v for 'name'", name)
|
||||
return nil, fmt.Errorf("expected string, found %T for 'name'", params["name"])
|
||||
}
|
||||
minAvailable, isString := params["min-available"].(string)
|
||||
if !isString {
|
||||
return nil, fmt.Errorf("expected string, found %v", minAvailable)
|
||||
return nil, fmt.Errorf("expected string, found %T for 'min-available'", params["min-available"])
|
||||
}
|
||||
selector, isString := params["selector"].(string)
|
||||
if !isString {
|
||||
return nil, fmt.Errorf("expected string, found %v", selector)
|
||||
return nil, fmt.Errorf("expected string, found %T for 'selector'", params["selector"])
|
||||
}
|
||||
delegate := &PodDisruptionBudgetV1Generator{Name: name, MinAvailable: minAvailable, Selector: selector}
|
||||
return delegate.StructuredGenerate()
|
||||
@ -122,7 +122,7 @@ func (PodDisruptionBudgetV2Generator) ParamNames() []GeneratorParam {
|
||||
{"name", true},
|
||||
{"min-available", false},
|
||||
{"max-unavailable", false},
|
||||
{"selector", false},
|
||||
{"selector", true},
|
||||
}
|
||||
}
|
||||
|
||||
@ -134,22 +134,22 @@ func (s PodDisruptionBudgetV2Generator) Generate(params map[string]interface{})
|
||||
|
||||
name, isString := params["name"].(string)
|
||||
if !isString {
|
||||
return nil, fmt.Errorf("expected string, saw %v for 'name'", name)
|
||||
return nil, fmt.Errorf("expected string, found %T for 'name'", params["name"])
|
||||
}
|
||||
|
||||
minAvailable, isString := params["min-available"].(string)
|
||||
if !isString {
|
||||
return nil, fmt.Errorf("expected string, found %v", minAvailable)
|
||||
return nil, fmt.Errorf("expected string, found %T for 'min-available'", params["min-available"])
|
||||
}
|
||||
|
||||
maxUnavailable, isString := params["max-available"].(string)
|
||||
maxUnavailable, isString := params["max-unavailable"].(string)
|
||||
if !isString {
|
||||
return nil, fmt.Errorf("expected string, found %v", maxUnavailable)
|
||||
return nil, fmt.Errorf("expected string, found %T for 'max-unavailable'", params["max-unavailable"])
|
||||
}
|
||||
|
||||
selector, isString := params["selector"].(string)
|
||||
if !isString {
|
||||
return nil, fmt.Errorf("expected string, found %v", selector)
|
||||
return nil, fmt.Errorf("expected string, found %T for 'selector'", params["selector"])
|
||||
}
|
||||
delegate := &PodDisruptionBudgetV2Generator{Name: name, MinAvailable: minAvailable, MaxUnavailable: maxUnavailable, Selector: selector}
|
||||
return delegate.StructuredGenerate()
|
||||
@ -204,7 +204,7 @@ func (s *PodDisruptionBudgetV2Generator) validate() error {
|
||||
return fmt.Errorf("a selector must be specified")
|
||||
}
|
||||
if len(s.MaxUnavailable) == 0 && len(s.MinAvailable) == 0 {
|
||||
return fmt.Errorf("one of min-available/max-available must be specified")
|
||||
return fmt.Errorf("one of min-available or max-unavailable must be specified")
|
||||
}
|
||||
if len(s.MaxUnavailable) > 0 && len(s.MinAvailable) > 0 {
|
||||
return fmt.Errorf("min-available and max-unavailable cannot be both specified")
|
||||
|
||||
@ -17,114 +17,322 @@ limitations under the License.
|
||||
package kubectl
|
||||
|
||||
import (
|
||||
"reflect"
|
||||
"testing"
|
||||
|
||||
policy "k8s.io/api/policy/v1beta1"
|
||||
apiequality "k8s.io/apimachinery/pkg/api/equality"
|
||||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
|
||||
"k8s.io/apimachinery/pkg/util/intstr"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestPodDisruptionBudgetV2Generator(t *testing.T) {
|
||||
minAvailableNumber := intstr.Parse("2")
|
||||
minAvailablePercent := intstr.Parse("50%")
|
||||
func TestPodDisruptionBudgetV1Generate(t *testing.T) {
|
||||
name := "foo"
|
||||
minAvailable := "5"
|
||||
minAvailableIS := intstr.Parse(minAvailable)
|
||||
defaultMinAvailableIS := intstr.Parse("1")
|
||||
selector := "app=foo"
|
||||
labelSelector, err := metav1.ParseToLabelSelector(selector)
|
||||
if err != nil {
|
||||
t.Errorf("unexpected error: %v", err)
|
||||
}
|
||||
|
||||
tests := map[string]struct {
|
||||
params map[string]interface{}
|
||||
expected *policy.PodDisruptionBudget
|
||||
expectErr bool
|
||||
params map[string]interface{}
|
||||
expectErrMsg string
|
||||
expectPDB *policy.PodDisruptionBudget
|
||||
}{
|
||||
"test valid case with number min-available": {
|
||||
"test-valid-use": {
|
||||
params: map[string]interface{}{
|
||||
"name": "foo",
|
||||
"selector": "app=nginx",
|
||||
"min-available": "2",
|
||||
"max-available": "",
|
||||
"name": name,
|
||||
"min-available": minAvailable,
|
||||
"selector": selector,
|
||||
},
|
||||
expected: &policy.PodDisruptionBudget{
|
||||
expectPDB: &policy.PodDisruptionBudget{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "foo",
|
||||
Name: name,
|
||||
},
|
||||
Spec: policy.PodDisruptionBudgetSpec{
|
||||
MinAvailable: &minAvailableNumber,
|
||||
Selector: &metav1.LabelSelector{
|
||||
MatchLabels: map[string]string{
|
||||
"app": "nginx",
|
||||
},
|
||||
},
|
||||
MinAvailable: &minAvailableIS,
|
||||
Selector: labelSelector,
|
||||
},
|
||||
},
|
||||
expectErr: false,
|
||||
},
|
||||
"test valid case with percent min-available": {
|
||||
"test-missing-name-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": "foo",
|
||||
"selector": "app=nginx",
|
||||
"min-available": "50%",
|
||||
"max-available": "",
|
||||
"min-available": minAvailable,
|
||||
"selector": selector,
|
||||
},
|
||||
expected: &policy.PodDisruptionBudget{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "foo",
|
||||
},
|
||||
Spec: policy.PodDisruptionBudgetSpec{
|
||||
MinAvailable: &minAvailablePercent,
|
||||
Selector: &metav1.LabelSelector{
|
||||
MatchLabels: map[string]string{
|
||||
"app": "nginx",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
expectErr: false,
|
||||
expectErrMsg: "Parameter: name is required",
|
||||
},
|
||||
"test missing required param": {
|
||||
"test-blank-name-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": "foo",
|
||||
"min-available": "2",
|
||||
"max-available": "",
|
||||
"name": "",
|
||||
"min-available": minAvailable,
|
||||
"selector": selector,
|
||||
},
|
||||
expectErr: true,
|
||||
expectErrMsg: "Parameter: name is required",
|
||||
},
|
||||
"test with invalid format params": {
|
||||
"test-invalid-name-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": "foo",
|
||||
"selector": "app=nginx",
|
||||
"min-available": 2,
|
||||
"max-available": "",
|
||||
"name": 1,
|
||||
"min-available": minAvailable,
|
||||
"selector": selector,
|
||||
},
|
||||
expectErr: true,
|
||||
expectErrMsg: "expected string, found int for 'name'",
|
||||
},
|
||||
"test min-available/max-available all not be specified": {
|
||||
"test-missing-min-available-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": "foo",
|
||||
"selector": "app=nginx",
|
||||
"name": name,
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "expected string, found <nil> for 'min-available'",
|
||||
},
|
||||
"test-blank-min-available-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": "",
|
||||
"max-available": "",
|
||||
"selector": selector,
|
||||
},
|
||||
expectPDB: &policy.PodDisruptionBudget{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: name,
|
||||
},
|
||||
Spec: policy.PodDisruptionBudgetSpec{
|
||||
MinAvailable: &defaultMinAvailableIS,
|
||||
Selector: labelSelector,
|
||||
},
|
||||
},
|
||||
expectErr: true,
|
||||
},
|
||||
"test min-available and max-unavailable cannot be both specified": {
|
||||
"test-invalid-min-available-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": "foo",
|
||||
"selector": "app=nginx",
|
||||
"min-available": "2",
|
||||
"max-available": "5",
|
||||
"name": name,
|
||||
"min-available": 1,
|
||||
"selector": selector,
|
||||
},
|
||||
expectErr: true,
|
||||
expectErrMsg: "expected string, found int for 'min-available'",
|
||||
},
|
||||
"test-missing-selector-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": minAvailable,
|
||||
},
|
||||
expectErrMsg: "Parameter: selector is required",
|
||||
},
|
||||
"test-blank-selector-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": minAvailable,
|
||||
"selector": "",
|
||||
},
|
||||
expectErrMsg: "Parameter: selector is required",
|
||||
},
|
||||
"test-invalid-selector-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": minAvailable,
|
||||
"selector": 1,
|
||||
},
|
||||
expectErrMsg: "expected string, found int for 'selector'",
|
||||
},
|
||||
}
|
||||
|
||||
generator := PodDisruptionBudgetV1Generator{}
|
||||
for name, test := range tests {
|
||||
obj, err := generator.Generate(test.params)
|
||||
switch {
|
||||
case test.expectErrMsg != "" && err != nil:
|
||||
if err.Error() != test.expectErrMsg {
|
||||
t.Errorf("test '%s': expect error '%s', but saw '%s'", name, test.expectErrMsg, err.Error())
|
||||
}
|
||||
continue
|
||||
case test.expectErrMsg != "" && err == nil:
|
||||
t.Errorf("test '%s': expected error '%s' and didn't get one", name, test.expectErrMsg)
|
||||
continue
|
||||
case test.expectErrMsg == "" && err != nil:
|
||||
t.Errorf("test '%s': unexpected error %s", name, err.Error())
|
||||
continue
|
||||
}
|
||||
if !reflect.DeepEqual(obj.(*policy.PodDisruptionBudget), test.expectPDB) {
|
||||
t.Errorf("test '%s': expected:\n%#v\nsaw:\n%#v", name, test.expectPDB, obj.(*policy.PodDisruptionBudget))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestPodDisruptionBudgetV2Generate(t *testing.T) {
|
||||
name := "foo"
|
||||
minAvailable := "1"
|
||||
minAvailableIS := intstr.Parse(minAvailable)
|
||||
maxUnavailable := "5%"
|
||||
maxUnavailableIS := intstr.Parse(maxUnavailable)
|
||||
selector := "app=foo"
|
||||
labelSelector, err := metav1.ParseToLabelSelector(selector)
|
||||
if err != nil {
|
||||
t.Errorf("unexpected error: %v", err)
|
||||
}
|
||||
|
||||
tests := map[string]struct {
|
||||
params map[string]interface{}
|
||||
expectErrMsg string
|
||||
expectPDB *policy.PodDisruptionBudget
|
||||
}{
|
||||
"test-valid-min-available": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": minAvailable,
|
||||
"max-unavailable": "",
|
||||
"selector": selector,
|
||||
},
|
||||
expectPDB: &policy.PodDisruptionBudget{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: name,
|
||||
},
|
||||
Spec: policy.PodDisruptionBudgetSpec{
|
||||
MinAvailable: &minAvailableIS,
|
||||
Selector: labelSelector,
|
||||
},
|
||||
},
|
||||
},
|
||||
"test-valid-max-available": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": "",
|
||||
"max-unavailable": maxUnavailable,
|
||||
"selector": selector,
|
||||
},
|
||||
expectPDB: &policy.PodDisruptionBudget{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: name,
|
||||
},
|
||||
Spec: policy.PodDisruptionBudgetSpec{
|
||||
MaxUnavailable: &maxUnavailableIS,
|
||||
Selector: labelSelector,
|
||||
},
|
||||
},
|
||||
},
|
||||
"test-missing-name-param": {
|
||||
params: map[string]interface{}{
|
||||
"min-available": "",
|
||||
"max-unavailable": "",
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "Parameter: name is required",
|
||||
},
|
||||
"test-blank-name-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": "",
|
||||
"min-available": "",
|
||||
"max-unavailable": "",
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "Parameter: name is required",
|
||||
},
|
||||
"test-invalid-name-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": 1,
|
||||
"min-available": "",
|
||||
"max-unavailable": "",
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "expected string, found int for 'name'",
|
||||
},
|
||||
"test-missing-min-available-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"max-unavailable": "",
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "expected string, found <nil> for 'min-available'",
|
||||
},
|
||||
"test-invalid-min-available-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": 1,
|
||||
"max-unavailable": "",
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "expected string, found int for 'min-available'",
|
||||
},
|
||||
"test-missing-max-available-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": "",
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "expected string, found <nil> for 'max-unavailable'",
|
||||
},
|
||||
"test-invalid-max-available-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": "",
|
||||
"max-unavailable": 1,
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "expected string, found int for 'max-unavailable'",
|
||||
},
|
||||
"test-blank-min-available-max-unavailable-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": "",
|
||||
"max-unavailable": "",
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "one of min-available or max-unavailable must be specified",
|
||||
},
|
||||
"test-min-available-max-unavailable-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": minAvailable,
|
||||
"max-unavailable": maxUnavailable,
|
||||
"selector": selector,
|
||||
},
|
||||
expectErrMsg: "min-available and max-unavailable cannot be both specified",
|
||||
},
|
||||
"test-missing-selector-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": "",
|
||||
"max-unavailable": "",
|
||||
},
|
||||
expectErrMsg: "Parameter: selector is required",
|
||||
},
|
||||
"test-blank-selector-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": "",
|
||||
"max-unavailable": "",
|
||||
"selector": "",
|
||||
},
|
||||
expectErrMsg: "Parameter: selector is required",
|
||||
},
|
||||
"test-invalid-selector-param": {
|
||||
params: map[string]interface{}{
|
||||
"name": name,
|
||||
"min-available": "",
|
||||
"max-unavailable": "",
|
||||
"selector": 1,
|
||||
},
|
||||
expectErrMsg: "expected string, found int for 'selector'",
|
||||
},
|
||||
}
|
||||
|
||||
generator := PodDisruptionBudgetV2Generator{}
|
||||
for name, test := range tests {
|
||||
obj, err := generator.Generate(test.params)
|
||||
if !test.expectErr && err != nil {
|
||||
t.Errorf("%s: unexpected error: %v", name, err)
|
||||
}
|
||||
if test.expectErr && err != nil {
|
||||
switch {
|
||||
case test.expectErrMsg != "" && err != nil:
|
||||
if err.Error() != test.expectErrMsg {
|
||||
t.Errorf("test '%s': expect error '%s', but saw '%s'", name, test.expectErrMsg, err.Error())
|
||||
}
|
||||
continue
|
||||
case test.expectErrMsg != "" && err == nil:
|
||||
t.Errorf("test '%s': expected error '%s' and didn't get one", name, test.expectErrMsg)
|
||||
continue
|
||||
case test.expectErrMsg == "" && err != nil:
|
||||
t.Errorf("test '%s': unexpected error %s", name, err.Error())
|
||||
continue
|
||||
}
|
||||
if !apiequality.Semantic.DeepEqual(obj.(*policy.PodDisruptionBudget), test.expected) {
|
||||
t.Errorf("%s:\nexpected:\n%#v\nsaw:\n%#v", name, test.expected, obj.(*policy.PodDisruptionBudget))
|
||||
if !reflect.DeepEqual(obj.(*policy.PodDisruptionBudget), test.expectPDB) {
|
||||
t.Errorf("test '%s': expected:\n%#v\nsaw:\n%#v", name, test.expectPDB, obj.(*policy.PodDisruptionBudget))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user