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

fix(devcontainer): chown regression for make codegen #13375

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented Jul 22, 2024

#13348 removed a recursive flag from chown. I believe was done because this is slow on macintosh filesystems.

This breaks codegen with

fatal: could not create leading directories of '/home/vscode/go/src/github.com/gogo/protobuf': Permission denied

This is a smaller recursive chown which fixes the issue for me.

argoproj#13348 removed a
recursive flag from chown. I believe this is because this is slow on
poorly performing filesystems.

This breaks codegen with
```
fatal: could not create leading directories of
'/home/vscode/go/src/github.com/gogo/protobuf': Permission denied
```

This is a smarter recursive chown which fixes the issue for me.

Signed-off-by: Alan Clucas <[email protected]>
@Joibel Joibel requested a review from agilgur5 July 22, 2024 09:09
@Joibel Joibel added type/regression Regression from previous behavior (a specific type of bug) area/contributing Contributing docs, ownership, etc. Also devtools like devcontainer and Nix labels Jul 22, 2024
@Joibel Joibel marked this pull request as ready for review July 22, 2024 09:10
@qingfeng777
Copy link
Contributor

It worked in my development environment.

@agilgur5
Copy link
Member

It worked in my development environment.

The fix works, to clarify

@agilgur5 agilgur5 changed the title fix(devcontainer): regression in make codegen fix(devcontainer): chown regression in make codegen Jul 22, 2024
@agilgur5 agilgur5 changed the title fix(devcontainer): chown regression in make codegen fix(devcontainer): chown regression for make codegen Jul 22, 2024
@agilgur5
Copy link
Member

This breaks codegen with

Dang, can confirm it does this in my devcontainer too. This might be due to similar reasons as #13145 that codegen didn't run.

Thank you both for catching this!

I believe was done because this is slow on macintosh filesystems.

Atrociously slow -- many minutes, I don't think I've ever once waited till the end, I have no idea how long it could take total. Since it worked fine even with me cancelling part-way, something about it isn't entirely necessary.
Although I'm not sure it's Mac FS specific per se, since the command is running inside the container on files that only exist in the container, and has no affect on the host FS. But it clearly doesn't happen everywhere, so idk what the root cause is.

From a quick trace, it does seem like a lot of time is spent on the .git directory; even if the error is silenced from #13111, it still recurses through it. I don't think we need to recurse through, especially not the entire source dir, we just need to figure out which directories need to have chown applied

@agilgur5 agilgur5 added area/build Build or GithubAction/CI issues and removed type/regression Regression from previous behavior (a specific type of bug) labels Jul 22, 2024
@agilgur5 agilgur5 self-assigned this Jul 22, 2024
these 3 seem to be the necessary ones

Signed-off-by: Anton Gilgur <[email protected]>
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 22, 2024
@agilgur5 agilgur5 enabled auto-merge (squash) July 22, 2024 23:55
@agilgur5 agilgur5 merged commit 7357a1b into argoproj:main Jul 22, 2024
16 checks passed
@Joibel
Copy link
Member Author

Joibel commented Jul 23, 2024

I believe was done because this is slow on macintosh filesystems.

Atrociously slow -- many minutes, I don't think I've ever once waited till the end, I have no idea how long it could take total. Since it worked fine even with me cancelling part-way, something about it isn't entirely necessary. Although I'm not sure it's Mac FS specific per se, since the command is running inside the container on files that only exist in the container, and has no affect on the host FS. But it clearly doesn't happen everywhere, so idk what the root cause is.

I'm not saying it's mac specific, but either APFS or the docker desktop virtualisation through to APFS is causing slowdown. The writes are happening in the docker overlay, but they presumably have to thunk through to understand the underlying files.

On Linux with the source files on ext4 and using btrfs as the docker storage driver this takes less than 2 seconds. When I wrote this chown originally I wasn't using btrfs, just overlay2, and it was a much slower computer, and it was still unnoticable as a part of startup.

@agilgur5
Copy link
Member

agilgur5 commented Jul 23, 2024

The writes are happening in the docker overlay, but they presumably have to thunk through to understand the underlying files.

Presumably, although something feels off that other FS commands aren't similarly slow

I'm curious if someone else has the same issue on Mac? Did Adrien have it too?

Although if these 3 directories are enough, then we won't need to worry about it either way (although it is possible another FS-type issue could crop up again with the devcontainer in the future)

From a quick trace, it does seem like a lot of time is spent on the .git directory

The other outlier is that my .git directory is probably larger than most, since I have lots of branches, stashes, a big reflog, a few remotes to other contributors etc.
Potentially an FS inefficiency combined with that causes the slowdown

@Joibel Joibel deleted the fix-codegen branch July 25, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues area/contributing Contributing docs, ownership, etc. Also devtools like devcontainer and Nix lgtm This PR has been approved by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants