From 59afc07d3bfd677dbdb716a7cecbe4985c9cbf4b Mon Sep 17 00:00:00 2001 From: Dan Bode Date: Sat, 21 Apr 2012 12:54:17 -0700 Subject: [PATCH] Refactor of swift server configs This commit performs a refactor of the swift::storage::config to use fragments. Updates server templates - makes workers,user, and mount_checks configurable - adds a default for concurrency - makes the pipeline configurable - remove vm_test_mode flag Updates swift::storage::server to use fragments for the config file. This has been refactored to allow the end user a greater level of flexibility over how they can configure custom plugins for swift. Also adds additional class params: pipeline, mount_check, user, workers, concurrency. Update the unit tests for swift::storage:server --- .gitignore | 1 + manifests/storage/server.pp | 32 ++++- spec/defines/swift_storage_server_spec.rb | 150 +++++++++++++--------- spec/spec_helper.rb | 5 + templates/account-server.conf.erb | 10 +- templates/container-server.conf.erb | 10 +- templates/object-server.conf.erb | 10 +- 7 files changed, 137 insertions(+), 81 deletions(-) diff --git a/.gitignore b/.gitignore index 4d0b6578..12998cd7 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ spec/fixtures/modules/rsync spec/fixtures/modules/ssh spec/fixtures/modules/stdlib spec/fixtures/modules/xinetd +spec/fixtures/modules/* diff --git a/manifests/storage/server.pp b/manifests/storage/server.pp index 5a5b8174..ab788221 100644 --- a/manifests/storage/server.pp +++ b/manifests/storage/server.pp @@ -6,15 +6,29 @@ define swift::storage::server( $type, $storage_local_net_ip, - $devices = '/srv/node', - $owner = 'swift', - $group = 'swift', - $max_connections = 25, + $devices = '/srv/node', + $owner = 'swift', + $group = 'swift', + $max_connections = 25, + $pipeline = ["${type}-server"], + $mount_check = 'false', + $user = 'swift', + $workers = '1', + $concurrency = $::processorcount, # this parameters needs to be specified after type and name $config_file_path = "${type}-server/${name}.conf" ) { + # TODO if array does not include type-server, warn + if( + (is_array($pipeline) and ! member($pipeline, "${type}-server")) or + $pipline != "${type}-server" + ) { + warning("swift storage server ${type} must specify ${type}-server") + } + include "swift::storage::$type" + include 'concat::setup' validate_re($name, '^\d+$') validate_re($type, '^object|container|account$') @@ -31,11 +45,17 @@ define swift::storage::server( read_only => false, } - file { "/etc/swift/${config_file_path}": - content => template("swift/${type}-server.conf.erb"), + concat { "/etc/swift/${config_file_path}": owner => $owner, group => $group, notify => Service["swift-${type}"], + mode => 640, } + # you can now add your custom fragments at the user level + concat::fragment { "swift-${type}-${name}": + target => "/etc/swift/${config_file_path}", + content => template("swift/${type}-server.conf.erb"), + order => '00', + } } diff --git a/spec/defines/swift_storage_server_spec.rb b/spec/defines/swift_storage_server_spec.rb index 4b8e50c2..fe849268 100644 --- a/spec/defines/swift_storage_server_spec.rb +++ b/spec/defines/swift_storage_server_spec.rb @@ -4,7 +4,8 @@ describe 'swift::storage::server' do let :facts do { :operatingsystem => 'Ubuntu', - :osfamily => 'Debian' + :osfamily => 'Debian', + :processorcount => 1 } end @@ -21,8 +22,6 @@ describe 'swift::storage::server' do :max_connections => '25'} end - - describe 'with an invalid title' do let :params do {:storage_local_net_ip => '127.0.0.1', @@ -39,66 +38,97 @@ describe 'swift::storage::server' do end ['account', 'object', 'container'].each do |t| - [{:storage_local_net_ip => '10.0.0.1', - :type => t}, - {:storage_local_net_ip => '127.0.0.1', - :type => t} - ].each do |param_set| - describe "when #{param_set == {} ? "using default" : "specifying"} class parameters" do - let :title do - '8000' - end - let :param_hash do - default_params.merge(param_set) - end - let :params do - param_set - end - let :config_file_path do - "#{t}-server/#{title}.conf" - end - it { should contain_package("swift-#{t}").with_ensure('present') } - it { should contain_service("swift-#{t}").with( - :ensure => 'running', - :enable => true, - :hasstatus => true - )} - it { should contain_file("/etc/swift/#{t}-server/").with( - :ensure => 'directory', - :owner => 'swift', - :group => 'swift' - )} - it { should contain_rsync__server__module("#{t}#{title}").with( - :path => param_hash[:devices], - :lock_file => "/var/lock/#{t}#{title}.lock", - :uid => param_hash[:owner], - :gid => param_hash[:group], - :max_connections => param_hash[:max_connections], - :read_only => false - )} - it { should contain_file("/etc/swift/#{config_file_path}").with( - :owner => param_hash[:owner], - :group => param_hash[:group] - )} - it 'should have some contents' do - content = param_value( - subject, - 'file', "/etc/swift/#{config_file_path}", - 'content' - ) - expected_lines = - [ - '[DEFAULT]', - "devices = #{param_hash[:devices]}", - "bind_ip = #{param_hash[:storage_local_net_ip]}", - "bind_port = #{title}" - ] - (content.split("\n") & expected_lines).should =~ expected_lines + + describe "for type #{t}" do + + let :title do + '8000' + end + + let :req_params do + {:storage_local_net_ip => '10.0.0.1', :type => t} + end + let :params do + req_params + end + + it { should contain_package("swift-#{t}").with_ensure('present') } + it { should contain_service("swift-#{t}").with( + :ensure => 'running', + :enable => true, + :hasstatus => true + )} + let :fragment_file do + "/var/lib/puppet/concat/_etc_swift_#{t}-server_#{title}.conf/fragments/00_swift-#{t}-#{title}" + end + + describe 'when parameters are overridden' do + { + :devices => '/tmp/foo', + :user => 'dan', + :mount_check => true, + :concurrency => 5, + :workers => 7, + :pipeline => 'foo' + }.each do |k,v| + describe "when #{k} is set" do + let :params do req_params.merge({k => v}) end + it { should contain_file(fragment_file) \ + .with_content(/^#{k.to_s}\s*=\s*#{v}\s*$/) + } + end + describe "when pipline is passed an array" do + let :params do req_params.merge({:pipeline => [1,2,3]}) end + it { should contain_file(fragment_file) \ + .with_content(/^pipeline\s*=\s*1 2 3\s*$/) + } + end end end - # TODO - I do not want to add tests for the upstart stuff - # I need to check the tickets and see if this stuff is fixed + describe 'with all allowed defaults' do + let :params do + req_params + end + + it { should contain_rsync__server__module("#{t}#{title}").with( + :path => '/srv/node', + :lock_file => "/var/lock/#{t}#{title}.lock", + :uid => 'swift', + :gid => 'swift', + :max_connections => 25, + :read_only => false + )} + + # verify template lines + it { should contain_file(fragment_file) \ + .with_content(/^devices\s*=\s*\/srv\/node\s*$/) + } + it { should contain_file(fragment_file) \ + .with_content(/^bind_ip\s*=\s*10\.0\.0\.1\s*$/) + } + it { should contain_file(fragment_file) \ + .with_content(/^bind_port\s*=\s*#{title}\s*$/) + } + it { should contain_file(fragment_file) \ + .with_content(/^mount_check\s*=\s*false\s*$/) + } + it { should contain_file(fragment_file) \ + .with_content(/^user\s*=\s*swift\s*$/) + } + it { should contain_file(fragment_file) \ + .with_content(/^log_facility\s*=\s*LOG_LOCAL2\s*$/) + } + it { should contain_file(fragment_file) \ + .with_content(/^workers\s*=\s*1\s*$/) + } + it { should contain_file(fragment_file) \ + .with_content(/^concurrency\s*=\s*1\s*$/) + } + it { should contain_file(fragment_file) \ + .with_content(/^pipeline\s*=\s*#{t}-server\s*$/) + } + end end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 0b8c6b3f..8d6d4c31 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -6,6 +6,11 @@ def param_value(subject, type, title, param) subject.resource(type, title).send(:parameters)[param.to_sym] end +def verify_contents(subject, title, expected_lines) + content = subject.resource('file', title).send(:parameters)[:content] + (content.split("\n") & expected_lines).should == expected_lines +end + RSpec.configure do |c| c.module_path = File.expand_path(File.join(File.dirname(__FILE__), 'fixtures/modules')) # Using an empty site.pp file to avoid: https://github.com/rodjek/rspec-puppet/issues/15 diff --git a/templates/account-server.conf.erb b/templates/account-server.conf.erb index 52f49f1e..53bf5af9 100644 --- a/templates/account-server.conf.erb +++ b/templates/account-server.conf.erb @@ -2,19 +2,19 @@ devices = <%= devices %> bind_ip = <%= storage_local_net_ip %> bind_port = <%= bind_port %> -mount_check = false -user = swift +mount_check = <%= mount_check %> +user = <%= user %> log_facility = LOG_LOCAL2 -workers = 2 +workers = <%= workers %> +concurrency = <%= concurrency %> [pipeline:main] -pipeline = account-server +pipeline = <%= pipeline.to_a.join(' ') %> [app:account-server] use = egg:swift#account [account-replicator] -vm_test_mode = yes [account-auditor] diff --git a/templates/container-server.conf.erb b/templates/container-server.conf.erb index de27c6c2..d35af417 100644 --- a/templates/container-server.conf.erb +++ b/templates/container-server.conf.erb @@ -2,19 +2,19 @@ devices = <%= devices %> bind_ip = <%= storage_local_net_ip %> bind_port = <%= bind_port %> -mount_check = false -user = swift +mount_check = <%= mount_check %> +user = <%= user %> log_facility = LOG_LOCAL2 -workers = 2 +workers = <%= workers %> +concurrency = <%= concurrency %> [pipeline:main] -pipeline = container-server +pipeline = <%= pipeline.to_a.join(' ') %> [app:container-server] use = egg:swift#container [container-replicator] -vm_test_mode = yes [container-updater] diff --git a/templates/object-server.conf.erb b/templates/object-server.conf.erb index 4eea4ddf..ac9451c0 100644 --- a/templates/object-server.conf.erb +++ b/templates/object-server.conf.erb @@ -2,19 +2,19 @@ devices = <%= devices %> bind_ip = <%= storage_local_net_ip %> bind_port = <%= bind_port %> -mount_check = false -user = swift +mount_check = <%= mount_check %> +user = <%= user %> log_facility = LOG_LOCAL2 -workers = 2 +workers = <%= workers %> +concurrency = <%= concurrency %> [pipeline:main] -pipeline = object-server +pipeline = <%= pipeline.to_a.join(' ') %> [app:object-server] use = egg:swift#object [object-replicator] -vm_test_mode = yes [object-updater]