config: copy go's file lock and use it instead of client-go's #5982
Conversation
nicksieger
force-pushed
the
file-locking-4814
branch
2 times, most recently
from
a5137c0 to
92f55e6
Compare
Fixes tilt-dev#4814. Signed-off-by: Nick Sieger <[email protected]>
Signed-off-by: Nick Sieger <[email protected]>
nicksieger
force-pushed
the
file-locking-4814
branch
from
92f55e6 to
05d6922
Compare
|
|
||
| func init() { | ||
| // We're using our own file locking mechanism | ||
| clientcmd.UseModifyConfigLock = false |
There was a problem hiding this comment.
i think my big concern with this is that it's global state.
it assumes you've updated all the call sites.
i'm not at all confident you've updated all the call sites (particularly the ones in our dependencies)
i'd honestly be more comfortable with a patched version of client-go that uses our locking mechanism. (we already patch apimachinery)
There was a problem hiding this comment.
Yeah, it's not ideal and could be easily overlooked if we started expanding the ways in which Tilt handles kubeconfigs. In this case, the only place in Tilt where we write config with client-go is the 2 places in this PR in the hud server controller package. I added read locks in places where we read the same config but client-go already didn't try to achieve read consistency, so those aren't strictly necessari. I could move all code dealing with reading and writing Tilt's kubeconfig to a separate package if you think that would help with the maintenance issues.
As it is, I consider this PR to be a temporary band-aid that should be fixed once Go has a public file locking API, at which point we or someone would send a PR to client-go to change the global. Realizing too that "temporary" fixes often become long-term ones, I feel that this change is worthwhile given the lower amount of maintenance we do on Tilt, the point above on how we only have two locations (in the same package) that do writes and the fact that write collisions on kubeconfigs in a dev environment are pretty unlikely.
|
I've seen it from time to time, never consistently. Recent report in slack made me realize that it's probably Tilt getting killed by some other process and the I/O of the HUD server de-registering itself is probably a convenient place for the process to pause, get killed and fail to delete the lock file. |
|
Still going to do some sanity check manual testing on Mac, Windows and Linux before merging this. Let me know if you have any more thoughts about the client-go global state issue. |
|
nope, no further thoughts, LGTM |
Fixes #4814.
Signed-off-by: Nick Sieger [email protected]