NO-JIRA: fix CUDN status condition tests for new TransportAccepted condition#30958
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughTest updates: assertions now allow matching an expected condition within a set (use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jluhrsen
force-pushed
the
jluhrsen/fix-cudn-status-tests
branch
from
ca2c889 to
620e6e2
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/networking/network_segmentation.go`:
- Around line 1484-1508: The helper currently selects the first condition with
Type "NetworkCreated"/"NetworkReady" (networkCond) which can be incorrect if a
later one is the valid condition; update the logic to scan all entries in
conditions and pick the first condition that satisfies all required checks
(Status == metav1.ConditionTrue, Reason is "NetworkAttachmentDefinitionCreated"
or "NetworkAttachmentDefinitionReady", Message contains
"NetworkAttachmentDefinition has been created in following namespaces:" and
contains every namespace from expectedActiveNsNames); only return success when
such a matching condition is found, otherwise return a clear error listing
conditions examined or that no matching network condition met all criteria.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d330c1bd-d697-4330-b16b-a064f19bf9c4
📒 Files selected for processing (1)
test/extended/networking/network_segmentation.go
| // Find NetworkCreated/NetworkReady condition among all conditions | ||
| var networkCond *metav1.Condition | ||
| for i := range conditions { | ||
| if conditions[i].Type == "NetworkCreated" || conditions[i].Type == "NetworkReady" { | ||
| networkCond = &conditions[i] | ||
| break | ||
| } | ||
| } | ||
| if c.Status != metav1.ConditionTrue { | ||
| return fmt.Errorf("expected True status in %v", c) | ||
| if networkCond == nil { | ||
| return fmt.Errorf("NetworkCreated/NetworkReady condition not found in conditions: %v", conditions) | ||
| } | ||
|
|
||
| if networkCond.Status != metav1.ConditionTrue { | ||
| return fmt.Errorf("expected True status in %v", networkCond) | ||
| } | ||
| if c.Reason != "NetworkAttachmentDefinitionCreated" && c.Reason != "NetworkAttachmentDefinitionReady" { | ||
| return fmt.Errorf("expected NetworkAttachmentDefinitionCreated/NetworkAttachmentDefinitionReady reason in %v", c) | ||
| if networkCond.Reason != "NetworkAttachmentDefinitionCreated" && networkCond.Reason != "NetworkAttachmentDefinitionReady" { | ||
| return fmt.Errorf("expected NetworkAttachmentDefinitionCreated/NetworkAttachmentDefinitionReady reason in %v", networkCond) | ||
| } | ||
| if !strings.Contains(c.Message, "NetworkAttachmentDefinition has been created in following namespaces:") { | ||
| return fmt.Errorf("expected \"NetworkAttachmentDefinition has been created in following namespaces:\" in %s", c.Message) | ||
| if !strings.Contains(networkCond.Message, "NetworkAttachmentDefinition has been created in following namespaces:") { | ||
| return fmt.Errorf("expected \"NetworkAttachmentDefinition has been created in following namespaces:\" in %s", networkCond.Message) | ||
| } | ||
|
|
||
| for _, ns := range expectedActiveNsNames { | ||
| if !strings.Contains(c.Message, ns) { | ||
| return fmt.Errorf("expected to find %q namespace in %s", ns, c.Message) | ||
| if !strings.Contains(networkCond.Message, ns) { | ||
| return fmt.Errorf("expected to find %q namespace in %s", ns, networkCond.Message) |
There was a problem hiding this comment.
This still assumes the first network condition is the right one.
It no longer depends on conditions[0], but it still breaks on the first NetworkCreated/NetworkReady entry. If both are present and only a later one matches the expected status/message, the helper still fails even though the CR status is valid.
Suggested fix
- // Find NetworkCreated/NetworkReady condition among all conditions
- var networkCond *metav1.Condition
- for i := range conditions {
- if conditions[i].Type == "NetworkCreated" || conditions[i].Type == "NetworkReady" {
- networkCond = &conditions[i]
- break
- }
- }
- if networkCond == nil {
- return fmt.Errorf("NetworkCreated/NetworkReady condition not found in conditions: %v", conditions)
- }
-
- if networkCond.Status != metav1.ConditionTrue {
- return fmt.Errorf("expected True status in %v", networkCond)
- }
- if networkCond.Reason != "NetworkAttachmentDefinitionCreated" && networkCond.Reason != "NetworkAttachmentDefinitionReady" {
- return fmt.Errorf("expected NetworkAttachmentDefinitionCreated/NetworkAttachmentDefinitionReady reason in %v", networkCond)
- }
- if !strings.Contains(networkCond.Message, "NetworkAttachmentDefinition has been created in following namespaces:") {
- return fmt.Errorf("expected \"NetworkAttachmentDefinition has been created in following namespaces:\" in %s", networkCond.Message)
- }
-
- for _, ns := range expectedActiveNsNames {
- if !strings.Contains(networkCond.Message, ns) {
- return fmt.Errorf("expected to find %q namespace in %s", ns, networkCond.Message)
- }
- }
- return nil
+ for i := range conditions {
+ cond := conditions[i]
+ if cond.Type != "NetworkCreated" && cond.Type != "NetworkReady" {
+ continue
+ }
+ if cond.Status != metav1.ConditionTrue {
+ continue
+ }
+ if cond.Reason != "NetworkAttachmentDefinitionCreated" && cond.Reason != "NetworkAttachmentDefinitionReady" {
+ continue
+ }
+ if !strings.Contains(cond.Message, "NetworkAttachmentDefinition has been created in following namespaces:") {
+ continue
+ }
+
+ missingNamespace := false
+ for _, ns := range expectedActiveNsNames {
+ if !strings.Contains(cond.Message, ns) {
+ missingNamespace = true
+ break
+ }
+ }
+ if !missingNamespace {
+ return nil
+ }
+ }
+ return fmt.Errorf("no NetworkCreated/NetworkReady condition matched expected active namespaces: %v", conditions)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Find NetworkCreated/NetworkReady condition among all conditions | |
| var networkCond *metav1.Condition | |
| for i := range conditions { | |
| if conditions[i].Type == "NetworkCreated" || conditions[i].Type == "NetworkReady" { | |
| networkCond = &conditions[i] | |
| break | |
| } | |
| } | |
| if c.Status != metav1.ConditionTrue { | |
| return fmt.Errorf("expected True status in %v", c) | |
| if networkCond == nil { | |
| return fmt.Errorf("NetworkCreated/NetworkReady condition not found in conditions: %v", conditions) | |
| } | |
| if networkCond.Status != metav1.ConditionTrue { | |
| return fmt.Errorf("expected True status in %v", networkCond) | |
| } | |
| if c.Reason != "NetworkAttachmentDefinitionCreated" && c.Reason != "NetworkAttachmentDefinitionReady" { | |
| return fmt.Errorf("expected NetworkAttachmentDefinitionCreated/NetworkAttachmentDefinitionReady reason in %v", c) | |
| if networkCond.Reason != "NetworkAttachmentDefinitionCreated" && networkCond.Reason != "NetworkAttachmentDefinitionReady" { | |
| return fmt.Errorf("expected NetworkAttachmentDefinitionCreated/NetworkAttachmentDefinitionReady reason in %v", networkCond) | |
| } | |
| if !strings.Contains(c.Message, "NetworkAttachmentDefinition has been created in following namespaces:") { | |
| return fmt.Errorf("expected \"NetworkAttachmentDefinition has been created in following namespaces:\" in %s", c.Message) | |
| if !strings.Contains(networkCond.Message, "NetworkAttachmentDefinition has been created in following namespaces:") { | |
| return fmt.Errorf("expected \"NetworkAttachmentDefinition has been created in following namespaces:\" in %s", networkCond.Message) | |
| } | |
| for _, ns := range expectedActiveNsNames { | |
| if !strings.Contains(c.Message, ns) { | |
| return fmt.Errorf("expected to find %q namespace in %s", ns, c.Message) | |
| if !strings.Contains(networkCond.Message, ns) { | |
| return fmt.Errorf("expected to find %q namespace in %s", ns, networkCond.Message) | |
| for i := range conditions { | |
| cond := conditions[i] | |
| if cond.Type != "NetworkCreated" && cond.Type != "NetworkReady" { | |
| continue | |
| } | |
| if cond.Status != metav1.ConditionTrue { | |
| continue | |
| } | |
| if cond.Reason != "NetworkAttachmentDefinitionCreated" && cond.Reason != "NetworkAttachmentDefinitionReady" { | |
| continue | |
| } | |
| if !strings.Contains(cond.Message, "NetworkAttachmentDefinition has been created in following namespaces:") { | |
| continue | |
| } | |
| missingNamespace := false | |
| for _, ns := range expectedActiveNsNames { | |
| if !strings.Contains(cond.Message, ns) { | |
| missingNamespace = true | |
| break | |
| } | |
| } | |
| if !missingNamespace { | |
| return nil | |
| } | |
| } | |
| return fmt.Errorf("no NetworkCreated/NetworkReady condition matched expected active namespaces: %v", conditions) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/networking/network_segmentation.go` around lines 1484 - 1508,
The helper currently selects the first condition with Type
"NetworkCreated"/"NetworkReady" (networkCond) which can be incorrect if a later
one is the valid condition; update the logic to scan all entries in conditions
and pick the first condition that satisfies all required checks (Status ==
metav1.ConditionTrue, Reason is "NetworkAttachmentDefinitionCreated" or
"NetworkAttachmentDefinitionReady", Message contains
"NetworkAttachmentDefinition has been created in following namespaces:" and
contains every namespace from expectedActiveNsNames); only return success when
such a matching condition is found, otherwise return a clear error listing
conditions examined or that no matching network condition met all criteria.
upstream ovn-kubernetes commit [0] added a new TransportAccepted status condition to ClusterUserDefinedNetwork resources. two tests and a helper function assumed the condition list only contained network conditions and broke when TransportAccepted appeared. these tests are duplicates of the upstream ovn-kubernetes e2e tests which have already been fixed in the same commit. the upstream tests will run in openshift CI via the OTE binary once that framework is in production. these tests can be removed at that point, but until then this keeps them working. fixes: - validateClusterUDNStatusReportsActiveNamespacesFunc: iterate conditions to find NetworkCreated/NetworkReady instead of assuming conditions - "should report not-ready" test: use ContainElement instead of ConsistOf so extra conditions don't cause failures [0] ovn-kubernetes/ovn-kubernetes@b855462 Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jluhrsen
force-pushed
the
jluhrsen/fix-cudn-status-tests
branch
from
620e6e2 to
6dcc541
Compare
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn openshift/ovn-kubernetes#3114 |
|
@jluhrsen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d6c9aec0-2faf-11f1-9119-eb5d42984525-0 |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/networking/network_segmentation.go (1)
1484-1508:⚠️ Potential issue | 🟠 MajorStill order-dependent across
NetworkCreated/NetworkReady.This no longer relies on
conditions[0], but it still validates only the first network condition it sees. If a laterNetworkCreated/NetworkReadyentry is the one that actually matches, the helper returns a false failure.Suggested fix
- // Find NetworkCreated/NetworkReady condition among all conditions - var networkCond *metav1.Condition - for i := range conditions { - if conditions[i].Type == "NetworkCreated" || conditions[i].Type == "NetworkReady" { - networkCond = &conditions[i] - break - } - } - if networkCond == nil { - return fmt.Errorf("NetworkCreated/NetworkReady condition not found in conditions: %v", conditions) - } - - if networkCond.Status != metav1.ConditionTrue { - return fmt.Errorf("expected True status in %v", networkCond) - } - if networkCond.Reason != "NetworkAttachmentDefinitionCreated" && networkCond.Reason != "NetworkAttachmentDefinitionReady" { - return fmt.Errorf("expected NetworkAttachmentDefinitionCreated/NetworkAttachmentDefinitionReady reason in %v", networkCond) - } - if !strings.Contains(networkCond.Message, "NetworkAttachmentDefinition has been created in following namespaces:") { - return fmt.Errorf("expected \"NetworkAttachmentDefinition has been created in following namespaces:\" in %s", networkCond.Message) - } - - for _, ns := range expectedActiveNsNames { - if !strings.Contains(networkCond.Message, ns) { - return fmt.Errorf("expected to find %q namespace in %s", ns, networkCond.Message) - } - } - return nil + for i := range conditions { + cond := conditions[i] + if cond.Type != "NetworkCreated" && cond.Type != "NetworkReady" { + continue + } + if cond.Status != metav1.ConditionTrue { + continue + } + if cond.Reason != "NetworkAttachmentDefinitionCreated" && cond.Reason != "NetworkAttachmentDefinitionReady" { + continue + } + if !strings.Contains(cond.Message, "NetworkAttachmentDefinition has been created in following namespaces:") { + continue + } + + missingNamespace := false + for _, ns := range expectedActiveNsNames { + if !strings.Contains(cond.Message, ns) { + missingNamespace = true + break + } + } + if !missingNamespace { + return nil + } + } + return fmt.Errorf("no NetworkCreated/NetworkReady condition matched expected active namespaces: %v", conditions)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/network_segmentation.go` around lines 1484 - 1508, The current check picks the first condition matching Type "NetworkCreated" or "NetworkReady" and fails if that one doesn't match expected status/reason/message; instead, update the logic that iterates the conditions (the loop that sets networkCond from the conditions slice) to evaluate every condition whose Type is "NetworkCreated" or "NetworkReady" and return success as soon as one such condition satisfies Status==metav1.ConditionTrue, Reason in {"NetworkAttachmentDefinitionCreated","NetworkAttachmentDefinitionReady"}, contains the expected message prefix, and includes all namespaces from expectedActiveNsNames; only return an error after all matching conditions are checked and none satisfy all checks. Ensure you continue to reference the existing variables/identifiers (conditions, networkCond, expectedActiveNsNames) so the change is localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/networking/network_segmentation.go`:
- Around line 1484-1508: The current check picks the first condition matching
Type "NetworkCreated" or "NetworkReady" and fails if that one doesn't match
expected status/reason/message; instead, update the logic that iterates the
conditions (the loop that sets networkCond from the conditions slice) to
evaluate every condition whose Type is "NetworkCreated" or "NetworkReady" and
return success as soon as one such condition satisfies
Status==metav1.ConditionTrue, Reason in
{"NetworkAttachmentDefinitionCreated","NetworkAttachmentDefinitionReady"},
contains the expected message prefix, and includes all namespaces from
expectedActiveNsNames; only return an error after all matching conditions are
checked and none satisfy all checks. Ensure you continue to reference the
existing variables/identifiers (conditions, networkCond, expectedActiveNsNames)
so the change is localized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a673bbd-4d9c-4e3f-85fd-f927b00a2cee
📒 Files selected for processing (1)
test/extended/networking/network_segmentation.go
|
Scheduling required tests: |
|
/lgtm |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@jluhrsen: This pull request explicitly references no jira issue. 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-robot
added
the
verified
|
@kyrtapz: This PR has been marked as verified by 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jluhrsen, kyrtapz 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 |
openshift-ci
bot
added
the
approved
|
/retest-required |
|
The tests succeeded and the lane failed during must-gather |
|
@kyrtapz: kyrtapz unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers. 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 kubernetes-sigs/prow repository. |
|
/retest-required |
|
@jluhrsen: all tests passed! 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. |
3 participants
Summary
TransportAcceptedstatus condition to all ClusterUserDefinedNetwork resourcesvalidateClusterUDNStatusReportsActiveNamespacesFuncassumedconditions[0]was the network condition — now iterates to findNetworkCreated/NetworkReadyshould report not-readytest usedConsistOfwhich requires exact match — changed toContainElementso extra conditions (likeTransportAccepted) don't cause failuresTest plan
should delete managed NAD in namespaces that no longer apply to namespace-selectorpasses with the newTransportAcceptedcondition presentwhen primary network exist, ClusterUserDefinedNetwork status should report not-readypasses with the newTransportAcceptedcondition present🤖 Generated with Claude Code