-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Storage registry: fail at init if config is missing any providers #4370
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
794aab2
to
c97fcca
Compare
Thanks Javi, but I'd rather implement a more tolerant behavior, that is the missing route would be discarded (with a log warning) without preventing reva from starting. The context I see here is at least for our own QA: we share the same database, but we may enable/disable some storage providers (e.g. the eosatlas one). Also, if a new route has to be added, we can easily decouple the db operation from the reconfiguration - until the storage provider appears, the new route remains hidden. |
This closes #4334 |
@glpatcern I've discussed that option with @javfg, but that will not be robust enough. The fault tolerant one is just a side effect of our internal deployment which should not be reflected in the code. This PR follows the same principles as other drivers, abort at init time if the runtime of the logic cannot be guaranteed at runtime. |
I gently disagree ;-) What is the present behavior in case a configuration entry is added to the dynamic registry in the toml file, but no database route points to it (essentially the other way around)?
Either way, IMHO we should just allow for mismatches in both directions, with appropriate logs, but without affecting the rest of the system. |
I understand flexibility is a good idea, in fact it was my first direction (as we discussed). But then we're presented with another problem: The router will not differentiate in any way an incorrect provider from a provider which is not present in the config. Whether you try to resolve This is bound to bring headaches in the future. :) Also, I think the correct behavior of a router would be to return errors when something is not routable. But then again, this is very nuanced and I understand both sides. I'll agree with whatever you decide. |
I'm fine with the router not being able to distinguish between a genuine misconfiguration and a "deliberate" one (and temporary, typically - see my example of introducing a new route in production). Anyway, what happens in the scenario above, i.e. entry in config but missing entry in db? |
Nothing, the rule just lays there and will never be used. I imagined this part similar to a DNS, following the router idea. So while it is OK to have extra records there, it's not equally fine to have a missing record when you want to resolve a storage id into an address. The downside is all entries in the table will need an entry in the config in all instances. |
Well, I understand the model but here we are deliberately saying that the DB is more important than the config, and that the system is not symmetric. To me, this is as good (or as bad!) as having it symmetric and tolerant. Clearly the option of being symmetric and not tolerant is a no-go for operational reasons, but the other two options (not symmetric vs symmetric and tolerant) are at least equally legitimate. In particular I yet fail to see any real advantage of this non-symmetric approach. Edit: I do see an advantage of the symmetric approach: when introducing new routes, we won't care about the order of the change (db vs toml), whereas with this approach, we MUST ensure the config is there prior to introducing the route in the DB. |
@glpatcern changes can always be introduced in the DB before they are in the config. Only on restart the changes need to be there, which is a good to catch for forgotten configs. |
And it's an excellent time bomb should a daemon restart be kicked inadvertently... OK, I won't complain further as you merged it, but IMHO this will hit us in the future. |
This PR changes the behavior of the dynamic storage registry so it fails on initialization if the configuration is not complete (there are storage provider entries in the database with no entry in the config file)