kb0149 task added

[MartyEwings]

On of your articles adam:

https://support.puppet.com/hc/en-us/articles/218763948-KB-0149-Resolve-Puppet-run-failures-with-SystemStackError-stack-level-too-deep-errors-for-Puppet-Enterprise-3-8-x-to-2018-1-x

[jarretlavallee]

Since travis isn't configured yet, below is the rake output.

$ bundle exec rake syntax lint metadata_lint check:symlinks check:git_ignore check:dot_underscore check:test_file rubocop shellcheck
---> syntax:manifests
---> syntax:templates
---> syntax:hiera:yaml
Running RuboCop...
Inspecting 1 file
.

1 file inspected, no offenses detected
shellcheck tasks/kb0149.sh

In tasks/kb0149.sh line 9:
        if $(/usr/bin/which grep) -q "ss2m" /etc/sysconfig/pe-puppetserver
             ^-- SC2230: which is non-standard. Use builtin 'command -v' instead.


In tasks/kb0149.sh line 11:
        else  $(/usr/bin/which sed) -i 's/\(JAVA_ARGS="\)/JAVA_ARGS="-Xss2m /g' /etc/sysconfig/pe-puppetserver
                ^-- SC2230: which is non-standard. Use builtin 'command -v' instead.


In tasks/kb0149.sh line 20:
        if $(/usr/bin/which grep) -q "ss2m" /etc/default/pe-puppetserver
             ^-- SC2230: which is non-standard. Use builtin 'command -v' instead.


In tasks/kb0149.sh line 22:
        else  $(/usr/bin/which sed) -i 's/\(JAVA_ARGS="\)/JAVA_ARGS="-Xss2m /g' /etc/default/pe-puppetserver
                ^-- SC2230: which is non-standard. Use builtin 'command -v' instead.

[abottchen]

One thing that could make this better would be to make the new stack size configurable. I'm thinking along the lines of specifying a target Xss size as a parameter to the task. If the parameter isn't there, just default to doubling what is currently there. Although this specific task may not be used all that often, the same code could then be re-used for other settings we will write tasks for down the line.

[MartyEwings]

yeah adam, i have some bash and sed that does that for jvm heap here from my early demonstration:

https://github.com/MartyEwings/kb9999/blob/master/tasks/kb9999.sh

we can pick that up for a "REAL" task

[abottchen]

One concern I have is if this task is run when Xss is already set, but to something the customer has already tuned. If they have anything other than Xss2m, this code is going to overwrite it with Xss2m (in fact, it will stack the additional Xss setting so they end up with two of them). That means if they have tuned it up to Xss4m, this code will actually de-tune it. I don't know how common that would be, but I would be inclined to make these tasks as safe as possible to prevent any potential problems for customers that might run them at unexpected times.

[MartyEwings]

i could turn the match to xss and not the 2m part

[abottchen]

That would be good for the task as it is now. Of course, if we are updating it to have the stack be configurable, it would be fine for Xss to already be there, the task would just be updating it to the new value. The text returned could then be "increased stack from X to Y" or "decreased stack from X to Y".

[jarretlavallee]

@abottchen Shouldn't we be specifying this setting in puppet_enterprise::profile::master::java_args instead of modifying it in the sysconfig file? It seems fragile to specify it manually in the sysconfig file, unless it would be used as a temporary workaround to getting the service online. The customer should be able to do something like the following.

 {"Xmx": "4096m", "Xms": "4096m", "Xss2m": ""}

Or in hiera:

puppet_enterprise::profile::master::java_args:
  Xmx: '4096m'
  Xms: '4096m'
  'Xss2m': ''

[jarretlavallee]

This will replace commented out lines in the file as customers will sometimes use those as a backup to specific settings. The regex would match any JAVA_ARGS. Can you modify the regex to match lines without # in them? Something like 's/^\s*\(JAVA_ARGS="\)/JAVA_ARGS="-Xss2m /g' would skip any commented lines.

[abottchen]

@jarretlavallee Yea, putting it in the console or Hiera would be a superior solution. Safely automating that becomes more of a challenge, however. We could pull the classifier data, update the field if present, and then import it back, but that could introduce race conditions if another user is messing with things in the console. Of course, updating Hiera is not possible since that would be in the customer's control repo, and thus out of our reach as a task.

Updating the file directly will work, even if it is less elegant. Setting puppet_enterprise::profile::master::java_args will only overwrite the manual stack size setting if Xss was explicitly added to the hash, so it wouldn't be just a temporary solution.

[MartyEwings]

@jarretlavallee @abottchen Iv updated the match pattern and changed the regex to skip commented lines However regarding the RAKE recommendations, command -v does not always return the binary path, in some cases, particularly with grep, it returns any alias set, so wouldnt work for this use case.

Considering the KB is targetted at RHEL and Ubuntu it should be ok to use Which.

[abottchen]

We will ultimately want to document in the KB that the task specifically sets the stack size to 2m, so if you already have it larger than that or need it larger than that, don't use it.

[MartyEwings]

@abottchen yep that will be part of the addition at the bottom, the manual steps set explicitly to 2m in terms of matching the Task to the original article, i think we are good, ill never get enough content together to launch if we start with every piece of functionality :)

[jarretlavallee]

I just pushed a commit to ignore the shellcheck lint check with a file directive. If we see this same check trigger in many different scripts we will update the travis.yml to ignore a set of checks since there is no rc file. Sorry for being so onto the lint checking and testing. I figure it is a basis for ensuring consistency and quality, so we might as well as start with it. Let's modify it to fit the needs we have as this repo evolves.

@abottchen That makes sense, thanks for clarifying. 👍

@MartyEwings I'll merge this, but I just want your review on the commit I pushed first. When you open the change to the KB with the task, would you mind adding the steps to make the change permanent with the pe module in conjunction to running the task?

[MartyEwings]

Will do! This is an unmanaged setting by default I've found puppet does not remove it unless you regen the whole file, but aye for completeness I'll totally stick it in.

And thanks for the checking this stuff, I'm totally making this up as I go along,