Enable pkg tests for systemd status on upgrade #67968
Open
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
Adds test for masking services to ensure that is preserved on upgrade.
Fixes test_salt_ownership_permission tests - these were previously failing because they were looking at ownership of /run/salt/salt-minion. The ownership of that get's messed - /run/salt-minion.pid is a more reliable way of checking what user the minion is running as. Also simplified the tests slightly to remove some of the duplication.
Splits out the systemd permissions tests and systemd service status preservation tests into seperate files as they were quite long. Also moves fixtures into conftest.py to allow sharing those. test_systemd_permissions could still do with some more simplification.
cdbd742 to
1efa462
Compare
Ensures the services are started for user tests as they rely on there being a PID file.
fd605f6 to
81bb117
Compare
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