Use framework's PluginConfig for default pod spreading

And add its own feature flag

Signed-off-by: Aldo Culquicondor <[email protected]>

Author: alculquicondor

keps/sig-scheduling/20190926-default-even-pod-spreading.md

@@ -31,17 +31,19 @@ see-also:
     - [Story 1](#story-1)
     - [Story 2](#story-2)
   - [Implementation Details/Notes/Constraints](#implementation-detailsnotesconstraints)
-    - [Relationship with SelectorSpreadingPriority](#relationship-with-selectorspreadingpriority)
+    - [Feature gate](#feature-gate)
+    - [Relationship with "DefaultPodTopologySpread" plugin](#relationship-with-defaultpodtopologyspread-plugin)
   - [Risks and Mitigations](#risks-and-mitigations)
 - [Design Details](#design-details)
   - [API](#api)
-  - [Default rules](#default-rules)
+  - [Default constraints](#default-constraints)
   - [How user stories are addressed](#how-user-stories-are-addressed)
   - [Implementation Details](#implementation-details)
     - [In the metadata/predicates/priorities flow](#in-the-metadatapredicatespriorities-flow)
     - [In the scheduler framework](#in-the-scheduler-framework)
   - [Test Plan](#test-plan)
   - [Graduation Criteria](#graduation-criteria)
+    - [Alpha (v1.18):](#alpha-v118)
 - [Implementation History](#implementation-history)
 - [Alternatives](#alternatives)
 <!-- /toc -->
@@ -71,7 +73,8 @@ But if they do, they can still set their own spreading constraints if they have
 
 ## Motivation
 
-In order for a workload (pod) to use `EvenPodsSpreadPriority`:
+In order for a workload (pod) to use `.spec.topologySpreadConstraints` (known as`PodTopologySpread`
+plugin or `EvenPodsSpreadPriority` in the old Policy API):
 
 1. Authors have to have an idea of the underlying topology.
 1. PodSpecs become less portable if their spreading constraints are tailored to a specific topology.
@@ -86,12 +89,12 @@ them suitable to provide default spreading constraints for all workloads in thei
 - Workloads are spread with the default constraints if they belong to the same service, replication controller,
   replica set or stateful set, and if they don't define `pod.spec.topologySpreadConstraints`.
 - Provide a k8s default for `topologySpreadConstraints` that produces a priority equivalent to
-  `SelectorSpreadPriority`, so that this algorithm can be removed from the default algorithms' provider.
+  `DefaultPodTopologySpread`, so that this plugin can be deprecated in the future.
 
 ### Non-Goals
 
 - Set defaults for specific namespaces or according to other selectors.
-- Removal of `ServiceSpreadingPriority` or `ServiceAntiAffinity` priorities.
+- Removal of `DefaultPodTopologySpread` plugin.
 
 ## Proposal
 
@@ -101,8 +104,8 @@ them suitable to provide default spreading constraints for all workloads in thei
 
 As a cluster operator, I want to set default spreading constraints for workloads in the cluster.
 Currently, `SelectorSpreadPriority` provides a canned priority that spreads across nodes
-and zones (`failure-domain.beta.kubernetes.io/zone`). However, the nodes in my cluster have
-custom topology keys (for physical host, rack, etc.).
+and zones (`topology.kubernetes.io/zone`). However, the nodes in my cluster have custom topology
+keys (for physical host, rack, etc.).
 
 #### Story 2
 
@@ -113,87 +116,83 @@ As a workload author, I want to spread the workload in the cluster, but:
 ### Implementation Details/Notes/Constraints
 
 
-#### Relationship with SelectorSpreadingPriority
+#### Feature gate
 
-Note that Default `topologySpreadConstraints` has a similar effect to `SelectorSpreadingPriority`.
-Given that the latter is not configurable, they could return conflicting priorities, which
-may not be the intention of an operator. On the other hand, a proper Default for `topologySpreadConstraints`
-could provide the same priority as `SelectorSpreadingPriority`. Thus, there's no need for the
-features to co-exist.
+Setting a default for `PodTopologySpread` will be guarded with the feature gate
+`DefaultEvenPodsSpread`. This feature gate will depend on `EvenPodsSpread` to also be enabled.
 
-K8s will set Default `topologySpreadConstraints` and remove `SelectorSpreadingPriority`
-from the k8s providers (`DefaultProvider` and `ClusterAutoscalerProvider`). The set
-[default](#default-rules) will have a similar effect.
+#### Relationship with "DefaultPodTopologySpread" plugin
 
-If an operator sets a Policy, these are the semantics of the presence of `SelectorSpreadingPriority`
-and/or `EvenPodsSpreadPriority`:
+Note that Default `topologySpreadConstraints` has a similar effect to `DefaultPodTopologySpread`
+plugin (`SelectorSpreadingPriority` when using the Policy API).
+Given that the latter is not configurable, they could return conflicting priorities, which
+may not be the intention of the cluster operator or workload author. On the other hand, a proper
+default for `topologySpreadConstraints` could provide the same score as
+`DefaultPodTopologySpread`. Thus, there's no need for the features to co-exist.
 
-| SelectorSpreading | EvenPodsSpread | Operator constraints | Valid | Pod spread constraints        |
-| :---------------: | :------------: | :------------------: | :---: | :---------------------------: |
-| N                 | Y              | Not provided         | Yes   | [k8s default](#default-rules) |
-| N                 | Y              | Provided             | Yes   | provided by operator          |
-| Y                 | Y              | Not provided         | Yes   | [k8s default](#default-rules) |
-| Y                 | Y              | Provided             | No    | -                             |
-| N                 | N              | -                    | Yes   | None                          |
-| Y                 | N              | -                    | No    | -                             |
+When the feature gate is enabled:
 
-Selecting `SelectorSpreadingPriority` but not `EvenPodsSpreadPriority` in a policy is an invalid
-configuration, because the latter is a requirement for the former.
+- K8s will set Default `topologySpreadConstraints` and remove `DefaultPodTopologySpread` from the
+k8s providers (`DefaultProvider` and `ClusterAutoscalerProvider`). The
+[Default](#default-constraints) will have a similar effect.
+- When a policy is used, `SelectorSpreadingPriority` will map to `PodTopologySpread`.
+- When setting plugins in the Component Config API, plugins are added as requested. Since an
+  operator is manually enabling the plugins, we assume they are aware of their intentions.
 
 ### Risks and Mitigations
 
-`EvenPodsSpreadPriority` has some overhead and we currently ensure that pods that don't use the
-feature get minimally affected. After Default `topologySpreadConstraints` is rolled out,
-all pods will run through the algorithms.
-However, we should ensure that the running overhead is not significantly higher than
-`SelectorSpreadingPriority` with k8s Default `topologySpreadConstraints`.
+The `PodTopologySpread` plugin has some overhead compared to other plugins. We currently ensure that
+pods that don't use the feature get minimally affected. After Default `topologySpreadConstraints`
+is rolled out, all pods will run through the plugin.
+We should ensure that the running overhead is not significantly higher than
+`DefaultPodTopologySpread` with the k8s Default.
 
 ## Design Details
 
 ### API
 
-A new structure called `TopologySpreadConstraint` is introduced to `KubeSchedulerConfiguration`.
+A new structure `Args` is introduced in `pkg/scheduler/framework/plugins/podtopologyspread`.
+Values are decoded from the `pluginConfig` slice in the kube-scheduler Component Config and used in
+`podtopologyspread.New`.
 
 ```go
-type KubeSchedulerConfiguration struct {
-	....
-	// TopologySpreadConstraints defines topology spread constraints to be applied to pods
+// pkg/scheduler/framework/plugins/podtopologyspread/plugin.go
+type Args struct {
+	// DefaultConstraints defines topology spread constraints to be applied to pods
 	// that don't define any in `pod.spec.topologySpreadConstraints`. Pod selectors must
 	// be empty, as they are deduced from the resources that the pod belongs to
 	// (includes services, replication controllers, replica sets and stateful sets). 
 	// If not specified, the scheduler applies the following default constraints:
 	// <default rules go here. See next section>
 	// +optional
-	TopologySpreadConstraints []corev1.TopologySpreadConstraint
-	....
+	DefaultConstraints []corev1.TopologySpreadConstraint
 }
 ```
 
-Note the use of `k8s.io/api/core/v1.TopologySpreadConstraint`. During validation,
-we verify that selectors are not defined.
+Note the use of `k8s.io/api/core/v1.TopologySpreadConstraint`. During validation, we verify that
+selectors are not defined.
 
-### Default rules
+### Default constraints
 
 These will be the default constraints for the cluster when the operator doesn't provide any:
 
 ```yaml
-defaultTopologySpreadConstraints:
+defaultConstraints:
   - maxSkew: 1
     topologyKey: "kubernetes.io/hostname"
     whenUnsatisfiable: ScheduleAnyway
   - maxSkew: 1
-    topologyKey: "failure-domain.beta.kubernetes.io/zone"
+    topologyKey: "topology.kubernetes.io/zone"
     whenUnsatisfiable: ScheduleAnyway
 ```
 
 ### How user stories are addressed
 
 Let's say we have a cluster that has a topology based on physical hosts and racks.
-Then, an operator can set the following scheduler configuration:
+Then, an operator can set the following configuration for the plugin:
 
 ```yaml
-apiVersion: componentconfig/v1alpha1
-defaultTopologySpreadConstraints:
+defaultConstraints:
   - maxSkew: 5
     topologyKey: "example.com/topology/physical_host"
     whenUnsatisfiable: ScheduleAnyway
@@ -231,14 +230,14 @@ and will be applied at runtime:
 ```yaml
 topologySpreadConstraints:
   - maxSkew: 5
-    TopologyKey: "example.com/topology/physical_host"
-    WhenUnsatisfiable: ScheduleAnyway
+    topologyKey: "example.com/topology/physical_host"
+    whenUnsatisfiable: ScheduleAnyway
     selector:
       matchLabels:
         app: demo
   - maxSkew: 15
-    TopologyKey: "example.com/topology/rack"
-    WhenUnsatisfiable: DoNotSchedule
+    topologyKey: "example.com/topology/rack"
+    whenUnsatisfiable: DoNotSchedule
     selector:
       matchLabels:
         app: demo
@@ -273,21 +272,19 @@ To ensure this feature to be rolled out in high quality. Following tests are man
 
 ### Graduation Criteria
 
-Alpha (v1.17):
+#### Alpha (v1.18):
 
-- [ ] Scheduler Component Config API changes.
-- [ ] Default, validation and generated code.
-- [ ] Priority Implementation.
-- [ ] Predicate implementation.
+- [ ] Args struct for `podtopologyspread.New`.
+- [ ] Defaults and validation.
+- [ ] Score extension point implementation.
+- [ ] Filter extension point implementation.
+- [ ] Disabling `DefaultPodTopologySpread` when the feature is enabled.
 - [ ] Test cases mentioned in the [Test Plan](#test-plan).
 
-Default `topologySpreadConstaints` will graduate from Alpha along with
-[Even Pods Spreading](/keps/sig-scheduling/20190221-even-pods-spreading.md).
-They will share the same feature gate.
-
 ## Implementation History
 
 - 2019-09-26: Initial KEP sent out for review.
+- 2020-01-20: KEP updated to make use of framework's PluginConfig.
 
 ## Alternatives