-
Notifications
You must be signed in to change notification settings - Fork 105
Change MQTT tuning procedure to a "custom-hiera"-based one #4262
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
base: master
Are you sure you want to change the base?
Conversation
The PR preview for 963bfd8 is available at theforeman-foreman-documentation-preview-pr-4262.surge.sh The following output files are affected by this PR: |
I'm not sure about this. A few months ago there was a conversation about how we should steer (Satellite) users away from using hiera (#3752 was the result). That this PR adds a step to edit custom-hiera.yaml seems to go against that. @pablomh Can you share a bit more context? Is using hiera justified here even for Satellite users? |
guides/common/modules/proc_increasing-host-limit-for-pull-based-rex-transport.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_increasing-host-limit-for-pull-based-rex-transport.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_increasing-host-limit-for-pull-based-rex-transport.adoc
Show resolved
Hide resolved
guides/common/modules/proc_increasing-host-limit-for-pull-based-rex-transport.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_increasing-host-limit-for-pull-based-rex-transport.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_increasing-host-limit-for-pull-based-rex-transport.adoc
Outdated
Show resolved
Hide resolved
Sure. The current problem is that the systemd configuration file will be wiped the next time In our performance testing we tend to run |
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've always been a bit torn by this. We don't support any content in custom-hiera.yaml
and by documenting it we effectively support it. I didn't test it, but I had the impression we didn't manage any drop ins so it wouldn't be pruned. Turns out I was wrong: https://github.com/voxpupuli/puppet-mosquitto/blob/3e88c0a2a447cb5399b4b5a71a864caf7dbff240/manifests/service.pp#L24-L38. So it does make sense in this case.
And then I also had a look at whether that's still needed. eclipse-mosquitto/mosquitto@1f31f14 has recently been merged, but is currently not in any release.
[options="nowrap", subs="+quotes,verbatim,attributes"] | ||
---- | ||
# {foreman-installer} --foreman-proxy-plugin-remote-execution-script-mode pull-mqtt | ||
cat >>/etc/foreman-installer/custom-hiera.yaml <<EOF |
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.
Typically we don't tell users to run shell commands to add content to files. It's very easy to mess up. In this case they can type cat > /etc/foreman-installer/custom-hiera.yaml
and wipe out any contents they had before.
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.
That's why I was careful to write '>>' in case someone copy-pasted it (as I would do). Do you have any recomendation?
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.
Looking at our writers, but I think we typically have some text that comes down to "add the following to /etc/foreman-installer/custom-hiera.yaml".
Another thing to keep in mind: if the user had already added the Hiera key then your command will break. By having a more generic description we put that responsibility on the user.
|
||
.Procedure | ||
. Enable pull-based remote execution on your {ProjectServer} or {SmartProxyServer}: | ||
This example configures the `mosquitto` service on a {ProjectServer} or {SmartProxyServer} to handle 5000 content hosts. |
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're moving away from content hosts
in favor of just hosts
Thanks for taking the time to explain @pablomh! Looking at the whole guide, I can see that it references the manual config file a few times already so at the very least, adding one more should be okay. Consider my concern addressed :) |
In an ideal world, we'd be OK if this was properly implemented, but I had little time to play with it and had to resort to this approach. |
A short term workaround that we deploy by default could be to enhance https://github.com/voxpupuli/puppet-mosquitto to gain a parameter. We can then always set that in our Hiera config. That way we don't need any tuning guide section but it just works out of the box for users. AFAIK increasing the maximum number of open files doesn't have a huge downside. |
That way the configuration file won't be deleted when `foreman-installer` is run again and won't be needed to reload or restart services manually.
I think I've included all the recommended changes (I don't know how to deal with the GitHub reviews, sorry). |
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 Pablo. I left three more minor suggestions.
---- | ||
# {foreman-installer} --foreman-proxy-plugin-remote-execution-script-mode pull-mqtt | ||
---- | ||
This example configures the `mosquitto` service on a {ProjectServer} or {SmartProxyServer} to handle up to 5000 hosts. |
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.
"{ProjectServer}" can resolve to "orcharhino Server", which would lead to "a orcharhino Server". I have two suggestions: Either ".. on your {SmartProxy}" or "on your {ProjectServer} or {SmartProxyServer}".
|
||
.Prerequisites | ||
* You have enabled pull-based remove execution on your {SmartProxy}. | ||
+ |
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 don't need a line break between these sentences. Example: https://github.com/theforeman/foreman-documentation/blob/master/guides/common/modules/proc_installing-and-configuring-puppet-agent-during-host-provisioning.adoc?plain=1#L10
.Procedure | ||
. On your {SmartProxy}, set the upper limit of connected hosts for pull-based remote execution in `/etc/foreman-installer/custom-hiera.yaml`: | ||
+ | ||
[options="nowrap", subs="+quotes,verbatim,attributes"] |
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.
[options="nowrap", subs="+quotes,verbatim,attributes"] | |
[source, yaml, options="nowrap", subs="+quotes,verbatim,attributes"] |
That way the configuration file won't be deleted when foreman-installer is run again and won't be needed to reload or restart services manually.
What changes are you introducing?
Use
custom-hiera.yaml
to tune MQTT.Why are you introducing these changes? (Explanation, links to references, issues, etc.)
Change some manual configuration instructions that would be reverted on the next call to
foreman-installer
.Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
Contributor checklists
Please cherry-pick my commits into:
Review checklists
Tech review (performed by an Engineer who did not author the PR; can be skipped if tech review is unnecessary):
Style review (by a Technical Writer who did not author the PR):