Skip to content
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

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

javfg
Copy link
Contributor

@javfg javfg commented Nov 28, 2023

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)

Copy link

update-docs bot commented Nov 28, 2023

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.

@javfg javfg force-pushed the dynamic-storage-provider-fixes branch from 794aab2 to c97fcca Compare November 28, 2023 14:14
@javfg javfg requested a review from a team as a code owner November 28, 2023 14:18
@glpatcern
Copy link
Member

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.

@glpatcern
Copy link
Member

This closes #4334

@glpatcern glpatcern linked an issue Nov 28, 2023 that may be closed by this pull request
@labkode
Copy link
Member

labkode commented Nov 28, 2023

@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.

@glpatcern
Copy link
Member

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)?

  1. If that is ignored, I don't see why we want to fail this case
  2. If that leads to an error, the system is fully consistent. BUT now adding routes requires a downtime (stop services - add entry in db - reconfigure - restart services), which we should rather avoid

Either way, IMHO we should just allow for mismatches in both directions, with appropriate logs, but without affecting the rest of the system.

@javfg
Copy link
Contributor Author

javfg commented Nov 28, 2023

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 eoshome-i04 (this being something which should exist but is missing because of a misconfiguration), or somethingthatdoesntexist (which will never be there), you will still get the same result: a nil storage provider.

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.

@glpatcern
Copy link
Member

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?

@javfg
Copy link
Contributor Author

javfg commented Nov 28, 2023

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.

@glpatcern
Copy link
Member

glpatcern commented Nov 28, 2023

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.

@labkode labkode merged commit 7b7c3e1 into cs3org:master Nov 28, 2023
24 checks passed
@labkode
Copy link
Member

labkode commented Nov 28, 2023

@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.

@glpatcern
Copy link
Member

which is a good 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dynamic routing issue when DB contains non-resolvable elements
3 participants