rsync::server::module installs an rsync server even when $ensure is absent
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	MatthewVernon
	Jun 21 2022, 1:37 PM

Description

The bullseye swift frontends that are _not_ the ring_manager are being flagged for puppet changing every run - this is ms-fe[1-2]0[10-12].

An example of this is https://puppetboard.wikimedia.org/report/ms-fe1010.eqiad.wmnet/d560127136686e70ffda8ce95c70a5de3a30ccf4

You can see that puppet is starting up the rsync service; which doesn't work because there is no rsync.conf:

Condition: start condition failed at Tue 2022-06-21 12:43:47 UTC; 19min ago
           └─ ConditionPathExists=/etc/rsyncd.conf was not met

But these nodes shouldn't be trying to run an rsync daemon anyway, I think, because these nodes have the $ensure parameter of swift::ring_manager set to 'absent', which they are passing to rsync::server::module:
https://github.com/wikimedia/puppet/blob/726260c85eb62eea5c7a69ae697d34aa65143ce1/modules/swift/manifests/ring_manager.pp#L71

rsync::server::module: does include ::rsync::server (I presume so that having more than one rsync::server::module on a system will work?), but rsync::server has $ensure_service which isn't set by rsync::server::module, which is how these systems are ending up with an unconfigured rsync daemon that then results in these always-changing puppet problems.

Could you suggest how to achieve the desired result, please? [rsync running on the ring_manager hosts, but not elsewhere]

Details

Subject	Repo	Branch	Lines +/-
C:rsync::server: convert to concat	operations/puppet	production	+77 -117
rsync: unbreak header-only conf in rsync::server	operations/puppet	production	+10 -7
P:thanos::swift::frontend: initiate the rsync::server explicitly	operations/puppet	production	+5 -0
P:swift::proxy: initiate the rsync::server explicitly	operations/puppet	production	+3 -0

Customize query in gerrit

Event Timeline

MatthewVernon created this task.Jun 21 2022, 1:37 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 21 2022, 1:37 PM

MatthewVernon added a subscriber: jbond.Jun 21 2022, 1:46 PM

I'm having the same issue with thanos-fe[1-2]00[1-2] servers. There's a combination of issues at play here from what I can see.
The command that creates the rsyncd config file at https://github.com/wikimedia/puppet/blob/bdddd1fbdcf6db5b4c817ca445cde68e36fdd52b/modules/rsync/manifests/server.pp#L91 can never work when there are no fragments since it exits before entering the conditional.

Since there's no configuration, then rsyncd doesn't start

I was at first trying to fix that in https://gerrit.wikimedia.org/r/c/operations/puppet/+/825793 because I thought this was the underlying issue. This change https://gerrit.wikimedia.org/r/c/operations/puppet/+/703452 actually addresses more or less the same issue by using concat instead of a command, which is cleaner.

Fixing that would make the puppet issue go away because we'd create a functioning rsyncd.conf file, rsyncd would start, no more changes.

Not knowing the architecture wanted for swift rings, I wondered if these non-ring-manager hosts should even have a running rsyncd, leading me to the same conclusion as @MatthewVernon

A quick way of fixing it at the swift::ring_manager level would be to wrap the rsync::server::module call in a conditional, which is Not Pretty™

Another at the rsync::server::module level would be to wrap the include ::rsync::server call itself in a conditional, which at least has the advantage of describing what I suppose to be the actual wanted general behaviour (no rsync::server if no rsync::server::module) but is a more widespread change.

Change 825793 had a related patch set uploaded (by Clément Goubert; author: Clément Goubert):

[operations/puppet@production] rsync: unbreak header-only conf in rsync::server

https://gerrit.wikimedia.org/r/825793

gerritbot added a project: Patch-For-Review.Aug 23 2022, 5:27 PM

Change 703452 had a related patch set uploaded (by Jbond; author: John Bond):

[operations/puppet@production] C:rsync::server: convert to concat

https://gerrit.wikimedia.org/r/703452

Change 832628 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] P:swift::proxy: initiate the rsycn::server explicitly

https://gerrit.wikimedia.org/r/832628

ultimatly i think moving to concat is the better option so reviews welcome :). however i think that we can fix this specific error by initiating rsync::server explicitly with the correct config

Change 832630 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] P:thanos::swift::frontend: initiate the rsync::server explicitly

https://gerrit.wikimedia.org/r/832630

Change 832628 merged by Jbond:

[operations/puppet@production] P:swift::proxy: initiate the rsync::server explicitly

https://gerrit.wikimedia.org/r/832628

Change 832630 merged by Jbond:

[operations/puppet@production] P:thanos::swift::frontend: initiate the rsync::server explicitly

https://gerrit.wikimedia.org/r/832630

Change 825793 abandoned by Clément Goubert:

[operations/puppet@production] rsync: unbreak header-only conf in rsync::server

Reason:

https://gerrit.wikimedia.org/r/825793

jhathaway claimed this task.Jan 8 2024, 3:39 PM

jhathaway triaged this task as Low priority.

@MatthewVernon & @Clement_Goubert rsync server has been converted to concat and the thanos swift frontend is now explicitly initiating the rsync::server class with an ensure parameter, so I think this issue is resolved, please reopen if that is not the case.

Change 703452 abandoned by Clément Goubert: