OCPNODE-4470: Add DRA consumable capacity e2e tests - #31448
Conversation
Introduce tests for DRA consumable capacity (bandwidth sharing on NIC devices): resource slice validation, multi-allocation on the same device, capacity exhaustion scheduling, and capacity release after pod deletion. Signed-ff-by: Sai Ramesh Vanka <svanka@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sairameshv: This pull request references OCPNODE-4470 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughChangesConsumable DRA capacity
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant ExampleDRA
participant ResourceClaim
participant Scheduler
participant Pod
participant CapacityValidator
TestSuite->>ExampleDRA: configure net profile
ExampleDRA-->>CapacityValidator: publish capacity devices
TestSuite->>ResourceClaim: create capacity request
TestSuite->>Pod: create consuming pod
Pod->>Scheduler: request placement
Scheduler-->>ResourceClaim: allocate device capacity
CapacityValidator-->>TestSuite: validate ConsumedCapacity
TestSuite->>ResourceClaim: delete claim and release capacity
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 |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 (3)
test/extended/node/dra/consumable/consumable_dra.go (2)
379-400: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnschedulable assertion depends on free-text scheduler condition message.
The check requires
cond.Messageto contain"claim","insufficient", or"unresolvable". This couples the test to the exact wording the DRA scheduler emits, which is not a documented/stable contract and could change across Kubernetes releases, causing this assertion to silently stop matching (test would then time out as "no DRA-related Unschedulable condition yet" rather than fail with a clear diagnosis).🤖 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/consumable/consumable_dra.go` around lines 379 - 400, Update the unschedulable verification in the overflow pod polling block to rely on stable pod scheduling status/reason fields rather than matching free-text cond.Message substrings. Preserve validation that the pod remains Pending and has a PodScheduled condition with False status, and provide a clear failure when that condition is absent.
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBrittle error-message substring matching for skip-vs-fail decision.
strings.Contains(prerequisitesError.Error(), "not found or failed")couples control flow to free-text error wording fromprereqInstaller.InstallAll. If that message ever changes, a genuine "tooling missing" case would incorrectlyg.Fail(or vice versa). Prefer a typed/sentinel error (orerrors.Is) fromPrerequisitesInstallerto distinguish "tooling unavailable" from other failures.🤖 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/consumable/consumable_dra.go` around lines 93 - 98, Replace the string-based check in the prerequisites handling block with a typed or sentinel error classification exposed by PrerequisitesInstaller, using errors.Is or the established equivalent to identify unavailable tooling. Keep g.Skip for that specific condition and route all other prerequisite installation errors to g.Fail.test/extended/node/dra/common/capacity_validator.go (1)
37-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
HasCapacityDevicesmasks real API errors as "no capacity".On a
Listfailure it logs and returnsfalse, identical to the legitimate "no devices publish capacity yet" case. This function is polled for up to 2 minutes inconsumable_dra.go'sBeforeAll(line 117-122); a persistent RBAC/API error would silently manifest as a suite-wideg.Skip("... no devices with Capacity published after upgrade")instead of a clear failure, hiding real infrastructure problems from CI.Consider returning
(bool, error)so the poller can fail fast on a genuine error and only continue polling on "not yet published".♻️ Proposed fix
-func (cv *CapacityValidator) HasCapacityDevices(ctx context.Context) bool { +func (cv *CapacityValidator) HasCapacityDevices(ctx context.Context) (bool, error) { sliceList, err := cv.client.ResourceV1().ResourceSlices().List(ctx, cv.listOptions()) if err != nil { - framework.Logf("Failed to check for capacity devices: %v", err) - return false + return false, fmt.Errorf("failed to list ResourceSlices: %w", err) } for _, slice := range sliceList.Items { for _, device := range slice.Spec.Devices { if len(device.Capacity) > 0 { - return true + return true, nil } } } - return false + 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/common/capacity_validator.go` around lines 37 - 51, Update CapacityValidator.HasCapacityDevices to return both the capacity result and any ResourceSlices List error instead of converting API failures to false. Propagate the error through consumable_dra.go's BeforeAll polling path so genuine API or RBAC failures fail fast, while only a successful false result continues polling for capacity publication.
🤖 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/consumable/consumable_dra.go`:
- Around line 237-282: Update the cleanup deferred functions for pod1, pod2,
claim1, and claim2 in the consumable DRA test to wait until each resource is
actually deleted after issuing Delete. Reuse the existing Kubernetes wait
helpers or ResourceClaim polling utilities, and ensure all four resources are
absent before the next Ordered spec begins.
---
Nitpick comments:
In `@test/extended/node/dra/common/capacity_validator.go`:
- Around line 37-51: Update CapacityValidator.HasCapacityDevices to return both
the capacity result and any ResourceSlices List error instead of converting API
failures to false. Propagate the error through consumable_dra.go's BeforeAll
polling path so genuine API or RBAC failures fail fast, while only a successful
false result continues polling for capacity publication.
In `@test/extended/node/dra/consumable/consumable_dra.go`:
- Around line 379-400: Update the unschedulable verification in the overflow pod
polling block to rely on stable pod scheduling status/reason fields rather than
matching free-text cond.Message substrings. Preserve validation that the pod
remains Pending and has a PodScheduled condition with False status, and provide
a clear failure when that condition is absent.
- Around line 93-98: Replace the string-based check in the prerequisites
handling block with a typed or sentinel error classification exposed by
PrerequisitesInstaller, using errors.Is or the established equivalent to
identify unavailable tooling. Keep g.Skip for that specific condition and route
all other prerequisite installation errors to g.Fail.
🪄 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: 380f6d89-e760-490b-82b8-1deacdd9d2c5
📒 Files selected for processing (4)
test/extended/include.gotest/extended/node/dra/common/capacity_validator.gotest/extended/node/dra/common/resource_builder.gotest/extended/node/dra/consumable/consumable_dra.go
| defer func() { | ||
| if delErr := dracommon.DeleteResourceClaim(ctx, oc.KubeFramework().ClientSet, oc.Namespace(), claim1Name); delErr != nil { | ||
| framework.Logf("Warning: failed to delete ResourceClaim %s/%s: %v", oc.Namespace(), claim1Name, delErr) | ||
| } | ||
| }() | ||
|
|
||
| g.By("Creating first pod pinned to target node") | ||
| pod1 := builder.BuildLongRunningPodWithClaim(pod1Name, claim1Name, "") | ||
| pod1.Spec.NodeSelector = map[string]string{"kubernetes.io/hostname": nodeName} | ||
| pod1, err = oc.KubeFramework().ClientSet.CoreV1().Pods(oc.Namespace()).Create(ctx, pod1, metav1.CreateOptions{}) | ||
| framework.ExpectNoError(err) | ||
| defer func() { | ||
| gracePeriod := int64(10) | ||
| if delErr := oc.KubeFramework().ClientSet.CoreV1().Pods(oc.Namespace()).Delete(ctx, pod1Name, metav1.DeleteOptions{GracePeriodSeconds: &gracePeriod}); delErr != nil && !errors.IsNotFound(delErr) { | ||
| framework.Logf("Warning: failed to delete pod %s/%s: %v", oc.Namespace(), pod1Name, delErr) | ||
| } | ||
| }() | ||
|
|
||
| g.By("Waiting for first pod to be running") | ||
| err = e2epod.WaitForPodRunningInNamespace(ctx, oc.KubeFramework().ClientSet, pod1) | ||
| framework.ExpectNoError(err, "First pod failed to start") | ||
|
|
||
| g.By("Creating second ResourceClaim requesting 10G ingress / 10G egress") | ||
| claim2 := builder.BuildResourceClaimWithCapacity(claim2Name, deviceClassName, 1, map[string]string{ | ||
| "ingressBandwidth": "10G", | ||
| "egressBandwidth": "10G", | ||
| }) | ||
| err = dracommon.CreateResourceClaim(ctx, oc.KubeFramework().ClientSet, oc.Namespace(), claim2) | ||
| framework.ExpectNoError(err) | ||
| defer func() { | ||
| if delErr := dracommon.DeleteResourceClaim(ctx, oc.KubeFramework().ClientSet, oc.Namespace(), claim2Name); delErr != nil { | ||
| framework.Logf("Warning: failed to delete ResourceClaim %s/%s: %v", oc.Namespace(), claim2Name, delErr) | ||
| } | ||
| }() | ||
|
|
||
| g.By("Creating second pod pinned to same node") | ||
| pod2 := builder.BuildLongRunningPodWithClaim(pod2Name, claim2Name, "") | ||
| pod2.Spec.NodeSelector = map[string]string{"kubernetes.io/hostname": nodeName} | ||
| pod2, err = oc.KubeFramework().ClientSet.CoreV1().Pods(oc.Namespace()).Create(ctx, pod2, metav1.CreateOptions{}) | ||
| framework.ExpectNoError(err) | ||
| defer func() { | ||
| gracePeriod := int64(10) | ||
| if delErr := oc.KubeFramework().ClientSet.CoreV1().Pods(oc.Namespace()).Delete(ctx, pod2Name, metav1.DeleteOptions{GracePeriodSeconds: &gracePeriod}); delErr != nil && !errors.IsNotFound(delErr) { | ||
| framework.Logf("Warning: failed to delete pod %s/%s: %v", oc.Namespace(), pod2Name, delErr) | ||
| } | ||
| }() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect DeleteResourceClaim/DeleteDeviceClass/CreateResourceClaim to see if they block until removal.
rg -n -B2 -A25 'func DeleteResourceClaim|func DeleteDeviceClass|func CreateResourceClaim' test/extended/node/dra/common
# Check whether exutil's per-spec project teardown waits for full namespace deletion.
rg -n -B2 -A20 'func NewCLIWithPodSecurityLevel|func \(c \*CLI\) TeardownProject|func.*[Tt]eardown' test/extended/utilRepository: openshift/origin
Length of output: 6260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test sections and surrounding Ordered structure.
sed -n '200,520p' test/extended/node/dra/consumable/consumable_dra.go
# Find the Ordered container and any shared cleanup between these specs.
rg -n 'Ordered|AfterEach|BeforeEach|DeferCleanup|TeardownProject|DeleteResourceClaim|WaitForPodNotFoundInNamespace' test/extended/node/dra/consumable/consumable_dra.go test/extended/util/client.go test/extended/node/dra/common/crud_helpers.goRepository: openshift/origin
Length of output: 19155
Wait for pod and ResourceClaim deletion before the next Ordered spec Delete() only sends the request here, so the following full-capacity allocation can race stale bandwidth. Add a wait for both resources to disappear before starting the next spec.
🤖 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/consumable/consumable_dra.go` around lines 237 - 282,
Update the cleanup deferred functions for pod1, pod2, claim1, and claim2 in the
consumable DRA test to wait until each resource is actually deleted after
issuing Delete. Reuse the existing Kubernetes wait helpers or ResourceClaim
polling utilities, and ensure all four resources are absent before the next
Ordered spec begins.
2 participants
Introduce tests for DRA consumable capacity (bandwidth sharing on NIC devices): resource slice validation, multi-allocation on the same device, capacity exhaustion scheduling, and capacity release after pod deletion.
Signed-ff-by: Sai Ramesh Vanka svanka@redhat.com
Summary by CodeRabbit