-
Notifications
You must be signed in to change notification settings - Fork 303
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.
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
Change healthchecksl4 sync mechanism #1812
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,6 @@ import ( | |
"testing" | ||
"time" | ||
|
||
"k8s.io/ingress-gce/pkg/healthchecksl4" | ||
"k8s.io/ingress-gce/pkg/loadbalancers" | ||
|
||
"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" | ||
|
@@ -80,7 +79,6 @@ func newServiceController(t *testing.T, fakeGCE *gce.Cloud) *L4Controller { | |
for _, n := range nodes { | ||
ctx.NodeInformer.GetIndexer().Add(n) | ||
} | ||
healthchecksl4.Fake(ctx.Cloud, ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
mutex But, now, if multiple tests running in parallel, it can be a race condition in Fake(), if 2 goroutines execute at first
then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
return NewILBController(ctx, stopCh) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this variable should only exist once? I like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it is initialised in the very beginning of execution, and then shared between every healthchecksl4