BREAKING: Ability to download from download.splunk.com #150

TraGicCode · 2017-10-01T03:12:00Z

This module used to be able to download splunk installer from splunk directly. Unfortunately, when splunk updates their http url paths this module didnt get updated as well. This updates the structure of where the installers should live along with hooking up docker containers for running in travis. Merging this would be considered a breaking change since the folder structure where people are placing the installers at on their local file system are changing to match how splunk NOW does it.

Summary

Updated expected folder structure for the splunk installers
Updated Documentation of folder structure
Updated default download location to be https://download.splunk.com in order to help people use this module easier instead of having to manually prestage the splunk installers.
Updated acceptance tests to pull from download.splunk.com
Updated .sync.yml to include the docker sets that should run under travis for beaker acceptance tests
Fixed a bug where the forwarder is not actually idempotent because of a file mode. 🐛
In order to get acceptance testing working in docker i had to actually get this module to work while inside a docker container for the splunk server using OPTIMISTIC_ABOUT_FILE_LOCKING ( reference: https://github.com/chef-cookbooks/chef-splunk/blob/611be81ac8e533af7a71f0b2836e8de3aa346373/.kitchen.dokken.yml#L114-L126 )

bastelfreak · 2017-10-01T08:10:52Z

.sync.yml

@@ -1,6 +1,8 @@
 ---
 .travis.yml:
  secure: "RrAmWtM6eTjZZzD954AIgR179Pqp14lzHhd/C9cpKbTPynLncuim08CEvjmq+7pgAy9XDg1d02x3udfZt4btR1sBdyNRpNN3yUhWptmGU61HRJdiZq+nSLQkIYsqXanhk+O3NndFojO58WRD01dkWEc6HRHjlivuYNxDXmMkg9k="
+  docker_sets:
+  - set: docker/ubuntu-16.04


Did you test if our centos/ubuntu14.04/debian8/debian9 (voxpupuli/modulesync_config#388) work as well?

Only enabled this one because of testing

Adding more of the docker sets now since i solved the issue with splunkserver running inside a docker container.

bastelfreak · 2017-10-01T08:11:33Z

spec/acceptance/splunk_forwarder_spec.rb

-      pp = <<-EOS
-      class { '::splunk::forwarder': }
-      EOS
+# describe 'splunk::forwarder class' do


isn't this spec file needed anymore, or did you disable it just for testing?

Disabled just for testing

Reenabled these since tests are now working inside a docker container.

TraGicCode · 2017-10-01T14:10:46Z

Before this can be merged i need to figure out why splunk is blowing up on the splunk start command when inside a container. Below is the issue i'm having and the suggested solution. I'm not entirely convinced this is the real solution or even if this works. I will be looking at how others are currently doing their splunk server docker containers.

https://answers.splunk.com/answers/323671/splunk-light-running-on-docker-throwing-unusable-f.html

wyardley · 2017-10-01T21:19:26Z

spec/acceptance/splunk_spec.rb

+      class { '::splunk::params':
+        version => '7.0.0',
+        build   => 'c8a78efdd40f',
+        src_root => 'https://download.splunk.com',


This should be unnecessary, no? (since it's the same as params)?

More generally, I'm not sure about passing stuff into params directly, though I know the module did this before as well.

I can remove this since i did change it to the defaults. Shall i remove this?

Also yes this module uses params in a non-standard way. Partly in fact because both the forwarder and the server try and reuse the params class.

I saw that postgresql module uses postgresql::global or something along those lines to work around setting things that are common to multiple classes (and then pulls those in / out of params as necessary) - not sure if that's a more standard pattern though... most modules these days seem to be leaning towards having the main class be the interface to everything, and then having toggles there (enable_forwarder = true in class splunk). Probably not worth changing in this PR, but might be worth asking about, and possibly changing down the road.

Yep I agree there are a couple different approaches. That is a beast for another day!

wyardley · 2017-10-01T21:20:20Z

spec/acceptance/splunk_forwarder_spec.rb

+        version      => '7.0.0',
+        build        => 'c8a78efdd40f',
+        src_root     => 'https://download.splunk.com',
+        splunkd_port => 8090,


Ditto here, this should be mostly default (except port?)

I had to do this because currently the acceptance test try to install both a splunk forwarder and a splunk enterprise instance on the same node. Each product has a management utility that listens on port 8089 by default. To get around this i just told the forwarder to be on the non-default port.

While this package does allow for uninstalling your splunk forwarder and splunk enterprise instance the uninstallation is technically not quite correct. In order to prevent adding more scope to this already existing bug i did what i did above.

But aren't version / build / src_root already set to those by default in params?
But if you're only setting the forwarder to a non-default port, can't splunkd_port be set in splunk::forwarder directly?

I suppose. that is the problem this module makes it confusing with its use of params.pp

I will:
1.) remove the defaults from params.pp
2.) move the splunkd_port to the forwarder class

wyardley · 2017-10-01T21:21:28Z

manifests/init.pp

@@ -103,6 +103,21 @@
    tag      => 'splunk_server',
  }

+  if $facts['virtual'] == 'docker' {
+    ini_setting { 'OPTIMISTIC_ABOUT_FILE_LOCKING':


Do you know if this is always needed in any Docker container? Or is it just required for the acceptance tests to work in Travis?

Both. I don't know the exact details but if you google splunk docker unusable filesystem This is pretty much the solution everyone provides. Even the chef-splunk repo did this but this was for their acceptance test. I Also could not find exact details on why/how this actually works. If you want to try this out locally under docker remove that line and watch it fail with the above error message.

Yeah, I saw the comment before. Just wondering whether it's best to do this in the module or in the acceptance test; I think a case could possibly be made either way, but seems kind of magical, esp. since there's no parameter to control it.

It sounds like if you setup the volumes correctly, it may not be necessary to set this?
https://hub.docker.com/r/alexanderm/docker-splunk/

To work around the unusable filesystem issue you need to mount in a volume from the host for the /opt/splunk/var/lib For example I started docker like this: docker run -v /data/splunk-test:/opt/splunk/var/lib -p 8000:8000 -it alexanderm/docker-splunk bash

@wyardley I saw that but that isn't "Applicable" i believe in our case. When running docker acceptance tests we are not taking a folder on the host and mapping it to a folder in the docker container. Make sense?

Ah, yeah, I thought this change was in the manifest itself, not in the test. I agree that this is a reasonable workaround in the acceptance test itself.

wyardley · 2017-10-02T03:50:51Z

spec/acceptance/splunk_spec.rb

@@ -5,6 +5,8 @@
    # Using puppet_apply as a helper
    it 'works idempotently with no errors' do
      pp = <<-EOS
+      class { '::splunk::params':
+      }


I don't think you need to invoke splunk::params explicitly at all, do you?

I agree :). Let me see if I can remove this without things going haywire

wyardley

This looks good to me. I think the splunk::params invocation is superfluous though?

TraGicCode · 2017-10-02T11:43:24Z

i'm going to try and add a few other docker sets to this before this gets merged.

TraGicCode · 2017-10-02T13:04:18Z

@wyardley I added more docker nodesets as noted below:

centos-6
centos-7
debian-7
debian-8

TraGicCode · 2017-10-02T13:08:19Z

closes #149

bastelfreak · 2017-10-03T21:26:44Z

.sync.yml

+  - set: docker/debian-7
+  - set: docker/debian-8
+  - set: docker/ubuntu-14.04
+  - set: docker/ubuntu-16.04


can you split the new nodesets at least into an own commit? Or a separate PR.

Sure thing!

… download.splunk.com

…a docker container ( both for travis-ci and users who want to prebuild docker containers with splunk installed ).

… i need to force the forwarder to listen on a different port. This prevents splunk server and splunk forwarder ( which is being run on the same machine ) from fightining over ports.

…is also needed to get the idempotence checks to not fail on travis-ci.

TraGicCode requested review from wyardley, bastelfreak and alexjfisher October 1, 2017 03:12

bastelfreak reviewed Oct 1, 2017

View reviewed changes

TraGicCode closed this Oct 1, 2017

TraGicCode reopened this Oct 1, 2017

wyardley reviewed Oct 1, 2017

View reviewed changes

wyardley reviewed Oct 2, 2017

View reviewed changes

TraGicCode added backwards-incompatible enhancement New feature or request labels Oct 2, 2017

TraGicCode removed the request for review from alexjfisher October 3, 2017 21:17

bastelfreak reviewed Oct 3, 2017

View reviewed changes

TraGicCode added 5 commits October 3, 2017 16:36

Updated code to start using the *new* download path being followed by…

e21b53c

… download.splunk.com

Add docker nodesets we want to run under travis-ci

f1d8ca1

Add code to actually allow this module to work properly while inside …

af3c081

…a docker container ( both for travis-ci and users who want to prebuild docker containers with splunk installed ).

Since there is a bug where splunk doesn't uninstall cleanly currently…

e3d9b6e

… i need to force the forwarder to listen on a different port. This prevents splunk server and splunk forwarder ( which is being run on the same machine ) from fightining over ports.

Fix a bug in the forwarder where it is not actually idempotent. This …

a933014

…is also needed to get the idempotence checks to not fail on travis-ci.

bastelfreak approved these changes Oct 3, 2017

View reviewed changes

bastelfreak merged commit e3b2b4f into voxpupuli:master Oct 3, 2017

TraGicCode removed the enhancement New feature or request label Oct 3, 2017

TraGicCode changed the title ~~[BREAKING CHANGE] Updated http download structure~~ BREAKING: Updated http download structure Oct 3, 2017

TraGicCode added the enhancement New feature or request label Oct 4, 2017

TraGicCode changed the title ~~BREAKING: Updated http download structure~~ BREAKING: Ability to download from download.splunk.com Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING: Ability to download from download.splunk.com #150

BREAKING: Ability to download from download.splunk.com #150

TraGicCode commented Oct 1, 2017 •

edited

Loading

bastelfreak Oct 1, 2017

TraGicCode Oct 1, 2017

TraGicCode Oct 1, 2017

bastelfreak Oct 1, 2017

TraGicCode Oct 1, 2017

TraGicCode Oct 1, 2017

TraGicCode commented Oct 1, 2017 •

edited

Loading

wyardley Oct 1, 2017

TraGicCode Oct 1, 2017

wyardley Oct 1, 2017

TraGicCode Oct 2, 2017

wyardley Oct 1, 2017

TraGicCode Oct 1, 2017

wyardley Oct 1, 2017

TraGicCode Oct 1, 2017

TraGicCode Oct 1, 2017

wyardley Oct 1, 2017

TraGicCode Oct 1, 2017

wyardley Oct 1, 2017

TraGicCode Oct 1, 2017

wyardley Oct 2, 2017

wyardley Oct 2, 2017

TraGicCode Oct 2, 2017

TraGicCode Oct 2, 2017

wyardley left a comment

TraGicCode commented Oct 2, 2017

TraGicCode commented Oct 2, 2017

TraGicCode commented Oct 2, 2017 •

edited

Loading

bastelfreak Oct 3, 2017

TraGicCode Oct 3, 2017

BREAKING: Ability to download from download.splunk.com #150

BREAKING: Ability to download from download.splunk.com #150

Conversation

TraGicCode commented Oct 1, 2017 • edited Loading

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

Choose a reason for hiding this comment

TraGicCode commented Oct 1, 2017 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyardley left a comment

Choose a reason for hiding this comment

TraGicCode commented Oct 2, 2017

TraGicCode commented Oct 2, 2017

TraGicCode commented Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TraGicCode commented Oct 1, 2017 •

edited

Loading

TraGicCode commented Oct 1, 2017 •

edited

Loading

TraGicCode commented Oct 2, 2017 •

edited

Loading