First review round

jsafrane · jsafrane · commit 82cccb880c19 · 2019-04-17T12:15:55.000+02:00
diff --git a/keps/sig-storage/20190408-volume-scheduling-limits.md b/keps/sig-storage/20190408-volume-scheduling-limits.md
@@ -75,7 +75,6 @@ It is problematic for CSI (`MaxCSIVolumeCountPred`) outlined in [#730](https://g
 
 - `ResourceName` is limited to 63 characters. We must prefix `ResourceName` with unique string (such as `attachable-volumes-csi-<driver name>`) so it cannot collide with existing resources like `cpu` or `memory`. But `<driver name>` itself is up to 63 character long, so we ended up with using SHA-sums of driver name to keep the `ResourceName` unique, which is not user readable.
 - CSI driver cannot share its limits with in-tree volume plugin e.g. when running pods with AWS EBS in-tree volumes and `ebs.csi.aws.com` CSI driver on the same node.
-- `node.status` size increases with each installed CSI driver. Node objects are big enough already.
 
 ### Goals
 
@@ -90,6 +89,14 @@ It is problematic for CSI (`MaxCSIVolumeCountPred`) outlined in [#730](https://g
 
 - Heterogenous clusters, i.e. clusters where access to storage is limited only to some nodes. Existing `PV.spec.nodeAffinity` handling, not modified by this KEP, will filter out nodes that don't have access to the storage, so predicates changed in this KEP don't need to worry about storage topology and can be simpler.
 
+- Scheduling based on availability / health of CSI drivers on nodes. `CSINode` can be used to check which nodes have a driver installed and it could be extended to report health, so scheduler puts pods only to nodes with installed and healthy driver. This is out of scope of this PR.
+
+- Multiple plugins sharing the same volume limits. We expect that every CSI driver will have its own limits, not shared with other CSI drivers. In this KEP we support only in-tree volume plugins sharing its limits with one hard-coded CSI driver each.
+
+- Multiple "units" per single volume. Each volume used on a node takes exactly 1 unit from `allocatable.volumes`, regardless of the volume size, its replica count, number of connections to remote servers or other underlying resources needed to use the volume. For example, multipath iSCSI volume with three paths (and thus three iSCSI connections to three different servers) still takes 1 unit from `CSINode.status.allocatable.volumes`.
+
+- Maximum capacity per node. Some cloud environments limit both number of number of attached volumes (covered in this KEP) and total capacity of attached volumes (not covered in this KEP). For example, this KEP will ensure that scheduler puts max. 128 volumes to a [typical GCE node](https://cloud.google.com/compute/docs/machine-types#predefined_machine_types), but it won't ensure that the total capacity of the volumes is less than 64 TB.
+
 ## Proposal
 
 * Track volume limits for both in-tree volume plugins and CSI drivers in `CSINode` objects instead of `Node`.
@@ -101,26 +108,25 @@ It is problematic for CSI (`MaxCSIVolumeCountPred`) outlined in [#730](https://g
     * Limit for in-tree volumes will be added by kubelet during CSINode creation. Name of corresponding CSI driver will be used as key in `CSINode.status.allocatable` and it will be discovered using [CSI translation library](https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/csi-translation-lib). If the library does not support migration of an in-tree volume plugin, the volume plugin has no limit.
     * If a CSI driver is registered for an in-tree volume plugin and it reports a different volume limit than in-tree volume plugin, the limit reported by CSI driver is used and kubelet logs a warning.
   * User may NOT change `CSINode.status.allocatable` to override volume plugin / CSI driver values, e.g. to "reserve" some attachment to the operating system. Kubelet will periodically reconcile `CSINode` and overwrite the value.
-    * Especially, `kubelet --kube-reserved` or `--system-reserved` cannot be used to "reserve" volumes for kubelet or the OS. It is not possible with current kubelet and this KEP does not change it.
-      * We expect that CSI drivers will have configuration options / cmdline arguments to reserve some volumes and they will report their limit already reduced by that reserved amount. 
+    * Especially, `kubelet --kube-reserved` or `--system-reserved` cannot be used to "reserve" volumes for kubelet or the OS. It is not possible with current kubelet and this KEP does not change it. We expect that CSI drivers will have configuration options / cmdline arguments to reserve some volumes and they will report their limit already reduced by that reserved amount.
 
   * Kubelet will continue filling `Node.status.allocatable` and `Node.status.capacity` for both in-tree and CSI volumes during deprecation period. After the deprecation period, it will stop using them completely.
     * Scheduler (all its storage predicates) will ignore `Node.status.allocatable` and `Node.status.capacity` if `CSINode.status.allocatable` is present.
     * If `CSINode.status.allocatable` (or whole `CSINode`) is missing, scheduler falls back to `Node.status.allocatable`. This solves version skew between old kubelet (using `Node.status`) and new scheduler.
     * After deprecation period, scheduler won't schedule any pods that use volumes to a node with missing `CSINode` instance. It is expected that it happens only during node registration when `Node` exists and `CSINode` doesn't and it self-heals quickly.
 
-* `CSINode.status.allocatable` is a map CSI Driver name -> int64. Following combinations are possible:
+* `CSINode.status.allocatable` is array of limits. Following combinations are possible:
 
-| Driver name       | value | Description  |
-| ----------------- | ----- | ------------ |
-| `ebs.csi.aws.com` | 0     | plugin / CSI driver exists and has zero limit, i.e. can attach no volumes |
-| `ebs.csi.aws.com` | X>0   | plugin / CSI driver exists and can attach X volumes (where X > 0) |
-| `ebs.csi.aws.com` | X<0   | negative values are blocked by validation
-| key is missing in `CSINode.status.allocatable`   | -     |  there is no limit of volumes on the node* |
+|  `Volumes` | Description  |
+|  --------- | ------------ |
+|  0         | plugin / CSI driver exists and has zero limit, i.e. can attach no volumes |
+|  X>0       | plugin / CSI driver exists and can attach X volumes (where X > 0) |
+|  X<0       | negative values are blocked by validation
+| Driver is missing in `CSINode.status.allocatable`   | there is no limit of volumes on the node* |
 
 *) This way we are not able to distinguish between a volume plugin / CSI driver not installed on a node or it has been installed and it has no limits.
   * Predicates modified in this KEP assume that storage provided by **in-tree** volume plugin is available all nodes in the cluster. Other predicate(s) will evaluate `PV.spec.nodeAffinity` and filter out nodes that don't have access to the storage.
-  * For CSI drivers, availability of a CSI driver on a node can be checked in `CSINode.spec`.  
+  * For CSI drivers, availability of a CSI driver on a node can be checked in `CSINode.spec`. Its handling is out of scope of this KEP, see non-goals.
 
 CSINode example:
 
@@ -133,15 +139,15 @@ spec:
 status:
   allocatable:
     # AWS node can attach max. 40 volumes, 1 is reserved for the system
-    ebs.csi.aws.com: 39
+    - name: ebs.csi.aws.com
+      volumes: 39
 ```
 
 * Existing scheduler predicates `MaxEBSVolumeCount`, `MaxGCEPDVolumeCount`, `MaxAzureDiskVolumeCount` and `MaxCinderVolumeCount` are already deprecated.
   * If any of them is enabled together with `MaxCSIVolumeCountPred`, the deprecated predicate will do nothing (`MaxCSIVolumeCountPred` does the job of counting both in-tree and CSI volumes).
-  * The deprecated predicates will do any useful work only when `MaxCSIVolumeCountPred` predicate is disabled.
+  * The deprecated predicates will do any useful work only when `MaxCSIVolumeCountPred` predicate is disabled or `CSINode` object does not have limits for a particular driver.
   * This way, we save CPU by running only one volume limit predicate during deprecation period.
 
-
 ### New API
 
 CSINode gets `Status` struct with `Allocatable`, holding limit of volumes for each volume plugin and CSI driver that can be scheduled to the node.
@@ -154,20 +160,27 @@ type CSINode struct {
 	Status CSINodeStatus `json:"status" protobuf:"bytes,3,opt,name=status"`
 }
 
+// CSINodeStatus holds information about the status of all CSI drivers installed on a node
+type CSINodeStatus struct {
+	// allocatable is a list of volume limits for each volume plugin and CSI driver on the node.
+	// +patchMergeKey=name
+	// +patchStrategy=merge
+	Allocatable []VolumeLimits `json:"allocatable" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,1,rep,name=allocatable"`
+}
+
 // VolumeLimits is map CSI driver name -> maximum count of volumes for the driver on the node.
 // For in-tree volume plugins, name of corresponding CSI driver is used.
 // Value can be either:
 // - Positive integer: that's the volume limit.
 // - Zero: such volume cannot be used on the node.
 // - key is missing in VolumeLimits: there is no volume limit, i.e. any number of volumes can be used on the node.
-type VolumeLimits map[string]int64
+type VolumeLimits struct {
+	// name of CSI driver. For in-tree volume plugins, name of corresponding CSI driver is used.
+	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
+	// volumes is maximum number of volumes provided by the CSI driver that can be used by the node
+	Volumes int64 `json:"volumes,omitempty" protobuf:"varint,2,opt,name=volumes"`
 
-// CSINodeStatus holds information about the status of all CSI drivers installed on a node
-type CSINodeStatus struct {
-	// allocatable is a list of volume limits for each volume plugin and CSI driver on the node.
-	// +patchMergeKey=name
-	// +patchStrategy=merge
-	Allocatable []VolumeLimits `json:"allocatable" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,1,rep,name=allocatable"`
+	// Future proof: max. total size of volumes on the node can be added later
 }
 ```
 
@@ -183,7 +196,7 @@ type CSINodeStatus struct {
 * This KEP depends on [CSI migration library](https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/csi-translation-lib). It can happen that CSI migration is redesigned / cancelled.
   * Countermeasure: [CSI migration](https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/20190129-csi-migration.md) and this KEP should graduate together.
 
-* This KEP depends on [CSI migration library](https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/csi-translation-lib) ability to handle in-line in-tree volumes. Scheduler will need to get CSI driver name + `VolumeHandle` from them to count them towards the limit. 
+* This KEP depends on [CSI migration library](https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/csi-translation-lib) ability to handle in-line in-tree volumes. Scheduler will need to get CSI driver name + `VolumeHandle` from them to count them towards the limit.
 
 ## Design Details
 
@@ -217,20 +230,59 @@ N/A (`AttachVolumeLimit` feature is already beta).
 
 It must graduate together with CSI migration.
 
-##### Removing a deprecated flag
+### Upgrade / Downgrade / Version Skew Strategy
 
-- Announce deprecation and support policy of the existing flag
-- Two versions passed since introducing the functionality which deprecates the flag (to address version skew)
-- Address feedback on usage/changed behavior, provided on GitHub issues
-- Deprecate the flag
+During upgrade, downgrade or version skew, kubelet may be older that scheduler. Kubelet will not fill `CSINode.status` with volume limits and it will fill volume limits into `Node.status`. Scheduler must fall back to `Node.status` when `CSINode` is not available or its `status` does not contain a volume plugin / CSI driver.
 
-### Upgrade / Downgrade / Version Skew Strategy
+#### Interaction with CSI migration
 
+In-tree volume plugins are being migrated to CSI in a [separate KEP](https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/20190129-csi-migration.md). This section covers possible situations:
 
-During upgrade, downgrade or version skew, kubelet may be older that scheduler. Kubelet will not fill `CSINode.status` with volume limits and it will fill volume limits into `Node.status`. Scheduler must fall back to `Node.status` when `CSINode` is not available or its `status` does not contain a volume plugin / CSI driver.
+* CSI migration feature is `off` for a volume plugin and CSI driver is not installed on a node:
+  * In-tree volume plugin is used for in-tree PVs.
+  * Kubelet gets volume limits from the plugin and creates `CSINode` during node registration. Scheduler and kubelet still needs to translate in-tree plugin name to CSI driver name to get the right `name` for `CSINode.status.allocatable`.
 
-## Implementation History
+* CSI migration feature is `off` for a volume plugin and CSI driver (for the same storage backend) is installed on a node:
+  * In-tree volume plugin is used for in-tree PVs.
+  * CSI driver is used for CSI PVs.
+  * Kubelet gets volume limits from the plugin and creates `CSINode` during node registration.
+  * When CSI driver is registered, kubelet gets its volume limit through CSI and updates `CSINode`, potentially overwriting in-tree plugin limit.
+
+* CSI migration feature is `on` for a volume plugin and there is no CSI driver installed:
+  * In-tree volume plugin is "off".
+  * CSI driver is used both for in-tree and CSI PVs (if the driver is installed).
+  * Kubelet creates `CSINode` during node registration with no limit for the volume plugin / CSI driver.
+  * When CSI driver is registered, kubelet gets its volume limit through CSI and updates `CSINode` with the new limit.
+    * During the period when no CSI driver is registered and CSINode exists, there is "no limit" for the CSI driver and scheduler can put pods there! See non-goals!
+
+* CSI migration feature is `on` for a volume plugin and in-tree plugin is removed:
+  * Same as above, kubelet creates `CSINode` during node registration with no limit for the volume plugin / CSI driver.
+  * When CSI driver is registered, kubelet gets its volume limit through CSI and updates `CSINode` with the new limit.
+    * During the period when no CSI driver is registered and CSINode exists, there is "no limit" for the CSI driver and scheduler can put pods there! See non-goals!
 
+For brevity, in-line volumes in pods are handled the same as PVs in all cases above,
+
+#### Interaction with old `AttachVolumeLimit` implementation
+
+Due to version skew, following situations are possible (scheduler is always with `AttachVolumeLimit` enabled and with this KEP implemented):
+
+* Kubelet has `AttachVolumeLimit` off:
+  * Scheduler does not see any volume limits in `CSINode` nor `Node`.
+  * Since `CSINode` is missing, scheduler falls back to `MaxEBSVolumeCount`, `MaxGCEPDVolumeCount`, `MaxAzureDiskVolumeCount` and `MaxCinderVolumeCount` predicates and schedules in-tree volumes the old way with hardcoded limits.
+  * From scheduler point of view, the node can handle any number of CSI volumes.
+
+* Kubelet has old implementation of `AttachVolumeLimit` and the feature is on (kubelet fills `Node.status.available`):
+  * Scheduler does not see any volume limits in `CSINode`.
+  * Since `CSINode` is missing, scheduler falls back to `MaxEBSVolumeCount`, `MaxGCEPDVolumeCount`, `MaxAzureDiskVolumeCount` and `MaxCinderVolumeCount` predicates and schedules in-tree volumes the old way.
+  * Scheduler falls back to told implementation of `MaxCSIVolumeCountPred` for CSI volumes and uses limits from `Node.status`.
+
+* Kubelet has new implementation of `AttachVolumeLimit` and the feature is on (kubelet fills `CSINode`):
+  * No issue here, see this KEP.
+  * Since `CSINode` is available, scheduler uses new implementation of `MaxCSIVolumeCountPred`.
+
+As implied by the above, the scheduler needs to have both old and new implementation of `MaxCSIVolumeCountPred` and switch between them based on `CSINode` availability for a particular node until the old implementation is deprecated and removed (2 releases).
+
+## Implementation History
 
 # Alternatives
 
