Skip to content

[autoscaler] Allow more than 5s from node creation to first heartbeat#3385

Merged
robertnishihara merged 5 commits into
ray-project:masterfrom
ericl:longer-grace-hb
Nov 27, 2018
Merged

[autoscaler] Allow more than 5s from node creation to first heartbeat#3385
robertnishihara merged 5 commits into
ray-project:masterfrom
ericl:longer-grace-hb

Conversation

@ericl

@ericl ericl commented Nov 22, 2018

Copy link
Copy Markdown
Contributor

What do these changes do?

I think this is due to a race condition between when we first mark a node as active and check for restarts. It's possible a node state changes in the background between these two checks, which would result in a spurious restart.

Related issue number

Closes #3361

@AmplabJenkins

Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/9537/
Test FAILed.

Comment thread python/ray/autoscaler/autoscaler.py Outdated
self.provider.internal_ip(node_id), 0)
key = self.provider.internal_ip(node_id)
if key not in self.load_metrics.last_heartbeat_time_by_ip:
self.load_metrics.last_heartbeat_by_ip = time.time()

@mattearllongshot mattearllongshot Nov 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line should read self.load_metrics.last_heartbeat_time_by_ip[key] = time.time() (attribute name, and you need the subscript)

@AmplabJenkins

Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/9547/
Test FAILed.

@AmplabJenkins

Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/9553/
Test FAILed.

@AmplabJenkins

Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/9560/
Test FAILed.

@ericl ericl added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Nov 23, 2018
key = self.provider.internal_ip(node_id)
if key not in self.load_metrics.last_heartbeat_time_by_ip:
self.load_metrics.last_heartbeat_time_by_ip[key] = time.time()
last_heartbeat_time = self.load_metrics.last_heartbeat_time_by_ip[key]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So this used to default to 0, which means that delta (below) would be enormous and then the autoscaler would attempt to restart the node, right?

@robertnishihara robertnishihara merged commit aa94d3d into ray-project:master Nov 27, 2018
@robertnishihara robertnishihara deleted the longer-grace-hb branch November 27, 2018 01:25
@robertnishihara

Copy link
Copy Markdown
Collaborator

@ericl monitor.cc already makes decisions about when a node should be considered dead. Wouldn't it make sense to just use those decisions? Instead of also having the autoscaler make that decision?

@ericl

ericl commented Nov 27, 2018 via email

Copy link
Copy Markdown
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests-ok The tagger certifies test failures are unrelated and assumes personal liability.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants