Skip to content
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

Jump to bottom

Deflake limitrange singleflight test #113736

Merged
merged 1 commit into from Nov 8, 2022
Merged

Deflake limitrange singleflight test #113736

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 24 additions & 19 deletions plugin/pkg/admission/limitranger/admission_test.go
View file
Open in desktop
Expand Up @@ -864,12 +864,21 @@ func TestLimitRanger_GetLimitRangesFixed22422(t *testing.T) {
limitRange := validLimitRangeNoDefaults()
limitRanges := []corev1.LimitRange{limitRange}

var count int64
mockClient := &fake.Clientset{}

unhold := make(chan struct{})
var (
testCount int64
test1Count int64
)
mockClient.AddReactor("list", "limitranges", func(action core.Action) (bool, runtime.Object, error) {
atomic.AddInt64(&count, 1)
switch action.GetNamespace() {
case "test":
atomic.AddInt64(&testCount, 1)
case "test1":
atomic.AddInt64(&test1Count, 1)
default:
t.Error("unexpected namespace")
}

limitRangeList := &corev1.LimitRangeList{
ListMeta: metav1.ListMeta{
Expand All @@ -881,8 +890,8 @@ func TestLimitRanger_GetLimitRangesFixed22422(t *testing.T) {
value.Namespace = action.GetNamespace()
limitRangeList.Items = append(limitRangeList.Items, value)
}
// he always blocking before sending the signal
<-unhold
// make the handler slow so concurrent calls exercise the singleflight
time.Sleep(time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

this has to be always higher than the time it takes to create the goroutines and hit the group

Copy link
Member Author

Choose a reason for hiding this comment

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

no, if it is not higher, the ones that missed the singleflight hit the cache, and the test still passes

Copy link
Member

Choose a reason for hiding this comment

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

I think that the concurrency pattern we want to implement is the one provided by sync.Cond, so we put all goroutines go to sleep here and we wake them up later ... but this approach is easier to read and in my local tests it seems very reliable

7m40s: 7249 runs so far, 0 failures
7m45s: 7344 runs so far, 0 failures
7m50s: 7440 runs so far, 0 failures
7m55s: 7488 runs so far, 0 failures
8m0s: 7584 runs so far, 0 failures
8m5s: 7680 runs so far, 0 failures
8m10s: 7728 runs so far, 0 failures
8m15s: 7824 runs so far, 0 failures
8m20s: 7920 runs so far, 0 failures
8m25s: 7968 runs so far, 0 failures
8m30s: 8064 runs so far, 0 failures
8m35s: 8157 runs so far, 0 failures
8m40s: 8208 runs so far, 0 failures
```

/lgtm

return true, limitRangeList, nil
})

Expand Down Expand Up @@ -926,34 +935,30 @@ func TestLimitRanger_GetLimitRangesFixed22422(t *testing.T) {
}
}()
}
// unhold all the calls with the same namespace handler.GetLimitRanges(attributes) calls, that have to be aggregated
unhold <- struct{}{}
go func() {
unhold <- struct{}{}
}()

// and here we wait for all the goroutines
wg.Wait()
// since all the calls with the same namespace will be holded, they must be catched on the singleflight group,
// There are two different sets of namespace calls
// hence only 2
if count != 2 {
t.Errorf("Expected 1 limit range, got %d", count)
if testCount != 1 {
t.Errorf("Expected 1 limit range call, got %d", testCount)
}
if test1Count != 1 {
t.Errorf("Expected 1 limit range call, got %d", test1Count)
}

// invalidate the cache
handler.liveLookupCache.Remove(attributes.GetNamespace())
go func() {
// unhold it is blocking until GetLimitRanges is executed
unhold <- struct{}{}
}()
_, err = handler.GetLimitRanges(attributes)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
close(unhold)

if count != 3 {
t.Errorf("Expected 2 limit range, got %d", count)
if testCount != 2 {
t.Errorf("Expected 2 limit range call, got %d", testCount)
}
if test1Count != 1 {
t.Errorf("Expected 1 limit range call, got %d", test1Count)
}
}