Skip to content

Gate atd service on package ensure. - #99

Draft
janbrono wants to merge 1 commit into
simp:masterfrom
janbrono:gate_atd_service
Draft

Gate atd service on package ensure.#99
janbrono wants to merge 1 commit into
simp:masterfrom
janbrono:gate_atd_service

Conversation

@janbrono

Copy link
Copy Markdown

PROBLEM:
The at class always declared service { 'atd': ensure => 'running' }, even when package_ensure was absent or purged. That made Puppet try to manage a running service while the at package was being removed or absent, which is inconsistent and can fail once the service unit is gone.

SOLUTION:
Introduce $manage_service = !($package_ensure in ['absent', 'purged']) and wrap the atd service resource in if $manage_service { ... } so the service is not declared when the package is not expected to be present. Document package_ensure accordingly, update REFERENCE.md, and add RSpec contexts for absent, purged, and latest.

TESTING METHODOLOGY:
CI.

OUTCOME:
Removing or purging the at package no longer triggers management of atd; installs using installed, present, latest, or a version string still get the service managed as before.

PROBLEM:
The at class always declared service { 'atd': ensure => 'running' }, even when package_ensure was absent or purged. That made Puppet try to manage a running service while the at package was being removed or absent, which is inconsistent and can fail once the service unit is gone.

SOLUTION:
Introduce $manage_service = !($package_ensure in ['absent', 'purged']) and wrap the atd service resource in if $manage_service { ... } so the service is not declared when the package is not expected to be present. Document package_ensure accordingly, update REFERENCE.md, and add RSpec contexts for absent, purged, and latest.

TESTING METHODOLOGY:
CI.

OUTCOME:
Removing or purging the at package no longer triggers management of atd; installs using installed, present, latest, or a version string still get the service managed as before.
@silug

silug commented May 11, 2026

Copy link
Copy Markdown
Contributor

Your change seems reasonable. Did you intend for it to still be in draft state?

@silug silug moved this from New to Todo in Org Triage May 11, 2026
@hcaballero2

Copy link
Copy Markdown
Contributor

Thanks for catching this @janbrono — the bug is real: with package_ensure => 'absent' or 'purged', the module keeps trying to manage a running atd while removing its package, so any node that legitimately opts out of at gets a permanently failing catalog.

We reviewed the PR and think the fix needs a few changes before it can merge. Documenting them here:

1. Declare the service stopped/disabled instead of omitting it. Gating service { 'atd': } out of the catalog delegates the stop/disable entirely to the RPM %preun scriptlet. If the package removal ever fails (dependency conflict, dnf lock, scriptlet error), atd stays running and enabled while Puppet reports clean runs forever — nothing in the catalog ever remediates it. It also breaks compilation for any external profile with require/notify => Service['atd']. The pattern in pupmod-simp-haveged handles both: in removal mode, declare

service { 'atd':
  ensure => 'stopped',
  enable => false,
  before => Package['at'],  # stop before removal; converges even if the erase fails
}

2. Gate the config files too. With the package absent/purged, the class still manages concat { '/etc/at.allow': } (with the root fragment) and forces /etc/at.deny absent — recreating access-control files every run on a host that no longer has at, which contradicts the PR's own rationale. These should move inside the "package present" branch (and arguably be ensure => absent in removal mode).

3. Housekeeping the RELENG CI enforces. The branch predates current master (metadata.json 1.0.0 vs the 2.0.1 tag), so it needs a rebase, a version bump past 2.0.1, and a matching CHANGELOG entry — pkg:compare_latest_tag fails as-is.

4. Smaller items: AGENTS.md on master documents the service as unconditionally declared and would need updating; the new spec contexts for absent/purged are verbatim copies (loopable), and the latest context duplicates the default service expectations (shareable via shared_examples).

One thing we verified while reviewing, in case it comes up: ensure => 'purged' does work with the yum/dnf providers — the purgeable feature is auto-granted because the provider implements purge — so documenting purged as supported is fine.

@hcaballero2 hcaballero2 added the needs-attention No human activity past this item's review threshold. Applied and removed automatically. label Aug 31, 2026
@simp-automation simp-automation Bot added community Contributed from outside the simp org. Applied automatically; not removed. and removed needs-attention No human activity past this item's review threshold. Applied and removed automatically. labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community Contributed from outside the simp org. Applied automatically; not removed.

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

4 participants