Skip to content
This repository has been archived by the owner on Mar 26, 2024. It is now read-only.

Adding support for fixed-sourcetype licenses #173

Closed
wants to merge 23 commits into from

Conversation

paul-hoff
Copy link
Contributor

Adding some logic to support a fixed-sourcetype license

recipes/license_server.rb Outdated Show resolved Hide resolved
recipes/license_server.rb Outdated Show resolved Hide resolved
@acharlieh
Copy link
Contributor

This isn't the only change to support fixed-sourcetype licenses... the licenses are put into a different directory, and the stack_ids are generated. For the single sourcetype case, it seems to be fixed-sourcetype_UPPERCASE(SHA256(sourcetype)) but a question is what happens with multiple.

@paul-hoff paul-hoff changed the title Added logic for fixed-sourcetype Adding support for fixed-sourcetype licenses Sep 27, 2018
Copy link
Contributor

@acharlieh acharlieh left a comment

Choose a reason for hiding this comment

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

We also need to make changes in the creation of the pools in server.conf, each pool has a stack_id associated with it, and that pool maps to the type we have here.

Unless we can figure out a way to handle multiple types of pools gracefully, we should short-circuit and fail if we have multiple types of licenses set.

hash[type] ||= {}
hash[type][key] = value
end
hash
end

unless sourcetype_name.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of breaking this out... we should just do the hash while we're parsing the licenses groups, and set the fixed-sourcetype_SHA as the type key... in that manner, we get rid of the this hash.

(lest we forget that we could have multiple stacks...)

But also, the seeding of that hash on line 29 should be taken out...

unless node.run_state['cerner_splunk']['total_allotted_pool_size'].nil?
total_allotted_pool_size = node.run_state['cerner_splunk']['total_allotted_pool_size']
fail "Sum of pool sizes is #{CernerSplunk.human_readable_size total_allotted_pool_size}. Exceeds total available pool size of #{CernerSplunk.human_readable_size total_available_license_quota}." if total_allotted_pool_size > total_available_license_quota
end

license_groups.each do |type, keys|
prefix = "#{node['splunk']['home']}/etc/licenses/#{type}"
prefix = if type == 'fixed-sourcetype'
Copy link
Contributor

Choose a reason for hiding this comment

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

If we put the hash into the type above then we get rid of this pattern, and also make use of the type variable in the block below.

recipes/license_server.rb Show resolved Hide resolved
quota = doc.at_xpath('/license/payload/quota/text()').to_s.to_i
expiration_time = doc.at_xpath('/license/payload/expiration_time/text()').to_s.to_i
total_available_license_quota += quota if type == 'enterprise' && expiration_time > Time.now.to_i
total_available_license_quota += quota if (type == 'enterprise' || type == 'fixed-sourcetype') && expiration_time > Time.now.to_i
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should restructure the license_groups hash, to have license_quota in there along with the map of licenses... That way we're not worried about the specific license type but also just maintain a quota for each set...

We probably should push the set of types into the run_state, that way we can use them for building out the pools in server.conf, since we need to set stack_id on all those as well.

recipes/license_server.rb Outdated Show resolved Hide resolved
Copy link
Member

@nmattam nmattam left a comment

Choose a reason for hiding this comment

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

Verify that you can successfully converge the license server.

vagrant up chef s_license

recipes/license_server.rb Outdated Show resolved Hide resolved
recipes/license_server.rb Outdated Show resolved Hide resolved
@paul-hoff
Copy link
Contributor Author

I verified that I can successfully converge the license server

==> s_license: Chef Client finished, 36/52 resources updated in 11 minutes 58 seconds

@@ -249,11 +249,11 @@

license_pools['pools'].each do |pool, pool_config|
pool_max_size = CernerSplunk.convert_to_bytes pool_config['size']
server_stanzas["lmpool:#{pool}"] = {
server_stanzas["lmpool:#{node.run_state['type']}"] = {
Copy link
Member

@nmattam nmattam Oct 4, 2018

Choose a reason for hiding this comment

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

Won't this value be empty since we are evaluating this at the compile time? Can you show what the server.conf looks like on the license server?

Copy link
Member

Choose a reason for hiding this comment

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

Also, are we grouping pools by license type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talking with Charlie, he mentioned that for the fixed-sourcetype license the folder and pool are the same, which is fixed-sourcetype_Uppercase(sha256(sourcetypeName)). So, I was trying to account for that here

Gemfile Show resolved Hide resolved
.rubocop.yml Outdated Show resolved Hide resolved
recipes/license_server.rb Outdated Show resolved Hide resolved
'slaves' => pool_config['GUIDs'].join(','),
'stack_id' => node.run_state['type']
}
allotted_pool_size += pool_max_size
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this moves the allotted_pool_size summation from compile time to execution time, I believe that makes the run state for the total_allotted_pool_size incorrect (since the node.run_state is set below, outside the ruby_block at compile time). Then the error checking on line 48-51 of the license_server recipe is no longer accurate.

If one part of it moves to execution time, then all the parts have to move. But then it won't fail the chef run until part way through, which makes that error checking considerably less useful since the bad server conf would have already been written out. Will Splunk fail to start if the pools configured in server.conf are greater than the available licenses?

Copy link
Contributor

@gravesb gravesb Oct 10, 2018

Choose a reason for hiding this comment

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

I checked and Splunk doesn't seem to care if the total allocation of the pools is greater than the amount of the license stack. The service will still start fine. Although if you create or edit a pool via the UI and try to make it larger, something in the web console will set the pool size equal to the amount of available license when it saves it.

Copy link
Contributor

@gravesb gravesb Oct 11, 2018

Choose a reason for hiding this comment

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

I think the easiest fix is to remove the ruby block here and simply re-order the operations in the license_server recipe so the data bag is processed prior to including this recipe. Just move line 18 down between lines 46 and 48 in the license_server recipe.

Copy link
Contributor

@gravesb gravesb left a comment

Choose a reason for hiding this comment

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

lines 73-87 of the license-server.rb recipe don't quite work any more. If you have one type of license and attempt to switch to a different type, the original license type is not cleaned up. For example, if I converge a node with an enterprise license and later converge it with a fixed-sourcetype license, the enterprise license remains.

recipes/license_server.rb Outdated Show resolved Hide resolved
@paul-hoff
Copy link
Contributor Author

Should I use some sort of regular expression on line 76 of license-server.rb instead of the type variable to fix lines 73-87?

@gravesb
Copy link
Contributor

gravesb commented Oct 24, 2018

Yeah, tinkering with that regex may work along with a bit more complex code to exclude the files we want to keep.

Another option would be to add something like this to that ruby block:

Dir.glob("#{node['splunk']['home']}/etc/licenses/*") { |dir| FileUtils.rm_rf(dir) unless dir.end_with?(type) }

may also need to require 'fileutils' somewhere in there along with similar logging to what's there for the other files we delete.

@paul-hoff
Copy link
Contributor Author

I have added this code to the ruby block, and was able to successfully converge the license server. I am unsure how to test its actual functionality since only a single demo license is being used. Is this kind of what you had in mind?

      existing_directory = Dir.glob("#{node['splunk']['home']}/etc/licenses/*")
      delete_directory = existing_directory.delete_if { |dir| FileUtils.rm_rf(dir) unless dir.end_with?(type) }
      delete_directory.each do |directory|
        Chef::Log.info("ruby_block[license cleanup] deleted unconfigured license directory #{directory}")
      end

@gravesb
Copy link
Contributor

gravesb commented Oct 26, 2018

yeah, something like that. Given that you're working on a PR for fixed-sourcetype license support I assume you have access to other licenses that you could use to test locally (though obviously you wouldn't want to commit those changes as part of this PR).

@@ -66,6 +72,11 @@
b = ruby_block 'license cleanup' do
block do
license_groups.each do |type, licenses|
existing_directory = Dir.glob("#{node['splunk']['home']}/etc/licenses/*")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that we only support one type now, but this logically should be outside of the license_groups.each loop

license_groups.each do |type, licenses|
existing_directory.each do |dir|
Copy link
Contributor

Choose a reason for hiding this comment

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

Pull this loop out of the license_groups loop too...

@@ -65,6 +71,11 @@

b = ruby_block 'license cleanup' do
block do
existing_directory = Dir.glob("#{node['splunk']['home']}/etc/licenses/*")
existing_directory.each do |dir|
FileUtils.rm_rf(dir) unless dir.end_with?(type)
Copy link
Contributor

@acharlieh acharlieh Nov 1, 2018

Choose a reason for hiding this comment

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

Alrighty now just need to fix the logic a bit... as we want to only delete those directories that we expect there to be no licenses, so something like: (double check me on this... this is rather haphazard, but should give you the general idea). (also because type is now not defined in this scope )

next if license_groups.keys.include? File.basename(dir)
Chef::Log.info( "... blah..." )
FileUtils.rm_rf(dir)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fixed with d4c6810

hash[type][key] = value
end
node.run_state['type'] = type
Copy link
Contributor

Choose a reason for hiding this comment

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

This should either move as is to right after the fail ... if on line 37
OR
This should be outside the inject loop after the end on line 42, slightly modified like this:

node.run_state['type'] = license_groups.keys.first

That type variable defined on line 31 feels like it's really intended to only be valid within the unless block (line 28 -> 39),

If you happen to be retrieving a level where skipping the keys 'id', 'chef_type' and 'data_bag' mattered, and one of those happened to be traversed prior to your actual key that you're interested in, you'll have a NameError: undefined local variable or method `type' on your hands

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should 'type' be 'license_type' or 'stack_id' ... something a bit more descriptive as a key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense, these changes are reflected in faaab86

gravesb pushed a commit that referenced this pull request Nov 5, 2018
* Add support for fixed-sourcetype licenses
@gravesb
Copy link
Contributor

gravesb commented Nov 5, 2018

Merged in b426f47.

@gravesb gravesb closed this Nov 5, 2018
@gravesb gravesb added this to the v2.23.0 milestone Feb 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants