proxy healthz server for dualstack clusters

[aroradaman]

What type of PR is this?

/kind cleanup
/sig network

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kube-proxy now reports its health more accurately in dual-stack clusters when there are problems with only one IP family.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

[aroradaman]

this will partly fix #116486

I have created this draft to get initial response/feedback on the approach for the fix the issue
/cc @danwinship @aojea @uablrek

[danwinship]

Overall I think this is probably right.

This is going to conflict with #116470. I'm not sure if @alexanderConstantinescu is working on that PR again currently?

Regardless of which is going to merge first, it would be good for you to look at that PR and make sure you're not changing things in ways that will need to be reverted or rewritten completely differently for that PR.

[alexanderConstantinescu]

I'm not sure if @alexanderConstantinescu is working on that PR again currently?

I am. I've pinged people off-band for a review on it. In case you have some cycles @danwinship: feel free to have a look.

[aroradaman]

@alexanderConstantinescu if you are just waiting for final review/approval on #116470 I'll wait for it to merge first.

[aroradaman]

/retest

[danwinship]

Yeah, I like this better, overall.

[danwinship]

This logic is wrong now, because we don't initialize oldestPendingQueuedMap to { v1.IPv4Protocol: zeroTime, v1.IPv6Protocol: zeroTime }. We initialize it to {}. So you want:

// Set oldestPendingQueuedMap[ipFamily] only if it's currently unset
if _, set := hs.oldestPendingQueuedMap[ipFamily]; !set {
        hs.oldestPendingQueuedMap[ipFamily] = hs.clock.Now()
}

[aroradaman]

if _, set := hs.oldestPendingQueuedMap[ipFamily]; !set {

This feels wrong to me, this will only allow the first QueuedUpdate() call to set hs.oldestPendingQueuedMap[ipFamily] to current time, all the subsequent calls won't be updating hs.oldestPendingQueuedMap[ipFamily] as the key will already be part of the map and proxy will always respond unhealthy.

I may be wrong here, but in the current code we do not initialize the oldestPendingQueued and being of type atomic.Value its zero value won't be zeroTime and hs.oldestPendingQueued.CompareAndSwap(zeroTime, hs.clock.Now()) won't work as old value is not zeroTime

kubernetes/pkg/proxy/healthcheck/proxier_health.go

Line 113 in 8e8ac86

hs.oldestPendingQueued.CompareAndSwap(zeroTime, hs.clock.Now())

I tried to play around with it here https://go.dev/play/p/q2onw6P3aZB, CompareAndSwap(zeroTime, currentTime) only works if we explicitly set oldestPendingQueued to zeroTime which I guess happens in the first Updated() call.

kubernetes/pkg/proxy/healthcheck/proxier_health.go

Line 104 in 8e8ac86

hs.oldestPendingQueued.Store(zeroTime)

[danwinship]

this will only allow the first QueuedUpdate() call to set hs.oldestPendingQueuedMap[ipFa

oh, right, Updated needs to be updated too.

Anyway, the stuff with comparing against zeroTime is just because, with the old code, oldestPendingQueued stored a time.Time, not a *time.Time, so we couldn't store nil to mean "nothing is queued", so we recognized the zero time as meaning that instead. Which is kind of weird... we really should have used a pointer.

Anyway, with the map, there's no reason to worry about comparing against the zero time. You should just have a value in the map if there is an update queued, and no value in the map when there is no update queued. So Updated should do delete(hs.oldestPendingQueuedMap, ipFamily).

[aroradaman]

got it, thanks !

[danwinship]

So again, this case would never get hit. The initial state is that there's no entry for that IP family, not a zero entry. But we don't need any special case for this here now; when the proxy is starting up (ie, oldestPendingQueuedMap is empty), the for loop just gets skipped and we return true below.

[aroradaman]

/retest

[danwinship]

/lgtm
/approve

/kind bug

@aroradaman I feel like this is worthy of a small release note. Something like "kube-proxy now reports its health more accurately in dual-stack clusters when there are problems with only one IP family." or something like that. Can you update the release note field in the initial comment, and then /hold cancel ?

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 375e63aee9ce61600c8c9a0db2bb54d9549a80a2

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aroradaman, danwinship

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• cmd/kube-proxy/OWNERS [danwinship]
• pkg/proxy/OWNERS [danwinship]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[aroradaman]

/hold cancel
I added the release-note.