Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add domain socket max path length check #18015

Open
wants to merge 1 commit into
base: master-2.x
Choose a base branch
from
Open

Add domain socket max path length check #18015

wants to merge 1 commit into from

Conversation

gp1314
Copy link
Contributor

@gp1314 gp1314 commented Aug 17, 2023

What changes are proposed in this pull request?

  • Short circuit read the alluxio.worker.data.server.domain.socket.address more than 108 throw NoSuchFileException address configuration if the path，especially alluxio.worker.data.server.domain.socket.as.uuid set true
  • On unix systems, the length of a domain socket path is limited to 108 ，however, this limitation is not checked in alluxio, resulting in only half of the uuid being created，such as

domainSocketPath is /xxx/xxx/xxx/xxx/alluxio_socket/65f21a84-a4b5-408e-8066-8d8c4c171cd5

What's actually generated is /xxx/xxx/xxx/xxx/alluxio_socket/65f21a84-a4b5-408e-8066-8d8c4c

Why are the changes needed?

  1. Add domainSocketPath length checks instead of throwing NoSuchFileException

@LuQQiu
Copy link
Contributor

LuQQiu commented Aug 21, 2023

@gp1314 is it more for log readability?

@gp1314
Copy link
Contributor Author

gp1314 commented Aug 22, 2023

Yes, it seems that java15 and below can only get the domain socket max path length of the environment through the JNI method

@apc999 apc999 force-pushed the main branch 2 times, most recently from 2fec0ec to b597c61 Compare October 17, 2023 19:11
acelyc111
acelyc111 reviewed Oct 18, 2023
View reviewed changes
dora/core/common/src/main/java/alluxio/util/OSUtils.java Outdated
@@ -28,6 +28,9 @@ public final class OSUtils {
public static final String JAVA_VENDOR_NAME = System.getProperty("java.vendor");
/** Indicates the current java vendor is IBM java or not. */
public static final boolean IBM_JAVA = JAVA_VENDOR_NAME.contains("IBM");
/** The maximum path length supported by a domain socket on unix varies depending on the
* operating system. The conservative limit is returned here */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* operating system. The conservative limit is returned here */
* operating system. The conservative limit is returned here. */

dora/core/common/src/main/java/alluxio/util/OSUtils.java Outdated
@@ -28,6 +28,9 @@ public final class OSUtils {
public static final String JAVA_VENDOR_NAME = System.getProperty("java.vendor");
/** Indicates the current java vendor is IBM java or not. */
public static final boolean IBM_JAVA = JAVA_VENDOR_NAME.contains("IBM");
/** The maximum path length supported by a domain socket on unix varies depending on the
* operating system. The conservative limit is returned here */
public static final int UNIX_SOCKET_MAX_PATH_LENGTH = 100;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it 108, why set it to 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are differences in the maximum length of domain socket paths among different operating systems. Therefore, it is recommended to limit the length of the socket path to the smallest common length range, such as around 100 characters.

@jiacheliu3
Copy link
Contributor

should this be pushing to master-2.x? Do we still keep domain socket in 3.x? @LuQQiu

@gp1314 gp1314 changed the base branch from main to master-2.x September 11, 2024 02:33
@alluxio-bot
Copy link
Contributor

Automated checks report:

  • PR title follows the conventions: PASS
  • Commits associated with Github account: FAIL
    • It looks like your commits can't be linked to a valid Github account.
      Your commits are made with the email zihao.zhao@alluxio.com, which does not allow your contribution to be tracked by Github.
      See this link for possible reasons this might be happening.
      To change the author email address that your most recent commit was made under, you can run: 
      git -c user.name="Name" -c user.email="Email" commit --amend --reset-author
      See this answer for more details about how to change commit email addresses.
      Once the author email address has been updated, update the pull request by running: 
      git push --force https://github.com/gp1314/alluxio.git dev/gup/domain_socket_max_path_length_check

Some checks failed. Please fix the reported issues and reply
alluxio-bot, check this please
to re-run checks.

@gp1314 gp1314 force-pushed the dev/gup/domain_socket_max_path_length_check branch from a007052 to b916f87 Compare September 11, 2024 02:55
@alluxio-bot
Copy link
Contributor

Automated checks report:

  • PR title follows the conventions: PASS
  • Commits associated with Github account: PASS

All checks passed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acelyc111 acelyc111 acelyc111 left review comments

@LuQQiu LuQQiu Awaiting requested review from LuQQiu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gp1314 @LuQQiu @jiacheliu3 @alluxio-bot @acelyc111