OCPBUGS-62144: Makes sure rendering for egress IP reachabilityTimeout triggers restart of ovnkube (node/control) pods#2955
Conversation
Signed-off-by: Raphael Rosa <raprosa@redhat.com>
openshift-ci-robot
added
jira/valid-reference
|
@raphaelvrosa: This pull request references Jira Issue OCPBUGS-62144, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughDocuments Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v0.8.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17356 characters] ... ired in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20251215205346-5ee0d033ba5b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.35.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.35.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: raphaelvrosa 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 (1)
pkg/network/ovn_kubernetes_test.go (1)
3951-3979: Add a same-value rerender case to verify hash stability.The table only checks
nil -> 0 -> 10. Add a repeated value case (for example,10again with a new pointer) and assert hashes do not change, so this test catches non-deterministic hash inputs.Suggested test case addition
{ name: "Reachability timeout changed to 10", reachabilityTimeout: ptrToUint32(10), expectHashChanged: true, expectKubernetesFeatureReachability: true, expectErr: false, }, + { + name: "Reachability timeout remains 10", + reachabilityTimeout: ptrToUint32(10), + expectHashChanged: false, + expectKubernetesFeatureReachability: true, + expectErr: false, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/network/ovn_kubernetes_test.go` around lines 3951 - 3979, Add a new test case in the testCases slice used in ovn_kubernetes_test.go that repeats the same value for reachabilityTimeout (e.g., use ptrToUint32(10) again with a freshly allocated pointer) and set expectHashChanged to false and expectKubernetesFeatureReachability to true; this ensures the code path that computes the hash (referenced by the testCases slice, reachabilityTimeout, and expectHashChanged) verifies stability across a rerender with identical logical values even when pointer instances differ.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/network/ovn_kubernetes.go`:
- Around line 507-509: The hash currently includes pointer addresses because
data.Data["ReachabilityTotalTimeoutSeconds"] is a *uint32; update the formatting
in the nodeNoOverlayHashData construction (and the similar hash at lines
~519–522) to dereference the pointer and use a stable scalar representation
(e.g., the uint32 value with %d or %v) instead of the pointer itself, and guard
with a nil-check to avoid panics when the pointer is nil; locate the
nodeNoOverlayHashData assembly and the other hash creation that reference
data.Data["ReachabilityTotalTimeoutSeconds"] and replace the pointer formatting
with the dereferenced value.
---
Nitpick comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 3951-3979: Add a new test case in the testCases slice used in
ovn_kubernetes_test.go that repeats the same value for reachabilityTimeout
(e.g., use ptrToUint32(10) again with a freshly allocated pointer) and set
expectHashChanged to false and expectKubernetesFeatureReachability to true; this
ensures the code path that computes the hash (referenced by the testCases slice,
reachabilityTimeout, and expectHashChanged) verifies stability across a rerender
with identical logical values even when pointer instances differ.
🪄 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: a52ef0ea-c53e-4daf-b648-e9d0a4a9457a
📒 Files selected for processing (3)
README.mdpkg/network/ovn_kubernetes.gopkg/network/ovn_kubernetes_test.go
Signed-off-by: Raphael Rosa <raprosa@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/network/ovn_kubernetes_test.go (1)
4034-4037: Avoid forcing HostedControlPlane in a generic reachability/hash rollout testLine 4034-Line 4037 forces the HyperShift path, which can under-cover the default self-hosted rendering path this PR targets. Prefer default infra here, or add explicit table coverage for both hosted and self-hosted modes.
Minimal scope fix
- // Set is as Hypershift hosted control plane. - bootstrapResult.Infra = bootstrap.InfraStatus{} - bootstrapResult.Infra.HostedControlPlane = &hypershift.HostedControlPlane{} + // Keep default self-hosted path in this test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/network/ovn_kubernetes_test.go` around lines 4034 - 4037, The test is forcing the HyperShift path by setting bootstrapResult.Infra.HostedControlPlane, which skips the default self-hosted rendering path; update the test so it leaves bootstrapResult.Infra as the default (do not set HostedControlPlane) or convert this case into table-driven tests that explicitly include both scenarios (one case with bootstrapResult.Infra.HostedControlPlane set and one with it unset) and assert renderOVNKubernetes outputs for each; locate the setup around bootstrapResult.Infra and the call to renderOVNKubernetes to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 4059-4067: The test currently only checks for the presence of the
key in ovnkubeConf but should assert the exact rendered timeout value; update
the branch that runs when tc.expectKubernetesFeatureReachability is true to
assert that ovnkubeConf contains the full key=value pair (e.g.
"egressip-reachability-total-timeout=<expected>" ) using the expected value from
the test case (add or use a field on tc such as
tc.egressIPReachabilityTotalTimeout or tc.egressipReachabilityTotalTimeout) so
the test fails if the timeout is rendered incorrectly.
---
Nitpick comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 4034-4037: The test is forcing the HyperShift path by setting
bootstrapResult.Infra.HostedControlPlane, which skips the default self-hosted
rendering path; update the test so it leaves bootstrapResult.Infra as the
default (do not set HostedControlPlane) or convert this case into table-driven
tests that explicitly include both scenarios (one case with
bootstrapResult.Infra.HostedControlPlane set and one with it unset) and assert
renderOVNKubernetes outputs for each; locate the setup around
bootstrapResult.Infra and the call to renderOVNKubernetes to make this change.
🪄 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: 3a133127-6819-4195-9d53-9a25368cdfe5
📒 Files selected for processing (2)
pkg/network/ovn_kubernetes.gopkg/network/ovn_kubernetes_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/network/ovn_kubernetes.go
Signed-off-by: Raphael Rosa <raprosa@redhat.com>
| } | ||
| data.Data["OVNKubeConfigHash"] = hex.EncodeToString(h.Sum(nil)) | ||
|
|
||
| var reachabilityTimeoutStr string |
There was a problem hiding this comment.
Make sure hash is consistent when values do not change.
|
Fix in some CI tests depend on: https://github.com/openshift/origin/pull/30961/changes |
/jira refresh |
|
/retest-required |
|
@raphaelvrosa: 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. |
raphaelvrosa
changed the title
openshift-ci-robot
added
jira/valid-bug
|
@raphaelvrosa: This pull request references Jira Issue OCPBUGS-62144, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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: GitHub didn't allow me to request PR reviews from the following users: huiran0826. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
|
@raphaelvrosa: This pull request references Jira Issue OCPBUGS-62144, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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: GitHub didn't allow me to request PR reviews from the following users: huiran0826. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 |
2 participants
Issue: EgressIP
ReachabilityTotalTimeoutSecondsis rendered by ovnkube-config (configmap), but ovnkube node and control pods are not restarted to have the new value applied in their settings.Solution: Make sure the Hash inside the specs in ovnkube node and control pods take into account the changes in the
ReachabilityTotalTimeoutSecondssetting. When this value is changed, it then triggers the restart of those pods, reloading the parameter in their configuration.Adds test cases to evaluate the rendering and the hash changes in ovnkube node and control pod specs.
Updates README to document that reachabilityTimeoutSeconds is enabled for runtime updates.