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.

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

fix staticcheck failures: apiserver/pkg/{storage,util} #95683

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 0 additions & 7 deletions hack/.staticcheck_failures
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,6 @@ vendor/k8s.io/apiserver/pkg/server/options
vendor/k8s.io/apiserver/pkg/server/options/encryptionconfig
vendor/k8s.io/apiserver/pkg/server/routes
vendor/k8s.io/apiserver/pkg/server/storage
vendor/k8s.io/apiserver/pkg/storage
vendor/k8s.io/apiserver/pkg/storage/cacher
vendor/k8s.io/apiserver/pkg/storage/etcd3
vendor/k8s.io/apiserver/pkg/storage/tests
vendor/k8s.io/apiserver/pkg/storage/value/encrypt/envelope
vendor/k8s.io/apiserver/pkg/util/webhook
vendor/k8s.io/apiserver/pkg/util/wsstream
vendor/k8s.io/cli-runtime/pkg/printers
vendor/k8s.io/client-go/discovery
vendor/k8s.io/client-go/rest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,7 @@ func TestCacherNoLeakWithMultipleWatchers(t *testing.T) {
stopCh := make(chan struct{})
time.AfterFunc(3*time.Second, func() { close(stopCh) })

errorCh := make(chan error, 1)
wg := &sync.WaitGroup{}

wg.Add(1)
Expand All @@ -663,7 +664,8 @@ func TestCacherNoLeakWithMultipleWatchers(t *testing.T) {
ctx, _ := context.WithTimeout(context.Background(), 3*time.Second)
w, err := cacher.Watch(ctx, "pods/ns", storage.ListOptions{ResourceVersion: "0", Predicate: pred})
if err != nil {
t.Fatalf("Failed to create watch: %v", err)
errorCh <- fmt.Errorf("Failed to create watch: %v", err)
return
}
w.Stop()
}
Expand All @@ -682,9 +684,15 @@ func TestCacherNoLeakWithMultipleWatchers(t *testing.T) {
}
}
}()
go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I do not follow why we need a new go routine here?

Copy link
Contributor Author

@obeyda obeyda Nov 3, 2020

Choose a reason for hiding this comment

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

this is so we don't block and allow receive on errorCh on line 693 if err = <- errorCh; err != nil {...}

// wait for adding/removing watchers to end
wg.Wait()
close(errorCh)
}()

// wait for adding/removing watchers to end
wg.Wait()
if err = <-errorCh; err != nil {
t.Fatal(err)
}

// wait out the expiration period and pop expired watchers
time.Sleep(2 * time.Second)
Expand Down Expand Up @@ -720,6 +728,7 @@ func testCacherSendBookmarkEvents(t *testing.T, allowWatchBookmarks, expectedBoo
t.Fatalf("Failed to create watch: %v", err)
}

errorCh := make(chan error, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

defer close this channel?

Copy link
Contributor Author

@obeyda obeyda Nov 4, 2020

Choose a reason for hiding this comment

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

I didn't think we'll need to close this since it's only used in a select statement, but if you think it's necessary I'll update that.

resourceVersion := uint64(1000)
go func() {
deadline := time.Now().Add(time.Second)
Expand All @@ -731,7 +740,7 @@ func testCacherSendBookmarkEvents(t *testing.T, allowWatchBookmarks, expectedBoo
ResourceVersion: fmt.Sprintf("%v", resourceVersion+uint64(i)),
}})
if err != nil {
t.Fatalf("failed to add a pod: %v", err)
errorCh <- fmt.Errorf("failed to add a pod: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Upon getting a non-nil error, we should return here? (as apposed to keep trying in the loop)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, will add it.

Suggested change
errorCh <- fmt.Errorf("failed to add a pod: %v", err)
errorCh <- fmt.Errorf("failed to add a pod: %v", err)
return

}
time.Sleep(100 * time.Millisecond)
}
Expand Down Expand Up @@ -765,6 +774,10 @@ func testCacherSendBookmarkEvents(t *testing.T, allowWatchBookmarks, expectedBoo
t.Fatal("Unexpected timeout to receive a bookmark event")
}
return
case err = <-errorCh: // Error from goroutine
if err != nil {
t.Fatal(err)
}
}
}
}
Expand Down Expand Up @@ -924,7 +937,6 @@ func TestDispatchingBookmarkEventsWithConcurrentStop(t *testing.T) {

select {
case <-done:
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because this is an ineffective break,
error from static check:

ineffective break statement. Did you mean to break out of the outer loop? (SA4011)

case <-time.After(time.Second):
t.Fatal("receive result timeout")
}
Expand Down Expand Up @@ -974,28 +986,45 @@ func TestBookmarksOnResourceVersionUpdates(t *testing.T) {
expectedRV := 2000

wg := sync.WaitGroup{}
done := make(chan struct{})
errorCh := make(chan error, 1)
wg.Add(1)
go func() {
defer wg.Done()
for {
event, ok := <-w.ResultChan()
if !ok {
t.Fatalf("Unexpected closed channel")
}
rv, err := cacher.versioner.ObjectResourceVersion(event.Object)
if err != nil {
t.Errorf("failed to parse resource version from %#v: %v", event.Object, err)
}
if event.Type == watch.Bookmark && rv == uint64(expectedRV) {
select {
case <-done:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I do not follow why we need the done channel to exit the function? Aren't the existing return statements in this function enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad 😄, this is actually not needed here (I had in mind this is multiple goroutines, but it's only one)

return
default:
event, ok := <-w.ResultChan()
if !ok {
errorCh <- fmt.Errorf("Unexpected closed channel")
return
}
rv, err := cacher.versioner.ObjectResourceVersion(event.Object)
if err != nil {
errorCh <- fmt.Errorf("failed to parse resource version from %#v: %v", event.Object, err)
return
}
if event.Type == watch.Bookmark && rv == uint64(expectedRV) {
return
}
}
}
}()

// Simulate progress notify event.
cacher.watchCache.UpdateResourceVersion(strconv.Itoa(expectedRV))

wg.Wait()
go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, not sure why a new go routine is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, to avoid blocking and allow receive on if err = <- errorCh; err != nil {...}

wg.Wait()
close(errorCh)
}()

if err = <-errorCh; err != nil {
close(done)
t.Fatal(err)
}
}

type fakeTimeBudget struct{}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ func TestCachingObjectRaces(t *testing.T) {

numWorkers := 1000
wg := &sync.WaitGroup{}
errorCh := make(chan error, numWorkers)

wg.Add(numWorkers)

for i := 0; i < numWorkers; i++ {
Expand All @@ -141,20 +143,34 @@ func TestCachingObjectRaces(t *testing.T) {
for _, encoder := range encoders {
buffer.Reset()
if err := object.CacheEncode(encoder.identifier, encoder.encode, buffer); err != nil {
t.Errorf("unexpected error: %v", err)
errorCh <- fmt.Errorf("unexpected error: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the staticcheck complaining these "Errorf" statements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the staticcheck is complaining about the Fatalf that exists in this goroutine, but since I fixed that, I also updated the Errorf statements to use the same approach.

return
}
if callsNumber := atomic.LoadInt32(&encoder.callsNumber); callsNumber != 1 {
t.Errorf("unexpected number of serializations: %d", callsNumber)
errorCh <- fmt.Errorf("unexpected number of serializations: %d", callsNumber)
return
}
}
accessor, err := meta.Accessor(object.GetObject())
if err != nil {
t.Fatalf("failed to get accessor: %v", err)
errorCh <- fmt.Errorf("failed to get accessor: %v", err)
return
}
if selfLink := accessor.GetSelfLink(); selfLink != "selfLink" {
t.Errorf("unexpected selfLink: %s", selfLink)
errorCh <- fmt.Errorf("unexpected selfLink: %s", selfLink)
return
}
}()
}
wg.Wait()

go func() {
wg.Wait()
close(errorCh)
}()

for err := range errorCh {
if err != nil {
t.Fatal(err)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,9 @@ func TestWaitUntilFreshAndList(t *testing.T) {
{IndexName: "l:not-exist-label", Value: "whatever"},
}
list, resourceVersion, err = store.WaitUntilFreshAndList(5, matchValues, nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if resourceVersion != 5 {
t.Errorf("unexpected resourceVersion: %v, expected: 5", resourceVersion)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1911,7 +1911,6 @@ func Test_decodeContinue(t *testing.T) {

func Test_growSlice(t *testing.T) {
type args struct {
t reflect.Type
initialCapacity int
v reflect.Value
maxCapacity int
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,10 @@ type Ignored struct {
ID string
}

type IgnoredList struct {
Items []Ignored
}

func (obj *Ignored) GetObjectKind() schema.ObjectKind { return schema.EmptyObjectKind }
func (obj *IgnoredList) GetObjectKind() schema.ObjectKind { return schema.EmptyObjectKind }
func (obj *Ignored) GetObjectKind() schema.ObjectKind { return schema.EmptyObjectKind }
func (obj *Ignored) DeepCopyObject() runtime.Object {
panic("Ignored does not support DeepCopy")
}
func (obj *IgnoredList) DeepCopyObject() runtime.Object {
panic("IgnoredList does not support DeepCopy")
}

func TestSelectionPredicate(t *testing.T) {
table := map[string]struct {
Expand Down
78 changes: 52 additions & 26 deletions staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -942,10 +942,10 @@ func TestWatchBookmarksWithCorrectResourceVersion(t *testing.T) {
defer watcher.Stop()

done := make(chan struct{})
errorCh := make(chan error)
var wg sync.WaitGroup
wg.Add(1)
defer wg.Wait() // We must wait for the waitgroup to exit before we terminate the cache or the server in prior defers
defer close(done) // call close first, so the goroutine knows to exit
wg.Add(2)

go func() {
defer wg.Done()
for i := 0; i < 100; i++ {
Expand All @@ -956,36 +956,62 @@ func TestWatchBookmarksWithCorrectResourceVersion(t *testing.T) {
pod := fmt.Sprintf("foo-%d", i)
err := createPod(etcdStorage, makeTestPod(pod))
if err != nil {
t.Fatalf("failed to create pod %v: %v", pod, err)
errorCh <- fmt.Errorf("failed to create pod %v: %v", pod, err)
return
}
time.Sleep(time.Second / 100)
}
}
}()

bookmarkReceived := false
lastObservedResourceVersion := uint64(0)
for event := range watcher.ResultChan() {
rv, err := v.ObjectResourceVersion(event.Object)
if err != nil {
t.Fatalf("failed to parse resourceVersion from %#v", event)
}
if event.Type == watch.Bookmark {
bookmarkReceived = true
// bookmark event has a RV greater than or equal to the before one
if rv < lastObservedResourceVersion {
t.Fatalf("Unexpected bookmark resourceVersion %v less than observed %v)", rv, lastObservedResourceVersion)
}
} else {
// non-bookmark event has a RV greater than anything before
if rv <= lastObservedResourceVersion {
t.Fatalf("Unexpected event resourceVersion %v less than or equal to bookmark %v)", rv, lastObservedResourceVersion)
go func() {
defer wg.Done()
bookmarkReceived := false
lastObservedResourceVersion := uint64(0)
for event := range watcher.ResultChan() {
select {
case <-done:
return
default:
rv, err := v.ObjectResourceVersion(event.Object)
if err != nil {
errorCh <- fmt.Errorf("failed to parse resourceVersion from %#v", event)
return
}
if event.Type == watch.Bookmark {
bookmarkReceived = true
// bookmark event has a RV greater than or equal to the before one
if rv < lastObservedResourceVersion {
errorCh <- fmt.Errorf("Unexpected bookmark resourceVersion %v less than observed %v)", rv, lastObservedResourceVersion)
return
}
} else {
// non-bookmark event has a RV greater than anything before
if rv <= lastObservedResourceVersion {
errorCh <- fmt.Errorf("Unexpected event resourceVersion %v less than or equal to bookmark %v)", rv, lastObservedResourceVersion)
return
}
}
lastObservedResourceVersion = rv
}
}
lastObservedResourceVersion = rv
}
// Make sure we have received a bookmark event
if !bookmarkReceived {
t.Fatalf("Unpexected error, we did not received a bookmark event")
// Make sure we have received a bookmark event
if !bookmarkReceived {
errorCh <- fmt.Errorf("Unpexected error, we did not received a bookmark event")
return
}

}()

go func() {
wg.Wait() // We must wait for the waitgroup to exit before we terminate the cache or the server
close(errorCh)
}()

for err := range errorCh {
if err != nil {
close(done) // call close first, so the goroutine knows to exit
t.Fatal(err)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ func TestTimeouts(t *testing.T) {
var (
service Service
err error
errorCh = make(chan error, 1)
data = []byte("test data")
kubeAPIServerWG sync.WaitGroup
kmsPluginWG sync.WaitGroup
Expand All @@ -138,8 +139,10 @@ func TestTimeouts(t *testing.T) {

service, err = NewGRPCService(socketName.endpoint, tt.callTimeout)
if err != nil {
t.Fatalf("failed to create envelope service, error: %v", err)
errorCh <- fmt.Errorf("failed to create envelope service, error: %v", err)
return
}
errorCh <- nil
defer destroyService(service)
kubeAPIServerWG.Done()
// Keeping kube-apiserver up to process requests.
Expand All @@ -153,17 +156,25 @@ func TestTimeouts(t *testing.T) {

f, err := mock.NewBase64Plugin(socketName.path)
if err != nil {
t.Fatalf("failed to construct test KMS provider server, error: %v", err)
errorCh <- fmt.Errorf("failed to construct test KMS provider server, error: %v", err)
return
}
if err := f.Start(); err != nil {
t.Fatalf("Failed to start test KMS provider server, error: %v", err)
errorCh <- fmt.Errorf("Failed to start test KMS provider server, error: %v", err)
return
}
errorCh <- nil
defer f.CleanUp()
kmsPluginWG.Done()
// Keeping plugin up to process requests.
testCompletedWG.Wait()
}()

// Error from kubeAPIServerWG goroutine
if err := <-errorCh; err != nil {
t.Fatal(err)
}

kubeAPIServerWG.Wait()
_, err = service.Encrypt(data)

Expand All @@ -175,6 +186,10 @@ func TestTimeouts(t *testing.T) {
t.Fatalf("got %q, want nil", err.Error())
}

// Error from kmsPluginWG goroutine
if err := <-errorCh; err != nil {
t.Fatal(err)
}
// Collecting kms-plugin - allowing plugin to clean-up.
kmsPluginWG.Wait()
})
Expand Down Expand Up @@ -228,7 +243,7 @@ func TestIntermittentConnectionLoss(t *testing.T) {
wg1.Done()
_, err := service.Encrypt(data)
if err != nil {
t.Fatalf("failed when executing encrypt, error: %v", err)
t.Errorf("failed when executing encrypt, error: %v", err)
}
}()

Expand Down