Require units for time and byte sized settings

[mikemccand]

I fixed ImmutableSettings (and the time/byte size value parsers it uses) to by default require units for time and byte/memory-size settings, and fixed a bunch of tests that were missing units in their settings.

SizeValue I left alone since the "singles" inherently has no unit.

I also added a boolean bwc setting settings_require_units (defaults to true) that you can set to false to get back to lenient default units. I put this in InternalSettingsPreparer so it runs "early on", and it's a static boolean in ImmutableSettings. Hopefully most users don't need to set this (i.e. they simply add the missing units to their settings on upgrade), but if they need to, they can just set it in config/elasticsearch.yml.

I fixed the parsing APIs to optionally take a setting name, so that the resulting ElasticsearchIllegalArgumentException includes the name of the troubled setting.

We still accept 0 and -1 without units...

Closes #7616

[jpountz]

Should be coming instead of added. There is a script that runs on releases that replaces all coming with added.

[mikemccand]

Thanks @jpountz I fixed those two.

[jpountz]

I tried to upgrade a node with the changes in this pull request, and there is an infinite loop that tries to allocate the index bu fails due to the fact that the refresh interval doesn't have a unit. I am wondering if we are ok with this behavior or if should only apply this change to new indices and node-level settings to improve the experience?

[clintongormley]

@jpountz definitely the latter. The intention, as I understood it, was to apply this constraint to new settings only

[mikemccand]

Hmm, ideally we would catch existing settings that are missing units, to notify users that they may have a mis-configuration? However, an infinite loop is clearly a bad way to notify the user ... and, also, these settings may be saved away by ES somewhere "secret", not necessarily easily changeable via e.g. elasticsearch.yml.

Hmm in fact if I try to create a new index with index.refresh_interval = 1 (missing units), it also hits apparent infinite loop of shards failing to be created.

Now I'm not sure how we can safely do this change...

[s1monw]

can we check if the index create version to apply these checks?

[clintongormley]

@mikemccand would love to revive this issue, perhaps getting it in for 2.0?

[mikemccand]

I think this issue is really important but I was unsure how to proceed given the back compat issues, e.g. I hit the "infinite loop" even on doing the check for a newly created index.

I think we need to somehow improve ES error handling before we can do this? Or maybe it has already improved since I last tried this ... I'll retest and see.

[mikemccand]

OK I merged master and pushed here; there are some new recently added failing tests because they fail to specify units in their settings...

I confirmed that ES still hits an infinite loop (with some long pause between retries of creating/starting the index) when refresh_interval is missing its unit in create index and also on upgrade.

However the exceptions that come to the node's console log make it clear something is very wrong, and I can improve that message to point to the workaround (disabling all unit checking, globally).

I think it's important that we notify upgrading users that they possibly have a bad mis-configuration e.g. assuming that refresh_interval defaults to seconds not msec, rather than silently ignore it.

Failing that I could explore adding extensive back compatibility code based on the index's create version, but I think that will wind up being quite invasive, since "can you require units" way down inside settings parsing must be plumbed down by the numerous places that get a time or byte sized setting.

I think we should just do a hard break in 2.0, requiring units across the board. Upgrading users can set the global setting to turn this off, and then fix their problematic settings on their own schedule...

[clintongormley]

Also see #10888

[s1monw]

I'd like to revisit this PR since I think it's super super important. I think we should make this a progress over perfection PR and require units only for indices created >= 2.0 and for node level settings. Another option is to require them everywhere and when we start up the cluster in 2.0 we upgrade all the settings to their defaults. I am not sure how feasible the latter is but even if we solve 80% here it's a huge step.

[mikemccand]

I talked to @s1monw (well, he talked, I took notes!) ... here's a tentative plan:

We'll upgrade a pre-2.0 cluster state by assigning default units to any settings that are missing units. This is a bit dangerous (default unit could be wrong!) but it's a nicer upgrade path than throwing hard exc / infinite loop that user can't easily fix. It also matches what pre-2.0 is doing ... I'll log a warning for each setting we did this for.

For node-level settings, we will just always throw an exc: user can go edit the es.yml to fix.

For index-level settings, since we know index.version.created, if the index is pre-2.0, we won't enforce units for the existing settings. If it's >= 2.0, we will. (Or maybe we can also "assign default unit on upgrade" here....?).

And then all APIs that update any cluster or index settings will require units coming in.

[mikemccand]

Closing this in favor of #11437