Drop deprecated flags

[ingvagabund]

To drop https://github.com/kubernetes-sigs/descheduler/blob/master/cmd/descheduler/app/options/options.go#L89-L93:

	fs.StringVar(&rs.NodeSelector, "node-selector", rs.NodeSelector, "DEPRECATED: selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2)")
	// max-no-pods-to-evict limits the maximum number of pods to be evicted per node by descheduler.
	fs.IntVar(&rs.MaxNoOfPodsToEvictPerNode, "max-pods-to-evict-per-node", rs.MaxNoOfPodsToEvictPerNode, "DEPRECATED: limits the maximum number of pods to be evicted per node by descheduler")
	// evict-local-storage-pods allows eviction of pods that are using local storage. This is false by default.
	fs.BoolVar(&rs.EvictLocalStoragePods, "evict-local-storage-pods", rs.EvictLocalStoragePods, "DEPRECATED: enables evicting pods using local storage by descheduler")

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ingvagabund]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[damemi]

I'd like to figure out our eventual plan for the componentconfig types. I started an issue for it a while back here #431, but I don't know if it's just redundant with Policy or what

[ingvagabund]

Good point. I will put those back.

[damemi]

shouldn't we keep this nil check instead of passing the variable directly?

[ingvagabund]

I check for the nil inside the pod evictor

[damemi]

I noticed that after I commented here, sorry. Still, is there any reason not to do that check earlier with the rest of the flags that are checked similarly here?

[ingvagabund]

Nil is a valid value

[damemi]

Got it, the other thread cleared that up for me too thanks

[damemi]

I believe this was checking >0 because the default of 0 = unlimited. So, removing this and making nil the default changes the existing behavior. I think we should still respect 0 as no limit

[ingvagabund]

Based on https://github.com/kubernetes-sigs/descheduler#policy-and-strategies:

The policy includes a common configuration that applies to all the strategies:

Name	Default Value	Description
nodeSelector	nil	limiting the nodes which are processed
evictLocalStoragePods	false	allows eviction of pods with local storage
evictSystemCriticalPods	false	[Warning: Will evict Kubernetes system pods] allows eviction of pods with any priority, including system pods like kube-dns
ignorePvcPods	false	set whether PVC pods should be evicted or ignored
maxNoOfPodsToEvictPerNode	nil	maximum number of pods evicted from each node (summed through all strategies)

maxNoOfPodsToEvictPerNode is nil by default and there's no mention of 0 being the unlimited case. Also, using 0 for unlimited is not intuitive since without any other information setting it to 0 is like saying "do not evict any pod from any node".

[damemi]

Ah okay, I was looking at the componentconfig types where it wasn't a pointer. So, 0 was just unlimited for the internal representation. Sounds good

[damemi]

/lgtm