Enable pkg tests for systemd status on upgrade #67968
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
31382c6 to
ebdaa0c
Compare
ebdaa0c to
a02bdef
Compare
a02bdef to
e514927
Compare
|
Hmm, looks like the user tests are breaking in just Photon OS. I'll pull a container for that and debug. |
Specify usergroup=True when adding test users - this isn't default on Photon OS so later assumption about group ownership of PID is wrong without it.
4504bfc to
7593a45
Compare
|
Fixed issue with Photon OS - it defaults to not adding a usergroup with useradd, so the test assumption that the user and group for the pid file would be the same didn't hold. Added |
twangboy
approved these changes
Jun 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Enables tests for systemd status (enabled, active etc). These were added in #66218 but disabled. We should actually be testing this.
Spotted this when reviewing #66688 - I was surprised that wasn't breaking tests. Assume the intent was that this should be enabled once #66218 was merged and pkg tests were running again
Previous Behavior
Tests for maintaining status of systemd services on upgrade were not being run
New Behavior
tests/pytest/pkg/upgrade/test_systemd_permissions.pyandtests/pytest/pkg/upgrade/test_systemd_service_preservation.pySaltPkgInstall/usr/sbin/policy-rc.dvia a fixture - this is part of the container image but alters service restart behaviour on package installtests/pytest/pkg/upgrade/test_salt_upgrade.pyruns last an immediately before integration tests in CIConcerns/questions
I've got some concerns about the approach of some of this and would like some feedback..
Installing the previous version, running the test and then re-installing the previous version is necessary to accurately test upgrades, but it is slow. I'm not sure how we could do this reasonably without that.
To give some idea of times, the Ubuntu 24.04 upgrade goes from ~5m to ~13m and Rocky Linux 9 Upgrade from ~6m to ~12m. Should these be marked with
slow_test?Running the install->upgrade->revert in the newly enabled tests means that when we run the tests in
test_salt_upgrade, although the old version is installed, the new version has been installed multiple times, so could have mutated system state.I'd perhaps prefer to split these new tests out into a separate set of tests in their own container instance. That would leave the upgrade+integration tests we currently run to be on a totally clean instance, then these tests in something like "Test Package Systemd Upgrade". Obviously that add some complexity to the workflow run setup..
Merge requirements satisfied?
N/A
Commits signed with SSH?
Yes