[Feature] Support user's app quota level limit #311

smallzhongfeng · 2022-11-11T18:13:51Z

What changes were proposed in this pull request?

For issue #211 and the design document https://docs.google.com/document/d/1MApSMFQgoS1VAoKbZjomqSRm0iTbSuKG1yvKNlWW65c/edit?usp=sharing

Why are the changes needed?

Better isolation of resources between different users.

Does this PR introduce any user-facing change?

Add config rss.coordinator.quota.default.app.num to set default app number each user and rss.coordinator.quota.default.path to set a path to record the number of apps that each user can run.

How was this patch tested?

Add uts.

jerqi · 2022-11-11T18:29:03Z

Could you modify the description to attach your design document? Do the shuffle server have quota concept? cc @Gustfh

jerqi · 2022-11-11T18:24:20Z

proto/src/main/proto/Rss.proto

@@ -302,6 +302,7 @@ service CoordinatorServer {

 message AppHeartBeatRequest {
  string appId = 1;
+  string user = 2;


Why do the heartbeat need the user?

When we register the shuffle, we put the user information to the shuffle server. Is it enough?

I think it isn't enough. Though user information to the shuffle server, we have to send server heartbeat to coordinator.So we use this app heartBeat to refresh each user' s app number which stored in coordinator.

For coordinator, could we pass the user information to the coordinator?

If the heartbeat of the driver is used to send to the coordinator, the implementation is simple. If the heartbeat of the shuffleServer is used, you need to add a collection attribute in the heartbeat request of the shuffleServer, record the user and the corresponding app list, and then summarize them in the coordinator. Right ? And I haven't found the advantages of doing this for the time being. At present, this pr and our current production environment deployment have enough restrictions.

I can't get your point. I feel it's unnecessary and strange that we put the user information to the heartbeat. We have register the user information to shuffle server. Why do we need heartbeat? We hope we can reduce the repeated information.

I got your point. The coordinator needs to rely on the user to update the life cycle of the app, so the user is added to the heartbeat of the app. As for the repetitive problem you mentioned, do you have a better solution?

If the heartbeat of the driver is used to send to the coordinator, the implementation is simple. If the heartbeat of the shuffleServer is used, you need to add a collection attribute in the heartbeat request of the shuffleServer, record the user and the corresponding app list, and then summarize them in the coordinator. Right ? And I haven't found the advantages of doing this for the time being. At present, this pr and our current production environment deployment have enough restrictions.

What's your restrictions?

At present,restrictions is the number of apps that each user can submit.

codecov-commenter · 2022-11-11T18:34:09Z

Codecov Report

Merging #311 (25b4b78) into master (79d2f54) will decrease coverage by 2.93%.
The diff coverage is 70.87%.

@@             Coverage Diff              @@
##             master     #311      +/-   ##
============================================
- Coverage     61.32%   58.39%   -2.94%     
- Complexity     1526     1552      +26     
============================================
  Files           186      193       +7     
  Lines          9441    10761    +1320     
  Branches        924      937      +13     
============================================
+ Hits           5790     6284     +494     
- Misses         3341     4103     +762     
- Partials        310      374      +64

Impacted Files	Coverage Δ
...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java	`0.00% <0.00%> (ø)`
...he/uniffle/client/impl/ShuffleWriteClientImpl.java	`28.38% <0.00%> (-1.76%)`	⬇️
...he/uniffle/coordinator/CoordinatorGrpcService.java	`2.09% <0.00%> (-0.21%)`	⬇️
.../org/apache/uniffle/coordinator/AccessManager.java	`94.28% <75.00%> (-5.72%)`	⬇️
...a/org/apache/uniffle/coordinator/QuotaManager.java	`88.63% <88.63%> (ø)`
...apache/uniffle/coordinator/ApplicationManager.java	`84.93% <91.17%> (+1.13%)`	⬆️
...apache/uniffle/coordinator/AccessQuotaChecker.java	`95.83% <95.83%> (ø)`
.../java/org/apache/uniffle/common/util/RssUtils.java	`63.35% <100.00%> (+0.28%)`	⬆️
.../apache/uniffle/coordinator/AccessCheckResult.java	`92.30% <100.00%> (+8.97%)`	⬆️
...ava/org/apache/uniffle/coordinator/AccessInfo.java	`92.30% <100.00%> (+1.39%)`	⬆️
... and 13 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

smallzhongfeng · 2022-11-12T06:57:44Z

Could you modify the description to attach your design document? Do the shuffle server have quota concept? cc @Gustfh

For now, shuffle server don't have quota concept.

jerqi · 2022-11-12T09:18:36Z

client-spark/spark2/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java

@@ -203,12 +206,12 @@ public RssShuffleManager(SparkConf sparkConf, boolean isDriver) {
  @Override
  public <K, V, C> ShuffleHandle registerShuffle(int shuffleId, int numMaps, ShuffleDependency<K, V, C> dependency) {
    // If yarn enable retry ApplicationMaster, appId will be not unique and shuffle data will be incorrect,
-    // appId + timestamp can avoid such problem,
+    // appId + uuid can avoid such problem,


Why do we need uuid?

We can't get the appId when we try Access, because the appId is generated after the RssManager is created. In order to support push down, we maintain the uuid as a substitute for the appId, and replace the uuid with the appId after the app heartbeat is reported to the coordinator.

We can put the appId to the AcessInfo when we try to access coordinator.

Do you mean to generate the uuid on the driver?

My fault. We ignore that I can't get the appId in the construct method. Let me think twice.

jerqi · 2022-11-12T14:40:42Z

If we have shuffle server quota, shuffle server will report the some metrics to the coordinator. The metrics will contain user quota info. Maybe we can reuse the information. @Gustfh WDTY?

jerqi · 2022-11-12T14:47:17Z

Maybe we could add a rpc method called registerApplicationInfo for Coordinator.

smallzhongfeng · 2022-11-12T18:42:09Z

Maybe we could add a rpc method called registerApplicationInfo for Coordinator.

You mean to add a request different from RssAppHeartbeat, right?

jerqi · 2022-11-13T02:54:38Z

Maybe we could add a rpc method called registerApplicationInfo for Coordinator.

You mean to add a request different from RssAppHeartbeat, right?

Yes.

smallzhongfeng · 2022-11-17T18:09:10Z

Maybe we could add a rpc method called registerApplicationInfo for Coordinator.

PTAL. @jerqi

jerqi · 2022-11-18T02:16:57Z

coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorGrpcService.java

-  public void appHeartbeat(
-      AppHeartBeatRequest request,
-      StreamObserver<AppHeartBeatResponse> responseObserver) {
+  public void registerApplicationInfo(


Why do we rename appHeartbeat to registerApplicationInfo? I mean that we can add a method registerApplicationInfo.

Maybe we could add a rpc method called registerApplicationInfo for Coordinator.

Didn't it say to add a new rpc method? If not, the shuffle server still get appHeartbeat contained the user's attribute

CoordinatorCrpcService should have heartbeat and registerApplicationInfo methods at the same time.

You mean to keep sendAppHeartbeat, and use registerApplicationInfo to send a request to the coordinator only once for recording. And still use sendAppHeartbeat later, right?

You mean to keep sendAppHeartbeat, and use registerApplicationInfo to send a request to the coordinator only once for recording. And still use sendAppHeartbeat later, right?

Yes. we use the registerApplicationInfo to coordinator and then send heartbeat to coordinator.

jerqi · 2022-11-18T02:41:11Z

coordinator/src/main/java/org/apache/uniffle/coordinator/ApplicationManager.java

@@ -282,6 +328,58 @@ private String getStorageHost(String remoteStoragePath) {
    return storageHost;
  }

+  public void detectUserResource() {


Should the application manager involve the quota logic?

Because we can configure rss.coordinator.quota.default.path to controls whether to execute the logic of detectUserResource. If not configured, the logic will not be executed.

Do we need a quota manager? Or will application manager manage both quota and application?

Maybe quota manager will be better.

jerqi · 2022-11-20T14:57:31Z

coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorGrpcService.java

@@ -228,7 +254,8 @@ public void accessCluster(AccessClusterRequest request, StreamObserver<AccessClu
            new AccessInfo(
                request.getAccessId(),
                Sets.newHashSet(request.getTagsList()),
-                request.getExtraPropertiesMap()
+                request.getExtraPropertiesMap(),
+                request.getUser()


Is this a compatible feature? What will happen if a old client request a new coordinator service?

Updated in 36fb720

jerqi · 2022-11-20T14:58:04Z

client-spark/spark3/pom.xml

@@ -39,6 +39,13 @@
            <artifactId>spark-core_${scala.binary.version}</artifactId>
            <version>${spark.version}</version>
            <scope>provided</scope>
+            <exclusions>


Why do we need this?

I forgot to remove this.

jerqi · 2022-11-20T14:59:29Z

client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java

@@ -239,6 +239,7 @@ public Thread newThread(Runnable r) {
      long heartbeatInterval = conf.getLong(RssMRConfig.RSS_HEARTBEAT_INTERVAL,
          RssMRConfig.RSS_HEARTBEAT_INTERVAL_DEFAULT_VALUE);
      long heartbeatTimeout = conf.getLong(RssMRConfig.RSS_HEARTBEAT_TIMEOUT, heartbeatInterval / 2);
+      client.registerApplicationInfo(appId, heartbeatTimeout, "user");


Could we remove this if mr don't need to register application info?

We need this because applicationManager remove expired app need user. When our Spark does not use the AppQuota checker, it also needs register application info, so it will not have much impact here.

It seems that this isn't compatible feature. If we use old client to connect new service, something wrong will happen.

Yes, because the old client does not have user information or method of registerApplicationInfo, app can only be updated at refreshApp, which I am compatible with.

jerqi · 2022-11-21T06:23:44Z

internal-client/src/main/java/org/apache/uniffle/client/impl/grpc/CoordinatorGrpcClient.java

@@ -222,6 +225,24 @@ public RssAppHeartBeatResponse sendAppHeartBeat(RssAppHeartBeatRequest request)
    return response;
  }

+  @Override
+  public RssApplicationInfoResponse sendApplicationInfo(RssApplicationInfoRequest request) {


sendApplicationInfo -> registerApplicationInfo?

jerqi

LGTM, wait for CI

smallzhongfeng · 2022-11-21T10:59:47Z

Because in the discussion of the last meeting, the shuffle server also needs to be limited, but for the number of apps, the number of apps on a sever may not directly determine the load, so I have not developed this place. WDYT? @jerqi

jerqi · 2022-11-21T11:03:40Z

Because in the discussion of the last meeting, the shuffle server also needs to be limited, but for the number of apps, the number of apps on a sever may not directly determine the load, so I have not developed this place. WDYT? @jerqi

It's ok for me. Wait for a moment. Let me see @Gustfh whether have another suggestion. If he don't reply us, I'll merge this pr tomorrow.

jerqi · 2022-11-22T01:06:08Z

merged. Thanks @smallzhongfeng

xianjingfeng · 2022-11-23T02:28:42Z

@jerqi @smallzhongfeng I think this feature should be an option. WDYT?

jerqi · 2022-11-23T02:34:27Z

@jerqi @smallzhongfeng I think this feature should be an option. WDYT?

Yes, this is an option now. You can see

incubator-uniffle/coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorConf.java

Line 76 in dea310b

    
           public static final ConfigOption<List<String>> COORDINATOR_ACCESS_CHECKERS = ConfigOptions

xianjingfeng · 2022-11-23T02:44:22Z

Yes, this is an option now. You can see

But QuotaManager always be created, and it will create a thread pool.

jerqi · 2022-11-23T04:06:02Z

Yes, this is an option now. You can see

But QuotaManager always be created, and it will create a thread pool.

You can raise a pr to fix this issue.

smallzhongfeng · 2022-11-24T06:23:48Z

There are some unreasonable places here. I will fix it. @jerqi @xianjingfeng

smallzhongfeng added 4 commits November 11, 2022 21:00

[Feature] User's app quota

134de9d

fix log

4a01ea2

fix log

884fb18

add license

85e2764

jerqi reviewed Nov 11, 2022

View reviewed changes

smallzhongfeng added 2 commits November 12, 2022 16:23

pass it

f61ce1a

pass it

e652a6d

jerqi reviewed Nov 12, 2022

View reviewed changes

smallzhongfeng and others added 2 commits November 18, 2022 01:51

add new rpc method registerApplicationInfo

ebdf28a

Update AccessClusterLoadChecker.java

5ee05ed

jerqi requested changes Nov 18, 2022

View reviewed changes

smallzhongfeng and others added 3 commits November 18, 2022 14:11

Merge branch 'master' into app-quota

d2c1156

revert rpc method

3bdd834

add quota manager

7fa69e8

jerqi reviewed Nov 20, 2022

View reviewed changes

compatible with lower version clients

36fb720

jerqi reviewed Nov 21, 2022

View reviewed changes

sendApplicationInfo -> registerApplicationInfo

25b4b78

jerqi approved these changes Nov 21, 2022

View reviewed changes

jerqi changed the title ~~[Feature] User's app quota~~ [Feature] Support user's app quota limit Nov 21, 2022

jerqi changed the title ~~[Feature] Support user's app quota limit~~ [Feature] Support user's app quota level limit Nov 21, 2022

jerqi merged commit 8ff41a5 into apache:master Nov 22, 2022

jerqi mentioned this pull request Nov 22, 2022

[Feature] User's resources quota #211

Closed

[Feature] Support user's app quota level limit #311

[Feature] Support user's app quota level limit #311

Conversation

smallzhongfeng commented Nov 11, 2022 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

jerqi commented Nov 11, 2022

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

smallzhongfeng Nov 12, 2022 • edited

Choose a reason for hiding this comment

codecov-commenter commented Nov 11, 2022 • edited

Codecov Report

smallzhongfeng commented Nov 12, 2022

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

jerqi commented Nov 12, 2022

jerqi commented Nov 12, 2022 • edited

smallzhongfeng commented Nov 12, 2022 • edited

jerqi commented Nov 13, 2022

smallzhongfeng commented Nov 17, 2022

Choose a reason for hiding this comment

smallzhongfeng Nov 18, 2022 • edited

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

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

smallzhongfeng Nov 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

smallzhongfeng commented Nov 21, 2022

jerqi commented Nov 21, 2022

jerqi commented Nov 22, 2022

xianjingfeng commented Nov 23, 2022

jerqi commented Nov 23, 2022

xianjingfeng commented Nov 23, 2022

jerqi commented Nov 23, 2022

smallzhongfeng commented Nov 24, 2022

smallzhongfeng commented Nov 11, 2022 •

edited

smallzhongfeng Nov 12, 2022 •

edited

codecov-commenter commented Nov 11, 2022 •

edited

jerqi commented Nov 12, 2022 •

edited

smallzhongfeng commented Nov 12, 2022 •

edited

smallzhongfeng Nov 18, 2022 •

edited

smallzhongfeng Nov 21, 2022 •

edited