Add quorum queue auto-reconciliation parameter support

[LangJV]

Pull Request (PR) description

Add quorum queue auto-reconciliation parameter support -- default undef all of them so it does nothing unless you enable.

This Pull Request (PR) fixes the following issues

Fixes #1029

[wyardley]

Thanks for the contribution, and thanks for getting the docs updated and tests passing! Overall, LGTM. few comments inline.

Other big question: do some / all of these parameters depend on each other, or do they all depend on $quorum_membership_reconciliation_enabled?

Currently, the template adds each item in isolation (and the spec test tests them each in isolation), but I believe this would let you set, say, $quorum_membership_reconciliation_interval without $quorum_membership_reconciliation_enabled if I'm reading the code right? I'm not sure whether the other settings have builtin defaults, but assuming $quorum_membership_reconciliation_enabled is the main parameter controlling this, it's also possible that the other parameters could have defaults set and not be optional. Assuming $quorum_membership_reconciliation_enabled is a top level parameter that controls whether this feature is enabled, I do think the template and test should be nested, verifying that none of the items is in the config when it's unset, and then testing the various flags for the case where it is set.

Similarly, are there parameters where, if one is set without the other, the config would be invalid? We probably don't have to make all of that perfect, but we should try to at least handle obvious cases to make invalid configs harder to create with the module.

If the (old) versions the acceptance tests run against support this config directive, it would be ideal to add an acceptance test as well, which would give a bit more confidence that all of this place nice together, though I probably won't block merging this without it if everything makes sense. I'd also be curious to know what version the feature was introduced in - gating it in the module on the rabbitmq version is probably overkill, but if it's very recent, may want to at least include that info in the docs.

Happy to make some time to chat about this on the Puppet Slack or IRC if it helps.

[wyardley]

I don't claim to be a big Rubyist, and can double-check if you think this is right, but IMO, ~= Undef isn't the right thing here, since a) it's an unnecessary regex and b) I think it's not directly checking for whether the var is defined.

Maybe

Suggested change

	<% unless $rabbitmq::config::quorum_membership_reconciliation_enabled =~ Undef {-%>
	<% if $rabbitmq::config::quorum_membership_reconciliation_enabled {-%>

if it works as expected -- I think this is preferred for the two boolean parameters in particular, or for the cases where they're not Bools and testing emptiness or not doesn't work, something like:

Suggested change

	<% unless $rabbitmq::config::quorum_membership_reconciliation_enabled =~ Undef {-%>
	<% if $rabbitmq::config::quorum_membership_reconciliation_enabled != undef {-%>

as seen elsewhere in this file (or unless ... == undef if you prefer).

Same for the other similar lines.

So, the too bools at least, let's for sure just do if rabbitmq::config::quorum_membership_reconciliation_enabled, and see what works for the int values.

[LangJV]

Thanks for the assistance with this. Ill admit I'm not a big rubyist either. Yesterday I ended up down a big rabbithole (pun intended) and google marathon getting this to work and test cases to pass. At the end of the day, there are 3 specific cases I wanted to implement and cover idempotently:
Case 1: If undef, undefined, nil -> should literally "do nothing" - that is, there should be no entry in rabbitmq.config at all, and it will therefore revert to the rabbitmq global default setting (if the rabbitmq version you are on has one). This keeps this code "safe" and fully backward compatible with any RabbitMQ version
Case 2: If "true" -> should explicitly write to the rabbitmq.config file the true value
Case 3: If "false" -> should explicitly write to the rabbitmq.config file our desired false setting

My logic here was:
-All Vars are configured optional and to default to undef. This makes this "safe" in it can be applied even to a rabbitmq version that doesn't support these parameters, you simply cannot "set" them until you upgrade to a supported version, Global defaults for all these variables exist in rabbitmq 3.13+ - so they don't need to be set unless you "want to" I did make mention in the Reference.MD doc that these parameters require 3.13+ I was toying with adding a notify or failure message if the RabbitMQ version was below that, but was hesitant to due to potential upgrade race conditions. If someone on 3.12 "upgrades" and at the same time tries to input these params net-new on a single puppet run, the first puppetrun would still register the rabbitmq version "fact" at 3.12. It wouldn't register 3.13 until the subsequent run. This would cause a failure/notify. Notify might work as it wouldn't block, but I didn't see other examples in the code that led me to believe it was normal/standard for this repo.

• Apparently, through my research (not a ruby expert) i found that undef, undefined, and nil can all be valid "empty/unset" as far as EPP templates are concerned. I actually pulled the "unless" modifier verbatim from the puppet Documentation on EPP templates: https://puppet.com/docs/puppet/5.5/lang_template_epp.html and that "seems" like what they suggest for an optional variable.
• Simply checking for "if true" (my initial test here) didn't cover the undef and false cases idempotetly, and I wanted to cover that to allow explicitly setting true AND false (to be able to fully override a global default, should rabbitmq change the global default in the future.

[wyardley]

Ah, yes, you're right that for it to be an optional bool, you need the undef / true / false behavior for the booleans too. That makes sense.
Sorry for missing that the version requirement was in there

I think it makes sense to not check the fact. There are some version guards that check for puppet version, but they're mostly in the provider code, I think.

You're also right about the Puppet examples using =~ Undef... going to need to ask some folks about that, but I think either should work.

[wyardley]

Also, is there a clear case where setting the enabled parameter to false explicitly is useful? If not, (and assuming the default is false), I think the best scenario for now might be to make it a boolean with a default of false vs. an optional bool.

[LangJV]

@wyardley - i think functionally defaulting to false, and having false "omit" from config altogether, with true adding to the config is functionally identical to my solution. The only case where it might cause an issue would be a specific hypothetical case in the future where RabbitMQ changes their default behavior for this to "true", In this case, i would no longer have a way to disable it, as flipping it to false would remove from the rabbitmq.config, inheriting the default to "true".
While that's a hypothetical future case, this is why i coded it to fully handle true/false/undef. However, if its not worth accounting for them ever changing the RabbitMQ default, i wouldn't be opposed to re-writing to be:

• Default False, if false, omit from rabbitmq.config and inherit global default (also preserves compatibility with RabbitMQ < 3.13)
  if true - set to true in rabbitmq.config

I think the integer values, i likely would still need to use the undef / (integer) optional construct, as I'd need to have some form of undef/false to exclude it altogether for RabbitMQ < 3.13 to avoid breaking things.

[wyardley]

Yes, those other configuration parameters should remain optional, as well as the other bool probably.

I'm guessing it's unlikely for the value default to change in the future, but could be worth asking the RMQ folks if there's an easy way? I think usually we try to avoid optional bools where possible.

[wyardley]

Also @binford2k straightened me out here... in this context, =~ is apparently a "type match operator", and not related to regex. So leave it as-is... maybe I'll go in and adjust the others in the file for consistency (separately) later.

[wyardley]

https://www.puppet.com/docs/puppet/8/lang_data_type#lang_data_type_usage-match-expressions

Docs on above ☝️

[LangJV]

Thanks for the contribution, and thanks for getting the docs updated and tests passing! Overall, LGTM. few comments inline.

Other big question: do some / all of these parameters depend on each other, or do they all depend on $quorum_membership_reconciliation_enabled?

I believe that is the global "enablement" parameter, yes. Looking at the documentation: https://www.rabbitmq.com/docs/quorum-queues#replica-reconciliation - all of the related parameters have default values, for example the "interval" one defaults to 3600000 even though the enabled defaults to false.
I wasn't clear on whether i should try to micro-manage this logic. I figured someone enabling them might choose to:

• Explicitly set all 5 to what "they want"
• Explicitly set only the ones they want to set, and let the global defaults work for the remainder

I do see this does open cases like the ability to forcibly set only "interval" for example, but it won't actually take action within RabbitMQ because "enabled" is set to default/false still. However I was concerned if I added logic to enforce those types of dependencies and combination requirements, then it might require maintenance/upkeep if that is ever augmented, updated, or changed, on the RabbitMQ side?

Is it simpler to just let puppet configuration essentially be a transparent relay/proxy here? Its loosely validating we pass in proper values (boolean, integer) in the right places, but RabbitMQ can do the validation(s) on - Param X is not going to actually do anything because Param Y wasn't also set to Z" type RabbitMQ logic?

Currently, the template adds each item in isolation (and the spec test tests them each in isolation), but I believe this would let you set, say, $quorum_membership_reconciliation_interval without $quorum_membership_reconciliation_enabled if I'm reading the code right? I'm not sure whether the other settings have builtin defaults, but assuming $quorum_membership_reconciliation_enabled is the main parameter controlling this, it's also possible that the other parameters could have defaults set and not be optional. Assuming $quorum_membership_reconciliation_enabled is a top level parameter that controls whether this feature is enabled, I do think the template and test should be nested, verifying that none of the items is in the config when it's unset, and then testing the various flags for the case where it is set.

This i wasn't clear on either and the documentation from RabbitMQ isnt super clear.
I would suspect i can probably (not that i would, but I could)

• Disable quorum_membership_reconciliation_enabled
• Enable quorum_membership_reconciliation_auto_remove
  This would disallow automatic "growing" but allow automatic "shrinking". This would mean there isn't one parameter that disables or enables this globally, but several that can be enabled/disabled for specific functions?
  As i wasn't super familiar with the underlying RabbitMQ functionality., I was again steering clear of implementing anything beyond the variable type/range/unset limitations within the RabbitMQ module.

Similarly, are there parameters where, if one is set without the other, the config would be invalid? We probably don't have to make all of that perfect, but we should try to at least handle obvious cases to make invalid configs harder to create with the module.

If the (old) versions the acceptance tests run against support this config directive, it would be ideal to add an acceptance test as well, which would give a bit more confidence that all of this place nice together, though I probably won't block merging this without it if everything makes sense. I'd also be curious to know what version the feature was introduced in - gating it in the module on the rabbitmq version is probably overkill, but if it's very recent, may want to at least include that info in the docs.

To the best I can tell from the docs - this feature was added in 3.13. I am not familiar at all with acceptance tests against old versions, is that a different "spec" test type I missed in my review? I think i could definitely add something for this as this "shouldn't" work (or really be set at all since it will prevent rabbitmq startup) in older versions for sure. Having trouble wrapping my head around what or how i would do this. Is there an example you might point me to i can try to reverse-engineer and apply to this situation?

Happy to make some time to chat about this on the Puppet Slack or IRC if it helps.

[wyardley]

I figured someone enabling them might choose to:

Explicitly set all 5 to what "they want"
Explicitly set only the ones they want to set, and let the global defaults work for the remainder

That sounds reasonable to me as far as the child parameters. But IMO, it's probably best to require the rabbitmq::config::quorum_membership_reconciliation_enabled parameter around the whole block so that the other params don't get put in the config unless it's true explicitly? I think this will also slightly simplify your test cases?

Should be Ok to test multiple parameters together within the enabled test case too, so I'd say (depending on where we end up on enabled being a bool or an optional bool)

• 1 spec that none of the config params are found in the rendered config when rabbitmq::config::quorum_membership_reconciliation_enabled is false (or undef, if we keep it optional). If we go the route of making it default to false, then that test should be called "with defaults" or something.
• 1 test with setting enabled, all other parameters unset (test that they're not set still)
• 1 test that tests all parameters with some setting set (test that they're set).

I am not familiar at all with acceptance tests against old versions, is that a different "spec" test type I missed in my review?

Until #926 or something like it ships, all the acceptance tests are shipping with the (distribution) vendor packages and a very moldy RMQ version, so it's a moot point anyway, as far as this goes.

But yes, there are acceptance tests defined here (which run in CI, and can run locally (see this and this)

[LangJV]

@wyardley

This sounds reasonable and workable for sure. Appreciate your insight and feedback. Let me get the tests updated to model this better!

[LangJV]

Sorry for the delay - this should be updated now with the logic we discussed - mainly:
All params are optional to and default to undef, means full backward compatibility with pre RabbitMQ 3.13
All params depend on the "enabled" param - if that isnt set - none of the others are either

The Reference.MD makes note of this as well.

[wyardley]

Thanks for updating this, and thanks for fixing that unrelated typo too!

Sorry for a little more back and forth, but have a couple other hopefully minor changes to request.

[wyardley]

Thanks for the contribution, and appreciate you sticking with all the back and forth and adjustments!

I'll try to cut a release with this within next couple of days, but tag me here if that doesn't happen.

[wyardley]

Ack, forgot to squash merge it or have you squash the commit in your branch, so commit history is going to be a little bit ugly.