-
Notifications
You must be signed in to change notification settings - Fork 430
Add deprecation warnings to "docker-entrypoint.sh" #424
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 deprecation warnings to "docker-entrypoint.sh" #424
Conversation
docker-entrypoint.sh
Outdated
@@ -181,6 +181,8 @@ done | |||
# if long and short hostnames are not the same, use long hostnames | |||
if [ "$(hostname)" != "$(hostname -s)" ]; then | |||
: "${RABBITMQ_USE_LONGNAME:=true}" | |||
|
|||
# TODO show warning about implied RABBITMQ_USE_LONGNAME variable setting |
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.
Are you thinking of removing this? I'm not sure what you meant by "implied variable setting".
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.
Yeah -- right now this block will set RABBITMQ_USE_LONGNAME
to true
if hostname
and hostname -s
have different values and RABBITMQ_USE_LONGNAME
is not already set. Do you think we should keep that behavior in the future?
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.
Friendly ping -- do you think it makes sense for us to continue setting RABBITMQ_USE_LONGNAME=true
for users when the result of hostname
and hostname -s
differ, or do you think we should deprecate that implied behavior and have users who need/want it be explicit about their environment variable instead?
docker-entrypoint.sh
Outdated
@@ -195,6 +197,8 @@ if [ "${RABBITMQ_ERLANG_COOKIE:-}" ]; then | |||
echo "$RABBITMQ_ERLANG_COOKIE" > "$cookieFile" | |||
fi | |||
chmod 600 "$cookieFile" | |||
|
|||
# TODO show warning about use of RABBITMQ_ERLANG_COOKIE variable |
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 env var is actually one of the bad necessities, as captured here: rabbitmq/rabbitmq-prometheus@83cff5a
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.
Oh, interesting -- in that case, do you think it makes sense to move the behavior/support for it into RabbitMQ itself, or would you rather it stay as a Docker-specific feature?
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 what I'm explicitly proposing here is that if this environment variable is deemed necessary, perhaps RabbitMQ 3.9 should check for RABBITMQ_ERLANG_COOKIE
to be non-empty (and use it if so) before it looks for/creates the cookie file?
docker-entrypoint.sh
Outdated
@@ -401,6 +407,8 @@ if [ "$haveSslConfig" ] && [[ "$1" == rabbitmq* ]] && [ ! -f "$combinedSsl" ]; t | |||
cat "$RABBITMQ_SSL_KEYFILE" | |||
} > "$combinedSsl" | |||
chmod 0400 "$combinedSsl" | |||
|
|||
# TODO show a warning that we created "combined" SSL file? |
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.
Yes, agreed.
docker-entrypoint.sh
Outdated
@@ -410,6 +418,8 @@ if [ "$haveSslConfig" ] && [ -f "$combinedSsl" ]; then | |||
sslErlArgs="-pa $ERL_SSL_PATH -proto_dist inet_tls -ssl_dist_opt server_certfile $combinedSsl -ssl_dist_opt server_secure_renegotiate true client_secure_renegotiate true" | |||
export RABBITMQ_SERVER_ADDITIONAL_ERL_ARGS="${RABBITMQ_SERVER_ADDITIONAL_ERL_ARGS:-} $sslErlArgs" | |||
export RABBITMQ_CTL_ERL_ARGS="${RABBITMQ_CTL_ERL_ARGS:-} $sslErlArgs" | |||
|
|||
# TODO show a warning that we set ERL_SSL_PATH, RABBITMQ_SERVER_ADDITIONAL_ERL_ARGS, and RABBITMQ_CTL_ERL_ARGS (with their set values) |
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.
RABBITMQ_SERVER_ADDITIONAL_ERL_ARGS
& RABBITMQ_CTL_ERL_ARGS
environment variables should be allowed. The docker-entrypoint shouldn't need to do anything about them though.
This contains all environment variables that get used before the Erlang VM starts, most of them used to configure the Erlang VM: https://github.com/rabbitmq/rabbitmq-server/blob/v3.8.5/scripts/rabbitmq-env
Here is a comprehensive list of all environment variables that RabbitMQ reads during the boot phase, after the Erlang VM starts: https://github.com/rabbitmq/rabbitmq-common/blob/v3.8.5/src/rabbit_env.erl#L39-L74
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 this and the "combined" file creation are really two parts of the same whole -- we only set these if we created a combined file, and these are set so that said combined file gets used everywhere, so I think we're fully in agreement there. 👍
Comments on lines not modified yetLines 36 to 40 in c58a834
Could these secrets be mounted in Lines 57 to 58 in c58a834
Could these be mounted as separate conf file instead? Lines 173 to 179 in c58a834
Why configure any defaults? Line 262 in c58a834
How is this is related to: https://github.com/rabbitmq/rabbitmq-server/blob/9b5b78d316e36c653736fa328517dc142192a1f6/scripts/rabbitmq-defaults#L25-L27 Lines 300 to 364 in c58a834
All of this looks terribly complicated. We can either ask users to configure this if limiting the container's memory or, my preference would be to handle this in RabbitMQ itself. If memory is limited via cgroups, RabbitMQ should use that value to compute the relative value. WDYT @michaelklishin? |
Yes, absolutely -- that's exactly the intent of the warning that'll be generated by https://github.com/infosiftr/rabbitmq/blob/17680a790251d50090aeff7179a76e252d262b19/docker-entrypoint.sh#L398 My plan there is to include any setting we're currently generating a configuration value for, and to print out the full config file contents (with all the final generated settings in it), so it's very obvious that literally every thing we configure is something they could easily provide via configuration file. What I do think this probably means we need to include is specific advice WRT Docker/Kubernetes "secret" objects, config objects, etc in the warning text itself, making it clear to users that they actually gain more flexibility from this than just environment variables can provide.
The end goal is not to, hence the warnings. We only do now for historical reasons. My approach with this PR was to go through the script and try and figure out the minimal set of places I could add warnings to cover any explicit or implied behavior our script is providing to warn users it'll be going away (so if they are relying on any settings/values we're putting in place, they can adjust their usage accordingly to provide those themselves). Once we've had a suitable deprecation period with warnings being printed, it might make sense to have a further period where we check for our variables, but error+exit instead of warn to force users to handle them before we start silently ignoring them? Maybe we even keep that detection/erroring until the next major release? (That code would be significantly simpler than our current script -- just looping over a list of environment variables and throwing an error for any of them being set. 😅)
Yeahhhh -- the gist of this code is "if the user gave us So, I agree it would be significantly less complicated if it were implemented in RabbitMQ itself, because I think the only change necessary is to consider both of total memory and the cgroup memory limit, and use whichever one is lower (where it currently only considers total memory). |
Also, friendly ping for thoughts on having RabbitMQ 3.9's built-in handling of Edit: I could try my hand at a PR, but I must warn you that my Erlang is very rusty. 😄 |
1efad3e
to
1dbba97
Compare
Updated most of the |
Regarding |
If you do want to check cgroups in RabbitMQ server itself, I'd suggest reading:
If either is a number, the "total memory" for the system should be the smallest value of the three ( Edit: ... basically https://github.com/kovyl2404/cg_mon, but actively maintained (and likely doing more robust calculations/detection like the code being changed in erlang/otp#2922, but hard-coding |
Copying the relevant bits of #440 (comment) so we can try to address the final outstanding bits for this: 🙏
@gerhard @michaelklishin any thoughts/opinions? 😅 😇 ❤️ |
I personally don't have an opinion on 1. I can't think of anything better, so let's keep it unless you have evidence that it confuses enough users?
If you can explain how we can compute absolute amount of memory using |
Sounds good, thanks for giving it some thought! 👍
Totally fair (and is incidentally why Docker Swarm's "secrets" functionality doesn't even support environment variables 😄) - so you think we should deprecate our code around it?
There's more details in #424 (comment), but the short version is that I'd suggest that where you're currently looking at |
Yes, definitenly
We use the proc filesystem even though I don't remember the specific location. So this suggestion sounds doable. |
Yes, mostly -- the difference is which version of the cgroups filesystem is in use:
The main situation I'm aware of where they wouldn't be available is containers on older versions of Docker (before it mounted the appropriate filesystem inside the containers), and potentially if RabbitMQ isn't in an explicit memory cgroup (I think), but falling back to total memory when they aren't available (or are set to the special value The |
You can also set up cgroup limits with systemd, so this would have value outside containers too (although more limited because I think that's going to be a lot less common). |
516bf71
to
9d5e671
Compare
With regard to |
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 personally think this is ready to go except for the wording in one warning message.
Requesting changes (sorry) instead of leaving a comment to make this more visible. This is almost there :)
9d5e671
to
5b8b491
Compare
1b1a4a5
to
2fab209
Compare
5b8b491
to
1b1a4a5
Compare
Is this good now, @michaelklishin? I'd love to merge this before we get 3.9 up so that all the 3.9 builds can remove the deprecated bits, if you agree with that aggressive of a timeline 😅 |
Changes: - docker-library/rabbitmq@59d7871: Merge pull request docker-library/rabbitmq#424 from infosiftr/entrypoint-deprecation - docker-library/rabbitmq@bc88db1: Merge pull request docker-library/rabbitmq#492 from mkuratczyk/master - docker-library/rabbitmq@051164d: Switch from SKS to Ubuntu keyserver - docker-library/rabbitmq@973c614: Add missing `\` - docker-library/rabbitmq@38ead37: Enable JIT, install g++ - docker-library/rabbitmq@8942a4b: Merge pull request docker-library/rabbitmq#490 from J0WI/alpine-3.14 - docker-library/rabbitmq@59c5913: Alpine 3.14 - docker-library/rabbitmq@2fab209: Add deprecation warnings to "docker-entrypoint.sh"
Changes: - docker-library/rabbitmq@226c6de: Update 3.8-rc to 3.8.18-rc.1 - docker-library/rabbitmq@4792573: Update 3.8 to 3.8.18 - docker-library/rabbitmq@59d7871: Merge pull request docker-library/rabbitmq#424 from infosiftr/entrypoint-deprecation - docker-library/rabbitmq@bc88db1: Merge pull request docker-library/rabbitmq#492 from mkuratczyk/master - docker-library/rabbitmq@051164d: Switch from SKS to Ubuntu keyserver - docker-library/rabbitmq@973c614: Add missing `\` - docker-library/rabbitmq@38ead37: Enable JIT, install g++ - docker-library/rabbitmq@8942a4b: Merge pull request docker-library/rabbitmq#490 from J0WI/alpine-3.14 - docker-library/rabbitmq@59c5913: Alpine 3.14 - docker-library/rabbitmq@2fab209: Add deprecation warnings to "docker-entrypoint.sh"
Changes: - docker-library/rabbitmq@ad1824a: Update 3.8-rc to otp 24.0.3 - docker-library/rabbitmq@5414666: Update 3.8 to otp 24.0.3 - docker-library/rabbitmq@226c6de: Update 3.8-rc to 3.8.18-rc.1 - docker-library/rabbitmq@4792573: Update 3.8 to 3.8.18 - docker-library/rabbitmq@59d7871: Merge pull request docker-library/rabbitmq#424 from infosiftr/entrypoint-deprecation - docker-library/rabbitmq@bc88db1: Merge pull request docker-library/rabbitmq#492 from mkuratczyk/master - docker-library/rabbitmq@051164d: Switch from SKS to Ubuntu keyserver - docker-library/rabbitmq@973c614: Add missing `\` - docker-library/rabbitmq@38ead37: Enable JIT, install g++ - docker-library/rabbitmq@8942a4b: Merge pull request docker-library/rabbitmq#490 from J0WI/alpine-3.14 - docker-library/rabbitmq@59c5913: Alpine 3.14 - docker-library/rabbitmq@2fab209: Add deprecation warnings to "docker-entrypoint.sh"
This is a strawman for discussing the changes discussed in #422 of finally deprecating our entrypoint script.
To start the conversation, I've just added some
TODO
comments in all the places I think we ought to add a deprecation warning, but would love feedback on whether I've gone too far (and some of this behavior is OK and should stay) and/or I haven't gone far enough and there's more places we should explicitly add a warning. 😄