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

Replacing a lock file with temp file and rename #28072

Closed
wants to merge 1 commit into from
Closed

Replacing a lock file with temp file and rename #28072

wants to merge 1 commit into from

Conversation

krousey
Copy link
Contributor

@krousey krousey commented Jun 25, 2016

See #23964 and the original PR #28034

cc @alex-mohr @lavalamp

@krousey krousey added this to the v1.3 milestone Jun 25, 2016
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jun 25, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 25, 2016

GCE e2e build/test passed for commit 45e2d84.

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

Removing LGTM because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-action-required", "release-note-experimental" or "release-note-none".
Please see: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/pull-requests.md#release-notes.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2016
dims
dims reviewed Jun 27, 2016
View reviewed changes
pkg/client/unversioned/clientcmd/loader.go

func lockName(filename string) string {
return filename + ".lock"
return os.Rename(tempName, filename)
Copy link
Member

Choose a reason for hiding this comment

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

does the defer for temp.Close() work ok with os.Rename(...,...) on all platforms?

@j3ffml
Copy link
Contributor

j3ffml commented Jun 27, 2016

Discussion in #28034 (comment) suggests we are dropping this approach and keeping the lock file. @erictune thoughts?

@erictune erictune mentioned this pull request Jun 28, 2016
Merged
@erictune
Copy link
Member

Apparently, rename is not atomic on OS X and Windows and with NFS homedir (http://stackoverflow.com/questions/5943853/atomic-rename-works-on-linux-is-there-a-way-to-emulate-it-on-windows-osx)

Also, if a user runs kubectl config set-context at the same time as a refresh, the update is silently lost.

So, bad suggestion on my part. Sorry for making you extra work. Suggest we keep it as it is.

@j3ffml j3ffml closed this Jun 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@j3ffml j3ffml

@cjcullen cjcullen

@erictune erictune

Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
8 participants
@krousey @k8s-bot @k8s-github-robot @j3ffml @erictune @dims @googlebot @cjcullen