Get node ID from nodes info in REST tests

[DaveCTurner]

We discussed recently that the cluster state API should be considered
"internal" and therefore our usual cast-iron stability guarantees do not hold
for this API.

However, there are a good number of REST tests that try to identify the master
node. Today they call GET /_cluster/state API and extract the master node ID
from the response. In fact many of these tests just want an arbitary node ID
(or perhaps a data node ID) so an alternative is to call GET _nodes or GET _nodes/data:true and obtain a node ID from the keys of the nodes map in the
response.

This change adds the ability for YAML-based REST tests to extract an arbitrary
key from a map so that they can obtain a node ID from the nodes info API
instead of using the master node ID from the cluster state API.

Relates #40047.

[elasticmachine]

Pinging @elastic/es-distributed

[elasticmachine]

Pinging @elastic/es-core-infra

[javanna]

Do we plan on removing the master node info from the cluster state output? I understand that cluster state is not a stable API but I wonder if/why we need to stop using it everywhere in our own tests? I would prefer if it could be replaced without adding new features to the yaml test runner I guess. cc @elastic/es-clients

[DaveCTurner]

No, there's no plan to remove anything else from the output @javanna. This PR arose out of a recent discussion about some another breaking change to the cluster state API (see #40061) and grew into a need to identify any users of this API and if possible to migrate them to an API that we could guarantee to be stable.

As I see it, the choices for identifying the master node are either (a) carry on using the cluster state API, (b) add a whole new API endpoint just for this, or (c) adjust the test runner to support an alternative way to get the master node ID.

I looked a bit harder at how the tests were using the master node ID, and in many cases we're just using it as an easy way to get an arbitrary node ID, with no real interest in whether it's the master or not. For instance in the shrink/split tests we do the shrink/split on the master because there doesn't seem to be an easy way to pick another node ID, although really we should be picking an arbitrary data node and not the master at all. Perhaps that's the missing feature in the test runner?

[javanna]

Agreed, as far as I remember those tests just need any node. anybody @elastic/es-clients has a preference on how to move forward?

[Mpdreamz]

I quite like this feature. Can be used to make tests less rigid, I remember looking at other tests with @estolfo in the past that could have benefited from this as well. Can't recall the specific tests though.

[DaveCTurner]

Hearing no objections, and at least some support, I think we should proceed with this.

[javanna]

LGTM thanks @DaveCTurner . Shall we add a check that wherever we use the feature we also have a skip for it? We do it for other features in general to avoid breaking other runners tests without knowing.

[DaveCTurner]

I do like your suggestion to fail tests if we use _arbitrary_key_ without having declared this feature, @javanna, but it looks like it involves an awful lot of plumbing to bring the declared features and the parsed path together into one place to perform this check. Perhaps I’m missing something? I could just look for the substring _arbitrary_key_ in the appropriate field of the SetSection perhaps? Could you take a look and see if you can find a better idea?

[javanna]

heya @DaveCTurner would it be possible to add the check to ClientYamlTestSuite#validateExecutableSections ? It should be similar to what we already have there, though this new feature is not just a feature of the do section like others, and this may complicate things. If it gets too complicated, I am fine with not performing this check.

[DaveCTurner]

I'm declaring this to be too complicated. The trouble with adding it to validateExecutableSections is that it's hard to tell if we're using this feature in any of the SetSections in the test.