Nodes stats & other APIs sometimes failing during Rest Compatibility tests

[gwbrown]

This has occurred in builds on a number of PRs against master, none of which seem related.

Build scans:
Failure from #75644: https://gradle-enterprise.elastic.co/s/ql4kt7zoo3ize
Failure from #75805: https://gradle-enterprise.elastic.co/s/jyln72zgaeh7s
Failure from #75819: https://gradle-enterprise.elastic.co/s/z36e76cz2qt7m

Repro line:

./gradlew ':rest-api-spec:yamlRestCompatTest' --tests "org.elasticsearch.test.rest.ClientYamlTestSuiteIT" -Dtests.method="test {yaml=nodes.stats/11_indices_metrics/indices shards total count test}" -Dtests.seed=48A8BB18284355E7 -Dtests.locale=cs -Dtests.timezone=Europe/Saratov -Druntime.java=11

Reproduces locally?: Nope.

Applicable branches: PRs based on master is where it's been observed thus far.

Failure history:
First failure appears to have been today, July 28.

The tests which have been failing most commonly:

ClientYamlTestSuiteIT.test {yaml=nodes.stats/11_indices_metrics/indices shards total count test}
ClientYamlTestSuiteIT.test {yaml=nodes.stats/11_indices_metrics/Metric - blank for indices shards}
ClientYamlTestSuiteIT.test {yaml=nodes.stats/11_indices_metrics/Metric - _all for indices shards}
LangPainlessClientYamlTestSuiteIT.test {yaml=painless/50_script_doc_values/geo_point}

Failure excerpt:

java.lang.AssertionError: Failure at [nodes.stats/11_indices_metrics:472]: field [nodes.$node_id.indices.shards] doesn't have a true value	
Expected: not null	
     but: was null	
	at __randomizedtesting.SeedInfo.seed([48A8BB18284355E7:C0FC84C286BF381F]:0)	
	at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.executeSection(ESClientYamlSuiteTestCase.java:451)	
	at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.test(ESClientYamlSuiteTestCase.java:428)	
	at jdk.internal.reflect.GeneratedMethodAccessor12.invoke(Unknown Source)	
	at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)	
	at java.lang.reflect.Method.invoke(Method.java:566)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1758)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:946)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:982)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:996)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:49)	
	at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)	
	at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:48)	
	at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)	
	at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:375)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:824)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:475)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:955)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:840)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:891)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:902)	
	at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:41)	
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)	
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)	
	at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)	
	at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)	
	at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:54)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:375)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl.lambda$forkTimeoutingTask$0(ThreadLeakControl.java:831)	
	at java.lang.Thread.run(Thread.java:834)	
Caused by: java.lang.AssertionError: field [nodes.$node_id.indices.shards] doesn't have a true value	
Expected: not null	
     but: was null	
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)	
	at org.junit.Assert.assertThat(Assert.java:956)	
	at org.elasticsearch.test.rest.yaml.section.IsTrueAssertion.doAssert(IsTrueAssertion.java:44)	
	at org.elasticsearch.test.rest.yaml.section.Assertion.execute(Assertion.java:65)	
	at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.executeSection(ESClientYamlSuiteTestCase.java:444)	
	at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.test(ESClientYamlSuiteTestCase.java:428)	
	at jdk.internal.reflect.GeneratedMethodAccessor12.invoke(Unknown Source)	
	at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)	
	at java.lang.reflect.Method.invoke(Method.java:566)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1758)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:946)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:982)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:996)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:49)	
	at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)	
	at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:48)	
	at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)	
	at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:375)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:824)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:475)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:955)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:840)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:891)	
	at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:902)	
	at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:41)	
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)	
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)	
	at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)	
	at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)	
	at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:54)	
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:375)	
	at com.carrotsearch.randomizedtesting.ThreadLeakControl.lambda$forkTimeoutingTask$0(ThreadLeakControl.java:831)	
	at java.lang.Thread.run(Thread.java:834)

[elasticmachine]

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

[pgomulka]

I have managed to reproduce this. This is expected behaviour - your branch did not have the latest master changes which were backported to 7.x. We check out latest 7.x when running rest-api-compatibility tests.

reproduce steps:
On this branch #75819 when checking out 09fb4bf16cde8dfd61f2c57476d8e3292ed8f8fa I can run the ./gradlew ':rest-api-spec:yamlRestCompatTest' --tests "org.elasticsearch.test.rest.ClientYamlTestSuiteIT" -Dtests.method="test {yaml=nodes.stats/11_indices_metrics/indices shards total count test}" -Dtests.seed=48A8BB18284355E7 -Dtests.locale=cs -Dtests.timezone=Europe/Saratov -Druntime.java=11 and get the same failure
The reason of this is, that when checking out the 7.x (latest when doing this now, but 64ff1304021e060d476dd5a4c13a16db087f7575 when the test failed on that branch) we don't have latest changes from master.
Those 2 PRs have been backported to 7.x #75760 #75760 and changed the 11_indices_metrics.yml and the logic (I did not dig deep enough to see what changed exactly, but it does not matter).
The branch that failed the CI did not have latest changes with #75433 (the change that was backported) and this caused a failure

After I checked out to 3f493b9706424660ab608123855b4e58b45cf104 (the commit that merged master to the failing branch and contains the #75433) the test passed

@mark-vieira @rjernst we discussed this yesterday on core-infra sync. Do you think we would be able to to "save" the 7.x revision after latest master is merged and then use that when rest-api-compatibilty tests are run?
At the same time I guess this affects other bwc tests in the same way (they check out latests 7x too)

cc @jakelandis

[mark-vieira]

@mark-vieira @rjernst we discussed this yesterday on core-infra sync. Do you think we would be able to to "save" the 7.x revision after latest master is merged and then use that when rest-api-compatibilty tests are run?
At the same time I guess this affects other bwc tests in the same way (they check out latests 7x too)

We pass -Dbwc.checkout.align=true to these jobs which should do what I think you are describing. That is, when I run the test on master we checkout the latest 7.x that is no later than the master commit. Is it possible that for compatibility testing we need the opposite behavior? The "fix" you describe was actually a change to the tests in 7.x rather than fixing compatibility in master. What should be the behavior here? When running rest compatibility tests should we always use the latest commit in 7.x?

[rjernst]

we checkout the latest 7.x that is no later than the master commit

The problem IMO is not on master, but on branches off of master. If I make a commit on my branch, the time of the commit is now later than when I branched from master, yet it will checkout 7.x up to the time of my current commit. What we really want is to use the time of when master was last merged into our current branch (or originally forked). I think what we want is instead of using HEAD to find the time, use git merge-base --fork-point master.

[mark-vieira]

What we really want is to use the time of when master was last merged into our current branch (or originally forked). I think what we want is instead of using HEAD to find the time, use git merge-base --fork-point master.

Ok, so this seems to be specific to pull requests. Isn't the "solution" in this secnario just to update your branch then?

[rjernst]

Yes, updating the branch works. 🤷

The problem often goes like this:

1. PR fails
2. look at failures
3. incomprehensible errors clearly unrelated to PR
4. Merge master
5. Failures go away. Was it merging master or a flaky test?

In this case Gordon created an issue to investigate. I think it can be closed, since Przemek confirmed it was due to being out of sync with a compat change.

[mark-vieira]

In this case Gordon created an issue to investigate. I think it can be closed, since Przemek confirmed it was due to being out of sync with a compat change.

I'm inclined to say the same. There are loads of scenarios where having an out of date PR will cause failures. We could mitigate some of those by effectively testing against older code more likely to be compatible with your branch, but that has the side-effect of possibly leading to shit blowing up after merge.

[pgomulka]

the side-effect of possibly leading to shit blowing up after merge.

I think this is a strong argument to not try testing against older 7.x code. I think the safest will be to just update the branch when bwc or rest-api-compatibility tests fail.
I will close the issue and document that in rest-api-compatibility dev guidebook