From 3ca36a6ce769165abefd433242ab9d5da0119a63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sat, 30 Sep 2023 09:27:09 +0200 Subject: [PATCH] Run migrations early in the supervision tree (#2241) Also add a migration version, improve docs, and document code that actually cannot be removed. --- lib/livebook/application.ex | 3 +- lib/livebook/file_system/s3.ex | 3 -- lib/livebook/migration.ex | 73 ++++++++++++++++++-------------- lib/livebook/notebook_manager.ex | 24 +++++------ 4 files changed, 54 insertions(+), 49 deletions(-) diff --git a/lib/livebook/application.ex b/lib/livebook/application.ex index 58c678a16..dbfdd09d5 100644 --- a/lib/livebook/application.ex +++ b/lib/livebook/application.ex @@ -18,6 +18,8 @@ defmodule Livebook.Application do {Task.Supervisor, name: Livebook.TaskSupervisor}, # Start the storage module Livebook.Storage, + # Run migrations as soon as the storage is running + Livebook.Migration, # Start the periodic version check Livebook.UpdateCheck, # Periodic measurement of system resources @@ -57,7 +59,6 @@ defmodule Livebook.Application do case Supervisor.start_link(children, opts) do {:ok, _} = result -> - Livebook.Migration.migrate() load_lb_env_vars() create_offline_hub() clear_env_vars() diff --git a/lib/livebook/file_system/s3.ex b/lib/livebook/file_system/s3.ex index c16d82386..2b5b8ed0a 100644 --- a/lib/livebook/file_system/s3.ex +++ b/lib/livebook/file_system/s3.ex @@ -86,10 +86,7 @@ defmodule Livebook.FileSystem.S3 do def id(_, nil), do: nil def id(nil, bucket_url), do: hashed_id(bucket_url) - - # For Personal hub we keep unprefixed ids def id(@personal_id, bucket_url), do: hashed_id(bucket_url) - def id(hub_id, bucket_url), do: "#{hub_id}-#{hashed_id(bucket_url)}" defp hashed_id(bucket_url) do diff --git a/lib/livebook/migration.ex b/lib/livebook/migration.ex index 0dd5ff14a..63af59dda 100644 --- a/lib/livebook/migration.ex +++ b/lib/livebook/migration.ex @@ -1,17 +1,40 @@ defmodule Livebook.Migration do - @doc """ - Runs all migrations. - """ - @spec migrate() :: :ok - def migrate() do - delete_local_host_hub() + use GenServer, restart: :temporary + + # We version our storage so we can remove migrations in the future + # by deleting the whole storage when finding too old versions. + # + # We also document tables and IDs used in previous versions, + # as those must be avoided in the future. + # + # ## v1 (Livebook v0.11, Oct 2023) + # + # * Deleted hubs.local-host + # * Migrated secrets to hub_secrets + # * Migrated filesystems to file_systems + # * Migrated settings.global.default_file_system_id to settings.global.default_dir + # + @migration_version 1 + + def start_link(_opts) do + GenServer.start_link(__MODULE__, :ok) + end + + def init(:ok) do insert_personal_hub() - move_app_secrets_to_personal_hub() + + # v1 add_personal_hub_secret_key() + delete_local_host_hub() + move_app_secrets_to_personal_hub() + + # TODO: remove on Livebook v0.12 update_file_systems_to_deterministic_ids() ensure_new_file_system_attributes() move_default_file_system_id_to_default_dir() - :ok + + Livebook.Storage.insert(:system, "global", migration_version: @migration_version) + :ignore end defp delete_local_host_hub() do @@ -49,15 +72,14 @@ defmodule Livebook.Migration do end end + # We changed S3 file system ids, such that they are deterministic + # for the same bucket, rather than random. We take this opportunity + # to rename the scope from :filesystem to :file_systems, which + # conveniently allows for easy check if there's anything to migrate. + # This migration can be removed in the future (at the cost of discarding + # very old file systems (which can be re-added). + # TODO: remove on Livebook v0.12 defp update_file_systems_to_deterministic_ids() do - # We changed S3 file system ids, such that they are deterministic - # for the same bucket, rather than random. We take this opportunity - # to rename the scope from :filesystem to :file_systems, which - # conveniently allows for easy check if there's anything to migrate. - # This migration can be removed in the future (at the cost of discarding - # very old file systems (which can be re-added). - # TODO: remove on Livebook v0.12 - case Livebook.Storage.all(:filesystem) do [] -> :ok @@ -89,23 +111,14 @@ defmodule Livebook.Migration do {:ok, new_id} <- Map.fetch(id_mapping, default_file_system_id) do Livebook.Storage.insert(:settings, "global", default_file_system_id: new_id) end - - # Livebook.NotebookManager is started before the migration runs, - # and it discards S3 files, since it can't find the file system. - # However, in this case it's fine; for recent notebooks it does - # not matter really and there are likely not many starred S3 - # notebooks at this point (and the user can easily star again) end end + # Note that this is already handled by update_file_systems_to_deterministic_ids/0, + # we add this migration so it also applies to people using Livebook main. + # TODO: remove on Livebook v0.12 defp ensure_new_file_system_attributes() do - # Note that this is already handled by update_file_systems_to_deterministic_ids/0, - # we add this migration so it also applies to people using Livebook main - - # TODO: remove on Livebook v0.12 - for attrs <- Livebook.Storage.all(:file_systems) do - # Ensure new file system fields new_attrs = %{ hub_id: Livebook.Hubs.Personal.id(), external_id: nil, @@ -118,10 +131,8 @@ defmodule Livebook.Migration do end end + # TODO: remove on Livebook v0.12 defp move_default_file_system_id_to_default_dir() do - # Convert default_file_system_id to default_dir - # TODO: remove on Livebook v0.12 - with {:ok, default_file_system_id} <- Livebook.Storage.fetch_key(:settings, "global", :default_file_system_id) do Livebook.Hubs.get_file_systems() diff --git a/lib/livebook/notebook_manager.ex b/lib/livebook/notebook_manager.ex index a9f4db244..069be2d76 100644 --- a/lib/livebook/notebook_manager.ex +++ b/lib/livebook/notebook_manager.ex @@ -256,7 +256,16 @@ defmodule Livebook.NotebookManager do end end - defp load_file(%{file_system_id: file_system_id, file_system_type: file_system_type, path: path}) do + defp load_file(%{file_system_id: file_system_id, path: path} = file) do + # In the past, not all files had a file_system_type, so we need to detect one from the id. + file_system_type = + Map.get_lazy(file, :file_system_type, fn -> + case file_system_id do + "local" -> "local" + "s3-" <> _ -> "s3" + end + end) + %FileSystem.File{ file_system_id: file_system_id, file_system_module: Livebook.FileSystems.type_to_module(file_system_type), @@ -265,19 +274,6 @@ defmodule Livebook.NotebookManager do } end - # TODO: remove on Livebook v0.12 - # NotebookManager starts before we run migrations, so we have a - # fallback here instead - defp load_file(%{file_system_id: file_system_id, path: path}) do - file_system_type = - case file_system_id do - "local" -> "local" - "s3-" <> _ -> "s3" - end - - load_file(%{file_system_id: "local", path: path, file_system_type: file_system_type}) - end - defp load_datetime(datetime) do DateTime.from_unix!(datetime, :microsecond) end