diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index 77296d8a9..c9c113056 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -408,6 +408,10 @@ module Vagrant error_key(:machine_guest_not_ready) end + class MachineLocked < VagrantError + error_key(:machine_locked) + end + class MachineNotFound < VagrantError error_key(:machine_not_found) end diff --git a/lib/vagrant/machine_index.rb b/lib/vagrant/machine_index.rb index 636318484..3ce35867a 100644 --- a/lib/vagrant/machine_index.rb +++ b/lib/vagrant/machine_index.rb @@ -1,6 +1,7 @@ require "json" require "pathname" require "securerandom" +require "thread" module Vagrant # MachineIndex is able to manage the index of created Vagrant environments @@ -35,44 +36,81 @@ module Vagrant class MachineIndex # Initializes a MachineIndex at the given file location. # - # @param [Pathname] data_file Path to the file that should be used - # to maintain the machine index. This file doesn't have to exist - # but this location must be writable. - def initialize(data_file) - @data_file = data_file + # @param [Pathname] data_dir Path to the directory where data for the + # index can be stored. This folder should exist and must be writable. + def initialize(data_dir) + @data_dir = data_dir + @index_file = data_dir.join("index") + @lock = Mutex.new @machines = {} + @machine_locks = {} - if @data_file.file? - data = nil - begin - data = JSON.load(@data_file.read) - rescue JSON::ParserError - raise Errors::CorruptMachineIndex, path: data_file.to_s - end - - if data - if !data["version"] || data["version"].to_i != 1 - raise Errors::CorruptMachineIndex, path: data_file.to_s - end - - @machines = data["machines"] || {} - end + with_index_lock do + unlocked_reload end end # Accesses a machine by UUID and returns a {MachineIndex::Entry} # + # The entry returned is locked and can't be read again or updated by + # this process or any other. To unlock the machine, call {#release} + # with the entry. + # + # You can only {#set} an entry (update) when the lock is held. + # # @param [String] uuid UUID for the machine to access. # @return [MachineIndex::Entry] - def [](uuid) - return nil if !@machines[uuid] - Entry.new(uuid, @machines[uuid].merge("id" => uuid)) + def get(uuid) + entry = nil + + @lock.synchronize do + with_index_lock do + return nil if !@machines[uuid] + + entry = Entry.new(uuid, @machines[uuid].merge("id" => uuid)) + + # Lock this machine + lock_file = lock_machine(uuid) + if !lock_file + raise Errors::MachineLocked, + name: entry.name, + provider: entry.provider + end + + @machine_locks[uuid] = lock_file + end + end + + entry + end + + # Releases an entry, unlocking it. + # + # This is an idempotent operation. It is safe to call this even if you're + # unsure if an entry is locked or not. + # + # After calling this, the previous entry should no longer be used. + # + # @param [Entry] entry + def release(entry) + @lock.synchronize do + lock_file = @machine_locks[entry.id] + if lock_file + lock_file.close + @machine_locks.delete(entry.id) + end + end end # Creates/updates an entry object and returns the resulting entry. # # If the entry was new (no UUID), then the UUID will be set on the - # resulting entry and can be used. + # resulting entry and can be used. Additionally, the a lock will + # be created for the resulting entry, so you must {#release} it + # if you want others to be able to access it. + # + # If the entry isn't new (has a UUID). then this process must hold + # that entry's lock or else this set will fail. # # @param [Entry] entry # @return [Entry] @@ -82,21 +120,82 @@ module Vagrant # Set an ID if there isn't one already set id = entry.id - id ||= SecureRandom.uuid - # Store the data - @machines[id] = struct - save + @lock.synchronize do + # Verify the machine is locked so we can safely write + # to it. + if !id + id = SecureRandom.uuid + lock_file = lock_machine(id) + if !lock_file + raise "Failed to lock new machine: #{entry.name}" + end + + @machine_locks[id] = lock_file + end + + if !@machine_locks[id] + raise "Unlocked write on machine: #{id}" + end + + with_index_lock do + # Reload so we have the latest machine data, then update + # this particular machine, then write. This allows other processes + # to update their own machines without conflicting with our own. + unlocked_reload + @machines[id] = struct + unlocked_save + end + end Entry.new(id, struct) end - # Saves the index. + protected + + # Locks a machine exclusively to us, returning the file handle + # that holds the lock. # - # This doesn't usually need to be called because {#set} will - # automatically save as well. - def save - @data_file.open("w") do |f| + # If the lock cannot be acquired, then nil is returned. + # + # @return [File] + def lock_machine(uuid) + lock_path = @data_dir.join("#{uuid}.lock") + lock_file = lock_path.open("w+") + if lock_file.flock(File::LOCK_EX | File::LOCK_NB) === false + lock_file.close + lock_file = nil + end + + lock_file + end + + # This will reload the data without locking the index. It is assumed + # the caller with lock the index outside of this call. + # + # @param [File] f + def unlocked_reload + return if !@index_file.file? + + data = nil + begin + data = JSON.load(@index_file.read) + rescue JSON::ParserError + raise Errors::CorruptMachineIndex, path: @index_file.to_s + end + + if data + if !data["version"] || data["version"].to_i != 1 + raise Errors::CorruptMachineIndex, path: @index_file.to_s + end + + @machines = data["machines"] || {} + end + end + + # Saves the index. + def unlocked_save + @index_file.open("w") do |f| f.write(JSON.dump({ "version" => 1, "machines" => @machines, @@ -104,6 +203,16 @@ module Vagrant end end + + # This will hold a lock to the index so it can be read or updated. + def with_index_lock + lock_path = "#{@index_file}.lock" + File.open(lock_path, "w+") do |f| + f.flock(File::LOCK_EX) + yield + end + end + # An entry in the MachineIndex. class Entry # The unique ID for this entry. This is _not_ the ID for the diff --git a/templates/locales/en.yml b/templates/locales/en.yml index b37e29214..a3aaa4bcf 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -729,6 +729,14 @@ en: Guest-specific operations were attempted on a machine that is not ready for guest communication. This should not happen and a bug should be reported. + machine_locked: |- + Vagrant can't use the requested machine because it is locekd! This + means that another Vagrant process is currently reading or modifying + the machine. Please wait for that Vagrant process to end and try + again. Details about the machine are shown below: + + Name: %{name} + Provider: %{provider} machine_not_found: |- The machine with the name '%{name}' was not found configured for this Vagrant environment. diff --git a/test/unit/vagrant/machine_directory_test.rb b/test/unit/vagrant/machine_directory_test.rb index f401985be..d340e9a65 100644 --- a/test/unit/vagrant/machine_directory_test.rb +++ b/test/unit/vagrant/machine_directory_test.rb @@ -9,12 +9,12 @@ require "vagrant/machine_index" describe Vagrant::MachineIndex do include_context "unit" - let(:data_file) { temporary_file } + let(:data_dir) { temporary_dir } - subject { described_class.new(data_file) } + subject { described_class.new(data_dir) } it "raises an exception if the data file is corrupt" do - data_file.open("w") do |f| + data_dir.join("index").open("w") do |f| f.write(JSON.dump({})) end @@ -23,7 +23,7 @@ describe Vagrant::MachineIndex do end it "raises an exception if the JSON is invalid" do - data_file.open("w") do |f| + data_dir.join("index").open("w") do |f| f.write("foo") end @@ -31,7 +31,7 @@ describe Vagrant::MachineIndex do to raise_error(Vagrant::Errors::CorruptMachineIndex) end - describe "#[]" do + describe "#get and #release" do before do data = { "version" => 1, @@ -46,17 +46,17 @@ describe Vagrant::MachineIndex do } } - data_file.open("w") do |f| + data_dir.join("index").open("w") do |f| f.write(JSON.dump(data)) end end it "returns nil if the machine doesn't exist" do - expect(subject["foo"]).to be_nil + expect(subject.get("foo")).to be_nil end it "returns a valid entry if the machine exists" do - result = subject["bar"] + result = subject.get("bar") expect(result.id).to eq("bar") expect(result.name).to eq("default") @@ -65,22 +65,47 @@ describe Vagrant::MachineIndex do expect(result.state).to eq("running") expect(result.updated_at).to eq("foo") end + + it "locks the entry so subsequent gets fail" do + result = subject.get("bar") + expect(result).to_not be_nil + + expect { subject.get("bar") }. + to raise_error(Vagrant::Errors::MachineLocked) + end + + it "can unlock a machine" do + result = subject.get("bar") + expect(result).to_not be_nil + subject.release(result) + + result = subject.get("bar") + expect(result).to_not be_nil + end end - describe "#set and #[]" do + describe "#set and #get" do let(:entry_klass) { Vagrant::MachineIndex::Entry } - it "adds a new entry" do - entry = entry_klass.new - entry.name = "foo" - entry.vagrantfile_path = "/bar" + let(:new_entry) do + entry_klass.new.tap do |e| + e.name = "foo" + e.vagrantfile_path = "/bar" + end + end - result = subject.set(entry) + it "adds a new entry" do + result = subject.set(new_entry) expect(result.id).to_not be_empty + # It should be locked + expect { subject.get(result.id) }. + to raise_error(Vagrant::Errors::MachineLocked) + # Get it froma new class and check the results - subject = described_class.new(data_file) - entry = subject[result.id] + subject.release(result) + subject = described_class.new(data_dir) + entry = subject.get(result.id) expect(entry).to_not be_nil expect(entry.name).to eq("foo") @@ -100,9 +125,12 @@ describe Vagrant::MachineIndex do nextresult = subject.set(result) expect(nextresult.id).to eq(result.id) + # Release it so we can test the contents + subject.release(nextresult) + # Get it froma new class and check the results - subject = described_class.new(data_file) - entry = subject[result.id] + subject = described_class.new(data_dir) + entry = subject.get(result.id) expect(entry).to_not be_nil expect(entry.name).to eq("bar") end