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

Provide persistent configuration for Livebook #633

Closed
3 tasks done
josevalim opened this issue Oct 22, 2021 · 21 comments · Fixed by #1017
Closed
3 tasks done

Provide persistent configuration for Livebook #633

josevalim opened this issue Oct 22, 2021 · 21 comments · Fixed by #1017
Labels
area:backend Related to the backend feature New feature or request
Milestone

Comments

@josevalim
Copy link
Contributor

josevalim commented Oct 22, 2021

Right now, all Livebook configuration is temporary. I suggest we implement different Livebook configuration backends so we can start persisting some information, such as authentication mode and S3 settings. This will be particularly important with the desktop app and cloud deployments.

Tasks:

  • Implement a basic persistent storage with ETS backend
  • Persist the ETS backend periodically to disk
  • Move FileSystem configuration from Livebook.Config to Livebook.Storage and make operations based on ID
@josevalim josevalim added area:backend Related to the backend feature New feature or request labels Oct 22, 2021
@Benjamin-Philip
Copy link
Contributor

Benjamin-Philip commented Nov 11, 2021

@josevalim,

In which files would configuration be persisted?

Normally, in desktop applications, I would expect a "global" config at $XDG_HOME_CONFIG/livebook/livebook.conf, a per project file at Project Root/.livebook/livebook.conf, and finally Env variables (for temporary changes), to configure it.

These would have the following "preference" (descending):

  1. Env vars
  2. Project level confs
  3. Global conf

So, Env vars would override any Project level and Global conf, Project level confs would overide Global conf, and everything else is inherited from the Global conf.

However, I am not sure if this would work in all "Filesystems". I especially can't say anything for Amazon S3 as I have no experience using it.

I also think that only sensitive information should be encrypted, and that the rest should be stored as plain text.

@josevalim
Copy link
Contributor Author

In which files would configuration be persisted?

Using xdg is a good idea. Erlang has an API for accessing those: https://www.erlang.org/doc/man/filename.html#basedir-2

One thing to keep in mind though is that we can have multiple instances, and we likely don't want to mix those configs? So we need to store the port or similar on those file names.

So, Env vars would override any Project level and Global conf, Project level confs would overide Global conf, and everything else is inherited from the Global conf.

I actually think the persisted configuration should win. Otherwise you can run into an issue where:

  1. You start livebook with some flag/env var
  2. You change it in the panel
  3. You restart it and your changes are reverted

We should always log whenever we load persisted configuration (and perhaps the keys that we are loading).

However, I am not sure if this would work in all "Filesystems".

This is unrelated with the Filesystem engine we have today. This will more likely be another engine, probably more about storage.

@Benjamin-Philip
Copy link
Contributor

You restart it and your changes are reverted

If you have exported the Env var (i.e. you have not passed the Env var when calling the livebook cmd) and restart it in the same shell, your changes would have no effect as Env vars have more importance. I am still am in the opinion that Envs win.

One thing to keep in mind though is that we can have multiple instances, and we likely don't want to mix those configs? So we need to store the port or similar on those file names.

Shouldn't information that vary from instance to instance be stored in memory? Also, in such a case, you would probably use an Env var. However using an Env var has a disadvantage of needing a shell instance for each livebook instance.

@josevalim
Copy link
Contributor Author

I am still am in the opinion that Envs win.

There actual use cases where this won't work. For example, take Fly.io. They generate a random password for your first access via env var. Now imagine you change this password in the UI. If for some reason the node is restarted, your new password is lost. Perhaps there is a way to tackle this at the UI level, but having Env vars always winning will definitely fail here.

Keep in mind this only matters if you are passing a secret key base. So local usage most likely will be unaffected.

@Benjamin-Philip
Copy link
Contributor

Benjamin-Philip commented Nov 11, 2021

@josevalim,

There actual use cases where this won't work. For example, take Fly.io. They generate a random password for your first access via env var. Now imagine you change this password in the UI. If for some reason the node is restarted, your new password is lost. Perhaps there is a way to tackle this at the UI level, but having Env vars always winning will definitely fail here.

Hmm. Maybe they are an issue here. The problem is how do we provide a quick way to apply instance only configs?

One solution is to admit that it is the user's fault that he forgot to update the passwd and we should perhaps print warnings/info msgs when taking configs Env vars. This however, is not really a solution as all it does is keep the user informed that x configs are being overridden by Env vars.

Keep in mind this only matters if you are passing a secret key base. So local usage most likely will be unaffected.

There might be similar scenarios which apply to local usage.

I also must admit that I am only thinking about it from a local usage perspective since only changes to local usage will affect me. So, apologies if I didn't consider some thing important for hosted solutions.

@josevalim
Copy link
Contributor Author

Another configuration we may want to add here is the autosave one. Currently it is an env var or a flag, but we may want to have it exclusively in the Settings page?

@eksperimental
Copy link
Contributor

eksperimental commented Dec 7, 2021

Runtime configuration will be covered by this one?
When Runtime is Mix Standalone, I need to reset re-set it manually every time I reopen a livebook.

@josevalim
Copy link
Contributor Author

What do you mean by resetting it? Can you describe the behavior?

@eksperimental
Copy link
Contributor

sorry, i need to RE-SET (to re configure)

@josevalim
Copy link
Contributor Author

Sorry, it is still not clear. Can you please describe what you are seeing and what you expect in details? I don't know for example whose runtime configuration you are talking about, Livebook's or the app you are running on? The more detail you can give, the better.

@eksperimental
Copy link
Contributor

eksperimental commented Dec 7, 2021

Sorry, I thought it would be enough. I've been using Livebook just for 3 days so I may be missing something.

The runtime configuration is not saved in the Livebook ( I don't expect this) neither in the local configuration of Livebook. I keep a Livebook inside my mix project, Livebook is launched from another dir, since this project run on an older Elixir version.
When I was the Livebook and reopen it, I need to go to "Runtime settings" > GEAR ICON > "Mix Standalone", navigate to the folder of the project, and hit RECONNECT., wait, and click on ESC.

I expect my local installation of Livebook remember this.

@josevalim
Copy link
Contributor Author

There is an environment variable for default runtime, but I believe Mix is not supported. I don't think this would be one of the settings we would add though. We need to look into it.

@Benjamin-Philip
Copy link
Contributor

Benjamin-Philip commented Dec 7, 2021

I except my local installation of livebook remember this.

I think this is a per livebook specific configuration, so I would not put this under persistent configuration.
I would however, allow for default values to be picked by persistent config.

@eksperimental
Copy link
Contributor

I except my local installation of livebook remember this.

I think this is a per livebook specific configuration, so I would not put this under persistent configuration. I would however, allow for default values to be picked by persistent config.

Well, it can remember the local location of your livebook or md5 hash of it, and remember the last Runtime configuration, since it is unlike to change from load to load.

@eksperimental
Copy link
Contributor

There is an environment variable for default runtime, but I believe Mix is not supported. I don't think this would be one of the settings we would add though. We need to look into it.

Should i create a new issue for this?

@josevalim
Copy link
Contributor Author

Please don’t. It is not clear we will support it and, if we will, it is only after this issue anyway. :)

@jonatanklosko
Copy link
Member

Hey @eksperimental, as Jose mentioned we have a config option for specifying the default runtime and it does in fact work for the mix one too. So if you start with livebook server --default-runtime mix:/path/to/project, then all sessions will use that runtime by default.

@Benjamin-Philip
Copy link
Contributor

@josevalim, could I tackle this?

I'll work on support for Global and project wise configuration, or maybe even just Global config.

Do note it may take about 2 months to send a PR. I hope there's no rush!

@josevalim
Copy link
Contributor Author

Hi @Benjamin-Philip, we may need this sooner as it is important for the desktop app. So I recommend picking up a smaller issue. thanks for asking!

@josevalim
Copy link
Contributor Author

Thank you @Qizot for the PR. I have updated this PR description with the tasks we identified and broke apart.

@jonatanklosko
Copy link
Member

@Qizot awesome job :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:backend Related to the backend feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants