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

Fix race condition in gcp auth provider plugin #36680

Merged
merged 1 commit into from
Nov 14, 2016
Merged

Fix race condition in gcp auth provider plugin #36680

merged 1 commit into from
Nov 14, 2016

Conversation

j3ffml
Copy link
Contributor

@j3ffml j3ffml commented Nov 11, 2016 •
edited by k8s-oncall
Loading

What this PR does / why we need it:

Fixes race condition in gcp auth provider plugin.

Which issue this PR fixes fixes #30175

Special notes for your reviewer:

Release note:

NONE

This change is Reviewable

@j3ffml j3ffml added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 11, 2016
@j3ffml j3ffml added this to the v1.5 milestone Nov 11, 2016
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 12, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit d0e38fad134177dfc2160258c82ee6e58f52e831. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 12, 2016
@saad-ali
Copy link
Member

Bug fix ok for post-code freeze merge.

@cjcullen
Copy link
Member

LGTM. I'll let @yujuhong take a look and apply the label.

@dims
Copy link
Member

dims commented Nov 12, 2016

No tests? :)

@j3ffml
Copy link
Contributor Author

j3ffml commented Nov 12, 2016

@dims, the added unit test covers regression; it repros the concurrent map access failure without gcp.go changes.

@dims
Copy link
Member

dims commented Nov 12, 2016

apologies @jlowdermilk looks like i did not scroll down enough!

@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 3e51ca2 into kubernetes:master Nov 14, 2016
@yujuhong yujuhong mentioned this pull request Nov 15, 2016
Closed
@hongchaodeng hongchaodeng mentioned this pull request Jan 14, 2017
Closed
ash2k
ash2k reviewed Jan 14, 2017
View reviewed changes
plugin/pkg/client/auth/gcp/gcp_test.go

func (f *fakePersister) read() map[string]string {
ret := map[string]string{}
f.lk.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

No Unlock()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. It's only called once at the end of the test, so I didn't notice.

@j3ffml j3ffml deleted the fix-gcp-plugin branch March 12, 2017 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ash2k ash2k ash2k left review comments

Assignees

@cjcullen cjcullen

@yujuhong yujuhong

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

kubernetes-soak-continuous-e2e-gke: broken test run
9 participants
@j3ffml @k8s-ci-robot @saad-ali @cjcullen @dims @k8s-github-robot @ash2k @googlebot @yujuhong