Improve test output for nodes should be ready test#31419
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
approved
WalkthroughNode consistency analysis now returns unready or unschedulable nodes with problematic conditions and includes those details in failure messages. Tests validate the structured result and condition-classification rules. ChangesNode consistency reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (14 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 (1)
pkg/e2eanalysis/e2e_analysis_test.go (1)
204-211: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the new structured condition payload.
The updated test verifies only node names, so a regression that drops
n.conditionswould still pass. Add expected condition data to the table and assert it, includingNodePIDPressure,NodeNetworkUnavailable, and the default classification branch.As per coding guidelines, validate unit-test changes with
go vet ./...,go test ./pkg/...,make verify, andmake check.Also applies to: 214-254
🤖 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 `@pkg/e2eanalysis/e2e_analysis_test.go` around lines 204 - 211, Add structured condition expectations to the test cases driving getUnreadyOrUnschedulableNodes, covering NodePIDPressure, NodeNetworkUnavailable, and the default classification branch. Update the assertions to compare each returned node’s conditions as well as names, preserving the existing name matching, then validate with go vet ./..., go test ./pkg/..., make verify, and make check.Source: Coding guidelines
🤖 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 `@pkg/e2eanalysis/e2e_analysis.go`:
- Around line 663-674: Update getUnreadyOrUnschedulableNodes and the unreadyNode
failure rendering to preserve scheduling blockers, including Spec.Unschedulable,
even when no problematic status condition exists. Record the unschedulable
reason alongside collected conditions for every blocker recognized by
IsNodeSchedulable, and ensure the output prints that reason instead of an empty
explanation.
---
Nitpick comments:
In `@pkg/e2eanalysis/e2e_analysis_test.go`:
- Around line 204-211: Add structured condition expectations to the test cases
driving getUnreadyOrUnschedulableNodes, covering NodePIDPressure,
NodeNetworkUnavailable, and the default classification branch. Update the
assertions to compare each returned node’s conditions as well as names,
preserving the existing name matching, then validate with go vet ./..., go test
./pkg/..., make verify, and make check.
🪄 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: 231d5edd-9bbd-4ee5-bc3e-3a9928d0bb07
📒 Files selected for processing (2)
pkg/e2eanalysis/e2e_analysis.gopkg/e2eanalysis/e2e_analysis_test.go
|
/test images |
|
Scheduling required tests: |
| names[i] = n.name | ||
| } | ||
| message := fmt.Sprintf("Found %d out of %d Nodes not Ready or unschedulable: %s\n", | ||
| len(notReadyNodes), len(nodeList.Items), strings.Join(names, ", ")) |
There was a problem hiding this comment.
Do we need the names listed out here, if they are going to be listed out one separate lines below, too? Or is the point to make the names prominent without having to scan through the rest of the message?
| for i, n := range notReadyNodes { | ||
| names[i] = n.name | ||
| } | ||
| message := fmt.Sprintf("Found %d out of %d Nodes not Ready or unschedulable: %s\n", |
There was a problem hiding this comment.
Consider using strings.Builder to build out the message.
| return c.Status != k8sv1.ConditionTrue | ||
| case k8sv1.NodeMemoryPressure, k8sv1.NodeDiskPressure, k8sv1.NodePIDPressure, k8sv1.NodeNetworkUnavailable: | ||
| return c.Status != k8sv1.ConditionFalse | ||
| default: |
There was a problem hiding this comment.
I am confused by this default case. If we want to include all unknown conditions that are not False, why are we explicitly specifying the four conditions above?
I suggest we either drop the case k8sv1.NodeMemoryPressure, k8sv1.NodeDiskPressure, k8sv1.NodePIDPressure, k8sv1.NodeNetworkUnavailable: or ignore unknown conditions.
There was a problem hiding this comment.
Yeah that doesn't make a lot of sense. I'll just do these four, I don't know what else is in there and if we see not ready but all of these are false, there are other artifacts to check.
Now allows us to see the problematic conditions directly in test output instead of fishing through artifacts
c7f2a67 to
b978b31
Compare
dgoodwin
left a comment
dgoodwin
left a comment
There was a problem hiding this comment.
Ready for another look
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/e2eanalysis/e2e_analysis.go (1)
575-584: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle the formatting write errors.
fmt.Fprintfandstrings.Builder.WriteStringreturn errors here, but all four are discarded. Propagate them throughcheckMachineNodeConsistency.Proposed fix
- fmt.Fprintf(&b, "Found %d out of %d Nodes not Ready or unschedulable:\n", - len(notReadyNodes), len(nodeList.Items)) + if _, err := fmt.Fprintf(&b, "Found %d out of %d Nodes not Ready or unschedulable:\n", + len(notReadyNodes), len(nodeList.Items)); err != nil { + return -1, fmt.Errorf("format node consistency report: %w", err) + } for _, n := range notReadyNodes { - fmt.Fprintf(&b, "Node %s:\n", n.name) + if _, err := fmt.Fprintf(&b, "Node %s:\n", n.name); err != nil { + return -1, fmt.Errorf("format node consistency report: %w", err) + } if n.unschedulable { - b.WriteString(" Spec.Unschedulable=true (cordoned)\n") + if _, err := b.WriteString(" Spec.Unschedulable=true (cordoned)\n"); err != nil { + return -1, fmt.Errorf("format node consistency report: %w", err) + } } for _, c := range n.conditions { - fmt.Fprintf(&b, " %s=%s reason=%s: %s\n", c.Type, c.Status, c.Reason, c.Message) + if _, err := fmt.Fprintf(&b, " %s=%s reason=%s: %s\n", c.Type, c.Status, c.Reason, c.Message); err != nil { + return -1, fmt.Errorf("format node consistency report: %w", err) + } } }As per path instructions, “Never ignore error returns.”
🤖 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 `@pkg/e2eanalysis/e2e_analysis.go` around lines 575 - 584, Update checkMachineNodeConsistency to handle and propagate errors from every fmt.Fprintf and strings.Builder.WriteString call while constructing the node consistency report, returning immediately on failure instead of discarding the errors.Source: Path instructions
🤖 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.
Nitpick comments:
In `@pkg/e2eanalysis/e2e_analysis.go`:
- Around line 575-584: Update checkMachineNodeConsistency to handle and
propagate errors from every fmt.Fprintf and strings.Builder.WriteString call
while constructing the node consistency report, returning immediately on failure
instead of discarding the errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d0da093a-4315-423d-a73e-b849cd7ed4b1
📒 Files selected for processing (2)
pkg/e2eanalysis/e2e_analysis.gopkg/e2eanalysis/e2e_analysis_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/e2eanalysis/e2e_analysis_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, mstaeble The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Scheduling required tests: |
|
@dgoodwin: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
Now allows us to see the problematic conditions directly in test output instead of fishing through artifacts
Summary by CodeRabbit
Bug Fixes
Tests