This makes sensu_gem work as advertised #877

tibers · 2018-01-27T16:23:24Z

Pull Request Checklist

Fixes the GEM_PATH which is causing embedded ruby to not do The Right Thing when RVM is present.

Description

Munge GEM_PATH appropriately.

Related Issue

#876

Fixes # .

Motivation and Context

Fixes installation of embedded ruby gems when RVM is present.

How Has This Been Tested?

Fired up a Vagrant instance with RVM installed, then installed the elasticsearch gems and made sure it could find the gems without further munging.

General

No testing required - it wasn't "broken" before. :)

majormoses

Seems reasonable, a couple questions/suggestions.

majormoses · 2018-01-27T22:13:14Z

lib/puppet/provider/package/sensu_gem.rb

@@ -15,6 +15,9 @@
    if RUBY_PLATFORM =~ /cygwin|mswin|mingw|bccwin|winse|emx/
      "#{ENV['SYSTEMDRIVE']}\\opt\\sensu\\embedded\\bin\\gem.cmd"
    else
+      # grab the newest version available of the embedded ruby
+      embedded_rvm_version = Dir.glob(File.join('/opt/sensu/embedded/lib/ruby/gems/', '*')).max { |a,b| File.ctime(a) <=> File.ctime(b) }
+      ENV['GEM_PATH'] = "/opt/sensu/embedded/lib/ruby/gems/#{embedded_rvm_version}"


Disclaimer I come from a chef world so I have very little idea on the internals of puppet. Would the ENV var be passed down to where it actually executes it? This function just gets the command not runs it.

I also was thinking about, "where should I put this?" My gut feeling is that if we have to use the embedded ruby, it's also appropriate to setup the ENV. Conversely we would not want to change the environment if we weren't using the sensu_gem provider. This sets up the environment only for the context of puppet, this isn't a system-wide change, so all the RVM stuff will remain untouched for everyone else.

Right but is that for the entire puppet process or just for this particular resource. I worry that it might interfere if say you are installing a system wide gem with rvm in puppet. Another question is chef also has it's own embedded ruby which I am not sure if puppet does, can you install gems in puppet into it's own embedded gems? If so this might also break that as well. Hopefully someone with more puppet experience than me can review and provide some better feedback.

majormoses · 2018-01-27T22:18:30Z

lib/puppet/provider/package/sensu_gem.rb

@@ -15,6 +15,9 @@
    if RUBY_PLATFORM =~ /cygwin|mswin|mingw|bccwin|winse|emx/
      "#{ENV['SYSTEMDRIVE']}\\opt\\sensu\\embedded\\bin\\gem.cmd"
    else
+      # grab the newest version available of the embedded ruby
+      embedded_rvm_version = Dir.glob(File.join('/opt/sensu/embedded/lib/ruby/gems/', '*')).max { |a,b| File.ctime(a) <=> File.ctime(b) }


a clever solution would you mind linking to the documentation? To those that are new to ruby this would be helpful.

tibers · 2018-01-27T23:13:56Z

Incidentally I'm not the best ruby person either but I can bang stuff out. I've added comments to the one liner to explain it a bit.

tibers · 2018-01-30T18:06:17Z

@majormoses I know you're positively holding your breath over this. ;)

I used rvm to install paperclip, which is something neither we use nor sensu uses. I verified in my vagrant installation that this is installed to the normal systemwide, rvm managed gempath. It looks like this change only affects the instantiation of sensu_gem, which is definitely what we want.

majormoses · 2018-01-30T18:09:27Z

Awesome I was watching it as I like learning stuff about different technologies. I am not a maintainer on the puppet module since there are people who know this stuff much better than I do. ping @ghoneycutt

ghoneycutt · 2018-01-30T18:14:47Z

lib/puppet/provider/package/sensu_gem.rb

+      # rip through everything in the directory and keep the newest one per comparison, omitting . and ..
+      # there's probably a faster/prettier way of doing this.
+      embedded_rvm_version = Dir.glob(File.join('/opt/sensu/embedded/lib/ruby/gems/', '*')).max { |a,b| File.ctime(a) <=> File.ctime(b) }
+      ENV['GEM_PATH'] = "/opt/sensu/embedded/lib/ruby/gems/#{embedded_rvm_version}"


I don't quite get what's going on here. It seems that an environment variable is being set to look in "/opt/sensu/embedded/lib/ruby/gems/#{embedded_rvm_version}" though then we still return a different path of "/opt/sensu/embedded/bin/gem".

Yes. Please observe the effect on GEM PATHS...

[root@vagrant ~]# gem env RubyGems Environment: - RUBYGEMS VERSION: 2.6.14 - RUBY VERSION: 2.4.0 (2016-12-24 patchlevel 0) [x86_64-linux] - INSTALLATION DIRECTORY: /usr/local/rvm/gems/ruby-2.4.0 - USER INSTALLATION DIRECTORY: /root/.gem/ruby/2.4.0 - RUBY EXECUTABLE: /usr/local/rvm/rubies/ruby-2.4.0/bin/ruby - EXECUTABLE DIRECTORY: /usr/local/rvm/gems/ruby-2.4.0/bin - SPEC CACHE DIRECTORY: /root/.gem/specs - SYSTEM CONFIGURATION DIRECTORY: /etc - RUBYGEMS PLATFORMS: - ruby - x86_64-linux - GEM PATHS: - /usr/local/rvm/gems/ruby-2.4.0 - /usr/local/rvm/gems/ruby-2.4.0@global - GEM CONFIGURATION: - :update_sources => true - :verbose => true - :backtrace => false - :bulk_threshold => 1000 - REMOTE SOURCES: - https://rubygems.org/ - SHELL PATH: - /usr/local/rvm/gems/ruby-2.4.0/bin - /usr/local/rvm/gems/ruby-2.4.0@global/bin - /usr/local/rvm/rubies/ruby-2.4.0/bin - /usr/local/bin - /sbin - /bin - /usr/sbin - /usr/bin - /opt/puppetlabs/bin - /usr/local/rvm/bin [root@vagrant ~]# GEM_PATH=/opt/sensu/embedded/lib/ruby/gems/2.4.0 /opt/sensu/embedded/bin/gem env RubyGems Environment: - RUBYGEMS VERSION: 2.6.10 - RUBY VERSION: 2.4.1 (2017-03-22 patchlevel 111) [x86_64-linux] - INSTALLATION DIRECTORY: /usr/local/rvm/gems/ruby-2.4.0 - USER INSTALLATION DIRECTORY: /root/.gem/ruby/2.4.0 - RUBY EXECUTABLE: /opt/sensu/embedded/bin/ruby - EXECUTABLE DIRECTORY: /usr/local/rvm/gems/ruby-2.4.0/bin - SPEC CACHE DIRECTORY: /root/.gem/specs - SYSTEM CONFIGURATION DIRECTORY: /opt/sensu/embedded/etc - RUBYGEMS PLATFORMS: - ruby - x86_64-linux - GEM PATHS: - /usr/local/rvm/gems/ruby-2.4.0 - /opt/sensu/embedded/lib/ruby/gems/2.4.0 - GEM CONFIGURATION: - :update_sources => true - :verbose => true - :backtrace => false - :bulk_threshold => 1000 - REMOTE SOURCES: - https://rubygems.org/ - SHELL PATH: - /usr/local/rvm/gems/ruby-2.4.0/bin - /usr/local/rvm/gems/ruby-2.4.0@global/bin - /usr/local/rvm/rubies/ruby-2.4.0/bin - /usr/local/bin - /sbin - /bin - /usr/sbin - /usr/bin - /opt/puppetlabs/bin - /usr/local/rvm/bin [root@vagrant ~]#

embedded_ruby_version

I submitted an update to the PR.

ghoneycutt · 2018-01-30T18:18:06Z

@majormoses @cwjohnston is RVM supported? I thought that we vendor ruby in /opt/sensu so that we can control which version of ruby is used by Sensu. This seems to go against that by allowing the user to specify a different ruby, such as one controlled by RVM.

tibers · 2018-01-30T18:49:23Z

@ghoneycutt - RVM does not seem to be supported. Sensu does use the ruby in /opt/sensu but it also configures the environment so gems installed via RVM will not be in the search path. Using the sensu_gem provider, we need to setup the environment also so that the gems sensu_gem installs end up in the sensu ruby search path. The whole point of this PR is so that if you're using sensu_gem, it installs the gems in the correct embedded ruby search path. If you want to manage the env through RVM (disabling the embedded ruby which is enabled by default), you totally can, just don't use the sensu_gem provider.

https://forge.puppet.com/sensu/sensu#installing-gems-into-the-embedded-ruby

majormoses · 2018-01-30T19:53:32Z

@ghoneycutt @tibers while it is recommended that you use embedded ruby as it provides more isolation and control we do support using system rubies if you want to though that comes with it's own set of challenges.

After thinking about this more as the function is called sensu_gem we should assume this should only really be used for embedded sensu setups as otherwise it is just another gem install which is an already solved problem. I actually had the same conversation today with someone in chef about this. I think that rather than changing the code we should update the documentation to explain to users about how to install gems into rubies other than the embedded ruby one.

tibers · 2018-01-31T14:05:51Z

@majormoses - that entire long thread I linked you to in slack was because installing gems with sensu_gem with rvm also installed makes sensu_gem pick up the rvm environment instead of the proper embedded one.

The PR fixes sensu_gem having environment problems. Documentation around how to install gems outside of sensu_gem doesn't fix the issue. :)

If you're foggy on the issue I would reference you to either the other issues raised around this or I would encourage you to fire up vagrant with rvm managing some gems and sensu's embedded ruby managing some different gems.

majormoses · 2018-01-31T17:41:42Z

@tibers maybe I am confusing you with someone else who had the same issues with sensu-install IMHO the fact that they have sensu in their name they should only be used for actual gems in sensu embedded ruby and otherwise use already existing tools.

If the problem is that having rvm installed messes with installing gems into it's embedded ruby then that makes a lot more sense. If you are installing gems into a non sensu embedded ruby it's not a sensu gem it's just a gem.

Apologies if my memory on the specifics are failing me here there are several people I have helped people out with related to installing gems and into system or embedded rubies.

majormoses · 2018-01-31T17:44:01Z

re-read the PR and it seems I am confusing you with someone else this fails when wanting to install gems into embedded ruby when rvm is installed. That makes sense again. I would still prefer rather than saying this is a rvm issue I think its a more broad issue as it may affect users with rbenv. Can you change the variable names up to be more accurate?

majormoses · 2018-01-31T17:44:49Z

lib/puppet/provider/package/sensu_gem.rb

+      # ruby slight of hand explaination: Ruby truncates full paths, so join the path, then
+      # rip through everything in the directory and keep the newest one per comparison, omitting . and ..
+      # there's probably a faster/prettier way of doing this.
+      embedded_rvm_version = Dir.glob(File.join('/opt/sensu/embedded/lib/ruby/gems/', '*')).max { |a,b| File.ctime(a) <=> File.ctime(b) }


embedded_ruby_version

majormoses · 2018-01-31T17:45:09Z

lib/puppet/provider/package/sensu_gem.rb

+      # rip through everything in the directory and keep the newest one per comparison, omitting . and ..
+      # there's probably a faster/prettier way of doing this.
+      embedded_rvm_version = Dir.glob(File.join('/opt/sensu/embedded/lib/ruby/gems/', '*')).max { |a,b| File.ctime(a) <=> File.ctime(b) }
+      ENV['GEM_PATH'] = "/opt/sensu/embedded/lib/ruby/gems/#{embedded_rvm_version}"


embedded_ruby_version

majormoses · 2018-01-31T20:32:46Z

On a side note (not sure why I didn't think of this sooner) because of all the fun with installing rubygems on systems with restrictive umasks we ran into needing something similar to update permissions post install in an idempotent way. In my sensu chef wrapper I have a helper library function like this:

def sensu_ruby_version(path = '/opt/sensu/embedded/bin/ruby')
  if ::File.exist?(path)
    ruby_version = Mixlib::ShellOut.new("#{path} --version")
    ruby_version.run_command
    ruby_version.error!
    # extract major.minor.patch from full text version
    version = ruby_version.stdout.split(' ')[1].split('p')[0]
    # set patch to 0 as we only care about major & minor
    version.gsub(/\.\d{1,}$/, '.0')
  end
end

This is more brittle than yours so I am probably gonna steal that for my internal cookbook.

tibers · 2018-02-01T15:23:29Z

Submitted a fixed up PR where I actually rename the variable instead of just renaming the instantiation. Anyway rather than having internal cookbooks, why not open a PR? ;)

majormoses · 2018-02-01T17:58:09Z

@ghoneycutt it looks reasonable to me now, any more thoughts?

ghoneycutt · 2018-02-03T10:09:13Z

sensu_gem is for using the embedded ruby, which this PR is not doing because of RVM. Perhaps I'm missing something but it seems like if you want to use RVM, we should just document how to do that as it seems misleading that sensu_gem is also used to install gems outside of sensu.

# in your profile
# based on using https://forge.puppet.com/maestrodev/rvm

include ::rvm

rvm_gemset { 'ruby-2.4.1':
  ensure  => present,
}

rvm_gem { 'some_sensu_gem':
  ensure  => '1.2.3',
  require => Rvm_gemset['ruby-2.4.1'],
}

# sensu stuff here

ghoneycutt · 2018-02-03T10:16:22Z

re-read the above comments and it seems that installing software with rvm is not enough, since sensu_gem will not see it.

I will merge this if you could provide a simple shell script for EL7 that installs RVM and shows the functionality works. We can then use this in the vagrant environment for testing.

# pseudocode

cat > /tmp/install_a_gemFoo_with_sensu_gem.pp <<EOF
  # puppet code
EOF

puppet apply /tmp/install_a_gemFoo_with_sensu_gem.pp

show that the gem Foo is installed

install rvm

cat > /tmp/install_a_gemBar_with_sensu_gem.pp <<EOF
  # puppet code
EOF

puppet apply  /tmp/install_a_gemBar_with_sensu_gem.pp

show that gem Foo is not installed and gem Bar is

majormoses · 2018-02-04T01:20:32Z

@ghoneycutt We are not looking to add support to have sensu_gem install into the system rvm as that does not make sense to me. This attempts to fix issues when you have rvm installed but want to use the embeded sensu, rvm is notoriously overwrites the $PATH and $ENV vars leading to apparently leading to installing the gem into the wrong context. I have not tested this myself but I understand the intent and it makes sense. I use rbenv on all my servers for additional rubies but I use the embedded sensu ruby and this is attempting to allow such a use case.

tibers · 2018-02-06T00:41:05Z

@ghoneycutt - I think you're misunderstanding this. It's not that the gem isn't installed, it's that it's installed outside of the embedded gem's GEM_PATH. This is a bugfix, not an enhancement request that adds functionality. Your existing tests actually still work.

ghoneycutt · 2018-02-06T09:12:21Z

If we're going to ensure that it works with RVM, then we need a way to set that up in the vagrant environment to ensure that it does. We need to see that it doesn't work without this patch and then does after it is applied.

tibers · 2018-02-06T14:46:50Z

I'm not familiar with your project - can you spin up a Centos7 VM with RVM and install some plugin, configure sensu to look for it, and validate it does not work?

mbbroberg · 2018-02-09T17:32:41Z

Hey @ghoneycutt, @tibers is deep on Puppet but not Sensu. Lmk if you're really pressed for time and we'll expect a delay accordingly! I'll reach out to the broader community to help out too. Tibers done all he can here (thanks Tibers!).

treydock · 2018-09-27T22:42:31Z

Maybe my test case is flawed but this is what I'm seeing:

[root@el7-client ~]# /vagrant/tests/test_sensu_gem_provider.sh
Notice: Compiled catalog for el7-client.example.com in environment production in 0.15 seconds
Notice: /Stage[main]/Main/Package[sensu-plugins-disk-checks]/ensure: created
Notice: Applied catalog in 1.67 seconds
GEM sensu-plugins-disk-checks installed
<OMIT RVM INSTALL OUTPUT>
GEM sensu-plugins-disk-checks not in RVM
Warning: GettextSetup is not available, skipping GettextSetup initialization for modules.
   (file & line not available)
Notice: Compiled catalog for el7-client.example.com in environment production in 0.13 seconds
Notice: /Stage[main]/Main/Package[sensu-plugins-memory-checks]/ensure: created
Notice: Applied catalog in 3.66 seconds
GEM sensu-plugins-memory-checks installed
GEM sensu-plugins-memory-checks found in RVM

Basically the sensu-plugins-memory-checks gem ends up in /usr/local/rvm. The script:

cat > /tmp/sensu_gem1.pp <<EOF
package { 'sensu-plugins-disk-checks':
  ensure   => 'installed',
  provider => 'sensu_gem',
}
EOF
puppet apply /tmp/sensu_gem1.pp

/opt/sensu/embedded/bin/gem which sensu-plugins-disk-checks &>/dev/null
if [ $? -eq 0 ]; then
  echo "GEM sensu-plugins-disk-checks installed"
else
  echo "GEM sensu-plugins-disk-checks NOT installed"
fi

gpg --keyserver hkp://keys.gnupg.net --recv-keys 409B6B1796C275462A1703113804BB82D39DC0E3 7D2BAF1CF37B13E2069D6956105BD0E739499BDB
\curl -sSL https://get.rvm.io | bash -s stable --ruby
source /usr/local/rvm/scripts/rvm

gem which sensu-plugins-disk-checks &>/dev/null
if [ $? -ne 0 ]; then
  echo "GEM sensu-plugins-disk-checks not in RVM"
else
  echo "GEM sensu-plugins-disk-checks found in RVM"
fi

cat > /tmp/sensu_gem2.pp <<EOF
package { 'sensu-plugins-memory-checks':
  ensure   => 'installed',
  provider => 'sensu_gem',
}
EOF
puppet apply /tmp/sensu_gem2.pp

/opt/sensu/embedded/bin/gem which sensu-plugins-memory-checks &>/dev/null
if [ $? -eq 0 ]; then
  echo "GEM sensu-plugins-memory-checks installed"
else
  echo "GEM sensu-plugins-memory-checks NOT installed"
fi

gem which sensu-plugins-memory-checks &>/dev/null
if [ $? -ne 0 ]; then
  echo "GEM sensu-plugins-memory-checks not in RVM"
else
  echo "GEM sensu-plugins-memory-checks found in RVM"
fi

ghoneycutt · 2018-09-27T23:10:44Z

@treydock could you please pull in @tibers work and add this check script to the repo so we can verify this in the future

treydock · 2018-09-27T23:16:46Z

@ghoneycutt I'm not sure this PR solves the problem, or my test case was incorrect. My script's steps ended up with sensu-plugins-memory-checks installed into RVM path and NOT the sensu embedded path.

[root@el7-client ~]# /opt/sensu/embedded/bin/gem which sensu-plugins-memory-checks
Ignoring executable-hooks-1.5.0 because its extensions are not built.  Try: gem pristine executable-hooks --version 1.5.0
Ignoring gem-wrappers-1.4.0 because its extensions are not built.  Try: gem pristine gem-wrappers --version 1.4.0
Error loading RubyGems plugin "/usr/local/rvm/gems/ruby-2.5.1@global/gems/executable-hooks-1.5.0/lib/rubygems_plugin.rb": cannot load such file -- executable-hooks/wrapper (LoadError)
Error loading RubyGems plugin "/usr/local/rvm/gems/ruby-2.5.1@global/gems/gem-wrappers-1.4.0/lib/rubygems_plugin.rb": cannot load such file -- gem-wrappers (LoadError)
Ignoring executable-hooks-1.5.0 because its extensions are not built.  Try: gem pristine executable-hooks --version 1.5.0
Ignoring gem-wrappers-1.4.0 because its extensions are not built.  Try: gem pristine gem-wrappers --version 1.4.0
/usr/local/rvm/gems/ruby-2.5.1/gems/sensu-plugins-memory-checks-3.2.0/lib/sensu-plugins-memory-checks.rb
[root@el7-client ~]# gem which sensu-plugins-memory-checks
Ignoring json-1.8.6 because its extensions are not built. Try: gem pristine json --version 1.8.6
Ignoring json-1.8.6 because its extensions are not built. Try: gem pristine json --version 1.8.6
/usr/local/rvm/gems/ruby-2.5.1/gems/sensu-plugins-memory-checks-3.2.0/lib/sensu-plugins-memory-checks.rb

ghoneycutt · 2018-09-28T00:10:01Z

@treydock if you applied the patch, then the output of the run is what's expected, that the plugins come from RVM at the end.

treydock · 2018-09-30T15:06:31Z

Added #987 that adds my script to verify the behavior.

ghoneycutt · 2018-10-01T20:02:31Z

Without this patch and with RVM present, checks will be installed with RVM according to this script. Either there is nothing left to do, the test script is not functioning as intended, or I'm missing this completely. Closing. Please re-open if this is still an issue along with how we can detect how things are broken before a patch and how they work after the patch.

Joshua Knarr added 3 commits January 27, 2018 10:40

fix for RVM munging up the environment

6a0b915

even with syntax highlighting, I still miss quotes

c97ae03

I am misunderstanding something in this ruby

74cef2d

majormoses reviewed Jan 27, 2018

View reviewed changes

now with comments and docs

6dcc296

ghoneycutt reviewed Jan 30, 2018

View reviewed changes

majormoses suggested changes Jan 31, 2018

View reviewed changes

renaming vars per request

5a966a0

I am an idiot and renamed a variable and didnt rename the invocation

db13973

treydock mentioned this pull request Sep 30, 2018

This makes sensu_gem work as advertised (cont) #987

Closed

ghoneycutt added the sensu v1 label Oct 1, 2018

ghoneycutt closed this Oct 1, 2018

This makes sensu_gem work as advertised #877

This makes sensu_gem work as advertised #877

Conversation

tibers commented Jan 27, 2018

Pull Request Checklist

Description

Related Issue

Motivation and Context

How Has This Been Tested?

General

majormoses left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tibers commented Jan 27, 2018

tibers commented Jan 30, 2018

majormoses commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghoneycutt commented Jan 30, 2018

tibers commented Jan 30, 2018

majormoses commented Jan 30, 2018 • edited Loading

tibers commented Jan 31, 2018

majormoses commented Jan 31, 2018

majormoses commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majormoses commented Jan 31, 2018 • edited Loading

tibers commented Feb 1, 2018

majormoses commented Feb 1, 2018

ghoneycutt commented Feb 3, 2018 • edited Loading

ghoneycutt commented Feb 3, 2018

majormoses commented Feb 4, 2018

tibers commented Feb 6, 2018

ghoneycutt commented Feb 6, 2018

tibers commented Feb 6, 2018

mbbroberg commented Feb 9, 2018

treydock commented Sep 27, 2018

ghoneycutt commented Sep 27, 2018

treydock commented Sep 27, 2018

ghoneycutt commented Sep 28, 2018

treydock commented Sep 30, 2018

ghoneycutt commented Oct 1, 2018

majormoses commented Jan 30, 2018 •

edited

Loading

majormoses commented Jan 31, 2018 •

edited

Loading

ghoneycutt commented Feb 3, 2018 •

edited

Loading