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

chore: docker build updates, automate npm publish #1394

Merged
merged 49 commits into from Mar 22, 2022

Conversation

KEGustafsson
Copy link
Contributor

@KEGustafsson KEGustafsson commented Feb 11, 2022

  • base image as ubuntu 20.04
  • needed apt-get packages
  • nodejs script installer (node 16.x)
  • mDNS enabled for node
  • startup.sh script to start mDNS and SK

- base image as ubuntu 20.04
- needed apt-get packages
- nodejs script installer (node 16.x)
- mDNS enabled for node
- startup.sh script to start mDNS and SK
@KEGustafsson
Copy link
Contributor Author

KEGustafsson commented Feb 11, 2022

When SK is restarted e.g. from admin webpage, then also container will stop or restart depending on settings. To keep container running while signalk-server is restarting, I could add PM2 here. This would be a bit more sophisticated method to run nodejs app.

There would 2 modifications needed
RUN npm install pm2 -g after nodejs installation script and
change to startup.sh to use pm2 /usr/bin/pm2-docker 'bin/signalk-server --securityenabled'

With PM2 included images sizes.
linux/amd64 448.77 MB
linux/arm64 438.79 MB

- pm2 (process manager) to run signalk-server
@KEGustafsson
Copy link
Contributor Author

In original Dockerfile, there is COPY empty_file tmp*/qemu-arm-stati[c] /usr/bin/
Is this specific for building docker with used actions?
I have used docker buildx build in action to build docker and above part is not needed there.

@tkurki
Copy link
Member

tkurki commented Feb 11, 2022

The COPY empty file is from cross building arm in Travis. I think that can be removed.

In my own use I have managed restarts outside the container, restarting the whole container if it exits. I am not sure what the best course of action here is. @jncarter123 do you have any advice?

@tkurki
Copy link
Member

tkurki commented Feb 11, 2022

For reference Dockerfile for node:16

@tkurki
Copy link
Member

tkurki commented Feb 11, 2022

One obvious difference is that the official image has yarn that we don't need.

@tkurki
Copy link
Member

tkurki commented Feb 11, 2022

Now that there's more than just a single Dockerfile: should we put everything under a single directory docker for example?

@tkurki
Copy link
Member

tkurki commented Feb 11, 2022

I think we should move forward with this, but I am just saying that the way the server is built here is kinda dumb and results in images that are not exactly 1 to 1 with what is published to npm.

What I mean is that instead of building the server and packages from scratch (and leaving devDependencies in place!) during docker build we should install from npm the ready made packages. But then we should have a github action that installs server's deps from npm (not via npm workspace), builds, tests and publishes to npm first and only then builds the docker image, either from the files built in build step or by installing the server from npm. The latter option is more like what end users do and would also work as a test of "can we install the version we just published to npm".

@KEGustafsson
Copy link
Contributor Author

I like the idea "can we install the version we just published to npm".

This is a bit outside of docker image build, but still very much related to it.

Every bit and pieces need to be ready before docker build is triggered, but that shouldn't be a problem if all necessary packages are available from npmjs or other locations.

For docker, there are still OS related stuff, like base image, needed additional packages, extra settings and startup stuff. Anyway, Signal K server installation could follow the same path as normal user.

Would there be then any need for ENV IS_IN_DOCKER?

@KEGustafsson
Copy link
Contributor Author

KEGustafsson commented Feb 11, 2022

Now that there's more than just a single Dockerfile: should we put everything under a single directory docker for example?

That would be a good idea.
I'll do needed changes for now.

@tkurki
Copy link
Member

tkurki commented Feb 11, 2022

IS_IN_DOCKER disables installing new server versions in the admin ui, so yes we want it.

@KEGustafsson
Copy link
Contributor Author

KEGustafsson commented Feb 12, 2022

What do you think about this approach?
Dockerfile
During docker image build, there would be intermediate steps to build, (test, not included) and publish node packages.
Then build folder will be removed and SK will be installed from just published packages and pushed to docker hub.
All-in-one.

Edit: Only one of the platforms (amd64) would do build, test and publish, not all. That is missing.

@tkurki
Copy link
Member

tkurki commented Feb 12, 2022

In principle yes, but I don't think docker build is the way to do other builds. But Github actions are. So everything but creating the docker image should be happening in github actions workflow: install, compile, test and then publish. Here is an example of the earlier stages, then after publish succeeds it should run a cross arch docker build (like the current workflow), installing from npm what was just released.

A small detail is that each command in a Dockerfile creates a file system layer. Running rm on its own will not decrease the image size, but actually slightly increase it, because it will create a new file system layer. If used rm must be on the same line as the command that creates the stuff that it is removing.

@KEGustafsson
Copy link
Contributor Author

KEGustafsson commented Feb 12, 2022

This was/is a study that I wanted to do to check whether it is possible/feasible. Seems to be too complicated vs github actions...
Also resulted bigger size images as you said, actually surprisingly big size, more than I expected.

How to proceed forward? Should Dockerfile be simplified to build image from npmjs package (signalk-server) or continue current path (install, build, having src available)?

I think simplified version could be more suitable and usable.
Dockerfile

Multiplatform docker image build time: 19min
linux/amd64, 306.85 MB
linux/arm/v7, 270.69 MB
linux/arm64, 294.39 MB

Amd64 and arm64 version working fine.

@tkurki
Copy link
Member

tkurki commented Feb 13, 2022

So with this Dockerfile the essential changes would be a smaller image and working mdns? Mdns requires host network mode, right?

Now the docker build in Github is triggered on pushing the tag for a release. Now this will not work, because in my release workflow I

  • tag locally the release with npm version
  • run npm run release that pushes changes, including the release tag, to Github and creates the Github Release
  • run npm publish

So the new release may or may not be published in npm and just running npm i will install essentially a random "whatever is latest" version in npm, BUT creating a version tagged image.

Also this approach won't be able to build docker images off master.

So there are uses to building off the local files.

So what if we (you...) back up a little: install as previously, but not from official node, and we get the advantages here.

Then we can revisit installing from npm separately:

  • keep the old approach for building master image
  • integrate npm publish and building the docker image for that with npm i in a single workflow

As for the dockerfile: is there a reason for back and fort changes between root and node - why not do everything as root in one go?

@tkurki
Copy link
Member

tkurki commented Feb 13, 2022

For the time being I'd like to keep things the way they are re: pm2 and not add it.

https://stackoverflow.com/questions/51191378/what-is-the-point-of-using-pm2-and-docker-together

@KEGustafsson
Copy link
Contributor Author

KEGustafsson commented Feb 13, 2022

So with this Dockerfile the essential changes would be a smaller image and working mdns? Mdns requires host network mode, right?

Jep, use of the same package as for normal installation process, quite much smaller image and working mDNS. I haven't tested other than host mode. Also all input and output ports mapping would require configuration changes to compose file.

Now the docker build in Github is triggered on pushing the tag for a release. Now this will not work, because in my release workflow I

* tag locally the release with `npm version`

* run `npm run release` that pushes changes, including the release tag, to Github and creates the Github Release

* run npm publish

So the new release may or may not be published in npm and just running npm i will install essentially a random "whatever is latest" version in npm, BUT creating a version tagged image.

RUN npm i -g signalk-server@TAG . I need to check how to pass ${{ steps.docker_meta.outputs.tags }} correctly to Dockerfile and map it to TAG. This would ensure that tagged version is also used to build docker image.

Also this approach won't be able to build docker images off master.

If master has tag then it should follow the same rules as any other tag.

So there are uses to building off the local files.

So what if we (you...) back up a little: install as previously, but not from official node, and we get the advantages here.

No problem to go back. So back to COPY repo to docker image.

re
Then we can revisit installing from npm separately:

* keep the old approach for building master image

Would this be for e.g. development etc... purposes with full repo included and

* integrate npm publish and building the docker image for that with `npm i` in a single workflow

and this for official release, slim and polished?

Would there be then actually two Dockerfiles: e.g. Dockerfile for dev and Dockerfile_Rel for releases?

As for the dockerfile: is there a reason for back and fort changes between root and node - why not do everything as root in one go?

Would you like that I remove USER node totally and there would be only USER root? No problem and that simplifies build a little bit. No need for sudo as user is already root.

For the time being I'd like to keep things the way they are re: pm2 and not add it.

I remove pm2 from this configuration but I'll keep testing it by myself.

@tkurki
Copy link
Member

tkurki commented Feb 14, 2022

Probably two dockerfiles eventually.

Node user is good, same as before and non root containers are docker best practise.

Compose could map the default 3000 port so that it is usable as such.

@KEGustafsson
Copy link
Contributor Author

KEGustafsson commented Feb 14, 2022

Now all root actions are done at first and then user is changed to node.

I would keep compose at host to keep things as simple as possible. If someone wants to do hardening etc.. and control, which port are open to/from container, that is doable. I could add commented code to compose, which adds this options and gives end user easy change from host to bridge network. Ok?

@sbender9
Copy link
Member

Why is this publishing to npm?

@tkurki
Copy link
Member

tkurki commented Mar 17, 2022

One thing lead to another: Docker release images should be built like a local install from npm => the release needs to be published in npm => we can automate that here also.

@tkurki tkurki changed the title chore: docker build updates chore: docker build updates, automate npm publish Mar 17, 2022
@sbender9
Copy link
Member

Ahh. Got it. I see you doing that elsewhere too. Nice.

@tkurki
Copy link
Member

tkurki commented Mar 18, 2022

Oh damn, I just realised there's preleases to think of: https://github.com/SignalK/signalk-server/blob/master/publishing.md

The main difference is in npm publish --tag beta. That could probably be automated with this?

And I don't get it why we have both releasing.md and publishing.md. If we get beta publishing integrated so that you just need to push a tag name ending in -beta.XX let's remove publishing.md and add this info in releasing.md.

@KEGustafsson
Copy link
Contributor Author

KEGustafsson commented Mar 18, 2022

Can beta release be named e.g. v1.41.4-beta.x if signalk-server package.json version is 1.41.4-beta.x?

Then v* release script can be modified to check if tag has -beta.x ending.

Npm signalk-server script would then be npm publish --tag beta and no changes needed to docker creation part.

Bit more IF-THEN to scripting...

@tkurki
Copy link
Member

tkurki commented Mar 19, 2022

Yes.

just to be precise: the ending needs to include the version number.

@KEGustafsson
Copy link
Contributor Author

KEGustafsson commented Mar 19, 2022

I'll add test if tag string contains substring *beta* then prerelease script will be run, otherwise normal release script will be run.
No need to check anything else for this additional IF-THEN.

@tkurki tkurki force-pushed the docker-update branch 2 times, most recently from b4f120f to 32c7edd Compare March 19, 2022 19:58
@tkurki tkurki merged commit 70da765 into SignalK:master Mar 22, 2022
@KEGustafsson KEGustafsson deleted the docker-update branch April 11, 2022 07:16
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.

None yet

3 participants