Fix test clusters to respect user-provided ordering of settings.

[mosche]

This came up in #137149 after migrating ILM REST tests away from the legacy framework.

If configuring dependent loggers, it's important to respect the provided ordering. If logger.org.elasticsearch.xpack.ilm is applied after logger.org.elasticsearch.xpack.ilm.history.ILMHistoryStore, the latter will be removed / overwritten:

     ...
        .setting("logger.org.elasticsearch.xpack.ilm", "TRACE")
        .setting("logger.org.elasticsearch.xpack.ilm.history.ILMHistoryStore", "INFO")

I'm a bit torn, relying on ordering is always a bit fragile. But at the same time I see the value of being able to do the above easily.

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[rjernst]

Isn't the framework just writing out the settings to elasticsearch.yml? In which case, is it ES itself that is not respecting the hierarchy? I suspect the problem is when we recursively set the logger level in child loggers we don't take into account whether the level there is the default or has already been overriden?

[mark-vieira]

We can certainly make this change but as you say, relying on this implicit behavior isn't ideal. Also, you're going to get unexpected results if you mix different APIs here since we evaluate SettingsProviders late and then merge the results so in an example like this, you might not get the ordering you expect:

.setting("my.other.setting", "bar")
.setting("my.setting", () -> "foo") // deferred

[mark-vieira]

Actually looks like it's even more complicated since you can mix settings at the cluster or node level. Here's how the order of precedence works. Keep in mind this is primarily in terms of which settings (if overriden) actually make it to elasticsearch.yml, not necessarily in what order they will appear in that file:

elasticsearch/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/LocalClusterSpec.java

Line 247 in 664a604

* Resolve node settings. Order of precedence is as follows:

[mosche]

Isn't the framework just writing out the settings to elasticsearch.yml?

@rjernst Yes, and there's the issue. When writing the map entries that happens in a stable, but from a users's perspective random order. I'm not sure if there's other dependent settings where ordering could be a problem as well.

In which case, is it ES itself that is not respecting the hierarchy? I suspect the problem is when we recursively set the logger level in child loggers we don't take into account whether the level there is the default or has already been overriden?

ES looks at these one by one in isolation, in the provided (random) order. Currently we're not able to influence that order in tests due to the way elasticsearch.yml is written.

We could of course track the loggers we've already applied to avoid overwriting these. Or alternatively sort them and apply in the right order here

elasticsearch/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java

Lines 196 to 215 in 5e61b1e

	public void apply(Settings value, Settings current, Settings previous) {
	for (String key : value.keySet()) {
	assert loggerPredicate.test(key);
	String component = key.substring("logger.".length());
	if ("level".equals(component)) {
	continue;
	}
	if ("_root".equals(component)) {
	final String rootLevel = value.get(key);
	if (rootLevel == null) {
	Loggers.setLevel(LogManager.getRootLogger(), Loggers.LOG_DEFAULT_LEVEL_SETTING.get(settings));
	} else {
	Loggers.setLevel(LogManager.getRootLogger(), rootLevel);
	}
	} else {
	Loggers.setLevel(LogManager.getLogger(component), value.get(key));
	}
	}
	}
	}

[mosche]

Actually looks like it's even more complicated since you can mix settings at the cluster or node level.

Thanks @mark-vieira, I think that makes this an invalid approach. This needs to be done in ES as @rjernst said. I'll open a PR to propose another approach.

[mosche]

Just for demonstration, currently we'd have to do something like this. This is unfortunate, internally everything is already sorted, but that's lost when generating the keySet.

Something similar would need to be done in ClusterSettings.LoggingSettingUpdater. The code there however doesn't use concrete settings and follows a different strategy. Very easy to get inconsistent behavior that way :/
This could benefit a lot from a more through redesign

[rjernst]

Instead of ordering these, could we keep track of whether a particular logger has an overridden level, or inherits it? Then regardless of which order we process updating, we can decide only to apply/descend into a logger if it is inheriting. Concretely imagine we are setting two log levels:

logger.foo.bar.baz: TRACE
logger.foo.bar: DEBUG

With the current code we would first set foo.bar.baz to trace, but then incorrectly override it to debug when processing the second logging setting. This happens because we blindly descend from foo.bar to set debug level. Ideally we could just look at the logger to see if it has an explicit level. Unfortunately we set the level at each part of the hierarchy, see #20463.

My thought is if we could keep track of something like the explicit level (but that knows about our descent when setting levels), we could skip by that part of the hierarchy, eg foo.bar.baz if we see an explicit level already set.

[rjernst]

Another relevant past issue/discussion I found: #65208

[mosche]

@rjernst Similar to what you suggested, I introduced a configuration context here #137319

In #65208 the issue seems to be a mismatch between cluster state and actual logger configurations in use. That sounds like a much larger and fairly complex issue.