From 7a192d7bea57990741398cb2fe120b51b7a3fd53 Mon Sep 17 00:00:00 2001 From: Wolf Noble Date: Mon, 18 Aug 2014 15:12:55 -0500 Subject: [PATCH] - add bits for updating apt - fix spec tests to include osfamily fact - add spec tests to verify current default behavior unimpacted. - manage the update-stamp file in puppet via content rather than a served file. - update custom fact to return -1 if the file doesn't exist - add spec test for custom fact - refactor to use a variable vs a collector/override - document parameters a bit more verbosely - remove empty unconstrained fact - Add osfamily fact to backports tests to facilitate functional tests on non-debian hosts --- README.md | 9 +- lib/facter/apt_update_last_success.rb | 18 +++ manifests/init.pp | 28 +++++ manifests/update.pp | 50 +++++++- spec/classes/apt_spec.rb | 8 ++ spec/classes/apt_update_spec.rb | 109 ++++++++++++++++++ spec/classes/backports_spec.rb | 12 +- spec/defines/builddep_spec.rb | 2 +- spec/defines/source_spec.rb | 4 + .../facter/apt_update_last_success_spec.rb | 28 +++++ 10 files changed, 260 insertions(+), 8 deletions(-) create mode 100644 lib/facter/apt_update_last_success.rb create mode 100644 spec/classes/apt_update_spec.rb create mode 100644 spec/unit/facter/apt_update_last_success_spec.rb diff --git a/README.md b/README.md index c7938a6..b0574a7 100644 --- a/README.md +++ b/README.md @@ -48,6 +48,7 @@ The parameters for `apt` are not generally required and are predominantly for de class { 'apt': always_apt_update => false, + apt_update_frequency => undef, disable_keys => undef, proxy_host => false, proxy_port => '8080', @@ -153,9 +154,9 @@ Moreover, when Puppet is told to hold a package, it holds it at the current vers At first glance, it seems this issue could also be solved by passing the version required to the ensure attribute---but that only means that Puppet will install that version after it processes the package. It does not inform apt that we want -this package to be held; that is, should another package want to upgrade this one (because of a version requirement in a dependency, for example), we want apt to refuse. +this package to be held; that is, should another package want to upgrade this one (because of a version requirement in a dependency, for example), we want apt to refuse. -To solve this issue, use apt::hold. Implement this by creating a preferences file with a priority of 1001. Under normal circumstances, this preference will always win. Because the priority is > 1000, apt will maintain the required version, downgrading the current package if necessary. +To solve this issue, use apt::hold. Implement this by creating a preferences file with a priority of 1001. Under normal circumstances, this preference will always win. Because the priority is > 1000, apt will maintain the required version, downgrading the current package if necessary. With this, you can now set a package's ensure attribute to 'latest' but get the version specified by apt::hold: @@ -211,7 +212,7 @@ If you would like to configure your system so the source is the Puppet Labs APT ### apt::update -Runs `apt-get update`, updating the list of available packages and their versions without installing or upgrading any packages. +Runs `apt-get update`, updating the list of available packages and their versions without installing or upgrading any packages. The update runs on the first Puppet run after you include the class, then whenever `notify => Exec['apt_update']` occurs---this should happen when config files get updated or other relevant changes occur. If you set the `always_apt_update` parameter, the update will run on every Puppet run. @@ -222,6 +223,7 @@ There are a few facts included in the apt module describing the state of the apt * `apt_updates` --- the number of updates available on the system * `apt_security_updates` --- the number of updates which are security updates * `apt_package_updates` --- the package names that are available for update. In Facter 2.0 and later, this will be a list type; in earlier versions, it is a comma-delimited string. +* `apt_update_last_success` --- The date in epochtime that `apt-get update` last ran successfully. This is determined by reading the mtime of the file `/var/lib/apt/periodic/update-success-stamp`. That file is generated by the `/etc/apt/apt.conf.d/15update-stamp` file. #### Hiera example 
@@ -332,3 +334,4 @@ A lot of great people have contributed to this module. A somewhat current list f
 * Zach Leslie 
 * Daniele Sluijters 
 * Daniel Paulus 
+* Wolf Noble 
diff --git a/lib/facter/apt_update_last_success.rb b/lib/facter/apt_update_last_success.rb
new file mode 100644
index 0000000..21c33d5
--- /dev/null
+++ b/lib/facter/apt_update_last_success.rb
@@ -0,0 +1,18 @@
+require 'facter'
+
+#This is derived from the file /var/lib/apt/periodic/update-success-stamp
+# This is generated upon a successful apt-get update run natively in ubuntu.
+# the Puppetlabs-apt module deploys this same functionality for other debian-ish OSes
+Facter.add('apt_update_last_success') do
+  confine :osfamily => 'Debian'
+  setcode do
+    if File.exists?('/var/lib/apt/periodic/update-success-stamp')
+      #get epoch time
+      lastsuccess = File.mtime('/var/lib/apt/periodic/update-success-stamp').to_i
+      lastsuccess
+    else
+      lastsuccess = -1
+      lastsuccess
+    end
+  end
+end
diff --git a/manifests/init.pp b/manifests/init.pp
index eb8c709..89fb684 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -5,15 +5,32 @@
 # Parameters:
 #   The parameters listed here are not required in general and were
 #     added for use cases related to development environments.
+#
 #   disable_keys - disables the requirement for all packages to be signed
+#
 #   always_apt_update - rather apt should be updated on every run (intended
 #     for development environments where package updates are frequent)
+#
+#   apt_update_frequency - *string* Supported values:
+#     **always**: Will fire `apt-get update` at every puppet run. Intended to
+#       deprecate the `always_apt_update` parameter.
+#     **daily**: Trigger `apt-get update` if the value of the fact
+#       `apt_update_last_success` is less than current epoch time - 86400.
+#        *notifying the apt_update exec will trigger apt-get update regardless*
+#     **weekly**: Trigger `apt-get update` if the value of the fact
+#       `apt_update_last_success` is less than current epoch time - 604800.
+#        *notifying the apt_update exec will trigger apt-get update regardless*
+#     **reluctantly**: *Default* only run apt-get update if the exec resource `apt_update` is notified.
+#
 #   purge_sources_list - Accepts true or false. Defaults to false If set to
 #     true, Puppet will purge all unmanaged entries from sources.list
+#
 #   purge_sources_list_d - Accepts true or false. Defaults to false. If set
 #     to true, Puppet will purge all unmanaged entries from sources.list.d
+#
 #   update_timeout - Overrides the exec timeout in seconds for apt-get update.
 #     If not set defaults to Exec's default (300)
+#
 #   update_tries - Number of times that `apt-get update` will be tried. Use this
 #     to work around transient DNS and HTTP errors. By default, the command
 #     will only be run once.
@@ -27,6 +44,7 @@
 
 class apt(
   $always_apt_update    = false,
+  $apt_update_frequency = 'reluctantly',
   $disable_keys         = undef,
   $proxy_host           = undef,
   $proxy_port           = '8080',
@@ -44,6 +62,8 @@ class apt(
     fail('This module only works on Debian or derivatives like Ubuntu')
   }
 
+  $frequency_options = ['always','daily','weekly','reluctantly']
+  validate_re($apt_update_frequency, $frequency_options)
   include apt::params
   include apt::update
 
@@ -61,6 +81,14 @@ class apt(
     }
   }
 
+  file { '/etc/apt/apt.conf.d/15update-stamp':
+    ensure  => 'file',
+    content => 'APT::Update::Post-Invoke-Success {"touch /var/lib/apt/periodic/update-success-stamp 2>/dev/null || true";};',
+    group   => 'root',
+    mode    => '0644',
+    owner   => 'root',
+  }
+
   $root           = $apt::params::root
   $apt_conf_d     = $apt::params::apt_conf_d
   $sources_list_d = $apt::params::sources_list_d
diff --git a/manifests/update.pp b/manifests/update.pp
index 725bb3b..9112c9b 100644
--- a/manifests/update.pp
+++ b/manifests/update.pp
@@ -1,10 +1,58 @@
 class apt::update {
   include apt::params
+  #TODO: to catch if $::apt_update_last_success has the value of -1 here. If we
+  #opt to do this, a info/warn would likely be all you'd need likely to happen
+  #on the first run, but if it's not run in awhile something is likely borked
+  #with apt and we'd want to know about it.
 
+  if $::apt::always_apt_update == false {
+    #if always_apt_update is true there's no point in parsing this logic.
+
+    case $apt::apt_update_frequency {
+      'always': {
+        $_kick_apt = true
+      }
+      'daily': {
+        #compare current date with the apt_update_last_success fact to determine
+        #if we should kick apt_update.
+        $daily_threshold = (strftime('%s') - 86400)
+        if $::apt_update_last_success {
+          if $::apt_update_last_success < $daily_threshold {
+            $_kick_apt = true
+          }
+        } else {
+          #if apt-get update has not successfully run, we should kick apt_update
+          $_kick_apt = true
+        }
+      }
+      'weekly':{
+        #compare current date with the apt_update_last_success fact to determine
+        #if we should kick apt_update.
+        $weekly_threshold = (strftime('%s') - 604800)
+        if $::apt_update_last_success {
+          if ( $::apt_update_last_success < $weekly_threshold ) {
+            $_kick_apt = true
+          }
+        } else {
+          #if apt-get update has not successfully run, we should kick apt_update
+          $_kick_apt = true
+        }
+      }
+      default: {
+        #catches 'recluctantly', and any other value (which should not occur).
+        #do nothing.
+      }
+    }
+  }
+  if $_kick_apt {
+    $_refresh = false
+  } else {
+    $_refresh = true
+  }
   exec { 'apt_update':
     command     => "${apt::params::provider} update",
     logoutput   => 'on_failure',
-    refreshonly => true,
+    refreshonly => $_refresh,
     timeout     => $apt::update_timeout,
     tries       => $apt::update_tries,
     try_sleep   => 1
diff --git a/spec/classes/apt_spec.rb b/spec/classes/apt_spec.rb
index 2c70e3e..0a1c0a1 100644
--- a/spec/classes/apt_spec.rb
+++ b/spec/classes/apt_spec.rb
@@ -37,6 +37,14 @@ describe 'apt', :type => :class do
       'notify' => 'Exec[apt_update]',
     })}
 
+    it 'should lay down /etc/apt/apt.conf.d/15update-stamp' do
+      should contain_file('/etc/apt/apt.conf.d/15update-stamp').with({
+        'group' => 'root',
+        'mode'  => '0644',
+        'owner' => 'root',
+      }).with_content('APT::Update::Post-Invoke-Success {"touch /var/lib/apt/periodic/update-success-stamp 2>/dev/null || true";};')
+    end
+
     it { should contain_file('old-proxy-file').that_notifies('Exec[apt_update]').only_with({
       'ensure' => 'absent',
       'path'   => '/etc/apt/apt.conf.d/proxy',
diff --git a/spec/classes/apt_update_spec.rb b/spec/classes/apt_update_spec.rb
new file mode 100644
index 0000000..d70efd3
--- /dev/null
+++ b/spec/classes/apt_update_spec.rb
@@ -0,0 +1,109 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+
+describe 'apt::update', :type => :class do
+  context 'when apt::always_apt_update is true' do
+    #This should completely disable all of this logic. These tests are to guarantee that we don't somehow magically change the behavior.
+    let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+    let (:pre_condition) { "class{'::apt': always_apt_update => true}" }
+    it 'should trigger an apt-get update run' do
+      #set the apt_update exec's refreshonly attribute to false
+      should contain_exec('apt_update').with({'refreshonly' => false })
+    end
+    ['always','daily','weekly','reluctantly'].each do |update_frequency|
+      context "when apt::apt_update_frequency has the value of #{update_frequency}" do
+        { 'a recent run' => Time.now.to_i, 'we are due for a run' => 1406660561,'the update-success-stamp file does not exist' => -1 }.each_pair do |desc, factval|
+          context "and $::apt_update_last_success indicates #{desc}" do
+            let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval } }
+            let (:pre_condition) { "class{'::apt': always_apt_update => true, apt_update_frequency => '#{update_frequency}' }" }
+            it 'should trigger an apt-get update run' do
+              # set the apt_update exec's refreshonly attribute to false
+              should contain_exec('apt_update').with({'refreshonly' => false})
+            end
+          end
+          context 'when $::apt_update_last_success is nil' do
+            let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+            let (:pre_condition) { "class{'::apt': always_apt_update => true, apt_update_frequency => '#{update_frequency}' }" }
+            it 'should trigger an apt-get update run' do
+              #set the apt_update exec\'s refreshonly attribute to false
+              should contain_exec('apt_update').with({'refreshonly' => false})
+            end
+          end
+        end
+      end
+    end
+  end
+
+  context 'when apt::always_apt_update is false' do
+    context "and apt::apt_update_frequency has the value of always" do
+      { 'a recent run' => Time.now.to_i, 'we are due for a run' => 1406660561,'the update-success-stamp file does not exist' => -1 }.each_pair do |desc, factval|
+        context "and $::apt_update_last_success indicates #{desc}" do
+          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval } }
+          let (:pre_condition) { "class{'::apt': always_apt_update => false, apt_update_frequency => 'always' }" }
+          it 'should trigger an apt-get update run' do
+            #set the apt_update exec's refreshonly attribute to false
+            should contain_exec('apt_update').with({'refreshonly' => false})
+          end
+        end
+      end
+      context 'when $::apt_update_last_success is nil' do
+        let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+        let (:pre_condition) { "class{ '::apt': always_apt_update => false, apt_update_frequency => 'always' }" }
+        it 'should trigger an apt-get update run' do
+          #set the apt_update exec\'s refreshonly attribute to false
+          should contain_exec('apt_update').with({'refreshonly' => false})
+        end
+      end
+    end
+    context "and apt::apt_update_frequency has the value of reluctantly" do
+      {'a recent run' => Time.now.to_i, 'we are due for a run' => 1406660561,'the update-success-stamp file does not exist' => -1 }.each_pair do |desc, factval|
+        context "and $::apt_update_last_success indicates #{desc}" do
+          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval} }
+          let (:pre_condition) { "class{ '::apt': always_apt_update => false, apt_update_frequency => 'reluctantly' }" }
+          it 'should not trigger an apt-get update run' do
+            #don't change the apt_update exec's refreshonly attribute. (it should be true)
+            should contain_exec('apt_update').with({'refreshonly' => true})
+          end
+        end
+      end
+      context 'when $::apt_update_last_success is nil' do
+        let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+        let (:pre_condition) { "class{ '::apt': always_apt_update => false, apt_update_frequency => 'reluctantly' }" }
+        it 'should not trigger an apt-get update run' do
+          #don't change the apt_update exec's refreshonly attribute. (it should be true)
+          should contain_exec('apt_update').with({'refreshonly' => true})
+        end
+      end
+    end
+    ['daily','weekly'].each do |update_frequency|
+      context "and apt::apt_update_frequency has the value of #{update_frequency}" do
+        { 'we are due for a run' => 1406660561,'the update-success-stamp file does not exist' => -1 }.each_pair do |desc, factval|
+          context "and $::apt_update_last_success indicates #{desc}" do
+            let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => factval } }
+            let (:pre_condition) { "class{ '::apt': always_apt_update => false, apt_update_frequency => '#{update_frequency}' }" }
+            it 'should trigger an apt-get update run' do
+              #set the apt_update exec\'s refreshonly attribute to false
+              should contain_exec('apt_update').with({'refreshonly' => false})
+            end
+          end
+        end
+        context 'when the $::apt_update_last_success fact has a recent value' do
+          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian', :apt_update_last_success => Time.now.to_i } }
+          let (:pre_condition) { "class{ '::apt': always_apt_update => false, apt_update_frequency => '#{update_frequency}' }" }
+          it 'should not trigger an apt-get update run' do
+            #don't change the apt_update exec\'s refreshonly attribute. (it should be true)
+            should contain_exec('apt_update').with({'refreshonly' => true})
+          end
+        end
+        context 'when $::apt_update_last_success is nil' do
+          let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
+          let (:pre_condition) { "class{ '::apt': always_apt_update => false, apt_update_frequency => '#{update_frequency}' }" }
+          it 'should trigger an apt-get update run' do
+            #set the apt_update exec\'s refreshonly attribute to false
+            should contain_exec('apt_update').with({'refreshonly' => false})
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/classes/backports_spec.rb b/spec/classes/backports_spec.rb
index 17c86eb..18f6337 100644
--- a/spec/classes/backports_spec.rb
+++ b/spec/classes/backports_spec.rb
@@ -5,7 +5,8 @@ describe 'apt::backports', :type => :class do
     let :facts do
       {
         'lsbdistcodename' => 'Karmic',
-        'lsbdistid'       => 'Ubuntu'
+        'lsbdistid'       => 'Ubuntu',
+        'osfamily'        => 'Debian'
       }
     end
 
@@ -36,7 +37,8 @@ describe 'apt::backports', :type => :class do
     let :facts do
       {
         'lsbdistcodename' => 'Karmic',
-        'lsbdistid'       => 'Ubuntu'
+        'lsbdistid'       => 'Ubuntu',
+        'osfamily'        => 'Debian'
       }
     end
 
@@ -57,6 +59,7 @@ describe 'apt::backports', :type => :class do
       {
         'lsbdistcodename' => 'Squeeze',
         'lsbdistid'       => 'Debian',
+        'osfamily'        => 'Debian'
       }
     end
 
@@ -77,6 +80,7 @@ describe 'apt::backports', :type => :class do
       {
         'lsbdistcodename' => 'debian',
         'lsbdistid'       => 'LinuxMint',
+        'osfamily'        => 'Debian'
       }
     end
 
@@ -97,6 +101,7 @@ describe 'apt::backports', :type => :class do
       {
         'lsbdistcodename' => 'qiana',
         'lsbdistid'       => 'LinuxMint',
+        'osfamily'        => 'Debian'
       }
     end
 
@@ -116,7 +121,8 @@ describe 'apt::backports', :type => :class do
     let :facts do
       {
         'lsbdistcodename' => 'Squeeze',
-        'lsbdistid'       => 'Debian'
+        'lsbdistid'       => 'Debian',
+        'osfamily'        => 'Debian'
       }
     end
 
diff --git a/spec/defines/builddep_spec.rb b/spec/defines/builddep_spec.rb
index 8836693..41152d5 100644
--- a/spec/defines/builddep_spec.rb
+++ b/spec/defines/builddep_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 describe 'apt::builddep', :type => :define do
 
-  let(:facts) { { :lsbdistid => 'Debian' } }
+  let(:facts) { { :lsbdistid => 'Debian', :osfamily => 'Debian' } }
   let(:title) { 'my_package' }
 
   describe "defaults" do
diff --git a/spec/defines/source_spec.rb b/spec/defines/source_spec.rb
index 960bb62..8327ed2 100644
--- a/spec/defines/source_spec.rb
+++ b/spec/defines/source_spec.rb
@@ -12,6 +12,7 @@ describe 'apt::source', :type => :define do
       {
         :lsbdistid       => 'Debian',
         :lsbdistcodename => 'wheezy',
+        :osfamily        => 'Debian'
       }
     end
 
@@ -36,6 +37,7 @@ describe 'apt::source', :type => :define do
       {
         :lsbdistid       => 'Debian',
         :lsbdistcodename => 'wheezy',
+        :osfamily        => 'Debian'
       }
     end
     let :params do
@@ -95,6 +97,7 @@ describe 'apt::source', :type => :define do
       {
         :lsbdistid       => 'Debian',
         :lsbdistcodename => 'wheezy',
+        :osfamily        => 'Debian'
       }
     end
     let :params do
@@ -114,6 +117,7 @@ describe 'apt::source', :type => :define do
       let :facts do
         {
           :lsbdistid       => 'Debian',
+          :osfamily        => 'Debian'
         }
       end
 
diff --git a/spec/unit/facter/apt_update_last_success_spec.rb b/spec/unit/facter/apt_update_last_success_spec.rb
new file mode 100644
index 0000000..08774cd
--- /dev/null
+++ b/spec/unit/facter/apt_update_last_success_spec.rb
@@ -0,0 +1,28 @@
+require 'spec_helper'
+
+describe 'apt_update_last_success fact' do
+  subject { Facter.fact(:apt_update_last_success).value }
+  after(:each) { Facter.clear }
+
+  describe 'on Debian based distro which has not yet created the update-success-stamp file' do
+    before {
+      Facter.fact(:osfamily).stubs(:value).returns 'Debian'
+      File.stubs(:exists?).returns false
+    }
+    it 'should have a value of -1' do
+      should == -1
+    end
+  end
+
+  describe 'on Debian based distro which has created the update-success-stamp' do
+    before {
+      Facter.fact(:osfamily).stubs(:value).returns 'Debian'
+      File.stubs(:exists?).returns true
+      File.stubs(:mtime).returns 1407660561
+    }
+    it 'should have the value of the mtime of the file' do
+      should == 1407660561
+    end
+  end
+
+end
-- 
2.45.2