OCPNODE-4538: Add e2e tests for DRA Partitionable Devices (KEP-4815)#31230
Draft
sabujmaity wants to merge 2 commits into
Draft
OCPNODE-4538: Add e2e tests for DRA Partitionable Devices (KEP-4815)#31230sabujmaity wants to merge 2 commits into
sabujmaity wants to merge 2 commits into
Conversation
Add three e2e tests validating the DRAPartitionableDevices feature gate using the upstream dra-example-driver with gpuPartitions enabled: 1. Validates ResourceSlice two-slice model (SharedCounters + ConsumesCounters) 2. Validates partition device allocation to pod via DRA ResourceClaim 3. Validates counter exhaustion renders additional claims unschedulable
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@sabujmaity: This pull request references OCPNODE-4538 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sabujmaity The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a PartitionableDevices E2E test suite, a CounterValidator helper, installer/Helm improvements (including HelmUpgrade and namespace cleanup), and test registration/OWNERS and static suite registration. ChangesPartitionableDevices Feature Testing
Sequence DiagramsequenceDiagram
participant TestSuite
participant PrerequisitesInstaller
participant Helm as Driver(Helm)
participant CounterValidator
participant DeviceClass
participant Pod
TestSuite->>PrerequisitesInstaller: InstallAll (ensureNamespaceGone)
TestSuite->>Helm: HelmUpgrade (partition mode)
TestSuite->>CounterValidator: ValidateSharedCounters
CounterValidator->>DeviceClass: List ResourceSlices
TestSuite->>DeviceClass: Create with requests
TestSuite->>Pod: Create with DeviceClaim
Pod->>Pod: Allocate partition devices
TestSuite->>Pod: Validate "partition" in names
TestSuite->>CounterValidator: Verify consumption
TestSuite->>Pod: Create exhaustion pod (pending)
Pod->>Pod: Unschedulable (insufficient)
TestSuite->>Helm: HelmUpgrade (restore non-partition)
🎯 3 (Moderate) | ⏱️ ~25 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/extended/node/dra/example/prerequisites_installer.go (1)
164-172: 💤 Low valueConsider logging unexpected API errors in the poll loop.
When
getErris non-nil but notNotFound, the current code logs "still exists, waiting for GC" which is misleading if the actual error is a network or auth failure. While this resilience pattern is reasonable for cleanup, logging the actual error would aid debugging.♻️ Suggested improvement
return wait.PollUntilContextTimeout(ctx, 3*time.Second, 3*time.Minute, true, func(ctx context.Context) (bool, error) { _, getErr := pi.client.CoreV1().Namespaces().Get(ctx, driverNamespace, metav1.GetOptions{}) if errors.IsNotFound(getErr) { framework.Logf("Namespace %s fully removed", driverNamespace) return true, nil } + if getErr != nil { + framework.Logf("Error checking namespace %s (will retry): %v", driverNamespace, getErr) + return false, nil + } framework.Logf("Namespace %s still exists, waiting for GC...", driverNamespace) return false, nil })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/node/dra/example/prerequisites_installer.go` around lines 164 - 172, The poll callback in wait.PollUntilContextTimeout that calls pi.client.CoreV1().Namespaces().Get currently treats any non-NotFound error as "still exists" which is misleading; modify the anonymous func used by wait.PollUntilContextTimeout (the closure referencing driverNamespace and getErr) to check if getErr != nil and !errors.IsNotFound(getErr) and, in that branch, log the actual getErr (e.g., using framework.Logf or the existing logger) with context before returning false,nil so retries continue—ensure you reference the same getErr, driverNamespace, and the poll closure so only the logging behavior changes.test/extended/node/dra/common/counter_validator.go (1)
33-54: 💤 Low valueDocstring claims "no Devices" constraint not enforced by code.
The docstring states counter slices have "SharedCounters, no Devices", but the implementation only checks for presence of SharedCounters. A slice with both would appear in both lists. While conforming drivers use the two-slice model, the code doesn't enforce the documented invariant.
Consider either updating the docstring to reflect actual behavior (categorizes by presence of each field) or adding the exclusion check if strict separation is intended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/node/dra/common/counter_validator.go` around lines 33 - 54, The docstring for GetResourceSlicesByType promises "SharedCounters, no Devices" but the implementation only checks SharedCounters and allows slices with both fields to be listed in both outputs; update the logic in GetResourceSlicesByType so counterSlices only includes slices where slice.Spec.SharedCounters is non-empty AND slice.Spec.Devices is empty (i.e., use the condition on slice.Spec.SharedCounters and slice.Spec.Devices), keep deviceSlices as slices with slice.Spec.Devices non-empty, and update the function docstring to match the enforced invariant; reference: GetResourceSlicesByType, slice.Spec.SharedCounters, slice.Spec.Devices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/node/dra/partitionable/partitionable_dra.go`:
- Around line 232-287: GetNodeWithDevices() can return a tainted fallback node
which causes the pinned pod to fail scheduling for taint reasons; after calling
counterValidator.GetNodeWithDevices(ctx) (and getting nodeName) fetch the Node
object via oc.KubeFramework().ClientSet.CoreV1().Nodes().Get(...) and inspect
node.Spec.Taints, and if any non-tolerable taints exist, iterate available
device-capable nodes (use counterValidator or list nodes with device resource
slices) to pick an untainted nodeName, updating exhaustPod.Spec.NodeSelector
accordingly; if no untainted node is available, fail the test with a clear
message.
---
Nitpick comments:
In `@test/extended/node/dra/common/counter_validator.go`:
- Around line 33-54: The docstring for GetResourceSlicesByType promises
"SharedCounters, no Devices" but the implementation only checks SharedCounters
and allows slices with both fields to be listed in both outputs; update the
logic in GetResourceSlicesByType so counterSlices only includes slices where
slice.Spec.SharedCounters is non-empty AND slice.Spec.Devices is empty (i.e.,
use the condition on slice.Spec.SharedCounters and slice.Spec.Devices), keep
deviceSlices as slices with slice.Spec.Devices non-empty, and update the
function docstring to match the enforced invariant; reference:
GetResourceSlicesByType, slice.Spec.SharedCounters, slice.Spec.Devices.
In `@test/extended/node/dra/example/prerequisites_installer.go`:
- Around line 164-172: The poll callback in wait.PollUntilContextTimeout that
calls pi.client.CoreV1().Namespaces().Get currently treats any non-NotFound
error as "still exists" which is misleading; modify the anonymous func used by
wait.PollUntilContextTimeout (the closure referencing driverNamespace and
getErr) to check if getErr != nil and !errors.IsNotFound(getErr) and, in that
branch, log the actual getErr (e.g., using framework.Logf or the existing
logger) with context before returning false,nil so retries continue—ensure you
reference the same getErr, driverNamespace, and the poll closure so only the
logging behavior changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 46dab00d-6005-4d5a-9fbc-233ea24214ae
📒 Files selected for processing (5)
test/extended/include.gotest/extended/node/dra/common/counter_validator.gotest/extended/node/dra/example/prerequisites_installer.gotest/extended/node/dra/partitionable/OWNERStest/extended/node/dra/partitionable/partitionable_dra.go
- Register openshift/dra-example test suite in standard_suites.go enabling CI to run all DRA tests via: openshift-tests run openshift/dra-example - Add API error logging in namespace cleanup for better debugging - Improve ExpectNoError message in partition allocation verification
sabujmaity
force-pushed
the
feat/OCPNODE-4538-dra-partitionable-devices-e2e
branch
from
c8e5526 to
bdab941
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
openshift/dra-exampletest suite instandard_suites.goso all DRA example driver tests are runnable via./openshift-tests run openshift/dra-exampleArchitecture:
CI Consolidation (companion openshift/release PR to follow):
JIRA
https://issues.redhat.com/browse/OCPNODE-4538
Summary by CodeRabbit
Tests
Chores