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

Switch from supervisord to s6-overlay #293

Merged
merged 17 commits into from
May 9, 2020

Conversation

alexwh
Copy link
Contributor

@alexwh alexwh commented May 7, 2020

Pulling in all of Python just for supervisord is pretty heavy, so we're using s6-overlay to manage running lrr and redis instead.

fix-attrs.d isn't used right now since it doesn't have the exact granular control that the previous entrypoint.sh script did (chowning some stuff's user recursively, but not the mode). The file is there for now, but not copied into the docker image.

Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a bunch of boomer-tier remarks, but overall this seems nice, how much space do you gain by removing python and supervisord?

You'll also need to rebase this to match the Dockerfile changes I made earlier.

tools/build/docker/s6/cont-init.d/01-lrr-setup Outdated Show resolved Hide resolved
tools/build/docker/Dockerfile Show resolved Hide resolved
tools/build/docker/Dockerfile Outdated Show resolved Hide resolved
tools/build/docker/Dockerfile Outdated Show resolved Hide resolved
.dockerignore Show resolved Hide resolved
tools/build/docker/s6/fix-attrs.d/01-lrr-dirs Show resolved Hide resolved
tools/build/docker/Dockerfile Show resolved Hide resolved
koma-cute added a commit to koma-cute/LANraragi that referenced this pull request May 7, 2020
@koma-cute
Copy link
Contributor

lrrtest                   latest              538fcbc17de3        165MB
difegue/lanraragi         nightly             ba07488a7cec        215MB

Should be inline with dev now, I think I solved the conflicts correctly.

koma-cute added a commit to koma-cute/LANraragi that referenced this pull request May 7, 2020
@koma-cute
Copy link
Contributor

merge commits are fucked, I rebased like I should have to begin with

@Difegue
Copy link
Owner

Difegue commented May 7, 2020

The tests are failing rn due to the nightly build being super fucked, things should go back to normal whenever actions are done rebuilding a full nightly image.
I'll re-review this afterwards.

Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments.
I pushed a quick-ass fix to the old supervisord entrypoint yesterday so you should probably do another rebase. 🙇‍♂️ Sorry about that.

tools/build/docker/Dockerfile Outdated Show resolved Hide resolved
changing anything in tools folder causes whole build to retrigger, avoid
that
previous removed in install-everything.sh
it gets excluded by the dockerignore file otherwise, and the copy
technically happens in the docker layer, so
redis tells us this on startup itself
we make extensive use of envvars in lrr and the setup scripts
there shouldn't be anything sensitive, so pass them to all processes
with this, rather than restrictively using `with-contenv`
@koma-cute
Copy link
Contributor

Rebased, should be inline.

@Difegue
Copy link
Owner

Difegue commented May 8, 2020

You forgot to re-add the install script to the COPY line, so it breaks the build.

@koma-cute
Copy link
Contributor

So I did, duh. Fixed now.

Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran some internal testing and found a few remaining issues.
Almost there, though! 🐳

tools/build/docker/s6/services.d/redis/run Outdated Show resolved Hide resolved
tools/build/docker/s6/cont-init.d/01-lrr-setup Outdated Show resolved Hide resolved
tools/build/docker/Dockerfile Outdated Show resolved Hide resolved
Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lookin' good!

@Difegue Difegue merged commit e7e4f66 into Difegue:dev May 9, 2020
@Difegue
Copy link
Owner

Difegue commented May 9, 2020

image
Windows Installers now clock in at 50MBs, so I'm pretty sure the compressed Docker images will be around that size too. Thanks a lot!

@koma-cute
Copy link
Contributor

Huh, interesting. My docker images were still around 150MB. Were there other optimizations to reduce it that much more? No problem, glad to help!

@Difegue
Copy link
Owner

Difegue commented May 9, 2020

This is the compressed size, so unpacked it'll still be around 150, same as the Docker images.

@koma-cute
Copy link
Contributor

Ah, gotcha. Thought that's what the docker images command displayed.

Difegue added a commit that referenced this pull request May 20, 2020
* initial s6 transition

* move install-everything.sh to Dockerfile

changing anything in tools folder causes whole build to retrigger, avoid
that

* add .dockerignore excluding documentation

previous removed in install-everything.sh

* move wsl.conf to docker folder

it gets excluded by the dockerignore file otherwise, and the copy
technically happens in the docker layer, so

* remove redis overcommit_memory messages

redis tells us this on startup itself

* set S6_KEEP_ENV

we make extensive use of envvars in lrr and the setup scripts
there shouldn't be anything sensitive, so pass them to all processes
with this, rather than restrictively using `with-contenv`

* set uid and gid for services

* terminate lrr also if the service fails

* set s6 stage 2 fail early and kill time opts

* remove manual inotify2 call

#293 (comment)

* revert install-everything.sh consolidation

* remove chmod +x and set exec directly on file

* fix old comment

* add install-everything.sh to tools

* Update tools/build/docker/s6/services.d/redis/run

Co-authored-by: Difegue <[email protected]>

* readd redis OVERCOMMIT warning

* move redis.conf back to tools/build/docker

Co-authored-by: koma-cute <[email protected]>
Co-authored-by: koma-cute <[email protected]>
Co-authored-by: Difegue <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants