Merge pull request #3538 from mitchellh/f-box-usage

Track box usage (local) to know if envs are using boxes

lib/vagrant/action/builtin/box_remove.rb

@@ -71,6 +71,49 @@ module Vagrant
           box = env[:box_collection].find(
             box_name, box_provider, box_version)
 
+          # Verify that this box is not in use by an active machine,
+          # otherwise warn the user.
+          users = []
+          env[:machine_index].each do |entry|
+            box_data = entry.extra_data["box"]
+            next if !box_data
+
+            # If all the data matches AND the entry is a seemingly
+            # valid entry, then track it.
+            if box_data["name"] == box.name &&
+              box_data["provider"] == box.provider.to_s &&
+              box_data["version"] == box.version.to_s &&
+              entry.valid?(env[:home_path])
+              users << entry
+            end
+          end
+
+          if !users.empty?
+            # Build up the output to show the user.
+            users = users.map do |entry|
+              "#{entry.name} (ID: #{entry.id})"
+            end.join("\n")
+
+            force_key = :force_confirm_box_remove
+            message   = I18n.t(
+              "vagrant.commands.box.remove_in_use_query",
+              name: box.name,
+              provider: box.provider,
+              version: box.version,
+              users: users) + " "
+
+            # Ask the user if we should do this
+            stack = Builder.new.tap do |b|
+              b.use Confirm, message, force_key
+            end
+
+            result = env[:action_runner].run(stack, env)
+            if !result[:result]
+              # They said "no", so just return
+              return @app.call(env)
+            end
+          end
+
           env[:ui].info(I18n.t("vagrant.commands.box.removing",
                               :name => box.name,
                               :provider => box.provider,

lib/vagrant/environment.rb

@@ -144,7 +144,7 @@ module Vagrant
       hook(:environment_plugins_loaded, runner: Action::Runner.new(env: self))
 
       # Call the environment load hooks
-      hook(:environment_load)
+      hook(:environment_load, runner: Action::Runner.new(env: self))
     end
 
     # Return a human-friendly string for pretty printed or inspected
@@ -165,6 +165,7 @@ module Vagrant
           :box_collection => boxes,
           :hook           => method(:hook),
           :host           => host,
+          :machine_index  => machine_index,
           :gems_path      => gems_path,
           :home_path      => home_path,
           :root_path      => root_path,

lib/vagrant/machine.rb

@@ -260,6 +260,15 @@ module Vagrant
           entry.state = "preparing"
           entry.vagrantfile_path = @env.root_path
           entry.vagrantfile_name = @env.vagrantfile_name
+
+          if @box
+            entry.extra_data["box"] = {
+              "name"     => @box.name,
+              "provider" => @box.provider.to_s,
+              "version"  => @box.version.to_s,
+            }
+          end
+
           entry = @env.machine_index.set(entry)
           @env.machine_index.release(entry)
 

lib/vagrant/machine_index.rb

@@ -378,6 +378,8 @@ module Vagrant
       # The parameter given should be nil if this is being created
       # publicly.
       def initialize(id=nil, raw=nil)
+        @extra_data = {}
+
         # Do nothing if we aren't given a raw value. Otherwise, parse it.
         return if !raw
 
@@ -401,6 +403,33 @@ module Vagrant
         @vagrantfile_path = Pathname.new(@vagrantfile_path) if @vagrantfile_path
       end
 
+      # Returns boolean true if this entry appears to be valid.
+      # The critera for being valid:
+      #
+      #   * Vagrantfile directory exists
+      #   * Vagrant environment contains a machine with this
+      #     name and provider.
+      #
+      # This method is _slow_. It should be used with care.
+      #
+      # @param [Pathname] home_path The home path for the Vagrant
+      #   environment.
+      # @return [Boolean]
+      def valid?(home_path)
+        return false if !vagrantfile_path
+        return false if !vagrantfile_path.directory?
+
+        # Create an environment so we can determine the active
+        # machines...
+        env = vagrant_env(home_path)
+        env.active_machines.each do |name, provider|
+          return true if name.to_s == self.name.to_s &&
+            provider.to_s == self.provider.to_s
+        end
+
+        false
+      end
+
       # Creates a {Vagrant::Environment} for this entry.
       #
       # @return [Vagrant::Environment]

plugins/commands/box/command/remove.rb

@@ -6,12 +6,18 @@ module VagrantPlugins
       class Remove < Vagrant.plugin("2", :command)
         def execute
           options = {}
+          options[:force] = false
+
           opts = OptionParser.new do |o|
             o.banner = "Usage: vagrant box remove <name>"
             o.separator ""
             o.separator "Options:"
             o.separator ""
 
+            o.on("-f", "--force", "Destroy without confirmation.") do |f|
+              options[:force] = f
+            end
+
             o.on("--provider PROVIDER", String,
                  "The specific provider type for the box to remove") do |p|
               options[:provider] = p
@@ -43,6 +49,7 @@ module VagrantPlugins
             :box_name     => argv[0],
             :box_provider => options[:provider],
             :box_version  => options[:version],
+            :force_confirm_box_remove => options[:force],
           })
 
           # Success, exit status 0

plugins/commands/global-status/command.rb

@@ -42,7 +42,7 @@ module VagrantPlugins
         @env.machine_index.each do |entry|
           # If we're pruning and this entry is invalid, skip it
           # and prune it later.
-          if options[:prune] && invalid?(entry)
+          if options[:prune] && !entry.valid?(@env.home_path)
             prune << entry
             next
           end
@@ -95,23 +95,6 @@ module VagrantPlugins
         # Success, exit status 0
         0
       end
-
-      protected
-
-      # Tests if a entry is invalid and should be pruned
-      def invalid?(entry)
-        return true if !entry.vagrantfile_path.directory?
-
-        # Create an environment so we can determine the active
-        # machines...
-        env = entry.vagrant_env(@env.home_path)
-        env.active_machines.each do |name, provider|
-          return false if name.to_s == entry.name.to_s &&
-            provider.to_s == entry.provider.to_s
-        end
-
-        true
-      end
     end
   end
 end

templates/locales/en.yml

@@ -1251,6 +1251,15 @@ en:
         vm_not_running: "VM is not currently running. Please, first bring it up with `vagrant up` then run this command."
       box:
         no_installed_boxes: "There are no installed boxes! Use `vagrant box add` to add some."
+        remove_in_use_query: |-
+          Box '%{name}' (v%{version}) with provider '%{provider}' appears
+          to still be in use by at least one Vagrant environment. Removing
+          the box could corrupt the environment. We recommend destroying
+          these environments first:
+
+          %{users}
+
+          Are you sure you want to remove this box? [y/N]
         removing: |-
           Removing box '%{name}' (v%{version}) with provider '%{provider}'...
       destroy:

test/unit/plugins/commands/box/command/remove_test.rb

@@ -34,11 +34,26 @@ describe VagrantPlugins::CommandBox::Command::Remove do
     it "invokes the action runner" do
       expect(action_runner).to receive(:run).with { |action, opts|
         expect(opts[:box_name]).to eq("foo")
+        expect(opts[:force_confirm_box_remove]).to be_false
         true
       }
 
       subject.execute
     end
+
+    context "with --force" do
+      let(:argv) { super() + ["--force"] }
+
+      it "invokes the action runner with force option" do
+        expect(action_runner).to receive(:run).with { |action, opts|
+          expect(opts[:box_name]).to eq("foo")
+          expect(opts[:force_confirm_box_remove]).to be_true
+          true
+        }
+
+        subject.execute
+      end
+    end
   end
 
   context "with two arguments" do

test/unit/vagrant/action/builtin/box_remove_test.rb

@@ -6,12 +6,16 @@ describe Vagrant::Action::Builtin::BoxRemove do
   let(:app) { lambda { |env| } }
   let(:env) { {
     box_collection: box_collection,
+    home_path: home_path,
+    machine_index: machine_index,
     ui: Vagrant::UI::Silent.new,
   } }
 
   subject { described_class.new(app, env) }
 
   let(:box_collection) { double("box_collection") }
+  let(:home_path) { "foo" }
+  let(:machine_index) { [] }
   let(:iso_env) { isolated_environment }
 
   let(:box) do
@@ -74,6 +78,71 @@ describe Vagrant::Action::Builtin::BoxRemove do
     expect(env[:box_removed]).to equal(box)
   end
 
+  context "checking if a box is in use" do
+    def new_entry(name, provider, version, valid=true)
+      Vagrant::MachineIndex::Entry.new.tap do |entry|
+        entry.extra_data["box"] = {
+          "name" => "foo",
+          "provider" => "virtualbox",
+          "version" => "1.0",
+        }
+
+        entry.stub(valid?: valid)
+      end
+    end
+
+    let(:action_runner) { double("action_runner") }
+
+    before do
+      env[:action_runner] = action_runner
+
+      box_collection.stub(
+        all: [
+          ["foo", "1.0", :virtualbox],
+          ["foo", "1.1", :virtualbox],
+        ])
+
+      env[:box_name] = "foo"
+      env[:box_version] = "1.0"
+    end
+
+    it "does delete if the box is not in use" do
+      expect(box_collection).to receive(:find).with(
+        "foo", :virtualbox, "1.0").and_return(box)
+      expect(box).to receive(:destroy!).once
+
+      subject.call(env)
+    end
+
+    it "does delete if the box is in use and user confirms" do
+      machine_index << new_entry("foo", "virtualbox", "1.0")
+
+      result = { result: true }
+      expect(action_runner).to receive(:run).
+        with(anything, env).and_return(result)
+
+      expect(box_collection).to receive(:find).with(
+        "foo", :virtualbox, "1.0").and_return(box)
+      expect(box).to receive(:destroy!).once
+
+      subject.call(env)
+    end
+
+    it "doesn't delete if the box is in use" do
+      machine_index << new_entry("foo", "virtualbox", "1.0")
+
+      result = { result: false }
+      expect(action_runner).to receive(:run).
+        with(anything, env).and_return(result)
+
+      expect(box_collection).to receive(:find).with(
+        "foo", :virtualbox, "1.0").and_return(box)
+      expect(box).to receive(:destroy!).never
+
+      subject.call(env)
+    end
+  end
+
   it "errors if the box doesn't exist" do
     box_collection.stub(all: [])
 

test/unit/vagrant/machine_index_test.rb

@@ -265,3 +265,74 @@ describe Vagrant::MachineIndex do
     end
   end
 end
+
+describe Vagrant::MachineIndex::Entry do
+  include_context "unit"
+
+  let(:env) {
+    iso_env = isolated_environment
+    iso_env.vagrantfile(vagrantfile)
+    iso_env.create_vagrant_env
+  }
+
+  let(:vagrantfile) { "" }
+
+  describe "#valid?" do
+    let(:machine) { env.machine(:default, :dummy) }
+
+    subject do
+      described_class.new.tap do |e|
+        e.name = "default"
+        e.provider = "dummy"
+        e.vagrantfile_path = env.root_path
+      end
+    end
+
+    it "should be valid with a valid entry" do
+      machine.id = "foo"
+      expect(subject).to be_valid(env.home_path)
+    end
+
+    it "should be invalid if no Vagrantfile path is set" do
+      subject.vagrantfile_path = nil
+      expect(subject).to_not be_valid(env.home_path)
+    end
+
+    it "should be invalid if the Vagrantfile path does not exist" do
+      subject.vagrantfile_path = Pathname.new("/i/should/not/exist")
+      expect(subject).to_not be_valid(env.home_path)
+    end
+
+    it "should be invalid if the machine is inactive" do
+      machine.id = nil
+      expect(subject).to_not be_valid(env.home_path)
+    end
+
+    context "with another active machine" do
+      let(:vagrantfile) do
+        <<-VF
+        Vagrant.configure("2") do |config|
+          config.vm.define "web"
+          config.vm.define "db"
+        end
+        VF
+      end
+
+      it "should be invalid if the wrong machine is active only" do
+        m = env.machine(:web, :dummy)
+        m.id = "foo"
+
+        subject.name = "db"
+        expect(subject).to_not be_valid(env.home_path)
+      end
+
+      it "should be valid if the correct machine is active" do
+        env.machine(:web, :dummy).id = "foo"
+        env.machine(:db, :dummy).id = "foo"
+
+        subject.name = "db"
+        expect(subject).to be_valid(env.home_path)
+      end
+    end
+  end
+end

test/unit/vagrant/machine_test.rb

@@ -21,7 +21,14 @@ describe Vagrant::Machine do
   let(:provider_name) { :test }
   let(:provider_options) { {} }
   let(:base)     { false }
-  let(:box)      { Object.new }
+  let(:box) do
+    double("box").tap do |b|
+      b.stub(name: "foo")
+      b.stub(provider: :dummy)
+      b.stub(version: "1.0")
+    end
+  end
+
   let(:config)   { env.vagrantfile.config }
   let(:data_dir) { Pathname.new(Dir.mktmpdir("vagrant")) }
   let(:env)      do
@@ -403,11 +410,32 @@ describe Vagrant::Machine do
     end
 
     it "is set one when setting an ID" do
+      # Setup the box information
+      box = double("box")
+      box.stub(name: "foo")
+      box.stub(provider: :bar)
+      box.stub(version: "1.2.3")
+      subject.box = box
+
       subject.id = "foo"
 
       uuid = subject.index_uuid
       expect(uuid).to_not be_nil
       expect(new_instance.index_uuid).to eq(uuid)
+
+      # Test the entry itself
+      entry = env.machine_index.get(uuid)
+      expect(entry.name).to eq(subject.name)
+      expect(entry.provider).to eq(subject.provider_name.to_s)
+      expect(entry.state).to eq("preparing")
+      expect(entry.vagrantfile_path).to eq(env.root_path)
+      expect(entry.vagrantfile_name).to eq(env.vagrantfile_name)
+      expect(entry.extra_data["box"]).to eq({
+        "name"     => box.name,
+        "provider" => box.provider.to_s,
+        "version"  => box.version,
+      })
+      env.machine_index.release(entry)
     end
 
     it "deletes the UUID when setting to nil" do