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

Fix path handling in sqlite and kv #2756

Closed
wants to merge 2 commits into from
Closed

Fix path handling in sqlite and kv #2756

wants to merge 2 commits into from

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Aug 26, 2024

The follow changes were made:

  • The "spin" key-value store handler's take an optional base_path. This path is used to determine the location for stores. If it is None, an in-memory database will always be used (regardless of if a path is set in the runtime config).
    • If the runtime-config doesn't provide a path, then an in-memory database will be used (regardless of whether a base path is set).
  • SQLite databases work differently. An optional base directory path is provided to the RuntimeConfigResolver - if it is set the default db will be created at that path - otherwise it will be in-memory. A second non-optional path is provided for the base path of SQLite databases with a path set in the runtime config. This will always be the parent directory of the database if a path is provided in the runtime-config.
    • In other words, there is no way to force all SQLite databases to be in-memory regardless of runtime config (unlike the mechanism above for key-value which can force all key-values to be in-memory, regardless of runtime-config). Should we also make this optional and provide the CLI with some configuration option?
  • Lots of tests were changed as they were simply not testing what they said they were. For example, some tests simply asserted that the temporary directory created at the beginning of test existed. Such tests would always pass as temporary directories are created immediately and not lazily.

}

impl SpinKeyValueStore {
/// Create a new SpinKeyValueStore with the given base path.
pub fn new(base_path: PathBuf) -> Self {
///
/// If the database directory is None, the database will always be in-memory.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't quite right. If the directory is None and there is an absolute path I would expect that to work fine. This might be important for SpinKube.

When the directory is None and the path is relative I'm not sure what the current behavior is but it seems like it would be least surprising for that to be a fatal error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would mean that runtimes where --state-dir="" will error when running apps that have runtime configs with key-value stores with relative paths. Do we want that to be the case? It seems like we would want to err on the side of running apps even if the exact behavior is slightly more confusing when considering all the possible combinations.

Copy link
Collaborator Author

@rylev rylev Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it so that absolute paths are still allowed when --state-dir is unset. However, this does mean a runtime can never fully force in-memory databases. 3d4fd06?w=0

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err on the side of running apps even if the exact behavior is slightly more confusing

I'm fine with that for spin up but probably not for other Spin runtimes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll error for now - probably easier in the future to make it less restrictive than it is to make it more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a runtime can never fully force in-memory databases

If I'm reading main correctly it looks like you can omit path to force an in-memory database.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that's in the runtime config not in the actual running of the runtime engine. Are we assuming that whoever is controlling the runtime is the same as whoever is setting the runtime config? We're requiring coordination between whoever is setting the runtime config and whoever is actually running the runtime binary.

@rylev rylev mentioned this pull request Aug 26, 2024
@rylev
Copy link
Collaborator Author

rylev commented Aug 26, 2024

Closing in favor of #2757

@rylev rylev closed this Aug 26, 2024
@rylev rylev deleted the fix-paths branch August 26, 2024 14:44
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.

2 participants