Previously, ValidateNodeSelector did not check that labels are valid. Now it
does for resource.k8s.io, regardless whether an object already was created with
invalid labels in an earlier Kubernetes release. Theoretically this is a
breaking change and could cause problems during an upgrade, but that is highly
unlikely in practice.
In contrast to node affinity, DRA does not ignore parse errors
(= uses NewNodeSelector, not NewLazyErrorNodeSelector), so invalid labels would
have been found instead of being silently ignored.
Even if some object has invalid labels, this only affects an alpha -> beta
upgrade which isn't guaranteed to work seamlessly.
* Test feature-gate enabled/disabled for validation
* Test pkg/registry/resource/resourceclaim
* Add Data and NetworkData to integration test
Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>
* Add status
* Add validation to check if fields are correct (Network field, device
has been allocated))
* Add feature-gate
* Drop field if feature-gate not set
Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>
This had been left out unintentionally earlier. Because theoretically there
might now be existing objects with parameters that are larger than whatever
limit gets enforced now, the limit only gets checked when parameters get
created or modified.
This is similar to the validation of CEL expressions and for consistency, the
same 10 Ki limit as for those is chosen.
Because the limit is not enforced for stored parameters, it can be increased in
the future, with the caveat that users who need larger parameters then depend
on the newer Kubernetes release with a higher limit. Lowering the limit is
harder because creating deployments that worked in older Kubernetes will not
work anymore with newer Kubernetes.
This enables a future extension where capacity of a single device gets consumed
by different claims. The semantic without any additional fields is the same as
before: a capacity cannot be split up and is only an attribute of a device.
Because its semantically the same as before, two-way conversion to v1alpha3 is
possible.
The line coverage is now at 98.5% and several more corner cases are
covered. The remaining lines are hard or impossible to reach.
The actual validation is the same as before, with some small tweaks to the
generated errors.
When failures are not as expected, it is useful to show what the expected and
actual failures look like to a user. Perhaps even better would be to put the
expected texts into the test files instead of the error structs. That would
be easier to review and shorter.
Using the "normal" logic for a feature gated field simplifies the
implementation of the feature gate.
There is one (entirely theoretic!) problem with updating from 1.31: if a claim
was allocated in 1.31 with admin access, the status field was not set because
it didn't exist yet. If a driver now follows the current definition of "unset =
off", then it will not grant admin access even though it should. This is
theoretic because drivers are starting to support admin access with 1.32, so
there shouldn't be any claim where this problem could occur.
The new DRAAdminAccess feature gate has the following effects:
- If disabled in the apiserver, the spec.devices.requests[*].adminAccess
field gets cleared. Same in the status. In both cases the scenario
that it was already set and a claim or claim template get updated
is special: in those cases, the field is not cleared.
Also, allocating a claim with admin access is allowed regardless of the
feature gate and the field is not cleared. In practice, the scheduler
will not do that.
- If disabled in the resource claim controller, creating ResourceClaims
with the field set gets rejected. This prevents running workloads
which depend on admin access.
- If disabled in the scheduler, claims with admin access don't get
allocated. The effect is the same.
The alternative would have been to ignore the fields in claim controller and
scheduler. This is bad because a monitoring workload then runs, blocking
resources that probably were meant for production workloads.
Drivers need to know that because admin access may also grant additional
permissions. The allocator needs to ignore such results when determining which
devices are considered as allocated.
In both cases it is conceptually cleaner to not rely on the content of the
ClaimSpec.
As pointed out during code review, the CEL cost estimates are not considered
perfectly reliable. Therefore it is better to also do runtime checks.
Some downstream users might decide to allow CEL expressions to run
longer. Therefore the cost limit is now part of an Options struct.
kube-scheduler uses the default cost limit defined in the resource.k8s.io API,
which is the same cost limit that also the apiserver uses during validation.
The main purpose is to protect against denial-of-service attacks. Scheduling
time depends a lot on unpredictable factors and expected scheduling time also
varies, so no attempt is made to limit the overall time spent on evaluating CEL
expressions per claim.
This removes the DRAControlPlaneController feature gate, the fields controlled
by it (claim.spec.controller, claim.status.deallocationRequested,
claim.status.allocation.controller, class.spec.suitableNodes), the
PodSchedulingContext type, and all code related to the feature.
The feature gets removed because there is no path towards beta and GA and DRA
with "structured parameters" should be able to replace it.
This is a complete revamp of the original API. Some of the key
differences:
- refocused on structured parameters and allocating devices
- support for constraints across devices
- support for allocating "all" or a fixed amount
of similar devices in a single request
- no class for ResourceClaims, instead individual
device requests are associated with a mandatory
DeviceClass
For the sake of simplicity, optional basic types (ints, strings) where the null
value is the default are represented as values in the API types. This makes Go
code simpler because it doesn't have to check for nil (consumers) and values
can be set directly (producers). The effect is that in protobuf, these fields
always get encoded because `opt` only has an effect for pointers.
The roundtrip test data for v1.29.0 and v1.30.0 changes because of the new
"request" field. This is considered acceptable because the entire `claims`
field in the pod spec is still alpha.
The implementation is complete enough to bring up the apiserver.
Adapting other components follows.
As agreed in https://github.com/kubernetes/enhancements/pull/4709, immediate
allocation is one of those features which can be removed because it makes no
sense for structured parameters and the justification for classic DRA is weak.
1. `apiGroup`: If set, it must be a valid DNS subdomain (e.g. 'example.com').
2. `kind` and `name`: It must be valid path segment name. It may not be '.' or '..' and it may not contain '/' and '%' characters.
While currently those objects only get published by the kubelet for node-local
resources, this could change once we also support network-attached
resources. Dropping the "Node" prefix enables such a future extension.
The NodeName in ResourceSlice and StructuredResourceHandle then becomes
optional. The kubelet still needs to provide one and it must match its own node
name, otherwise it doesn't have permission to access ResourceSlice objects.
Like the current device plugin interface, a DRA driver using this model
announces a list of resource instances. In contrast to device plugins, this
list is made available to the scheduler together with attributes that can be
used to select suitable instances when they are not all alike.
Because this is the first structured parameter model, some checks that
previously were not possible, in particular "is one structured parameter field
set", now gets enabled. Adding another structured parameter model will be
similar.
The applyconfigs code generator assumes that all types in an API are defined in
a single package. If it wasn't for that, it would be possible to place the
"named resources" types in separate packages, which makes their names in the Go
code more natural and provides an indication of their stability level because
the package name could include a version.
NodeResourceSlice will be used by kubelet to publish resource information on
behalf of DRA drivers on the node. NodeName and DriverName in
NodeResourceSlice must be immutable. This simplifies tracking the different
objects because what they are for cannot change after creation.
The new field in ResourceClass tells scheduler and autoscaler that they are
expected to handle allocation.
ResourceClaimParameters and ResourceClassParameters are new types for telling
in-tree components how to handle claims.
The generated ResourceClaim name and the names of the ResourceClaimTemplate and
ResourceClaim referenced by a pod must be valid according to the resource API,
otherwise the pod cannot start.
Checking this was removed from the original implementation out of concerns
about validating fields in core against limitations imposed by a separate,
alpha API. But as this was pointed out again in
https://github.com/kubernetes/kubernetes/pull/116254#discussion_r1134010324
it gets added back.
The same strings that worked before still work now. In particular, the
constraints for a spec.resourceClaim.name are still the same (DNS label).
The name "PodScheduling" was unusual because in contrast to most other names,
it was impossible to put an article in front of it. Now PodSchedulingContext is
used instead.
This fixes the following warning (error?) in the apiserver:
E0126 18:10:38.665239 16370 fieldmanager.go:210] "[SHOULD NOT HAPPEN] failed to update managedFields" err="failed to convert new object (test/claim-84; resource.k8s.io/v1alpha1, Kind=ResourceClaim) to smd typed: .status.reservedFor: element 0: associative list without keys has an element that's a map type" VersionKind="/, Kind=" namespace="test" name="claim-84"
The root cause is the same as in e50e8a0c91:
nothing in Kubernetes outright complains about a list of items where the item
type is comparable in Go, but not a simple type. This nonetheless isn't
supposed to be done in the API and can causes problems elsewhere.
For the ReservedFor field, everything seems to work okay except for the
warning. However, it's better to follow conventions and use a map. This is
possible in this case because UID is guaranteed to be a unique key.
Validation is now stricter than before, which is a good thing: previously,
two entries with the same UID were allowed as long as some other field was
different, which wasn't a situation that should have been allowed.
This is in response to review feedback. Checking for valid node names and the
set property catches programming mistakes in the components that have write
permission.
This adds a new resource.k8s.io API group with v1alpha1 as version. It contains
four new types: resource.ResourceClaim, resource.ResourceClass, resource.ResourceClaimTemplate, and
resource.PodScheduling.
