Update the delta file name on upload (#2305)

After a recent update of mine the deltas we uploaded to s3 had the path
of: `/firmware/[org
id]/generated_delta_zip_file-[random]-[random]-[integer]`

This PR changes it to `/firmware/[org id]/[source uuid]/[target
uuid].delta.fw`

I've also added the Oban Job id to the Logger metadata for easier log
searching, and reduced the time diff in the delta generation timeout
Oban job from 120,000 seconds (33 hours) to 960 seconds, (15 minutes).

Author: joshk

lib/nerves_hub/firmwares.ex

@@ -421,55 +421,58 @@ defmodule NervesHub.Firmwares do
     {:ok, source_url} = firmware_upload_config().download_file(source_firmware)
     {:ok, target_url} = firmware_upload_config().download_file(target_firmware)
 
-    case update_tool().create_firmware_delta_file(source_url, target_url) do
+    case update_tool().create_firmware_delta_file(
+           {source_firmware.uuid, source_url},
+           {target_firmware.uuid, target_url}
+         ) do
       {:ok, created} ->
-        firmware_delta_filename = Path.basename(created.filepath)
-
-        result =
-          Repo.transaction(
-            fn ->
-              with upload_metadata <-
-                     firmware_upload_config().metadata(org.id, firmware_delta_filename),
-                   {:ok, firmware_delta} <-
-                     complete_firmware_delta(
-                       firmware_delta,
-                       created.tool,
-                       created.size,
-                       created.source_size,
-                       created.target_size,
-                       created.tool_metadata,
-                       upload_metadata
-                     ),
-                   {:ok, firmware_delta} <- get_firmware_delta(firmware_delta.id),
-                   :ok <-
-                     firmware_upload_config().upload_file(created.filepath, upload_metadata),
-                   :ok <- update_tool().cleanup_firmware_delta_files(created.filepath) do
-                Logger.info(
-                  "Created firmware delta successfully.",
+        Repo.transact(
+          fn ->
+            with upload_metadata <-
+                   firmware_upload_config().delta_metadata(
+                     org.id,
+                     source_firmware.uuid,
+                     target_firmware.uuid
+                   ),
+                 {:ok, firmware_delta} <-
+                   complete_firmware_delta(
+                     firmware_delta,
+                     created.tool,
+                     created.size,
+                     created.source_size,
+                     created.target_size,
+                     created.tool_metadata,
+                     upload_metadata
+                   ),
+                 {:ok, firmware_delta} <- get_firmware_delta(firmware_delta.id),
+                 :ok <-
+                   firmware_upload_config().upload_file(created.filepath, upload_metadata),
+                 :ok <- update_tool().cleanup_firmware_delta_files(created.filepath) do
+              Logger.info(
+                "Created firmware delta successfully.",
+                product_id: source_firmware.product_id,
+                source_firmware: source_firmware.uuid,
+                target_firmware: target_firmware.uuid
+              )
+
+              {:ok, firmware_delta}
+            else
+              {:error, error} ->
+                update_tool().cleanup_firmware_delta_files(created.filepath)
+
+                Logger.error(
+                  "Failed to create firmware delta: #{inspect(error)}",
                   product_id: source_firmware.product_id,
                   source_firmware: source_firmware.uuid,
                   target_firmware: target_firmware.uuid
                 )
 
-                firmware_delta
-              else
-                {:error, error} ->
-                  update_tool().cleanup_firmware_delta_files(created.filepath)
-
-                  Logger.error(
-                    "Failed to create firmware delta: #{inspect(error)}",
-                    product_id: source_firmware.product_id,
-                    source_firmware: source_firmware.uuid,
-                    target_firmware: target_firmware.uuid
-                  )
-
-                  Repo.rollback(error)
-              end
-            end,
-            timeout: 30_000
-          )
-
-        case result do
+                {:error, error}
+            end
+          end,
+          timeout: 30_000
+        )
+        |> case do
           {:ok, _delta} ->
             :ok
 

lib/nerves_hub/firmwares/update_tool.ex

@@ -89,7 +89,10 @@ defmodule NervesHub.Firmwares.UpdateTool do
   @doc """
   Called to create a firmware delta file on the local filesystem
   """
-  @callback create_firmware_delta_file(String.t(), String.t()) ::
+  @callback create_firmware_delta_file(
+              {source_id :: String.t(), source_url :: String.t()},
+              {target_id :: String.t(), target_url :: String.t()}
+            ) ::
               {:ok, delta_created()} | {:error, term()}
 
   @doc """

lib/nerves_hub/firmwares/update_tool/fwup.ex

@@ -45,9 +45,8 @@ defmodule NervesHub.Firmwares.UpdateTool.Fwup do
   end
 
   @impl NervesHub.Firmwares.UpdateTool
-  def create_firmware_delta_file(source_url, target_url) do
-    uuid = Ecto.UUID.generate()
-    work_dir = Path.join(System.tmp_dir(), uuid) |> Path.expand()
+  def create_firmware_delta_file({source_uuid, source_url}, {target_uuid, target_url}) do
+    work_dir = Path.join(System.tmp_dir(), "#{source_uuid}_#{target_uuid}")
     _ = File.mkdir_p(work_dir)
 
     try do
@@ -57,7 +56,7 @@ defmodule NervesHub.Firmwares.UpdateTool.Fwup do
       dl!(source_url, source_path)
       dl!(target_url, target_path)
 
-      case do_delta_file(source_path, target_path, work_dir) do
+      case do_delta_file({source_uuid, source_path}, {target_uuid, target_path}, work_dir) do
         {:ok, output} ->
           {:ok, output}
 
@@ -139,7 +138,7 @@ defmodule NervesHub.Firmwares.UpdateTool.Fwup do
     end)
   end
 
-  def do_delta_file(source_path, target_path, work_dir) do
+  def do_delta_file({source_uuid, source_path}, {target_uuid, target_path}, work_dir) do
     source_work_dir = Path.join(work_dir, "source")
     target_work_dir = Path.join(work_dir, "target")
     output_work_dir = Path.join(work_dir, "output")
@@ -210,7 +209,7 @@ defmodule NervesHub.Firmwares.UpdateTool.Fwup do
             end
         end
 
-      {:ok, delta_zip_path} = Plug.Upload.random_file("generated_delta_zip_file")
+      {:ok, delta_zip_path} = Plug.Upload.random_file("#{source_uuid}_#{target_uuid}_delta")
 
       {:ok, _} =
         :zip.create(to_charlist(delta_zip_path), generate_file_list(output_work_dir),

lib/nerves_hub/firmwares/upload.ex

@@ -33,4 +33,13 @@ defmodule NervesHub.Firmwares.Upload do
   Called to generate the upload_metadata that will be persisted for later lookup.
   """
   @callback metadata(Org.id(), String.t()) :: upload_metadata()
+
+  @doc """
+  Called to generate the upload_metadata specific to delta firmwares that will be persisted for later lookup.
+  """
+  @callback delta_metadata(
+              Org.id(),
+              source_firmware_uuid :: String.t(),
+              target_firmware_uuid :: String.t()
+            ) :: upload_metadata()
 end

lib/nerves_hub/firmwares/upload/file.ex

@@ -35,8 +35,9 @@ defmodule NervesHub.Firmwares.Upload.File do
 
     config = Application.get_env(:nerves_hub, __MODULE__)
 
-    common_path = "#{org_id}"
-    local_path = Path.join([config[:local_path], common_path, filename])
+    common_path = Path.join([key_prefix(), Integer.to_string(org_id), filename])
+
+    local_path = Path.join([config[:local_path], common_path])
 
     port =
       if Enum.member?([443, 80], web_config[:url][:port]),
@@ -53,4 +54,42 @@ defmodule NervesHub.Firmwares.Upload.File do
       local_path: local_path
     }
   end
+
+  @impl NervesHub.Firmwares.Upload
+  def delta_metadata(org_id, source_firmware_uuid, target_firmware_uuid) do
+    web_config = Application.get_env(:nerves_hub, NervesHubWeb.Endpoint)
+
+    config = Application.get_env(:nerves_hub, __MODULE__)
+
+    common_path =
+      Path.join([
+        key_prefix(),
+        Integer.to_string(org_id),
+        source_firmware_uuid,
+        "#{target_firmware_uuid}.delta.fw"
+      ])
+
+    local_path = Path.join([config[:local_path], common_path])
+
+    port =
+      if Enum.member?([443, 80], web_config[:url][:port]),
+        do: "",
+        else: ":#{web_config[:url][:port]}"
+
+    public_path =
+      Path.join([
+        "#{web_config[:url][:scheme]}://#{web_config[:url][:host]}#{port}/",
+        config[:public_path],
+        common_path
+      ])
+
+    %{
+      public_path: public_path,
+      local_path: local_path
+    }
+  end
+
+  def key_prefix() do
+    "firmware"
+  end
 end

lib/nerves_hub/firmwares/upload/s3.ex

@@ -55,6 +55,19 @@ defmodule NervesHub.Firmwares.Upload.S3 do
     %{"s3_key" => Path.join([key_prefix(), Integer.to_string(org_id), filename])}
   end
 
+  @impl NervesHub.Firmwares.Upload
+  def delta_metadata(org_id, source_firmware_uuid, target_firmware_uuid) do
+    %{
+      "s3_key" =>
+        Path.join([
+          key_prefix(),
+          Integer.to_string(org_id),
+          source_firmware_uuid,
+          "#{target_firmware_uuid}.delta.fw"
+        ])
+    }
+  end
+
   def bucket() do
     Application.get_env(:nerves_hub, __MODULE__)[:bucket]
   end

lib/nerves_hub/workers/firmware_delta_builder.ex

@@ -13,14 +13,15 @@ defmodule NervesHub.Workers.FirmwareDeltaBuilder do
   alias NervesHub.ManagedDeployments
 
   @impl Oban.Worker
-  def perform(%Oban.Job{args: %{"source_id" => source_id, "target_id" => target_id}}) do
+  def perform(%Oban.Job{id: id, args: %{"source_id" => source_id, "target_id" => target_id}}) do
     source = Firmwares.get_firmware!(source_id)
     target = Firmwares.get_firmware!(target_id)
 
     Logger.metadata(
       product_id: source.product_id,
       source_firmware: source.uuid,
-      target_firmware: target.uuid
+      target_firmware: target.uuid,
+      job_id: id
     )
 
     :ok = maybe_create_firmware_delta(source, target)

lib/nerves_hub/workers/firmware_delta_timeout.ex

@@ -5,7 +5,8 @@ defmodule NervesHub.Workers.FirmwareDeltaTimeout do
 
   alias NervesHub.Firmwares
 
-  @delta_generation_timeout 120_000
+  # 15min timeout
+  @delta_generation_timeout 960
 
   @impl Oban.Worker
   def perform(_) do

test/nerves_hub/firmwares/update_tool_test.exs

@@ -197,7 +197,7 @@ defmodule NervesHub.Firmwares.UpdateToolTest do
          source_size: ^source_size,
          target_size: ^target_size
        }} =
-        Fwup.do_delta_file(fw_a, fw_b, Path.join(dir, "work"))
+        Fwup.do_delta_file({"aaa", fw_a}, {"bbb", fw_b}, Path.join(dir, "work"))
 
       assert %{size: ^delta_size} = File.stat!(delta_path)
 
@@ -243,7 +243,7 @@ defmodule NervesHub.Firmwares.UpdateToolTest do
          source_size: ^source_size,
          target_size: ^target_size
        }} =
-        Fwup.do_delta_file(fw_a, fw_b, Path.join(dir, "work"))
+        Fwup.do_delta_file({"aaa", fw_a}, {"bbb", fw_b}, Path.join(dir, "work"))
 
       assert %{size: ^delta_size} = File.stat!(delta_path)
 
@@ -289,7 +289,7 @@ defmodule NervesHub.Firmwares.UpdateToolTest do
          source_size: ^source_size,
          target_size: ^target_size
        }} =
-        Fwup.do_delta_file(fw_a, fw_b, Path.join(dir, "work"))
+        Fwup.do_delta_file({"aaa", fw_a}, {"bbb", fw_b}, Path.join(dir, "work"))
 
       assert %{size: ^delta_size} = File.stat!(delta_path)
 
@@ -335,7 +335,7 @@ defmodule NervesHub.Firmwares.UpdateToolTest do
          source_size: ^source_size,
          target_size: ^target_size
        }} =
-        Fwup.do_delta_file(fw_a, fw_b, Path.join(dir, "work"))
+        Fwup.do_delta_file({"aaa", fw_a}, {"bbb", fw_b}, Path.join(dir, "work"))
 
       assert %{size: ^delta_size} = File.stat!(delta_path)
 
@@ -403,7 +403,7 @@ defmodule NervesHub.Firmwares.UpdateToolTest do
          source_size: ^source_size,
          target_size: ^target_size
        }} =
-        Fwup.do_delta_file(fw_a, fw_b, Path.join(dir, "work"))
+        Fwup.do_delta_file({"aaa", fw_a}, {"bbb", fw_b}, Path.join(dir, "work"))
 
       assert %{size: ^delta_size} = File.stat!(delta_path)
 
@@ -443,7 +443,7 @@ defmodule NervesHub.Firmwares.UpdateToolTest do
       fw_b = build_fw!(Path.join(dir, "b.fw"), raw_conf_path, data_path_2)
 
       {:error, [err]} =
-        Fwup.do_delta_file(fw_a, fw_b, Path.join(dir, "work"))
+        Fwup.do_delta_file({"aaa", fw_a}, {"bbb", fw_b}, Path.join(dir, "work"))
 
       assert err =~
                "Target uses raw deltas and source firmware uses encryption for the same resource but target firmware has no cipher or"
@@ -467,7 +467,7 @@ defmodule NervesHub.Firmwares.UpdateToolTest do
       fw_b = build_fw!(Path.join(dir, "b.fw"), fwup_conf_path, data_path_2)
 
       {:error, :no_delta_support_in_firmware} =
-        Fwup.do_delta_file(fw_a, fw_b, Path.join(dir, "work"))
+        Fwup.do_delta_file({"aaa", fw_a}, {"bbb", fw_b}, Path.join(dir, "work"))
     end
 
     @tag :tmp_dir
@@ -497,7 +497,7 @@ defmodule NervesHub.Firmwares.UpdateToolTest do
          source_size: ^source_size,
          target_size: ^target_size
        }} =
-        Fwup.do_delta_file(fw_a, fw_b, Path.join(dir, "work"))
+        Fwup.do_delta_file({"aaa", fw_a}, {"bbb", fw_b}, Path.join(dir, "work"))
 
       %{size: ^delta_size} = File.stat!(delta_path)
 
@@ -561,7 +561,7 @@ defmodule NervesHub.Firmwares.UpdateToolTest do
          source_size: ^source_size,
          target_size: ^target_size
        }} =
-        Fwup.do_delta_file(fw_a, fw_b, Path.join(dir, "work"))
+        Fwup.do_delta_file({"aaa", fw_a}, {"bbb", fw_b}, Path.join(dir, "work"))
 
       assert %{size: ^delta_size} = File.stat!(delta_path)
 

test/nerves_hub/firmwares_test.exs

@@ -272,7 +272,8 @@ defmodule NervesHub.FirmwaresTest do
       expect(UploadFile, :download_file, fn ^source -> {:ok, source_url} end)
       expect(UploadFile, :download_file, fn ^target -> {:ok, target_url} end)
 
-      expect(UpdateToolDefault, :create_firmware_delta_file, fn ^source_url, ^target_url ->
+      expect(UpdateToolDefault, :create_firmware_delta_file, fn {_, ^source_url},
+                                                                {_, ^target_url} ->
         {:ok,
          %{
            filepath: firmware_delta_path,
@@ -340,7 +341,8 @@ defmodule NervesHub.FirmwaresTest do
       expect(UploadFile, :download_file, fn ^target -> {:ok, target_url} end)
 
       # Force error
-      expect(UpdateToolDefault, :create_firmware_delta_file, fn ^source_url, ^target_url ->
+      expect(UpdateToolDefault, :create_firmware_delta_file, fn {_, ^source_url},
+                                                                {_, ^target_url} ->
         {:error, :delta_not_created}
       end)