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

RPM: Remove System V initialization script #409

Merged
merged 5 commits into from
Jul 26, 2023

Conversation

philfry
Copy link
Contributor

@philfry philfry commented Jun 19, 2023

With this PR only the native startup mechanism (sysvinit or systemd) is installed and used.
For RHEL 7 and up as well as Fedora 15 and up, systemd is used, for RHEL/Fedora below these versions, the sysv initscript.

The second commit cleans up the specfile a bit, replacing hardcoded paths with their corresponding rpm macros. I also removed the %clean section because it is no longer needed.

Please note that the %changelog section might get stripped depending on the RHEL version, so that only my entry is left. If you don't want that, either add %_changelog_trimtime 0 to your global rpm macros (e.g. /etc/rpm/macros.changelog) when building the packages or simply remove my changelog entries. I'm fine with it.

Testing done

tested with RHEL6 and RHEL9:

sed -i '/^export MSI/d' setup.mk
wget https://get.jenkins.io/war-stable/2.401.1/jenkins.war
WAR=${PWD}/jenkins.war make rpm BRAND=./branding/jenkins.mk BUILDENV=./env/release.mk VERSION=2.401.1

Submitter checklist

Fixes #408

@philfry philfry requested a review from a team as a code owner June 19, 2023 13:30
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

We no longer support any RPM-based systems that use SysV init rather than systemd, so the conditional logic is not needed. Simpler to just remove the SysV init scripts for RPM-based systems. The SysV init scripts are still needed for Debian-based systems without systemd (some of which we still support).

@philfry
Copy link
Contributor Author

philfry commented Jun 21, 2023

I guess that makes it easier.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

The tests are asserting that /etc/systemd/system/jenkins.service.d/override.conf does not exist after installing the package, and this assertion is now failing.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

The automated tests now fail to start the service with:

Unable to start service jenkins: Job for jenkins.service failed because the control process exited with error code. See systemctl status jenkins.service and journalctl -xeu jenkins.service for details.

@philfry
Copy link
Contributor Author

philfry commented Jun 26, 2023

Yet, I don't see why. Tested the rpm in an OL9 vm and the service came up without problems. I'll investigate further.

@philfry
Copy link
Contributor Author

philfry commented Jun 27, 2023

I don't get it. Even the molecule tests work fine for me.
Here's what I did:

# import the rpm signing key and announce it to rpm
gpg --import credentials/test.secret.gpg
echo '%_gpg_name Bogus Test (This is test only key) <[email protected]>' > ~/.rpmmacros

# remove the msi thingy
sed -i '/^export MSI/d' setup.mk

# get the war
export ver="2.401.1"
wget https://get.jenkins.io/war-stable/${ver}/jenkins.war

# build the rpm
WAR=${PWD}/jenkins.war make rpm BRAND=./branding/jenkins.mk BUILDENV=./env/release.mk VERSION=${ver}

# as I use podman instead of docker I needed some minor adaptions
sed -i 's/docker/podman/' requirements.txt molecule/default/molecule.yml
python3 -m venv venv
source venv/bin/activate
pip install -r requirements.txt

# get the dokken images
podman pull docker.io/dokken/oraclelinux-8:latest docker.io/dokken/oraclelinux-9:latest

# run the tests
molecule test -p oraclelinux-8
molecule test -p oraclelinux-9

molecule results are here:

@basil
Copy link
Member

basil commented Jun 27, 2023

Looks like you are building the RPM on a Red Hat system rather than on a Debian system as the Jenkins automation does.

@philfry
Copy link
Contributor Author

philfry commented Jun 27, 2023

Today I learned… Debian is using different path definitions: On RH-based systems %{_sharedstatedir} is /var/lib, on Debian, it's /usr/com. My cleanup-commit fscked that up. Fixed now.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Nice job getting the tests to pass!

rpm/build/SPECS/jenkins.spec Show resolved Hide resolved
Comment on lines -90 to -107
function chownIfNecessary {
logger -t %{name}.installer "Checking ${2} ownership"
if [ -f "${1}" ] ; then
owner=$(cat "$1")
rm -f "$1"
logger -t %{name}.installer "Found previous owner of ${2}: ${owner} "
fi
if [ "${owner:-%{name}}" != "${JENKINS_USER:-%{name}}" ] ; then
logger -t %{name}.installer "Previous owner of ${2} is different than configured JENKINS_USER... Doing a recursive chown of ${2} "
chown -R ${JENKINS_USER:-%{name}} "$2"
elif [ "${JENKINS_USER:-%{name}}" != "%{name}" ] ; then
# User has changed ownership of files and JENKINS_USER, chown only the folder
logger -t %{name}.installer "Configured JENKINS_USER is different than default... Doing a non recursive chown of ${2} "
chown ${JENKINS_USER:-%{name}} "$2"
else
logger -t %{name}.installer "No chown needed for ${2} "
fi
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little worried about the outright deletion of this logic. Suppose a user of the existing package is running with a custom user and group, and then they upgrade to a new version with your changes. Have you explored this scenario logically as well as with empirical testing? I am afraid that this test would fail. There is no automated test for this. In fact, the same bug was present in the Debian version: #344

Copy link
Contributor Author

@philfry philfry Jun 27, 2023

Choose a reason for hiding this comment

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

It's a bad style to re-chown/chgrp packaged files:

chown nobody: /var/lib/jenkins
rpm -V --noconfig jenkins
# .....UG..    /var/lib/jenkins

When running jenkins under a different user/group, a different working directory (that's not included in the rpm) should be used. And this different working directory would be reflected in /etc/systemd/system/jenkins.service.d/override.conf (at least it should).
But you're right, this would be a breaking change. I'll add a workaround to the specfile in the next days.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this?

--- a/rpm/build/SPECS/jenkins.spec
+++ b/rpm/build/SPECS/jenkins.spec
@@ -58,9 +58,17 @@ rm -rf "%{buildroot}"
 	-d "%{workdir}" %{name} &>/dev/null
 
 %post
+if [ $1 -eq 1 ]; then
+    %__install -d -m 0755 -o %{name} -g %{name} %{workdir}
+    %__install -d -m 0750 -o %{name} -g %{name} %{_localstatedir}/log/%{name}
+    %__install -d -m 0750 -o %{name} -g %{name} %{_localstatedir}/cache/%{name}
+fi
 %systemd_post %{name}.service
 
 %preun
+if [ $1 -eq 0 ]; then
+    %__rm -rf %{_localstatedir}/cache/%{name}/war
+fi
 %systemd_preun %{name}.service
 
 %postun
@@ -68,9 +76,9 @@ rm -rf "%{buildroot}"
 
 %files
 %{_javadir}/%{name}.war
-%attr(0755,%{name},%{name}) %dir %{workdir}
-%attr(0750,%{name},%{name}) %{_localstatedir}/log/%{name}
-%attr(0750,%{name},%{name}) %{_localstatedir}/cache/%{name}
+%ghost %{workdir}
+%ghost %{_localstatedir}/log/%{name}
+%ghost %{_localstatedir}/cache/%{name}
 %config %{_sysconfdir}/logrotate.d/%{name}
 %{_unitdir}/%{name}.service
 %{_bindir}/%{name}

When installing the package for the first time, the directories /var/lib/jenkins, /var/log/jenkins and /var/cache/jenkins are created with the corresponding attributes. Since the directories are marked as %ghost no changes are made to the directories on package upgrades. Upon deinstallation, the /var/cache/jenkins/war directory gets removed to leave no temporary data behind.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds about right to me. I wonder if we really need special handling for /var/log/jenkins. While the default systemd configuration does put JENKINS_HOME in /var/lib/jenkins and does put the exploded WAR into /var/cache/jenkins/war, it does not (anymore) put the log in /var/log/jenkins. Rather systemd-journald(8) is used and this line is commented out by default:

#Environment="JENKINS_LOG=%L/@@ARTIFACTNAME@@/@@ARTIFACTNAME@@.log"

One could argue that if a user is moving from systemd-journald(8) to a log file by uncommenting this line, then it is their responsibility to create the log directory and ensure it has appropriate permissions. And in the default configuration, where systemd-journald(8) is being used, it might be confusing and unnecessary for that empty directory to be created when installing the package.

I don't feel strongly about this either way, though. It's possible I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO the log directory became obsolete with systemd/journald. Having a %ghost log directory won't hurt, but yes, we can skip creating that directory.
If we drop /var/log/jenkins, we should remove the logrotate config, too.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that we can drop the logrotate config. Happy if you want to do that in this PR, or it can be deferred to a separate issue/PR if desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I removed the logrotate config as well.
The specfile shrank quite a lot with all these changes.

%post
%{_datadir}/%{name}/migrate "/etc/sysconfig/%{name}" || true
Copy link
Member

Choose a reason for hiding this comment

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

We shipped support for systemd over a year ago, so I think it is safe to drop support for migrating from System V configuration. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Is there any risk that users of a recent release (e.g., the last LTS) might have been still using the System V init configuration rather than the systemd configuration by accident, simply by running the /etc/init.d scripts rather than using the service or systemctl commands?

Perhaps a more conservative choice would be to change

[ -f "${from}" ] || die "${from} does not exist"
to simply exit 0 rather 1 when the System V configuration does not exist (or, equivalently, change the caller to avoid calling migrate.sh when the System V configuration does not exist). migrate.sh already exits 0 when /etc/systemd/system/jenkins.service.d/override.conf exists. If we make the change I suggested, then we could continue shipping the migration script, which would just do nothing for new installs or upgrades from a systemd-based installation, while continuing to support migration from an older System V configuration.

The reason I'm worried about this is that if this breaks some user's configuration, I'm going to get the support call. Feel free to let me know if you think I'm overthinking this or being overly paranoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would anyone want to invoke /etc/init.d/jenkins directly? Best I could imagine is to have a poorly configured process monitor/supervisor like monit, runit or supervisord. These can easily be set up to use the systemd unit instead of the script. If we want service to use the systemd unit, we need to remove /etc/init.d/jenkins completely, which would break that hypothetical scenario (process supervisors) anyway.

Regarding the migration script – I see your point. How about that?

 %post
-%systemd_post %{name}.service
 if [ $1 -eq 1 ]; then
     %__install -d -m 0755 -o %{name} -g %{name} %{workdir}
     %__install -d -m 0750 -o %{name} -g %{name} %{_localstatedir}/cache/%{name}
+elif [ -f "%{_sysconfdir}/sysconfig/%{name}" ]; then
+    %{_datadir}/%{name}/migrate "/etc/sysconfig/%{name}" || true
 fi
+%systemd_post %{name}.service

Copy link
Member

Choose a reason for hiding this comment

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

Why would anyone want to invoke /etc/init.d/jenkins directly?

You are right, I cannot see a good reason for that.

If we want service to use the systemd unit

Yes, I think we do want service to use the systemd unit.

Comment on lines 11 to 13
Source3: jenkins.logrotate
Source4: jenkins.service
Source5: jenkins.sh
Copy link
Member

Choose a reason for hiding this comment

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

Nice, but can we now please renumber these sources starting from the first number to reflect the fact that there are now only four sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do that.

- drop sysvinit support
- remove logrotate config and log folder because we're using
  systemd-journald
- mark workdir and cachedir as %ghost to avoid re-chowning on
  upgrades
philfry and others added 2 commits July 26, 2023 00:07
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Looks good from a code perspective. I will want to do some manual testing before merging and releasing this. If you could summarize the manual testing you have done so far, that will help guide me as far as what to focus on.

@basil
Copy link
Member

basil commented Jul 26, 2023

I did some manual testing of this and it looks good. Thanks for the PR!

@basil basil enabled auto-merge (squash) July 26, 2023 17:19
@basil basil added the removed label Jul 26, 2023
@basil basil disabled auto-merge July 26, 2023 17:31
@basil basil changed the title rpm: use either systemd unit or sysv init script depending on rhel version RPM: Remove System V initialization script Jul 26, 2023
@basil basil merged commit 5481d1d into jenkinsci:master Jul 26, 2023
2 checks passed
@basil
Copy link
Member

basil commented Jul 26, 2023

+++++++++++++++++++----------------------------------------------------------------------
 4 files changed, 22 insertions(+), 467 deletions(-)

🔥

@basil
Copy link
Member

basil commented Jul 26, 2023

@philfry Many thanks for your contribution Philippe. This cleanup was long overdue.

I have filed #419 to track the same cleanup for the SUSE package. If you are interested, we would love a contribution for that. If not, this PR was a big step in the right direction.

@basil
Copy link
Member

basil commented Jul 26, 2023

@kmartens27 Once this is released in the 2.417 weekly I would recommend adding a changelog entry noting that RPM users with a custom log directory will no longer have a logrotate(8) configuration out-of-the-box. The same should likely be mentioned in the LTS upgrade guide for the LTS after 2.414 LTS (i.e., the first LTS > 2.417 that contains this PR).

@kmartens27
Copy link
Contributor

Awesome, thanks for the heads up @basil, I'll be sure to include this accordingly.

@basil
Copy link
Member

basil commented Aug 4, 2023

This was released in 2.417, seemingly without regression. Nice job, @philfry!

@philfry
Copy link
Contributor Author

philfry commented Aug 4, 2023

I saw the updated rpm this morning 😃 Thanks!

Regarding 419 – I'm not a SuSE user, but I can take a look. This won't happen in the next few weeks, though, as I lack the time.

@philfry philfry deleted the rpm-systemd branch August 4, 2023 19:07
@MarkEWaite
Copy link
Contributor

First report from a user that was depending on the System V init scripts is included in JENKINS-71787. I've closed that issue as "Won't fix" because we have required systemd since March 2022. The fact that it continued to work on Amazon Linux AMI is an untested and unexpected accident.

Amazon Linux AMI support ends Dec 31, 2023. I'll need to add it to the end of life operating system detector. I mistakenly assumed that no one would be running that operating system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rpm: please include either systemd unit or sysvinit script
4 participants