You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "turboFei (via GitHub)" <gi...@apache.org> on 2024/01/17 06:58:07 UTC
[PR] Check local disk and ignore bad disk when initializing worker storage manager [incubator-celeborn]
turboFei opened a new pull request, #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### 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?
--
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-1230] Check working directory writable when getting device disk info [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1939565255
> Another alternative is to extract `LocalDeviceMonitor#startCheck#run` into a method, and explicitly call the method after base working directories are created, so it will be marked as `READ_OR_WRITE_FAILURE` in the initialization of `StorageManager` if the disk is not available, and no slots will be allocated on it. `LocalDeviceMonitor#readWriteError` will always try to create the directory, so even though the base working directory is not created in startup, whenever the disk goes back to healthy, the directory will be created.
checking
--
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-1230] Check local disk and ignore bad disk when initializing worker storage manager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1895220681
> ... if only one disk is used and the disk is bad, the worker still serves traffic, but all the requests will fail.
I suppose the design of Celeborn can handle bad / run-out-of-space issues at runtime dynamically. Permanently excluding a bad disk at startup is too rough.
--
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-1230] Check local disk and ignore bad disk when initializing worker storage manager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#discussion_r1454767302
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/storage/StorageManager.scala:
##########
@@ -61,7 +61,18 @@ final private[worker] class StorageManager(conf: CelebornConf, workerSource: Abs
// (deviceName -> deviceInfo) and (mount point -> diskInfo)
val (deviceInfos, diskInfos) = {
val workingDirInfos =
- conf.workerBaseDirs.map { case (workdir, maxSpace, flusherThread, storageType) =>
+ conf.workerBaseDirs.flatMap { case (dir, maxCapacity, flushThread, diskType) =>
+ try {
+ Files.createDirectories(new File(dir, conf.workerWorkingDir).toPath)
+ Some((dir, maxCapacity, flushThread, diskType))
+ } catch {
+ case e: IOException =>
+ log.error(
Review Comment:
nit: logError
--
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-1230] Check working directory read and write error without init delay [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1947335979
thanks @waitinfuture
--
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-1230] Check local disk and ignore bad disk when initializing worker storage manager [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1895212579
> Previously, it could tolerate temporary disk issues, e.g. some configured disks have no permission to write due to wrong setup, and the disk will be back once the permission issue gets fixed. But after this change, the issued disks will be excluded forever until restarting the process?
Yes.
But previously, if only one disk is used and the disk is bad, the worker still serves traffic, but all the requests will fail.
--
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-1230] Check working directory read and write error without init delay [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1945137873
> Another alternative is to extract `LocalDeviceMonitor#startCheck#run` into a method, and explicitly call the method after base working directories are created, so it will be marked as `READ_OR_WRITE_FAILURE` in the initialization of `StorageManager` if the disk is not available, and no slots will be allocated on it. `LocalDeviceMonitor#readWriteError` will always try to create the directory, so even though the base working directory is not created in startup, whenever the disk goes back to healthy, the directory will be created.
How about just set the init delay for LocalDeviceMonitor#startCheck task scheduling to 0? @waitinfuture
--
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-1230] Check local disk and ignore bad disk when initializing worker storage manager [incubator-celeborn]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1936932236
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.
--
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-1230] Check local disk and ignore bad disk when initializing worker storage manager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1895179548
It seems to change the behavior.
Previously, it could tolerate temporary disk issues, e.g. some configured disks have no permission to write due to wrong setup, and the disk will be back once the permission issue gets fixed. But after this change, the issued disks will be excluded forever until restarting the process?
--
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-1230] Check working directory writable when getting device disk info [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#discussion_r1490201657
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/storage/DeviceMonitor.scala:
##########
@@ -317,4 +319,105 @@ object DeviceMonitor extends Logging {
}
def EmptyMonitor(): DeviceMonitor = EmptyDeviceMonitor
+
+ /**
+ * @param workingDirs array of (workingDir, max usable space, flush thread count, storage type)
+ * @return it will return two maps
+ * (deviceName -> deviceInfo)
+ * (mount point -> diskInfo)
+ */
+ def getDeviceAndDiskInfos(
+ workingDirs: Seq[(File, Long, Int, StorageInfo.Type)],
+ conf: CelebornConf): (util.Map[String, DeviceInfo], util.Map[String, DiskInfo]) = {
+ val deviceNameToDeviceInfo = new util.HashMap[String, DeviceInfo]()
+ val mountPointToDeviceInfo = new util.HashMap[String, DeviceInfo]()
+
+ val dfResult = runCommand("df -ah").trim
+ logger.info(s"df result\n$dfResult")
+ // (/dev/vdb, /mnt/disk1)
+ val fsMounts = dfResult
+ .split("[\n\r]")
+ .tail
+ .map(line => {
+ val tokens = line.trim.split("[ \t]+")
+ (tokens.head, tokens.last)
+ })
+
+ // (vda, vdb)
+ val lsBlockResult = runCommand("ls /sys/block/").trim
+ logger.info(s"ls block\n$lsBlockResult")
+ val blocks = lsBlockResult.split("[ \n\r\t]+")
+
+ fsMounts.foreach { case (fileSystem, mountPoint) =>
+ val deviceName = fileSystem.substring(fileSystem.lastIndexOf('/') + 1)
+ var index = -1
+ var maxLength = -1
+ blocks.zipWithIndex.foreach(block => {
+ if (deviceName.startsWith(block._1) && block._1.length > maxLength) {
+ index = block._2
+ maxLength = block._1.length
+ }
+ })
+
+ val newDeviceInfoFunc =
+ new util.function.Function[String, DeviceInfo]() {
+ override def apply(s: String): DeviceInfo = {
+ val deviceInfo = new DeviceInfo(s)
+ if (index < 0) {
+ // device not found in /sys/block/
+ deviceInfo.deviceStatAvailable = false
+ }
+ deviceInfo
+ }
+ }
+
+ val deviceInfo =
+ if (index >= 0) {
+ deviceNameToDeviceInfo.computeIfAbsent(blocks(index), newDeviceInfoFunc)
+ } else {
+ deviceNameToDeviceInfo.computeIfAbsent(deviceName, newDeviceInfoFunc)
+ }
+ mountPointToDeviceInfo.putIfAbsent(mountPoint, deviceInfo)
+ }
+
+ val retDeviceInfos = JavaUtils.newConcurrentHashMap[String, DeviceInfo]()
+ val retDiskInfos = JavaUtils.newConcurrentHashMap[String, DiskInfo]()
+
+ workingDirs.groupBy { case (dir, _, _, _) =>
+ getMountPoint(dir.getCanonicalPath, mountPointToDeviceInfo.keySet())
+ }.foreach {
+ case (mountPoint, dirs) =>
+ if (mountPoint.nonEmpty) {
+ val deviceInfo = mountPointToDeviceInfo.get(mountPoint)
+ val diskInfo = new DiskInfo(
+ mountPoint,
+ dirs.map(_._1).toList,
+ deviceInfo,
+ conf)
+ val (dir, maxUsableSpace, threadCount, storageType) = dirs(0)
+ diskInfo.configuredUsableSpace = maxUsableSpace
+ diskInfo.threadCount = threadCount
+ diskInfo.storageType = storageType
+ if (readWriteError(conf, dir)) {
+ diskInfo.status = DiskStatus.READ_OR_WRITE_FAILURE
+ }
Review Comment:
just move the code from DeviceInfo.scala to DeviceMonitor.scala and check the readWriteError here.
--
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-1230] Check working directory read and write error without init delay [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1945325721
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2233?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`c183753`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/c1837536a159454287e16664bc6be9b90a09a023?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 48.63% compared to head [(`675d4e5`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2233?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 48.71%.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2233 +/- ##
==========================================
+ Coverage 48.63% 48.71% +0.08%
==========================================
Files 208 208
Lines 12832 12832
Branches 1104 1104
==========================================
+ Hits 6240 6250 +10
+ Misses 6191 6179 -12
- Partials 401 403 +2
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2233?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-1230] Check local disk and ignore bad disk when initializing worker storage manager [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1902466185
> > ... if only one disk is used and the disk is bad, the worker still serves traffic, but all the requests will fail.
>
> I suppose the design of Celeborn can handle bad / run-out-of-space issues at runtime dynamically. Permanently excluding a bad disk at startup is too rough.
Currently Celeborn will not retry creating base working directories if it fails to create when startup. It only manages inner directories inside the base working directories. So I think this PR is necessary. Maybe we can add check-and-retry for base working dirs in future.
--
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-1230] Check local disk and ignore bad disk when initializing worker storage manager [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1902467994
Another alternative is to extract `LocalDeviceMonitor#startCheck#run` into a method, and explicitly call the method after base working directories are created, so it will be marked as `READ_OR_WRITE_FAILURE`, and no slots will be allocated on it.
--
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-1230] Check working directory read and write error without init delay [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2233:
URL: https://github.com/apache/incubator-celeborn/pull/2233#issuecomment-1945254053
> LocalDeviceMonitor
This is a good idea.
--
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-1230] Check working directory read and write error without init delay [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2233: [CELEBORN-1230] Check working directory read and write error without init delay
URL: https://github.com/apache/incubator-celeborn/pull/2233
--
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