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

Stub Device Plugin refactor: 600x speedup #52612

Closed
wants to merge 7 commits into from
Closed

Stub Device Plugin refactor: 600x speedup #52612

wants to merge 7 commits into from

Conversation

RenaudWasTaken
Copy link
Contributor

@RenaudWasTaken RenaudWasTaken commented Sep 17, 2017 •
edited

What this PR does / why we need it:

Real size of PR is +65 −40
Real impact of the PR:

  • Current test time fixed 112s
  • Test time after this PR: 0.206
  • That's around 600x speedup

Found a few more things which were not closed correctly:

  • Depends on Refactor Device Plugin Registration #52616 for re-registration test
  • panics if the gRPC server in stub returns with an error before the end of the test
  • Closes the stream in ListAndWatch
  • Stub Device Plugin now uses glog

Which issue this PR fixes: fixes #52561

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 17, 2017
@k8s-github-robot k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 17, 2017
@RenaudWasTaken RenaudWasTaken force-pushed the fix-device-plugin-tests branch 2 times, most recently from ee0ca54 to af37755 Compare September 17, 2017 18:23
@RenaudWasTaken RenaudWasTaken mentioned this pull request Sep 17, 2017
Closed
@feiskyer
Copy link
Member

feiskyer commented Sep 18, 2017 •
edited

Replaced the mutex in manager.go by a sync.Map to fix data race in go test

sync.Map is introduced in go1.9, but kubernetes is still building with go1.8.3. I think this should wait a while until go1.8.x is not supported or use mutex instead.

@xiangpengzhao
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 18, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2017
@RenaudWasTaken
Copy link
Contributor Author

@jiayingz can you take a look ?
I've vastly improved speed and reliability of the tests in this new version.

Running the tests with -count 30 now takes 30-32 seconds instead of > 10 minutes and none of them are flaky.

@RenaudWasTaken RenaudWasTaken changed the title Fix data race and go routine leak in Device Plugin tests Stub Device Plugin refactor Oct 18, 2017
@RenaudWasTaken RenaudWasTaken force-pushed the fix-device-plugin-tests branch 3 times, most recently from 81b1347 to b8261a7 Compare October 18, 2017 23:35
@heeeello
Copy link

heeeello commented Oct 20, 2017 via email

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2017
@RenaudWasTaken RenaudWasTaken force-pushed the fix-device-plugin-tests branch 4 times, most recently from d03863c to f311ba6 Compare October 21, 2017 16:34
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2017
@feiskyer feiskyer removed their assignment Dec 20, 2017
@RenaudWasTaken RenaudWasTaken mentioned this pull request Dec 21, 2017
Merged
@RenaudWasTaken RenaudWasTaken force-pushed the fix-device-plugin-tests branch 3 times, most recently from 12f5699 to 2a0a83e Compare December 22, 2017 12:12
@RenaudWasTaken RenaudWasTaken changed the title Stub Device Plugin refactor Stub Device Plugin refactor: 600x speedup Dec 22, 2017
@k8s-github-robot
Copy link

@RenaudWasTaken PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 25, 2017
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 25, 2018
@vikaschoudhary16 vikaschoudhary16 mentioned this pull request Apr 16, 2018
Closed
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 24, 2018
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@RenaudWasTaken RenaudWasTaken deleted the fix-device-plugin-tests branch September 24, 2018 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vikaschoudhary16 vikaschoudhary16 vikaschoudhary16 left review comments

Assignees

@pmorie pmorie

@vikaschoudhary16 vikaschoudhary16

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@RenaudWasTaken @feiskyer @xiangpengzhao @heeeello @vikaschoudhary16 @k8s-ci-robot @tengqm @k8s-github-robot @fejta-bot @pmorie