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

[OPS-8056] xz sure why not #292

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

cafuego
Copy link
Contributor

@cafuego cafuego commented Jan 28, 2022

No description provided.

@cafuego cafuego requested a review from orakili January 28, 2022 03:13
orakili
orakili previously approved these changes Jan 28, 2022
Copy link
Collaborator

@orakili orakili left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@orakili orakili self-requested a review January 28, 2022 05:18
@orakili orakili dismissed their stale review January 28, 2022 05:19

The content of the archive doesn't seem to contain what is needed (ex: '/init').

Copy link
Collaborator

@orakili orakili left a comment

Choose a reason for hiding this comment

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

Looks like they separated the content into at least 2 mandatory packages: https://github.com/just-containers/s6-overlay#quickstart:

  • s6-overlay-noarch-$VERSION-tar.xz
  • s6-overlay-$ARCH-$VERSION-tar.xz

alpine-base-s6/Dockerfile.tmpl Outdated Show resolved Hide resolved
@cafuego
Copy link
Contributor Author

cafuego commented Jan 28, 2022

Welp, poopie. Thanks for the fix :-)

orakili
orakili previously approved these changes Jan 28, 2022
Copy link
Collaborator

@orakili orakili left a comment

Choose a reason for hiding this comment

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

That seems ok now but it would be good to have someone else test as well to confirm.

@orakili orakili dismissed their stale review January 28, 2022 08:17

Issues with nginx (ex: php-k8s images) and s6.

@orakili
Copy link
Collaborator

orakili commented Jan 28, 2022

Sorry, dismissing my review again. While the images based on this alpine-base-s6 now start with the added noarch, images starting nginx like the php-k8s images seem to not work. For example netstat -tulpn inside a container running such image doesn't show nginx listening on port 80 for example and curl for example fails with a Connection refused.

That may not be an issue withe s6-overlay but I need to investigate more...

Advice: read the https://github.com/just-containers/s6-overlay README.md because they changed a lot of things in 3.0.0.0 and it looks like all the run_xxx scripts that we used to put in /etc/services.d need to be modified: https://github.com/just-containers/s6-overlay#writing-a-service-script

@teodorescuserban
Copy link
Contributor

Sorry, dismissing my review again. While the images based on this alpine-base-s6 now start with the added noarch, images starting nginx like the php-k8s images seem to not work. For example netstat -tulpn inside a container running such image doesn't show nginx listening on port 80 for example and curl for example fails with a Connection refused.

That may not be an issue withe s6-overlay but I need to investigate more...

Advice: read the https://github.com/just-containers/s6-overlay README.md because they changed a lot of things in 3.0.0.0 and it looks like all the run_xxx scripts that we used to put in /etc/services.d need to be modified: https://github.com/just-containers/s6-overlay#writing-a-service-script

It does say "We encourage you to switch to the new format, but if you don't need its benefits, you can stick with regular service directories in /etc/services.d, it will work just as well." so nothing breaking there, right?

@orakili
Copy link
Collaborator

orakili commented Jan 28, 2022

That's what they say but in practice, even with the "compatibility" assets mentioned in the installation section: https://github.com/just-containers/s6-overlay#installation, I could not get any images with nginx to listen the ports.

But maybe I broke something else.

@cafuego
Copy link
Contributor Author

cafuego commented Jan 30, 2022

I did a build and I think I see the problem, the run scripts aren't executable. Fixing and re-testing.

@cafuego
Copy link
Contributor Author

cafuego commented Jan 30, 2022

And all the helper scripts are now in /package/admin/s6/* and symlinked from /command. So the run scripts will indeed all need updating. Sigh.

@cafuego
Copy link
Contributor Author

cafuego commented Jan 31, 2022

And annoyingly v3 overwites any PATH value to try to pass via an env var. That breaks all our stacks (they need /srv/www/vendor/bin) and I don't want to dump that in the new global config file either, so I've included a hacked up /init that doesn't reset PATH and ignore what we want. Also added a whiny comment to the s6-overlay repo and hopefully my fix will get in to the next version.

In the mean time, the current HEAD of this branch should be able to build you a working Raspberry Pi image.

@teodorescuserban
Copy link
Contributor

For NodeJS, I just grabbed the v2.2.0.3 os s6-overlay and thats that. I don't think it's worth the time to rewrite all the stratup scripts and locations just for the sake of running s6-overlay v3.x.

@teodorescuserban
Copy link
Contributor

I did a build and I think I see the problem, the run scripts aren't executable. Fixing and re-testing.

That's going to create issues as well if the scripts are mapped from the docker compose stack. Probably a better approach to add a pre-init script to make those run scripts executable.

@orakili
Copy link
Collaborator

orakili commented Feb 1, 2022

Note, they are probably going to change the URLs again and remove the version from the filenames: just-containers/s6-overlay#386 (comment).

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