Adding lock files for kubeconfig updating #28034

krousey · 2016-06-24T18:12:01Z

This is to prevent concurrent executions from corrupting the kubeconfig file.

Also, release note?
#23964

lavalamp · 2016-06-24T18:15:40Z

pkg/client/unversioned/clientcmd/loader.go

@@ -371,6 +371,12 @@ func Load(data []byte) (*clientcmdapi.Config, error) {
 // WriteToFile serializes the config to yaml and writes it out to a file.  If not present, it creates the file with the mode 0600.  If it is present
 // it stomps the contents
 func WriteToFile(config clientcmdapi.Config, filename string) error {


Is this the only place this file is written?

I believe so.

@cjcullen Told me so.

j3ffml · 2016-06-24T18:20:39Z

Looks reasonable to me. Definitely mention in relase note.

j3ffml · 2016-06-24T18:22:47Z

cc @deads2k

lavalamp · 2016-06-24T18:29:26Z

LGTM

k8s-bot · 2016-06-24T18:43:46Z

GCE e2e build/test passed for commit 2a72e4f7afdd2abaf2a7b3957c2cef7d2da08d9d.

dims · 2016-06-24T18:46:30Z

pkg/client/unversioned/clientcmd/loader.go

+func lockFile(filename string) error {
+	// TODO: find a way to do this with actual file locks. Will
+	// probably need seperate solution for windows and linux.
+	f, err := os.OpenFile(lockName(filename), os.O_CREATE|os.O_EXCL, 0)


Do we want to use this? https://github.com/docker/docker/blob/a1e319e847f41e648ebede7f9d79999d75bd14c8/vendor/src/github.com/coreos/etcd/pkg/fileutil/lock_unix.go

https://github.com/docker/docker/blob/a1e319e847f41e648ebede7f9d79999d75bd14c8/vendor/src/github.com/coreos/etcd/pkg/fileutil/lock_windows.go#L41

Only if we don't care about windows.

Ah thanks. Looks like other folks are using windows API directly - https://github.com/pinterest/knox/blob/master/client/flock_windows.go

@krousey Thanks for the explanation

@dims I would definitely consider that for a later release, but I didn't want to try to rush something that hairy.

@krousey yes of course.

lavalamp · 2016-06-24T20:12:07Z

Unit test failure might be for real.

krousey · 2016-06-24T20:13:43Z

@lavalamp It kind of was. test-cmd.sh was running with kubeconfig in a non-existent tmpdir. I moved the file locking code to below the code that takes care of making sure the directory tree exists. Please take a look.

k8s-bot · 2016-06-24T20:45:23Z

GCE e2e build/test passed for commit 88e2a31.

lavalamp · 2016-06-24T20:53:51Z

lgtm again

k8s-github-robot · 2016-06-24T21:37:50Z

Automatic merge from submit-queue

erictune · 2016-06-24T23:13:38Z

This requires all editors to know about this lock file, and to remember to remove the lock file, and if something crashes for the user to know how to delete the lock file.

Another solution would be for anything that writes a kubectl to write it atomically, by writing the entire thing to a temp file, and then renaming the temp file over the original. That way it either suceeds or fails regardless of what other writers do.

At any rate, I don't understand why this is a P0, so I am removing the priority and the cherrypick-candidate labels.

lavalamp · 2016-06-25T00:06:36Z

@erictune This is breaking the gke-serial test, and any GKE user who runs multiple instances of kubectl.

I agree the write & rename approach is nicer but do we need to wait for that? gke-serial won't be green without some sort of fix.

girishkalele · 2016-06-25T00:16:11Z

The latest e2e-gke run failed with a large stream of errors like this. Could it be the corrupted gke cert issue ?

Error: Get https://104.197.35.156/api/v1/namespaces/e2e-tests-volume-provisioning-b5o4k/pods/pvc-volume-tester-fvqh3: x509: certificate signed by unknown authority

Full logs:
http://kubekins.dls.corp.google.com/view/Critical%20Builds/job/kubernetes-e2e-gke/9850/consoleFull

alex-mohr · 2016-06-25T00:38:12Z

re-flagging with priority bits and cherrypick-candidate so @erictune revisits per @lavalamp 's update to #23964

alex-mohr · 2016-06-25T00:40:15Z

but fwiw, agree with his concerns re: what if kubectl crashes while lock file exists -- doesn't seem discoverable by users which lockfile to remove to clean it up. still, I think we're better off with this vs. without, though perhaps we can get something better in for 1.3.0 or 1.3.1?

erictune · 2016-06-25T14:23:13Z

Multiple concurrent kubectl set in the same directory does not seem common at all.

krousey · 2016-06-25T15:25:01Z

@erictune @cjcullen says this was triggered by automatic IAM token refreshes, not explicit kubectl set calls. Hence why it was only showing up on GKE e2e tests.

krousey · 2016-06-25T15:38:12Z

@erictune Agreed that relinking would be better. It was an option, but we wanted to lock the config file itself. When there wasn't an easy cross-platform way to do that, we settled on a lock file. We should have gone back to re-linking.

If I can make that change, would it be cherry-picked?

lavalamp · 2016-06-25T20:09:05Z

@erictune, as mentioned, it's not explicit writes, it's token refreshes,
which happen every 5 minutes or so.

On Sat, Jun 25, 2016 at 8:38 AM, krousey notifications@github.com wrote:

@erictune https://github.com/erictune Agreed that relinking would be
better. It was an option, but we wanted to lock the config file itself.
When there wasn't an easy cross-platform way to do that, we settled on a
lock file. We should have gone back to re-linking.

If I can make that change, would it be cherry-picked?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28034 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAngln8kZ0gwmNuwHeNsU7VK_-NbYLiuks5qPUttgaJpZM4I9_5v
.

erictune · 2016-06-27T15:57:31Z

Hmm. So, if a user does a kubectl set at the same time as a token refresh is happening, then they will lose their edits with the re-linking approach? Maybe re-linking is not the best solution.

erictune · 2016-06-28T00:28:58Z

Following up in #28072

Automatic merge from submit-queue Adding lock files for kubeconfig updating This is to prevent concurrent executions from corrupting the kubeconfig file. Also, release note? kubernetes#23964 (cherry picked from commit cd422ad)

Automatic merge from submit-queue Batch update for 1.3 #28030: Revert "Federation e2e supports aws" #28026: Address outstanding review comments in #27999. #28034: Adding lock files for kubeconfig updating #28004: return nil from NewClientConfig instead of empty struct #28032: Increase pod CPU/memory for fluentd, dns and kube-proxy. #27208: Bump minimum API version for docker to 1.21 #28061: Remove extra double quotes in --federations. #28060: rkt: Fix the 'privileged' check when stage1 annotation is provided. #27996: Image GC logic should compensate for reserved blocks #28044: rkt: Bump required rkt version to 1.9.1. #28040: Tracked addition of federation, sed support in kube DNS #28043: Set grace period to 0 when deleting namespaces after the test. #28002: Fix startup type error in initializeCaches #28087: Hotfix: Fixup the hyperkube dns manifest from a breaking federation PR #28108: Fix initialization of volume controller caches. #28056: Increase kube-dns requirements on CoreOS. #28147: Fix error checks after cloning. #28159: Use : as seccomp security option operator for Docker 1.10 #28165: Refactored, expanded and fixed federated-services e2e tests. #28095: Kubelet should mark VolumeInUse before checking if it is Attached #28172: Build: Add KUBE_GCS_RELEASE_BUCKET_MIRROR option to push-ci-build.sh #28207: Bump cluster autoscaler to 0.2.2 #28160: Volume manager must verify containers terminated before deleting for ungracefully terminated pods #28211: Fix federation e2e tests by correctly managing cluster clients #27944: Fix pvc label selector validation error #28186: federation: Upgrading the groupversion to v1beta1

k8s-cherrypick-bot · 2016-06-29T23:25:39Z

Commit found in the "release-1.3" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

A PR merged in June 24 [1] introduced the use of lock files when updating the cluster configuration. We need to make the KUBECONFIG dir writable for development, otherwise commands such as "oc project <name>" fail, for they cannot write the lock file and update the config. [1] kubernetes/kubernetes#28034

krousey added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. team/ux labels Jun 24, 2016

krousey assigned lavalamp and j3ffml Jun 24, 2016

googlebot added the cla: yes label Jun 24, 2016

lavalamp added this to the v1.3 milestone Jun 24, 2016

lavalamp reviewed Jun 24, 2016
View reviewed changes

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jun 24, 2016

lavalamp added release-note Denotes a PR that will be considered when it comes time to generate release notes. retest-not-required and removed release-note-label-needed labels Jun 24, 2016

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2016

dims reviewed Jun 24, 2016
View reviewed changes

lavalamp added the cherrypick-candidate label Jun 24, 2016

Adding lock files for kubeconfig updating

88e2a31

krousey force-pushed the kubeconfig_filelock branch from 2a72e4f to 88e2a31 Compare June 24, 2016 20:12

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2016

cjcullen mentioned this pull request Jun 24, 2016

e2e flake: "x509: certificate signed by unknown authority" #27612

Closed

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2016

alex-mohr mentioned this pull request Jun 24, 2016

kubectl ModifyConfig doesn't prevent concurrent access #23964

Closed

k8s-github-robot merged commit cd422ad into kubernetes:master Jun 24, 2016

erictune removed cherrypick-candidate priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Jun 24, 2016

alex-mohr added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. cherrypick-candidate labels Jun 25, 2016

krousey mentioned this pull request Jun 25, 2016

Replacing a lock file with temp file and rename #28072

Closed

erictune added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate and removed cherrypick-candidate labels Jun 27, 2016

eparis mentioned this pull request Jun 27, 2016

Batch update for 1.3 #28119

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Jun 29, 2016

cjcullen mentioned this pull request Jun 30, 2016

Rework kubeconfig reading/writing to be less complex/racy #28260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding lock files for kubeconfig updating #28034

Adding lock files for kubeconfig updating #28034

krousey commented Jun 24, 2016

lavalamp Jun 24, 2016

j3ffml Jun 24, 2016

krousey Jun 24, 2016

j3ffml commented Jun 24, 2016

j3ffml commented Jun 24, 2016

lavalamp commented Jun 24, 2016

k8s-bot commented Jun 24, 2016

dims Jun 24, 2016

krousey Jun 24, 2016

dims Jun 24, 2016

krousey Jun 24, 2016

dims Jun 24, 2016

lavalamp commented Jun 24, 2016

krousey commented Jun 24, 2016

k8s-bot commented Jun 24, 2016

lavalamp commented Jun 24, 2016

k8s-github-robot commented Jun 24, 2016

erictune commented Jun 24, 2016

lavalamp commented Jun 25, 2016

girishkalele commented Jun 25, 2016

alex-mohr commented Jun 25, 2016

alex-mohr commented Jun 25, 2016

erictune commented Jun 25, 2016

krousey commented Jun 25, 2016

krousey commented Jun 25, 2016

lavalamp commented Jun 25, 2016

erictune commented Jun 27, 2016

erictune commented Jun 28, 2016

k8s-cherrypick-bot commented Jun 29, 2016

Adding lock files for kubeconfig updating #28034

Adding lock files for kubeconfig updating #28034

Conversation

krousey commented Jun 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j3ffml commented Jun 24, 2016

j3ffml commented Jun 24, 2016

lavalamp commented Jun 24, 2016

k8s-bot commented Jun 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jun 24, 2016

krousey commented Jun 24, 2016

k8s-bot commented Jun 24, 2016

lavalamp commented Jun 24, 2016

k8s-github-robot commented Jun 24, 2016

erictune commented Jun 24, 2016

lavalamp commented Jun 25, 2016

girishkalele commented Jun 25, 2016

alex-mohr commented Jun 25, 2016

alex-mohr commented Jun 25, 2016

erictune commented Jun 25, 2016

krousey commented Jun 25, 2016

krousey commented Jun 25, 2016

lavalamp commented Jun 25, 2016

erictune commented Jun 27, 2016

erictune commented Jun 28, 2016

k8s-cherrypick-bot commented Jun 29, 2016