Avoid potential dependency cycles when used with other modules #463

remicalixte · 2018-10-22T16:36:54Z

Hopefully fixes the dependency cycle

truthbk

ensure_packages I believe is part of the puppet stdlib (see: https://github.com/puppetlabs/puppetlabs-stdlib#ensure_packages)... can you elaborate a little on how it creates a dependency cycle? Could you also add to the PR what the dependency cycle is?

Thanks!

remicalixte · 2018-10-23T15:42:39Z

We require Package[apt-transport-https] to run Class[apt_update] but some other packages require the opposite, creating a dependency cycle. I know it is very ugly to install this package like this but I can't find any other solution. However, I can't reproduce the issue myself so I had to deduce this from some logs, thus I might be wrong

remicalixte · 2018-10-25T15:39:24Z

The dependency cycle is Exec[apt_update] => Class[Apt::Update] => Package[apt-transport-https] => Apt::Source[datadog6] => Apt::Setting[list-datadog6] => File[/etc/apt/sources.list.d/datadog6.list] => Class[Apt::Update] => Exec[apt_update]

remicalixte · 2018-10-31T21:57:11Z

@truthbk

Dependency graph before:
expanded_relationships.pdf
Dependency graph after:
expanded_relationships.pdf

Since we don't use Package[apt-transport-https] anymore the Package[apt-transport-https] => Apt::Source[datadog6] link is gone and it should break the cycle.

Some external module is adding the Class[Apt::Update] => Package[apt-transport-https] link (Which is really weird) to the graph and causing the cycle. We have to break the cycle on the part of the graph we can control.

remicalixte · 2018-11-14T17:07:35Z

@truthbk I've removed some unrelated changes and made the commit messages clearer. Hopefully we can merge this soon and release a new version of the puppet module

truthbk

Some notes, in general the PR looks good, but if possible I would like to keep ensure_packages() around as a sane, standard way to ensure some required packages are available, I think that would be preferable. If you are sure we can't use it due to the dependency cycle, I understand.

We also need to make sure these changes don't make the module chatty, we don't want to be calling apt-get update every time the manifest runs.

Finally, I am unable to spot the cycle in the first PDF. Being a DAG I see no actual loops in the graph, but I may be missing something as I see repeated node names.

truthbk · 2018-12-19T20:04:52Z

manifests/ubuntu/agent6.pp

-      before => Apt::Source['datadog6'],
+    $key = {
+      'id' => $apt_key,
+      'server' => $apt_keyserver
    }
  }


I can't remember if it's necessary to do this in case $skip_apt_key_trusting is true.

else { $key = {} }

I guess it doesn't hurt to add it, and it is a bit more explicit

manifests/ubuntu/agent6.pp

truthbk · 2018-12-19T20:07:33Z

manifests/ubuntu/agent6.pp

@@ -16,11 +16,14 @@
  Optional[String] $apt_keyserver = undef,
 ) inherits datadog_agent::params {

-  ensure_packages(['apt-transport-https'])
+  exec { 'apt-transport-https':


I would like to continue using ensure_packages to be honest. I'm not sure how it would cause a cycle. If you're positive it's partly to blame we can go ahead and do this, but I'd rather not. Also, the line replaced should probably be:

ensure_packages(['apt-transport-https'], {'ensure' => 'present'})

truthbk · 2018-12-19T20:11:42Z

manifests/ubuntu/agent6.pp

-    require  => Package['apt-transport-https'],
-    notify   =>  Exec['apt_update'],
+    key      => $key,
+    require  => Exec['apt-transport-https'],


If we decide to keep ensure_packages, we could probably remove this line as ensure_packages would do its thing and ensure the package is available. I wonder if the reason this was here was due to the order in which puppet applied these statements, we'll have to test that out.

manifests/ubuntu/agent6.pp

manifests/ubuntu/install_key.pp

truthbk · 2018-12-19T20:15:35Z

manifests/ubuntu/agent5.pp

-    require  => Package['apt-transport-https'],
-    notify   => [Exec['datadog_apt-get_remove_agent6'],Exec['apt_update']],
+    key      => $key,
+    require  => Exec['apt-transport-https'],


As mentioned for A6, I would like to keep ensure_packages(), unless it's 100% clear that we cannot, and just remove this require.

truthbk · 2018-12-19T20:44:53Z

Hoping to address: #423

…ithub.com/puppetlabs/puppetlabs-apt\#adding-new-sources-or-ppas

… with some other modules

#463)" This reverts commit 8a29860.

#463)" (#506) This reverts commit 8a29860.

…og#463) * Use native pgp key support of the apt module * apt::source notify apt_update by default * require Class['apt::update'] as the documentation suggests: https://github.com/puppetlabs/puppetlabs-apt\#adding-new-sources-or-ppas * Use 'exec' instead of 'ensure_packages' to break a dependencies cycle with some other modules * Fix spec tests * Set $key to empty dict if skipping apt key trusting

DataDog#463)" (DataDog#506) This reverts commit 8a29860.

truthbk reviewed Oct 23, 2018

View reviewed changes

remicalixte force-pushed the remicalixte/puppet-dependency-cycle branch 2 times, most recently from 1ecda90 to d9bb827 Compare October 23, 2018 18:21

remicalixte force-pushed the remicalixte/puppet-dependency-cycle branch from d9bb827 to 207a6c1 Compare November 14, 2018 17:00

remicalixte changed the title ~~Some improvements for the puppet module~~ Avoid potential dependency cycles when used with other modules Nov 14, 2018

remicalixte force-pushed the remicalixte/puppet-dependency-cycle branch from 207a6c1 to 42c1ea0 Compare November 14, 2018 17:06

remicalixte force-pushed the remicalixte/puppet-dependency-cycle branch 2 times, most recently from 134765b to 685e919 Compare November 15, 2018 21:11

truthbk reviewed Dec 19, 2018

View reviewed changes

remicalixte added 6 commits December 19, 2018 16:13

Use native pgp key support of the apt module

c65793b

apt::source notify apt_update by default

20fb5ab

require Class['apt::update'] as the documentation suggests: https://g…

4d5d8a6

…ithub.com/puppetlabs/puppetlabs-apt\#adding-new-sources-or-ppas

Use 'exec' instead of 'ensure_packages' to break a dependencies cycle…

d6c82dd

… with some other modules

Fix spec tests

c5fceb7

Set $key to empty dict if skipping apt key trusting

7ef196e

remicalixte force-pushed the remicalixte/puppet-dependency-cycle branch from 685e919 to 7ef196e Compare December 19, 2018 21:19

truthbk added this to the 2.4.0 milestone Dec 20, 2018

Merge branch 'master' into remicalixte/puppet-dependency-cycle

0d1596f

truthbk merged commit 8a29860 into master Dec 27, 2018

truthbk deleted the remicalixte/puppet-dependency-cycle branch December 27, 2018 12:34

truthbk mentioned this pull request Jan 29, 2019

[2.4.0] apt-get install -y -q apt-transport-https always executed #491

Closed

truthbk added a commit that referenced this pull request Mar 21, 2019

Revert "Avoid potential dependency cycles when used with other modules (

9015bee

#463)" This reverts commit 8a29860.

truthbk mentioned this pull request Mar 21, 2019

Revert "Avoid potential dependency cycles when used with other modules" #506

Merged

truthbk added a commit that referenced this pull request Mar 21, 2019

Revert "Avoid potential dependency cycles when used with other modules (

e8f3f6d

#463)" (#506) This reverts commit 8a29860.

cegeka-jenkins pushed a commit to cegeka/puppet-datadog_agent that referenced this pull request Apr 6, 2020

Revert "Avoid potential dependency cycles when used with other modules (

1a362c5

DataDog#463)" (DataDog#506) This reverts commit 8a29860.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid potential dependency cycles when used with other modules #463

Avoid potential dependency cycles when used with other modules #463

remicalixte commented Oct 22, 2018

truthbk left a comment

remicalixte commented Oct 23, 2018

remicalixte commented Oct 25, 2018

remicalixte commented Oct 31, 2018 •

edited

Loading

remicalixte commented Nov 14, 2018

truthbk left a comment

truthbk Dec 19, 2018

remicalixte Dec 19, 2018

truthbk Dec 19, 2018

truthbk Dec 19, 2018

truthbk Dec 19, 2018

truthbk commented Dec 19, 2018

Avoid potential dependency cycles when used with other modules #463

Avoid potential dependency cycles when used with other modules #463

Conversation

remicalixte commented Oct 22, 2018

truthbk left a comment

Choose a reason for hiding this comment

remicalixte commented Oct 23, 2018

remicalixte commented Oct 25, 2018

remicalixte commented Oct 31, 2018 • edited Loading

remicalixte commented Nov 14, 2018

truthbk left a comment

Choose a reason for hiding this comment

truthbk Dec 19, 2018

Choose a reason for hiding this comment

remicalixte Dec 19, 2018

Choose a reason for hiding this comment

truthbk Dec 19, 2018

Choose a reason for hiding this comment

truthbk Dec 19, 2018

Choose a reason for hiding this comment

truthbk Dec 19, 2018

Choose a reason for hiding this comment

truthbk commented Dec 19, 2018

remicalixte commented Oct 31, 2018 •

edited

Loading