-
Notifications
You must be signed in to change notification settings - Fork 56
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
Install linux distribution-specific versions #40
Conversation
@p-mongo The Linux distro is generally useful to detect and use for both community & enterprise builds as the distro-specific MongoDB tarballs can include additional dependencies that are not in the generic 64-bit Linux legacy MongoDB builds (for example, TLS/SSL). I think it would be better to add distro detection with a fallback to generic builds if a distro tarball doesn't exist. There may also be a more generic approach (perhaps an Would you be interested in creating a more generic PR to detect the distro version? Regards, |
OK will work on that. |
Neither suse 12 (via evergreen) nor opensuse 13 (via https://app.vagrantup.com/bento) nor amazon linux have lsb_release on them, apparently. |
I am only going to handle distros listed on https://www.mongodb.com/download-center/enterprise/releases/archive. Other users can send PRs to handle their systems (for example, I'm not going to do anything for Fedora seeing how rhel and centos are detected in entirely different ways). |
Handy reference courtesy of Brian: https://github.com/chef/ohai/blob/master/lib/ohai/plugins/linux/platform.rb |
@stennie Tested this on:
Added non-enterprise support. I think this is usable as is. The one caveat is if there isn't an exact match between host OS and what mongodb is built for and e.g. suse11 package is installed on suse12, this isn't guaranteed to work. |
Thanks @p-mongo .. looks great! Will sanity check on supported distros before merging. Regards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for this submission! I had a look and have a few comments about how the code could be improved. Are you happy to go ahead make these changes? Most of them should be fairly straightforward. But if you don't have bandwidth then just let us know, and we should be able to wrangle them.
distro_version=12 ;; | ||
esac | ||
distro_id=debian | ||
elif test -f /etc/os-release; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this file is present, then this will also happen even if lsb_release
gave results earlier. So I think we want something like:
if test -z "$distro_version"; then
if test -f /etc/debian_version; then
...
elif test -f /etc/os-release; then
...
fi
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression is lsb_release and os-release are mutually exclusive on systems I tested, hence if you do make this change I would advise checking what lsb_release would produce on systems that os-release currently covers.
# which is not good | ||
distro_version=`cat /etc/debian_version` | ||
case "$distro_version" in | ||
etch*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with Debian and the exact format of this file, but perhaps slightly safer might be etch|etch/*)
(and same for all the below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, though I don't really see this making a difference in practice.
@@ -316,13 +315,144 @@ install_bin() { | |||
sslbuild=$os | |||
fi | |||
|
|||
local distro_id= distro_version= | |||
if lsb_release >/dev/null 2>&1; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually possible in some cut-down distros (eg. Docker Ubuntu images) for the lsb_release
utility to not be installed (or the lsb-release
package might have been uninstalled). On Ubuntu this causes m
to download the Debian packages, which don't work (thanks to @kevinadi for finding this!).
Therefore, in the fallback section below, we should probably look for and use /etc/lsb-release
before looking for /etc/debian_version
and /etc/os-release
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is surely a good thing to do, I seem to recall a suggestion to just use lsb_release hence the implementation I produced favors it.
distro_id=debian | ||
elif test -f /etc/os-release; then | ||
# Suse, opensuse, amazon linux, centos (but not redhat) | ||
distro_id=`(. /etc/os-release && echo $ID) | tr '[:upper:]' '[:lower:]'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not thrilled by sourcing this file, it feels like it could be an attack vector if malicious commands can be injected into it. The uppercasing also isn't necessary here, since it happens in all cases below.
So better would probably be something like:
distro_id=`sed -ne 's/^ID=//p' /etc/os-release | sed -e 's/^["'"'"']//' -e 's/["'"'"']$//' -e 's/\\\(.\)/\1/g'`
And similarly to parse VERSION_ID
below, and also in the newly-added /etc/lsb-release
fallback section above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in theory, in practice I don't think this matters hence the simpler implementation of sourcing the file.
|
||
distro_id=`echo "$distro_id" | tr '[:upper:]' '[:lower:]'` | ||
distro_version=`echo "$distro_version" | tr '[:upper:]' '[:lower:]'` | ||
if test "$distro_id" = sles || test "$distro_id" = opensuse; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A case
statement is probably marginally cleaner, for these.
# builds available for distributions that the latest version supports. | ||
# | ||
# The logic generally is to start with the correct distribution, then try | ||
# one version older and one version newer. This should handle most cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the version built for the newer distro ever work in the distro? My expectation is that this would basically never work, eg. I wouldn't expect the debian92 build to work in debian-8. In which case, if downloading it is doomed to fail, it would be better to get the legacy MongoDB build instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m currently installs rhel70 builds everywhere, if that works there are good chances of debian 9 builds installing on debian 8. I do favor older builds over newer builds.
ubuntu-16*) | ||
distros="ubuntu1604 ubuntu1404 ubuntu1204" ;; | ||
ubuntu-18*) | ||
distros="ubuntu1604 ubuntu1404 ubuntu1204" ;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have (or will soon have) ubuntu1804 builds, so this should be pre-pended.
ubuntu-18*) | ||
distros="ubuntu1604 ubuntu1404 ubuntu1204" ;; | ||
ubuntu-*) | ||
distros="ubuntu1604 ubuntu1404 ubuntu1204" ;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And ubuntu1804 here also.
|
||
amzn-*) | ||
if test "$community" = 1; then | ||
distros=amazon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double-quotes.
*) | ||
distros="" ;; | ||
esac | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to have a failsafe option to disable all distro-specific stuff, in case some user is having problems and just wants to get the legacy version, same as earlier versions of m
. Perhaps here we can check if --legacy
(or something similar like --no-distro
) has been specified, and if so, set distros=""
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
If you can make these changes that would be my preference, thanks. |
Sure, no problem. I carefully checked the version compatibility against a variety of Docker-based distros. Results summary, (alternate view). The testing mechanism was to use this
and run it with:
Future versions tend to only work in older distros sporadically/rarely, so I've taken them out. The rhel70 default was just because there needed to be a default for Enterprise, I think (which has never had a "legacy" build). I've otherwise put in "backups" between rhel/amazon and ubuntu/debian where they seem to be reliable across MongoDB versions. I also added support for Amazon Linux 2. @kevinadi, can you please test the updated code at https://github.com/devkev/m/tree/distros for me, and confirm that it works as expected? |
The diff is looking good, it's clearer too except for the sed seas of quotes as would be expected. If you are handling the ubuntu without lsb_release situation the comment saying it isn't handled should probably be removed. |
Good catch, thanks - I've updated the comments accordingly. |
I pushed a new branch on my fork at https://github.com/kevinadi/m/tree/distros which fixes some minor issues. I tested the script and check to see if the installed With
(x) doesn't work out of the box. Requires missing Amazon Linux 1 Without
(x) doesn't work out of the box. Requires missing Missing
@devkev @stennie please review the results above. Is there anything else that we can do to make the experience better? Some docker images provides a very bare bone installation that lacks basic tools such as |
That Amazon Linux |
I reviewed the commit on your branch, and left 2 comments. |
Addressed the comments in my branch. Rerun the tests just in case. Verified that all is well and the test results are unchanged from the earlier table I posted. |
No love for debian 10? |
Thanks for everyone's help testing the Linux distro pantheon! @kevinadi included all the changes from @p-mongo and @devkev in PR #40. I'm going to update the README to mention the new
Debian 10 isn't GA yet, so there aren't any associated distro-specific MongoDB packages to test. Regards, |
This is needed for enterprise server versions to reference the correct libsnmp binary, for instance.