[Autoscaler] Get_Head_Node should return an up-to-date node

[ijrsvt]

Why are these changes needed?

• If multiple head nodes exist (because one of them is in some sort of failed state), calling get_head_node can return the failed node. This PR ensures that get_head_node only returns a node in the up-to-date state.

Related issue number

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[AmeerHajAli]

Hi @ijrsvt , I realize this issue is happening and thanks for taking the initiative here.
My concern is that the tag status we are checking is not always the tag we are looking for. For example, immediately after successfully creating the head node the state is:

head_node_tags[TAG_RAY_NODE_STATUS] = STATUS_UNINITIALIZED

Checking that the status is up to date is not going to catch that.

[AmeerHajAli]

I think the right solution is that when creating a head node, if it fails to create it, we should just terminate it.

[ijrsvt]

Checking that the status is up to date is not going to catch that.

@AmeerHajAli The head node is set to STATUS_UP_TO_DATE before create_or_update_cluster returns. I don't think we need to be worrying about a use case where a user is trying to manipulate the cluster in an two separate places.

I think the right solution is that when creating a head node, if it fails to create it, we should just terminate it.

I don't think this is a good idea because this adds a lot of latency to iterating through writing setup/initialization commands. For example, if a user has a typo in their setup_commands, the behavior of leaving the node running and re-using it for the next attempt avoids the time of (node_shutdown + node_startup).

At it's core, this PR is defending against a bad implementation of a NodeProvider (one that does not properly re-use the failed head_node).

[AmeerHajAli]

The issue is that if there was a bug in the setup commands, the head node will stay running and the status tag will be STATUS_UPDATE_FAILED but the node will be non terminated and the node can be reused in that case.

[AmeerHajAli]

Oh, wait, @ijrsvt, this is not modifying create_or_update_cluster right?

[ijrsvt]

Oh, wait, @ijrsvt, this is not modifying create_or_update_cluster right?

Yep, this does not change create_or_update_cluster!

[AmeerHajAli]

Do you mind adding some warning that other head nodes exist in failed states if there are head nodes with non STATUS_UP_TO_DATE tag?

[ijrsvt]

@AmeerHajAli I can add that, but let me switch my implementation to only call non_terminated_nodes once.

[AmeerHajAli]

LGTM.