Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set STATUS_UNINITIALIZED TAG launching head node #14293

Merged
merged 90 commits into from
Feb 24, 2021
Merged

set STATUS_UNINITIALIZED TAG launching head node #14293

merged 90 commits into from
Feb 24, 2021

Conversation

AmeerHajAli
Copy link
Contributor

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
Copy link
Contributor Author

AmeerHajAli commented Feb 23, 2021

@ericl FYI. We need this because non_terminated_nodes is getting KeyError for the head node is when we check the status. This might not happen when we start autoscaler after the head node already started (because the node updater sets some tags in the process and hence the STATUS tag will be in the tags).

Copy link
Contributor

@wuisawesome wuisawesome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should include this in at least one of the testGetOrCreateHeadNode type tests. Other than that lgtm

@AmeerHajAli
Copy link
Contributor Author

We should include this in at least one of the testGetOrCreateHeadNode type tests. Other than that lgtm

are you ok with a patch to the node provider create_node that calls the node provider create node and asserts that the tag was set? Something like:

        def _create_node(....):
            self.provider.create_node()
            assert RAY_STATUS in tags
        with patch self.provider.create_node = _create_node:
            commands.get_or_create_head_node(
                SMALL_CLUSTER,
                printable_config_file=config_path,
                no_restart=False,
                restart_only=False,
                yes=True,
                override_cluster_name=None,
                _provider=self.provider,
                _runner=runner)

@wuisawesome
Copy link
Contributor

Yeah you'll have to add a special case check for unmanaged nodes, but that actually sounds better since we're directly asserting that all nodes started by the autoscaler have a status tag set.

@AmeerHajAli
Copy link
Contributor Author

@wuisawesome , does it look good to you?

@AmeerHajAli AmeerHajAli merged commit 5155673 into ray-project:master Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants