Better handling of (potentially) transient errors in the configuration values

A discussion sparkled in the comments to a commit in my personal fork - since that commit is going to be rebased, I copy the discussion here so we don't lose it.

lerks commented on 0d768d6 16 days ago
I always felt that configuration loading was poorly handled: thanks for taking the time to look at it and try to fix it.

In my opinion two kinds of errors are "related" to the configuration file: the file is "intrinsically" wrong (invalid JSON, missing required fields, etc.) or it doesn't match with the "reality" (the specified address/port isn't available to listen to, the database is unreachable with the given connection string, etc.). Note that the second ones could be caused by external factors: the network interface or the database could be down. The line between the two types is somewhat blurry: when a (service, shard) isn't found in the config, is the config missing it or shouldn't that service be running at all?

Your commit mostly ignores the first type (whose handling remains unchanged: a print on stderr and an exit(1)) and focuses on the second one. The name you chose for the exception (ConfigError) and the behavior you confirmed (dying) suggests that you consider these error to be caused exclusively by a wrong configuration file. I don't like that. I'd prefer the service to assume they're caused by external factors: log an error (suggesting to check the config file) but insist trying (waiting for the interface to come up/retry connecting to the database).

(It'd be also cool for services to detect when a network interface goes down and, instead of dying, wait for it to come up again to reconnect)

[minor] In the cases where assuming external causes isn't feasible (e.g. an unknown service/shard) I think it's ok to terminate the service, but I'd use a name like SetupError instead of ConfigError.

stefano-maggiolo commented on 0d768d6 16 days ago
For example in the case of the inability to connect to the database, what should the service do? Enter into a limbo state waiting for the connection to be established? Since this check happens at startup time, for me it is sensible to require the admins to look into the problem and restart the service; I don't see any real practical advantage in keep retrying.

Also, if we want to protect ourselves from transient errors, we should consider the errors coming to life in the middle of the execution of the service, and that is much harder.

Finally, the philosophy has always been "it's not a problem if a service dies", and I think this makes more appropriate for the service to die instead of entering in the limbo state I was referring to earlier.

Considered these, I think the commit is an improvement and should be submitted, but I wouldn't object if someone else wants to handle better errors coming from the config load and also transient errors. :)

lerks commented on 0d768d6 16 days ago
In that case I think the service should repeatedly fail. Database access isn't required for service initialization. Requests (RPC + HTTP) that are received when there's no connection just fail (exception raised + error logged), exactly as they do now with "transient errors". In fact, what I want is to have no difference between "transient conditions" and conditions already present at start.
Said that, I like the idea of a "check" (at start) that, when unsuccessful, logs "Hey, admin, do you know I cannot reach the DB? Go check that!", but no more than this.

I don't see a limbo here. Where I see a limbo, though, is in the respawn loop of ResourceService caused by a service that instantly dies when started. IMO, the "it's not a problem if a service dies" philosophy doesn't imply "a service can just kill itself whenever it wants": dying and relying on someone else (RS or admin) to get restarted isn't the best way to handle these (foreseeable) adverse conditions. That philosophy applies for unforeseeable conditions (kernel panic, power loss, rain floods, locusts, meteorites, ...)

stefano-maggiolo commented on 0d768d6 15 days ago
I see the point. Still, in case of the specific error we are talking about, in 100% of the cases I have experienced was caused by a misconfiguration, and not by a transient problem, hence it is more convenient to kill the service (since to load a new configuration you need to restart it anyway). Especially if it is RS already that refuses to start.

lerks commented on 0d768d6 7 days ago
I'd like to hear @giomasce's opinion on the matter.

giomasce commented on 0d768d6 7 days ago
I'd say I mostly agree with @lerks: unless it really can't parse the configuration, a service shouldn't reject it just because it can operate. If there are problems, it should try again and/or fail incoming requests (in the case of *WS, for instance).

The problem of synchronizing the configuration is important, but shoud be fixed in a different way. I've been thinking for some time about this and I have some ideas, although no code and not even a complete proposal. I would like to move as much of the configuration as possible inside the database and leave in the cms.conf file virtually only the database URL.

stefano-maggiolo commented on 0d768d6 4 days ago
I am trying to think of a better behavior, but having a hard time finding it. In my opinion, ideally CMS should never throw stack traces to the user. And wrapping all accesses to the database in try catch seems way overkill. Add to that that we have also this crazy behavior that when the configuration has a syntax error we simply use default values, those probability of being correct is exactly 0.

I agree with gio that finally most of the configuration must go in the database, but until we get to there I really cannot see why you prefer a service that logs a perfectly actionable error and then crashes to a service that just log the actionable error and terminate correctly.

lerks commented on 0d768d6 3 days ago
In the current situation this is what I'd like to have:

In case of syntax errors (e.g. invalid JSON, missing fields, etc.) the service should die immediately. I don't care that much how: either raise an exception (and, if you want to avoid tracebacks, catch it) or just sys.exit. (Of course the user should somehow be informed of the cause of death).
In case of (possible) transient errors (e.g. database unreachable, inability to listen on network, etc.) the service should log an error (that should be as clear as possible, suggesting to check the configuration file) but it should try not to die. Since we already enclose almost all database accesses in a with SessionGen(): context manager I think we could do the check there. That is: when exiting from that block we check if an exception has been raised and, in case, log the message (and suppress the exception?).
Additionally, I'm totally in favor of having a test_db_connection called when the services start, but in my mind it should just do:

with SessionGen() as session:
session.execute("select 0;")
(i.e. if it fails log an error, else do nothing)

I really cannot see why you prefer a service that logs a perfectly actionable error and then crashes to a service that just log the actionable error and terminate correctly.
The service should avoid crashing.

No file or test path is named. Start by tracing SessionGen() and the service startup path, then inspect how database and network exceptions are currently handled. The issue proposes a test_db_connection check using SELECT 0, preserving immediate failure for syntax errors while avoiding crashes on potentially transient failures.