fixup! review comments

Author: pohly

keps/sig-scheduling/5027-dra-admin-controlled-device-attributes/README.md

@@ -94,6 +94,8 @@ SIG Architecture for cross-cutting KEPs).
 - [Implementation History](#implementation-history)
 - [Drawbacks](#drawbacks)
 - [Alternatives](#alternatives)
+  - [Admin-intent in ResourceSlice](#admin-intent-in-resourceslice)
+  - [Storing result of patching in ResourceSlice](#storing-result-of-patching-in-resourceslice)
 <!-- /toc -->
 
 ## Release Signoff Checklist
@@ -224,6 +226,16 @@ caching the patched devices and
 (re-)applying patches only when they or the device definitions change, which
 should be rare.
 
+Patching directly in the informer event handlers may be fast enough. If it
+turns out to slow down those handlers too much, then a workqueue with workers
+may be needed to decouple updating the cache from the events which trigger
+updating and to avoid slowing down the informers.
+
+The scheduler's "slice changed" cluster events must be driven by that cache,
+not the original informers, otherwise a ResourceSlice or ResourceSlicePatch
+change could trigger a pod scheduling attempt before the slice cache is
+up-to-date again.
+
 ## Design Details
 
 ### API
@@ -237,7 +249,7 @@ are feature-gated.
 
 ```Go
 type ResourceSlicePatch struct {
-metav1.TypeMeta
+    metav1.TypeMeta
     // Standard object metadata
     // +optional
     metav1.ObjectMeta
@@ -278,7 +290,7 @@ type DevicePatch struct {
     // be marked as empty by setting their null field. Such entries remove the
     // corresponding attribute in a ResourceSlice, if there is one, instead of
     // overriding it. Because entries get removed and are not allowed in
-    // slices, CEL expressions do not need need to deal with null values.
+    // slices, CEL expressions do not need to deal with null values.
     //
     // The maximum number of attributes and capacities in the DevicePatch combined is 32.
     // This is an alpha field and requires enabling the DRAAdminControlledDeviceAttributes
@@ -288,6 +300,11 @@ type DevicePatch struct {
     // +featureGate:DRAAdminControlledDeviceAttributes
     Attributes map[FullyQualifiedName]NullableDeviceAttribute
 
+    // ^^^
+    // The size limit is the same as for attributes and capacities in a ResourceSlice.
+    // We could make it larger here because we are less constrained by overall object
+    // size, but it seems unnecessary.
+
     // Capacity defines the set of capacities to patch for matching devices.
     // The name of each capacity must be unique in that set and
     // include the domain prefix.
@@ -707,15 +724,15 @@ No.
 ###### Will enabling / using this feature result in increasing time taken by any operations covered by existing SLIs/SLOs?
 
 Pod scheduling should be as fast as would be without this feature, because in
-both cases it starts with listing all devices. That information is local can
+both cases it starts with listing all devices. That information is local and
 comes either from an informer cache or a cache of patched devices.
 
 ###### Will enabling / using this feature result in non-negligible increase of resource usage (CPU, RAM, disk, IO, ...) in any components?
 
 Filtering and patching are local operations, with no impact on the cluster. To
 prevent doing the same work repeatedly, it will be implemented so that it gets
 done once and then only processes changes. This increases CPU and RAM
-consumption. But even all devices should get patched (which is unlikely), memory
+consumption. But even if all devices should get patched (which is unlikely), memory
 will be shared between objects in the informer cache and in the patch cache, so
 it will not be doubled.
 
@@ -766,7 +783,30 @@ harder for users to get a complete view.
 
 ## Alternatives
 
+### Admin-intent in ResourceSlice
+
 Instead of ResourceSlicePatch as a separate type, new fields in the
 ResourceSlice status could be modified by an admin. That has the problem that
 the ResourceSlice object might get deleted while doing cluster maintenance like
 a driver update, in which case the admin intent would get lost.
+
+### Storing result of patching in ResourceSlice
+
+A controller could read ResourceSlicePatches and apply them to
+ResourceSlices. Then consumers like the scheduler and users would only need to
+look at ResourceSlices. This has several drawbacks.
+
+We would need to duplicate the attributes in the slice status. If we didn't and
+directly modified the spec, this patch controller and the CSI driver as the
+owner of the slice spec would fight against each other. Also, after removing a
+patch the original state must be available somewhere, otherwise the controller
+cannot restore it.
+
+Duplicating the attributes might make a slice too large. The limits were chosen
+so that we have some space left for a status, but not enough for a status that
+is potentially as large as the spec.
+
+Creating a single ResourceSlicePatch could force the controller to update a
+potentially large number of ResourceSlices. When using rate limiting, updating
+them all will take longer than client-side patching. When not using rate
+limiting, this could overwhelm the apiserver.

keps/sig-scheduling/5055-dra-device-taints-and-tolerations/README.md

@@ -158,7 +158,7 @@ ResourceClaim.
 
 ### Goals
 
-- Enable taking devices offline for maintenance while still allowing tests pods
+- Enable taking devices offline for maintenance while still allowing test pods
   to request and use those devices. Being able to do this one device at a time
   minimizes service level disruption.
 
@@ -200,7 +200,7 @@ hardware instance to support hot-swapping. Admins might favor using the names
 whereas health monitoring might prefer to be specific and use a vendor-defined
 unique ID. Both are supported, which creates additional complexity.
 
-Without a kubectl extension similar to `kubectl taint nodes` the user
+Without a kubectl extension similar to `kubectl taint nodes`, the user
 experience for admins will be a bit challenging. They need to decide how to
 identify the device (by name or with a CEL expression), manually create a
 ResourceSlicePatch with a unique name, then remember to remove that
@@ -382,7 +382,9 @@ code. Alternatively, that code can be copied.
 The DevicePatch also gets extended. It is possible to use
 admin-controlled taints without enabling attribute overrides by enabling the
 `v1alpha3` API and only the `DRADeviceTaints` feature, while leaving
-`DRAAdminControlledDeviceAttributes` disabled.
+`DRAAdminControlledDeviceAttributes` disabled, because then the
+ResourceSlicePatch type is available with only the fields needed for
+taints.
 
 ```Go
 type DevicePatch struct {
@@ -759,7 +761,7 @@ harder for users to get a complete view.
 ## Alternatives
 
 The existing taint-eviction-controller could be extended to cover device
-taints. Cloning it lowers the risk of breaking existing stable functionality.
+taints. However, cloning it lowers the risk of breaking existing stable functionality.
 
 Tolerations for device taints could also be added to individual pods. This
 seems less useful because if pods share the same claim, they are typically part