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

GPII-3483: Docker image build fails on 'postinstall' script running the 'npm install' command #704

Merged
merged 7 commits into from Nov 7, 2018

Conversation

Projects
None yet
4 participants
@klown
Copy link
Contributor

klown commented Oct 30, 2018

https://issues.gpii.net/projects/GPII/issues/GPII-3483

@stepanstipl Here is a fix. I'm not an npm install expert and the --unsafe-perm flag sounds dangerous, but it works. The npm documentation says that when running npm install as root, it changes its UID to something less privileged, and that's why the postinstall script fails. Using the flag keeps the UID as root. See:
https://docs.npmjs.com/misc/scripts#user

When building docker images, is the effective user root, or has root privileges?

GPII-3483: Docker image build fails on 'postinstall' script.
Changed the 'npm install' command to use the 'unsafe-perm' flag.
See:  https://docs.npmjs.com/misc/scripts#user
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Oct 30, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Oct 31, 2018

When building docker images, is the effective user root, or has root privileges?

Answering my own question: yes. The user is root until after all the various "apk" packages are added and npm install is run. The owner of the working directory is set to "node" after everything is built, and the last step is to set the user to 'node' just before setting the default command to npm start.

I tried setting the directory privileges and user to "node" early, but then ran into another permission error when compiling code for the level-down package:
gyp WARN EACCES user "node" does not have permission to access the dev dir "/app/node_modules/leveldown/.node-gyp/8.11.4"

This stackoverflow article suggests a way to fix the gyp permissions issue is to use the --unsafe-perm flag.

It looks like using the --unsafe-perm option is the way to go.

GPII-3483: Modified dataloader shell script to use build directory
Modified the setting of the environment variable to default to the
build snapset folder, not '/tmp'.
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 1, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 2, 2018

Tested using AWS:

  • on first run: creates fresh database and loads the snapsets
  • on second run: deletes old snapsets and replaces them with current ones.
  • on third run: leaves user prefs safe intact while replacing snapsets.
  • on fourth run: updates _design/views document and snapsets.
  • on fifth run: updates snapsets.
@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 5, 2018

Since the new docker image is also used by the flowmanager and the preferences server, tested with the scripts at the following gist: https://gist.github.com/klown/9acdc77637ac48e56e279c8bee6afbff

These scripts are based on tests in @cindyli's "Learn AWS and gpii-infra", section "Test the personal cloud".

Running the scripts returned expected output.

@stepanstipl

This comment has been minimized.

Copy link
Contributor

stepanstipl commented Nov 5, 2018

@klown sorry it took me a while to get to review this. I'm not a big fan of the --unsafe-perms, even though it probably doesn't make much difference in this case since it's only during container build phase.

After trying a couple of things, the "nicest" way to achieve postInstall to succeed seems to be changing lines 8 and 9 of the Dockerfile to:

    chown -R node:node . && \
    npm install --user node -g && \

that is

  • swapping the order to run the chown first (if we run postInstall as user node, it needs permissiosn to write to the to the build directory)
  • and telling npm to use user node and install packages globally

I think it's also nicer because the postinstall conversion scripts will run under the user node, which is consistent with how the dataloader runs during runtime.

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 5, 2018

Thanks @stepanstipl

I'm not a big fan of the --unsafe-perms ...

Yes, I'm reluctant to use it too, but I couldn't find a way around it. I tried your suggested changes, running the vagrantCloudBasedContainers.sh script, but it failed in one respect.

The leveldown C++ code did not compile when executing npm install --user node -g. When vagrantCloudBasedContainers.sh tries to run the production tests at line 104:

20:28:05.684:  FATAL ERROR: Uncaught exception: Could not locate the bindings file. Tried:
 → /app/node_modules/leveldown/build/leveldown.node
 → /app/node_modules/leveldown/build/Debug/leveldown.node
 → /app/node_modules/leveldown/build/Release/leveldown.node
 → /app/node_modules/leveldown/out/Debug/leveldown.node
 → /app/node_modules/leveldown/Debug/leveldown.node
 → /app/node_modules/leveldown/out/Release/leveldown.node
 → /app/node_modules/leveldown/Release/leveldown.node
 → /app/node_modules/leveldown/build/default/leveldown.node
 → /app/node_modules/leveldown/compiled/8.11.4/linux/x64/leveldown.node

However, when I change line 9 of the Dockerfile back to npm install, it all worked. (Which is puzzling because I ran into permission issues compiling theleveldown code as user node).

I'm going to push the one change to line 8 of the Dockerfile and see what CI does.

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 5, 2018

However, when I change line 9 of the Dockerfile back to npm install, it all worked.

Heh, no it doesn't. Thepostinstall script fails to run in that case, which I missed in the generated log. Likely CI will fail in the same way. Hmmm....

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 5, 2018

The problem is that running npm install ...:

  • user needs to be other than root in order to run postinstall script,
  • user needs to be root for node-gyp (for compiling leveldown code).
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 5, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 6, 2018

As suspected, the postinstall script failed on CI. Next, trying it as you suggested @stepanstipl. Let's see what CI does.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 6, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 6, 2018

ok to test

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 6, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 6, 2018

npm install --user node -g fails on CI duplicating what I found in my VM. Currently, looking for a way to configure npm or node-gyp or some combination of them such that node-gyp can run as user node.

@stepanstipl

This comment has been minimized.

Copy link
Contributor

stepanstipl commented Nov 6, 2018

@klown thanks for trying, I've also tried a couple of things and this seems to do the job:

    chown -R node:node . && \
    su node -c 'npm install' && \

I did not dig into why exaclty it makes difference, and to be honest at a point of almost giving up on this :). The vagrantCloudBasedContainers.sh runs fine, npm test as well.

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 6, 2018

to be honest at a point of almost giving up on this :).

Me too :-). I was about to write that after trying this from the npm documentation (option 2), and failing, that I'm just going to go with the --unsafe-perm flag unless you can find a way around it. I think I'm spending too much time on this.

I'll try your latest solution and see what CI does.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 6, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 6, 2018

@stepanstipl, your changes were successful. Congratulations!

I'm going to test in my AWS dev environment.

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 6, 2018

I'm going to test in my AWS dev environment.

LGTM. The docker image I used is: https://hub.docker.com/r/herrclown/vagrant-universal/

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 7, 2018

Could @cindyli or @amb26 take a look at this and merge it if you feel it's ready? @stepanstipl and I have tested it and it looks good to us.

@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Nov 7, 2018

@klown can you please merge in the master? Thanks.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 7, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

klown commented Nov 7, 2018

CI failed running the node tests (a PouchDB test), but running those tests locally on my VM succeeded.

ok to test.

@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Nov 7, 2018

If CI continues to fail on the pouchdb test, @the-t-in-rtf's recent update on gpii-pouchdb should help to fix it.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 7, 2018

@@ -3,7 +3,7 @@ GPII_APP_DIR=${GPII_APP_DIR:-"/app"}

GPII_STATIC_DATA_DIR=${GPII_STATIC_DATA_DIR:-"${GPII_APP_DIR}/testData/dbData"}
GPII_PREFERENCES_DATA_DIR=${GPII_PREFERENCES_DATA_DIR:-"${GPII_APP_DIR}/testData/preferences"}
GPII_BUILD_DATA_DIR=${GPII_BUILD_DATA_DIR:-'/tmp/build/dbData'}
GPII_BUILD_DATA_DIR=${GPII_BUILD_DATA_DIR:-"${GPII_APP_DIR}/build/dbData/snapset"}

This comment has been minimized.

Copy link
@cindyli

cindyli Nov 7, 2018

Contributor

The name of GPII_BUILD_DATA_DIR gives an impression of the build directory at "${GPII_APP_DIR}/build". Might be better to rename it to GPII_SNAPSET_DATA_DIR. Please keep in mind to update the documentation if the renaming does happen. Thanks.

This comment has been minimized.

Copy link
@klown

klown Nov 7, 2018

Author Contributor

Done. CI passed.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Nov 7, 2018

@cindyli cindyli merged commit 7a7f06c into GPII:master Nov 7, 2018

1 check passed

default Build finished.
Details
@cindyli

This comment has been minimized.

Copy link
Contributor

cindyli commented Nov 7, 2018

Merged at 18384c7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.