Fix max pods to evict per node.

[aveshagarwal]

@ravisantoshgudimetla @ingvagabund @wjiangjay

[ingvagabund]

Other than the violation of the DRY, it looks good.

[ingvagabund]

This is the second copy of the p1.Spec.Affinity. What about:

func defaultAffinity() *v1.Affinity {
  	return &v1.Affinity{
 		PodAntiAffinity: &v1.PodAntiAffinity{
 			RequiredDuringSchedulingIgnoredDuringExecution: []v1.PodAffinityTerm{
 				{
 					LabelSelector: &metav1.LabelSelector{
 						MatchExpressions: []metav1.LabelSelectorRequirement{
 							{
 								Key:      "foo",
 								Operator: metav1.LabelSelectorOpIn,
 								Values:   []string{"bar"},
 							},
 						},
 					},
 					TopologyKey: "region",
 				},
  			},
  		},
  	}
}

p1.Spec.Affinity = defaultAffinity()
p3.Spec.Affinity = defaultAffinity()
p4.Spec.Affinity = defaultAffinity()

?

[ingvagabund]

This fixes only the second issue @wjiangjay reported. What about the first one?

[aveshagarwal]

This fixes only the second issue @wjiangjay reported. What about the first one?

I dont see any issue with having default logging at 0? In downstream, as part of the installer, I have made it configurable. so there is always an option to configure it at higher level always. Or do you think we should increase default level to some higher level?

[aveshagarwal]

Other than the violation of the DRY,

What do you mean by it?

[ingvagabund]

Other than the violation of the DRY,

What do you mean by it?

The code is repeated three times. It should not as long as it is not intentional.

[aveshagarwal]

@ingvagabund PTAL.

[ghost]

For this issue, I prefer 2 solutions here:

1. declare the default value 0 for max pods to evict per node in help info. Most easy one.
2. change the default value at least 1 when no --max-pod-to-evict-per-node option is given and also declare this in help info

Another thing is that I have no idea if the PR is for the solution 2, do I misunderstand?

[ingvagabund]

/test all

[ingvagabund]

/lgtm

[aveshagarwal]

For this issue, I prefer 2 solutions here:
declare the default value 0 for max pods to evict per node in help info. Most easy one.

Yes that's what this PR does. golang by default sets int value to 0 and this default or unset value of 0 means no affect of this flag. Or IOWs, evict to the maximum possible as determined by the algorithms.

Also it does not make any sense to set to 0, because why you would run descheduler when you dont want to evict any pods from nodes.

change the default value at least 1 when no --max-pod-to-evict-per-node option is given and also declare this in help info

that would lead to confusion IMO.

Another thing is that I have no idea if the PR is for the solution 2, do I misunderstand?

So this PR does as the solution 1.

[aveshagarwal]

I am merging this as otherwise, by default, descheduler wont work at all. I think we should have been more careful before merging PR related to max-pod-per-node. Also we can continue further discussion in the issue.