-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
Adding support for Ubuntu 15.04 #163
Conversation
Added fact 'is_systemd' to detect if Ubuntu 15.04 is running in upstart or systemd mode, systemd is default but still optional. Added support in params to detect Ubuntu 15.04. Added support for Ubuntu 15.04 path to systemctl by changing how the path ENV is handled in config.pp (Ubuntu 15.04 uses /bin/systemctl)
Fixed up the boolean checks, and quoted strings. Had 4 Travis CI quote/trailing whitespace issues. Fixed
# Introducing fact 'is_systemd' since Ubuntu 15.04 can be systemd OR upstart (defaults to systemd though) | ||
# Fact allows us to retrieve the client side value. | ||
# We convert to boolean incase some people may have stringify facts turned on or not set in puppet 3.x still. | ||
$is_systemd = str2bool($::is_systemd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we condense this into:
$init_style = str2bool($::is_systemd) ? {
true => 'systemd',
default => 'upstart',
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure can. I'll do that in a few minutes. I'm working through your code at the moment as well to patch it up as It doesn't update consul version if it's already installed. This is due to the 'creates' section in staging and how this module is using it. I have a plan of attack to make use of versioning and am about half way through patching it... well fully done with consul, investigating web_ui at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice! This has been a LONG standing issue with the url install method. I myself have been wanting to write one just hadn't had the time.
This is as close as we've gotten #118
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, I saw that. I'm pretty sure mine will work, theoretically it does :) But I may call it a weekend before I get any thorough testing done on it and return to fixing it on Monday. I'm hoping to deploy consul using this to our University soon enough so I want to make sure everything is up to snuff. Since there is no yum packages or apt that I know of for consul in the distros that we're using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No rush dude! heh. Yah, unfortunately there are no packages. When I have some time I'll build a package and try to deploy it to a public apt repo.
Let me know how the consul deploy goes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey AJ.
You've been really helpful so far with the responses to my PR. You mind just peeking at this commit with the working consul upgrade/downgrade (web_ui) not done yet but part way in.
sfu-rcg@8fbbf44
It tests fine for me. The Exec { provider => shell } is because staging module doesn't allow us to push path to the exec and I can't tie the test, grep, and rm down to specific directories in the 'onlyif' statement as they are located in different locations in Ubuntu vs CentOS. provider => shell fixes that up :)
I'm not sure how constrained the Exec { } is when called there. Does it only affect local items to the manifest plus things it calls or?
As I could be more specific and do Exec["extract ${consul::real_download_file}"] { provider => shell }
@@ -29,9 +29,21 @@ | |||
if $::operatingsystem == 'Ubuntu' { | |||
if versioncmp($::lsbdistrelease, '8.04') < 1 { | |||
$init_style = 'debian' | |||
} else { | |||
} elsif versioncmp($::operatingsystemrelease, '15.04') < 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we either use $::lsbdistrelease
here or replace the above with $::operatingsystemrelease
for consistency sake? I'm in favor of the latter because $::operatingsystemrelease
is used everywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't be authoritative on that comment. I don't know Ubuntu that far back to know if puppet supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware. Then lets change this to $::lsbdistrelease
as well since we know it exists even on the latest versions. My concern is we don't want to troubleshoot a bug where these two facts have different values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started programming puppet after version 3 and I've been a fan of less dependencies. So I was always trying to convert our old stuff to operatingsystemrelease and operatingsystemmajrelease.
lsbdistrelease is relying on redhat-lsb and whatever the package is in Ubuntu and other distros. I believe the operatingsystem values are coming straight from facter Facter::Operatingsystem.implementation
Though i'm not sure where it gets that from. I'm pretty sure the standing thought in puppet programming was to phase lsb out but that might be a personal preference of people... as I said, I haven't been doing this long enough. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://launchpad.net/ubuntu/vivid/+package/lsb-release
"The Linux Standard Base (http://www.linuxbase.org/) is a standard
core system that third-party applications written for Linux can
depend upon"
So I think we are safe 😄 my vote is for consistency over deprecation/dependencies, in this specific case. But I agree with you in general, we should try to move away from legacy stuff as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if you are curious, this is how $::operatingsystemrelease
gets its fact: https://docs.puppetlabs.com/facter/1.6/core_facts.html#operatingsystemrelease
https://docs.puppetlabs.com/facter/2.3/core_facts.html#operatingsystemrelease
As you can see its a mess lol
As per AJ and SolarKennedy, we do provide $init_style as a parameter available in the main class. We should provide sane defaults for the most common cases but not the far edge cases, if the current ability for the user to modify it is already there. Changed Ubuntu 15.04 and greater to default systemd.
@asasfu this looks great! Can you please write a unit test for this? After that this should be good to merge! |
…temrelease Changed Ubuntu 8 away from lsbdistrelease so had to update the spec test.
I'm not sure what happened on that Travis CI build but it kinda went tits up... I have never done a unit test before... :( I think I got the unit test wrote up. Just realized that test box I used to add those last few commits doesn't have a working ntp on it and has an old date. Will that fuck up the commit history? |
Also, i'm curious, how do you guys handle the 'consul reload' .... if the ENV variable(s) are not set such as CONSUL_RPC_ADDR= |
All params are set in In this case, if you want a specific rpc address just pass the If you dont set |
Ah you're right, it doesn't affect the puppet run's consul reload, just the user if he wants to type |
Ok, my commit history time issue was corrected and I fixed up the spec tests. This should be ready for production. On a side note: When Puppet loads all of its providers and checks its confines to decide which Service provider matches your system. Do you guys know if there's a way to |
🚄 LGTM
I'm not exactly sure if thats possible 😕 |
Thanks! Yea you can't get a provider for something in puppet and have that data influence the catalog; the catalog is compile first and then applied to the host where providers are determined at the last moment. |
Adding support for Ubuntu 15.04
Hey hey, you wanna know something you can do?! While i'm preparing the ability for URL upgrade/downgrade I finally figured out how to stop the service before removing consul bin. service_name = 'consul' require 'puppet' service.provider.send(actionstop) ####### Complete replacement of version, including CONSUL BIN service.provider.send(actionstart) So I just need to create a function that does all the work and does those steps as well. |
@aj-jester and @solarkennedy you mind looking at this commit to see if you have any suggestions or gripes. I know it has some conditionals in it but it works for all cases that should be presented. It also leaves staging files behind per version rather than cleaning up, something I can add(cleanup) if wanted. Actually it's probably better to reference this comparison as well, since AJ and I have already talked on it: |
My work is likely considered complete on that patch. Can you guys take a peek. I'll be submitting it as a pull request tomorrow. It has tested good on both my masterless and mastered puppet client. I'm not sure what or any tests you may want written for this prior to merging? |
Also now that I've done my testing, I've finally come across what I was looking for, that might be helpful to you guys when doing up params to decide init_style. again though, this only supports providers already setup in confines by puppet, this doesn't support any site.pp overrides for provider or anything, so those slight edge cases like Ubuntu 15.04 and Fedora 22 in Puppet 3.x will still return the wrong provider as this is ASKING puppet what they think the provider should be. But still altogether, really cool. |
Ok part 2, even better. Provider with and without override provided by an upper class or site.pp via: With override, what I'm doing here is calling to defaults and return provider: Without override, here I'm retrieving service provider with no allowance for anyone to have overridden it earlier and they'd have to use consul::init_style to actually override it then for example: Finally, no parsing needed this time, tested and showing proof: |
Last part, part 3. This is the final piece of code as I noticed a parse needed to happen on the default provider on part2 and then this supports merging. If you provide a Service { provider => systemd } in your site.pp or even parent class calling consul then you would override service { 'consul': } unless it was service { 'consul': provider => 'upstart' } as the more specific class wins if it defines that parameter or hash key/value. $merged_override_wins = inline_template("<% @puppet_decided = Puppet::Type.type(:service).newservice(:name => 'anyservice')[:provider].to_s %><% @OverRide = scope.lookupdefaults('Service')[:provider].instance_variable_get(:@value) %><% @OverRide = @puppet_decided if @override.nil? || @override.empty? %><%= @OverRide %>") notify { "Merged provider with override being the winner: ${merged_override_wins}": } $non_overridden = inline_template("<%= Puppet::Type::Service.new(:name => 'anyservice')[:provider] %>") notify { "Provider is: ${non_overridden}": } Might be a consideration to use this code to decide your init_style rather than what is currently in params. Then you would be modifying your conditional to match what they have in /usr/lib/ruby/vendor_ruby/puppet/provider/service/ Then you'd just have a selector stating what init script you're building based off that. No need to say, centos < 7 is redhat and centos >= 7 is systemd, ubuntu < 8 is debian, ubuntu > 8 is upstart, ubuntu > 15 is systemd. Kinda seems like a lot of decisions we're making that puppet or the user already does for us now that we know how to grab their decisions. Thoughts? |
Added fact 'is_systemd' to detect if Ubuntu 15.04 is running in upstart or systemd mode, systemd is default but still optional.
Added support in params to detect Ubuntu 15.04.
Added support for Ubuntu 15.04 path to systemctl by changing how the path ENV is handled in config.pp (Ubuntu 15.04 uses /bin/systemctl)