[#1058] Serially deploy modules that share a cachedir #1059

justinstoller · 2020-05-01T21:49:39Z

When we raised the default threadpool for deploying modules above 1, we
began to see issues when caching a remote locally if that remote/cache
was shared by multiple modules within the Puppetfile.

To resolve this, when we create the module we group it with any other
module that may share its cachedir. We then pull modules by cachedir off
of the queue and serially process those.

Please add all notable changes to the "Unreleased" section of the CHANGELOG.

When we raised the default threadpool for deploying modules above 1, we began to see issues when caching a remote locally if that remote/cache was shared by multiple modules within the Puppetfile. To resolve this, when we create the module we group it with any other module that may share its cachedir. We then pull modules by cachedir off of the queue and serially process those.

Magisus

This looks like it'll do the trick, while maintaining as much parallelism as is safe.

Magisus · 2020-05-05T15:27:53Z

I heard plans to add an acceptance test, which I think we should do. Was that going to be added to this PR? Or should we add this in, use the existing test that failed because of this as-is to verify it, and add the new one later (in the interest of the release timeline)?

mwaggett · 2020-05-05T17:20:14Z

spec/unit/puppetfile_spec.rb

+      mod2 = spy('module2')
+      mod3 = spy('module3')
+      mod4 = spy('module4')
+      mod5 = spy('module4')


is this intentionally still 'module4'? or should it be 5?

I think it doesn't really matter, since calling spy will still create unique objects regardless of that string. But I'll update it.

mwaggett · 2020-05-05T17:20:56Z

spec/unit/puppetfile_spec.rb

+
+      queue = subject.modules_queue(visitor)
+      expect(queue.length).to be 4
+      queue_array = 4.times.map { queue.pop }


Can this be length.times.map instead of hard-coding 4?

mwaggett · 2020-05-05T17:22:44Z

If we're in a rush, I'm okay using the existing test, but I do think we should ultimately have one that's explicitly testing this. I'll leave it up to @justinstoller to decide whether it can be done quick enough to be included here.

adrienthebo · 2020-05-05T21:01:52Z

I was under the mistaken assumption that serializing access to caches had been resolved as part of the parallel modules pull request. Given that this is still an issue we may want to consider dropping the default pool_size back to 1 while we verify that parallelization is safe. Part of this concern is that we have the WithModules environment that has another entrypoint into interacting with modules that this pull request might not account for.

justinstoller · 2020-05-05T22:06:53Z

How about this? #1060

justinstoller · 2020-05-05T22:13:03Z

Ugh, it looks like the WithModules implementation maybe just copied and pasted code from the Puppetfile implementation? We probably should refactor out that shared code.

After updating the default pool_size, a race condition was found when initializing our local git cache. See puppetlabs#1058 for the issue in detail. When attempting a fix we realized that there were multiple copies of the code to iterate over modules to manage. See puppetlabs#1059 for a first attempt at this problem and discussion. In order to meet our current timelines, the default will need to reverted until we can fix this issue properly. Hence this commit and a55536c. This commit was necessary because the CHANGELOG has been refactored since the commit referencing the default was added.

adrienthebo · 2020-05-05T21:03:25Z

lib/r10k/puppetfile.rb

@@ -180,6 +189,19 @@ def accept(visitor)

  private

+  def module_vcs_cachedir(mod)


As this method deals with properties of modules, we can push this detail down into the modules themselves instead of probing objects for methods and acting from there.

adrienthebo · 2020-05-05T21:03:42Z

lib/r10k/puppetfile.rb

@@ -212,15 +234,22 @@ def concurrent_accept(visitor, pool_size)
  def modules_queue(visitor)
    Queue.new.tap do |queue|
      visitor.visit(:puppetfile, self) do
-        modules.each { |mod| queue << mod }
+        modules_by_cachedir = modules_by_vcs_cachedir.clone


This might have some interesting interactions with https://github.com/puppetlabs/r10k/blob/master/lib/r10k/environment/with_modules.rb#L55-L64.

I would feel safer if this was addressed before the PR is merged. Just to avoid any potential unforeseen issues.

I may be missing something, but I'm not sure how this would matter in WithModules. That class doesn't have a concurrent_accept mode so far as I can tell, so its modules would be processed first, always in serial, and then the Puppetfile would be processed and this logic would handle the potential conflict.

I can't find any other other places that respect pool_size besides what's in the Puppetfile here. What did y'all think was potentially problematic?

dhollinger

I don't really have anything additional to add that @adrienthebo and @mwaggett haven't already pointed out. I would like to see the potential interaction with WithModules to be solved before merging though

justinstoller · 2020-07-02T18:43:58Z

Yeah, sorry, I didn't mean to ghost, y'all. When we thought we could make this in before a deadline we gave it a go, and when it became apparent we weren't going to make it for that deadline it dropped way down our priority list.

Magisus · 2020-07-23T23:47:55Z

I don't appear to be able to push to this branch. Is that something @justinstoller can change, or do I need to open a new PR to pick this up?

justinstoller · 2020-07-24T17:42:20Z

I pushed this code to this branch so anyone can take it over: https://github.com/puppetlabs/r10k/tree/GH-1058_dont-concurrently-modify-same-cache

Magisus · 2020-07-29T17:00:09Z

Superseded by #1081

justinstoller requested review from adrienthebo and dhollinger as code owners May 1, 2020 21:49

justinstoller requested a review from a team May 1, 2020 21:49

justinstoller force-pushed the GH-1058_dont-concurrently-modify-same-cache branch from a58c621 to ffe111c Compare May 2, 2020 00:04

justinstoller mentioned this pull request May 2, 2020

Git cache breaks with pool_size > 1 if managing the same git source twice #1058

Closed

Magisus approved these changes May 5, 2020

View reviewed changes

mwaggett reviewed May 5, 2020

View reviewed changes

justinstoller mentioned this pull request May 6, 2020

Revert increase in default pool size #1060

Merged

adrienthebo reviewed May 6, 2020

View reviewed changes

dhollinger reviewed Jul 2, 2020

View reviewed changes

This was referenced Jul 24, 2020

[#1058] Avoid concurrent cache updates when downloading modules on multiple threads #1080

Closed

[#1058] Avoid concurrent cache updates when downloading modules on multiple threads #1081

Merged

Magisus closed this Jul 29, 2020

justinstoller deleted the GH-1058_dont-concurrently-modify-same-cache branch April 27, 2021 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1058] Serially deploy modules that share a cachedir #1059

[#1058] Serially deploy modules that share a cachedir #1059

justinstoller commented May 1, 2020

Magisus left a comment

Magisus commented May 5, 2020 •

edited

Loading

mwaggett May 5, 2020

Magisus Jul 27, 2020

mwaggett May 5, 2020

mwaggett commented May 5, 2020

adrienthebo commented May 5, 2020 •

edited by justinstoller

Loading

justinstoller commented May 5, 2020

justinstoller commented May 5, 2020

adrienthebo May 5, 2020

adrienthebo May 5, 2020

dhollinger Jul 2, 2020

Magisus Jul 23, 2020

dhollinger left a comment

justinstoller commented Jul 2, 2020

Magisus commented Jul 23, 2020

justinstoller commented Jul 24, 2020

Magisus commented Jul 29, 2020

		@@ -180,6 +189,19 @@ def accept(visitor)

		private

		def module_vcs_cachedir(mod)

[#1058] Serially deploy modules that share a cachedir #1059

[#1058] Serially deploy modules that share a cachedir #1059

Conversation

justinstoller commented May 1, 2020

Magisus left a comment

Choose a reason for hiding this comment

Magisus commented May 5, 2020 • edited Loading

mwaggett May 5, 2020

Choose a reason for hiding this comment

Magisus Jul 27, 2020

Choose a reason for hiding this comment

mwaggett May 5, 2020

Choose a reason for hiding this comment

mwaggett commented May 5, 2020

adrienthebo commented May 5, 2020 • edited by justinstoller Loading

justinstoller commented May 5, 2020

justinstoller commented May 5, 2020

adrienthebo May 5, 2020

Choose a reason for hiding this comment

adrienthebo May 5, 2020

Choose a reason for hiding this comment

dhollinger Jul 2, 2020

Choose a reason for hiding this comment

Magisus Jul 23, 2020

Choose a reason for hiding this comment

dhollinger left a comment

Choose a reason for hiding this comment

justinstoller commented Jul 2, 2020

Magisus commented Jul 23, 2020

justinstoller commented Jul 24, 2020

Magisus commented Jul 29, 2020

Magisus commented May 5, 2020 •

edited

Loading

adrienthebo commented May 5, 2020 •

edited by justinstoller

Loading