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 6.0 release #553

Merged
merged 2 commits into from
Aug 24, 2022
Merged

Add 6.0 release #553

merged 2 commits into from
Aug 24, 2022

Conversation

yosifkit
Copy link
Member

fixes #552

@tianon
Copy link
Member

tianon commented Jul 21, 2022

I pushed https://github.com/docker-library/mongo/compare/1886739107c864c9a1b18e8f2d176acaf6f0e4af..091bdab11774adf475955fc46b28b4dc69fe89a2 which removes Client from the Windows builds (although as I type this, realizing I forgot to remove mongo --version too 🙈) -- next will be looking at the failing test and what we'll need to do to update it to mongosh.

Edit: removed mongo --version appropriately in https://github.com/docker-library/mongo/compare/091bdab11774adf475955fc46b28b4dc69fe89a2..f2777f31b2c0222b8b8e67151c94a6938b78a079

@tianon
Copy link
Member

tianon commented Jul 21, 2022

Just to record my digging on the latest Windows failure, I think this is responsible for the build failing (although the wmi log output makes that hard to be sure about):

MSI (s) (A0:1C) [17:32:29:801]: Executing op: ActionStart(Name=InstallCompassScript,Description=Installing MongoDB Compass... (this may take a few minutes),)
MSI (s) (A0:1C) [17:32:29:879]: Executing op: CustomActionSchedule(Action=InstallCompassScript,ActionType=1089,Source=BinaryData,Target=WixQuietExec64,CustomActionData="C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe" -NoLogo -NoProfile -NonInteractive -ExecutionPolicy Bypass -Command "& '' ; exit $($Error.Count)")
MSI (s) (A0:F0) [17:32:29:882]: Invoking remote custom action. DLL: C:\Windows\Installer\MSI9E27.tmp, Entrypoint: WixQuietExec64
WixQuietExec64:  The expression after '&' in a pipeline element produced an object that was not valid. It must result in a command 
WixQuietExec64:  name, a script block, or a CommandInfo object.
WixQuietExec64:  At line:1 char:3
WixQuietExec64:  + & '' ; exit $($Error.Count)
WixQuietExec64:  +   ~~
WixQuietExec64:      + CategoryInfo          : InvalidOperation: (:String) , RuntimeException
WixQuietExec64:      + FullyQualifiedErrorId : BadExpression
WixQuietExec64:   
WixQuietExec64:  Error 0x80070001: Command line returned an error.
WixQuietExec64:  Error 0x80070001: QuietExec64 Failed
WixQuietExec64:  Error 0x80070001: Failed in ExecCommon method

From what I can tell, https://github.com/mongodb/mongo/blob/r6.0.0/src/mongo/installer/msi/wxs/Installer_64.wxs#L199-L203 is somehow expanding [#InstallCompassScript] to the empty string. 😅

@yosifkit
Copy link
Member Author

Oops lol 🙈, the current windows install failure is our fault; the log we are digging through is what it looks like on success.

if (-Not (Test-Path C:\mongodb\bin\mongo.exe -PathType Leaf)) { \
Write-Host 'Installer failed!'; \
Get-Content install.log; \
exit 1; \
}; \

@yosifkit yosifkit force-pushed the add-6 branch 2 times, most recently from 3b44174 to 02f03df Compare July 21, 2022 18:46
@yosifkit
Copy link
Member Author

Fixed the check for mongo.exe and fixed nanoserver to not try running mongo when it doesn't exist 😆

@tianon
Copy link
Member

tianon commented Jul 21, 2022

I've fixed the test in docker-library/official-images#12836 (so once that merges, we can restart the builds here and validate that we're still good on old versions and fixed on the new one 👍)

@yosifkit
Copy link
Member Author

More mongo assumptions causing a failure:

mongo=( mongo --host 127.0.0.1 --port 27017 --quiet )

@tianon
Copy link
Member

tianon commented Jul 21, 2022

Gah, that one has deeper implications than the one in our test suite. 😅 🙈

Up until now, any .js files in /docker-entrypoint-initdb.d have been implied to be run by mongo ... (and any .sh files invoking "${mongo[@]}" have been able to assume that's running mongo ...), and us just quietly changing that to mongosh, even if we only do it for 6.0+, is a little bit of a breaking change that I think we need to spend a little time considering more carefully. 😇

It's inevitable that we have to do something, because mongo is literally gone now, but I think we need to spend some time thinking about what we do instead of just knee-jerking s/mongo/mongosh/ as my own personal tendency would be. The fact that even our absolute bare-minimum, total-basics test ran into several rough edges in the conversion that required some minor modification makes me think that perhaps just swapping is probably going to have some pretty gnarly downstream effect.

We've never been particularly happy with how this script is implemented and the sorts of hacks it has to do to even function remotely in a way that makes sense, and I have to stop and wonder if we can avoid making an already annoying-to-maintain script even worse in this transition. 😬 🙈

(My gut says probably not, but I'm trying to stay optimistic -- this entrypoint script is totally out of control already with custom behavior we maintain above and beyond what MongoDB upstream maintains, supports, or even recommends, which is why we avoided creating it for so long in the first place!)

@Romain7495

This comment was marked as spam.

versions.json Outdated
@@ -230,5 +230,66 @@
}
},
"version": "5.0.9"
},
"6.0": {
"changes": "https://jira.mongodb.org/issues/?jql=project%20%3D%20SERVER%20AND%20fixVersion%20%3D%20%226.0.0%22%20ORDER%20BY%20status%20DESC%2C%20priority%20DESC",

Choose a reason for hiding this comment

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

This issues doesn't seem to exists. Is this an internal one?

Copy link
Member Author

Choose a reason for hiding this comment

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

That comes directly from https://downloads.mongodb.org/current.json. it is not something we actively use; it is just a link taken from the upstream json.

@sepatel
Copy link

sepatel commented Aug 8, 2022

Gah, that one has deeper implications than the one in our test suite. sweat_smile see_no_evil

Up until now, any .js files in /docker-entrypoint-initdb.d have been implied to be run by mongo ... (and any .sh files invoking "${mongo[@]}" have been able to assume that's running mongo ...), and us just quietly changing that to mongosh, even if we only do it for 6.0+, is a little bit of a breaking change that I think we need to spend a little time considering more carefully. innocent

To add some color, there are quite a few breaking changes in mongo 6 vs mongo 5 actually and things are going to break. With mongosh specifically for example, there is no more .save() method on the collection so every script whether it is .js or .sh break because a LOT of people were doing .save() in the past. Also, the need to support .sh based scripts in general went away completely since you have a full fledge nodejs 16 to play with now which can do everything including running fork processes and shell commands.

Even if you created something custom to run scripts and stuff like before, the contents of all those scripts would break because it isn't backwards compatible. There is a module you can install (mongocompat I think it was) that is suppose to enable it to be backwards compatible as well but it in and of itself is a bit of a source of problem as well.

So either you can include the legacy shell in your container package which then causes docker container version to do things that other versions including what is officially supported by cloud mongo to not work out of the box correctly (breaking things in a worse way i'd imagine), or you embrace the fact that 6.x means it has breaking changes from 5.x (just like how 5.x had breaking changes from 4.x) and that in order to upgrade, you will have to make changes to things.

My preference is to forget backwards compatibility, axe the .sh script processing stuff as well, and embrace the power of mongosh as the new way of doing things cleanly.

@sohaha

This comment was marked as spam.

@shahyar
Copy link

shahyar commented Aug 11, 2022

It's a major release version. No one can expect to blindly upgrade their image to 6.0 and have it work. With that said, from a cursory glance of the state of things right now, I'd hope it would be working (or close to it) if we s/ mongo / mongosh / the whole pipeline.

But yeah, this does look tough to maintain. There seems to be a lot of handholding of Docker users in this code, for what seems to be to avoid running into potentially cryptic config/setup issues with MongoDB? Gut it all and let people prep for 6.0 the right (hard 🥲) way.

@xianzhuo-sky

This comment was marked as spam.

@tianon tianon force-pushed the add-6 branch 2 times, most recently from d51abc4 to 3f1ee37 Compare August 18, 2022 19:14
@tianon
Copy link
Member

tianon commented Aug 18, 2022

I've added 3f1ee37 here -- this is explicitly a breaking change as it removes all the (very, very hacky) environment variable support in favor of users doing this "correctly". I'm not thrilled about this because doing so "correctly" is quite complicated, but there are so many edge cases here that I really believe we did more harm than good by introducing them. 🙈 😞

@tianon
Copy link
Member

tianon commented Aug 18, 2022

https://www.mongodb.com/compatibility/docker references MONGO_INITDB_ROOT_USERNAME/MONGO_INITDB_ROOT_PASSWORD, but https://www.mongodb.com/docs/manual/tutorial/install-mongodb-enterprise-with-docker/ and https://www.mongodb.com/compatibility/deploying-a-mongodb-cluster-with-docker both seem to explicitly not reference any of our MONGO_ variables (except for the ARG values we still include in the Dockerfile), so I think this is probably still in line with MongoDB's expectations for this image? 😬

(I really wish we could find a good contact to ask questions like this to make sure we're representing them well 🙈)

@Wazbat
Copy link

Wazbat commented Aug 21, 2022

it removes all the (very, very hacky) environment variable support in favor of users doing this "correctly"

Ah, so this would remove support for ENV variables like MONGO_INITDB_ROOT_USERNAME? I understand why this could be a difficult choice, and I completely understand why you might be worried about how this "represents" mongodb as a whole.

Setting things up with only a few environment variables is easy after all, and it feels like one of mongo's selling points to new devs is that it's an easy to use database... It's also how other other docker images like mysql are ran

Is there noone on the mongodb who can weigh in on this?

@razvanavram
Copy link

razvanavram commented Aug 22, 2022

For anyone trying to get mongo:6.0.0 with Docker until a version is officially released use this Dockerfile (along with entrypoint) link

Also, in Dockerfile edit with this:

RUN chmod 755 /usr/local/bin/docker-entrypoint.sh
ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]

@ehiggs
Copy link

ehiggs commented Aug 22, 2022

@tianon is there a tracking issue for the tests?

@sepatel
Copy link

sepatel commented Aug 22, 2022

it removes all the (very, very hacky) environment variable support in favor of users doing this "correctly"

Ah, so this would remove support for ENV variables like MONGO_INITDB_ROOT_USERNAME? I understand why this could be a difficult choice, and I completely understand why you might be worried about how this "represents" mongodb as a whole.

Setting things up with only a few environment variables is easy after all, and it feels like one of mongo's selling points to new devs is that it's an easy to use database... It's also how other other docker images like mysql are ran

Is there noone on the mongodb who can weigh in on this?

Just to reiterate, I'm personally in favor of removing all the custom stuff like that. I love how easy it is to do but it should be mongo's job to officially support it better. If support for that kind of stuff is what people feel should exist, this should be a thing that uses the core mongo:6 as a baseline with a different thing to suppose customizations. Maybe something like mongo:6-easy or something as I've seen other products have different tags to represent the same product with different packaging/rules associated with them.

@tianon
Copy link
Member

tianon commented Aug 22, 2022

Untested test fix in docker-library/official-images#13006

Comment on lines 399 to 409
# MongoDB 3.6+ defaults to localhost-only binding
haveBindIp=
if _mongod_hack_have_arg --bind_ip "$@" || _mongod_hack_have_arg --bind_ip_all "$@"; then
haveBindIp=1
elif _parse_config "$@" && jq --exit-status '.net.bindIp // .net.bindIpAll' "$jsonConfigFile" > /dev/null; then
haveBindIp=1
fi
if [ -z "$haveBindIp" ]; then
# so if no "--bind_ip" is specified, let's add "--bind_ip_all"
set -- "$@" --bind_ip_all
fi
Copy link
Member

@tianon tianon Aug 22, 2022

Choose a reason for hiding this comment

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

🤦 this part is kind of important 😂

(rediscovered while testing docker-library/official-images#13006)

Edit: fixed via CMD in 0d43dd0

@yosifkit
Copy link
Member Author

Just to add my thoughts, I don't think we should remove the current entrypoint functionality. While the entrypoint is "hacky" and should probably not be used for production database setup*, it does make the image much more user friendly for local development purposes.

* since a production setup is likely much more complex with things like sharding and replicas (like needing an external service to know when and which one should have rs.init applied and how to join or remove new replicas). Our script can't currently keep up with all the different production-like configuration setups we have to change or remove in order to start the temporary server for first initialization (like #509).

@tianon
Copy link
Member

tianon commented Aug 22, 2022

That's fair - feel free to get rid of my commit (that's part of why I left it separate) and come up with an alternative fix 👀

@xianzhuo-sky

This comment was marked as off-topic.

@tianon tianon merged commit b149de1 into docker-library:master Aug 24, 2022
@tianon tianon deleted the add-6 branch August 24, 2022 18:29
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Aug 24, 2022
Changes:

- docker-library/mongo@b149de1: Merge pull request docker-library/mongo#553 from infosiftr/add-6
- docker-library/mongo@8e7e80e: Update "latest" to the 6.0 series
- docker-library/mongo@06b403b: Add 6.0 release
- docker-library/mongo@39f62d0: Add Debian Bookworm and Ubuntu Jammy in versions.sh (both unused)
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.

Create MongoDB 6.0 Docker image