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

Add a synced folder to persist chef omnibus packages #248

Merged
merged 4 commits into from
Nov 23, 2016

Conversation

afiune
Copy link

@afiune afiune commented Nov 14, 2016

Setting up the new chef_directory interface within the Driver. Then with this directory we will allow the the instance to have a cache directory where omnitruck will download the artifacts, so we can cache the packages locally and avoid re-pulling down artifacts that you already have on your workstation.

Signed-off-by: Salim Afiune [email protected]

@@ -143,6 +143,19 @@ def finalize_config!(instance)
finalize_vm_hostname!
finalize_pre_create_command!
finalize_synced_folders!
# Verify if we are using the Provisioner::ChefBase that now has a
# config parameter for the Chef omnibus cache, if that is set then
# we would like # to sync a local folder to the instance so we can

Choose a reason for hiding this comment

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

extra # inline here

Copy link
Author

Choose a reason for hiding this comment

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

Yup, thanks! 😄

# @return [String] full absolute path to the kitchen cache directory
# @api private
def local_kitchen_cache
@local_kitchen_cache ||= File.expand_path('~/.kitchen/cache')

Choose a reason for hiding this comment

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

I'm not sure this format will work on Windows. We might need to do something a little more Windows friendly?

Copy link

Choose a reason for hiding this comment

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

Astonishingly, this actually does the right thing on windows:

irb(main):002:0> File.expand_path '~/.chef'
=> "C:/Users/thom/.chef"

Choose a reason for hiding this comment

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

Oh cool!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's actually a bigger problem on Unix systems:

$ env HOME=/tmp ruby -e 'puts File.expand_path("~/foo")'
/tmp/foo

When running in a jenkins job or similar, it's easy for $HOME to not be set correctly since that's usually handled by the login shell. Here be dragons. Might be better to explicitly get the current user's homedir.

Copy link
Member

Choose a reason for hiding this comment

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

The unfortunate issue with this path is that it makes this cache cookbook local. A user may test many cookbooks (very common) and each one will need to maintain their own cache in its local .kitchen folder. Would be nice to move it to a temp directory of the host.

Copy link
Author

Choose a reason for hiding this comment

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

@mwrock Apologize Matt but I couldn't understand your concern about the cache directory in the scope of testing multiple cookbooks, the cache is uniq per user, that is why we are storing it inside the home directory, that means that it will be shared across suites, instances and even projects (different repositories).

Another important thing to mention is that this cache is for chef packages and not cookbooks, maybe that cache that you mention is the actual berkshelf cache which is different.

Let me know if I am helping with this comment or confusing you more. 😄

Copy link
Member

Choose a reason for hiding this comment

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

argh. nevermind, I was confusing local/remote locations.

@tduffield
Copy link

Also, would love to see some tests! :)

Salim Afiune added 2 commits November 16, 2016 15:32
Consume the new chef_omnibus_cache directory by syncing it to the
instance where omnitruck will download the artifacts, so we can cache
the packages locally and avoid re-pulling down artifacts that you
already have on your workstation.

Signed-off-by: Salim Afiune <[email protected]>
@afiune afiune force-pushed the afiune/COOL-321/kitchen-cache branch from a346c99 to 589ef04 Compare November 16, 2016 20:32
@afiune afiune force-pushed the afiune/COOL-321/kitchen-cache branch from 3bf0710 to f60e90f Compare November 22, 2016 18:56
Signed-off-by: Salim Afiune <[email protected]>
@afiune afiune force-pushed the afiune/COOL-321/kitchen-cache branch from f60e90f to 20858aa Compare November 22, 2016 19:45
Copy link
Contributor

@smurawski smurawski left a comment

Choose a reason for hiding this comment

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

giphy 1

Copy link
Member

@mwrock mwrock left a comment

Choose a reason for hiding this comment

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

This PR looks good but after testing this I think we have a problem in Test-Kitchen that is going to severely limit the reach of this feature. That is that it only "kicks in" when provisioner options include chef_omnibus_install_options: -P chef and as far as I have seen, only our own internal CI processes use that. We should really address this in TK so that this caching gets used when the provisioner options are simply:

provisioner:
  name: chef_zero

or something similarly minimal.

@mwrock mwrock merged commit aa748b2 into test-kitchen:master Nov 23, 2016
@afiune
Copy link
Author

afiune commented Nov 23, 2016

@mwrock that is so weird because the code explicitly expect chef_omnibus_install_options to be nil (code) - But I wonder if the problem is that it is not nil and instead it is empty ""... 🤔

@mwrock
Copy link
Member

mwrock commented Nov 23, 2016

I was mainly looking at

      def install_options
        add_omnibus_directory_option if instance.driver.cache_directory
        project = /\s*-P (\w+)\s*/.match(config[:chef_omnibus_install_options])
        {
          :omnibus_url => config[:chef_omnibus_url],
          :project => project.nil? ? nil : project[1],
          :install_flags => config[:chef_omnibus_install_options],
          :sudo_command => sudo_command
        }.tap do |opts|
          opts[:root] = config[:chef_omnibus_root] if config.key? :chef_omnibus_root
          [:install_msi_url, :http_proxy, :https_proxy].each do |key|
            opts[key] = config[key] if config.key? key
          end
        end
      end

Only the first case will use chef_omnibus_install_options

@mwrock
Copy link
Member

mwrock commented Nov 23, 2016

when I added product to my options (which no one does) it started caching

@vinyar
Copy link
Contributor

vinyar commented May 9, 2017

So, what's the status of this feature?

It's loosely documented and does it work as intended? @mwrock

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants