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

[3006.x] Make deb scripts more compliant with Debian policy #66688

Open
wants to merge 2 commits into
base: 3006.x
Choose a base branch
from

Conversation

baby-gnu
Copy link

@baby-gnu baby-gnu commented Jul 3, 2024

What does this PR do?

  • make the deb scripts manage systemd status during upgrade
  • improve script compliance with debian policy based on dh-make examples

What issues does this PR fix or reference?

Fixes #66507

Previous Behavior

When upgrading debian salt packages, the current status of the services are not managed:

  • previously disabled packages became enable
  • masked packages make the upgrade fail
Status of the services before upgrade
○ salt-master.service - The Salt Master Server
     Loaded: loaded (/lib/systemd/system/salt-master.service; disabled; preset: enabled)
     Active: inactive (dead)
       Docs: man:salt-master(1)
             file:///usr/share/doc/salt/html/contents.html
             https://docs.saltproject.io/en/latest/contents.html

○ salt-minion.service - The Salt Minion
     Loaded: loaded (/lib/systemd/system/salt-minion.service; disabled; preset: enabled)
     Active: inactive (dead)
       Docs: man:salt-minion(1)
             file:///usr/share/doc/salt/html/contents.html
             https://docs.saltproject.io/en/latest/contents.html

○ salt-api.service
     Loaded: masked (Reason: Unit salt-api.service is masked.)
     Active: inactive (dead)
Error log during upgrade
Status of the services after failed upgrade
○ salt-master.service - The Salt Master Server
     Loaded: loaded (/lib/systemd/system/salt-master.service; enabled; preset: enabled)
     Active: inactive (dead)
       Docs: man:salt-master(1)
             file:///usr/share/doc/salt/html/contents.html
             https://docs.saltproject.io/en/latest/contents.html

○ salt-minion.service - The Salt Minion
     Loaded: loaded (/lib/systemd/system/salt-minion.service; disabled; preset: enabled)
     Active: inactive (dead)
       Docs: man:salt-minion(1)
             file:///usr/share/doc/salt/html/contents.html
             https://docs.saltproject.io/en/latest/contents.html

○ salt-api.service
     Loaded: masked (Reason: Unit salt-api.service is masked.)
     Active: inactive (dead)

New Behavior

The upgrade should work with masked services and keep previous status after upgrade.

I can't test it because I did not find the way to build custom packages to test

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices, including the
PR Guidelines.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@baby-gnu baby-gnu requested a review from a team as a code owner July 3, 2024 13:13
@baby-gnu baby-gnu requested a review from dwoz July 3, 2024 13:13
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Make deb scripts more compliant with Debian policy [master] Make deb scripts more compliant with Debian policy Jul 3, 2024
@baby-gnu baby-gnu force-pushed the fix/debian-scripts-use-debhelper branch from fe03c81 to 6732121 Compare July 3, 2024 13:23
@baby-gnu
Copy link
Author

baby-gnu commented Jul 3, 2024

To me, my PR is working fire, the generated scripts now looks like this:

salt-master.preinst
#!/bin/sh
# preinst script for salt-package.
#
# See: dh_installdeb(1).

set -e

# Summary of how this script can be called:
#        * <new-preinst> 'install'
#        * <new-preinst> 'install' <old-version>
#        * <new-preinst> 'upgrade' <old-version>
#        * <old-preinst> 'abort-upgrade' <new-version>
# for details, see https://www.debian.org/doc/debian-policy/ or
# the debian-policy package.


case "$1" in
  install|upgrade)
    [ -z "$SALT_HOME" ] && SALT_HOME=/opt/saltstack/salt
    [ -z "$SALT_USER" ] && SALT_USER=salt
    [ -z "$SALT_NAME" ] && SALT_NAME="Salt"
    [ -z "$SALT_GROUP" ] && SALT_GROUP=salt
    PY_VER=$(/opt/saltstack/salt/bin/python3 -c "import sys; sys.stdout.write('{}.{}'.format(*sys.version_info)); sys.stdout.flush();")

    # Reset permissions to fix previous installs
    find ${SALT_HOME} /etc/salt /var/log/salt /var/cache/salt /var/run/salt \
        \! \( -path /etc/salt/cloud.deploy.d\* -o -path /var/log/salt/cloud -o -path /opt/saltstack/salt/lib/python${PY_VER}/site-packages/salt/cloud/deploy\* \) -a \
        \( -user ${SALT_USER} -o -group ${SALT_GROUP} \) -exec chown root:root \{\} \;

  # remove incorrectly installed ufw salt-master directory - issue 57712
  test -d /etc/ufw/applications.d/salt-master && rm -rf /etc/ufw/applications.d/salt-master || /bin/true

  ;;

  abort-upgrade)
  ;;

  *)
    echo "preinst called with unknown argument '$1'" >&2
    exit 1
  ;;
esac

# dh_installdeb will replace this with shell code automatically
# generated by other debhelper scripts.



exit 0
salt-master.postinst
#!/bin/sh
# postinst script for salt-master
#
# see: dh_installdeb(1)

set -e

# summary of how this script can be called:
#        * <postinst> `configure' <most-recently-configured-version>
#        * <old-postinst> `abort-upgrade' <new version>
#        * <conflictor's-postinst> `abort-remove' `in-favour' <package>
#          <new-version>
#        * <postinst> `abort-remove'
#        * <deconfigured's-postinst> `abort-deconfigure' `in-favour'
#          <failed-install-package> <version> `removing'
#          <conflicting-package> <version>
# for details, see https://www.debian.org/doc/debian-policy/ or
# the debian-policy package

case "$1" in
  configure)
    if [ ! -e "/var/log/salt/master" ]; then
      touch /var/log/salt/master
      chmod 640 /var/log/salt/master
    fi
    if [ ! -e "/var/log/salt/key" ]; then
      touch /var/log/salt/key
      chmod 640 /var/log/salt/key
    fi
    chown -R salt:salt /etc/salt/pki/master /etc/salt/master.d /var/log/salt/master /var/log/salt/key /var/cache/salt/master /var/run/salt/master
  ;;
  abort-upgrade|abort-remove|abort-deconfigure)
  ;;

  *)
    echo "postinst called with unknown argument '$1'" >&2
    exit 1
  ;;
esac

# dh_installdeb will replace this with shell code automatically
# generated by other debhelper scripts.

# Automatically added by dh_systemd_enable/13.11.4
if [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ] || [ "$1" = "abort-deconfigure" ] || [ "$1" = "abort-remove" ] ; then
	# The following line should be removed in trixie or trixie+1
	deb-systemd-helper unmask 'salt-master.service' >/dev/null || true

	# was-enabled defaults to true, so new installations run enable.
	if deb-systemd-helper --quiet was-enabled 'salt-master.service'; then
		# Enables the unit on first installation, creates new
		# symlinks on upgrades if the unit file has changed.
		deb-systemd-helper enable 'salt-master.service' >/dev/null || true
	else
		# Update the statefile to add new symlinks (if any), which need to be
		# cleaned up on purge. Also remove old symlinks.
		deb-systemd-helper update-state 'salt-master.service' >/dev/null || true
	fi
fi
# End automatically added section
# Automatically added by dh_systemd_start/13.11.4
if [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ] || [ "$1" = "abort-deconfigure" ] || [ "$1" = "abort-remove" ] ; then
	if [ -d /run/systemd/system ]; then
		systemctl --system daemon-reload >/dev/null || true
		if [ -n "$2" ]; then
			_dh_action=restart
		else
			_dh_action=start
		fi
		deb-systemd-invoke $_dh_action 'salt-master.service' >/dev/null || true
	fi
fi
# End automatically added section


exit 0
salt-master.prerm
#!/bin/sh
set -e
# Automatically added by dh_systemd_start/13.11.4
if [ -z "${DPKG_ROOT:-}" ] && [ "$1" = remove ] && [ -d /run/systemd/system ] ; then
	deb-systemd-invoke stop 'salt-master.service' >/dev/null || true
fi
# End automatically added section
salt-master.postrm
#!/bin/sh
set -e
# Automatically added by dh_systemd_start/13.11.4
if [ "$1" = remove ] && [ -d /run/systemd/system ] ; then
	systemctl --system daemon-reload >/dev/null || true
fi
# End automatically added section
# Automatically added by dh_systemd_enable/13.11.4
if [ "$1" = "purge" ]; then
	if [ -x "/usr/bin/deb-systemd-helper" ]; then
		deb-systemd-helper purge 'salt-master.service' >/dev/null || true
	fi
fi
# End automatically added section

@baby-gnu
Copy link
Author

baby-gnu commented Jul 4, 2024

I fixed the find in salt-master.preinst which do try to run on nonexistent directory if service are disabled or masked.

@baby-gnu
Copy link
Author

baby-gnu commented Oct 1, 2024

I found my issue, it was because the salt-master configuration must be define before salt-cloud call to db_get salt-master/user.

I'll rebase the branch to sign commits and remove the -x debug flag when ready to be merged.

@dmurphy18
Copy link
Contributor

@baby-gnu FYI salt-api, salt-syndic and salt-cloud all require salt-master before installing, thought this was well-known and stated so in the docs

twangboy
twangboy previously approved these changes Oct 8, 2024
@baby-gnu
Copy link
Author

@baby-gnu FYI salt-api, salt-syndic and salt-cloud all require salt-master before installing, thought this was well-known and stated so in the docs

Yes, I verified, now the packaging will always order correctly the packaging scripts.

@dwoz should I still rebase the branch from time to time to update it?

Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understand need for 'set -e -x' when developing, but need to remove '-x' before this PR can be merged. Cannot have debug information going to the screen etc.

And would rebase against current branch after updating/adjusting PR

This dependency is required for ordering scripts to make sure
`salt-cloud.postinst` can access debconf `salt-master/user`.
@baby-gnu
Copy link
Author

Understand need for 'set -e -x' when developing, but need to remove '-x' before this PR can be merged. Cannot have debug information going to the screen etc.

And would rebase against current branch after updating/adjusting PR

Thanks @dmurphy18, I remove the commit which added the -x and rebased the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test:pkg Run the package tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Failed to enable unit: Unit file /etc/systemd/system/salt-master.service is masked.
6 participants