feat: added package persistence info in packages profile#3684
feat: added package persistence info in packages profile#3684
Conversation
Reviewer's GuideAdds persistence metadata to package profiles and implements ostree-aware detection of persistent vs transient packages using rpm-ostree on layered systems. Sequence diagram for ostree-aware package persistence detectionsequenceDiagram
actor SystemAdmin
participant PackageProfileUploadScript
participant RPMProfile
participant OstreeHelpers
participant RpmOstree as rpm_ostree
participant ProfileJson as profile_json_file
SystemAdmin->>PackageProfileUploadScript: run package_profile_upload
PackageProfileUploadScript->>RPMProfile: construct RPMProfile(from_file=None)
PackageProfileUploadScript->>RPMProfile: _accumulate_profile(rpm_header_list)
RPMProfile->>OstreeHelpers: _is_ostree_system()
alt ostree system
OstreeHelpers->>OstreeHelpers: _get_immutable_packages()
OstreeHelpers->>RpmOstree: rpm-ostree status --json
RpmOstree-->>OstreeHelpers: deployments with checksum
OstreeHelpers->>RpmOstree: rpm-ostree db list base_checksum
RpmOstree-->>OstreeHelpers: package list text
OstreeHelpers-->>RPMProfile: immutable_packages set
else non ostree system
OstreeHelpers-->>RPMProfile: empty immutable_packages set
end
loop for each rpm_header
RPMProfile->>RPMProfile: determine persistence
alt name not in immutable_packages and is_ostree
RPMProfile->>RPMProfile: persistence = transient
else
RPMProfile->>RPMProfile: persistence = persistent
end
RPMProfile->>RPMProfile: create Package with persistence
end
RPMProfile-->>PackageProfileUploadScript: list of Package
PackageProfileUploadScript->>PackageProfileUploadScript: serialize to dict list with persistence
PackageProfileUploadScript->>ProfileJson: write profile.json
ProfileJson-->>SystemAdmin: profile includes persistence for each package
Updated class diagram for package persistence in profile generationclassDiagram
class Package {
+str name
+str version
+str release
+str arch
+int epoch
+str vendor
+str persistence
+to_dict() dict
+__eq__(other Package) bool
+_normalize_string(value str) str
}
class RPMProfile {
+__init__(from_file TextIOWrapper)
+to_dict() dict
-_accumulate_profile(rpm_header_list List~dict~) List~Package~
}
class parse_rpm_string {
+parse_rpm_string(rpm_string str) dict
}
class _is_ostree_system {
+_is_ostree_system() bool
}
class _get_immutable_packages {
+_get_immutable_packages() set
}
class OSTreeLibrary {
+Sysroot new_default()
+Sysroot load(cancellable)
+Sysroot get_booted_deployment()
}
class RpmOstreeCli {
+status_json()
+db_list(base_checksum str)
}
RPMProfile ..> Package : creates
RPMProfile ..> _is_ostree_system : uses
RPMProfile ..> _get_immutable_packages : uses
_get_immutable_packages ..> parse_rpm_string : uses
_is_ostree_system ..> OSTreeLibrary : uses when gi_available
_get_immutable_packages ..> RpmOstreeCli : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Coverage (computed on Fedora latest) •
|
||||||||||||||||||||||||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- When loading an RPMProfile from an existing file,
pkg_dict["persistence"]will raise a KeyError for previously cached profiles that don't have this field; consider usingpkg_dict.get("persistence")with a sensible default (e.g."persistent"orNone). parse_rpm_stringcan returnNone, but_get_immutable_packagesassumes it always returns a dict and accessespackage_dict["name"]; guard against aNonereturn or broaden the exception handling to avoid unexpected TypeErrors.- In
Package.__eq__,persistenceis compared directly while other string fields are normalized; it may be safer to normalizepersistencetoo (or treatNoneconsistently) to avoid equality differences due to representation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When loading an RPMProfile from an existing file, `pkg_dict["persistence"]` will raise a KeyError for previously cached profiles that don't have this field; consider using `pkg_dict.get("persistence")` with a sensible default (e.g. `"persistent"` or `None`).
- `parse_rpm_string` can return `None`, but `_get_immutable_packages` assumes it always returns a dict and accesses `package_dict["name"]`; guard against a `None` return or broaden the exception handling to avoid unexpected TypeErrors.
- In `Package.__eq__`, `persistence` is compared directly while other string fields are normalized; it may be safer to normalize `persistence` too (or treat `None` consistently) to avoid equality differences due to representation.
## Individual Comments
### Comment 1
<location> `src/rhsm/profile.py:512` </location>
<code_context>
arch=pkg_dict["arch"],
epoch=pkg_dict["epoch"],
vendor=pkg_dict["vendor"],
+ persistence=pkg_dict["persistence"]
)
)
</code_context>
<issue_to_address>
**issue (bug_risk):** Directly indexing 'persistence' breaks backward compatibility with existing profile files
Older profile files won’t have a `"persistence"` key, so `pkg_dict["persistence"]` will raise a `KeyError` when loading them. Use `pkg_dict.get("persistence", <sensible_default>)` instead so deserialization remains backward compatible.
</issue_to_address>
### Comment 2
<location> `src/rhsm/profile.py:40-49` </location>
<code_context>
except ImportError:
yum = None
+try:
+ import gi
+ gi.require_version("OSTree", "1.0")
</code_context>
<issue_to_address>
**suggestion:** parse_rpm_string(None) is not handled, so a bad line can abort the whole rpm-ostree parsing loop
If `parse_rpm_string(line)` returns `None` for an unexpected format, `package_dict["name"]` will raise a `TypeError`. Because the inner handler only catches `ValueError` and `IndexError`, this `TypeError` will fall through to the outer `except Exception` and stop processing further lines. Please either skip `None` results before indexing (e.g. `if not package_dict: continue`) or add `TypeError` to the inner `except` so a single malformed line doesn’t abort the loop.
Suggested implementation:
```python
for line in rpm_output.splitlines():
try:
package_dict = parse_rpm_string(line)
if not package_dict:
# parse_rpm_string returned None or an empty dict; skip this malformed line
continue
package = {
"name": package_dict["name"],
"version": package_dict["version"],
"release": package_dict["release"],
"arch": package_dict["arch"],
}
packages.append(package)
except (ValueError, IndexError):
log.debug("Skipping unparsable rpm line: %s", line)
```
I inferred the surrounding loop and variable names (`rpm_output`, `packages`, and `log`) from your description, since that part of the file wasn’t shown. You will need to adjust the `SEARCH` block to exactly match the existing loop that calls `parse_rpm_string(line)` in `src/rhsm/profile.py` (e.g. the iterable, the container type being appended to, and the logging call), and then apply the same `if not package_dict: continue` pattern immediately after the `parse_rpm_string` call.
</issue_to_address>
### Comment 3
<location> `src/rhsm/profile.py:434-438` </location>
<code_context>
+ return False
+
+
+def _get_immutable_packages() -> set:
+ """
+ Get the set of packages from the immutable ostree deployment.
+ For bootc systems, uses rpm-ostree to get the true base commit packages.
+ Returns a set of (name, version, release, arch, epoch) tuples.
+ """
+ immutable_packages = set()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Return type and docstring describe tuples but the implementation returns a set of names
This currently only adds `package_dict['name']` to the set. Either update the docstring/return type to reflect a set of names, or change the implementation to store full `(name, version, release, arch, epoch)` tuples as documented.
Suggested implementation:
```python
immutable_packages = set()
```
```python
immutable_packages.add((
package_dict.get("name"),
package_dict.get("version"),
package_dict.get("release"),
package_dict.get("arch"),
package_dict.get("epoch"),
))
```
If there are any other places in `_get_immutable_packages()` (or related helper functions) where `immutable_packages.add(...)` is called with only the name or any subset of the fields, update them to add the same 5-tuple `(name, version, release, arch, epoch)` for consistency with the docstring.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
mjcr99
force-pushed
the
macano/add-persistence-information-in-package-profile
branch
2 times, most recently
from
ef972db to
f3f10f5
Compare
mjcr99
force-pushed
the
macano/add-persistence-information-in-package-profile
branch
2 times, most recently
from
870c5cd to
8077735
Compare
|
While doing some preliminary testing with a new build of subscription-manager for this pull request, I discovered that attempts to install a transient package on an image mode system with selinux turned on (Enforcing) will trigger the following selinux denials...
As a result, the package profile "persistence" attribute for the transiently installed package will be "persistent" instead of "transient". When selinux is turned off (Permissive), the selinux denials are not encountered and the expected "transient" value is set in the package profile. Hence it appears that a new selinux-policy is needed for this new feature to work as desired on an image mode (layered) system. |
jirihnidek
left a comment
jirihnidek
left a comment
There was a problem hiding this comment.
Thanks for the PR. I can confirm that it works as expected. I have some comments and requests.
In general you mix usage of OSTree Python module and parsing of rpm-ostree output. It seems sub-optimal. Maybe, there is some technical reason for it, but I do not see it. If there is any, then it should be mentioned in some comment. If there is no technical reason for using both approaches, then I would try to use only one approach Python API or parsing rpm-ostree.
|
I gave this a test on a system registered to Satellite. Newly installed packages are being marked as transient, but updated ones seems to still be marked as persistent. For example: Installing:
vim-enhanced x86_64 2:9.1.083-6.el10_1 rhel-10-for-x86_64-appstream-rpms 1.9 M
Upgrading:
vim-data noarch 2:9.1.083-6.el10_1 rhel-10-for-x86_64-baseos-rpms 17 k
vim-minimal x86_64 2:9.1.083-6.el10_1 rhel-10-for-x86_64-baseos-rpms 799 k
Installing dependencies:
vim-common x86_64 2:9.1.083-6.el10_1 rhel-10-for-x86_64-appstream-rpms 7.7 M
vim-filesystem noarch 2:9.1.083-6.el10_1 rhel-10-for-x86_64-baseos-rpms 16 k
xxd ... and 
I'd expect updated packages to be transient as well, is this unexpected? |
mjcr99
force-pushed
the
macano/add-persistence-information-in-package-profile
branch
from
68ae996 to
1509dcf
Compare
|
Hi @ianballou, thanks for testing it. |
Great, this is working as I expected now 👍 |
|
The /run/ostree-booted file has an incorrect context. It needs to be troubleshooted how it was created. Subsequently, rhsmcertd needs to be allowed the needed permissions. |
|
Seeking clarification from @zpytela
Are you saying that context As you see above, the |
|
The SELinux conversation has moved to RHEL-141391. |
| ) | ||
|
|
||
| except (ValueError, IndexError) as e: | ||
| log.debug(f"Failed to parse package line '{line}': {e}") |
There was a problem hiding this comment.
The severity of this log message should be at least warning or error.
| immutable_packages = set() | ||
| if is_ostree: | ||
| immutable_packages = _get_immutable_packages() | ||
| log.debug(f"Running on ostree system with {len(immutable_packages)} persistent packages") |
There was a problem hiding this comment.
I would write here the debug message here in the else case, when the system is not detected as ostree based system. It could be important to print here for troubleshooting some edge cases (like SELinux issues, etc.).
| result = _get_immutable_packages() | ||
|
|
||
| self.assertIsInstance(result, set) | ||
| self.assertEqual(len(result), 0) |
There was a problem hiding this comment.
There is not unit test for the case, when there is at least one transient package.
|
@ianballou Do you really need only "persistent" and "transient" states? What about the use cases, when ostree based system is used and some package is installed using |
@jirihnidek my focus has been the "dnf --transient" for bootc use case. DNF's documentation mentions this persistence feature is for bootc machines specifically, so I didn't want to impose a new concept onto "vanilla" ostree. From a technical standpoint, bootc users shouldn't be using rpm-ostree. I can understand though wanting to also have this feature make some sense for vanilla ostree hosts. I don't know enough about how rpm-ostree works to say if its behavior would mix well with the bootc persistent principles being followed here. Something to consider: bootc machines are expected to work well with subscription-manager due to the dnf connection and a wish to reduce differences with normal EL machines. Do ostree systems have a similar expectation? |
mjcr99
force-pushed
the
macano/add-persistence-information-in-package-profile
branch
from
1509dcf to
44d77c6
Compare
5 participants
Description
Hi team,
this PR introduces the tracking of persistence information for packages using
rpm-ostreeto check which packages are persistent or transient in a layered system.Testing
RHEL 10
This development has been tested in the following machine:
Testing machine:
In this type of system, all the packages are understood to be persistent since there are no layers of installed packages.
The following command is run to upload the packages profile:
Then the following information appears related to the persistence of the packages:
And as expected, no packages are shown as transient:
Fedora 43 CoreOS
To test this development
Testing machine:
This system is layered and uses the
rpm-ostreetool to install packages in layers that can be rolled back to older versions, allowing the user to better manage the installed packages.This system, as seen below, does not allow the user to install, by default, packages using
dnf:To allow the installation of packages with dnf, it's required to run the following command:
As seen in the command output, the installed packages using
dnfwill be present in the system until it's rebooted, this means these packages are the ones to be marked as transient.The
treepackage is installed to test if the implemented mechanism is able to detect it as a transient package:To test the implemented functions easily, the following script is provided:
test.py
After the previous package installation, the testing script is run and the obtained result is the expected: