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

add forked docker process #39

Merged
merged 17 commits into from
Jun 24, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions launchSearch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*!
* Copyright © 2023 United States Government as represented by the
* Administrator of the National Aeronautics and Space Administration.
* All Rights Reserved.
*
* SPDX-License-Identifier: Apache-2.0
*/

import Dockerode from 'dockerode'

const dataDir = process.argv[2]
const logsDir = process.argv[3]
const engine = process.argv[4]
const port = process.argv[5]
const options = process.argv[6].split(',')
Courey marked this conversation as resolved.
Show resolved Hide resolved

type Message = {
action: string
}

let dockerConatiner: Dockerode.Container

async function launchDocker() {
const Image =
engine === 'elasticsearch'
? 'elastic/elasticsearch:8.6.2'
: 'opensearchproject/opensearch:2.11.0'
console.log('Launching Docker container', Image)
const docker = new Dockerode()

const container = await docker.createContainer({
Env: [...options, 'path.data=/var/lib/search', 'path.logs=/var/log/search'],
HostConfig: {
AutoRemove: true,
Mounts: [
{ Source: dataDir, Target: '/var/lib/search', Type: 'bind' },
{ Source: logsDir, Target: '/var/log/search', Type: 'bind' },
],
PortBindings: {
[`${port}/tcp`]: [{ HostIP: '127.0.0.1', HostPort: `${port}` }],
},
},
Image,
})
const stream = await container.attach({ stream: true, stderr: true })
stream.pipe(process.stderr)
await container.start()
dockerConatiner = container
}

async function stopping() {
if (process.send) process.send('containerStopped')
}

async function waiting() {
await dockerConatiner.wait()
stopping()
}

launchDocker()

process.once('message', async (message: Message) => {
if (message.action === 'wait') {
waiting()
Courey marked this conversation as resolved.
Show resolved Hide resolved
} else {
await dockerConatiner.kill()
}
})

process.on('SIGTERM', async () => {
await dockerConatiner.kill()
process.exit(0)
})

process.on('SIGINT', async () => {
await dockerConatiner.kill()
process.exit(0)
})
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@
],
"type": "module",
"files": [
"index.js"
"index.js",
"launchSearch.js"
],
"scripts": {
"prepare:husky": "husky install",
"prepare:esbuild": "esbuild index.ts --bundle --packages=external --outfile=index.js --platform=node --format=esm --tree-shaking=true",
"prepare:docker": "esbuild launchSearch.ts --bundle --packages=external --outfile=launchSearch.js --platform=node --format=esm --tree-shaking=true",
Courey marked this conversation as resolved.
Show resolved Hide resolved
"prepare": "run-p prepare:*"
},
"engines": {
Expand Down
47 changes: 22 additions & 25 deletions run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import { mkdirP, temp } from './paths.js'
import rimraf from 'rimraf'
import { spawn, untilTerminated } from './processes.js'
import type { SandboxEngine } from './engines.js'
import Dockerode from 'dockerode'
import { UnexpectedResolveError, neverResolve } from './promises.js'
import { ForkOptions, fork } from 'child_process'

type SearchEngineLauncherFunction<T = object> = (
props: T & {
Expand Down Expand Up @@ -63,37 +63,34 @@ const launchDocker: SearchEngineLauncherFunction = async ({
port,
options,
}) => {
const Image =
engine === 'elasticsearch'
? 'elastic/elasticsearch:8.6.2'
: 'opensearchproject/opensearch:2.11.0'
console.log('Launching Docker container', Image)
const docker = new Dockerode()
const container = await docker.createContainer({
Env: [...options, 'path.data=/var/lib/search', 'path.logs=/var/log/search'],
HostConfig: {
AutoRemove: true,
Mounts: [
{ Source: dataDir, Target: '/var/lib/search', Type: 'bind' },
{ Source: logsDir, Target: '/var/log/search', Type: 'bind' },
],
PortBindings: {
[`${port}/tcp`]: [{ HostIP: '127.0.0.1', HostPort: `${port}` }],
},
},
Image,
const subprocess = fork(
'./node_modules/@nasa-gcn/architect-plugin-search/launchSearch.js',
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not hardcode this path. Can you __filename like in the example code in https://nodejs.org/api/child_process.html#child_processforkmodulepath-args-options?

Copy link
Contributor Author

@Courey Courey Jun 12, 2024

Choose a reason for hiding this comment

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

There are 3 main ways to get where a file is located. __dirname to get the directory, __filename to get the file, or process.cwd() to get the current working directory of the process. I had to do a little bit of fiddling to get __dirname and __filename to work:
Screenshot 2024-06-12 at 1 24 52 PM

As long as the package is not symlinked it works as expected:
Screenshot 2024-06-12 at 1 37 54 PM

however, if you symlink the package to work locally, it will not work:
Screenshot 2024-06-12 at 1 23 36 PM
The problem is that it points to the path of code that you are symlinking, not the copy in .node_modules (even though it does exist in .node_modules, which is why hard coding worked)

I am not sure how often not being able to symlink is a dealbreaker for local development. I use it, but I don't know if it's widely used by others using the package.

Would you still like me to use __dirname + the file name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added code in the last commit that checks to see if the directory includes node_modules in the path. if it does it just uses __dirname but if it doesn't (which would mean a symlink) then it returns the hard coded path to the node_modules file.
It's not too much cleaner than just hard coding it, but a little better.

Copy link
Member

Choose a reason for hiding this comment

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

Take another look at the sample code fork child_process.fork in the Node.js documentation:

if (process.argv[2] === 'child') {
  setTimeout(() => {
    console.log(`Hello from ${process.argv[2]}!`);
  }, 1_000);
} else {
  const { fork } = require('node:child_process');
  const controller = new AbortController();
  const { signal } = controller;
  const child = fork(__filename, ['child'], { signal });
  child.on('error', (err) => {
    // This will be called with err being an AbortError if the controller aborts
  });
  controller.abort(); // Stops the child process
}

It's the same file. You have some variable --- in this case process.argv --- that you use to check if you are in the parent process or the child process.

This is analogous to the typical usage of the fork() syscall. See example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you are saying exactly here.
It sounds like you are giving me advice on how to tell what is the parent vs child process. This is not an issue that I am having.

The issue that you initially pointed out was that the filename was hard coded. You wanted me to use something similar to __filename. (__filename doesn't work out of the box in es6 so you have to do a few extra steps to define it.)

The issue I encountered is that if have symlinked architect-plugin-search in another application for development, the path to the launchSearch.js file is different than if it is not symlinked. When symlinked the path to the file points to my local architect-plugin-search repo which is a different path than when it is in node_modules.

Presumably the two are in sync since they are a simlink, so whatever changes you make in the imported package from the node_modules directory will be reflected in your local repo. Which makes this not really a big deal (it will run either way). It just is weird that the path would point to the linked repo instead of the node_module within the application. So my solution is just to define __dirname and leave it at that. It can run in the symlinked repo or it can run in the node_modules if not symlinked. Either way works.

So I just ended up removing the check to see if node_modules is in the path. It'll work either way, it'll just be pointing to the external repo instead of the node_modules if symlinked.

Copy link
Member

Choose a reason for hiding this comment

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

I understand now that __filename only works in CommonJS modules. Just use import.meta.url instead. No need to convert it to a path, because child_process.fork accepts URLs.

Why is it a problem that this might return the target of the symlink?

Copy link
Contributor Author

@Courey Courey Jun 13, 2024

Choose a reason for hiding this comment

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

import.meta.url is architect-plugin-search/index.js so that can't be used directly. Here is what I'm doing there and why it needs to have multiple steps:

  1. import.meta.url: file:///Users/courey/dev/architect-plugin-search/index.js this is getting the name of the file that is currently running. in this case it's index.js. I don't want index.js, I want launchSearch.js.
  2. so we then pass that into fileURLToPath so __filename would then be: /Users/courey/dev/architect-plugin-search/index.js. This is preparing it so that we can use path.
  3. I don't want the file, just the directory. Since we converted that to a path with fileURLToPath we can use path.dirname() to get just the path of the directory instead of the file that is running. that makes __dirname: /Users/courey/dev/architect-plugin-search
  4. I add /launchSearch.js to the end of that path to have the proper path and proper file name.

stuff that doesn't work

If I just used import.meta.url we get this error:

Error: Cannot find module '/Users/courey/dev/gcn.nasa.gov/file:/Users/courey/dev/architect-plugin-search/index.js'

if I added /launchSearch.js to the import.meta.url we get:

Error: Cannot find module '/Users/courey/dev/gcn.nasa.gov/file:/Users/courey/dev/architect-plugin-search/index.js/launchSearch.js

import.meta.url passed to fileURLToPath and appended with file gives us this: /Users/courey/dev/architect-plugin-search/index.js/launchSearch.js

import.meta.url passed to path.dirname(): file:///Users/courey/dev/architect-plugin-search

Error: Cannot find module '/Users/courey/dev/gcn.nasa.gov/file:/Users/courey/dev/architect-plugin-search/launchSearch.js'

if you pass import.meta.url directly into path.dirname() you get this:
file:///Users/courey/dev/architect-plugin-search

If I skipped all that and went straight to using import.meta.dirname (which should exist in nodejs 20.11) instead it is undefined, which would give us:

Error: Cannot find module '/Users/courey/dev/gcn.nasa.gov/undefined/launchSearch.js'

Copy link
Member

Choose a reason for hiding this comment

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

Is it undefined because you are doing a CommonJS build rather than an ESM build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for posterity in case someone gets here and doesn't read all the other comments, we answered this in another comment. It is not supported prior to node v.20.11.0, so it is undefined in any version earlier than that.

[dataDir, logsDir, engine, port, options] as ForkOptions
Courey marked this conversation as resolved.
Show resolved Hide resolved
)

subprocess.stdout?.on('data', (data) => {
console.log(`subprocess stdout: ${data}`)
})

subprocess.stderr?.on('data', (data) => {
console.error(`subprocess stderr: ${data}`)
})
Courey marked this conversation as resolved.
Show resolved Hide resolved

const waitUntilStopped = new Promise<void>((resolve) => {
subprocess.on('message', (message) => {
if (message === 'containerStopped') {
resolve()
}
})
})
const stream = await container.attach({ stream: true, stderr: true })
stream.pipe(process.stderr)
await container.start()

return {
async kill() {
console.log('Killing Docker container')
await container.kill()
subprocess.send({ action: 'kill' })
Courey marked this conversation as resolved.
Show resolved Hide resolved
},
async waitUntilStopped() {
await container.wait()
await waitUntilStopped
},
Courey marked this conversation as resolved.
Show resolved Hide resolved
}
}
Expand Down
Loading