Split ping probe into ping4 and ping6 for dual-stack support - #1498
Split ping probe into ping4 and ping6 for dual-stack support#1498mkowalski wants to merge 1 commit into
Conversation
kubevirt-bot
added
the
release-note
kubevirt-bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
The single "ping" connectivity probe checked only one default gateway (whichever appeared first), making it impossible to detect when only one IP stack broke after applying an NNCP on dual-stack systems. Split into separate "ping4" (0.0.0.0/0) and "ping6" (::/0) probes that track each IP stack independently. To avoid a 120s timeout penalty on single-stack clusters, Select() now pre-checks which address families have a default gateway and only includes the applicable ping probes. Both hasGw4 and hasGw6 default to false so that a transient pre-check failure skips ping probes entirely rather than adding a probe whose trial would time out for 120s on clusters lacking that address family. Fixes: nmstate#1411 Signed-off-by: Mat Kowalski <mkowalski@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
233afb8 to
af3cae9
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dual-stack support for connectivity probes by refactoring gateway detection and ping logic into IPv4 and IPv6 specific implementations. It also optimizes the probe selection process by pre-checking for available gateways, which prevents unnecessary timeouts on single-stack clusters. The review feedback suggests simplifying the calculation of ProbesTotalTimeout to avoid over-estimation and removing redundant logging within the ping polling loop to reduce log noise.
| ProbesTotalTimeout = defaultGwProbeTimeout + // Select: ping4 | ||
| defaultGwProbeTimeout + // Select: ping6 | ||
| defaultDNSProbeTimeout + // Select: dns | ||
| defaultGwProbeTimeout + // Run: ping4 | ||
| defaultGwProbeTimeout + // Run: ping6 | ||
| defaultDNSProbeTimeout + // Run: dns | ||
| apiServerProbeTimeout + // Run: api-server | ||
| nodeReadinessProbeTimeout // Run: node-readiness |
There was a problem hiding this comment.
The ProbesTotalTimeout is over-calculated. A probe is only included in the Run() phase if it successfully passes the Select() phase. If a probe fails in Select(), it consumes its timeout there but is not executed in Run(). If it succeeds in Select(), it consumes negligible time there and may consume its full timeout in Run(). Thus, each probe (ping4, ping6, dns, api-server, node-readiness) contributes at most once to the total worst-case timeout. The total should be the sum of the timeouts for these 5 distinct probes. This aligns with the preference for simpler implementations that cover known requirements without unnecessary complexity.
ProbesTotalTimeout = defaultGwProbeTimeout + // ping4
defaultGwProbeTimeout + // ping6
defaultDNSProbeTimeout + // dns
apiServerProbeTimeout + // api-server
nodeReadinessProbeTimeout // node-readinessReferences
- Changes that add complexity to 'future-proof' the code against hypothetical scenarios require strong justification. Without it, prefer the simpler implementation that covers known requirements.
| if err != nil { | ||
| log.Error(err, "failed to retrieve default gw") | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Logging an error here is redundant because defaultGwByDestination already logs (at debug level) when a gateway is missing. Furthermore, since this is called within a poll loop, it will log an error every second for 120 seconds if the gateway is not found, which is excessively noisy. Returning false, nil is sufficient for the poll to continue.
| if err != nil { | |
| log.Error(err, "failed to retrieve default gw") | |
| return false, nil | |
| } | |
| gw, err := gwFunc(gjsonCurrentState) | |
| if err != nil { | |
| return false, nil | |
| } |
|
@mkowalski: The following test failed, say
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
The single "ping" connectivity probe checked only one default gateway (whichever appeared first), making it impossible to detect when only one IP stack broke after applying an NNCP on dual-stack systems.
Split into separate "ping4" (0.0.0.0/0) and "ping6" (::/0) probes that track each IP stack independently. To avoid a 120s timeout penalty on single-stack clusters, Select() now pre-checks which address families have a default gateway and only includes the applicable ping probes.
Both hasGw4 and hasGw6 default to false so that a transient pre-check failure skips ping probes entirely rather than adding a probe whose trial would time out for 120s on clusters lacking that address family.
Fixes: #1411
Is this a BUG FIX or a FEATURE ?:
What this PR does / why we need it:
Special notes for your reviewer:
Release note: