This repository has been archived by the owner on Oct 13, 2023. It is now read-only.
[ART-3965] A new mechanism to detect rpm changes in stream builds #798
Conversation
This file contains 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
vfreex
force-pushed
the
new-outdated-rpm-detection
branch
from
acfe933
to
effa5d0
Compare
vfreex
changed the title
vfreex
force-pushed
the
new-outdated-rpm-detection
branch
from
effa5d0
to
ea946ae
Compare
vfreex
requested review from
sosiouxme,
joepvd,
thegreyd,
Ximinhan and
locriandev
sosiouxme
approved these changes
Jul 14, 2023
doozerlib/cli/scan_sources.py
Outdated
| @@ -109,6 +110,7 @@ def add_image_meta_change(meta, rebuild_hint: RebuildHint): | |||
| oldest_image_event_ts = None | |||
| newest_image_event_ts = 0 | |||
| for image_meta in runtime.image_metas(): | |||
| dgk = image_meta.distgit_key | |||
There was a problem hiding this comment.
appears nothing uses this?
There was a problem hiding this comment.
dkg is used in line 167 and 170 for logging but we forgot to define it within the loop.
Maybe adding this variable here is not obvious because the actual use is far away from this line.
I will remove this and open another PR for the fix.
There was a problem hiding this comment.
feels like there had to be an existing module that already does all this, but i haven't actually looked for one! oh, well.
There was a problem hiding this comment.
I noticed there are existing modules to read the content of primary.xml.gz, but didn't find anything for modules.yaml.gz. Fortunately it is not hard to parse those content so decided to do it by myself (also as a learning experience)
doozerlib/repodata.py
Outdated
|
|
||
|
|
||
| class OutdatedRPMFinder: | ||
| async def find_non_latest_rpms(self, rpms_to_check: List[Dict], repodatas: List[Repodata], logger: Optional[Logger] = None) -> List[Tuple[str, str, str]]: |
There was a problem hiding this comment.
looong method. looks like it would be easy to break up into smaller testable methods?... i mean you managed to test it anyway, just seems like it would've been cleaner broken out.
| @@ -188,72 +192,26 @@ def conf_section(self, repotype, arch=ARCH_X86_64, enabled=None, section_name=No | |||
|
|
|||
| return result | |||
|
|
|||
| # Map arch to compatible arches | |||
There was a problem hiding this comment.
so much being removed! 🤩
There was a problem hiding this comment.
Yeah repoquery command needs them otherwise it will not list all supported arches. It is not needed any more since because this PR reads repodata directly.
vfreex
added a commit
to vfreex/ocp-build-data-validator
that referenced
this pull request
Jul 14, 2023
This is to support openshift-eng/doozer#798.
vfreex
added a commit
to vfreex/ocp-build-data-validator
that referenced
this pull request
Jul 14, 2023
This is to support openshift-eng/doozer#798.
Currently we use a tag based approach to detect rpm changes in stream builds. It considers a package X is outdated if a newer version Y is tagged into any of X's tags. This approach has some limitations. Especially, it can't detect rpm changes after RHEL version moves or the image moves to a different repo (see https://issues.redhat.com/browse/ART-3965 for a real problem). I will give you an example to show how it fails to detect a change: - Image A enables a RHEL 8.4 repo. - Package `foo-1.0.0-1.el8_3.x86_64.rpm` is installed in image A. It has `rhel-8.3.0-z` tag in Brew. - A newer version `foo-1.1.0-1.el8_4.x86_64.rpm` is released and has `rhel-8.4.0-z` tag. - Doozer doesn't think `foo-1.0.0-1.el8_3.x86_64.rpm` is outdated because the newer version `foo-1.1.0-1.el8_4.x86_64.rpm` doesn't have `rhel-8.3.0-z` tag. To address this limitation, a new approach is proposed to actually query the content of all enabled repos and compare the installed versions with the latest versions in repos. I will call it "content based" approach. This content based approach has been proved in `gen-payload` command (invoked in `build-sync` job) with the merge of openshift-eng#764. If you look at [the logs of build-sync build](https://saml.buildvm.hosts.prod.psi.bos.redhat.com:8888/job/aos-cd-builds/job/build%252Fbuild-sync/34458/consoleFull), you will see it shows you which images contain outdated rpms. It contains entries like: ```yaml - code: OUTDATED_RPMS_IN_STREAM_BUILD msg: Found outdated RPM (perl-Pod-Perldoc-3.28-396.el8) installed in openshift-enterprise-builder-container-v4.14.0-202307111728.p0.gfe78ee6.assembly.stream (x86_64) when perl-Pod-Perldoc-3.28.01-443.module+el8.6.0+13324+628a2397 was available in repo rhel-8-appstream-rpms ``` However, the above example is actually a false positive. It is because package `perl-Pod-Perldoc-3.28-396.el8` comes from a modular repo but our code is not module-aware. A modular rpm repo provides multiple upgrade paths. We should only consider a newer version is upgradable if it belongs to the same stream. For more information about modules, see https://docs.fedoraproject.org/en-US/modularity/. That is a major reason why this new approach is adopted to `scan-sources`, otherwise it would constantly trigger unnecessary rebuilds! This PR not only updates `scan-sources` to use the new approach, but only adds module-awareness to eliminate false positives. The new algorithm (probably oversimplified): 1. For an image build, get all modular rpms in enabled repos. 2. For each installed rpm, check if the rpm is contained by a module. 3. If yes, we will consider that module stream is "enabled" for this image. 4. For each enabled module stream, find the module with the largest version. 5. Collect all modular rpms from those modules as collection M. 6. Collect all non-modular rpms with the latest version as collection N. If the package name of a non-modular rpm is already included in the modular rpms collection, exclude it. 7. For each installed rpm, check if it is a modular rpm or not. If it is modular, check if it is older than the one in collection M. Otherwise, check if it is older than the one in collection N. 8. If it is older, we consider is it an outdated rpm. Note: 1. There is no Brew API or any other cheap way to precisely determine which module streams are enabled during an image build. The method used in the above algorithm is actually *guess*. It is not perfect, but my tests conclude it is good enough for our use cases (probably because we don't use a lot of modular rpms in our images). 2. Some images use pins in their Dockerfiles. We should exempt them from scan-sources. The exemption can be defined in ocp-build-data image metadata. e.g. ```yaml scan_sources: exempted_packages: - openvswitch3.1 - python3-openvswitch3.1 ```
vfreex
force-pushed
the
new-outdated-rpm-detection
branch
from
ea946ae
to
17cc8b6
Compare
Build #7 |
|
@sosiouxme Thank you so much for the review! I've addressed your review suggestions in the latest commit. |
Build #8 |
sosiouxme
pushed a commit
to openshift-eng/ocp-build-data-validator
that referenced
this pull request
Jul 14, 2023
This is to support openshift-eng/doozer#798.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Currently we use a tag based approach to detect rpm changes in stream builds. It considers a package X is outdated if a newer version Y is tagged into any of X's tags. This approach has some limitations. Especially, it can't detect rpm changes after RHEL version moves or the image moves to a different repo (see https://issues.redhat.com/browse/ART-3965 for a real problem).
I will give you an example to show how it fails to detect a change:
foo-1.0.0-1.el8_3.x86_64.rpmis installed in image A. It hasrhel-8.3.0-ztag in Brew.foo-1.1.0-1.el8_4.x86_64.rpmis released and hasrhel-8.4.0-ztag.foo-1.0.0-1.el8_3.x86_64.rpmis outdated because the newer versionfoo-1.1.0-1.el8_4.x86_64.rpmdoesn't haverhel-8.3.0-ztag.To address this limitation, a new approach is proposed to actually query the content of all enabled repos and compare the installed versions with the latest versions in repos. I will call it "content based" approach.
This content based approach has been proved in
gen-payloadcommand (invoked inbuild-syncjob) with the merge of #764. If you look at the logs of build-sync build, you will see it shows you which images contain outdated rpms. It contains entries like:However, the above example is actually a false positive. It is because package
perl-Pod-Perldoc-3.28-396.el8comes from a modular repo but our code is not module-aware. A modular rpm repo provides multiple upgrade paths. We should only consider a newer version is upgradable if it belongs to the same stream. For more information about modules, see https://docs.fedoraproject.org/en-US/modularity/. That is a major reason why this new approach is adopted toscan-sources, otherwise it would constantly trigger unnecessary rebuilds!This PR not only updates
scan-sourcesto use the new approach, but only adds module-awareness to eliminate false positives. The new algorithm (probably oversimplified):Note: