[Core][Node Labels 3/n]Add node labels to node resources and publish to all node #36009

larrylian · 2023-06-02T03:36:23Z

Why are these changes needed?

Add node labels to node resources and publish to all node

Related issue number

Enhancing node affinity scheduling feature through node labels #34894
(P1)Parse the configuration parameters for node labels and save them in the NodeInfo data structure.

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
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 added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
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

jjyao · 2023-06-02T04:02:46Z

src/ray/raylet/scheduling/cluster_resource_manager.cc

+  if (it == nodes_.end()) {
+    NodeResources node_resources;
+    it = nodes_.emplace(node_id, node_resources).first;
+  }


it's a bit weird that we can have node with only labels but empty resources. We should be able to set labels and resources together when we add a node?

I have reconsidered and still think it's better to keep this interface.

it's not weird because the original code logic already had cases where the total resources of a node were not set.

To update resources, use UpdateResourceCapacity, and to update labels, use UpdateNodeLabels. This way, the interface is more focused, and the code is simpler.

I'm actually wondering when we may not have total resources. Should it be a check instead of warning? Let me check with the team.

Can we actually change if (it == nodes_.end()) { to CHECK which makes code easier to reason about.

My current implementation requires me to retain this logic. Since the labels information is already available in 'GcsNodeInfo', I will first call 'ResetNodeLabels()', and then update the resources.

jjyao · 2023-06-02T04:04:21Z

src/ray/raylet/scheduling/cluster_resource_data.cc

  NodeResources node_resources;
  node_resources.total = ResourceMapToResourceRequest(resource_map_total, false);
  node_resources.available = ResourceMapToResourceRequest(resource_map_available, false);
+  node_resources.labels = node_labels;


After we have labels, the current naming (NodeResources) is no longer that accurate. We should probably have something like

class Node { NodeResources resources; map<string, string> labels; }

Labels can also be regarded as a resource of node. So it is very suitable to put it in NodeResources, and it reduces a lot of code changes.

NodeResources do not need to be computable. The ResourceRequest in NodeResources is Computable. Adding labels to NodeResources does not affect the original available/tatal, etc.

Cool! Got the design.

src/ray/gcs/gcs_server/gcs_resource_manager.cc

jiwq · 2023-06-05T10:53:26Z

src/ray/raylet/scheduling/cluster_resource_data.cc

  NodeResources node_resources;
  node_resources.total = ResourceMapToResourceRequest(resource_map_total, false);
  node_resources.available = ResourceMapToResourceRequest(resource_map_available, false);
+  node_resources.labels = node_labels;


Cool! Got the design.

jjyao · 2023-06-13T00:05:18Z

src/ray/raylet/scheduling/cluster_resource_manager.cc

+  if (it == nodes_.end()) {
+    NodeResources node_resources;
+    it = nodes_.emplace(node_id, node_resources).first;
+  }


Can we actually change if (it == nodes_.end()) { to CHECK which makes code easier to reason about.

src/ray/raylet/node_manager.h

src/ray/gcs/gcs_server/gcs_resource_manager.cc

jjyao · 2023-06-13T00:20:19Z

src/ray/raylet/scheduling/cluster_resource_manager.cc

+    const scheduling::NodeID &node_id,
+    const absl::flat_hash_map<std::string, std::string> &labels) {
+  if (labels.empty()) {
+    return;


I don't think we should early return here. If labels are empty then we should just set node labels to empty.

Same as above.
I've thought about it, and for the scenario where nodes are added, the semantics should be 'reset' instead of 'update', since the labels are static. Therefore, I have changed the interface to 'ResetNodeLabels()'. If dynamic labels are added in the future, I will create a new interface called 'UpdateNodeLabels()'.

src/ray/raylet/scheduling/cluster_resource_manager.cc

jjyao · 2023-06-13T16:02:21Z

src/ray/gcs/gcs_server/gcs_resource_manager.cc

+
+  absl::flat_hash_map<std::string, std::string> labels(node.labels().begin(),
+                                                       node.labels().end());
+  cluster_resource_manager_.ResetNodeLabels(scheduling_node_id, labels);


nit: SetNodeLabels()

jjyao · 2023-06-13T16:31:00Z

src/ray/raylet/node_manager.h

@@ -237,6 +237,7 @@ class NodeManager : public rpc::NodeManagerServiceHandler,
  /// Handler for the addition or updation of a resource in the GCS
  /// \param node_id ID of the node that created or updated resources.
  /// \param createUpdatedResources Created or updated resources.
+  /// \param labels Created or updated labels of this node.


remove this line

jjyao · 2023-06-13T16:38:22Z

src/ray/raylet/scheduling/cluster_resource_scheduler.cc

@@ -44,10 +44,11 @@ ClusterResourceScheduler::ClusterResourceScheduler(
    const absl::flat_hash_map<std::string, double> &local_node_resources,
    std::function<bool(scheduling::NodeID)> is_node_available_fn,
    std::function<int64_t(void)> get_used_object_store_memory,
-    std::function<bool(void)> get_pull_manager_at_capacity)
+    std::function<bool(void)> get_pull_manager_at_capacity,
+    const absl::flat_hash_map<std::string, std::string> &local_node_labels)


Where is the change that creates the ClusterResourceScheduler and pass in the labels?

all node Signed-off-by: LarryLian <554538252@qq.com>

…to all node(ray-project#36009) Signed-off-by: LarryLian <554538252@qq.com> Signed-off-by: e428265 <arvind.chandramouli@lmco.com>

larrylian requested review from scv119, jjyao, wumuzi520 and SongGuyang June 2, 2023 03:36

larrylian requested a review from a team as a code owner June 2, 2023 03:36

larrylian assigned jjyao Jun 2, 2023

jjyao reviewed Jun 2, 2023

View reviewed changes

jiwq reviewed Jun 5, 2023

View reviewed changes

larrylian force-pushed the node_labels_3 branch from 4ab63dd to 3c9d48e Compare June 12, 2023 09:14

larrylian requested review from jjyao and jiwq June 12, 2023 09:21

jjyao reviewed Jun 13, 2023

View reviewed changes

larrylian force-pushed the node_labels_3 branch 2 times, most recently from 3781ee3 to 3942b14 Compare June 13, 2023 04:00

jjyao approved these changes Jun 13, 2023

View reviewed changes

[Core][Node Labels 3/n]Add node labels to node resources and publish to

a114500

all node Signed-off-by: LarryLian <554538252@qq.com>

larrylian force-pushed the node_labels_3 branch from 3942b14 to a114500 Compare June 14, 2023 02:17

larrylian mentioned this pull request Jun 14, 2023

[Core] Enhancing node affinity scheduling feature through node labels #34894

Open

28 tasks

jjyao merged commit 887c03f into ray-project:master Jun 14, 2023

akshay-anyscale mentioned this pull request Jul 21, 2023

Add service deployment instructions to stable diffusion template #37645

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core][Node Labels 3/n]Add node labels to node resources and publish to all node #36009

[Core][Node Labels 3/n]Add node labels to node resources and publish to all node #36009

larrylian commented Jun 2, 2023 •

edited

Loading

jjyao Jun 2, 2023

larrylian Jun 12, 2023

jjyao Jun 12, 2023

jjyao Jun 13, 2023

larrylian Jun 13, 2023

jjyao Jun 2, 2023

larrylian Jun 2, 2023

jiwq Jun 5, 2023

jiwq Jun 5, 2023

jjyao Jun 13, 2023

jjyao Jun 13, 2023

larrylian Jun 13, 2023 •

edited

Loading

jjyao Jun 13, 2023

larrylian Jun 14, 2023

jjyao Jun 13, 2023

larrylian Jun 14, 2023

jjyao Jun 13, 2023

larrylian Jun 14, 2023

[Core][Node Labels 3/n]Add node labels to node resources and publish to all node #36009

[Core][Node Labels 3/n]Add node labels to node resources and publish to all node #36009

Conversation

larrylian commented Jun 2, 2023 • edited Loading

Why are these changes needed?

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

Choose a reason for hiding this comment

larrylian Jun 13, 2023 • edited Loading

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

larrylian commented Jun 2, 2023 •

edited

Loading

larrylian Jun 13, 2023 •

edited

Loading