Fix idempotency when using scaling with docker-compose #805

canihavethisone · 2022-03-23T11:28:58Z

No description provided.

CLAassistant · 2022-03-23T11:37:33Z

All committers have signed the CLA.

in case they are referenced, otherwise compose update will fail on first run

canihavethisone

fixed lint

canihavethisone · 2022-03-29T06:23:17Z

Can I get tests run on this again please? I have fixed the lint, but am also looking at the acceptance test as it doesn't include scaling for docker-compose. Will look at that unless someone more familiar with this module can do it faster.

puppet-community-rangefinder · 2022-03-29T07:00:21Z

docker is a class

Breaking changes to this file WILL impact these 17 modules (exact match):

Breaking changes to this file MAY impact these 24 modules (near match):

This module is declared in 6 of 579 indexed public `Puppetfiles`.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

canihavethisone · 2022-03-29T07:47:22Z

There appears to be the error below on some tests. Any insight on if this needs to be stubbed or managed otherwise? It works IRL.

#   Permission denied @ dir_s_mkdir - /tmp_docker
#   ./lib/puppet/provider/docker_compose/ruby.rb:17:in `mkdir'

canihavethisone · 2022-03-30T00:02:34Z

I have tried using the fileutils resource as an alternative, however the above issue still occurs, and only in testing. My guess is that the test is trying to create the /tmp_docker folder on the testing host, and failing.

I have changed the tmp_docker path with removed leading slash, on production Linux hosts this now creates under /root, unsure if I am ok with this and where it would be created on Windows hosts (opinions appreciated). However it now passes spec tests locally.

I have also added scaling to the acceptance tests, however am unable to run this locally, so if a mod could please rerun the automated tests and provide advice on acceptability to have the TMPDIR variable pointing to /root/tmp_docker, or how to otherwise solve this.

I have tried stubbing the existence of the /tmp_docker folder as below, but cannot get it to work.

Allow(Dir).to receive(:exist?).with('/tmp_docker').and_return(true)

add scale to docker_compose acceptance test

chelnak · 2022-03-30T08:25:46Z

@canihavethisone Hey thanks! I think i'm with you regarding the root directory.. maybe we should aim for a safer directory.

@david22swan Have you seen anything like this before?

canihavethisone · 2022-03-30T09:23:21Z

@chelnak thanks for assisting with this. I spent ages trying to mock/stub rspec to not create the /tmp_docker folder during spec test, I think that whatever folder is used we don't want rspec shelling out to actually create it! I can't get a stub to work, but feel it is the way to get it passing.

In the meantime I have changed the path back to /tmp_docker as I see the non-absolute path failed in acceptance tests. At this stage I will leave it to the maintainers as I'm a bit out of ideas. Again, it (the /tmp_docker path) works IRL, at least on Linux.

spec tests now pass

canihavethisone · 2022-04-08T08:13:57Z

Now added a successful stub for /tmp_docker. Can you please re-run tests, and merge if acceptable. Thanks.

chelnak · 2022-04-08T08:23:39Z

@canihavethisone Thanks for this. Tests are running!

canihavethisone · 2022-04-08T08:40:22Z

@chelnak : Interesting, it looks like the spec test for define docker::registry is failing now. While the stub could be added to that test, I am unsure why it is referencing the mkdir from the compose?

chelnak · 2022-04-08T08:46:24Z

I'm not sure off the top of my head. I'd have to pull the code down and take a look. I'll try and get to that today but I'll also ping this to the team. Someone might be able to spot something.

canihavethisone · 2022-04-08T08:48:09Z

Thanks. I'm running spec tests with 'pdk test unit' and that wasn't failing locally, perhaps it's only running tests in /spec and not /functions

chelnak · 2022-04-08T09:06:24Z

Thats some good info.. It looks like the spec tests are run with bundle exec rake parallel_spec in GHA.

Can you try that and let me know the outcome?

canihavethisone · 2022-04-08T10:26:35Z

@chelnak thanks for the parallel_spec command. I have got all spec tests to pass now by:

moving the stub from the spec to the unit/lib/puppet/type test
adding another stub to the init spec
adding the stub to the defines/registry_spec.rb <--- still unsure why the registry define needed the dir stub!

Looks like it should be fine now, although I did see one of the acceptance tests fail on connection error previously which seems unrelated. Boy, I don't like the way rspec stub work!

chelnak · 2022-04-08T10:30:34Z

Great job! Thanks for putting in the work!!

canihavethisone · 2022-04-08T10:33:33Z

No worries. I'm actually fixing 3 issues in this merge request:

the known 'docker_compose can't run execs in /tmp unless remounted' issue
images now compiled before docker_compose runs
scaling in docker compose wasn't idempotent, or tested in acceptance

I appreciate the opportunity to get this working. A colleague actually helped me with the stub syntax as I was stuck.

canihavethisone · 2022-04-08T11:07:23Z

@chelnak Passed! :)

chelnak · 2022-04-08T11:09:22Z

Thanks for your contribution! 🚀

kenyon · 2022-04-11T16:29:22Z

lib/puppet/provider/docker_compose/ruby.rb

@@ -13,6 +13,11 @@
    environment(HOME: '/root')
  end

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


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

Unsure why you are getting permission denied, it works for me IRL and also passes acceptance tests. What OS are you using?

/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?

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

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

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.

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.

@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.

kenyon · 2022-04-11T22:45:35Z

spec/classes/init_spec.rb

@@ -196,6 +196,12 @@
              }
            else
              it {
+                # Stub /tmp_docker dir to prevent shelling out during spec test


My guess is that these are hiding real errors.

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

Update ruby.rb

5681f0f

canihavethisone requested a review from a team as a code owner March 23, 2022 11:28

canihavethisone added 3 commits March 24, 2022 10:26

Images must be built before compose

4529099

in case they are referenced, otherwise compose update will fail on first run

indentation

6755b38

Fix docker-compose /tmp issue with new dir and var

c688137

Aaron-Rob added the community label Mar 24, 2022

Update ruby.rb

480c36d

canihavethisone commented Mar 28, 2022

View reviewed changes

chelnak closed this Mar 29, 2022

chelnak reopened this Mar 29, 2022

canihavethisone added 2 commits March 30, 2022 12:12

Update ruby.rb

8588359

Update compose_v3_spec.rb

e565d5b

add scale to docker_compose acceptance test

canihavethisone added 2 commits March 30, 2022 20:27

Update ruby.rb

6f48fa2

Stub /tmp_docker dir to prevent shelling out during spec test

00d09d4

spec tests now pass

canihavethisone added 2 commits April 8, 2022 19:09

Fix lint, remove unnecessary code in stub

521d895

Moved directory stub and added to registry_spec

8a6d086

fix spacing

c551270

chelnak approved these changes Apr 8, 2022

View reviewed changes

chelnak merged commit 4819ea7 into puppetlabs:main Apr 8, 2022

david22swan added the bugfix label Apr 11, 2022

kenyon reviewed Apr 11, 2022

View reviewed changes

kenyon mentioned this pull request Apr 11, 2022

Permission denied from mkdir /tmp_docker in v4.2.0 #819

Closed

kenyon reviewed Apr 11, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix idempotency when using scaling with docker-compose #805

Fix idempotency when using scaling with docker-compose #805

canihavethisone commented Mar 23, 2022

CLAassistant commented Mar 23, 2022 •

edited

Loading

canihavethisone left a comment •

edited

Loading

canihavethisone commented Mar 29, 2022

puppet-community-rangefinder bot commented Mar 29, 2022

canihavethisone commented Mar 29, 2022 •

edited

Loading

canihavethisone commented Mar 30, 2022 •

edited

Loading

chelnak commented Mar 30, 2022

canihavethisone commented Mar 30, 2022 •

edited

Loading

canihavethisone commented Apr 8, 2022 •

edited

Loading

chelnak commented Apr 8, 2022

canihavethisone commented Apr 8, 2022

chelnak commented Apr 8, 2022

canihavethisone commented Apr 8, 2022

chelnak commented Apr 8, 2022

canihavethisone commented Apr 8, 2022 •

edited

Loading

chelnak commented Apr 8, 2022

canihavethisone commented Apr 8, 2022 •

edited

Loading

canihavethisone commented Apr 8, 2022

chelnak commented Apr 8, 2022

kenyon Apr 11, 2022 •

edited

Loading

canihavethisone Apr 11, 2022 •

edited

Loading

kenyon Apr 11, 2022

kenyon Apr 11, 2022

kenyon Apr 11, 2022

canihavethisone Apr 12, 2022 •

edited

Loading

kenyon Apr 11, 2022

canihavethisone Apr 12, 2022

Fix idempotency when using scaling with docker-compose #805

Fix idempotency when using scaling with docker-compose #805

Conversation

canihavethisone commented Mar 23, 2022

CLAassistant commented Mar 23, 2022 • edited Loading

canihavethisone left a comment • edited Loading

Choose a reason for hiding this comment

canihavethisone commented Mar 29, 2022

puppet-community-rangefinder bot commented Mar 29, 2022

docker is a class

This module is declared in 6 of 579 indexed public Puppetfiles.

canihavethisone commented Mar 29, 2022 • edited Loading

canihavethisone commented Mar 30, 2022 • edited Loading

chelnak commented Mar 30, 2022

canihavethisone commented Mar 30, 2022 • edited Loading

canihavethisone commented Apr 8, 2022 • edited Loading

chelnak commented Apr 8, 2022

canihavethisone commented Apr 8, 2022

chelnak commented Apr 8, 2022

canihavethisone commented Apr 8, 2022

chelnak commented Apr 8, 2022

canihavethisone commented Apr 8, 2022 • edited Loading

chelnak commented Apr 8, 2022

canihavethisone commented Apr 8, 2022 • edited Loading

canihavethisone commented Apr 8, 2022

chelnak commented Apr 8, 2022

kenyon Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

canihavethisone Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

kenyon Apr 11, 2022

Choose a reason for hiding this comment

kenyon Apr 11, 2022

Choose a reason for hiding this comment

kenyon Apr 11, 2022

Choose a reason for hiding this comment

canihavethisone Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

kenyon Apr 11, 2022

Choose a reason for hiding this comment

canihavethisone Apr 12, 2022

Choose a reason for hiding this comment

CLAassistant commented Mar 23, 2022 •

edited

Loading

canihavethisone left a comment •

edited

Loading

This module is declared in 6 of 579 indexed public `Puppetfiles`.

canihavethisone commented Mar 29, 2022 •

edited

Loading

canihavethisone commented Mar 30, 2022 •

edited

Loading

canihavethisone commented Mar 30, 2022 •

edited

Loading

canihavethisone commented Apr 8, 2022 •

edited

Loading

canihavethisone commented Apr 8, 2022 •

edited

Loading

canihavethisone commented Apr 8, 2022 •

edited

Loading

kenyon Apr 11, 2022 •

edited

Loading

canihavethisone Apr 11, 2022 •

edited

Loading

canihavethisone Apr 12, 2022 •

edited

Loading