Add Mosquitto tuning

[Imaanpreet]

• I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

• Foreman 3.4/Katello 4.6
• Foreman 3.3/Katello 4.5
• Foreman 3.2/Katello 4.4
• Foreman 3.1/Katello 4.3
• For Foreman 3.0 or older, please create a separate PR.
• We do not accept PRs for Foreman 2.3 or older.

[ekohl]

If this is common (and I'm quite sure it is), I'd prefer to support this properly in our installer. I'm worried our installer actually purges this file again.

Having said that: EL8 has a default file limit of 262144 so you're SERIOUSLY lowering it. See systemctl show -p DefaultLimitNOFILE and systemctl show -p LimitNOFILE foreman-mosquitto.service as well.

Are you sure you weren't testing this on EL7 which has a much lower limit of 4096?

[Imaanpreet]

Hello @ekohl , we are already calling it here [1] right?

[1] https://github.com/theforeman/foreman-documentation/blob/master/guides/common/modules/con_performance-tuning-quickstart.adoc.

@pablomh could you please answer about the 4096 limit thing ?

[ekohl]

What I mean is that in our documentation we want to avoid users managing individual files. If this is something that we want to support, the installer should have some option to do so. It may take tuning profiles into account, depending on how we choose to implement it.

[pablomh]

Hi @ekohl. In our mosquitto testing we found the following issue in a medium tuned (not that it matters in this case) capsule when trying to register the 1015th client:

Unable to accept new connection, system socket count has been exceeded. Try increasing "ulimit -n" or equivalent.

We were able to overcome the issue making the modifications that we propose in this PR, so maybe the mosquitto system unit is not inheriting properly that property.

[ekohl]

https://github.com/systemd/systemd/blob/23f3a6f5ff864fd26063c6c35fdaa6d85de566c7/NEWS#L5880-L5914 does have quite a bit to say on this.

Note that the soft limit remains at 1024 for compatibility reasons: the
traditional UNIX select() call cannot deal with file descriptors >=
1024 and increasing the soft limit globally might thus result in
programs unexpectedly allocating a high file descriptor and thus
failing abnormally when attempting to use it with select() (of
course, programs shouldn't use select() anymore, and prefer
poll()/epoll, but the call unfortunately remains undeservedly popular
at this time).

Then later:

Programs that want to take benefit of the increased limit have to "opt-in" into
high file descriptors explicitly by raising their soft limit.

That 1015 is seriously close to 1024 that it makes me suspect it's observing the soft limit by default and I suspect that if Mosquitto was modified to raise its soft limit, it would automatically get a lot more file descriptors.

I think we should pursue the path of fixing it upstream first. For now I've opened eclipse/mosquitto#2674 to see if it's a viable solution.

[jhutar]

Thank you Ewoud! Do we want to add a reference to that upstream ticket to the Tuning Guide in this PR?

[mjivraja]

Hi @Imaanpreet

I have comments on some lines adhering to the style guide. I hope, it will be helpful to you.

[maximiliankolb]

@Imaanpreet Thanks for applying feedback. I've restarted the GHA pipeline. Please also have a look at used upstream terminology such as "Foreman" or "Katello" in your PR. In Foreman documentation, we generally use attributes, see https://github.com/theforeman/foreman-documentation/blob/master/guides/common/attributes-base.adoc

[github-actions]

The PR preview for 77b2446 is available at theforeman-foreman-documentation-preview-pr-1747.surge.sh

The following output files are affected by this PR:

• Tuning_Performance/index-foreman-deb.html
• Tuning_Performance/index-foreman-el.html
• Tuning_Performance/index-katello.html
• Tuning_Performance/index-orcharhino.html
• Tuning_Performance/index-satellite.html

show diff

show diff as HTML

[jhutar]

Hello @maximiliankolb and @mjivraja . Please does this look OK now?

[maximiliankolb]

This procedure still needs to make use of attributes, e.g. "satellite-installer" > "{foreman-installer}" and so on.

Also, it's not included, therefore we cannot see the built HTML output.

I also feel like the introduction is more of a background story then technical documentation. Would you mind rewording it?

Please also stick to "one sentence per line" as per our conventions.

[mjivraja]

Hello @maximiliankolb and @mjivraja . Please does this look OK now?

Some comments are yet to be addressed. Can you have a look once again?

[Imaanpreet]

Hello @maximiliankolb & @mjivraja : could you please check now?

[ehelms]

@adamruzicka Would you mind having a glance over this?

[maximiliankolb]

Made some suggestions.

[Imaanpreet]

Hello @maximiliankolb : I have applied some more changes as per the suggestions. Could you please check now? Please correct me if I have missed anything. Thank you in advance :)

[maximiliankolb]

A few more comments:

Right now, the first four lines are four paragraphs. It's probably best to remove the empty lines 5, 7, and 9 so that it'll be one paragraph.
Second, you have to actually include your procedure in an assembly. Right now, your content is not part of any guide.
Also, please ensure the file name, anchor, and title match.

[Imaanpreet]

A few more comments:

Right now, the first four lines are four paragraphs. It's probably best to remove the empty lines 5, 7, and 9 so that it'll be one paragraph. Second, you have to actually include your procedure in an assembly. Right now, your content is not part of any guide. Also, please ensure the file name, anchor, and title match.

I hope now made changes will help.

[maximiliankolb]

Looks like there is a merge conflict right now. Please rebase to master & then we can trigger the GHA so you'll see your change rendered.

[Imaanpreet]

Looks like there is a merge conflict right now. Please rebase to master & then we can trigger the GHA so you'll see your change rendered.

done. could you please check now.

[ehelms]

I think we can drop this line, as it won't age well release to release.

[ehelms]

I think we should drop the temporal part of this statement:

Suggested change

	Now, {Project} has a pull-based transport that has the features of REX but with a deployment model similar to katello-agent.
	{Project} has a pull-based transport that has the features of REX but with a deployment model similar to katello-agent.

[ehelms]

@Imaanpreet If you could address my two comments, and then also squash into a single commit we should be good to go.

[Imaanpreet]

@Imaanpreet If you could address my two comments, and then also squash into a single commit we should be good to go.

done @ehelms
thank you

[Imaanpreet]

@ehelms : Hello, could you please check once? I have all the commits in one commit.
77b2446

[ehelms]

This can be merged -- I am not doing so at present as I cannot open the cherry picks presently.

[maximiliankolb]

Thanks @Imaanpreet for your PR! Merged to master and cherry-picked: ✔️
84a5bd0..9fe3e5f 3.5 -> 3.5
a351fd5..60b7f60 3.4 -> 3.4
0f62814..ac07b9b 3.3 -> 3.3