From a706637594c2b4e95f3c1f65d31d56e23c4390ea Mon Sep 17 00:00:00 2001 From: Vincent Untz Date: Tue, 2 Feb 2016 11:11:34 +0100 Subject: [PATCH 1/5] crowbar: Use temporary directory when exporting chef data It's really horrible to put the files in the db directory, so don't do that. Also properly clean up files at the end. --- .../app/controllers/support_controller.rb | 33 +++++++++++-------- crowbar_framework/app/models/chef_object.rb | 10 +++--- crowbar_framework/app/models/proposal.rb | 4 +-- .../controllers/support_controller_spec.rb | 9 +++-- 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/crowbar_framework/app/controllers/support_controller.rb b/crowbar_framework/app/controllers/support_controller.rb index 77304b9ed2..30817e3c8c 100644 --- a/crowbar_framework/app/controllers/support_controller.rb +++ b/crowbar_framework/app/controllers/support_controller.rb @@ -103,22 +103,29 @@ def export_supportconfig def export_chef begin - Rails.root.join("db").children.each do |file| - file.unlink if file.extname == ".json" - end - - NodeObject.all.each { |n| n.export } - RoleObject.all.each { |r| r.export } - Proposal.all.each { |p| p.export } - filename = "crowbar-chef-#{Time.now.strftime("%Y%m%d-%H%M%S")}.tgz" - pid = Process.fork do - exports = Dir.glob(Rails.root.join("db", "*.json").to_s) - cmd = ["tar", "-czf", Rails.root.join("tmp", filename).to_s, *exports] + tmpdir = Dir.mktmpdir + tmpdirpath = Pathname.new(tmpdir) + tmpfile_path = tmpdirpath.join(filename) + + begin + NodeObject.all.each { |n| n.export(tmpdirpath) } + RoleObject.all.each { |r| r.export(tmpdirpath) } + Proposal.all.each { |p| p.export(tmpdirpath) } + rescue StandardError => e + FileUtils.remove_entry_secure tmpdir + raise e + end - ok = system(*cmd) - File.rename(Rails.root.join("tmp", filename), export_dir.join(filename)) if ok + pid = Process.fork do + begin + cmd = ["tar", "czf", tmpfile_path.to_s, "*"] + _stdout, ok = Open3.capture2(*cmd, chdir: tmpdir) + File.rename(tmpfile_path, export_dir.join(filename)) if ok + ensure + FileUtils.remove_entry tmpdir + end end Process.detach(pid) diff --git a/crowbar_framework/app/models/chef_object.rb b/crowbar_framework/app/models/chef_object.rb index ff240dceb9..50fff48359 100644 --- a/crowbar_framework/app/models/chef_object.rb +++ b/crowbar_framework/app/models/chef_object.rb @@ -51,17 +51,17 @@ def self.chef_escape(str) str.gsub("-:") { |c| '\\' + c } end - # FIXME: the second argument was added so that the Proposal model + # FIXME: the third argument was added so that the Proposal model # can be exported in a same way as the ProposalObject. When the replacement # is completed, remove it and implement the export in the Proposal. # Also check the logging barclamp that it did not break. - def self.export(obj, name = nil) + def self.export(obj, dirpath, name = nil) name ||= obj.respond_to?(:name) ? obj.name : "unknown" - file = Rails.root.join("db", "#{obj.chef_type}_#{name}.json") + file = dirpath.join("#{obj.chef_type}_#{name}.json") File.open(file, "w") { |f| f.write(obj.to_json) } end - def export(name = nil) - self.class.export(self, name) + def export(dirpath, name = nil) + self.class.export(self, dirpath, name) end end diff --git a/crowbar_framework/app/models/proposal.rb b/crowbar_framework/app/models/proposal.rb index b592ce9633..4e1e429c40 100644 --- a/crowbar_framework/app/models/proposal.rb +++ b/crowbar_framework/app/models/proposal.rb @@ -40,8 +40,8 @@ def key end end - def export - ChefObject.export(self) + def export(dirpath) + ChefObject.export(self, dirpath) end # FIXME: this is not correct, the item of ProposalObject returns diff --git a/crowbar_framework/spec/controllers/support_controller_spec.rb b/crowbar_framework/spec/controllers/support_controller_spec.rb index 895322bb70..73b0f17ce4 100644 --- a/crowbar_framework/spec/controllers/support_controller_spec.rb +++ b/crowbar_framework/spec/controllers/support_controller_spec.rb @@ -52,19 +52,22 @@ it "exports known data into db dir" do begin now = Time.now + tmpdir = Rails.root.join("tmp", "chef-export") + tmpdir.mkpath + allow(Time).to receive(:now).and_return(now) allow(Process).to receive(:fork).and_return(0) + allow(Dir).to receive(:mktmpdir).and_return(tmpdir.to_s) filename = "crowbar-chef-#{now.strftime("%Y%m%d-%H%M%S")}.tgz" - export = Rails.root.join("db", filename) get :export_chef expect(flash[:alert]).to be_nil expect(response).to redirect_to(utils_url(waiting: true, file: filename)) - expect(Dir.glob(Rails.root.join("db", "*.json")).count).to_not be_zero + expect(Dir.glob(tmpdir.join("*.json")).count).to_not be_zero ensure - Dir.glob(Rails.root.join("db", "*.json")).each { |json| FileUtils.rm(json) } + tmpdir.rmtree end end end From 1eebb64a496fbdc5e0c70fffc7c1a64d1bead3df Mon Sep 17 00:00:00 2001 From: Vincent Untz Date: Tue, 9 Feb 2016 00:52:17 +0100 Subject: [PATCH 2/5] crowbar: Use temporary directory when exporting supportconfig While not strictly needed, it's slightly better in order to avoid non-crowbar owned files in the rails app tmp directory. We use a chown at the end, of course, but if the rails app is stopped in the middle, then the tmp directory contains files that can't be removed. Using a proper temporary directory helps (since it may be cleaned up on reboot, or by scripts). --- .../app/controllers/support_controller.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/crowbar_framework/app/controllers/support_controller.rb b/crowbar_framework/app/controllers/support_controller.rb index 30817e3c8c..cf993fed2a 100644 --- a/crowbar_framework/app/controllers/support_controller.rb +++ b/crowbar_framework/app/controllers/support_controller.rb @@ -70,25 +70,24 @@ def destroy def export_supportconfig begin - base = "supportconfig-#{Time.now.strftime("%Y%m%d-%H%M%S")}" - filename = "#{base}.tbz" + filename = "supportconfig-#{Time.now.strftime("%Y%m%d-%H%M%S")}.tbz" pid = Process.fork do begin - tmp = Rails.root.join("tmp", base).to_s + tmpdir = Dir.mktmpdir - supportconfig = ["sudo", "-i", "supportconfig", "-Q", "-R", tmp] - chown = ["sudo", "-i", "chown", "-R", "#{Process.uid}:#{Process.gid}", tmp] + supportconfig = ["sudo", "-i", "supportconfig", "-Q", "-R", tmpdir] + chown = ["sudo", "-i", "chown", "-R", "#{Process.uid}:#{Process.gid}", tmpdir] ok = system(*supportconfig) ok &= system(*chown) - tarball = Dir.glob("#{tmp}/*.tbz").first + tarball = Dir.glob("#{tmpdir}/*.tbz").first File.rename tarball, export_dir.join(filename) if tarball && ok rescue => e Rails.logger.warn(e.message) ensure - FileUtils.rm_rf(tmp) + FileUtils.remove_entry_secure tmpdir end end From 38712d1ad2bd256247f994193cbc6cd8d3549d95 Mon Sep 17 00:00:00 2001 From: Vincent Untz Date: Tue, 9 Feb 2016 01:02:57 +0100 Subject: [PATCH 3/5] crowbar: Remove non-implemented /utils/{import,upload} routes They were probably used a long time ago... --- crowbar_framework/config/routes.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/crowbar_framework/config/routes.rb b/crowbar_framework/config/routes.rb index 690af704e6..84cffb82cc 100644 --- a/crowbar_framework/config/routes.rb +++ b/crowbar_framework/config/routes.rb @@ -67,8 +67,6 @@ get "utils/supportconfig(.:format)", controller: "support", action: "export_supportconfig", as: "export_supportconfig" get "utils/:controller/1.0/export(.:format)", action: "export", as: "utils_export" get "utils/:controller/1.0(.:format)", action: "utils", as: "utils_barclamp" - get "utils/import/:id(.:format)", controller: "support", action: "import", constraints: { id: /[^\/]+/ }, as: "utils_import" - get "utils/upload/:id(.:format)", controller: "support", action: "upload", constraints: { id: /[^\/]+/ }, as: "utils_upload" get "utils/repositories(.:format)", controller: "repositories", action: "index", as: "repositories" post "utils/repositories/sync(.:format)", controller: "repositories", action: "sync", as: "sync_repositories" post "utils/repositories/activate(.:format)", controller: "repositories", action: "activate", as: "activate_repository" From f4efe8f0bb4c2ec750f54c90ddc1df795a5bd95d Mon Sep 17 00:00:00 2001 From: Vincent Untz Date: Tue, 9 Feb 2016 01:07:00 +0100 Subject: [PATCH 4/5] crowbar: Use proper delete methods for deleting exported files --- crowbar_framework/app/views/support/index.html.haml | 2 +- crowbar_framework/config/routes.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crowbar_framework/app/views/support/index.html.haml b/crowbar_framework/app/views/support/index.html.haml index b9c00d3e13..e5e545b592 100644 --- a/crowbar_framework/app/views/support/index.html.haml +++ b/crowbar_framework/app/views/support/index.html.haml @@ -41,7 +41,7 @@ - else = link_to file, "/export/#{file}" - = link_to icon_tag("trash"), utils_files_path(:id => file), :title => t(".delete_hint"), :class => "pull-right" + = link_to icon_tag("trash"), utils_files_path(:id => file), :title => t(".delete_hint"), :class => "pull-right", :method => :delete - else .alert.alert-info = t(".none_exported") diff --git a/crowbar_framework/config/routes.rb b/crowbar_framework/config/routes.rb index 84cffb82cc..06906741d4 100644 --- a/crowbar_framework/config/routes.rb +++ b/crowbar_framework/config/routes.rb @@ -62,7 +62,7 @@ #support paths get "utils(.:format)", controller: "support", action: "index", as: "utils" - get "utils/files/:id(.:format)", controller: "support", action: "destroy", constraints: { id: /[^\/]+/ }, as: "utils_files" + delete "utils/files/:id(.:format)", controller: "support", action: "destroy", constraints: { id: /[^\/]+/ }, as: "utils_files" get "utils/chef(.:format)", controller: "support", action: "export_chef", as: "export_chef" get "utils/supportconfig(.:format)", controller: "support", action: "export_supportconfig", as: "export_supportconfig" get "utils/:controller/1.0/export(.:format)", action: "export", as: "utils_export" From 7b0cb79f71ca050cb3b203f04cc8dcb006fbe5ec Mon Sep 17 00:00:00 2001 From: Vincent Untz Date: Tue, 9 Feb 2016 01:08:01 +0100 Subject: [PATCH 5/5] crowbar: Use Exported Files instead of Exported Items in navigation This is the same as the title of the page. --- crowbar_framework/config/locales/crowbar/en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crowbar_framework/config/locales/crowbar/en.yml b/crowbar_framework/config/locales/crowbar/en.yml index 88f4a0f5b6..39c11577bc 100644 --- a/crowbar_framework/config/locales/crowbar/en.yml +++ b/crowbar_framework/config/locales/crowbar/en.yml @@ -81,7 +81,7 @@ en: utils: title: 'Utilities' queue: 'Deployment Queue' - logs: 'Exported Items' + logs: 'Exported Files' repositories: 'Repositories' backup: 'Backup & Restore' help: 'Help'