Skip to content

Split ping probe into ping4 and ping6 for dual-stack support #1498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mkowalski wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Split ping probe into ping4 and ping6 for dual-stack support #1498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 87 additions & 25 deletions pkg/probe/probes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,24 @@ type Route struct {
}

const (
defaultGwRetrieveTimeout = 120 * time.Second
defaultGwProbeTimeout = 120 * time.Second
defaultDNSProbeTimeout = 120 * time.Second
apiServerProbeTimeout = 120 * time.Second
nodeReadinessProbeTimeout = 120 * time.Second
mainRoutingTableID = 254
ProbesTotalTimeout = defaultGwRetrieveTimeout +
defaultDNSProbeTimeout +
defaultDNSProbeTimeout +
apiServerProbeTimeout +
nodeReadinessProbeTimeout

// ProbesTotalTimeout is the worst-case total time for Select() + Run()
// on a dual-stack cluster where both ping4 and ping6 are active.
// Select: ping4 + ping6 + dns
// Run: ping4 + ping6 + dns + api-server + node-readiness
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
Comment on lines +70 to +77

@gemini-code-assist gemini-code-assist Bot Apr 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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-readiness
References
  1. 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.

)

func currentStateAsGJson() (gjson.Result, error) {
Expand Down Expand Up @@ -140,12 +147,20 @@ func checkNodeReadiness(ctx context.Context, cli client.Client) (bool, error) {
return false, nil
}

func defaultGw(currentState gjson.Result) (Route, error) {
func defaultGw4(currentState gjson.Result) (Route, error) {
return defaultGwByDestination(currentState, "0.0.0.0/0")
}

func defaultGw6(currentState gjson.Result) (Route, error) {
return defaultGwByDestination(currentState, "::/0")
}

func defaultGwByDestination(currentState gjson.Result, destination string) (Route, error) {
var found Route
currentState.Get("routes.running").ForEach(
func(_, v gjson.Result) bool {
// we want to pick the next hop related to the "main" table because we may have multiple tables
if (v.Get("destination").String() == "0.0.0.0/0" || v.Get("destination").String() == "::/0") &&
if v.Get("destination").String() == destination &&
v.Get("table-id").Int() == mainRoutingTableID {
found.nextHop = net.ParseIP(v.Get("next-hop-address").String())
found.iface = v.Get("next-hop-interface").String()
Expand All @@ -156,8 +171,8 @@ func defaultGw(currentState gjson.Result) (Route, error) {
)

if found.nextHop == nil {
msg := "default gw missing"
defaultGwLog := log.WithValues("path", "routes.running.next-hop-address", "table-id", mainRoutingTableID)
msg := fmt.Sprintf("default gw missing for destination %s", destination)
defaultGwLog := log.WithValues("path", "routes.running.next-hop-address", "table-id", mainRoutingTableID, "destination", destination)
defaultGwLogDebug := defaultGwLog.V(1)
if defaultGwLogDebug.Enabled() {
defaultGwLogDebug.Info(msg, "state", currentState.String())
Expand All @@ -169,25 +184,31 @@ func defaultGw(currentState gjson.Result) (Route, error) {
return found, nil
}

func pingCondition(cli client.Client, timeout time.Duration) wait.ConditionWithContextFunc {
func ping4Condition(cli client.Client, timeout time.Duration) wait.ConditionWithContextFunc {
return func(context.Context) (bool, error) {
return runPingByFamily(cli, defaultGw4)
}
}

func ping6Condition(cli client.Client, timeout time.Duration) wait.ConditionWithContextFunc {
return func(context.Context) (bool, error) {
return runPing(cli)
return runPingByFamily(cli, defaultGw6)
}
}

func runPing(_ client.Client) (bool, error) {
func runPingByFamily(_ client.Client, gwFunc func(gjson.Result) (Route, error)) (bool, error) {
gjsonCurrentState, err := currentStateAsGJson()
if err != nil {
return false, errors.Wrap(err, "failed retrieving current state to retrieve default gw")
}

defaultGw, err := defaultGw(gjsonCurrentState)
gw, err := gwFunc(gjsonCurrentState)
if err != nil {
log.Error(err, "failed to retrieve default gw")
return false, nil
}
Comment on lines 206 to 209

@gemini-code-assist gemini-code-assist Bot Apr 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
if err != nil {
log.Error(err, "failed to retrieve default gw")
return false, nil
}
gw, err := gwFunc(gjsonCurrentState)
if err != nil {
return false, nil
}


pingOutput, err := ping(defaultGw)
pingOutput, err := ping(gw)
if err != nil {
log.Error(err, fmt.Sprintf("error pinging default gateway -> output: '%s'", pingOutput))
return false, nil
Expand Down Expand Up @@ -270,18 +291,59 @@ func runDNS(_ client.Client, timeout time.Duration) (bool, error) {
// the internal connectivity probes
func Select(ctx context.Context, cli client.Client) []Probe {
probes := []Probe{}
externalConnectivityProbes := []Probe{
{
name: "ping",
externalConnectivityProbes := []Probe{}

// Pre-check which address families have a default gateway to avoid
// waiting for the full probe timeout on single-stack clusters.
// Default to including both families (conservative). Only skip a
// family when the other family already has a gateway, proving the
// network is initialized and the missing family genuinely doesn't
// exist. When neither family has a gateway the network may still
// be initializing (e.g. DHCP pending), so we include both and let
// the trial retry until the gateway appears.
hasGw4, hasGw6 := true, true
currentState, err := currentStateAsGJson()
if err != nil {
log.Info(fmt.Sprintf("WARNING: failed to retrieve current state for gateway pre-check, including all ping probes: %v", err))
} else {
_, err4 := defaultGw4(currentState)
_, err6 := defaultGw6(currentState)
if err4 == nil || err6 == nil {
// At least one family has a gateway — network is initialized.
// Only include families that actually have a gateway.
hasGw4 = err4 == nil
hasGw6 = err6 == nil
if !hasGw4 {
log.Info("No IPv4 default gateway found, skipping ping4 probe selection")
}
if !hasGw6 {
log.Info("No IPv6 default gateway found, skipping ping6 probe selection")
}
} else {
log.Info("No default gateway found for any address family, including all ping probes for retry")
}
}

if hasGw4 {
externalConnectivityProbes = append(externalConnectivityProbes, Probe{
name: "ping4",
timeout: defaultGwProbeTimeout,
condition: pingCondition,
},
{
name: "dns",
timeout: defaultDNSProbeTimeout,
condition: dnsCondition,
},
condition: ping4Condition,
})
}
if hasGw6 {
externalConnectivityProbes = append(externalConnectivityProbes, Probe{
name: "ping6",
timeout: defaultGwProbeTimeout,
condition: ping6Condition,
})
}

externalConnectivityProbes = append(externalConnectivityProbes, Probe{
name: "dns",
timeout: defaultDNSProbeTimeout,
condition: dnsCondition,
})

for _, p := range externalConnectivityProbes {
err := wait.PollUntilContextTimeout(ctx, time.Second, p.timeout, true /*immediate*/, p.condition(cli, p.timeout))
Expand Down
108 changes: 88 additions & 20 deletions pkg/probe/probes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
)

// nolint: funlen
func TestDefaultGatewayParsing(t *testing.T) {
func TestDefaultGw4Parsing(t *testing.T) {
tests := []struct {
desc string
status string
Expand All @@ -32,7 +32,7 @@ func TestDefaultGatewayParsing(t *testing.T) {
shouldErr bool
}{
{
desc: "one single gateway",
desc: "one single IPv4 gateway",
status: `routes:
running:
- destination: 172.30.0.0/16
Expand All @@ -48,7 +48,7 @@ func TestDefaultGatewayParsing(t *testing.T) {
expectedGw: "10.46.55.254",
expectedIface: "eth1",
}, {
desc: "two gateways, one on custom routing table",
desc: "two IPv4 gateways, one on custom routing table",
status: `routes:
running:
- destination: 172.30.0.0/16
Expand All @@ -68,7 +68,7 @@ func TestDefaultGatewayParsing(t *testing.T) {
expectedGw: "10.46.55.254",
expectedIface: "eth1",
}, {
desc: "no next-hop-address",
desc: "no IPv4 default gateway",
status: `routes:
running:
- destination: 172.30.0.0/16
Expand All @@ -78,6 +78,69 @@ func TestDefaultGatewayParsing(t *testing.T) {
`,
shouldErr: true,
}, {
desc: "only IPv6 gateway present",
status: `routes:
running:
- destination: ::/0
next-hop-interface: eth0
next-hop-address: fe80::dead:beef:fe51:782d
table-id: 254
`,
shouldErr: true,
}, {
desc: "dual-stack picks IPv4 gateway",
status: `routes:
running:
- destination: 0.0.0.0/0
next-hop-interface: eth0
next-hop-address: 10.46.55.254
table-id: 254
- destination: ::/0
next-hop-interface: eth0
next-hop-address: fe80::dead:beef:fe51:782d
table-id: 254
`,
expectedGw: "10.46.55.254",
expectedIface: "eth0",
},
}

for _, test := range tests {
t.Run(test.desc, func(t *testing.T) {
gJSON, err := yamlToGJson(test.status)
if err != nil {
t.Fatalf("failed to parse test status, %v", err)
}
gw, err := defaultGw4(gJSON)
if err != nil && !test.shouldErr {
t.Fatalf("unexpected error %v", err)
}
if test.shouldErr && err == nil {
t.Fatalf("expecting error, did not fail")
}

expectedRoute := Route{
nextHop: net.ParseIP(test.expectedGw),
iface: test.expectedIface,
}

if !expectedRoute.nextHop.Equal(gw.nextHop) || expectedRoute.iface != gw.iface {
t.Fatalf("expecting %+v, got %+v", expectedRoute, gw)
}
})
}
}

// nolint: funlen
func TestDefaultGw6Parsing(t *testing.T) {
tests := []struct {
desc string
status string
expectedGw string
expectedIface string
shouldErr bool
}{
{
desc: "one single IPv6 gateway",
status: `routes:
running:
Expand All @@ -104,35 +167,40 @@ func TestDefaultGatewayParsing(t *testing.T) {
expectedGw: "fe80::dead:beef:fe51:782d",
expectedIface: "eth0",
}, {
desc: "dual-stack with single gateway per IP stack",
desc: "no IPv6 default gateway",
status: `routes:
running:
- destination: 0.0.0.0/0
- destination: 172.30.0.0/16
next-hop-interface: eth0
next-hop-address: 10.46.55.254
next-hop-address: 169.254.169.4
table-id: 254
- destination: ::/0
next-hop-interface: eth0
next-hop-address: fe80::dead:beef:fe51:782d
`,
shouldErr: true,
}, {
desc: "only IPv4 gateway present",
status: `routes:
running:
- destination: 0.0.0.0/0
next-hop-interface: eth1
next-hop-address: 10.46.55.254
table-id: 254
`,
expectedGw: "10.46.55.254",
expectedIface: "eth0",
shouldErr: true,
}, {
desc: "dual-stack with missing IPv4 default gateway",
desc: "dual-stack picks IPv6 gateway",
status: `routes:
running:
- destination: 172.30.0.0/16
- destination: 0.0.0.0/0
next-hop-interface: eth0
next-hop-address: 169.254.169.4
next-hop-address: 10.46.55.254
table-id: 254
- destination: ::/0
next-hop-interface: eth1
next-hop-interface: eth0
next-hop-address: fe80::dead:beef:fe51:782d
table-id: 254
`,
expectedGw: "fe80::dead:beef:fe51:782d",
expectedIface: "eth1",
expectedIface: "eth0",
},
}

Expand All @@ -142,7 +210,7 @@ func TestDefaultGatewayParsing(t *testing.T) {
if err != nil {
t.Fatalf("failed to parse test status, %v", err)
}
defaultGw, err := defaultGw(gJSON)
gw, err := defaultGw6(gJSON)
if err != nil && !test.shouldErr {
t.Fatalf("unexpected error %v", err)
}
Expand All @@ -155,8 +223,8 @@ func TestDefaultGatewayParsing(t *testing.T) {
iface: test.expectedIface,
}

if !expectedRoute.nextHop.Equal(defaultGw.nextHop) || expectedRoute.iface != defaultGw.iface {
t.Fatalf("expecting %+v, got %+v", expectedRoute, defaultGw)
if !expectedRoute.nextHop.Equal(gw.nextHop) || expectedRoute.iface != gw.iface {
t.Fatalf("expecting %+v, got %+v", expectedRoute, gw)
}
})
}
Expand Down