-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Add e2e tests to validate DRA APIs #30462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- DynamicResourceAllocation has been graduated to GA - We need a few e2e tests defined downstream for this featuregate to be enabled to the Default cluster type Signed-off-by: Sai Ramesh Vanka <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sairameshv The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
depends on the DynamicResourceAllocation featuregate Signed-off-by: Sai Ramesh Vanka <[email protected]>
|
@sairameshv: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: c97ac81
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: c97ac81
New tests seen in this PR at sha: c97ac81
|
tkashem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should verify beta and alpha DRA APIs are disabled adds value for OpenShift, other tests are already covered by the upstream tests
| pollInterval = 2 * time.Second | ||
| ) | ||
|
|
||
| var _ = g.Describe("[sig-scheduling][OCPFeatureGate:DynamicResourceAllocation]", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DRA spans across multiple sigs, but if we have to pick a sig, i would probably go with sig-node, the upstream DRA tests use sig-node as well if i am not mistaken
Also, can we add the label "DRA" to these tests? (so searching with DRA shows both upstream and these tests)
|
|
||
| g.Context("Dynamic Resource Allocation", func() { | ||
|
|
||
| g.It("should create and list DeviceClass resources [apigroup:resource.k8s.io]", func(ctx context.Context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upstream conformance test https://github.com/kubernetes/kubernetes/blob/544dfee60a99eaec9962c3853674dc1e4d7f0c8d/test/e2e/dra/dra.go#L87 already covers what this test does
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to delete ResourceClaim") | ||
|
|
||
| g.By("verifying the ResourceClaim was deleted") | ||
| err = wait.PollUntilContextTimeout(ctx, pollInterval, draTestTimeout, true, func(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need polling here? i would expect the first invocation of Get to succeed
| o.Expect(len(dcList.Items)).To(o.BeNumerically(">", 0), "expected at least one DeviceClass") | ||
| }) | ||
|
|
||
| g.It("should create and manage ResourceClaim lifecycle [apigroup:resource.k8s.io]", func(ctx context.Context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's covered by the upstream test https://github.com/kubernetes/kubernetes/blob/544dfee60a99eaec9962c3853674dc1e4d7f0c8d/test/e2e/dra/dra.go#L115
| o.Expect(createdPod.Spec.ResourceClaims[0].ResourceClaimTemplateName).NotTo(o.BeNil()) | ||
|
|
||
| g.By("verifying a generated ResourceClaim was created from the template") | ||
| // The generated claim name follows the pattern: <pod-name>-<claim-name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OwnerReferences of the expected claim should include the pod as an owner
| // The generated claim name follows the pattern: <pod-name>-<claim-name> | ||
|
|
||
| var generatedClaim *resourcev1.ResourceClaim | ||
| err = wait.PollUntilContextTimeout(ctx, pollInterval, draTestTimeout, true, func(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: gomega.Eventually?
| o.Expect(err).NotTo(o.HaveOccurred(), "external ResourceClaim should persist after pod deletion") | ||
| }) | ||
|
|
||
| g.It("should create and manage ResourceClaimTemplate [apigroup:resource.k8s.io]", func(ctx context.Context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to list ResourceSlices - API should be accessible") | ||
|
|
||
| // ResourceSlices are created by DRA drivers, so we may not have any in a cluster | ||
| // without DRA drivers installed. Just verify the API is accessible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think this test providing any value (upstream tests already cover this)
| framework.Logf("Available versions for resource.k8s.io: %v", resourceAPIGroup.Versions) | ||
|
|
||
| // Verify only v1 is in the list | ||
| hasV1 := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we just compare to an array with the expected v1?
"versions": [
{
"groupVersion": "resource.k8s.io/v1",
"version": "v1"
}
],
| }) | ||
|
|
||
| // Adding a test for the DynamicResourceAllocation field in Scheduler ProfileCustomizations that depends on downstream DynamicResourceAllocation feature gate. | ||
| g.It("should support DynamicResourceAllocation field in Scheduler ProfileCustomizations [apigroup:config.openshift.io]", func(ctx context.Context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what value this test provides specific to DRA, if we want to validate that setting this value to either true or false enables/disables the dra plugin in the scheduler, then the test needs to wait for the scheduler to rollout and then validates appropriately.
Helps in promoting openshift/api#2498
Tests generated by Claude
cc: @tkashem @haircommander