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.

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

metadata 'gem' installer always calls bundle install, even if nothing to do #8753

Open
jjustice6 opened this issue Jul 18, 2019 · 10 comments

Comments

@jjustice6
Copy link
Contributor

commented Jul 18, 2019

Description

https://github.com/chef/chef/blob/master/lib/chef/cookbook/gem_installer.rb

The gem_installer code that handles the gem 'foo' lines in metadata.rb appears to invoke the dependency resolver on every single Chef run.

We have an artifactory instance which hosts a great many things, Chef invoking bundler in its regular cron job is the single largest load on artifactory.

This will affect anyone who's mirroring rubygems internally, whether using artifactory or some other mirroring software; and may even be a noticeable fraction of the load on rubygems.org due to chef nodes that are set up to use the internet at large.

Chef Version

14.7.17, code appears to be the same through current master (15.1.54 is the latest version at time of writing).

Platform Version

It's OS-independent.

Replication Case

Use chef-sugar or some other popular cookbook with a gem line in metadata.rb, such as chef-vault.

Client Output

Here's an output from a node running a policy with just chef-sugar and chef-vault.

---- Begin output of bundle install ----
STDOUT: Don't run Bundler as root. Bundler can ask for sudo if it is needed, and
installing your bundle as root will break this application for all non-root
users on this machine.
Fetching source index from http://artifactory.CORPORATE.com/artifactory/api/gems/rubygems/
Resolving dependencies...
Using bundler 1.16.1
Using chef-sugar 5.0.1
Using chef-vault 3.4.3
Bundle complete! 2 Gemfile dependencies, 3 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.

Notice that neither of these gems have any additional dependencies outside of development, and this happens on every chef run, even though those versions of the relevant gems are already installed.

Stacktrace

N/A

@jjustice6

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

One of the things to note about the existing code is it invokes Dir.mktmpdir every run; so there's no keeping the Gemfile.lock around currently.

@jbotelho2-bb

This comment has been minimized.

Copy link

commented Jul 18, 2019

Keeping around the Gemfile.lock in combination with running bundler in deployment would avoid the resolution on every Chef run. Unfortunately, the bundle install command and shell environment seem to be hard-coded currently:

so = shell_out!("bundle install", cwd: dir, env: { "PATH" => path_with_prepended_ruby_bin })

@lamont-granquist

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

deleting the lockfile and recreating it i think is a deliberate choice. otherwise you could have a 2 month old stale lockfile that is still pulling down old versions of the gem. you'd also need to deal with the situation where the cookbooks update and the lockfile becomes out of date with the gemfile itself.

@jjustice6

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Agreed, it's got to be something more robust than just keeping the lockfile. Both the examples I reference do not specify a particular gem version, so they're getting the latest gem version (modulo any dependency resolving shenanigans, though for those two gems in particular that shouldn't matter). I wonder if it's viable to simply check if all gems in the gemfile are the latest version of the gem and if so, don't invoke bundle install; but then there you're still having to query rubygems somehow. It doesn't look like there's a trivial solution, but there's got to be some kind of solution. Maybe the fix actually needs to be in Bundler to not start depsolving if it's already got everything?

@joewood

This comment has been minimized.

Copy link

commented Jul 18, 2019

Could it only delete the lockfile if it's older than a configured duration?

@jjustice6

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Here's an alternative thought process: what if building a policyfile entailed bundling up the specific versions of the gems as part of the policy?

That's a bigger change, but I think it'd be semantically quite solid, as now the version of the gem would be explicitly tied to what was tested with that version of the cookbook at policy build time.

@jjustice6

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

(From there I think you'd need a client.rb option to choose between doing things the current way, and using the metadata gem bundle that comes with the policy, in order to not break existing users/non-policy users. I'm 100% fine with that, myself.)

@lamont-granquist

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

yeah that's a lot of surgery but that's saner. so you move the bundle install/update to the client workstation or whatever that generates the policy and the required gem versions are frozen as part of the policy and then the installation of the equality pinned versions happens at runtime.

@jjustice6

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Yeah. Does that sound like something we could get on the roadmap? I imagine this is something you'd tie to a major version like 16, but it seems really useful to be able to pin gem versions when building a policy rather than having to rely on cookbook authors to pin them in metadata.rb, which they aren't doing. Right now, in theory chef-sugar could actually induce breakage on a locked policy by removing a function in the gem (as a hypothetical, I don't think kind of breakage would be deliberately induced :) ).

You could even take it to the next level by uploading the gems to the chef server like for cookbooks, and eliminate the need for an external source of gem downloads altogether - even better for airgapped environments.

@lamont-granquist

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

So the way it was expected that the feature would be used would be that a site would have a wrapper cookbook or even their base role that would also pull in chef-sugar but would pin it via gem "chef-sugar", "= 1.2.3" or whatever policy they'd use, and the code which generates the lockfile will de-dup that with the gem "chef-sugar" that floats in the chef-sugar cookbook.

And I definitely don't have time to pick this up, so if you're asking me then it probably doesn't get on the roadmap. It would need to go through as part of a contract to get staffed via teams that handle those kinds of issues.

My reaction though is that this process is starting to feel wrong and the gem metadata approach feels like it has so many edge cases that it shouldn't be used. Its only really a solution to the chef-sugar and chef-vault use cases and maybe those should just be pulled into core chef rather than needing to do early installation of those gems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.