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

Duplicated --pidfile during init if passed to mongod at startup #298

Merged
merged 1 commit into from
Aug 30, 2018
Merged

Conversation

yoz2326
Copy link
Contributor

@yoz2326 yoz2326 commented Aug 29, 2018

I'm passing --pidfilepath=/data/tmp/mongod.pid. When init is finished and mongod is shutdown, this parameter becomes duplicated. So updated the shutdown command to mongod --pidfilepath="$pidfile" --shutdown (line 327)

Copy link
Member

@yosifkit yosifkit left a comment

Choose a reason for hiding this comment

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

See comments below.

@@ -7,6 +7,16 @@ fi

originalArgOne="$1"

# Allow me to run some scripts for all container startups
# i.e. setting up a cluster keyFile when using /docker-entrypoint-initdb.d/* is not enough
for f in /docker-entrypoint-extras.d/*; do
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary since you can already hook a script to run before the entrypoint by running it before the entrypoint:

COPY my-setup-script.sh /docker-entrypoint-extras.d/my-setup-script.sh
# could also just be CMD if you want to put your mongod args into the file
ENTRYPOINT ["/docker-entrypoint-extras.d/my-setup-script.sh"]
CMD ["docker-entrypoint.sh", "--keyFile", "/path/to/keyfile"]

Contents of my-setup-script.sh:

#!/usr/bin/env bash

# pre-mongod setup 
chmod 400 /path/to/keyfile

# ie: docker-entrypoint.sh --keyFile /path/to/keyfile
exec "$@"

Copy link
Contributor Author

@yoz2326 yoz2326 Aug 30, 2018

Choose a reason for hiding this comment

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

Good point, this should do it.
Will drop this change.

@@ -312,7 +324,7 @@ if [ "$originalArgOne" = 'mongod' ]; then
echo
done

"$@" --pidfilepath="$pidfile" --shutdown
mongod --pidfilepath="$pidfile" --shutdown
Copy link
Member

Choose a reason for hiding this comment

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

oh, maybe this should be using "${mongodHackedArgs[@]}" rather than "$@"? cc @tianon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I think --pidfile needs to be removed first, if present:

_mongod_hack_ensure_no_arg_val --pidfilepath "${mongodHackedArgs[@]}"
_mongod_hack_ensure_arg_val --pidfilepath "$pidfile" "${mongodHackedArgs[@]}"

and then do:
"${mongodHackedArgs[@]}" --shutdown

Copy link
Member

Choose a reason for hiding this comment

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

The hacked args should already have the pid file path (see the --fork line above), so this line should really be just what you referenced without any new ensures. 👍

Copy link
Member

Choose a reason for hiding this comment

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

(I've force-pushed an update to this for you, and updated all five copies of this file. 👍)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks for doing this!

@@ -240,6 +250,8 @@ if [ "$originalArgOne" = 'mongod' ]; then
if [ "$MONGO_INITDB_ROOT_USERNAME" ] && [ "$MONGO_INITDB_ROOT_PASSWORD" ]; then
_mongod_hack_ensure_no_arg_val --replSet "${mongodHackedArgs[@]}"
fi
_mongod_hack_ensure_no_arg_val --keyFile "${mongodHackedArgs[@]}"
_mongod_hack_ensure_no_arg_val --clusterAuthMode "${mongodHackedArgs[@]}"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like both of these would be removed from config for the temp config since they are under security, so this part seems reasonable.

jq 'del(.systemLog, .processManagement, .net, .security)' "$jsonConfigFile" > "$tempConfigFile"

Any value but the default (keyfile) on clusterAuthMode requires sslMode.

Providing a keyFile doesn't stop the user setup or the server from running.

$ docker run -d \
  --name some-mongo \
  -e MONGO_INITDB_ROOT_USERNAME=test \
  -e MONGO_INITDB_ROOT_PASSWORD=12345 \
  --user 1000:1000 \
  -v /home/user/docker/test/mongo/some.key:/some.key \
  -v /home/user/docker/test/mongo/data/:/data/db/ \
  mongo --keyFile /some.key --clusterAuthMode keyFile
$ docker run -it --rm --link some-mongo:mongo mongo mongo --host mongo -u test -p 12345 --authenticationDatabase admin some-db

Copy link
Contributor Author

@yoz2326 yoz2326 Aug 30, 2018

Choose a reason for hiding this comment

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

Yes, that's correct. Did this a while back and don't remember exactly what prompted me to remove those agrs.
Will drop this change, thanks for checking it!

@yoz2326 yoz2326 changed the title Run mongo replicaset on Kubernetes using keyFile cluster auth mode Duplicated --pidfile during init if passed to mongod at startup Aug 30, 2018
@yosifkit yosifkit merged commit 9c95830 into docker-library:master Aug 30, 2018
tianon added a commit to infosiftr/stackbrew that referenced this pull request Sep 6, 2018
- `docker`: 18.09.0-ce-tp6
- `drupal`: 8.5.7
- `ghost`: 2.1.1
- `hello-seattle`: update URL (docker-library/hello-world#51)
- `hello-world`: update URL (docker-library/hello-world#51)
- `hola-mundo`: update URL (docker-library/hello-world#51)
- `mongo`: 3.4.17, 3.2.21, fix duplicated `--pidfile` (docker-library/mongo#298)
- `openjdk`: 11-ea+28 (debian)
- `php`: 7.3.0beta3, ship `php.ini-production` and `php.ini-development` in `$PHP_INI_DIR` (docker-library/php#711)
- `redis`: add `tzdata` to Alpine variants (redis/docker-library-redis#162)
- `rocket.chat`: 0.69.1
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