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

Feedback before writing: bootstrap using node command #724

Closed
kevynb opened this issue Jun 27, 2020 · 4 comments
Closed

Feedback before writing: bootstrap using node command #724

kevynb opened this issue Jun 27, 2020 · 4 comments

Comments

@kevynb
Copy link
Collaborator

kevynb commented Jun 27, 2020

Context: This is used to share my initial draft of a new best practice to get feedback before writing it down.
Title: Bootsrap the code using 'node' command, avoid 'npm run' scripts

TL;DR:

  • Can cause problems if you’re using child-processes
  • Signals not forwarded, cannot handle them correctly
  • Extra logs on shutdown because nom prints its exit errors (weak argument, but can be problematic when auto exporting logs in json to things like ELK)
  • Use —init if using child-processes
  • Use CMD array syntax: CMD ["node", "server.js"] not CMD "node server.js"
  • Handle SIGTERM in node process

Should we add code examples for non working cases?
Should we add a code example showing how to handle signals or is this not the subject here?

Links:

https://maximorlov.com/process-signals-inside-docker-containers/

https://github.com/nodejs/docker-node/blob/master/docs/BestPractices.md#handling-kernel-signals

Sorry for the poor formatting, I’m doing this from my phone, I forgot my laptop for the weekend 😅

@goldbergyoni
Copy link
Owner

@kevynb Great start.

Some thoughts:

  1. Note that Docker graceful shutdown - 1st draft #716 is about graceful shutdown so maybe refer to it and here the added value is also the child-process thing as you wrote
  2. --init doesn't work at K8S
  3. Can be useful to show code examples of CMD command, using TINY, etc
  4. You may emphasize that using 'npm start' results with adding a process to the container that actually does nothing

@kevynb
Copy link
Collaborator Author

kevynb commented Jul 10, 2020

I’m sorry for not having the time to open a proposer PR, here’s the “issue” version:

Bootstrap container using node command

8.X Bootstrap container using node command, avoid npm start

** TL;DR ** use CMD ['node','server.js'] to start your app. This prevents problems with child-process, signal handling and avoid creating unnecessary processes.

Read More: Bootstrap container using node command, avoid npm start

——————

Bootstrap container using node command instead of npm

One paragraph explainer

We are used to see code examples where folks start their app using CMD 'npm start'. This is a bad practice. The npm binary will not forward signals to your app which prevents graceful shutdown (see #716). If you are using Child-processes they won’t be cleaned up correctly in case of unexpected shutdown, leaving zombie processes on your host. npm start also results in having an extra process for no benefit. To start you app use CMD ['node','server.js'].

Code example

FROM node:12-slim AS build
WORKDIR /usr/src/app
COPY package.json package-lock.json ./
RUN npm ci --production && npm clean cache --force

CMD ['node', 'server.js']

Antipatterns

Using npm start

FROM node:12-slim AS build
WORKDIR /usr/src/app
COPY package.json package-lock.json ./
RUN npm ci --production && npm clean cache --force

# don’t do that!
CMD “npm start”

Using node in a single string will start a bash/ash shell process to execute your command. That is almost the same as using npm

FROM node:12-slim AS build
WORKDIR /usr/src/app
COPY package.json package-lock.json ./
RUN npm ci --production && npm clean cache --force

# don’t do that, it will start bash
CMD "node server.js"

Starting with npm, here’s the process tree:

$ ps falx
  UID   PID  PPID   COMMAND
    0     1     0   npm
    0    16     1   sh -c node server.js
    0    17    16    \_ node server.js

There is no advantage to those two extra process.

Sources:

https://maximorlov.com/process-signals-inside-docker-containers/

https://github.com/nodejs/docker-node/blob/master/docs/BestPractices.md#handling-kernel-signals

@goldbergyoni
Copy link
Owner

@kevynb Want me to copy to a PR or wait? Your call, I'll gladly do both:)

@kevynb
Copy link
Collaborator Author

kevynb commented Jul 22, 2020

I finally opened the PR #733 , thanks @goldbergyoni !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants