Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update all - check for upgradable packages #644

Merged
merged 20 commits into from
Jul 3, 2024

Conversation

ashnamehrotra
Copy link
Contributor

@ashnamehrotra ashnamehrotra commented May 30, 2024

Adds a check for upgradable packages before updating all when patching without scanner input.

Wanted feedback for microdnf check - discussed with @cpuguy83 to install dnf to call check-update option since microdnf does not support it, but the only way to remove it after is with rpm -e dnf.

Describe the changes in this pull request using active verbs such as Add, Remove, Replace ...

Closes #594

Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 2.94118% with 33 lines in your changes missing coverage. Please review.

Project coverage is 33.01%. Comparing base (2602d59) to head (60649af).
Report is 94 commits behind head on main.

Files Patch % Lines
pkg/pkgmgr/rpm.go 0.00% 24 Missing ⚠️
pkg/pkgmgr/dpkg.go 0.00% 6 Missing ⚠️
pkg/pkgmgr/apk.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #644      +/-   ##
==========================================
+ Coverage   32.51%   33.01%   +0.50%     
==========================================
  Files          17       18       +1     
  Lines        1621     1578      -43     
==========================================
- Hits          527      521       -6     
+ Misses       1062     1024      -38     
- Partials       32       33       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ashnamehrotra ashnamehrotra marked this pull request as ready for review June 3, 2024 22:21
@cpuguy83
Copy link
Contributor

cpuguy83 commented Jun 3, 2024

For dnf based distros, have you tried using the tooling image and mounting the rootfs of the image you want to check?

Rough example:

llb.Image(myToolingImage).Run(
  llb.AddMount(rootfsToCheck, "/tmp/rootfs"),
  llb.Args([]string{"sh", "-c", "dnf --installroot=/tmp/rootfs check-update"})

@cpuguy83
Copy link
Contributor

cpuguy83 commented Jun 3, 2024

I say dnf, any distro that uses rpm+yum/dnf style repos and configs.

@ashnamehrotra
Copy link
Contributor Author

ashnamehrotra commented Jun 12, 2024

For dnf based distros, have you tried using the tooling image and mounting the rootfs of the image you want to check?

Rough example:

llb.Image(myToolingImage).Run(
  llb.AddMount(rootfsToCheck, "/tmp/rootfs"),
  llb.Args([]string{"sh", "-c", "dnf --installroot=/tmp/rootfs check-update"})

I made the changes to get this to work and was able to test it with yum based images. However, for microdnf based images (like quay.io/calico/cni:v3.15.1) it will always detect no upgrade when using dnf, even if the image is unpatched and would detect upgrades when running microdnf install dnf; dnf check-update in the container.

@cpuguy83
Copy link
Contributor

That is interesting and I wonder if that is related to this:

  • configuration file and reposdir are searched inside the
         installroot first. If they are not present, they are taken from
         the host system.  Note:  When a path is specified within a
         command line argument (--config=<config file> in case of
         configuration file and --setopt=reposdir=<reposdir> for
         reposdir) then this path is always relative to the host with no
         exceptions.

ref: https://www.man7.org/linux/man-pages/man8/dnf.8.html

It could be falling back to the tooling image's repo config and/or dnf config.
We should probably make sure to set those properly so they don't fallback that way.

@cpuguy83
Copy link
Contributor

We investigated this a bit yesterday and it looks like --installroot is just broken with this case.
It loads the correct repos but does do the right thing at all and just always says that there are no updates.
I'm sure this could probably be fixed upstream in dnf/tdnf but that will probably take a long time.

The work-around we discussed is to solve an intermediate LLB state with dnf installed in the target image and run it like normal and write some file to the rootfs that we'll extract to determine if there are updates.

Signed-off-by: ashnamehrotra <[email protected]>
Signed-off-by: ashnamehrotra <[email protected]>
Signed-off-by: ashnamehrotra <[email protected]>
pkg/pkgmgr/rpm.go Outdated Show resolved Hide resolved
@ashnamehrotra
Copy link
Contributor Author

@sozercan in order to end patching workflow, we need to return an error when there are no upgradable packages (similar to with scanner https://github.com/project-copacetic/copacetic/blob/main/pkg/pkgmgr/pkgmgr.go#L51-L54). Do we want to change this in all places?

Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@ashnamehrotra ashnamehrotra merged commit eb7cdcd into project-copacetic:main Jul 3, 2024
14 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Copa should also check if there are any package updates. If not, it should fast fail
3 participants