[Placement Group] Fix placement group removal leak #19138

rkooo567 · 2021-10-06T13:46:47Z

Why are these changes needed?

This is reproducible when

placement group ready task is queued
New node is up and pg is created
ready task is scheduled as soon as pg is created

The issue was that when the new bundle is created, AddLocalInstances function doesn't properly update the total resources which causes the removal to fail (since we don't have enough total resources, freeing resources fails). This causes the resource leakage.

Detailed scenario

1 nodes A: {CPU 16}
1 pg created {CPU: 1, GPU:1} + {CPU:1} * 30

- ready task is scheduled. It is queued since pg is not created yet
- new node is added B: {CPU: 16, GPU:1}
- Now bundles are created. Imagine in node A, 15 bundles are created
- Each bundle adds its wildcard resources -> bundle_[pg_id] will become 1000, 2000, 3000, ... 15000
- Now ready task is scheduled. This occupies the bundle resource. 
- Imagine this happens before all bundles are created; let's say after bundle_[pg_id]: 3000, this happens
- Now available resources is bundle_[pg_id]: 2999 .999
- And then new bundle is created. The calculation logic here is wrong so the resource will become `bundle_[pg_id]: {total: 3999.999, avail:3999.999}`
- Now the bundle is "unremoveable" because before we remove the placement group, we need 4000 bundles
- - ^ This calculation should be fixed in a way that the bundle id would become `bundle_[pg_id]: {total: 4000, avail:3999.999}` -> this is what the PR does

Related issue number

Closes #19131

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 :(

src/ray/raylet/scheduling/cluster_resource_scheduler.cc

rkooo567 · 2021-10-07T05:26:45Z

src/ray/raylet/scheduling/cluster_resource_scheduler.cc

@@ -456,8 +456,7 @@ void ClusterResourceScheduler::AddLocalResourceInstances(

  for (size_t i = 0; i < instances.size(); i++) {
    node_instances->available[i] += instances[i];
-    node_instances->total[i] =
-        std::max(node_instances->total[i], node_instances->available[i]);
+    node_instances->total[i] += instances[i];


This was a bug

Seems reasonable, though I don't get what the previous code was trying to do. @wuisawesome any comment? It seems to blame to f52c855

it is already merged, but check #19138 (comment). (seems like he doesn't remember)

rkooo567 · 2021-10-07T05:26:54Z

src/ray/raylet/scheduling/cluster_task_manager.cc

@@ -268,7 +268,6 @@ void ClusterTaskManager::DispatchScheduledTasksToWorkers(
    WorkerPoolInterface &worker_pool,
    std::unordered_map<WorkerID, std::shared_ptr<WorkerInterface>> &leased_workers) {
  if (num_tasks_waiting_for_dispatch_ == 0) {
-    RAY_LOG(DEBUG) << "No new tasks since last call to dispatch, skipping";


Changed logs in this file are too noisy

ericl · 2021-10-07T17:41:20Z

src/ray/raylet/scheduling/cluster_resource_data.h

@@ -138,7 +138,7 @@ class TaskResourceInstances {
  /// Check whether there are no resource instances.
  bool IsEmpty() const;
  /// Returns human-readable string for these resources.
-  std::string DebugString() const;
+  [[nodiscard]] std::string DebugString(const StringIdMap &string_id_map) const;


What is nodiscard? Can we remove this?

I added it because the lint told me to do this (https://buildkite.com/ray-project/ray-builders-pr/builds/15386#e43a5b55-796a-485c-b5a6-7f41d2ce57e0)

cc @mwtian is this necessary or best practices? Should we keep including this in the linter?

This can be useful in cases where a status code is returned, but it seems too pedantic here. Added you to reviewers of #19175 which disables this check. I can also disable this in a separate PR.

Why would we want to call DebugString() and ignore the result?
It sounds like a bug.

(I like [[nodiscard]] here)

hmm I see. If that's what it means it makes sense to have it. I think it is up to what's the convention we'd like to use

@sasha-s I agree [[nodiscard]] can be useful in many cases, but for a function like DebugString() the benefit seems to be small since ignoring its result is probably not a serious error. The cost here is making the code look too different from "normal" C++. In an alternative world, I think [[nodiscard]], noexcept and const should be the default requiring no annotations, and only the opposite behavior needs annotations. Not requiring these annotations and only tagging them strategically seem more realistic right now.

I would argue that if you have DebugString() in your code and do not check the output, then, most likely, you have some post-debugging artifacts.
Would you want those to be checked in?

Also, to me, "normal" c++ code has [[nodiscard]] almost everywhere, both on functions and on types.

This is a stylistic decision. Given that we currently don't have [[nodiscard]], we should probably defer to the time-tested rule of being consistent with current style decisions in the codebase, which is to not have [[nodiscard]].

Otherwise, we should add it everywhere (definitely should be a separate PR, that would be pretty huge).

rkooo567 added 3 commits October 6, 2021 06:44

Initial work done. not sure if it will work

af70fc7

Merge branch 'master' into fix-gpu-only-node-issue

5aacf1f

done

a2be062

rkooo567 commented Oct 6, 2021

View reviewed changes

src/ray/raylet/scheduling/cluster_resource_scheduler.cc Outdated Show resolved Hide resolved

rkooo567 changed the title ~~Fix gpu only node issue~~ [Placement Group] Fix placement group removal leak Oct 6, 2021

rkooo567 changed the title ~~[Placement Group] Fix placement group removal leak~~ [WIP][Placement Group] Fix placement group removal leak Oct 6, 2021

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 6, 2021

rkooo567 added 2 commits October 6, 2021 21:44

Merge branch 'master' into fix-gpu-only-node-issue

3e99480

Clean up done

41617d6

rkooo567 commented Oct 7, 2021

View reviewed changes

rkooo567 assigned ericl, scv119 and fishbone Oct 7, 2021

rkooo567 removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 7, 2021

rkooo567 changed the title ~~[WIP][Placement Group] Fix placement group removal leak~~ [Placement Group] Fix placement group removal leak Oct 7, 2021

Fix lint error

8bf6170

rkooo567 mentioned this pull request Oct 7, 2021

[Core] RPC based graceful shutdown for raylet #19072

Closed

6 tasks

ericl reviewed Oct 7, 2021

View reviewed changes

ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 7, 2021

Merge branch 'master' into fix-gpu-only-node-issue

d9d99fb

rkooo567 removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 7, 2021

ericl approved these changes Oct 8, 2021

View reviewed changes

ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 8, 2021

ericl merged commit afaee05 into ray-project:master Oct 8, 2021

rkooo567 mentioned this pull request Oct 8, 2021

[Bug] ray monitor reports negative usage #19237

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Placement Group] Fix placement group removal leak #19138

[Placement Group] Fix placement group removal leak #19138

rkooo567 commented Oct 6, 2021 •

edited

Loading

rkooo567 Oct 7, 2021

ericl Oct 8, 2021

rkooo567 Oct 8, 2021

rkooo567 Oct 7, 2021

ericl Oct 7, 2021

rkooo567 Oct 7, 2021

rkooo567 Oct 7, 2021

mwtian Oct 7, 2021

sasha-s Oct 8, 2021

rkooo567 Oct 8, 2021

mwtian Oct 8, 2021 •

edited

Loading

sasha-s Oct 8, 2021

ericl Oct 8, 2021

[Placement Group] Fix placement group removal leak #19138

[Placement Group] Fix placement group removal leak #19138

Conversation

rkooo567 commented Oct 6, 2021 • edited Loading

Why are these changes needed?

Detailed scenario

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwtian Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkooo567 commented Oct 6, 2021 •

edited

Loading

mwtian Oct 8, 2021 •

edited

Loading