Skip to content

(IAC-1218) - docker_params_changed should be run by agent #705

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.

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

Merged
merged 9 commits into from
Feb 1, 2021
184 changes: 184 additions & 0 deletions lib/puppet/functions/docker_params_changed.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
# frozen_string_literal: true

Puppet::Functions.create_function(:docker_params_changed) do
dispatch :detect_changes do
param 'Hash', :opts
return_type 'String'
end

def run_with_powershell(cmd)
"powershell.exe -Command \"& {#{cmd}}\" "
end

def remove_cidfile(cidfile, osfamily, _logger = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see _logger being set to nil in the methods within this file, but I never see _logger being used or passed anywhere else where it is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I let the logger parameter to use in a 'debug' mode. If you want to see some logs from just need to uncomment the line with the specific log.

delete_command = if osfamily == 'windows'
run_with_powershell("del #{cidfile}")
else
"rm -f #{cidfile}"
end
_stdout, _stderr, _status = Open3.capture3(delete_command)
# logger.puts("### BEGIN remove_cidfile\n\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're not outputting these logs anywhere, we should probably delete these lines. I assume they were there to assist in the development process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to send the logs to the journalctl, but didn't work as I want to work(without pre-configuration) so I decide to comment them and change them sometime in the future.

# logger.puts("command: #{delete_command}\nstdout: #{stdout}\nstderr: #{stderr}\nstatus: #{status}\n\n")
# logger.puts("### END remove_cidfile\n\n")
end

def start_container(name, osfamily, _logger = nil)
start_command = if osfamily == 'windows'
run_with_powershell("docker start #{name}")
else
"docker start #{name}"
end
_stdout, _stderr, _status = Open3.capture3(start_command)
# logger.puts("### BEGIN start_container\n\n")
# logger.puts("command: #{start_command}\nstdout: #{stdout}\nstderr: #{stderr}\nstatus: #{status}\n\n")
# logger.puts("### END start_container\n\n")
end

def stop_container(name, osfamily, _logger = nil)
stop_command = if osfamily == 'windows'
run_with_powershell("docker stop #{name}")
else
"docker stop #{name}"
end
_stdout, _stderr, _status = Open3.capture3(stop_command)
# logger.puts("### BEGIN stop_container\n\n")
# logger.puts("command: #{stop_command}\nstdout: #{stdout}\nstderr: #{stderr}\nstatus: #{status}\n\n")
# logger.puts("### END stop_container\n\n")
end

def remove_container(name, osfamily, stop_wait_time, cidfile, logger = nil)
stop_command = if osfamily == 'windows'
run_with_powershell("docker stop --time=#{stop_wait_time} #{name}")
else
"docker stop --time=#{stop_wait_time} #{name}"
end
_stdout, _stderr, _status = Open3.capture3(stop_command)
# logger.puts("### BEGIN remove_container\n\n")
# logger.puts("command: #{stop_command}\nstdout: #{stdout}\nstderr: #{stderr}\nstatus: #{status}\n\n")

remove_command = if osfamily == 'windows'
run_with_powershell("docker rm -v #{name}")
else
"docker rm -v #{name}"
end
_stdout, _stderr, _status = Open3.capture3(remove_command)
# logger.puts("command: #{remove_command}\nstdout: #{stdout}\nstderr: #{stderr}\nstatus: #{status}\n\n")

remove_cidfile(cidfile, osfamily, logger)
# logger.puts("### END remove_container\n\n")
end

def create_container(cmd, osfamily, image, _logger = nil)
# logger.puts("### BEGIN create_container\n\n")
pull_command = if osfamily == 'windows'
run_with_powershell("docker pull #{image} -q")
else
"docker pull #{image} -q"
end
_stdout, _stderr, _status = Open3.capture3(pull_command)
# logger.puts("command: #{pull_command}\nstdout: #{stdout}\nstderr: #{stderr}\nstatus: #{status}\n\n")
create_command = if osfamily == 'windows'
run_with_powershell(cmd)
else
cmd
end
_stdout, _stderr, _status = Open3.capture3(create_command)
# logger.puts("command: #{create_command}\nstdout: #{stdout}\nstderr: #{stderr}\nstatus: #{status}\n\n")
# logger.puts("### END create_container\n\n")
end

def detect_changes(opts) # rubocop:disable Metrics/CyclomaticComplexity,Metrics/PerceivedComplexity
Copy link
Contributor

@sanfrancrisko sanfrancrisko Jan 26, 2021

Choose a reason for hiding this comment

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

This method is quite big - it might be good to refactor some of the code out in to separate methods or potentially create an object to handle the docker inspect output and allow us to query what we want from it a bit easier. E.g.

class DockerInspectInspector
    def initialize(opts, docker_inspect_output)
      @opts = opts
      @docker_inspect_output = JSON.parse(docker_inspect_output)[0]
      @opt_volumes_count = @opts['volumes'].scan(%r{(?=:)}).count
      @os_family_windows = opts['osfamily'] == 'windows'
      @opts_volumes_valid = @opts['volumes'].is_a? String
    end

    def mounts_changed
      return false unless @opts_volumes_valid
      @opts['volumes'].include?(':') && @opts['volumes'] != @docker_inspect_output['Mounts'].to_a[0]
    end

    def volumes_changed
      return false unless @opts_volumes_valid
      !@opts['volumes'].include?(':') && @opts['volumes'] != @docker_inspect_output['Config']['Volumes'].to_a[0]
    end

    def volume_count_equal(i)
      return false unless @opts_volumes_valid
      @opt_volumes_count == i
    end

    def param_changed?
      return true if mounts_changed && !@os_family_windows
      return true if volumes_changed && !@os_family_windows
      return true if volume_count_equal(2) && volumes_changed && !@os_family_windows
      volume_count_equal(1) && volumes_changed && @os_family_windows
    end

  end

...and then we can call the methods within the class to achieve what we want:

stdout, stderr, status = Open3.capture3("docker inspect #{opts['sanitised_title']}")
docker_inspect_inspector = DockerInspectInspector.new(opts, stdout)

param_changed = docker_inspect_inspector.params_changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

That class is a very rough idea - do let me know if you want to go down that road before implementing. It is likely not the best way to do it, but hopefully it serves as a potential idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great idea. I have thought to split into multiple functions, but using OOP seems to be a more readable solution.
I think this implementation will not remain in this state, it will be changed using resource API as @DavidS's recommendation.
I think all improvements will be implemented in a further ticket.
There are many improvements to be done: splitting this big function into small functions(using OOP), using ruby-pwsh,, using journalctl for logging and many other small improvements.

require 'open3'
require 'json'
return_value = 'NO_CHANGE_DETECTED'

# log = File.open("#{opts['logfile_path']}/docker_params.txt", 'a')
# log.puts("### BEGIN detect_changes\n\n")
if opts['sanitised_title'] && opts['osfamily']
stdout, stderr, status = Open3.capture3("docker inspect #{opts['sanitised_title']}")
# log.puts("command: #{"docker inspect #{opts['sanitised_title']}"}\nstdout: #{stdout}\nstderr: #{stderr}\nstatus: #{status}\n\n")
# log.puts("#{opts['sanitised_title']}\n\n")
if stderr.to_s == '' && status.to_s.include?('exit 0')
param_changed = false
inspect_hash = JSON.parse(stdout)[0]

# check if the image was changed
param_changed = true if opts['image'] && opts['image'] != inspect_hash['Config']['Image']

# check if something on volumes or mounts was changed(a new volume/mount was added or removed)
param_changed = true if opts['volumes'].is_a?(String) && opts['volumes'].include?(':') && opts['volumes'] != inspect_hash['Mounts'].to_a[0] && opts['osfamily'] != 'windows'
param_changed = true if opts['volumes'].is_a?(String) && !opts['volumes'].include?(':') && opts['volumes'] != inspect_hash['Config']['Volumes'].to_a[0] && opts['osfamily'] != 'windows'
param_changed = true if opts['volumes'].is_a?(String) && opts['volumes'].scan(%r{(?=:)}).count == 2 && opts['volumes'] != inspect_hash['Mounts'].to_a[0] && opts['osfamily'] == 'windows'
param_changed = if opts['volumes'].is_a?(String) && opts['volumes'].scan(%r{(?=:)}).count == 1 && opts['volumes'] != inspect_hash['Config']['Volumes'].to_a[0] && opts['osfamily'] == 'windows'
true
else
param_changed
end

pp_paths = opts['volumes'].reject { |item| item.include?(':') } if opts['volumes'].is_a?(Array) && opts['osfamily'] != 'windows'
pp_mounts = opts['volumes'].select { |item| item.include?(':') } if opts['volumes'].is_a?(Array) && opts['osfamily'] != 'windows'
pp_paths = opts['volumes'].select { |item| item.scan(%r{(?=:)}).count == 1 } if opts['volumes'].is_a?(Array) && opts['osfamily'] == 'windows'
pp_mounts = opts['volumes'].select { |item| item.scan(%r{(?=:)}).count == 2 } if opts['volumes'].is_a?(Array) && opts['osfamily'] == 'windows'

inspect_paths = if inspect_hash['Config']['Volumes']
inspect_hash['Config']['Volumes'].keys
else
[]
end
param_changed = true if pp_paths != inspect_paths

names = inspect_hash['Mounts'].map { |item| item.values[1] } if inspect_hash['Mounts']
pp_names = pp_mounts.map { |item| item.split(':')[0] } if pp_mounts
names = names.select { |item| pp_names.include?(item) } if names && pp_names
destinations = inspect_hash['Mounts'].map { |item| item.values[3] } if inspect_hash['Mounts']
pp_destinations = pp_mounts.map { |item| item.split(':')[1] } if pp_mounts && opts['osfamily'] != 'windows'
pp_destinations = pp_mounts.map { |item| "#{item.split(':')[1].downcase}:#{item.split(':')[2]}" } if pp_mounts && opts['osfamily'] == 'windows'
destinations = destinations.select { |item| pp_destinations.include?(item) } if destinations && pp_destinations

param_changed = true if pp_names != names
param_changed = true if pp_destinations != destinations
param_changed = true if pp_mounts != [] && inspect_hash['Mounts'].nil?

# check if something on ports was changed(some ports were added or removed)

ports = inspect_hash['HostConfig']['PortBindings'].keys
ports = ports.map { |item| item.split('/')[0] }
pp_ports = opts['ports'].sort if opts['ports'].is_a?(Array)
pp_ports = [opts['ports']] if opts['ports'].is_a?(String)

param_changed = true if pp_ports && pp_ports != ports

if param_changed
remove_container(opts['sanitised_title'], opts['osfamily'], opts['stop_wait_time'], opts['cidfile'])
create_container(opts['command'], opts['osfamily'], opts['image'])
return_value = 'PARAM_CHANGED'
# log.puts(return_value)
end
else
create_container(opts['command'], opts['osfamily'], opts['image']) unless File.exist?(opts['cidfile'])
_stdout, _stderr, status = Open3.capture3("docker inspect #{opts['sanitised_title']}")
# log.puts("command: #{"docker inspect #{opts['sanitised_title']}"}\nstdout: #{stdout}\nstderr: #{stderr}\nstatus: #{status}\n\n")
unless status.to_s.include?('exit 0')
remove_cidfile(opts['cidfile'], opts['osfamily'])
create_container(opts['command'], opts['osfamily'], opts['image'])
end
return_value = 'NO_CHANGE_DETECTED'
# log.puts(return_value)
end
else
return_value = 'ARG_REQUIRED_MISSING'
# log.puts(return_value)
end

if opts['container_running']
start_container(opts['sanitised_title'], opts['osfamily'])
else
stop_container(opts['sanitised_title'], opts['osfamily'])
end

# log.puts(return_value)
# log.puts("### END detect_changes\n\n")
# log.close
return_value
end
end
78 changes: 0 additions & 78 deletions lib/puppet/parser/functions/docker_params_changed.rb

This file was deleted.

100 changes: 34 additions & 66 deletions manifests/run.pp
Original file line number Diff line number Diff line change
Expand Up @@ -414,93 +414,61 @@
$inspect = [ "${docker_command} inspect ${sanitised_title}", ]

if $custom_unless {
$exec_unless = $custom_unless
$exec_unless = concat($custom_unless, $inspect)
} else {
$exec_unless = $inspect
}

$detect_changes = docker_params_changed(
{
sanitised_title => $sanitised_title,
osfamily => $facts['os']['family'],
image => $image,
volumes => $volumes,
ports => $ports,
if versioncmp($facts['puppetversion'], '6') < 0 {
exec { "run ${title} with docker":
command => join($run_with_docker_command, ' '),
unless => $exec_unless,
environment => $exec_environment,
path => $exec_path,
provider => $exec_provider,
timeout => $exec_timeout,
}
)

case $detect_changes {
'CONTAINER_NOT_FOUND': {
exec { "run ${title} with docker":
command => join($run_with_docker_command, ' '),
unless => $exec_unless,
environment => $exec_environment,
path => $exec_path,
provider => $exec_provider,
timeout => $exec_timeout,
}
}
'PARAM_CHANGED': {
if $running == false {
exec { "stop ${title} with docker":
command => "${docker_command} stop --time=${stop_wait_time} ${sanitised_title}",
onlyif => "${docker_command} inspect ${sanitised_title}",
onlyif => $container_running_check,
environment => $exec_environment,
path => $exec_path,
provider => $exec_provider,
timeout => $exec_timeout,
}

exec { "remove ${title} with docker":
command => "${docker_command} rm -v ${sanitised_title}",
onlyif => "${docker_command} inspect ${sanitised_title}",
environment => $exec_environment,
path => $exec_path,
provider => $exec_provider,
timeout => $exec_timeout,
}

file { $cidfile:
ensure => absent,
}

exec { "run ${title} with docker":
command => join($run_with_docker_command, ' '),
unless => $exec_unless,
} else {
exec { "start ${title} with docker":
command => "${docker_command} start ${sanitised_title}",
unless => $container_running_check,
environment => $exec_environment,
path => $exec_path,
provider => $exec_provider,
timeout => $exec_timeout,
}
}
'ARG_REQUIRED_MISSING': {
fail(translate('sanitised_title and osfamily are required for docker_params_changed function'))
}
default: {
# Use ONLY on debugging - DO NOT UNCOMMENT OTHERWISE
# notify { 'this case should not be executed':
# message => "detect_changes: ${detect_changes}",
# withpath => true,
# }
} else {
$docker_params_changed_args = {
sanitised_title => $sanitised_title,
osfamily => $facts['os']['family'],
command => join($run_with_docker_command, ' '),
cidfile => $cidfile,
image => $image,
volumes => $volumes,
ports => $ports,
stop_wait_time => $stop_wait_time,
container_running => $running,
# logfile_path => ($facts['os']['family'] == 'windows') ? {
# true => ::docker_user_temp_path,
# default => '/tmp',
# },
}
}

if $running == false {
exec { "stop ${title} with docker":
command => "${docker_command} stop --time=${stop_wait_time} ${sanitised_title}",
onlyif => $container_running_check,
environment => $exec_environment,
path => $exec_path,
provider => $exec_provider,
timeout => $exec_timeout,
}
} else {
exec { "start ${title} with docker":
command => "${docker_command} start ${sanitised_title}",
unless => $container_running_check,
environment => $exec_environment,
path => $exec_path,
provider => $exec_provider,
timeout => $exec_timeout,
$detect_changes = Deferred('docker_params_changed', [$docker_params_changed_args])

notify { 'docker_params_changed':
message => $detect_changes,
}
}
}
Expand Down
Loading