fix apply chown permissions in parallel for large workspace #73
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for opening this pull request! Be sure to follow the pull request template!
|
I am a bot, here are the test results for this PR: |
|
[EDIT] Updated PR to include automatic process allocation based upon available number of cores in container |
|
log...
setting permissions::workspace
chown: missing operand after ‘abc:abc’
Try 'chown --help' for more information.
...Fixed in |
|
@aptalca Ready for evaluation |
|
I am a bot, here are the test results for this PR: |
2 similar comments
|
I am a bot, here are the test results for this PR: |
|
I am a bot, here are the test results for this PR: |
|
@aptalca Any update ? (Rebased / Synced due to bot update) |
|
I am a bot, here are the test results for this PR: |
|
@gjrtimmer thanks for the PR. I've been quite busy with real life and other projects. To be honest, when I first put in the chown in there, I didn't consider that folks may have many gigs of data in the config folder. In such cases (like plex or emby where they have hundreds of gigs), we usually check the top folder and if the perms there are good, we skip the chown. I'm thinking we can do the same here for the workspace folder, where we check It also got me thinking, since we provide sudo/root access in this container, the user may have files in workspace with different perms or owners intentionally. We certainly don't want to mess with them. Let me know if you see any issues with that plan. If not, please modify it accordingly (only chown Thanks |
|
@aptalca I totally understand, I work myself in medical IT, so I know how taxing real life can be. Your suggestions got me also thinking, what if we make a few changes and introduce some variables, this way we can do even both, have the user decide and provide some flexibility which even can be ported to other containers. I will change the PR according to your suggestions, and keep your initial suggestion as the default action. I would still like to preserve some of this improvement because as you said, we can recursively chown the rest of I've included a recent overview of my current running ❯ du -sh *
24K crontabs
0 custom-cont-init.d
0 custom-services.d
279M data
435M extensions
0 npm-global
11G workspaceAs you can see, I this current container there is not much npm data, but as you can see, the
|
No this is intended, to block compromised apps to be able to write to files that are executed by root. I can also mention that I mounted my projects to another folder, mainly because my /config is backed up. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@Roxedus Thank you. |
gjrtimmer
force-pushed
the
fix/chown
branch
from
4c1c208 to
992e44f
Compare
|
I am a bot, here are the test results for this PR: |
|
I'm not in favor of adding an env var (let alone 2) for permissions. I think we should simply chown everything under config recursively except for workspace, which we'll chown non-recursive (so code-server can access it on start). Anything inside workspace will be the user's own responsibility. |
|
I am a bot, here are the test results for this PR: |
|
@aptalca | I understand, will provide fix within 5m |
|
Thanks so much. While you're at it, can you also update the changelog entry to today's date? |
gjrtimmer
force-pushed
the
fix/chown
branch
from
992e44f to
98c517e
Compare
|
@aptalca | Fix provided, corrections made upon comments, please review |
|
I am a bot, here are the test results for this PR: |
|
From the container startup log: |
gjrtimmer
force-pushed
the
fix/chown
branch
from
98c517e to
2ef2329
Compare
|
@aptalca during the rewrite, I forget to put the |
|
I am a bot, here are the test results for this PR: |
|
@aptalca DONE |
|
I am a bot, here are the test results for this PR: |
chownis always bound to a single core. See man pagechown. Applying the permissions through a single process on a large workspace causes the container boot process to be slow. In order to mitigate slow container start times for large work spaceschowncan be run in parallel. This PR provides that functionality.Gains
chownprocess which for a large config directory and/or workspace can easily increase to over 1.2 GB of additional memory.Description:
Apply permissions through
chownin parallel.Benefits of this PR and context:
Benefits are pure speed when it comes to starting the container.
The following test results are from a workspace with the following details.
Results before PR
Results after PR
How Has This Been Tested?
timecommand, recorded differences and attached them in this PR.