DRA: update allocator unit tests #49
Conversation
bart0sh
force-pushed
the
PR149-DRA-update-allocator-unittests
branch
2 times, most recently
from
966005b
to
d360839
Compare
|
@pohly This is where I am so far. PTAL |
| @@ -227,7 +228,7 @@ func (sharedAllocator *Allocator) Allocate(ctx context.Context, node *v1.Node) ( | |||
| // Constraints are assumed to be monotonic: once a constraint returns | |||
| // false, adding more devices will not cause it to return true. This | |||
| // allows the search to stop early once a constraint returns false. | |||
| var constraints = make([]constraint, len(claim.Spec.Devices.Constraints)) | |||
| var constraints = make([]constraint, 0, len(claim.Spec.Devices.Constraints)) | |||
There was a problem hiding this comment.
I think in this case it's simpler to keep the original slice allocation and below use
constraints[i] = m
I use the style with append when only an upper bound is known, but here we know the exact size.
I'm not always consistent, which led to this mismatch here... 🥵
| @@ -301,16 +759,153 @@ devices: | |||
|
|
|||
| expectResults: []any{allocatedTwoDeviceClaim}, | |||
| }, | |||
| "devices split across different slices": { | |||
There was a problem hiding this comment.
Please don't use spaces in Go test names. They get replaced with underscores, which makes it harder to put together a go test -run invocation (go test -run=".*/devices split across different slices" doesn't work) or to find a failed test in the source code (has a different name).
| expectResults: []any{allocatedFourDeviceClaim}, | ||
| }, | ||
| "obsolete slices": { | ||
| claimsToAllocate: objects(twoDeviceClaim), |
There was a problem hiding this comment.
Can the tests be derived from the "simple" case as often as possible?
Here the relevant difference is the addition of node1ObsoleteSlice. That it allocates two devices seems unnecessary.
There was a problem hiding this comment.
You are probably right with saying that generating the test data with Go functions is better. Then instead of referencing pre-generated test data in all tests, each test can generate its own test data slightly differently and the difference between the tests will be more obvious.
Right now one has to remember all test data or jump back-and-forth to verify the correctness of each test case.
There was a problem hiding this comment.
In addition to that editing YAML is a pain in go code, at least with VSCode. It inserts tabs all the time and I had to replace them with spaces after the test runtime failures caused by yaml parser errors.
| `) | ||
|
|
||
| // Second slice on the same node, with different devices. | ||
| node1slice2 := unmarshal[resourceapi.ResourceSlice](t, ` |
There was a problem hiding this comment.
Different how?
I was reading the test below where this appeared and in the test it wasn't obvious why the result is different than in other tests.
Perhaps a more descriptive name would help.
|
@pohly all review comments should be addressed now, except of switching to go structs. Will do that in a separate commit later today. |
|
I have merged all commits up to 8c96fcd (current latest) into my branch. I had to squash, so you are listed as co-author in the second commit. Please rebase (should leave an empty diff) and continue updating the tests here. |
pohly
force-pushed
the
dra-1.31
branch
4 times, most recently
from
9a7aad8
to
1841742
Compare
…t startup Any redundant object must get deleted, but not the ones of other names.
2e34e18 enabled kubelet to do List and Watch requests with the caveat that kubelet should better use a field selector (which it does). The same is now also needed for DeleteCollection because kubelet will use that to clean up in one operation instead of using multiple.
This is a first step towards making kubelet independent of the resource.k8s.io API versioning because it now doesn't need to copy structs defined by that API from the driver to the API server. The next step is removing the other direction (reading ResourceClaim status and passing the resource handle to drivers). The drivers must get deployed so that they have their own connection to the API server. Securing at least the writes via a validating admission policy should be possible. As before, the kubelet removes all ResourceSlices for its node at startup, then DRA drivers recreate them if (and only if) they start up again. This ensures that there are no orphaned ResourceSlices when a driver gets removed while the kubelet was down. While at it, logging gets cleaned up and updated to use structured, contextual logging as much as possible. gRPC requests and streams now use a shared, per-process request ID and streams also get logged.
This is the second and final step towards making kubelet independent of the resource.k8s.io API versioning because it now doesn't need to copy structs defined by that API from the driver to the API server.
In reality, the kubelet plugin of a DRA driver is meant to be deployed as a daemonset with a service account that limits its permissions. https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/#additional-metadata-in-pod-bound-tokens ensures that the node name is bound to the pod. In E2E testing, we emulate that via impersonation. This ensures that the plugin does not accidentally depend on additional permissions.
The previous changes are an API break, therefore we need a new version.
If the node authorizer is active, RBAC rules are not needed. But if it's disabled, kubelet needs to get permission through RBAC. In contrast to the authorizer code which is a bit more flexible and isn't directly tied to the current kubelet implementation (i.e. it allows list+delete instead of just deletecollection), the RBAC entry is just for what the current kubelet does because it's a bit easier to change.
As agreed in kubernetes/enhancements#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.
Now all claims are shareable up to the limit imposed by the size of the "reserverFor" array. This is one of the agreed simplifications for 1.31.
Repeating the version in the import of the API packages is not really required. It was done for a while to support simpler grepping for usage of alpha APIs, but there are better ways for that now. So during this transition, "resourceapi" gets used instead of "resourcev1alpha3" and the version gets dropped from informer and lister imports. The advantage is that the next bump to v1beta1 will affect fewer source code lines. Only source code where the version really matters (like API registration) retains the versioned import.
Logging and sub-tests were added to help debug this problem:
the test passes for ResourceClaim (same defaulting!) and fails
for the list, but only if run together with the other test cases?!
$ go test ./pkg/api/testing
--- FAIL: TestDefaulting (1.76s)
--- FAIL: TestDefaulting/resource.k8s.io/v1alpha3,_Kind=ResourceClaimList (0.01s)
defaulting_test.go:238: expected resource.k8s.io/v1alpha3, Kind=ResourceClaimList to trigger defaulting due to fuzzing
FAIL
FAIL k8s.io/kubernetes/pkg/api/testing 17.294s
FAIL
$ go test -run=TestDefaulting/resource.k8s.io/v1alpha3,_Kind=ResourceClaimList ./pkg/api/testing
ok k8s.io/kubernetes/pkg/api/testing 0.062s
What fixed that problem was increasing the likelihood of generating the right
test object by iterating more often before giving up.
This is a complete revamp of the original API. Some of the key difference: - 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 The implementation is complete enough to bring up the apiserver. Adapting other components follows.
Publishing ResourceSlices now supports network-attached devices and the new v1alpha3 API. The logic for splitting up across different slices is missing.
The advantages of using a validation admission policy (VAP) are that no changes are needed in Kubernetes and that admins have full flexibility if and how they want to control which users are allowed to use "admin access" in their requests. The downside is that without admins taking actions, the feature is enabled out-of-the-box in a cluster. Documentation for DRA will have to make it very clear that something needs to be done in multi-tenant clusters. The test/e2e/testing-manifests/dra/admin-access-policy.yaml shows how to do this. The corresponding E2E tests ensures that it actually works as intended. For some reason, adding the namespace to the message expression leads to a type check errors, so it's currently commented out.
The resource claim controller is completely agnostic to the claim spec. It doesn't care about classes or devices, therefore it needs no changes in 1.31 besides the v1alpha2 -> v1alpha3 renaming from a previous commit.
The structured parameter allocation logic was written from scratch in staging/src/k8s.io/dynamic-resource-allocation/structured where it might be useful for out-of-tree components. Besides the new features (amount, admin access) and API it now supports backtracking when the initial device selection doesn't lead to a complete allocation of all claims. Co-authored-by: Ed Bartosh <[email protected]>
bart0sh
force-pushed
the
PR149-DRA-update-allocator-unittests
branch
from
8c96fcd
to
09ac0cf
Compare
|
@pohly replaced yamls with object generators and go structures, PTAL. It looks more readable this way, at least to me. And it's much smaller: There is still a place for improvements, I just want to know if this looks ok to you. |
|
Looks okay, I copied 09ac0cf into my branch. As before I'll squash it before publishing. |
pohly
force-pushed
the
dra-1.31
branch
18 times, most recently
from
68ad8b2
to
d611ce4
Compare
pohly
force-pushed
the
dra-1.31
branch
2 times, most recently
from
43567fc
to
d11b58e
Compare
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
It adds a bunch of test cases to the DRA allocator unit test and fixes minor bugs in the allocator code discovered by the new test cases.
Does this PR introduce a user-facing change?