alluxio icon indicating copy to clipboard operation
alluxio copied to clipboard

Fix missing mount points

Open suninsky opened this issue 2 years ago • 6 comments

What changes are proposed in this pull request?

Improves security by preventing mount point loss.

Why are the changes needed?

Since MountId is a random number, it may already be a key in mMountIdToUfsInfoMap. If you execute mMountIdToUfsInfoMap.put(mountId, new UfsClient(() -> getOrAdd(ufsUri, ufsConf), ufsUri)); Cause the original UfsClient to be lost. To avoid this, add a parameter check. The probability of this happening is very small, but it is very dangerous.

Does this PR introduce any user facing changes?

No

suninsky avatar Aug 04 '22 14:08 suninsky

Thank you for your pull request. In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement (CLA). It's all electronic and will take just a few minutes. Please download CLA form here, sign, and e-mail back to [email protected]

alluxio-bot avatar Aug 04 '22 14:08 alluxio-bot

Automated checks report:

  • 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 [email protected], 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/suninsky/alluxio.git fix_missing_mount_points
      
  • PR title follows the conventions: FAIL
    • The title of the PR does not pass all the checks. Please fix the following issues:
      • First word of title ("Fixed") is not an imperative verb. Please use one of the valid words

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

alluxio-bot avatar Aug 04 '22 14:08 alluxio-bot

Automated checks report:

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

All checks passed!

alluxio-bot avatar Aug 04 '22 15:08 alluxio-bot

This is good, but I think some places use a fixed mount id, and overwrite it, that may be where some of the test failures are coming from. So maybe this check should happen in DefaultFileSystemMaster instead? Or should take a parameter that should say if it is valid to overwrite the id or not?

I guess also generating the random mount ID should also be sure not to use one of the predefined fixed ones.

Also it would be helpful if the exception message mentioned that it was a transient error, and the client should retry the mount.

tcrain avatar Aug 04 '22 18:08 tcrain

I added a check where the random mount ID was generated and retried if failed.

suninsky avatar Aug 05 '22 02:08 suninsky

Thanks, I think the only other worry could be two concurrent mount operations creating the same ID, but I think mount concurrency is being addressed here https://github.com/Alluxio/alluxio/pull/15928, so it should be fine.

tcrain avatar Aug 08 '22 22:08 tcrain