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

various fixes to get puppet/puppetmaster configurable on redhat #6

Merged
merged 10 commits into from
Jun 20, 2013

Conversation

pkilambi
Copy link

@rstarmer
Copy link
Contributor

👍

@iawells
Copy link
Contributor

iawells commented May 29, 2013

  •            if !defined( Service[$apache_service] ) {
    
  •               service { $apache_service:
    
  •                    ensure => "running",
    
  •                    notify => Service["puppetmaster"]
    
  •               }
    
  •            } 
    

Puppet expert required: is this safe? Seems to me like it would depend on puppet's module parse order, and would work iff the apache module is parsed before this module.

@rickerc
Copy link
Member

rickerc commented May 29, 2013

It's not using the apache module that I see -- that $apache_service is defined earlier within this init in the case statement at the top

@ghost ghost assigned iawells and bodepd Jun 11, 2013
@markvoelker
Copy link

@pkilambi : Looks like this needs rebasing...

@bodepd
Copy link

bodepd commented Jun 11, 2013

sorry for the slow response, I did not realize this was assigned to me. I would prefer that we look at moving to a different module so that we can add redhat support, upgrade to puppet 3.2, and add puppetdb with the same task. I have the following code working

https://github.com/bodepd/puppet-COI/blob/master/manifests/profiles/puppet/master.pp

with the following modules:

https://github.com/bodepd/openstack-installer/blob/puppetfile_update/Puppetfile#L50

, would you consider abandoning these changes and testing out this module?

I will go ahead and review the code as well.

@@ -8,12 +8,27 @@
$mysql_root_password = 'changeMe',
$mysql_password = 'changeMe') {

package { puppet-common:
case $::osfamily {
Copy link

Choose a reason for hiding this comment

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

the typical pattern is for these types of platform differences to be separated in a file called module_name::params

Copy link

Choose a reason for hiding this comment

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

the spacing here looks quite strange, the puppet convention is two space.

@markvoelker
Copy link

@bodepd:

sorry for the slow response, I did not realize this was assigned to me.

It wasn't until this evening, so I'd say no apology is necessary. =)

@pkilambi
Copy link
Author

@bodepd I'm totally ok with moving to upstream module. But i'm not sure if it makes sense for us to do that this late in the release? Perhaps we can merge this fix in for the upcoming release and investigate the new module for the next iteration ?

@markvoelker
Copy link

@pkilambi : Sounds reasonable to me. I'll test this today (thanks for rebasing). @bodepd, any comment on @iawells's question (#6 (comment))?

@bodepd
Copy link

bodepd commented Jun 11, 2013

@iawells - good catch, if ! defined is not a good idea b/c 'notify => Service["puppetmaster"]' wil not be added if the resource comes from somewhere else.

Instead, it's probably better to declare the dependency separately:

Service['apache'] ~> Service['puppetmaster']

This can result in an issue is the apache service already uses the metaparameter subscribe (this will show up as the error cannot call << on hash, I would not consider this a blocker, just be aware that this change could result in that issue)

On a side note, if !defined could be problematic with the other params b/c it can not determine if a resource that was already parsed contains those same params. Perhaps have a look at ensure_resource from stdlib?

@markvoelker
Copy link

@pkilambi : I think that last patch (c255d57) isn't quite right. Fails with:
Could not find resource 'Service[apache2]' for relationship on 'Service[puppetmaster]' on node buildnode.domain


file { "/etc/cron.d/puppet_cleanup":
content => template('puppet/puppet_cleanup.erb'),
require => Package[puppet-common],
require => Package[$::puppet::params::puppet],
Copy link

Choose a reason for hiding this comment

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

I much prefer making references to things based on static titles (only the namevar should be different for resources) both the namevar and title are guaranteed to be unique, and I feel titles are a little more clear.

@markvoelker
Copy link

@pkilambi : Getting this on Ubuntu (with everything up to 7606a2a):
Could not find resource 'Service[httpd]' for relationship on 'Service[puppetmaster]' on node cvf2-server-f1.cisco.com

Pradeep Kilambi added 2 commits June 14, 2013 15:29
@markvoelker
Copy link

Ok, that last set of patches seems to have things working on the Ubuntu side again. +1

@robertstarmer
Copy link
Contributor

Just tested:
notice: /Stage[main]/Apache/Service[httpd]: Triggered 'refresh' from 2 events
info: /Stage[main]/Apache/Service[httpd]: Scheduling refresh of Service[puppetmaster]
err: /Stage[main]/Puppet/Service[puppetmaster]: Could not evaluate: Could not find init script for 'puppetmaster'
err: /Stage[main]/Puppet/Service[puppetmaster]: Failed to call refresh: Could not find init script for 'puppetmaster'

@pkilambi
Copy link
Author

So talking to Mark, turns out ubuntu doesnt have a puppetmaster service and only relies on httpd. But on red hat it has both and thats why even though this error happens on ubuntu the services still work as apache is still up. I'll stick in an if check to define this service only on redhat.

@markvoelker
Copy link

This is also why the puppetmaster still comes up and functions on Ubuntu in spite of the error. Essentially it fails to start the puppetmaster service (because there isn't one) but it does deal with Apache appropriately (and that's all that's necessary on Ubuntu since it's configure via sites-enabled and started with Apache itself).

@markvoelker
Copy link

That last patch gave me a clean run. +1

@robertstarmer
Copy link
Contributor

Likewise. 👍 Clean test run.

robertstarmer pushed a commit that referenced this pull request Jun 20, 2013
various fixes to get puppet/puppetmaster configurable on redhat
@robertstarmer robertstarmer merged commit 848e3b9 into CiscoSystems:grizzly Jun 20, 2013
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.

None yet

7 participants