nodeshift-archived / centos7-s2i-nodejs

DEPRECATED OpenShift S2I builder images for Node.js ✨
Apache License 2.0
33 stars 55 forks source link

nodejs:10 image not working properly in Online with odo #122

Closed jorgemoralespou closed 6 years ago

jorgemoralespou commented 6 years ago

@lance We're working with odo and using nodejs for some sample applications. While doing this in OpenShift online we have found some issues with the nodejs images, that I'll enumerate.

How you can reproduce this in OpenShift Online:

Download odo and once logged into your Online cluster do:

$ git clone https://github.com/openshift-evangelists/Wild-West-Frontend frontend 
$ cd frontend
$ odo project odo-demo
$ odo app create js-demo
$ odo create nodejs:8 frontend-8
$ odo push
$ odo create nodejs:10 frontend-10
$ odo push

You should see the nodejs:10 push is ok, and the nodejs:10 push fails. You can oc rsh <POD> into each of the 2 pods to validate the described differences.

This might be related to how the images are built, and since the base image can not easily be changed/updated, the assemble script needs to be looked at. @GrahamDumpleton is happy to provide guidance on the best practices we've found in these years of working on understanding/fixing behavior on scl s2i images.

You can ping me or him anytime.

lance commented 6 years ago

@jorgemoralespou thanks for the detailed write up. We will take a look and get this taken care of!

lance commented 6 years ago

@jorgemoralespou one thing to note... In the OpenShift Online catalog, the Node.js version 8 is different than the Node.js version RHOAR-8 and does not have parity with Node.js version 10. So the reproduction above should be the following.

$ git clone https://github.com/openshift-evangelists/Wild-West-Frontend frontend 
$ cd frontend
$ odo project odo-demo
$ odo app create js-demo
$ odo create nodejs:8-RHOAR frontend-8
$ odo push
$ odo create nodejs:10 frontend-10
$ odo push
lance commented 6 years ago

@jorgemoralespou can you please link to the fix-permissions script you reference here?

In the assemble scripts there are multiple chmod operations that should be replaced with fix-permissions script call instead. This script takes care of properly fixing permissions in an s2i image.

It's not obvious where to find this from the links provided.

GrahamDumpleton commented 6 years ago

The fix-permissions script is already in the base image and is put there for this specific purpose.

You do not need to copy it, just execute it.

jorgemoralespou commented 6 years ago

@lance

In the OpenShift Online catalog, the Node.js version 8 is different than the Node.js version RHOAR-8 and does not have parity with Node.js version 10

This is just such a mess. :-(

No wonder why I saw the differences. nodejs:8 is built by the scl team and at least they have implemented all the good practices when writing the assemble.

I guess then that nodejs:8-RHOAR should also be fixed.

lance commented 6 years ago

@GrahamDumpleton thanks.

@jorgemoralespou this versioning was done on purpose - although I can't say it's the best. The concern was that we cannot remove or change the existing Node.js '8', but the RHOAR images are going to be used going forward. So, making a distinction between '8' and RHOAR-8 was necessary. But there is no distinction necessary with 10, since SCL won't have one available in a reasonable time frame (i.e. Node 10.9.0 was released late last night and we should have a release out this afternoon).

I'm curious why fix-permissions is not in the core image instead of the base image. We have a desire to minimize the size of our images and have been considering moving to s2i-core-container.

lance commented 6 years ago

I'm curious why fix-permissions is not in the core image instead of the base image. We have a desire to minimize the size of our images and have been considering moving to s2i-core-container.

My mistake - in spite of the repo name, this appears to be in core.

lance commented 6 years ago

@jorgemoralespou @GrahamDumpleton I've created a fix for the permissions problems here: https://github.com/bucharest-gold/centos7-s2i-nodejs/pull/124. If you have a moment could you please take a look and provide any feedback if you have it? Thanks.

lance commented 6 years ago

@jorgemoralespou @GrahamDumpleton The fix for nss_wrapper is here: https://github.com/bucharest-gold/centos7-s2i-nodejs/pull/125. If you have a moment, could you please take a look here as well? Thanks.

lance commented 6 years ago

This should be resolved with the two referenced pull requests above. If there are additional problems, please open a new issue. Thanks!

lance commented 6 years ago

@GrahamDumpleton @jorgemoralespou these changes have been pushed to docker hub in latest, 10.8.0 and 8.11.3 for the centos7-s2i-nodejs image, and should be available in both upstream/community (centos7) and downstream (rhel) product releases 10.9.0 and 8.11.4 early next week, when @helio-frota has published the new node.js releases.