Do not prepend default path because it can override modules. #2677

Merged
merged 1 commit into from Dec 18, 2013

Conversation

Projects
None yet
10 participants
Contributor

purpleidea commented Dec 17, 2013

It is common for Puppet to manage itself. If the puppet code you are
deploying pushes files to /etc/puppet/modules/, then prepending this
path can break deployment because it will override the module path if
the deployment code is changing. There is no good reason to include this
path. Puppet has built in defaults for this reason.

@purpleidea purpleidea Do not prepend default path because it can override modules.
It is common for Puppet to manage itself. If the puppet code you are
deploying pushes files to /etc/puppet/modules/, then prepending this
path can break deployment because it will override the module path if
the deployment code is changing. There is no good reason to include this
path. Puppet has built in defaults for this reason.
d4c76d1
Contributor

purpleidea commented Dec 17, 2013

Just came across this bug. Here's the fix, it's an easy one. If you can backport this to 1.3.5 that would be awesome.

Thanks

Owner

mitchellh commented Dec 18, 2013

That's interesting. I'm not even sure why I prepended it to begin with. Let me do some testing and I'll merge this. Thanks!

mitchellh merged commit 9ac4ec8 into mitchellh:master Dec 18, 2013

1 check passed

default The Travis CI build passed
Details
Collaborator

phinze commented Dec 18, 2013

@mitchellh FWIW the relevant issue that spurred the code was #1207

Contributor

purpleidea commented Dec 18, 2013

@mitchellh Indeed! @phinze tracked down the original thread. I looked at it, and tbh, I don't agree with the logic. So I'm comfortable with this patch.

I am okay with a different patch that someone wants to append modules on, but you'd have to specify them, it shouldn't just happen.

Ask if you need more info.

Cheers

klebba commented Dec 19, 2013

I've been relying on this default prepend "feature" for installing puppet modules in the guest prior to Vagrant's puppet apply -- what should I do now instead?

Contributor

purpleidea commented Dec 19, 2013

@klebba How did the modules get into /etc/puppet/modules in the first place?

If you give me more info about your setup, I'm happy to try and suggest some ideas. You can ping me offline or on IRC if you don't want it public in this tracker.

Cheers

klebba commented Dec 19, 2013

I have a shell script provisioned by Vagrant that runs 'puppet module install puppetlabs-apt' -- since I can't count on the host to provide third party modules this seemed to be a sane approach.

As a workaround I'm manually setting the --modulepath flag for puppet apply via the Vagrant puppet provider config option. This breaks the puppet provider's magic module syncing, so I folder sync my custom modules manually from host to guest.

On Dec 18, 2013, at 6:07 PM, James notifications@github.com wrote:

@klebba How did the modules get into /etc/puppet/modules in the first place?

If you give me more info about your setup, I'm happy to try and suggest some ideas. You can ping me offline or on IRC if you don't want it public in this tracker.

Cheers


Reply to this email directly or view it on GitHub.

@klebba @purpleidea I used this as well, for similar reasons. There is no other way to add module directories from the guest to the Puppet path currenty, I presume?

@lunaryorn lunaryorn added a commit to flycheck/flycheck that referenced this pull request Dec 19, 2013

@lunaryorn lunaryorn Use our own module path for Vagrant provisioning
Since 1.4.1 Vagrant does not longer add the default module path, which
breaks our setup of installing modules to /etc/puppet/modules in the
guest VM.  Hence, we now use your own module path, based on our
explicitly shared folder.

See mitchellh/vagrant#2677
af43b00
Contributor

purpleidea commented Dec 19, 2013

Hey guys,

I'm proper sick, but I'll write up a real example probably later tonight.

On Thu, Dec 19, 2013 at 8:02 AM, Sebastian Wiesner <notifications@github.com

wrote:

@klebba https://github.com/klebba @purpleideahttps://github.com/purpleideaI used this as well, for similar reasons. There is no other way to add
module directories from the guest to the Puppet path currenty, I presume?


Reply to this email directly or view it on GitHubhttps://github.com/mitchellh/vagrant/pull/2677#issuecomment-30926637
.

The ability to install modules on the box is a pretty important feature of Puppet, and has been Vagrant behavior for a long time. You're going to see more and more people whose build environments are now broken. This seems too big a change for a dot release, and I would argue that the previous behavior was correct—certainly for default behavior.

Having a bunch of standard modules installed on base boxes and then using Vagrant to push just add a couple of specific modules is a very common pattern in the wild.

I'd strongly urge this change be rolled back ASAP unless there's a way to trigger the old behavior. The old behavior was better.

Contributor

purpleidea commented Dec 21, 2013

Okay, so I've got the flu, but I'll try to write about what I do, and we can all discuss. I'll try to post all my code and modules within the week if I can.
To make matters worse, I just wrote this up, and then I bumped the back button on my keyboard! Going "forward" takes you to a fresh github page where all your writing is lost!

Let's start by looking at the code. What does it do?

It checks to see if at least one module is being specified, and only then will it prepend /etc/puppet/modules/ from the guest.
In other words, if you're trying to set a modules path yourself, then the provisioner steps in and says, mine has priority.

Can someone please explain why this even makes sense?

So how do I do "work" with puppet and vagrant?

Well, I usually provision all my guest with the :puppet_server (agent) provisioner, and I use:

puppet.options = '--test'

so that I can see all the output as it happens. My base images have the puppet service stopped on boot, but if they don't, you could use a shell provisioner to do this.

For this, you need a puppetmaster. If you care about multi machine systems and exported resources (and you should), then you'll need one too. If you don't, or if in my situation, I have the bootstrapping problem that I want to provision the puppetmaster itself, then here's what I do:

Since I use puppet, I use the :puppet (apply) provisioner. My Vagrantfile contains:

vm.vm.provision :puppet do |puppet|
    puppet.module_path = 'puppet/modules'
    puppet.manifests_path = 'puppet/manifests'
    puppet.manifest_file = 'site.pp'
    # custom fact
    puppet.facter = {
        'vagrant' => '1',
    }
end

As you can see, I specify a modules directory from my host. The problem is, that when I do, it can get overridden by the prepending that we talked about above!
In any case, this should work for your setups. If it doesn't, let me know why! You'll see that I included a fact, but it's only for demonstration purposes.

Lastly, as I mentioned, I use this to provision my puppetmaster. For this I have a puppet-puppet module, which installs puppet, puppetdb, and so on. It also has a puppet::deploy class which I use which locally rsyncs on the guest vm from /vagrant/puppet/ to /etc/puppet/{modules,manifests,etc...}

I hope this has been useful to you. Please let me know what you think.
If this doesn't help you, then please explain why, and I'll try and either find you a solution, or if need be, I'll try and patch the provisioner where appropriate.

Cheers,
James
https://ttboj.wordpress.com/

Contributor

purpleidea commented Dec 21, 2013

By the way, the reason the original bug #1207 makes no sense, is that the OP for it is mixing up his use of puppet apply running with all of the code that vagrant puts there on each 'provision' run, and the /etc/puppet/ directory which should only really matter on the host that the_:puppet_server_ provisioner uses.

I hope I explained that clearly.

Cheers

@purpleidea Fine, you told me how your setup looks like and why the old behavior was bad for you. Ok, I understand that, and I agree that it should be fixed.

But still, your setup is just your setup, and I see not why fixing your setup should have to break mine. I do not care whether you think that guest modules should not matter for Puppet Apply.

They matter for me, because I use Puppet Apply this way. I neither want to use Puppet Server nor put the modules on the host, because my machines should be self-contained, and come with as little external dependencies as possible.

And in either case, your fix is a backwards incompatible change in a patch release, which is a no-go in and by itself for any software that promises stability, regardless of whether the change was reasonable or not.

As @sarahgerweck, I urge you to revert this change and publish a new minor release to restore the old behavior. Afterwards, fix this issue in a backwards-compatible way, e.g. add a new boolean default_path option, which disables the default module path, when set to false.

Contributor

purpleidea commented Dec 21, 2013

@lunaryorn

In my opinion, here are some facts:

  • Using puppet apply as a standalone binary doesn't depend on the /etc/puppet/modules directory! Read the manual: http://docs.puppetlabs.com/references/stable/man/apply.html and try to understand why 'apply' exists.

  • The old behaviour wasn't just "bad for me" it was a bug!

  • The reason why fixing this breaks your setup is because there is a bug in your setup.

  • This isn't an issue of a backwards incompatible change, it fixes a bug (as we've discussed). Furthermore, Vagrant isn't something that you're typically using to run a bank's database. It's a hacker's tool, so be agile and fix your code.

  • You're overreacting. Has this patch even hit your workstation yet?

  • I've given you the fix above. If you don't want to fix your code, but you want a workaround so that you keep this (incorrect) behaviour, add this to your Vagrantfile:

    puppet.options = "--modulepath=/etc/puppet/modules"
    

    You can also pass other options such as the location of a config file.

  • I think your attitude kind of sucked in your last comment. I'm trying to make vagrant better and help you too, but you're making it hard for me to want to.

I've spent enough time discussing a 2-3 line patch. If I do anymore, I think I'll be feeding your troll. https://en.wikipedia.org/wiki/Troll_(Internet)

Cheers!

klebba commented Dec 26, 2013

@purpleidea Three users have come here to offer input regarding why this change is a breaking change for their configuration. Your suggested workaround is in fact only a fix for your specific use case and not mine or theirs. Identifying contributing users of Vagrant as trolls on an issue ticket is bizarre. Despite the terseness in @lunaryorn's message his assessment is 100% correct and your rebuttal is not helpful. I'm glad to know that you consider Vagrant a hacker's tool and not something reliable -- but this completely contradicts the "trusted by" logo bank on the Vagrant homepage.

Ultimately here's what I did to work around this breaking change:

# sync the host's puppet modules folder into Vagrant's expected location
config.vm.synced_folder './modules', '/tmp/vagrant-puppet-1/modules-0'

config.vm.provision :puppet do |puppet|
  puppet.manifests_path = 'manifests'
  puppet.manifest_file  = 'project.pp'
  # puppet.module_path    = ['modules'] # handled the module sync manually above
  # workaround for 1.4.1 Vagrant breaking change - https://github.com/mitchellh/vagrant/pull/2677
  puppet.options        = '--modulepath "/etc/puppet/modules:/tmp/vagrant-puppet-1/modules-0"'
end

Basically all this is doing is recreating the magic sync that Vagrant provides for puppet.module_path and allowing you to configure it manually for host AND guest sourced Puppet modules.

Good luck!

Contributor

purpleidea commented Dec 26, 2013

@klebba glad to hear my --modulepath workaround helped you!

Unfortunately nobody complaining about this issue has actually publicized their setup so that it can be critiqued and improved. At least @lunaryorn admits that his setup was broken.

Good luck!

@purpleidea You may want to read my comment again. I didn't "admit" any such thing.

My setup is simple: I have modules on my host and on my guest, and I'd like to use both at the same time.

I could easily do before you "fixed" Vagrant, and I cannot easily do now anymore without the flaky workaround suggested by @klebba.

I find it funny how you can consider my setup buggy without even knowing it.

But since you successfully managed to get this on the personal level by challenging my attitude and effectively calling me a troll, I have no intention to continue this conversion.

klebba commented Dec 26, 2013

Just to be clear; the --modulepath "workaround" is not the magic sauce here. Using it without the manual folder sync I suggest above will make it impossible to use modules sourced from the host using the Puppet Apply provisioning approach suggested in the docs.

Although your snarky reply is not lost on me, I want to reiterate the reason why I'm here: I was relying on the design of the tool, which changed in 1.4.1 and therefore broke my working, bug free implementation. According to #1207 this has been the expected behavior of Vagrant up until 1.4.1 -- not a bug! Get with it man!

Just a heads up so you guys are aware of how pervasive this issue with the modulepath of the puppet provisioner no longer including the default module path is.

Services like https://puphpet.com/ use a shell script to install modules using librarian. The installation directory is:

# Directory in which librarian-puppet should manage its modules directory
PUPPET_DIR=/etc/puppet/

There are a number of projects out there that currently are shipped using a Vagrantfile and associated resources from this and similar sites that install modules like this. We encountered this issue when some of our developers started to notice they were no longer able to provision their development environments for several of our projects.

It is important everyone realizes that this is not just a few developers that are going to be inconvenienced due to this change, it's potentially a much larger issue due to the popularity of sites like the one above.

Agreed with the above, this is a major breaking change for many people's Vagrant configs. This should be rolled back. If the new method is intended then it should be in 1.5 at least rather than a minor dot release. Additionally, there needs to be a method to tell Vagrant additional puppet module paths that do not need to be mounted from the host.

For what it's worth, this change broke our configuration also. My workaround looks something like this:

config.vm.synced_folder "../../puppet/modules/", "/modules"
config.vm.provision :puppet do |puppet|
puppet.manifests_path = "../../puppet/manifests"
puppet.manifest_file = "site.pp"
puppet.options = [
"--modulepath=/modules:/etc/puppet/modules",
]

I'm only using puppet apply to manage our hosts (no puppet agent/master). There are advantages and disadvantages to both approaches. It's good that Vagrant supports both. Slightly annoying that the behavior changed, but testing and fixing is just part of using open source software.

Yeah, that solution works. The solution I'm using is just specifying the following in Puppet options "--modulepath=/etc/puppet/modules:/vagrant/puppet/modules"

I didn't see a need to mount an extra folder since the Vagrant folder always mounts itself to /vagrant.

Again though, if this is the way things are meant to be that's fine, but I just want to stress that a major breaking change like this does not belong in a minor release.

@michaelgrosser - I tried that also, but it only worked sometimes because Vagrant added two "--modulepath" options to the "puppet apply" command-line. These seem to show up in random order when Vagrant runs "puppet apply". It worked fine when the one that contained /etc/puppet/modules was last but not when it was first ("last one wins"). I had better luck removing the "puppet.module_path" option from the Vagrantfile and manually sharing the local modules.

@dsnyder0pc Yeah, I removed the puppet.module_path line enitrely.

This is the Puppet-related part of my Vagrant file:

  # Do puppet provisioning
  config.vm.provision :puppet do |puppet|
    puppet.facter = {
      "ssh_username" => "vagrant",
      "fqdn"         => config.vm.hostname
    }

    puppet.manifests_path = "puppet/manifests"
    puppet.options = ["--hiera_config /vagrant/puppet/hiera.yaml", "--parser future", "--modulepath=/etc/puppet/modules:/vagrant/puppet/modules"]
  end

@sennerholm sennerholm added a commit to AvegaGroup/deploymentPipeline that referenced this pull request Jan 23, 2014

@sennerholm sennerholm Ugly workaround to handle changed behavior of vagrant 1.4.1 and futur…
…e. More information in mitchellh/vagrant#2677
5b8bb13

It's really not enough to just hard-code modules-0 into your modulepath. Besides being a really bad practice to assume where Vagrant is going to map your directories, Vagrant is designed to pluggably allow multiple provisioners, including with user-directed provisioning. This is a big part of what makes Vagrant useful.

If the user has a puppet provisioner that installs their own SSH key and their shell settings, then all of a sudden you're not vagrant-puppet-0, you're vagrant-puppet-1.

It's acceptable (but still wrong) for the modulepath not to include /etc/puppet/modules, but if the default behavior is to ignore the Puppet standard module location, then there must be a way to add folders to the module path without hard-coding the whole thing.

I don't for the life of me understand why the solution wasn't just to move the globals to the end of the path rather than making it effectively impossible to reliably use global modules. This would have kept all the old builds working.

@sennerholm sennerholm added a commit to AvegaGroup/deploymentPipeline that referenced this pull request Feb 18, 2014

@sennerholm sennerholm Ugly workaround to handle changed behavior of vagrant 1.4.1 and futur…
…e. More information in mitchellh/vagrant#2677
7d8fb64

@mitchellh mitchellh added a commit that referenced this pull request Feb 24, 2014

@mitchellh mitchellh Revert "Merge pull request #2677 from purpleidea/fix_puppet_apply"
This reverts commit 9ac4ec8, reversing
changes made to 8dbad22.
32c45aa
Owner

mitchellh commented Feb 24, 2014

Note in 1.5 that the default module path will once again be appended to the module paths.

@mitchellh Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment