Usage of `systemd::escape` in `systemd::timer_wrapper` creates weird names

saz commented 3 months ago

Affected Puppet, Ruby, OS and module versions/distributions

Puppet: 7.29.1
Ruby: 2.7.8p225
Distribution: Ubuntu 20.04.6
Module version: 6.6.0

How to reproduce (e.g Puppet code you use)

  systemd::timer_wrapper { 'prometheus-node-exporter-deleted_libraries-custom':
    ensure                 => present,
    command                => '/bin/sh -c "/usr/share/prometheus-node-exporter-collec
tors/deleted_libraries.py | sponge /var/lib/prometheus/node-exporter/lvm.prom"',
    on_boot_sec            => 0,
    on_unit_active_sec     => '15min',
    timer_unit_overrides   => {
      'Description' => 'Run deleted libraries metrics collection every 15 minutes',
    },
    service_unit_overrides => {
      'Description' => 'Collect deleted libraries metrics for prometheus-node-exporte
r',
    },
    service_overrides      => {
      'Environment' => 'TMPDIR=/var/lib/prometheus/node-exporter',
    },
  }

What are you seeing

Due to the usage of systemd::escape in https://github.com/voxpupuli/puppet-systemd/blob/45e09532b9dc392fe767b555813945c94113422c/manifests/timer_wrapper.pp#L78 a - will be escaped and becomes \x2d.

● prometheus\x2dnode\x2dexporter\x2ddeleted_libraries\x2dcustom.timer - Run deleted libraries metrics collection every 15 minutes
     Loaded: loaded (/etc/systemd/system/prometheus\x2dnode\x2dexporter\x2ddeleted_libraries\x2dcustom.timer; enabled; vendor preset: enabled)
     Active: active (waiting) since Wed 2024-04-10 15:11:05 CEST; 17min ago
    Trigger: Wed 2024-04-10 15:41:06 CEST; 12min left
   Triggers: ● prometheus\x2dnode\x2dexporter\x2ddeleted_libraries\x2dcustom.service

Apr 10 15:11:05 host systemd[1]: Started Run deleted libraries metrics collection every 15 minutes.

What behaviour did you expect instead

Timer and service are named prometheus-node-exporter-deleted_libraries-custom not prometheus\\x2dnode\\x2dexporter\\x2ddeleted_libraries\\x2dcustom

Output log

Any additional information you'd like to impart

Link to the discussion on the usage of systemd::escape https://github.com/voxpupuli/puppet-systemd/pull/419#discussion_r1510333656

saz commented 3 months ago

Only timer_wrapper escapes unit names. manage_unit and unit_file are just matching a specific pattern.

TheMeier commented 2 months ago

Hm well that is actually what systemd-escape does. Since / gets replaced by -, the dash gets replaced by \x2d.

$ systemd-escape  a/b-c 
a-b\x2dc

There is the --mangle option that does ignore - but it sadly has a sideffect:

and possibly automatically append an appropriate unit type suffix to the string.

 systemd-escape  -m  a/b-c 
a-b-c.service

Not sure what we should do here.

Use systemd::escape everywhere and have consistent names?
Have systemd::escape modified to use --mangle (the call to systemd::escape should then be used on the full unit name including the type suffix)?
Encourage the use of _ instead of - in unit names?

TheMeier commented 2 months ago

BTW \x2d ist quite common. From my fedora system:

$ systemctl list-units --all  | grep '\\x2d' | wc -l
70

saz commented 2 months ago

BTW \x2d ist quite common. From my fedora system:
$ systemctl list-units --all  | grep '\\x2d' | wc -l
70

Yes, for units containing path names for example, but not "normal" services.

I'd suggest to drop the systemd::escape usage here and leave it to the user. I wasn't expecting, that my unit name gets changed to prometheus\\x2d\node... and wondered, why it's not creating the systemd unit.

TheMeier commented 2 months ago

Hm maybe you are right. Just use the users input and fail if it is an invalid name, e.g. containing /. This is a holdover I ported from themeier-systemd_cron

TheMeier commented 2 months ago

But I guess we have to mark it as breaking change then :/

saz commented 2 months ago

But I guess we have to mark it as breaking change then :/

It might get even more complicated, as the name changes, the old unit won't be managed and will stay on the system, while the new one gets created. But cleaning it up should be pretty easy to do within this module.

voxpupuli / puppet-systemd