You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "otterc (via GitHub)" <gi...@apache.org> on 2024/03/07 10:22:18 UTC
[PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
otterc opened a new pull request, #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363
### What changes were proposed in this pull request?
This enables a Celeborn Worker to retrieve the application meta from the Master if it hasn't received the secret from the Master before the application attempts to connect to it. Additionally, the Celeborn Worker's SecretRegistry has been converted into an LRU cache to prevent unbounded growth of the registry.
### Why are the changes needed?
This is last change needed for Auth support in Celeborn (https://issues.apache.org/jira/browse/CELEBORN-1011)
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added UTs and part of a bigger change which will be tested end-to-end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#issuecomment-1983205095
@RexXiong @FMX @pan3793 @mridulm @akpatnam25 @rmcyang @waitinfuture @SteNicholas @melonist Please help review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1516282142
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1099,6 +1103,26 @@ private[celeborn] class Master(
}.asJava
}
+ private def handleRequestForApplicationMeta(
+ context: RpcCallContext,
+ pb: PbApplicationMetaRequest): Unit = {
+ val appId = pb.getAppId
+ logDebug(
+ s"Handling request for application meta info $appId.")
+ if (!secretRegistry.isRegistered(appId)) {
+ logWarning(s"Couldn't find the app .")
Review Comment:
fixed it with other changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1516239687
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/WorkerSecretRegistryImpl.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.service.deploy.worker;
+
+import com.google.common.base.Preconditions;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.client.MasterClient;
+import org.apache.celeborn.common.network.sasl.SecretRegistry;
+import org.apache.celeborn.common.protocol.PbApplicationMeta;
+import org.apache.celeborn.common.protocol.PbApplicationMetaRequest;
+
+/** A secret registry that fetches the secret from the master if it is not found locally. */
+public class WorkerSecretRegistryImpl implements SecretRegistry {
+ private static final Logger LOG = LoggerFactory.getLogger(WorkerSecretRegistryImpl.class);
+
+ // MasterClient is created in Worker after the secret registry is created and this order currently
+ // cannot be changed.
+ // So, we need to set the masterClient after the secret registry is created.
+ private MasterClient masterClient;
+ private final Cache<String, String> secretCache;
+
+ public WorkerSecretRegistryImpl(long maxCacheSize) {
+ secretCache = CacheBuilder.newBuilder().maximumSize(maxCacheSize).build();
Review Comment:
I don't see a reason to have an expiration on entries. A more complete solution for this is to remove completed applications from the registry for which I created this jira: https://issues.apache.org/jira/browse/CELEBORN-1268. For now, I think just bounding this cache is sufficient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1517644659
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/WorkerSecretRegistryImpl.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.service.deploy.worker;
+
+import java.util.concurrent.ExecutionException;
+
+import com.google.common.base.Preconditions;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.client.MasterClient;
+import org.apache.celeborn.common.network.sasl.SecretRegistry;
+import org.apache.celeborn.common.protocol.PbApplicationMeta;
+import org.apache.celeborn.common.protocol.PbApplicationMetaRequest;
+
+/** A secret registry that fetches the secret from the master if it is not found locally. */
+public class WorkerSecretRegistryImpl implements SecretRegistry {
+ private static final Logger LOG = LoggerFactory.getLogger(WorkerSecretRegistryImpl.class);
+
+ private final int maxCacheSize;
+ // MasterClient is created in Worker after the secret registry is created and this order currently
+ // cannot be changed.
+ // So, we need to set the masterClient after the secret registry is created.
+ private MasterClient masterClient;
+ private LoadingCache<String, String> secretCache;
+
+ public WorkerSecretRegistryImpl(int maxCacheSize) {
+ this.maxCacheSize = maxCacheSize;
+ }
+
+ public void initialize(MasterClient masterClient) {
+ this.masterClient = Preconditions.checkNotNull(masterClient);
+ CacheLoader<String, String> cacheLoader =
+ new CacheLoader<String, String>() {
+ @Override
+ public String load(String appId) {
+ LOG.debug("Missing the secret for {}; fetching it from the master", appId);
+ PbApplicationMetaRequest pbApplicationMetaRequest =
+ PbApplicationMetaRequest.newBuilder().setAppId(appId).build();
+ try {
+ PbApplicationMeta pbApplicationMeta =
+ masterClient.askSync(pbApplicationMetaRequest, PbApplicationMeta.class);
+ LOG.debug(
+ "Successfully fetched the application meta info for "
+ + appId
+ + " from the master");
+ return pbApplicationMeta.getSecret();
+ } catch (Throwable e) {
+ // We catch Throwable here because masterClient.askSync declares it in its definition.
+ // If the secret is null, the authentication will fail so just logging the exception
+ // here.
+ LOG.error(
+ "Failed to fetch the application meta info for {} from the master", appId, e);
+ }
+ return null;
+ }
+ };
+ secretCache = CacheBuilder.newBuilder().maximumSize(maxCacheSize).build(cacheLoader);
+ }
+
+ /** Gets an appropriate SASL secret key for the given appId. */
+ @Override
+ public String getSecretKey(String appId) {
+ try {
+ return secretCache.get(appId);
+ } catch (ExecutionException ee) {
+ LOG.error("Failed to retrieve the the application meta info for {} ", appId, ee);
+ }
+ return null;
+ }
+
+ @Override
+ public boolean isRegistered(String appId) {
+ return secretCache.getIfPresent(appId) != null;
+ }
+
+ @Override
+ public void register(String appId, String secret) {
+ String existingSecret = secretCache.getIfPresent(appId);
+ if (existingSecret != null && !existingSecret.equals(secret)) {
+ throw new IllegalArgumentException(
Review Comment:
Registration for an application occurs only once, and the associated secret cannot be updated. This approach aligns with the Sasl Authentication mechanism provided by the External Shuffle Service in Spark. Updating the secret is non-trivial and doesn't offer additional benefits in terms of authentication, so it is not supported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#issuecomment-1983224576
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2363?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `85.71429%` with `1 lines` in your changes are missing coverage. Please review.
> Project coverage is 48.97%. Comparing base [(`835437f`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/835437f0b9d89e5c3ce44f5aacad2a7d396da1a8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`f32cb7c`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2363?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1 commits behind head on main.
| [Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2363?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...cala/org/apache/celeborn/common/CelebornConf.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2363?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL0NlbGVib3JuQ29uZi5zY2FsYQ==) | 85.72% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2363?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2363 +/- ##
==========================================
+ Coverage 48.83% 48.97% +0.14%
==========================================
Files 208 208
Lines 12984 12991 +7
Branches 1115 1115
==========================================
+ Hits 6340 6361 +21
+ Misses 6233 6221 -12
+ Partials 411 409 -2
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2363?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1517069226
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4632,4 +4634,12 @@ object CelebornConf extends Logging {
.version("0.5.0")
.intConf
.createWithDefault(8)
+
+ val WORKER_APPLICATION_REGISTRY_CACHE_SIZE: ConfigEntry[Int] =
+ buildConf("celeborn.worker.applicationRegistryCache.size")
+ .categories("worker", "auth")
+ .doc("Cache size of the application registry on Workers.")
+ .version("0.5.0")
+ .intConf
+ .createWithDefault(10000)
Review Comment:
@otterc, @mridulm, could the default value be `workerInfo.getApplicationIdSet.size`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1516060418
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4632,4 +4634,12 @@ object CelebornConf extends Logging {
.version("0.5.0")
.intConf
.createWithDefault(8)
+
+ val WORKER_APPLICATION_REGISTRY_CACHE_SIZE: ConfigEntry[Int] =
+ buildConf("celeborn.worker.applicationRegistryCache.size")
Review Comment:
```suggestion
buildConf("celeborn.worker.applicationRegistry.cache.size")
```
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1099,6 +1103,26 @@ private[celeborn] class Master(
}.asJava
}
+ private def handleRequestForApplicationMeta(
+ context: RpcCallContext,
+ pb: PbApplicationMetaRequest): Unit = {
+ val appId = pb.getAppId
+ logDebug(
+ s"Handling request for application meta info $appId.")
+ if (!secretRegistry.isRegistered(appId)) {
+ logWarning(s"Couldn't find the app .")
Review Comment:
```suggestion
logWarning(s"Could not find the application meta of $appId.")
```
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4632,4 +4634,12 @@ object CelebornConf extends Logging {
.version("0.5.0")
.intConf
.createWithDefault(8)
+
+ val WORKER_APPLICATION_REGISTRY_CACHE_SIZE: ConfigEntry[Int] =
+ buildConf("celeborn.worker.applicationRegistryCache.size")
+ .categories("worker", "auth")
+ .doc("Cache size of the application registry on Workers.")
+ .version("0.5.0")
+ .intConf
+ .createWithDefault(10000)
Review Comment:
Why does the default value of cache size is 10000?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2363: [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master
URL: https://github.com/apache/incubator-celeborn/pull/2363
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1516961960
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/WorkerSecretRegistryImpl.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.service.deploy.worker;
+
+import java.util.concurrent.ExecutionException;
+
+import com.google.common.base.Preconditions;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.client.MasterClient;
+import org.apache.celeborn.common.network.sasl.SecretRegistry;
+import org.apache.celeborn.common.protocol.PbApplicationMeta;
+import org.apache.celeborn.common.protocol.PbApplicationMetaRequest;
+
+/** A secret registry that fetches the secret from the master if it is not found locally. */
+public class WorkerSecretRegistryImpl implements SecretRegistry {
+ private static final Logger LOG = LoggerFactory.getLogger(WorkerSecretRegistryImpl.class);
+
+ private final int maxCacheSize;
+ // MasterClient is created in Worker after the secret registry is created and this order currently
+ // cannot be changed.
+ // So, we need to set the masterClient after the secret registry is created.
+ private MasterClient masterClient;
+ private LoadingCache<String, String> secretCache;
+
+ public WorkerSecretRegistryImpl(int maxCacheSize) {
+ this.maxCacheSize = maxCacheSize;
+ }
+
+ public void initialize(MasterClient masterClient) {
+ this.masterClient = Preconditions.checkNotNull(masterClient);
+ CacheLoader<String, String> cacheLoader =
+ new CacheLoader<String, String>() {
+ @Override
+ public String load(String appId) {
+ LOG.debug("Missing the secret for {}; fetching it from the master", appId);
+ PbApplicationMetaRequest pbApplicationMetaRequest =
+ PbApplicationMetaRequest.newBuilder().setAppId(appId).build();
+ try {
+ PbApplicationMeta pbApplicationMeta =
+ masterClient.askSync(pbApplicationMetaRequest, PbApplicationMeta.class);
+ LOG.debug(
+ "Successfully fetched the application meta info for "
+ + appId
+ + " from the master");
+ return pbApplicationMeta.getSecret();
+ } catch (Throwable e) {
+ // We catch Throwable here because masterClient.askSync declares it in its definition.
+ // If the secret is null, the authentication will fail so just logging the exception
+ // here.
+ LOG.error(
+ "Failed to fetch the application meta info for {} from the master", appId, e);
+ }
+ return null;
+ }
+ };
+ secretCache = CacheBuilder.newBuilder().maximumSize(maxCacheSize).build(cacheLoader);
+ }
+
+ /** Gets an appropriate SASL secret key for the given appId. */
+ @Override
+ public String getSecretKey(String appId) {
+ try {
+ return secretCache.get(appId);
+ } catch (ExecutionException ee) {
+ LOG.error("Failed to retrieve the the application meta info for {} ", appId, ee);
+ }
+ return null;
+ }
+
+ @Override
+ public boolean isRegistered(String appId) {
+ return secretCache.getIfPresent(appId) != null;
+ }
+
+ @Override
+ public void register(String appId, String secret) {
+ String existingSecret = secretCache.getIfPresent(appId);
+ if (existingSecret != null && !existingSecret.equals(secret)) {
+ throw new IllegalArgumentException(
+ "AppId " + appId + " is already registered. Cannot re-register.");
+ }
+ secretCache.put(appId, secret);
Review Comment:
super nit: add if `existingSecret != null`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1517071828
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/WorkerSecretRegistryImpl.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.service.deploy.worker;
+
+import java.util.concurrent.ExecutionException;
+
+import com.google.common.base.Preconditions;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.client.MasterClient;
+import org.apache.celeborn.common.network.sasl.SecretRegistry;
+import org.apache.celeborn.common.protocol.PbApplicationMeta;
+import org.apache.celeborn.common.protocol.PbApplicationMetaRequest;
+
+/** A secret registry that fetches the secret from the master if it is not found locally. */
+public class WorkerSecretRegistryImpl implements SecretRegistry {
+ private static final Logger LOG = LoggerFactory.getLogger(WorkerSecretRegistryImpl.class);
+
+ private final int maxCacheSize;
+ // MasterClient is created in Worker after the secret registry is created and this order currently
+ // cannot be changed.
+ // So, we need to set the masterClient after the secret registry is created.
+ private MasterClient masterClient;
+ private LoadingCache<String, String> secretCache;
+
+ public WorkerSecretRegistryImpl(int maxCacheSize) {
+ this.maxCacheSize = maxCacheSize;
+ }
+
+ public void initialize(MasterClient masterClient) {
+ this.masterClient = Preconditions.checkNotNull(masterClient);
+ CacheLoader<String, String> cacheLoader =
+ new CacheLoader<String, String>() {
+ @Override
+ public String load(String appId) {
+ LOG.debug("Missing the secret for {}; fetching it from the master", appId);
+ PbApplicationMetaRequest pbApplicationMetaRequest =
+ PbApplicationMetaRequest.newBuilder().setAppId(appId).build();
+ try {
+ PbApplicationMeta pbApplicationMeta =
+ masterClient.askSync(pbApplicationMetaRequest, PbApplicationMeta.class);
+ LOG.debug(
+ "Successfully fetched the application meta info for "
+ + appId
+ + " from the master");
+ return pbApplicationMeta.getSecret();
+ } catch (Throwable e) {
+ // We catch Throwable here because masterClient.askSync declares it in its definition.
+ // If the secret is null, the authentication will fail so just logging the exception
+ // here.
+ LOG.error(
+ "Failed to fetch the application meta info for {} from the master", appId, e);
+ }
+ return null;
+ }
+ };
+ secretCache = CacheBuilder.newBuilder().maximumSize(maxCacheSize).build(cacheLoader);
+ }
+
+ /** Gets an appropriate SASL secret key for the given appId. */
+ @Override
+ public String getSecretKey(String appId) {
+ try {
+ return secretCache.get(appId);
+ } catch (ExecutionException ee) {
+ LOG.error("Failed to retrieve the the application meta info for {} ", appId, ee);
+ }
+ return null;
+ }
+
+ @Override
+ public boolean isRegistered(String appId) {
+ return secretCache.getIfPresent(appId) != null;
+ }
+
+ @Override
+ public void register(String appId, String secret) {
+ String existingSecret = secretCache.getIfPresent(appId);
+ if (existingSecret != null && !existingSecret.equals(secret)) {
+ throw new IllegalArgumentException(
Review Comment:
Could the `secret` of the `appId` be updated with different secret? Or unregister the old secret, then register the new secret?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1516662852
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1099,6 +1103,26 @@ private[celeborn] class Master(
}.asJava
}
+ private def handleRequestForApplicationMeta(
+ context: RpcCallContext,
+ pb: PbApplicationMetaRequest): Unit = {
+ val appId = pb.getAppId
+ logDebug(
+ s"Handling request for application meta info $appId.")
Review Comment:
super nit: single line for logDebug ? :-)
```suggestion
logDebug(s"Handling request for application meta info $appId.")
```
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1099,6 +1103,26 @@ private[celeborn] class Master(
}.asJava
}
+ private def handleRequestForApplicationMeta(
+ context: RpcCallContext,
+ pb: PbApplicationMetaRequest): Unit = {
+ val appId = pb.getAppId
+ logDebug(
+ s"Handling request for application meta info $appId.")
+ if (!secretRegistry.isRegistered(appId)) {
+ logWarning(s"Could not find the application meta of $appId.")
+ context.sendFailure(new CelebornException(s"$appId is not registered."))
+ } else {
+ val pbApplicationMeta = PbApplicationMeta.newBuilder()
+ .setAppId(appId)
+ .setSecret(secretRegistry.getSecretKey(appId))
+ .build()
Review Comment:
There is a potential race here between `isRegistered` and `getSecretKey`, right ?
Do we want to do `getSecretKey` always and use non-null value to distinguish whether it is registered ?
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/WorkerSecretRegistryImpl.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.service.deploy.worker;
+
+import com.google.common.base.Preconditions;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.client.MasterClient;
+import org.apache.celeborn.common.network.sasl.SecretRegistry;
+import org.apache.celeborn.common.protocol.PbApplicationMeta;
+import org.apache.celeborn.common.protocol.PbApplicationMetaRequest;
+
+/** A secret registry that fetches the secret from the master if it is not found locally. */
+public class WorkerSecretRegistryImpl implements SecretRegistry {
+ private static final Logger LOG = LoggerFactory.getLogger(WorkerSecretRegistryImpl.class);
+
+ // MasterClient is created in Worker after the secret registry is created and this order currently
+ // cannot be changed.
+ // So, we need to set the masterClient after the secret registry is created.
+ private MasterClient masterClient;
+ private final Cache<String, String> secretCache;
+
+ public WorkerSecretRegistryImpl(long maxCacheSize) {
+ secretCache = CacheBuilder.newBuilder().maximumSize(maxCacheSize).build();
+ }
+
+ /** Gets an appropriate SASL secret key for the given appId. */
+ @Override
+ public String getSecretKey(String appId) {
+ String secret = secretCache.getIfPresent(appId);
+ if (secret == null) {
+ LOG.debug("Missing the secret for {}; fetching it from the master", appId);
+ PbApplicationMetaRequest pbApplicationMetaRequest =
+ PbApplicationMetaRequest.newBuilder().setAppId(appId).build();
+ try {
+ PbApplicationMeta pbApplicationMeta =
+ masterClient.askSync(pbApplicationMetaRequest, PbApplicationMeta.class);
+ LOG.debug(
+ "Successfully fetched the application meta info for " + appId + " from the master");
+ register(pbApplicationMeta.getAppId(), pbApplicationMeta.getSecret());
+ secret = pbApplicationMeta.getSecret();
+ } catch (Throwable e) {
+ // We catch Throwable here because masterClient.askSync declares it in its definition.
+ // If the secret is null, the authentication will fail so just logging the exception here.
+ LOG.error("Failed to fetch the application meta info for {} from the master", appId, e);
+ }
+ }
Review Comment:
Do we want to move this into a CacheLoader for the cache ?
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4632,4 +4634,12 @@ object CelebornConf extends Logging {
.version("0.5.0")
.intConf
.createWithDefault(8)
+
+ val WORKER_APPLICATION_REGISTRY_CACHE_SIZE: ConfigEntry[Int] =
+ buildConf("celeborn.worker.applicationRegistryCache.size")
+ .categories("worker", "auth")
+ .doc("Cache size of the application registry on Workers.")
+ .version("0.5.0")
+ .intConf
+ .createWithDefault(10000)
Review Comment:
To clarify, internally we would be overriding this to a much higher value ... it essentially should be a reasonable approximation of max number of apps.
If app is not found in cache, it will trigger a query to master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#issuecomment-1984816289
> Looks good to me. This is the last PR for Authn, right @otterc ? Awesome to see this feature complete - thanks a lot to the community for the very quick and responsive reviews !!
Yes, this is the last PR for https://issues.apache.org/jira/browse/CELEBORN-1011 :-).
Huge thanks to everyone for the quick feedback and the reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1516811504
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1099,6 +1103,26 @@ private[celeborn] class Master(
}.asJava
}
+ private def handleRequestForApplicationMeta(
+ context: RpcCallContext,
+ pb: PbApplicationMetaRequest): Unit = {
+ val appId = pb.getAppId
+ logDebug(
+ s"Handling request for application meta info $appId.")
+ if (!secretRegistry.isRegistered(appId)) {
+ logWarning(s"Could not find the application meta of $appId.")
+ context.sendFailure(new CelebornException(s"$appId is not registered."))
+ } else {
+ val pbApplicationMeta = PbApplicationMeta.newBuilder()
+ .setAppId(appId)
+ .setSecret(secretRegistry.getSecretKey(appId))
+ .build()
Review Comment:
Done
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1099,6 +1103,26 @@ private[celeborn] class Master(
}.asJava
}
+ private def handleRequestForApplicationMeta(
+ context: RpcCallContext,
+ pb: PbApplicationMetaRequest): Unit = {
+ val appId = pb.getAppId
+ logDebug(
+ s"Handling request for application meta info $appId.")
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1516058180
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/WorkerSecretRegistryImpl.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.service.deploy.worker;
+
+import com.google.common.base.Preconditions;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.client.MasterClient;
+import org.apache.celeborn.common.network.sasl.SecretRegistry;
+import org.apache.celeborn.common.protocol.PbApplicationMeta;
+import org.apache.celeborn.common.protocol.PbApplicationMetaRequest;
+
+/** A secret registry that fetches the secret from the master if it is not found locally. */
+public class WorkerSecretRegistryImpl implements SecretRegistry {
+ private static final Logger LOG = LoggerFactory.getLogger(WorkerSecretRegistryImpl.class);
+
+ // MasterClient is created in Worker after the secret registry is created and this order currently
+ // cannot be changed.
+ // So, we need to set the masterClient after the secret registry is created.
+ private MasterClient masterClient;
+ private final Cache<String, String> secretCache;
+
+ public WorkerSecretRegistryImpl(long maxCacheSize) {
+ secretCache = CacheBuilder.newBuilder().maximumSize(maxCacheSize).build();
Review Comment:
Does `secretCache` need to support expiring?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1516276672
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4632,4 +4634,12 @@ object CelebornConf extends Logging {
.version("0.5.0")
.intConf
.createWithDefault(8)
+
+ val WORKER_APPLICATION_REGISTRY_CACHE_SIZE: ConfigEntry[Int] =
+ buildConf("celeborn.worker.applicationRegistryCache.size")
+ .categories("worker", "auth")
+ .doc("Cache size of the application registry on Workers.")
+ .version("0.5.0")
+ .intConf
+ .createWithDefault(10000)
Review Comment:
I am open to a better default. We have more than 200K apps running per day. The number of apps active at a given will be much smaller than that. I thought 10K is a conservative default and approx will take a few MBs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1516282574
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4632,4 +4634,12 @@ object CelebornConf extends Logging {
.version("0.5.0")
.intConf
.createWithDefault(8)
+
+ val WORKER_APPLICATION_REGISTRY_CACHE_SIZE: ConfigEntry[Int] =
+ buildConf("celeborn.worker.applicationRegistryCache.size")
Review Comment:
fixed with other changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1179] Add support in Celeborn Workers to fetch application meta from the Master [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2363:
URL: https://github.com/apache/incubator-celeborn/pull/2363#discussion_r1518466706
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4632,4 +4634,12 @@ object CelebornConf extends Logging {
.version("0.5.0")
.intConf
.createWithDefault(8)
+
+ val WORKER_APPLICATION_REGISTRY_CACHE_SIZE: ConfigEntry[Int] =
+ buildConf("celeborn.worker.applicationRegistryCache.size")
+ .categories("worker", "auth")
+ .doc("Cache size of the application registry on Workers.")
+ .version("0.5.0")
+ .intConf
+ .createWithDefault(10000)
Review Comment:
workerInfo.getApplicationIdSet changes with new apps being added or removed. I am not sure how we could configure the default value of this config to be the size of that set. I did update the code to unregister an app from the worker's secretRegistry from the cleaner thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org