Closed MonzElmasry closed 3 years ago
@MonzElmasry you might want to run gofmt
against the whole code to fix the indentation and spaces for the code
@MonzElmasry what happens if the url is technically correct but points to nothing, like the database doesnt actually exist or the network is down.
In this scenario, I was expecting k3s to actually fail to start, though I'm not even sure if that is the right behavior. @ibuildthecloud should k3s fail to start or sit in an endless retry loop waiting for the db connection to be successful? Should we introduce some sort of connection test?
@MonzElmasry while we wait for @ibuildthecloud's response, can you investigate around making a test connection? Just some research and a prototype. I think there may be some established best practices around testing connections to sql dbs, but dont know for sure.
@MonzElmasry I'd also like you to consider how we could potentially write tests for your changes. To my knowledge, kine doesnt currently have meaningful tests and I haven't put much thought into how we could introduce them. Maybe we pull the framework we have in k3s for hooking k3s up to containerized databases into this repository and start with some simple smoke tests of connecting to the db backends.
So when we first talked about this GH issue I believe that yes we were suggesting that K3s should NOT start at all on the initial connection to the DB. If that connection fails, K3s should not run and a sensible error should be shown to the user (or in logs, whatever).
UPDATE: So yes, seems like in v1.18.2, v1.17.4, v1.17.3 we gate running k3s - db connection has to work there. We also throw a sensible error. So maybe this was an issue in earlier builds? There was a GH issue someone entered a while back of someone having an issue where they would still connect to db but it would fail and it was due to proper DNS but bad dbname I think. Perhaps we just need a solution to validate dbnames? We should perhaps discuss in our next standup tomorrow.
@ibuildthecloud can we get final review and LGTM from you on this PR? cc: @cjellick
Can this PR be split into two commits. All the CI changes in one and the code changes in another. Also, don't delete Dockerfile, that file is used.
the pr basically fixed two things:
And add drone automation
https://github.com/rancher/k3s/issues/1668