From f1a11582b8bd6985ac024c02c52f5397e96dc414 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= <romain@blogreen.org>
Date: Wed, 23 Dec 2020 10:19:21 -1000
Subject: [PATCH] (#605) Rework cache storage

Store each cached file in a single directory using the file checksum as
filename.  That was, if two tasks distribute a file with the exact same
content, it is storred only once on disk in the cache directory.

The original filename is used to build the spool hierarchy so given the
following specification:

```
[
  {
    "filename": "database_delete.sh",
    "sha256": "27cf85fc65b33bd3492e0188533d4a25d67cd810d91f61b4baf6e89cc675539a",
    "size_bytes": 140,
    "uri": {
      "path": "/puppet/v3/static_file_content/modules/odoo/tasks/database_delete.sh",
      "params": {
        "environment": "production",
        "code_id": "cf812ee55e84bf63a0b8ee4a08b2c3fad26dc313"
      }
    }
  },
  {
    "filename": "odoo/files/database-delete.py",
    "sha256": "deffdfa89fac5e02d689d26ae43c933e47cb10ea12d56dd6c72184043bc56f25",
    "size_bytes": 96,
    "uri": {
      "path": "/puppet/v3/static_file_content/modules/odoo/files/database-delete.py",
      "params": {
        "environment": "production",
        "code_id": "cf812ee55e84bf63a0b8ee4a08b2c3fad26dc313"
      }
    }
  }
]
```

we end-up with the following hierarchy of files in the cache:

```
<cache-directory>
|-> 27cf85fc65b33bd3492e0188533d4a25d67cd810d91f61b4baf6e89cc675539a
`-> deffdfa89fac5e02d689d26ae43c933e47cb10ea12d56dd6c72184043bc56f25
```
---
 lib/mcollective/util/tasks_support.rb            | 14 +++-----------
 spec/unit/mcollective/util/tasks_support_spec.rb | 11 +----------
 2 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/lib/mcollective/util/tasks_support.rb b/lib/mcollective/util/tasks_support.rb
index f8a29fa..2330179 100644
--- a/lib/mcollective/util/tasks_support.rb
+++ b/lib/mcollective/util/tasks_support.rb
@@ -552,16 +552,8 @@ def parse_task(task)
       #
       # @param file [Hash] a file hash as per the task metadata
       # @return [String] the directory the file would go into
-      def task_dir(file)
-        File.join(cache_dir, file["sha256"])
-      end
-
-      # Determines the full path to cache the task file into
-      #
-      # @param file [Hash] a file hash as per the task metadata
-      # @return [String] the file path to cache into
       def task_file_name(file)
-        File.join(task_dir(file), file["filename"])
+        File.join(cache_dir, file["sha256"])
       end
 
       # Does a HTTP GET against the Puppet Server
@@ -660,7 +652,6 @@ def task_file?(file)
 
         Log.debug("Checking if file %s is cached using %s" % [file_name, file.pretty_inspect])
 
-        return false unless File.directory?(task_dir(file))
         return false unless File.exist?(file_name)
         return false unless file_size(file_name) == file["size_bytes"]
         return false unless file_sha256(file_name) == file["sha256"]
@@ -684,7 +675,8 @@ def cache_task_file(file)
         http_get(path, "Accept" => "application/octet-stream") do |resp|
           raise("Failed to request task content %s: %s: %s" % [path, resp.code, resp.body]) unless resp.code == "200"
 
-          FileUtils.mkdir_p(task_dir(file), :mode => 0o0750)
+          FileUtils.mkdir_p(cache_dir, :mode => 0o0750)
+          FileUtils.rm_rf(file_name) if File.directory?(file_name)
 
           task_file = Tempfile.new("tasks_%s" % file["filename"])
           task_file.binmode
diff --git a/spec/unit/mcollective/util/tasks_support_spec.rb b/spec/unit/mcollective/util/tasks_support_spec.rb
index e0762cc..c043a29 100644
--- a/spec/unit/mcollective/util/tasks_support_spec.rb
+++ b/spec/unit/mcollective/util/tasks_support_spec.rb
@@ -597,26 +597,18 @@ module Util
       end
 
       describe "#task_file" do
-        it "should fail if the dir does not exist" do
-          File.expects(:directory?).with(ts.task_dir(file)).returns(false)
-          expect(ts.task_file?(file)).to be(false)
-        end
-
         it "should fail if the file does not exist" do
-          File.expects(:directory?).with(ts.task_dir(file)).returns(true)
           File.expects(:exist?).with(ts.task_file_name(file)).returns(false)
           expect(ts.task_file?(file)).to be(false)
         end
 
         it "should fail if the file has the wrong size" do
-          File.expects(:directory?).with(ts.task_dir(file)).returns(true)
           File.expects(:exist?).with(ts.task_file_name(file)).returns(true)
           ts.expects(:file_size).with(ts.task_file_name(file)).returns(1)
           expect(ts.task_file?(file)).to be(false)
         end
 
         it "should fail if the file has the wrong sha256" do
-          File.expects(:directory?).with(ts.task_dir(file)).returns(true)
           File.expects(:exist?).with(ts.task_file_name(file)).returns(true)
           ts.expects(:file_size).with(ts.task_file_name(file)).returns(149)
           ts.expects(:file_sha256).with(ts.task_file_name(file)).returns("")
@@ -625,7 +617,6 @@ module Util
         end
 
         it "should pass for correct files" do
-          File.expects(:directory?).with(ts.task_dir(file)).returns(true)
           File.expects(:exist?).with(ts.task_file_name(file)).returns(true)
           ts.expects(:file_size).with(ts.task_file_name(file)).returns(149)
           ts.expects(:file_sha256).with(ts.task_file_name(file)).returns("f3b4821836cf7fe6fe17dfb2924ff6897eba43a44cc4cba0e0ed136b27934ede")
@@ -683,7 +674,7 @@ module Util
 
       describe "#task_file_name" do
         it "should determine the correct file name" do
-          expect(ts.task_file_name(task_fixture["files"][0])).to eq(File.join(cache, "f3b4821836cf7fe6fe17dfb2924ff6897eba43a44cc4cba0e0ed136b27934ede", "ls.rb"))
+          expect(ts.task_file_name(task_fixture["files"][0])).to eq(File.join(cache, "f3b4821836cf7fe6fe17dfb2924ff6897eba43a44cc4cba0e0ed136b27934ede"))
         end
       end