Commit 2274d5104f6e413a2b8899a3c3111a17bbb2f4d7 optimized network usage for the case where a file is already in the filebucket. However, it took away the ability to store paths.
This change restores the ability to store paths while maintaining optimal network usage for the case where the file is already in the filebucket with the given path. This is expected to be the most common case. Paired-with: Jesse Wolfe <[email protected]> Signed-off-by: Paul Berry <[email protected]> --- lib/puppet/file_bucket/dipper.rb | 5 +- lib/puppet/indirector/file_bucket_file/file.rb | 55 ++++++++--- spec/unit/indirector/file_bucket_file/file_spec.rb | 109 ++++++++++++++++++-- 3 files changed, 144 insertions(+), 25 deletions(-) diff --git a/lib/puppet/file_bucket/dipper.rb b/lib/puppet/file_bucket/dipper.rb index f4bef28..de4c01b 100644 --- a/lib/puppet/file_bucket/dipper.rb +++ b/lib/puppet/file_bucket/dipper.rb @@ -34,11 +34,12 @@ class Puppet::FileBucket::Dipper contents = ::File.read(file) begin file_bucket_file = Puppet::FileBucket::File.new(contents, :bucket_path => @local_path) - dest_path = "#{@rest_path}#{file_bucket_file.name}" + files_original_path = absolutize_path(file) + dest_path = "#{@rest_path}#{file_bucket_file.name}#{files_original_path}" # Make a HEAD request for the file so that we don't waste time # uploading it if it already exists in the bucket. - unless Puppet::FileBucket::File.head("#{@rest_path}#{file_bucket_file.checksum_type}/#{file_bucket_file.checksum_data}") + unless Puppet::FileBucket::File.head("#{@rest_path}#{file_bucket_file.checksum_type}/#{file_bucket_file.checksum_data}#{files_original_path}") file_bucket_file.save(dest_path) end diff --git a/lib/puppet/indirector/file_bucket_file/file.rb b/lib/puppet/indirector/file_bucket_file/file.rb index 8bea2d7..0fd8a91 100644 --- a/lib/puppet/indirector/file_bucket_file/file.rb +++ b/lib/puppet/indirector/file_bucket_file/file.rb @@ -14,10 +14,12 @@ module Puppet::FileBucketFile end def find( request ) - checksum = request_to_checksum( request ) - file_path = path_for(request.options[:bucket_path], checksum, 'contents') + checksum, files_original_path = request_to_checksum_and_path( request ) + dir_path = path_for(request.options[:bucket_path], checksum) + file_path = ::File.join(dir_path, 'contents') return nil unless ::File.exists?(file_path) + return nil unless path_match(dir_path, files_original_path) if request.options[:diff_with] hash_protocol = sumtype(checksum) @@ -32,32 +34,47 @@ module Puppet::FileBucketFile end def head(request) - checksum = request_to_checksum(request) - file_path = path_for(request.options[:bucket_path], checksum, 'contents') - ::File.exists?(file_path) + checksum, files_original_path = request_to_checksum_and_path(request) + dir_path = path_for(request.options[:bucket_path], checksum) + + ::File.exists?(::File.join(dir_path, 'contents')) and path_match(dir_path, files_original_path) end def save( request ) instance = request.instance + checksum, files_original_path = request_to_checksum_and_path(request) - save_to_disk(instance) + save_to_disk(instance, files_original_path) instance.to_s end private - def save_to_disk( bucket_file ) + def path_match(dir_path, files_original_path) + return true unless files_original_path # if no path was provided, it's a match + paths_path = ::File.join(dir_path, 'paths') + return false unless ::File.exists?(paths_path) + ::File.open(paths_path) do |f| + f.each do |line| + return true if line.chomp == files_original_path + end + end + return false + end + + def save_to_disk( bucket_file, files_original_path ) filename = path_for(bucket_file.bucket_path, bucket_file.checksum_data, 'contents') - dirname = path_for(bucket_file.bucket_path, bucket_file.checksum_data) + dir_path = path_for(bucket_file.bucket_path, bucket_file.checksum_data) + paths_path = ::File.join(dir_path, 'paths') # If the file already exists, do nothing. if ::File.exist?(filename) verify_identical_file!(bucket_file) else # Make the directories if necessary. - unless ::File.directory?(dirname) + unless ::File.directory?(dir_path) Puppet::Util.withumask(0007) do - ::FileUtils.mkdir_p(dirname) + ::FileUtils.mkdir_p(dir_path) end end @@ -68,15 +85,27 @@ module Puppet::FileBucketFile ::File.open(filename, ::File::WRONLY|::File::CREAT, 0440) do |of| of.print bucket_file.contents end + ::File.open(paths_path, ::File::WRONLY|::File::CREAT, 0640) do |of| + # path will be written below + end + end + end + + unless path_match(dir_path, files_original_path) + ::File.open(paths_path, 'a') do |f| + f.puts(files_original_path) end end end - def request_to_checksum( request ) - checksum_type, checksum, path = request.key.split(/\//, 3) # Note: we ignore path if present. + def request_to_checksum_and_path( request ) + checksum_type, checksum, path = request.key.split(/\//, 3) + if path == '' # Treat "md5/<checksum>/" like "md5/<checksum>" + path = nil + end raise "Unsupported checksum type #{checksum_type.inspect}" if checksum_type != 'md5' raise "Invalid checksum #{checksum.inspect}" if checksum !~ /^[0-9a-f]{32}$/ - checksum + [checksum, path] end def path_for(bucket_path, digest, subfile = nil) diff --git a/spec/unit/indirector/file_bucket_file/file_spec.rb b/spec/unit/indirector/file_bucket_file/file_spec.rb index 9187f4d..0c33593 100755 --- a/spec/unit/indirector/file_bucket_file/file_spec.rb +++ b/spec/unit/indirector/file_bucket_file/file_spec.rb @@ -22,13 +22,97 @@ describe Puppet::FileBucketFile::File do Puppet[:bucketdir] = tmpdir('bucketdir') end - describe "when diffing files" do - def save_bucket_file(contents) - bucket_file = Puppet::FileBucket::File.new(contents) - bucket_file.save - bucket_file.checksum_data + def save_bucket_file(contents, path = "/who_cares") + bucket_file = Puppet::FileBucket::File.new(contents) + bucket_file.save("md5/#{Digest::MD5.hexdigest(contents)}#{path}") + bucket_file.checksum_data + end + + describe "when servicing a save request" do + describe "when supplying a path" do + it "should store the path if not already stored" do + checksum = save_bucket_file("stuff", "/foo/bar") + dir_path = "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a" + File.read("#{dir_path}/contents").should == "stuff" + File.read("#{dir_path}/paths").should == "foo/bar\n" + end + + it "should leave the paths file alone if the path is already stored" do + checksum = save_bucket_file("stuff", "/foo/bar") + checksum = save_bucket_file("stuff", "/foo/bar") + dir_path = "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a" + File.read("#{dir_path}/contents").should == "stuff" + File.read("#{dir_path}/paths").should == "foo/bar\n" + end + + it "should store an additional path if the new path differs from those already stored" do + checksum = save_bucket_file("stuff", "/foo/bar") + checksum = save_bucket_file("stuff", "/foo/baz") + dir_path = "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a" + File.read("#{dir_path}/contents").should == "stuff" + File.read("#{dir_path}/paths").should == "foo/bar\nfoo/baz\n" + end + end + + describe "when not supplying a path" do + it "should save the file and create an empty paths file" do + checksum = save_bucket_file("stuff", "") + dir_path = "#{Puppet[:bucketdir]}/c/1/3/d/8/8/c/b/c13d88cb4cb02003daedb8a84e5d272a" + File.read("#{dir_path}/contents").should == "stuff" + File.read("#{dir_path}/paths").should == "" + end + end + end + + describe "when servicing a head/find request" do + describe "when supplying a path" do + it "should return false/nil if the file isn't bucketed" do + Puppet::FileBucket::File.head("md5/0ae2ec1980410229885fe72f7b44fe55/foo/bar").should == false + Puppet::FileBucket::File.find("md5/0ae2ec1980410229885fe72f7b44fe55/foo/bar").should == nil + end + + it "should return false/nil if the file is bucketed but with a different path" do + checksum = save_bucket_file("I'm the contents of a file", '/foo/bar') + Puppet::FileBucket::File.head("md5/#{checksum}/foo/baz").should == false + Puppet::FileBucket::File.find("md5/#{checksum}/foo/baz").should == nil + end + + it "should return true/file if the file is already bucketed with the given path" do + contents = "I'm the contents of a file" + checksum = save_bucket_file(contents, '/foo/bar') + Puppet::FileBucket::File.head("md5/#{checksum}/foo/bar").should == true + find_result = Puppet::FileBucket::File.find("md5/#{checksum}/foo/bar") + find_result.should be_a(Puppet::FileBucket::File) + find_result.checksum.should == "{md5}#{checksum}" + find_result.to_s.should == contents + end + end + + describe "when not supplying a path" do + [false, true].each do |trailing_slash| + describe "#{trailing_slash ? 'with' : 'without'} a trailing slash" do + trailing_string = trailing_slash ? '/' : '' + + it "should return false/nil if the file isn't bucketed" do + Puppet::FileBucket::File.head("md5/0ae2ec1980410229885fe72f7b44fe55#{trailing_string}").should == false + Puppet::FileBucket::File.find("md5/0ae2ec1980410229885fe72f7b44fe55#{trailing_string}").should == nil + end + + it "should return true/file if the file is already bucketed" do + contents = "I'm the contents of a file" + checksum = save_bucket_file(contents, '/foo/bar') + Puppet::FileBucket::File.head("md5/#{checksum}#{trailing_string}").should == true + find_result = Puppet::FileBucket::File.find("md5/#{checksum}#{trailing_string}") + find_result.should be_a(Puppet::FileBucket::File) + find_result.checksum.should == "{md5}#{checksum}" + find_result.to_s.should == contents + end + end + end end + end + describe "when diffing files" do it "should generate an empty string if there is no diff" do checksum = save_bucket_file("I'm the contents of a file") Puppet::FileBucket::File.find("md5/#{checksum}", :diff_with => checksum).should == '' @@ -102,7 +186,7 @@ HERE key = "md5/#{@digest}" if supply_path - key += "//path/to/file" + key += "/path/to/file" end @request = Puppet::Indirector::Request.new(:indirection_name, :find, key, request_options) @@ -116,10 +200,15 @@ HERE it "should return an instance of Puppet::FileBucket::File created with the content if the file exists" do make_bucketed_file - bucketfile = @store.find(@request) - bucketfile.should be_a(Puppet::FileBucket::File) - bucketfile.contents.should == @contents - @store.head(@request).should == true + if supply_path + @store.find(@request).should == nil + @store.head(@request).should == false # because path didn't match + else + bucketfile = @store.find(@request) + bucketfile.should be_a(Puppet::FileBucket::File) + bucketfile.contents.should == @contents + @store.head(@request).should == true + end end it "should return nil if no file is found" do -- 1.7.2 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
