Skip to content
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

[BUG] systemd_service: automatic daemon-reload after unit file changes just may happen on first invocation #66864

Open
3 tasks done
hurzhurz opened this issue Aug 30, 2024 · 0 comments
Labels
Bug broken, incorrect, or confusing behavior needs-triage

Comments

@hurzhurz
Copy link
Contributor

Description
As per the documentation, the systemd_service module can detect if unit files were changed and a daemon-reload is necessary, so there is no need to add service.systemctl_reload if you also already have a service.running.

Though the check if a daemon-reload is needed just happens once (the first time), even if there are multiple occasions for the same service where it could be needed.

For example if you have two SLS files:
a.sls changes /etc/systemd/system/dummy.service.d/a.conf and does a service.running for "dummy" to activate the change by restart
b.sls changes /etc/systemd/system/dummy.service.d/b.conf and also does a service.running for "dummy" for the same reason

Both work fine if applied alone.
But if you for example execute them both in a highstate, the daemon-reload is only done after the first change but not after the second change.

Here is the relevant code section:

def _check_for_unit_changes(name):
"""
Check for modified/updated unit files, and run a daemon-reload if any are
found.
"""
contextkey = f"systemd._check_for_unit_changes.{name}"
if contextkey not in __context__:
if _untracked_custom_unit_found(name) or _unit_file_changed(name):
systemctl_reload()
# Set context key to avoid repeating this check
__context__[contextkey] = True

It is stores in __context__ that it has already checked if daemon-reload is needed.
If called again for the same service, it doesn't check anymore.

I don't know the reason for why it is done this way.
But maybe the __context__ part could simply be removed?

Setup
Here is an example SLS for demonstration that include two tasks that could be split in two separate SLS files:

  1. creates and starts a dummy service
  2. adds additional systemd config and restarts dummy service
create-dummy-service:
  file.managed:
    - name: /etc/systemd/system/dummy.service
    - contents: |
        [Unit]
        Description = Dummy
        [Service]
        ExecStart = /bin/bash -c "while [ 1 ]; do sleep 1 ; done"

start-dummy-service:
  service.running:
    - name: dummy

# to show if daemon-reload warning is in status output (visible in stderr) -> will show nothing
test-status-1:
  cmd.run:
    - name: "systemctl status dummy >/dev/null"

# change unit config
/etc/systemd/system/dummy.service.d/restart.conf:
  file.managed:
    - makedirs: True
    - contents: |
        # Timestamp as dummy change: {{ salt.system.get_system_date_time() }}
        [Service]
        Restart=on-failure

# to show if daemon-reload warning is in status output (visible in stderr) -> will show necessary daemon-reload because of change
test-status-2:
  cmd.run:
    - name: "systemctl status dummy >/dev/null"

# restart on change
restart-salt-minion:
  service.running:
    - name: dummy
    - watch:
      - file: /etc/systemd/system/dummy.service.d/restart.conf

# to show if daemon-reload warning is in status output (visible in stderr) -> will still show necessary daemon-reload
test-status-3:
  cmd.run:
    - name: "systemctl status dummy >/dev/null"
  • VM (Virtualbox, KVM, etc. please specify)
  • container (Kubernetes, Docker, containerd, etc. please specify)
  • onedir packaging

Expected behavior
systemd_service should detect if a daemon-reload is needed on every invocation.

Versions Report

salt --versions-report
Salt Version:
          Salt: 3006.9

Python Version:
        Python: 3.10.14 (main, Jun 26 2024, 11:44:37) [GCC 11.2.0]

Dependency Versions:
          cffi: 1.14.6
      cherrypy: unknown
  cryptography: 42.0.5
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.4
       libgit2: Not Installed
  looseversion: 1.0.2
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 22.0
     pycparser: 2.21
      pycrypto: Not Installed
  pycryptodome: 3.19.1
        pygit2: Not Installed
  python-gnupg: 0.4.8
        PyYAML: 6.0.1
         PyZMQ: 23.2.0
        relenv: 0.17.0
         smmap: Not Installed
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: 4.3.4

System Versions:
          dist: ubuntu 22.04.4 jammy
        locale: utf-8
       machine: x86_64
       release: 5.15.153.1-microsoft-standard-WSL2
        system: Linux
       version: Ubuntu 22.04.4 jammy
@hurzhurz hurzhurz added Bug broken, incorrect, or confusing behavior needs-triage labels Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior needs-triage
Projects
None yet
Development

No branches or pull requests

1 participant