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.

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

create unique temporary directories to prevent permission issues #7173

Merged
merged 2 commits into from
Jul 27, 2024

Conversation

ErikApption
Copy link
Contributor

@ErikApption ErikApption commented Jun 9, 2024

Fixes #7172

This is a tentative fix for #7172 where ml.net fails when multiple users are using same directory. This fix checks if there is already a ml_dotnet if so, it will increase the number until the path is available.

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

@michaelgsharp
Copy link
Member

so Path.GetRandomFileName() should make sure we are getting unique temp directories. What problems are you seeing in regards to this?

@ErikApption
Copy link
Contributor Author

ErikApption commented Jun 26, 2024

so Path.GetRandomFileName() should make sure we are getting unique temp directories. What problems are you seeing in regards to this?

Current code has this

string path = Path.Combine(Path.GetFullPath(tempPath), "ml_dotnet", Path.GetRandomFileName());

Issue is the directory name ml_dotnet, not the temp file name. The Path.Combine will create a subdirectory and a random file name inside that directory. The directory name is not unique. When one user processes creates /tmp/ml_dotnet/ for ML.net and a second user runs the same ML.net application at the same time, the second user cannot create /tmp/ml_dotnet/<temp file name> because permissions on /tmp/ml_dotnet/ are restricted to user 1. User 2 doesn't have write permissions (at least on default ubuntu but would be surprised this is different on other distros).

@michaelgsharp
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

codecov bot commented Jul 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.83%. Comparing base (4bc753a) to head (eb36173).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7173      +/-   ##
==========================================
+ Coverage   68.66%   68.83%   +0.16%     
==========================================
  Files        1262     1267       +5     
  Lines      257774   259769    +1995     
  Branches    26660    26946     +286     
==========================================
+ Hits       176991   178800    +1809     
- Misses      73971    74090     +119     
- Partials     6812     6879      +67     
Flag Coverage Δ
Debug 68.83% <100.00%> (+0.16%) ⬆️
production 63.08% <100.00%> (+0.13%) ⬆️
test 88.99% <ø> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/Microsoft.ML.Core/Data/Repository.cs 79.93% <100.00%> (+0.21%) ⬆️

... and 56 files with indirect coverage changes

@michaelgsharp michaelgsharp merged commit b4a4b10 into dotnet:main Jul 27, 2024
25 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Directory permission exception with multiple concurrent users on linux
2 participants