Fix IPVS proxier to update stale real server after restart

[aryan9600]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Update the IPVS proxier to have a bool updateWeights which is set to
true during the initial syncs performed by OnEndpointSlicesSynced and
OnServiceSynced to make sure any real servers with stale weights are
updated accordingly at startup. This logic is gated behind a bool to
avoid doing this during every sync as it's an expensive operation.

Which issue(s) this PR fixes:

Fixes #108319

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

[k8s-ci-robot]

@aryan9600: 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.

[k8s-ci-robot]

Hi @aryan9600. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[aryan9600]

/sig network

[aojea]

/assign @uablrek @andrewsykim
/ok-to-test

[uablrek]

Remove the proxier.updateWeights = false from here and instead do it where it's checked (see below)

[uablrek]

same as above

[uablrek]

Add proxier.updateWeights = false here. The mutex is held so it's safe

[uablrek]

(btw, you may make a comment that the mutex is held and that it's a one-time event)

[aryan9600]

we can't add this here, as this is inside a loop. adding this outside the loop instead.

[uablrek]

Outside the "for _, ep := range newEndpoints.List() {" loop you mean? Seens reasonable.

[aryan9600]

on second thought, is it completely safe to have proxier.updateWeights = false inside syncProxyRules() instead of OnServicesSynced()/OnEndpointSynced()?
i'm asking because in OnServicesSynced() we do something like:

proxier.mu.Lock()
...
proxier.updateWeights = true
proxier.mu.Unlock()

proxier.syncProxyRules()

so even though proxier.syncProxyRules() would capture the mutex first thing, theoretically another goroutine which called syncProxyRules() could execute proxier.updateWeights = false at the exact moment between OnServiceSynced let go of the lock and proxier.syncProxyRules() acquired the lock.

[uablrek]

Silly mistake, now it works https://go.dev/play/p/CuVZg3B2itE

[aryan9600]

yes, i noticed first needs to be initialized to true instead of false

[uablrek]

Seems go has support for "Once"; https://golangcode.com/run-code-once-with-sync/

You may see if you can use it.

[uablrek]

I am unsure if it's better since we already have the proxier.mu mutex.

[aryan9600]

I think it's better to avoid sync.Once because if in the future we want to do more things only on startup, the underlying function will get large. Having a flag gives us flexibility about where we want to run the one-time logic during sync.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aryan9600, uablrek

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:

• pkg/proxy/ipvs/OWNERS [uablrek]

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

[uablrek]

EDIT; this is a bad solution since it checks proxier.initialized directly. Please see the updated proposal below

I was a bit hasty with the approve button 😊 The code looks good but it doesn't solve the problem. It seems there is a need to do the update both for endpoints and endpointSlices. I found my old tests for the issue and as it is now the weight stays a 0.

I altered the code to;

func (proxier *Proxier) syncProxyRules() {
	proxier.mu.Lock()
	defer proxier.mu.Unlock()
	// To ensure complete initialization we can only consider the
	// initial sync done when the proxier is initialized.
	defer func() {
		if proxier.initialized == 1 {
			proxier.initialSync = false
		}
	}()

then it works.

[aryan9600]

okay, thanks for catching this. let me confirm this and update 👍

[aojea]

I was a bit hasty with the approve button The code looks good but it doesn't solve the problem. It seems there is a need to do the update both for endpoints and endpointSlices. I found my old tests for the issue and as it is now the weight stays a 0.

/hold

just to avoid we merge unintentionally , once you are good unhold it, is just a precaution

[uablrek]

Taking another look and I see that proxier.initialized is checked just a few lines down and with an atomic operation. So the defer function just have to be moved to after the check;

func (proxier *Proxier) syncProxyRules() {
	proxier.mu.Lock()
	defer proxier.mu.Unlock()

	// don't sync rules till we've received services and endpoints
	if !proxier.isInitialized() {
		klog.V(2).InfoS("Not syncing ipvs rules until Services and Endpoints have been received from master")
		return
	}

	defer func() {
		proxier.initialSync = false
	}()

[uablrek]

(the proposed fix above is tested and works)

[uablrek]

Tested and works.

/lgtm
/unhold