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

Add dumb-init to rootless docker #21775

Merged
merged 2 commits into from
Dec 4, 2022

Conversation

viceice
Copy link
Contributor

@viceice viceice commented Nov 11, 2022

Add dumb-init as process reaper to the rootless image.
I've very often defunc hung git processes, which are not cleaned by gitea.

@viceice
Copy link
Contributor Author

viceice commented Nov 11, 2022

I'm using it here1 and it's working fine. Docuum is running docker cli which could also hung some times.

Footnotes

  1. https://github.com/visualon/docker-docuum/blob/fe8284b66b187dc3d53157886e0581b8e0738420/linux/Dockerfile#L45

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 11, 2022
@ghost ghost added pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! theme/docker labels Nov 12, 2022
@silverwind
Copy link
Member

silverwind commented Nov 12, 2022

I've very often defunc hung git processes, which are not cleaned by gitea

I think we better fix the root issue so that deployments in docker-less environments can benefit too. Or at least, do the reaping in-process.

@viceice
Copy link
Contributor Author

viceice commented Nov 12, 2022

I've very often defunc hung git processes, which are not cleaned by gitea

I think we better fix the root issue so that deployments in docker-less environments can benefit too. Or at least, do the reaping in-process.

on normal systems the init process does the process reaping, so no need to do anything there.

the reaping should do the process with id zero.

https://iximiuz.com/en/posts/dealing-with-processes-termination-in-Linux/

@silverwind
Copy link
Member

The rootful images uses s6 which seems to include a reaper. I'm not sure the reasoning why the rootless images does not have one, but I don't think we should have two different ones.

@viceice
Copy link
Contributor Author

viceice commented Nov 13, 2022

so should I refactor to use s6 instead of dumb-init?

dumb-init was a much simpler solution 😅

@silverwind
Copy link
Member

I'm not sure what the state-of-the-art reaper is but your dumb-init seems generally more popular than s6. Maybe we should get rid of s6, but please wait on more opinions, I'm no expert on this and we might overlook functionality provided by s6 that dumb-init does not have.

https://github.com/Yelp/dumb-init
https://github.com/skarnet/s6

@viceice
Copy link
Contributor Author

viceice commented Nov 13, 2022

I'm not sure what the state-of-the-art reaper is but your dumb-init seems generally more popular than s6. Maybe we should get rid of s6, but please wait on more opinions, I'm no expert on this and we might overlook functionality provided by s6 that dumb-init does not have.

https://github.com/Yelp/dumb-init
https://github.com/skarnet/s6

ok, s6 is more like systemd, it can watch and restart multiple processes. dumb-init just starts one child process and exits when child dies.

@techknowlogick
Copy link
Member

it can watch and restart multiple processes

Yup, that's why it is used for the rootful one as we use opensshd in it as well. I think it's reasonable to use dumb-init for rootless.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 4, 2022
@zeripath
Copy link
Contributor

zeripath commented Dec 4, 2022

I'd also be happy if we could just do the reaping from within Gitea itself but I'm not against dumb-init being used.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 4, 2022
@zeripath zeripath merged commit 84d2a82 into go-gitea:main Dec 4, 2022
@zeripath
Copy link
Contributor

zeripath commented Dec 4, 2022

Please send backport to 1.18

@zeripath zeripath added the outdated/backport/v1.18 This PR should be backported to Gitea 1.18 label Dec 4, 2022
@zeripath
Copy link
Contributor

zeripath commented Dec 4, 2022

@xin-u how is this breaking?

@viceice
Copy link
Contributor Author

viceice commented Dec 4, 2022

Please send backport to 1.18

will do tomorrow

@ghost
Copy link

ghost commented Dec 4, 2022

@xin-u how is this breaking?

ENTRYPOINT was changed.

@lafriks
Copy link
Member

lafriks commented Dec 4, 2022

@xin-u how is this breaking?

ENTRYPOINT was changed.

But arguments will still be passed to entrypoint.sh and and if you have provided custom entrypoint it would still work so I don't see use case that would break 🤔

@ghost ghost removed the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Dec 4, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 5, 2022
* giteaofficial/main:
  Ensure that Chinese punctuation is not ambiguous when locale is Chinese (go-gitea#22019)
  Use GhostUser if needed for TrackedTimes (go-gitea#22021)
  Add dumb-init to rootless docker (go-gitea#21775)
  On tag/branch-exist check, dont panic if repo is nil (go-gitea#21787)
  Fix ListBranches to handle empty case (go-gitea#21921)
  fix(web): reduce page jitter on browsers that support overlay scrollbar (go-gitea#21850)
  [skip ci] Updated licenses and gitignores
  Do not emit ambiguous character warning on rendered pages (go-gitea#22016)
@silverwind
Copy link
Member

silverwind commented Dec 6, 2022

If user would pass --entrypoint to docker, they would overwrite the whole entrypoint, but I guess all bets are off if that is done. Arguments should still pass through without breakage.

@viceice
Copy link
Contributor Author

viceice commented Dec 6, 2022

If user would pass --entrypoint to docker, they would overwrite the whole entrypoint, but I guess all bets are off if that is done. Arguments should still pass through without breakage.

yes, but it's not breaking. it overrided before the same.

@viceice
Copy link
Contributor Author

viceice commented Dec 6, 2022

Please send backport to 1.18

@viceice viceice deleted the fix/rootless-process-reaper branch December 6, 2022 09:22
lunny pushed a commit that referenced this pull request Dec 6, 2022
@lafriks
Copy link
Member

lafriks commented Dec 6, 2022

If user would pass --entrypoint to docker, they would overwrite the whole entrypoint, but I guess all bets are off if that is done. Arguments should still pass through without breakage.

But it would still work for user with his custom entrypoint as no paths or anything else was changed, it would still work for him as it was before just dumb init would not be used

@zeripath zeripath added the backport/done All backports for this PR have been created label Jan 13, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
@delvh delvh added topic/distribution This PR changes something about the packaging of Gitea and removed theme/docker labels Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. outdated/backport/v1.18 This PR should be backported to Gitea 1.18 topic/distribution This PR changes something about the packaging of Gitea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants