From 1570be5c75c29871fe142d3ea98ba1383312a590 Mon Sep 17 00:00:00 2001 From: Thomas Equeter Date: Thu, 7 Feb 2019 17:11:04 +0100 Subject: [PATCH] avoid ipptool errors when Cups is slow to start Problem statement: on some OSes (most notably RHEL7 and derivatives), the Cups service may have started and not listen on the IPP port yet. This causes a problem when this module (re)starts the service and expects to be able to add/remove printers with ipptool right after. Thus, modify the standard cups.socket service on RHEL7 through a drop-in to also manage the IPP port. This way, systemd will accept the incoming connections and hand them off to Cups once it is really started. This is mostly a kludge for a 3rd party bug, but still required to make the module behave reliably. Fix some cases of #35. Breaking changes: - Bump the Puppet minimum version to 4.10.0 for Hiera 5. - Depend on puppetlabs/stdlib and camptocamp/systemd (and its dependency puppetlabs/inifile). --- .fixtures.yml | 5 +- README.md.erb | 5 ++ data/os/RedHat-7.yaml | 2 + hiera.yaml | 5 ++ manifests/init.pp | 5 ++ manifests/server.pp | 4 ++ manifests/server/socket.pp | 54 +++++++++++++++++++ metadata.json | 15 +++++- .../puppet/cups_queue_after_start.rb | 52 ++++++++++++++++++ spec/classes/init_spec.rb | 30 +++++++++++ spec/classes/server_spec.rb | 3 ++ spec/spec_helper_acceptance.rb | 8 +-- 12 files changed, 182 insertions(+), 6 deletions(-) create mode 100644 data/os/RedHat-7.yaml create mode 100644 hiera.yaml create mode 100644 manifests/server/socket.pp create mode 100644 spec/acceptance/puppet/cups_queue_after_start.rb diff --git a/.fixtures.yml b/.fixtures.yml index 9b1391d2..8cdf53c2 100644 --- a/.fixtures.yml +++ b/.fixtures.yml @@ -2,4 +2,7 @@ fixtures: symlinks: cups: "#{source_dir}" - + forge_modules: + systemd: camptocamp/systemd + stdlib: puppetlabs/stdlib + inifile: puppetlabs/inifile diff --git a/README.md.erb b/README.md.erb index 836d53df..3e39a215 100644 --- a/README.md.erb +++ b/README.md.erb @@ -552,6 +552,11 @@ Installs, configures, and manages the CUPS service. * `service_names`: A name or an array of names of all CUPS services to be managed. Defaults to `cups`. +* `socket_manage`: Workaround a CUPS/OS bug (see `cups::server::socket`). + Enabled by default on RHEL7, disabled elsewhere. + +* `socket_ensure`: Should match `service_ensure`, the default. + * `web_interface`: Boolean value to enable or disable the server's web interface. #### Type: `cups_queue` diff --git a/data/os/RedHat-7.yaml b/data/os/RedHat-7.yaml new file mode 100644 index 00000000..c65a23c7 --- /dev/null +++ b/data/os/RedHat-7.yaml @@ -0,0 +1,2 @@ +--- +cups::socket_manage: true diff --git a/hiera.yaml b/hiera.yaml new file mode 100644 index 00000000..41a774d0 --- /dev/null +++ b/hiera.yaml @@ -0,0 +1,5 @@ +--- +version: 5 +hierarchy: + - name: OS configuration + path: "os/%{facts.os.family}-%{facts.os.release.major}.yaml" diff --git a/manifests/init.pp b/manifests/init.pp index b539e25e..10c86045 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -53,6 +53,9 @@ # @param service_ensure Whether the CUPS services should be `running` or `stopped`. # @param service_manage Whether to manage services at all. # @param service_names A name or an array of names of all CUPS services to be managed. +# @param socket_manage Workaround a CUPS/OS bug (see `cups::server::socket`). +# Enabled by default on RHEL7, disabled elsewhere. +# @param socket_ensure Should match `service_ensure`, the default. # @param web_interface Boolean value to enable or disable the server's web interface. # class cups ( @@ -78,6 +81,8 @@ String $service_ensure = 'running', Boolean $service_manage = true, Variant[String, Array[String]] $service_names = 'cups', + Boolean $socket_manage = false, + String[1] $socket_ensure = $service_ensure, Optional[Boolean] $web_interface = undef, ) inherits cups::params { diff --git a/manifests/server.pp b/manifests/server.pp index 6f5a535e..523c8a15 100644 --- a/manifests/server.pp +++ b/manifests/server.pp @@ -11,9 +11,13 @@ class cups::server inherits cups { contain cups::server::config + contain cups::server::socket contain cups::server::services Class[cups::server::config] ~> Class[cups::server::services] + Class[cups::server::config] + -> Class[cups::server::socket] + -> Class[cups::server::services] } diff --git a/manifests/server/socket.pp b/manifests/server/socket.pp new file mode 100644 index 00000000..c1173724 --- /dev/null +++ b/manifests/server/socket.pp @@ -0,0 +1,54 @@ +# @private +# +# @summary Avoid ipptool errors when Cups is slow to start +# +# Problem statement: on some OSes (most notably RHEL7 and derivatives), the +# Cups service may have started and not listen on the IPP port yet. This causes +# a problem when this module (re)starts the service and expects to be able to +# add/remove printers with ipptool right after. +# +# If `$cups::socket_manage` is enabled, this class modifies the standard +# `cups.socket` service to also manage the IPP port. This way, systemd will +# accept the incoming connections and hand them off to Cups once it is really +# started. This is mostly a kludge for a 3rd party bug, but still required to +# make the module behave reliably. +# +# If `$cups::socket_manage` is disabled, this workaround is removed. +# +# The `$cups::socket_ensure` argument may optionnally be set to `stopped`, and +# this setting should probably match `$cups::service_ensure`. +# +# The `$cups::service_names` argument is used to generate the socket services, +# ie. each X.service will have its X.socket managed as described above. +# +# @author Thomas Equeter +# @since 3.0.0 +# +class cups::server::socket inherits cups::server { + + $_dropin_file = systemd::dropin_file { 'workaround-puppet-cups-35.conf': + ensure => if $cups::socket_manage { 'present' } else { 'absent' }, + unit => 'cups.socket', + content => @(END), + [Socket] + ListenStream=[::1]:631 + |END + } + + if $cups::socket_manage { + Array($cups::service_names, true).each |$_service_name| { + $_safe_name = shell_escape($_service_name) + + service { "${_service_name}.socket": + ensure => $cups::socket_ensure, + # Both units listen to port 631, however cups.service will play nice if + # cups.socket is started first. See sd_listen_fds(3). + start => "systemctl stop ${_safe_name}.service && systemctl start ${_safe_name}.socket", + restart => "systemctl stop ${_safe_name}.service && systemctl restart ${_safe_name}.socket", + require => Class['systemd::systemctl::daemon_reload'], + subscribe => $_dropin_file, + } + } + } +} + diff --git a/metadata.json b/metadata.json index 9d33cc71..a89b7ce2 100644 --- a/metadata.json +++ b/metadata.json @@ -13,7 +13,18 @@ "printing" ], "dependencies": [ - + { + "name": "camptocamp/systemd", + "version_requirement": ">= 1.1.0 < 3.0.0" + }, + { + "name": "puppetlabs/stdlib", + "version_requirement": ">= 4.13.1 < 6.0.0" + }, + { + "name": "puppetlabs/inifile", + "version_requirement": ">= 1.6.0 < 3.0.0" + } ], "operatingsystem_support": [ { @@ -92,7 +103,7 @@ "requirements": [ { "name": "puppet", - "version_requirement": ">= 4.0.0 < 7.0.0" + "version_requirement": ">= 4.10.0 < 7.0.0" } ], "data_provider": null, diff --git a/spec/acceptance/puppet/cups_queue_after_start.rb b/spec/acceptance/puppet/cups_queue_after_start.rb new file mode 100644 index 00000000..69112317 --- /dev/null +++ b/spec/acceptance/puppet/cups_queue_after_start.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper_acceptance' + +RSpec.describe 'Custom type `cups_queue`' do + describe 'when the Cups service is starting' do + before(:each) do + ensure_cups_is_running + # Cups takes time to listen to IPP after service start, depending on the + # amount of printers defined. Just a few printers are enough to cause the + # cups_queue ipptool calls to fail right after a restart, but adding 50 + # here makes it future-proof for the ever-faster testing environments. + printer_names = (1..50).map { |n| "Office#{n}" } + printer_names.each do |name| + shell("lpadmin -E -p #{name} -m drv:///sample.drv/generic.ppd -o printer-is-shared=false", silent: true) + end + remove_queues('Office') + + # Cleanup + if fact('systemd') then + shell('rm -f /etc/systemd/system/cups.socket.d/workaround-puppet-cups-35.conf') + shell('systemctl daemon-reload', accept_all_exit_codes: true) + shell('systemctl stop cups.socket cups.service', accept_all_exit_codes: true) + else + # Not needed, but for coherence with the systemd version + shell('service cups stop') + end + end + + manifest = <<-MANIFEST + class { 'cups': + service_ensure => 'running', + } + cups_queue { 'Office': + ensure => 'printer', + } + MANIFEST + + it 'can refresh the service and create a queue' do + # Do NOT use ensure_cups_is_running here, it calls the cups class and + # installs the dropin file, which is exactly what we want to test below. + shell('service cups start') + # Cause a rewrite of the configuration and trigger a service refresh. + shell('rm /etc/cups/cupsd.conf') + apply_manifest(manifest, expect_changes: true) + end + + it 'can start the service and create a queue' do + apply_manifest(manifest, expect_changes: true) + end + end +end diff --git a/spec/classes/init_spec.rb b/spec/classes/init_spec.rb index b8d73c06..5c4010f8 100644 --- a/spec/classes/init_spec.rb +++ b/spec/classes/init_spec.rb @@ -562,6 +562,36 @@ end end + describe 'socket_manage' do + let(:facts) { any_supported_os } + + context 'when set to false' do + let(:params) do + { + socket_manage: false + } + end + + it { is_expected.to_not contain_service('cups.socket') } + it { is_expected.to contain_systemd__dropin_file('workaround-puppet-cups-35.conf').with_ensure('absent') } + end + + %w[stopped running].each do |socket_ensure| + context "when set to true and ensure => #{socket_ensure}" do + let(:params) do + { + socket_manage: true, + socket_ensure: socket_ensure + } + end + + it { is_expected.to contain_systemd__dropin_file('workaround-puppet-cups-35.conf').with_ensure('present') } + it { is_expected.to contain_systemd__dropin_file('workaround-puppet-cups-35.conf').that_notifies('Service[cups.socket]') } + it { is_expected.to contain_service('cups.socket').with_ensure(socket_ensure) } + end + end + end + describe 'server_alias' do let(:facts) { any_supported_os } diff --git a/spec/classes/server_spec.rb b/spec/classes/server_spec.rb index 8e658c10..268e76c7 100644 --- a/spec/classes/server_spec.rb +++ b/spec/classes/server_spec.rb @@ -7,7 +7,10 @@ let(:facts) { any_supported_os } it { is_expected.to contain_class('cups::server::config').that_notifies('Class[cups::server::services]') } + it { is_expected.to contain_class('cups::server::config').that_comes_before('Class[cups::server::socket]') } it { is_expected.to contain_class('cups::server::services') } + + it { is_expected.to contain_class('cups::server::socket').that_comes_before('Class[cups::server::services]') } end end diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb index 12ab6ec8..9c250b5e 100644 --- a/spec/spec_helper_acceptance.rb +++ b/spec/spec_helper_acceptance.rb @@ -5,6 +5,8 @@ require 'beaker' require 'beaker-puppet' require 'beaker-rspec' +require 'beaker/puppet_install_helper' +require 'beaker/module_install_helper' # Beaker related configuration # http://www.rubydoc.info/github/puppetlabs/beaker/Beaker/DSL @@ -12,9 +14,9 @@ project_root = File.expand_path(File.join(File.dirname(__FILE__), '..')) c.before(:suite) do hosts.each do |host| - shell("sed -i 's/^nameserver.*/nameserver 8.8.8.8/' /etc/resolv.conf") - install_puppet_agent_on(host, puppet_collection: 'puppet6') - copy_module_to(host, module_name: 'cups', source: project_root, target_module_path: '/etc/puppetlabs/code/modules') + run_puppet_install_helper_on(host) unless ENV['BEAKER_provision'] == 'no' + install_module_on(host) + install_module_dependencies_on(host) scp_to(host, File.join(project_root, 'spec/fixtures/ppd/textonly.ppd'), '/tmp/') end end