Manage /etc/elasticsearch/jvm.options for ES5

[cmclaughlin]

I'm using a combination of /etc/sysconfig/elasticsearch which is managed by this formula and /etc/elasticsearch/jvm.options which is not. I think it's best to give users the option to use all the config files, so this simply populates jvm.options from Pillar. I've added the defaults from jvm.options on a fresh installation to the example Pillar too.

Closes issue #20.

[cmclaughlin]

@blbradley

[cmclaughlin]

Oh, the change went untested. Looks like I need to specify Pillar data in kitchen.yaml...

[cmclaughlin]

OK, it's tested now. Let me know what you think of this.

[blbradley]

Hello! I'm doing a review, but I think it would be best if @aboe76 took a look as well.

[blbradley]

I'm curious as to what happens when major_version == 5 and jvm_opts is not set. I'm going to run a test for that. Otherwise, LGTM.

[blbradley]

States run successfully for the case I indicated above. So, LGTM. Thanks for your patience.

I'm gonna give @aboe76 a bit more time to chime in.

[aboe76]

@blbradley and @cmclaughlin thanks for this, I have tested the changes too on a jessie vm and it works, I can now set the jvm.options

[blbradley]

Great success! Thanks @cmclaughlin

[hackel]

I feel like the default JVM options should be included here. All I need to change is the heap size, but that means copying all of the other options, which (had I not found them in your original commit above) is quite tedious, particularly because they may change in the future.

Could the behaviour be changed based on whether elasticsearch:jvm_opts is a string or a dictionary, to maintain backward compatibility?

Then I could do either:

elasticsearch:
  # Just modify defaults:
  jvm_opts:
    Xms: 4g
    Xmx: 4g
    Dlog4j.skipJansi: false
  # Explicitly set complete file contents:
  jvm_opts: |
    -Xms4g
    -Xmx4g
    -Whatever else

[cmclaughlin]

@hackel - As you noted, the defaults may change by version... so the formula would need to track different defaults for different versions. That might be difficult to maintain.

[cmclaughlin]

@hackel ... I was thinking about this a bit more. I totally agree it would be nice not to have to specify all the opts if you only want to change one. We just need to come up with something that's manageable. One way of possibly improving this would be adding two pillars - one that feeds into file.replace... for replacing/changing the defaults, and another that feeds into file.append... for adding options. Please let me know if you have any other ideas.

[hackel]

It would indeed be annoying having to track the defaults, but I believe this is how most salt formulas handle config files. As long as there is flexibility to override it completely, I think it would be reasonable.

The file.replace approach would be dependent on the config file being installed by the package in an expected format. While that's probably fine, it seems like it's probably the wrong approach just in case the format ever changes. I don't know, it's definitely a tough issue that probably just requires more manual care and attention when upgrading.

[blbradley]

@hackel Please file a separate issue if you'd like to continue the discussion.