Change healthchecksl4 sync mechanism

[panslava]

Removed shared global struct, instead only use shared mutex
for fake, return healthchecks instance with independent mutex

I also want to make our package pass go test -race, and current fake approach makes it fail

[panslava]

/assign @kl52752

[kl52752]

So right now all test for controllers are creating the health checks with shared mutex because it is created in NewHandler function. Is it desired?

[panslava]

Before this PR, NewHandler creates L4 with "synced" healthchecks, and then in all the tests, after NewHandler(), we override them with "Fake" health checks, which are supposed to be not synced. This approach is still questionable, but to fix it, it needs bigger refactoring

but still, if you look at the current Fake code

func Fake(cloud *gce.Cloud, recorderFactory events.RecorderProducer) *l4HealthChecks {
	instance = &l4HealthChecks{
		cloud:           cloud,
		recorderFactory: recorderFactory,
		hcProvider:      healthchecksprovider.NewHealthChecks(cloud, meta.VersionGA),
	}
	return instance
}

mutex sharedResourcesLock sync.Mutex created automatically, cause it is member of struct by "value", so it is suppose to be "unsynced" and it's own for each of result of Fake()

But, now, if multiple tests running in parallel, it can be a race condition in Fake(), if 2 goroutines execute at first

instance = &l4HealthChecks{
		cloud:           cloud,
		recorderFactory: recorderFactory,
		hcProvider:      healthchecksprovider.NewHealthChecks(cloud, meta.VersionGA),
	}

then return instance will return the same instance for both of them

[kl52752]

but in controller test you don't use Fake for health check you just depend on the controller to create it's own l4 hander and it is using default function and here.
ANd in test code you create for every controller different fakeGCE
And for me there is no race condition unless test use the same fakeGCE (It probably is within one test not across the tests which run in parallel).
ANd for me it looks like all test are locked with the same mutex and use different fakeGCE.
Does it make sense to you?

[panslava]

You are right, just seems that l4controller_test and l4netlbcontroller_test don't use Parallel (cause they check "global" metrics") so it will not help if the mutex of health checks will be not shared

[panslava]

/assign cezarygerard

[cezarygerard]

see my 2 small comments
but overall LGTM
mu better than my singleton attempt ;-)

[cezarygerard]

why not call this just Fake?

[panslava]

Okay, I renamed to Fake, difference is not that big,
I just thought that it is so similar to just normal l4HealthChecks, that it is not that clear, what is "fake" here

[cezarygerard]

So this variable should only exist once? I like this

[panslava]

yep, it is initialised in the very beginning of execution, and then shared between every healthchecksl4

[cezarygerard]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, panslava

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:

• OWNERS [cezarygerard,panslava]

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