Only update the target firmware file with deltas that are smaller (#2308)

Instead of creating a new firmware file from scratch, this only
updates/replaces files which have changed.

This is an extension of #2307

---------

Co-authored-by: Lars Wikman <[email protected]>

Author: joshk

Dockerfile

@@ -154,7 +154,7 @@ FROM ${RUNNER_IMAGE} AS app
 
 RUN apt-get update -y && \
     apt-get upgrade -y && \
-    apt-get install -y openssl ca-certificates locales bash xdelta3 && \
+    apt-get install -y openssl ca-certificates locales bash xdelta3 zip unzip && \
     apt-get autoremove -y && \
     apt-get clean && \
     rm -rf /var/lib/apt/lists/*

lib/nerves_hub/firmwares/update_tool/fwup.ex

@@ -16,6 +16,10 @@ defmodule NervesHub.Firmwares.UpdateTool.Fwup do
   @very_safe_version "1.13.0"
   @oldest_version Version.parse!("0.2.0")
 
+  # If a payload is smaller than this there is no point to do a delta, the overhead takes more space
+  # in the best case
+  @delta_overhead_limit 22
+
   @impl NervesHub.Firmwares.UpdateTool
   def get_firmware_metadata_from_file(filepath) do
     with {:ok, firmware_metadata} <- FwupUtil.metadata(filepath),
@@ -61,7 +65,7 @@ defmodule NervesHub.Firmwares.UpdateTool.Fwup do
           {:ok, output}
 
         {:error, reason} ->
-          Logger.warning("Could not create a firmware delta: #{inspect(reason)}",
+          Logger.warning("Firmware delta creation failed: #{inspect(reason)}",
             source_url: source_url,
             target_url: target_url
           )
@@ -158,7 +162,7 @@ defmodule NervesHub.Firmwares.UpdateTool.Fwup do
          {:ok, all_delta_files} <- delta_files(deltas) do
       Logger.info("Generating delta for files: #{Enum.join(all_delta_files, ", ")}")
 
-      _ =
+      file_list =
         for absolute <- Path.wildcard(target_work_dir <> "/**"), not File.dir?(absolute) do
           path = Path.relative_to(absolute, target_work_dir)
 
@@ -168,62 +172,107 @@ defmodule NervesHub.Firmwares.UpdateTool.Fwup do
           |> Path.dirname()
           |> File.mkdir_p!()
 
-          _ =
-            case path do
-              "meta." <> _ ->
-                File.cp!(Path.join(target_work_dir, path), Path.join(output_work_dir, path))
-
-              "data/" <> subpath ->
-                if subpath in all_delta_files do
-                  source_filepath = Path.join(source_work_dir, path)
-                  target_filepath = Path.join(target_work_dir, path)
-
-                  case File.stat(source_filepath) do
-                    {:ok, %{size: f_source_size}} ->
-                      args = [
-                        "-A",
-                        "-S",
-                        "-f",
-                        "-s",
-                        source_filepath,
-                        target_filepath,
-                        output_path
-                      ]
-
-                      %{size: f_target_size} = File.stat!(target_filepath)
-
-                      {_, 0} = System.cmd("xdelta3", args, stderr_to_stdout: true, env: [])
-                      %{size: f_delta_size} = File.stat!(output_path)
-
-                      Logger.info(
-                        "Generated delta for #{path}, from #{Float.round(f_source_size / 1024 / 1024, 1)} MB to #{Float.round(f_target_size / 1024 / 1024, 1)} MB via delta of #{Float.round(f_delta_size / 1024 / 1024, 1)} MB"
-                      )
-
-                    {:error, :enoent} ->
-                      File.cp!(target_filepath, output_path)
-                  end
-                else
-                  File.cp!(Path.join(target_work_dir, path), Path.join(output_work_dir, path))
-                end
-            end
+          maybe_generate_delta(
+            path,
+            source_work_dir,
+            target_work_dir,
+            output_work_dir,
+            all_delta_files
+          )
         end
+        |> Enum.reject(&is_nil(&1))
+
+      {:ok, delta_zip_path} = Plug.Upload.random_file("#{source_uuid}_#{target_uuid}_delta.zip")
+      _ = File.rm(delta_zip_path)
+      _ = File.cp(target_path, delta_zip_path)
+
+      with {true, :changes_in_delta} <- {Enum.any?(file_list), :changes_in_delta},
+           :ok <- update_changed_files(file_list, delta_zip_path, output_work_dir),
+           {:ok, %{size: delta_size}} <- File.stat(delta_zip_path),
+           {true, :delta_smaller} <- {delta_size < target_size, :delta_smaller} do
+        {:ok,
+         %{
+           filepath: delta_zip_path,
+           size: delta_size,
+           source_size: source_size,
+           target_size: target_size,
+           tool: "fwup",
+           tool_metadata: tool_metadata
+         }}
+      else
+        {false, :changes_in_delta} -> {:error, :no_changes_in_delta}
+        {false, :delta_smaller} -> {:error, :delta_larger_than_target}
+      end
+    end
+  end
 
-      {:ok, delta_zip_path} = Plug.Upload.random_file("#{source_uuid}_#{target_uuid}_delta")
+  defp update_changed_files(file_list, delta_zip_path, output_work_dir) do
+    for file <- file_list do
+      args = ["-9", delta_zip_path, String.replace_prefix(file, "#{output_work_dir}/", "")]
+      {_output, 0} = System.cmd("zip", args, cd: output_work_dir)
+    end
 
-      {:ok, _} =
-        :zip.create(to_charlist(delta_zip_path), generate_file_list(output_work_dir), cwd: to_charlist(output_work_dir))
+    :ok
+  end
 
-      {:ok, %{size: size}} = File.stat(delta_zip_path)
+  defp maybe_generate_delta("meta." <> _, _, _, _, _) do
+    nil
+  end
 
-      {:ok,
-       %{
-         filepath: delta_zip_path,
-         size: size,
-         source_size: source_size,
-         target_size: target_size,
-         tool: "fwup",
-         tool_metadata: tool_metadata
-       }}
+  defp maybe_generate_delta(
+         "data/" <> subpath = path,
+         source_work_dir,
+         target_work_dir,
+         output_work_dir,
+         all_delta_files
+       ) do
+    output_path = Path.join(output_work_dir, path)
+    target_filepath = Path.join(target_work_dir, path)
+
+    if subpath in all_delta_files do
+      source_filepath = Path.join(source_work_dir, path)
+
+      case File.stat(source_filepath) do
+        {:ok, %{size: f_source_size}} ->
+          args = [
+            "-A",
+            "-S",
+            "-f",
+            "-s",
+            source_filepath,
+            target_filepath,
+            output_path
+          ]
+
+          %{size: f_target_size} = File.stat!(target_filepath)
+
+          if f_target_size < @delta_overhead_limit do
+            Logger.info("Skipping generating delta for #{path} it is under 22 bytes.")
+            nil
+          else
+            {_, 0} = System.cmd("xdelta3", args, stderr_to_stdout: true, env: [])
+            %{size: f_delta_size} = File.stat!(output_path)
+
+            if f_delta_size < f_target_size do
+              Logger.info(
+                "Generated delta for #{path}, from #{Float.round(f_source_size / 1024 / 1024, 1)} MB to #{Float.round(f_target_size / 1024 / 1024, 1)} MB via delta of #{Float.round(f_delta_size / 1024 / 1024, 1)} MB"
+              )
+
+              output_path
+            else
+              Logger.info(
+                "Skipping generated delta for #{path}, delta is larger: #{Float.round(f_source_size / 1024 / 1024, 1)} MB to #{Float.round(f_target_size / 1024 / 1024, 1)} MB via delta of #{Float.round(f_delta_size / 1024 / 1024, 1)} MB"
+              )
+
+              nil
+            end
+          end
+
+        {:error, :enoent} ->
+          nil
+      end
+    else
+      nil
     end
   end
 
@@ -239,25 +288,6 @@ defmodule NervesHub.Firmwares.UpdateTool.Fwup do
     end
   end
 
-  defp generate_file_list(workdir) do
-    # firmware archive files order matters:
-    # 1. meta.conf.ed25519 (optional)
-    # 2. meta.conf
-    # 3. other...
-    [
-      "meta.conf.*",
-      "meta.conf",
-      "data"
-    ]
-    |> Enum.map(fn glob -> workdir |> Path.join(glob) |> Path.wildcard() end)
-    |> List.flatten()
-    |> Enum.map(fn file ->
-      file
-      |> String.replace_prefix("#{workdir}/", "")
-      |> to_charlist()
-    end)
-  end
-
   defp get_tool_metadata(meta_conf_path) do
     with {:ok, feature_usage} <- Confuse.Fwup.get_feature_usage(meta_conf_path) do
       tool_metadata =

test/fixtures/fwup/raw-two-files.conf

@@ -0,0 +1,35 @@
+    define(ROOTFS_A_PART_OFFSET, 1024)
+    define(ROOTFS_A_PART_COUNT, 1024)
+    define(ROOTFS_B_PART_OFFSET, 2048)
+    define(ROOTFS_B_PART_COUNT, 1024)
+
+    file-resource first {
+            host-path = "${TEST_1}"
+    }
+    file-resource second {
+            host-path = "${TEST_2}"
+    }
+
+    task complete {
+        on-init {
+            raw_memset(${ROOTFS_B_PART_OFFSET}, ${ROOTFS_B_PART_COUNT}, 0)
+        }
+        on-resource first {
+          raw_write(${ROOTFS_A_PART_OFFSET})
+        }
+        on-resource second {
+          raw_write(${ROOTFS_A_PART_OFFSET})
+        }
+    }
+    task upgrade {
+        on-resource first {
+            delta-source-raw-offset=${ROOTFS_A_PART_OFFSET}
+            delta-source-raw-count=${ROOTFS_A_PART_COUNT}
+            raw_write(${ROOTFS_B_PART_OFFSET})
+        }
+        on-resource second {
+            delta-source-raw-offset=${ROOTFS_A_PART_OFFSET}
+            delta-source-raw-count=${ROOTFS_A_PART_COUNT}
+            raw_write(${ROOTFS_B_PART_OFFSET})
+        }
+    }