Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Lockfile to mitigate r10k race condition. #268

Merged
merged 3 commits into from
Jan 5, 2017
Merged

Lockfile to mitigate r10k race condition. #268

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5b6c28
Lockfile to mitigate r10k race condition.
binford2k Feb 23, 2016
dd0a762
Add acceptance test for webhook locking
binford2k Dec 2, 2016
7b37890
make rubocop happy
binford2k Dec 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion spec/acceptance/basic_webhook_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class {'r10k::webhook::config':
use_mcollective => false,
notify => Service['webhook'],
}

class {'r10k::webhook':
require => Class['r10k::webhook::config'],
}
Expand Down Expand Up @@ -58,5 +58,17 @@ class {'r10k::webhook':
expect(r.exit_code).to eq(0)
end
end

it 'should successfully lock when hammered with multiple requests' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the it tests inside the shell loop to remedy the rubocop warnings on too many expectations. The other rubocop warnings should be pretty explanatory except the array one, but I've not seen that before, sorry.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't really good validations. Having four separate it blocks implies that it's testing four different things, and it's not. It's testing one.

Not using the word "should" is fine, but all the other tests do, so that becomes inconsistent.

Not allowing a newline to delineate different blocks is just asinine, especially when theres a cascade of ends, like below. Which one ends which block?

          end
        end
      end
    end
  end
end

Its style guide even praises using newlines. https://github.com/bbatsov/ruby-style-guide#empty-lines-between-methods but has nothing for or against newlines around blocks.

The regex rule linked to doesn't actually say to always use %r, it says "Use %r only for regular expressions matching at least one '/' character." This regex does not, it simply matches the word "success". I'd change it anyways to make the rule happy, but then this test would be inconsistent with the rest.

I fixed the times.map (wasn't really needed anyway), took out the readability whitespace, and added a disable comment for the multiple expectations. If you want the other rules to pass, you ought to make a single commit to change all of them at once for consistency.

4.times.map do
Thread.new do
shell('/usr/bin/curl -d \'{ "ref": "refs/heads/production" }\' -H "Accept: application/json" "http://localhost:8088/payload" -k -q') do |r|
expect(r.stdout).to match(/^.*success.*$/)
expect(r.exit_code).to eq(0)
end
end
end
end

end
end
25 changes: 17 additions & 8 deletions templates/webhook.bin.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ require 'open3'

WEBHOOK_CONFIG = '/etc/webhook.yaml'
PIDFILE = '/var/run/webhook/webhook.pid'
LOCKFILE = '/var/run/webhook/webhook.lock'
APP_ROOT = '/var/run/webhook'

if (File.exists?(WEBHOOK_CONFIG))
Expand Down Expand Up @@ -181,15 +182,23 @@ end
end

def run_command(command)
if Open3.respond_to?('capture3')
stdout, stderr, exit_status = Open3.capture3(command)
message = "triggered: #{command}\n#{stdout}\n#{stderr}"
else
message = "forked: #{command}"
Process.detach(fork{ exec "#{command} &"})
exit_status = 0
message = ''
File.open(LOCKFILE, 'w+') do |file|
# r10k has a small race condition which can cause failed deploys if two happen
# more or less simultaneously. To mitigate, we just lock on a file and wait for
# the other one to complete.
file.flock(File::LOCK_EX)

if Open3.respond_to?('capture3')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh god, can we please, please let Ruby 1.8.7 die? Ruby 1.9.3 has this (https://ruby-doc.org/stdlib-1.9.3/libdoc/open3/rdoc/Open3.html#method-c-capture3), and even that version is EOL'd.

Please, Ruby 1.8.7 is old and tired. It just wants to go out to pasture, graze quietly, and enjoy the twilight years of its life. Stop making Ruby 1.8.7 suffer on.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ruby 1.8 is not well

stdout, stderr, exit_status = Open3.capture3(command)
message = "triggered: #{command}\n#{stdout}\n#{stderr}"
else
message = "forked: #{command}"
Process.detach(fork{ exec "#{command} &"})
exit_status = 0
end
raise "#{stdout}\n#{stderr}" if exit_status != 0
end
raise "#{stdout}\n#{stderr}" if exit_status != 0
message
end

Expand Down