Allow specifying a kubeconfig per task in the internal API #746
Conversation
Rakefile
Outdated
| end | ||
|
|
||
| desc("Run cli tests") | ||
| Rake::TestTask.new(:cli_test) do |t| | ||
| t.libs << "test" | ||
| t.libs << "lib" | ||
| t.test_files = FileList['test/exe/**/*_test.rb'] | ||
| t.warning = false |
There was a problem hiding this comment.
I was really struggling to crawl through the test output with the warnings on. Happy to move this elsewhere though if there's more to do.
There was a problem hiding this comment.
What warnings were you seeing? When I ran the tests, the only output I got was the test suite summaries, and a large log output from a failed assertion upon the log string.
There was a problem hiding this comment.
I don't remember exactly, it's possible it was just in CI.
There was a problem hiding this comment.
Alrighty. I'll open a separate draft PR (targeting this one) with the original rakefile and I'll see if I can repro.
|
This looks reasonable to me, but folks like @timothysmith0609 and @kddeisz may have stronger opinions. |
ajshepley
force-pushed
the
internal-api-kubeconfig
branch
from
3ad4457
to
d191e65
Compare
There was a problem hiding this comment.
This is really good. Only thing I think left is don't delegate directly to the ivar, instead delegate to the getter method.
Reason being is if we end up changing that thing to wrap the object in some way, it'll require further changes, whereas if it's just a method call it won't.
|
Since deploying this last week, we haven't seen any of the corruption issues crop up in core. |
|
Ah, I missed that and only thought about the task config internally. Whoops. Will fix, thanks. |
ajshepley
force-pushed
the
internal-api-kubeconfig
branch
from
bb7a1bb
to
99b28d8
Compare
ajshepley
force-pushed
the
internal-api-kubeconfig
branch
from
99b28d8
to
cb298a1
Compare
|
I accidentally overlooked some lint errors (the buildkite job spits out a looooot of stuff and the error is in the middle somewhere) but it should be good now :) |
Use method delegate rather than ivars.
ajshepley
force-pushed
the
internal-api-kubeconfig
branch
from
cb298a1
to
fa18cc1
Compare
Adds 2.0.1 section and info about #746
What are you trying to accomplish with this PR?
Being primarily a cli tool, Krane currently relies on some global state in the environment - notable for this issue is the kubeconfig path. Deploying our main monolith at Shopify is a bit of a pathological case - we use Krane as a library, and deploy to multiple targets simultaneously using the internal API and threads.
Occasionally we are seeing some errors I believe are ultimately thread safety issues - e.g. apparent corruption of the kubeconfig as contexts are 'missing' (when we check the files, they are not), or base64 certificate data is invalid and un-decodeable. I have not been able to determine the precise cause here (e.g. fd leaks) as reproducing is very difficult, but would like to at least work around the symptom we're seeing by giving each deploy task its own kubeconfig instead of sharing one everywhere. This PR alters the internal API to allow this, while not changing the behaviour for cli users, and in doing so remove a little of this global state.
How is this accomplished?
Having the various
Taskclasses takekubeconfigas a parameter, and ensuring that is passed through toKubectlandKubeclientinstances. Ifkubeconfigis nil, the value ofKUBECONFIGin the environment is used, preserving the current behaviour.There is some prior art here. An explicit
--kubeconfigoption was added in #428 and then reverted in #468 as the cli flag did not support parsing multiple files. This approach overrides theKUBECONFIGenvvar in the kubectl child process, which should avoid that issue.I have removed a bunch of memoizing from the test suite as some tests currently rely on mutating global state, notably the multiple config file integration test. It's likely we can keep some of this optimisation by reworking these tests, but it could use someone with better knowledge of the test suite than me.
The various
Taskclasses share a common set of options and setup, it might be worth refactoring these to share a base class and dedup some of the changes here, but I consider that out of scope for this PR.What could go wrong?
As I noted above a change similar to this was attempted before and abandoned. I believe this approach of modifying the subprocess env avoids the previous issue, but maybe there's some other problem. I hope to test this branch by deploying our monolith to check for other issues, and that it actually resolves this issue we're seeing.
I have done some sanity 🎩 locally with minikube and fixtures.