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

Fix idempotency when using scaling with docker-compose #805

Merged
merged 12 commits into from
Apr 8, 2022
Merged

Fix idempotency when using scaling with docker-compose #805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5681f0f
Update ruby.rb
canihavethisone Mar 23, 2022
4529099
Images must be built before compose
canihavethisone Mar 23, 2022
6755b38
indentation
canihavethisone Mar 23, 2022
c688137
Fix docker-compose /tmp issue with new dir and var
canihavethisone Mar 24, 2022
480c36d
Update ruby.rb
canihavethisone Mar 28, 2022
8588359
Update ruby.rb
canihavethisone Mar 30, 2022
e565d5b
Update compose_v3_spec.rb
canihavethisone Mar 30, 2022
6f48fa2
Update ruby.rb
canihavethisone Mar 30, 2022
00d09d4
Stub /tmp_docker dir to prevent shelling out during spec test
canihavethisone Apr 8, 2022
521d895
Fix lint, remove unnecessary code in stub
canihavethisone Apr 8, 2022
8a6d086
Moved directory stub and added to registry_spec
canihavethisone Apr 8, 2022
c551270
fix spacing
canihavethisone Apr 8, 2022
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
10 changes: 7 additions & 3 deletions lib/puppet/provider/docker_compose/ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@
environment(HOME: '/root')
end

has_command(:docker_compose, command(:dockercompose)) do
Dir.mkdir('/tmp_docker') unless Dir.exist?('/tmp_docker')
Copy link
Contributor

@kenyon kenyon Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm getting permission denied from this, even when puppet agent is running as root. Besides that, it seems wrong to be creating this directory in the root of the filesystem. Also, what is this for? The TMPDIR env var is never used in this code. Seems like this is here by mistake for the tests only. Bug report: #819

Copy link
Contributor Author

@canihavethisone canihavethisone Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Unsure why you are getting permission denied, it works for me IRL and also passes acceptance tests. What OS are you using?
  2. /tmp_docker is in the base path so that it is adjacent to /tmp, which is the dir that docker-compose uses by default. What location do you suggest?
  3. the $TMPDIR env var is respected by the Python underlying docker-compose. Using a path other than /tmp and setting that var overcomes the well-documented issue of docker-compose failing to run when noexec is set on /tmp. The change is there to overcome this and not for testing purposes only.

I have requested investigation into the reports of this issue

Copy link
Contributor

Choose a reason for hiding this comment

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

On Ubuntu. What is the issue that this pull request is trying to address?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the problem is that this code is running on the Puppet Server, not the agent, because the error occurs during catalog compilation. I can't find any evidence of this error other than during the puppet agent run, so I'm not sure why it's getting permission denied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it would be good if this were somehow made optional, so that we don't end up with /tmp_docker on every system, even where docker isn't used.

Copy link
Contributor Author

@canihavethisone canihavethisone Apr 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@kenyon to address your comments:

  • the pull request addresses the known issue with docker-compose executing from /tmp as also reported at /tmp directory requires "exec" mode for docker-compose execution docker/compose#8041
  • I agree that the issue, while appearing only on the first puppet run, may be trying to apply the dir on the server. It may be linked to other reported issues I referenced on your issue raised as a result of this unexpected outcome
  • the actual creation of the dir only occurs if resources are applied by docker_compose, and will not actually occur otherwise. Further, if you have this module on your puppetserver but not applied to nodes, they do not appear to be affected.

I agree that it is both unexpected and needs to be addressed, and have reported more detailed observations in your raised issue and also requested maintainer input.

Thank you for reporting the issue.

ENV.store('TMPDIR', '/tmp_docker')
end

def exists?
Puppet.info("Checking for compose project #{name}")
compose_services = {}
Expand All @@ -31,18 +36,17 @@ def exists?
"label=com.docker.compose.project=#{name}",
]).split("\n")
compose_containers.push(*containers)
compose_containers.uniq!

compose_services = compose_output['services']

if compose_services.count != compose_containers.count
if compose_services.count != compose_containers.uniq.count
return false
end

counts = Hash[*compose_services.each.map { |key, array|
image = (array['image']) ? array['image'] : get_image(key, compose_services)
Puppet.info("Checking for compose service #{key} #{image}")
["#{key}-#{image}", compose_containers.count("#{key}-#{image}")]
[key, compose_containers.count("#{key}-#{image}")]
}.flatten]

# No containers found for the project
Expand Down
1 change: 1 addition & 0 deletions manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,7 @@
-> Docker::Registry <||>
-> Docker::Image <||>
-> Docker::Run <||>
-> Docker_compose <||>
} else {
contain 'docker::repos'
contain 'docker::install'
Expand Down
10 changes: 8 additions & 2 deletions spec/acceptance/compose_v3_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,11 @@ class { 'docker::compose':
let(:install_pp) do
<<-MANIFEST
docker_compose { 'web':
compose_files => ['#{tmp_path}/docker-compose-v3.yml'],
ensure => present,
compose_files => ['#{tmp_path}/docker-compose-v3.yml'],
ensure => present,
scale => {
compose_test => 2
}
}
MANIFEST
end
Expand All @@ -66,6 +69,9 @@ class { 'docker::compose':
docker_compose { 'web1':
compose_files => ['#{tmp_path}/docker-compose-v3.yml', '#{tmp_path}/docker-compose-override-v3.yml'],
ensure => present,
scale => {
compose_test => 2
}
}
MANIFEST

Expand Down
6 changes: 6 additions & 0 deletions spec/classes/init_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,12 @@
}
else
it {
# Stub /tmp_docker dir to prevent shelling out during spec test
Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is that these are hiding real errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This stub/mock is to address the spec test only, not acceptance tests (which passed)

allow(Dir).to receive(:exist?).and_wrap_original do |original_method, a|
original_method.call(a)
end
allow(Dir).to receive(:exist?).with('/tmp_docker').and_return(true)

is_expected.to contain_class('docker::repos').that_comes_before('Class[docker::install]')
is_expected.to contain_class('docker::install').that_comes_before('Class[docker::config]')
is_expected.to contain_class('docker::config').that_comes_before('Class[docker::service]')
Expand Down
9 changes: 9 additions & 0 deletions spec/defines/registry_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,15 @@

require 'spec_helper'

# Stub /tmp_docker dir to prevent shelling out during spec test
class Dir
class << self
def exist?(var)
return true if var == '/tmp_docker'
end
end
end

tests = {
'with ensure => absent' => {
'ensure' => 'absent',
Expand Down
9 changes: 9 additions & 0 deletions spec/unit/lib/puppet/type/docker_compose_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,15 @@

require 'spec_helper'

# Stub /tmp_docker dir to prevent shelling out during spec test
class Dir
class << self
def exist?(var)
return true if var == '/tmp_docker'
end
end
end

compose = Puppet::Type.type(:docker_compose)

describe compose do
Expand Down