RPM: Remove System V initialization script #409
Conversation
There was a problem hiding this comment.
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).
|
I guess that makes it easier. |
|
Yet, I don't see why. Tested the rpm in an OL9 vm and the service came up without problems. I'll investigate further. |
|
I don't get it. Even the molecule tests work fine for me. # 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-9molecule results are here:
|
|
Looks like you are building the RPM on a Red Hat system rather than on a Debian system as the Jenkins automation does. |
|
Today I learned… Debian is using different path definitions: On RH-based systems |
| 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 | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
It's a bad style to re-chown/chgrp packaged files:
chown nobody: /var/lib/jenkins
rpm -V --noconfig jenkins
# .....UG.. /var/lib/jenkinsWhen 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
👍 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 |
There was a problem hiding this comment.
We shipped support for systemd over a year ago, so I think it is safe to drop support for migrating from System V configuration. 👍
There was a problem hiding this comment.
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
Line 440 in c3546bf
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.
There was a problem hiding this comment.
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}.serviceThere was a problem hiding this comment.
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.
rpm/build/SPECS/jenkins.spec
Outdated
| Source3: jenkins.logrotate | ||
| Source4: jenkins.service | ||
| Source5: jenkins.sh |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
|
I did some manual testing of this and it looks good. Thanks for the PR! |
basil
changed the title
🔥 |
|
@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 |
|
Awesome, thanks for the heads up @basil, I'll be sure to include this accordingly. |
|
This was released in 2.417, seemingly without regression. Nice job, @philfry! |
|
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. |
|
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. |
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
%cleansection because it is no longer needed.Please note that the
%changelogsection 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 0to 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:
Submitter checklist
Fixes #408