Working #3139 - scheduling moved to resource harness

We previously had the schedule checking code in Puppet::Type,
but it's more of a transactional function, and in order to
do proper auditing in the transactional area, we need the
cache checking done there.  Scheduling is one
of the few functions that actually uses cached data currently.

Signed-off-by: Luke Kanies <luke@puppetlabs.com>

lib/puppet/transaction.rb

@@ -318,12 +318,11 @@ class Puppet::Transaction
 
     # Is the resource currently scheduled?
     def scheduled?(resource)
-        self.ignoreschedules or resource.scheduled?
+        self.ignoreschedules or resource_harness.scheduled?(resource)
     end
 
     # Should this resource be skipped?
     def skip?(resource)
-        skip = false
         if missing_tags?(resource)
             resource.debug "Not tagged with %s" % tags.join(", ")
         elsif ! scheduled?(resource)
@@ -356,11 +355,10 @@ class Puppet::Transaction
 
     # Is this resource tagged appropriately?
     def missing_tags?(resource)
-        not appropriately_tagged?(resource)
-    end
+        return false if ignore_tags?
+        return false if tags.empty?
 
-    def appropriately_tagged?(resource)
-        self.ignore_tags? or tags.empty? or resource.tagged?(*tags)
+        not resource.tagged?(*tags)
     end
 end
 

lib/puppet/transaction/resource_harness.rb

@@ -73,6 +73,28 @@ class Puppet::Transaction::ResourceHarness
         @transaction = transaction
     end
 
+    def scheduled?(resource)
+        return true if Puppet[:ignoreschedules]
+        return true unless schedule = schedule(resource)
+
+        # We use 'checked' here instead of 'synced' because otherwise we'll
+        # end up checking most resources most times, because they will generally
+        # have been synced a long time ago (e.g., a file only gets updated
+        # once a month on the server and its schedule is daily; the last sync time
+        # will have been a month ago, so we'd end up checking every run).
+        return schedule.match?(resource.cached(:checked).to_i)
+    end
+
+    def schedule(resource)
+        unless resource.catalog
+            resource.warning "Cannot schedule without a schedule-containing catalog"
+            return nil
+        end
+
+        return nil unless name = resource[:schedule]
+        resource.catalog.resource(:schedule, name) || resource.fail("Could not find schedule #{name}")
+    end
+
     private
 
     def absent_and_not_being_created?(current, param)

lib/puppet/type.rb

@@ -1669,45 +1669,6 @@ class Type
         end.flatten.reject { |r| r.nil? }
     end
 
-    ###############################
-    # All of the scheduling code.
-
-    # Look up the schedule and set it appropriately.  This is done after
-    # the instantiation phase, so that the schedule can be anywhere in the
-    # file.
-    def schedule
-         unless catalog
-             warning "Cannot schedule without a schedule-containing catalog"
-             return nil
-         end
-
-        unless defined? @schedule
-            if name = self[:schedule]
-                if sched = catalog.resource(:schedule, name)
-                    @schedule = sched
-                else
-                    self.fail "Could not find schedule %s" % name
-                end
-            else
-                @schedule = nil
-            end
-        end
-        @schedule
-    end
-
-    # Check whether we are scheduled to run right now or not.
-    def scheduled?
-        return true if Puppet[:ignoreschedules]
-        return true unless schedule = self.schedule
-
-        # We use 'checked' here instead of 'synced' because otherwise we'll
-        # end up checking most resources most times, because they will generally
-        # have been synced a long time ago (e.g., a file only gets updated
-        # once a month on the server and its schedule is daily; the last sync time
-        # will have been a month ago, so we'd end up checking every run).
-        return schedule.match?(self.cached(:checked).to_i)
-    end
-
     # Define the initial list of tags.
     def tags=(list)
         tag(self.class.name)
@@ -1895,10 +1856,6 @@ class Type
 
     # Set up all of our autorequires.
     def finish
-        # Scheduling has to be done when the whole config is instantiated, so
-        # that file order doesn't matter in finding them.
-        self.schedule
-
         # Make sure all of our relationships are valid.  Again, must be done
         # when the entire catalog is instantiated.
         self.class.relationship_params.collect do |klass|

spec/integration/transaction.rb

@@ -181,8 +181,6 @@ describe Puppet::Transaction do
         catalog.apply
         FileTest.should be_exists(fname)
 
-        exec.should_not be_scheduled
-
         # Now remove it, so it can get created again
         File.unlink(fname)
 
@@ -234,4 +232,16 @@ describe Puppet::Transaction do
         FileTest.should_not be_exists(file1[:path])
         FileTest.should_not be_exists(file2[:path])
     end
+
+    # #801 -- resources only checked in noop should be rescheduled immediately.
+    it "should immediately reschedule noop resources" do
+        Puppet::Type.type(:schedule).mkdefaultschedules
+        resource = Puppet::Type.type(:notify).new(:name => "mymessage", :noop => true)
+        catalog = Puppet::Resource::Catalog.new
+        catalog.add_resource resource
+
+        trans = catalog.apply
+
+        trans.resource_harness.should be_scheduled(resource)
+    end
 end

spec/unit/transaction.rb

@@ -241,39 +241,84 @@ describe Puppet::Transaction do
 
     describe "when skipping a resource" do
         before :each do
-            @resource = stub_everything 'res'
+            @resource = Puppet::Type.type(:notify).new :name => "foo"
             @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
             @transaction = Puppet::Transaction.new(@catalog)
         end
 
         it "should skip resource with missing tags" do
             @transaction.stubs(:missing_tags?).returns(true)
-            @transaction.skip?(@resource).should be_true
+            @transaction.should be_skip(@resource)
         end
 
-        it "should ask the resource if it's tagged with any of the tags" do
-            tags = ['one', 'two']
-            @transaction.stubs(:ignore_tags?).returns(false)
-            @transaction.stubs(:tags).returns(tags)
-
-            @resource.expects(:tagged?).with(*tags).returns(true)
-
-            @transaction.missing_tags?(@resource).should be_false
-        end
-
-        it "should skip not scheduled resources" do
+        it "should skip unscheduled resources" do
             @transaction.stubs(:scheduled?).returns(false)
-            @transaction.skip?(@resource).should be_true
+            @transaction.should be_skip(@resource)
         end
 
         it "should skip resources with failed dependencies" do
-            @transaction.stubs(:failed_dependencies?).returns(false)
-            @transaction.skip?(@resource).should be_true
+            @transaction.stubs(:failed_dependencies?).returns(true)
+            @transaction.should be_skip(@resource)
         end
 
         it "should skip virtual resource" do
             @resource.stubs(:virtual?).returns true
-            @transaction.skip?(@resource).should be_true
+            @transaction.should be_skip(@resource)
+        end
+    end
+
+    describe "when determining if tags are missing" do
+        before :each do
+            @resource = Puppet::Type.type(:notify).new :name => "foo"
+            @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
+            @transaction = Puppet::Transaction.new(@catalog)
+
+            @transaction.stubs(:ignore_tags?).returns false
+        end
+
+        it "should not be missing tags if tags are being ignored" do
+            @transaction.expects(:ignore_tags?).returns true
+
+            @resource.expects(:tagged?).never
+
+            @transaction.should_not be_missing_tags(@resource)
+        end
+
+        it "should not be missing tags if the transaction tags are empty" do
+            @transaction.tags = []
+            @resource.expects(:tagged?).never
+            @transaction.should_not be_missing_tags(@resource)
+        end
+
+        it "should otherwise let the resource determine if it is missing tags" do
+            tags = ['one', 'two']
+            @transaction.tags = tags
+            @resource.expects(:tagged?).with(*tags).returns(false)
+            @transaction.should be_missing_tags(@resource)
+        end
+    end
+
+    describe "when determining if a resource should be scheduled" do
+        before :each do
+            @resource = Puppet::Type.type(:notify).new :name => "foo"
+            @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
+            @transaction = Puppet::Transaction.new(@catalog)
+        end
+
+        it "should always schedule resources if 'ignoreschedules' is set" do
+            @transaction.ignoreschedules = true
+            @transaction.resource_harness.expects(:scheduled?).never
+
+            @transaction.should be_scheduled(@resource)
+        end
+
+        it "should let the resource harness determine whether the resource should be scheduled" do
+            @transaction.resource_harness.expects(:scheduled?).with(@resource).returns "feh"
+
+            @transaction.scheduled?(@resource).should == "feh"
         end
     end
 

spec/unit/transaction/resource_harness.rb

@@ -265,4 +265,65 @@ describe Puppet::Transaction::ResourceHarness do
             @harness.should_not be_allow_changes(@resource)
         end
     end
+
+    describe "when finding the schedule" do
+        before do
+            @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
+        end
+
+        it "should warn and return nil if the resource has no catalog" do
+            @resource.catalog = nil
+            @resource.expects(:warning)
+
+            @harness.schedule(@resource).should be_nil
+        end
+
+        it "should return nil if the resource specifies no schedule" do
+            @harness.schedule(@resource).should be_nil
+        end
+
+        it "should fail if the named schedule cannot be found" do
+            @resource[:schedule] = "whatever"
+            @resource.expects(:fail)
+            @harness.schedule(@resource)
+        end
+
+        it "should return the named schedule if it exists" do
+            sched = Puppet::Type.type(:schedule).new(:name => "sched")
+            @catalog.add_resource(sched)
+            @resource[:schedule] = "sched"
+            @harness.schedule(@resource).to_s.should == sched.to_s
+        end
+    end
+
+    describe "when determining if a resource is scheduled" do
+        before do
+            @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
+        end
+
+        it "should return true if 'ignoreschedules' is set" do
+            Puppet[:ignoreschedules] = true
+            @resource[:schedule] = "meh"
+            @harness.should be_scheduled(@resource)
+        end
+
+        it "should return true if the resource has no schedule set" do
+            @harness.should be_scheduled(@resource)
+        end
+
+        it "should return the result of matching the schedule with the cached 'checked' time if a schedule is set" do
+            t = Time.now
+            @resource.expects(:cached).with(:checked).returns(t)
+
+            sched = Puppet::Type.type(:schedule).new(:name => "sched")
+            @catalog.add_resource(sched)
+            @resource[:schedule] = "sched"
+
+            sched.expects(:match?).with(t.to_i).returns "feh"
+
+            @harness.scheduled?(@resource).should == "feh"
+        end
+    end
 end

test/ral/manager/type.rb

@@ -315,54 +315,4 @@ class TestType < Test::Unit::TestCase
         exec = Puppet::Type.newexec(:title => "yay", :command => "/bin/echo yay")
         assert_equal("Exec[yay]", exec.ref)
     end
-
-    # Partially test #704, but also cover the rest of the schedule management bases.
-    def test_schedule
-        schedule = Puppet::Type.type(:schedule).new(:name => "maint")
-        catalog = mk_catalog(schedule)
-
-        {"maint" => true, nil => false, :fail => :fail}.each do |name, should|
-            args = {:name => tempfile, :ensure => :file}
-            if name
-                args[:schedule] = name
-            end
-            resource = Puppet::Type.type(:file).new(args)
-            catalog.add_resource(resource)
-
-            if should == :fail
-                assert_raise(Puppet::Error, "Did not fail on missing schedule") do
-                    resource.schedule
-                end
-            elsif should == false
-                assert_nil(resource.schedule, "Set the schedule tho it is set to nil")
-            else
-                sched = nil
-                assert_nothing_raised("Failed when schedule was %s" % sched) do
-                    sched = resource.schedule
-                end
-
-                assert(sched, "Did not find schedule %s" % sched.inspect)
-
-                if should
-                    assert_equal(name, sched.name, "did not get correct schedule back")
-                end
-            end
-            catalog.remove_resource(resource)
-        end
-    end
-
-    # #801 -- resources only checked in noop should be rescheduled immediately.
-    def test_reschedule_when_noop
-        Puppet::Type.type(:schedule).mkdefaultschedules
-        file = Puppet::Type.type(:file).new(:path => "/tmp/whatever", :mode => "755", :noop => true, :schedule => :daily, :ensure => :file)
-        catalog = Puppet::Resource::Catalog.new
-        catalog.add_resource
-
-        assert(file.noop?, "File not considered in noop")
-        assert(file.scheduled?, "File is not considered scheduled")
-
-        catalog.apply
-
-        assert(file.scheduled?, "File is not considered scheduled even though only a noop run was made")
-    end
 end