depsolving: mark packages as `user` #28

supakeen · 2023-07-14T09:42:52Z

When DNF installs packages it marks the reason a package was installed. The available reasons are:

user: directly requested by the user (e.g: dnf install vim, marks vim as user and all dependencies as dependency).
group: top packages in a group directly requested by the user (e.g: dnf install @core marks vim, foo, and bar as group).

There's also dependency and weak-dependency which are more self-explanatory.

This PR introduces this concept to dnf-json so we can propagate the information to osbuild which can then mark packages with the correct reasons after installing them as rpms.

Not marking packages correctly can lead to issues; the default behavior by dnf is to have packages unknown to it be marked as dependency. This means that when the user installs a package on one of our images (marked as user) then removes that package that dependencies of that package might be cleaned up as there are no leaf nodes with user or group anymore; alternatively packages might not be removed when they should.

The elephant in the room are group markings. When a package is marked as group it also marks for which group. This information is currently unavailable in both dnf and dnf5. I've got an open report; currently I've chosen to mark packages that are group as user. This isn't ideal as one can't do a dnf remove @core and have it remove that comps packages however this will let us put this in end-to-end. I've written this PR in such a way that if/when we have a way to get this information only dnf-json needs to be adjusted.

A fun complication is that a package can be marked as dependency in the first resolve of a transaction chain; however it can then be directly selected (user) in the second transaction chain. This conditional reason-upgrade is allowed (and should also be applied to group). Similarly, we also allow upgrading weak-dependency to dependency.

supakeen · 2023-07-14T09:59:47Z

In the current state, this would add a stage like so for (for example) fedora-39 with vmdk image type, note that @groups are missing and replaced with user for now.

            "type": "org.osbuild.dnf.mark",
            "options": {
              "packages": [
                {
                  "name": "ModemManager-glib",
                  "mark": "dependency"
                },
                {
                  "name": "NetworkManager",
                  "mark": "user"
                },
                {
                  "name": "NetworkManager-libnm",
                  "mark": "dependency"
                },
                {
                  "name": "alternatives",
                  "mark": "dependency"
                },
                {
                  "name": "amd-gpu-firmware",
                  "mark": "weak-dependency"
                },
                {
                  "name": "atheros-firmware",
                  "mark": "weak-dependency"
                },
                {
                  "name": "audit",
                  "mark": "user"
                },
                {
                  "name": "audit-libs",
                  "mark": "dependency"
                },
                {
                  "name": "authselect",
                  "mark": "dependency"
                },
                {
                  "name": "authselect-libs",
                  "mark": "dependency"
                },
                {
                  "name": "basesystem",
                  "mark": "user"
                },

supakeen · 2023-07-17T07:36:11Z

This PR has been updated to pull the information from libdnf in dnf-json and to then propagate it down into rpmmd which then ends up in the os manifest.

Do we want to drop the mark dependency? It's the default but I do like to be explicit. See the issue for a linked bugreport as for why this won't do group-marking correctly yet.

supakeen · 2023-07-20T16:56:11Z

This PR has been updated to work around problems with chain-depsolving marking packages as user installed when selected from the previous transaction.

supakeen · 2023-07-20T17:04:47Z

I've marked this PR ready for review as I'd really like some input. This will initially generate a dnf marking stage for the disk image types (which all use liveImage) for Fedora rawhide.

Note that this PR cannot be merged before the actual org.osbuild.dnf.mark stage has been merged into osbuild: osbuild/osbuild#1333 which is the next step but I'd quite like some input on the approach taken.

supakeen · 2023-07-20T21:17:15Z

I realized that I forgot a thing. A package can be conditionally upgraded to user-selected if it was selected directly in a newer transaction. Added some code for that.

group-selected can also upgrade package reasons; but we don't have that information yet.

teg · 2023-08-11T08:58:07Z

Love it!

ondrejbudai · 2023-08-11T09:35:41Z

pkg/osbuild/dnf_mark_stage.go

+type DNFMarkStagePackageOptions struct {
+	Name  string `json:"name"`
+	Mark  string `json:"mark"`
+	Group string `json:"group,omitempty"`


I think that the latest version of the osbuild PR doesn't have group in the stage options.

ondrejbudai · 2023-08-11T09:38:58Z

pkg/runner/fedora.go

@@ -15,5 +15,6 @@ func (p *Fedora) GetBuildPackages() []string {
 		"glibc",   // ldconfig
 		"systemd", // systemd-tmpfiles and systemd-sysusers
 		"python3", // osbuild
+		"dnf",     // temporary


temporary? What does this mean?

Ah yes, good that this was caught. I am wondering if it's correct to add dnf to the build packages here.

@achilleas-k wdyt?

If we're going to be marking packages for all fedora builds, then we'll need it in the build root unconditiaonally, yes.

supakeen · 2023-08-21T06:49:23Z

PR rebased and updated both concerns addressed and the comment changed to clarify that the package is added to the build root for marking.

Is it possible to have a stage conditionally add something to the build root?

thozza · 2023-08-21T08:58:18Z

Is it possible to have a stage conditionally add something to the build root?

Not stage, but the pipeline implementation can do it based on its configuration which also affects if a stage is added to the pipeline or not.

See https://github.com/osbuild/images/blob/main/pkg/manifest/os.go#L262

achilleas-k · 2023-08-21T18:27:18Z

Is it possible to have a stage conditionally add something to the build root?

Not stage, but the pipeline implementation can do it based on its configuration which also affects if a stage is added to the pipeline or not.

See https://github.com/osbuild/images/blob/main/pkg/manifest/os.go#L262

Since we have the option set on the os, and the stage isn't added for all versions, best to add the package to the build root conditionally like we do for everything else.

supakeen · 2023-08-22T08:40:50Z

Nice; then I've moved DNF from the runner into the getBuildPackages for the os pipeline :)

thozza

Overall nice work. I added a few inline comments. Most notably, I don't think that putting the option into DiskImage is the right place. Instead, it should be part of the ImageConfig which then translates to OSCustomizations... Distro / image type - specific ImageConfig can be then used in Fedora to enable this feature where needed.

pkg/manifest/os.go

thozza · 2023-08-22T08:49:48Z

pkg/manifest/os.go

@@ -157,6 +157,8 @@ type OS struct {
 	containerSpecs   []container.Spec
 	ostreeParentSpec *ostree.CommitSpec

+	MarkPackages bool


Could you please elaborate on the reasoning behind putting the option into the pipeline options, instead of putting it into OSCustomizations?

thozza · 2023-08-22T08:55:28Z

pkg/image/disk.go

@@ -25,6 +25,7 @@ type DiskImage struct {
 	Workload         workload.Workload
 	Filename         string
 	Compression      string
+	MarkPackages     bool


I would argue that marking packages is not really specific to DiskImage, since we may want to do this for any other type of images. It is IMO specific to the OS pipeline, which builds the base OS tree, regardless of the artifact format. Making the option specific to the OS pipeline (by putting it to OSCustomizations), removes the need to pass this option in each image type implementation which uses the OS pipeline. We also already have functions, which transform the ImageConfig to OSCustomizations, which would make it easy to make this image type / distro (version) specific thing...

I've moved this into ImageConfig and then set it to true by default in fedora/images.go.

thozza · 2023-08-22T08:56:32Z

pkg/osbuild/dnf_mark_stage.go

@@ -0,0 +1,64 @@
+package osbuild


nitpick: Any reason to not put the 4 into the file name as well? 🤔

Oh, I wanted to put both the dnf4 and dnf5 stage in dnf_mark_stage.go. I'll rename it to be specific since it only has 4 for now.

thozza · 2023-08-22T08:58:47Z

pkg/distro/fedora/images.go

@@ -252,6 +252,10 @@ func diskImage(workload workload.Workload,

 	img.Filename = t.Filename()

+	distro := t.Arch().Distro()
+
+	img.MarkPackages = true


This IMO scatters the image configuration into multiple places, while in reality it should be IMO part of the default distro ImageConfig or image type specific ImageConfig.

In addition, package marking is enabled for all Fedora version AFAICT, which contradicts the commit message which states that it should be enabled only for rawhide.

Updated the commit message; I'm going to move all of this into ImageConfig(s).

thozza · 2023-08-24T15:05:34Z

pkg/distro/fedora/images.go

+	if !t.rpmOstree {
+		osc.MarkPackages = true
+	}


I think that the correct way would be to unconditionally set osc.MarkPackages = imageConfig.MarkPackages.

Then set to true in

images/pkg/distro/fedora/distro.go

Lines 404 to 407 in d9e892f

var defaultDistroImageConfig = &distro.ImageConfig{

Timezone: common.ToPtr("UTC"),

Locale: common.ToPtr("en_US"),

}

and overridden for edge images. There may be probably a more elegant way, but the thing is that now, the value can't be actually configured via ImageConfig.

In addition, it may make sense to extend the osCustomizations() in each distro even if not used there, to prevent any surprises in the future (since ImageConfig and OS pipeline implementation are used globally).

Add the dnf marking stage that can be used to mark packages in the dnf state database.

Package marking is enabled per image type and propagated down to the OS pipeline. The package marking information is output by `dnf-json` based on what `dnf` gave as reasons during the depsolve transactions. This is stored on the `rpmmd.PackageSpec`. Note that we currently output 'user' for any 'group'-installed package. There's also a bit of complication here; as `dnf-json` does chain depsolving it marks all packages from the previous transaction for `install`. This leads to all packages from the previous transaction being marked as `user`-installed. To circumvent this we keep track of the reasons a package was selected and the oldest one wins. Another complication is that a package could be initially resolved as a dependency but be user-requested in a follow up transaction. Some logic is involved to allow upgrades from dependency and weak-dependency to user. We also blanket-allow upgrading weak-dependency to dependency. Package marking is enabled for all Fedora image types.

github-actions · 2023-10-01T04:00:37Z

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

github-actions · 2023-11-01T04:00:40Z

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

github-actions · 2023-11-09T04:00:41Z

This PR was closed because it has been stalled for 30+7 days with no activity.

supakeen linked an issue Jul 14, 2023 that may be closed by this pull request

osbuild does not produce images with populated dnf state database osbuild/osbuild#455

Open

supakeen force-pushed the dnf-marking branch 2 times, most recently from f5f4fb0 to de60b1a Compare July 14, 2023 09:56

supakeen force-pushed the dnf-marking branch 2 times, most recently from 84b6e90 to 451d44b Compare July 14, 2023 12:28

supakeen mentioned this pull request Jul 14, 2023

osbuild does not produce images with populated dnf state database osbuild/osbuild#455

Open

supakeen force-pushed the dnf-marking branch from 451d44b to 2dd1fcb Compare July 17, 2023 07:35

supakeen force-pushed the dnf-marking branch 11 times, most recently from 405a598 to c192748 Compare July 20, 2023 16:55

supakeen marked this pull request as ready for review July 20, 2023 16:56

supakeen force-pushed the dnf-marking branch 2 times, most recently from 64a5f81 to 982b708 Compare July 20, 2023 17:03

supakeen requested review from achilleas-k and thozza July 20, 2023 17:04

supakeen force-pushed the dnf-marking branch from 982b708 to 6c7a471 Compare July 20, 2023 21:15

supakeen force-pushed the dnf-marking branch from 6c7a471 to ff63d23 Compare July 21, 2023 09:07

ondrejbudai reviewed Aug 11, 2023

View reviewed changes

supakeen force-pushed the dnf-marking branch from c07bb66 to c512d27 Compare August 11, 2023 13:19

supakeen force-pushed the dnf-marking branch from c512d27 to 4f07b58 Compare August 21, 2023 06:48

supakeen force-pushed the dnf-marking branch from 1c05b0d to 707be61 Compare August 21, 2023 11:13

supakeen marked this pull request as ready for review August 21, 2023 13:34

supakeen force-pushed the dnf-marking branch from 707be61 to 13c4b4d Compare August 22, 2023 08:40

supakeen force-pushed the dnf-marking branch from 13c4b4d to 11c67a5 Compare August 22, 2023 08:42

thozza reviewed Aug 22, 2023

View reviewed changes

supakeen force-pushed the dnf-marking branch 5 times, most recently from 43163ab to ec6c5cf Compare August 24, 2023 09:47

thozza reviewed Aug 24, 2023

View reviewed changes

supakeen added 3 commits August 31, 2023 11:44

chore: manifest/os use getPackageSpecs

4162179

osbuild: dnf_mark stage

1c24f39

Add the dnf marking stage that can be used to mark packages in the dnf state database.

supakeen force-pushed the dnf-marking branch from ec6c5cf to 36acf86 Compare August 31, 2023 09:44

github-actions bot added the Stale label Oct 1, 2023

supakeen removed the Stale label Oct 1, 2023

github-actions bot added the Stale label Nov 1, 2023

github-actions bot closed this Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

depsolving: mark packages as `user` #28

depsolving: mark packages as `user` #28

supakeen commented Jul 14, 2023 •

edited

Loading

supakeen commented Jul 14, 2023 •

edited

Loading

supakeen commented Jul 17, 2023 •

edited

Loading

supakeen commented Jul 20, 2023

supakeen commented Jul 20, 2023 •

edited

Loading

supakeen commented Jul 20, 2023

teg commented Aug 11, 2023

ondrejbudai Aug 11, 2023

ondrejbudai Aug 11, 2023

supakeen Aug 11, 2023

ondrejbudai Aug 14, 2023

achilleas-k Aug 14, 2023 •

edited

Loading

supakeen commented Aug 21, 2023

thozza commented Aug 21, 2023

achilleas-k commented Aug 21, 2023

supakeen commented Aug 22, 2023

thozza left a comment

thozza Aug 22, 2023

thozza Aug 22, 2023

supakeen Aug 24, 2023

thozza Aug 22, 2023

supakeen Aug 24, 2023

thozza Aug 22, 2023

thozza Aug 22, 2023

supakeen Aug 23, 2023

thozza Aug 24, 2023

github-actions bot commented Oct 1, 2023

github-actions bot commented Nov 1, 2023

github-actions bot commented Nov 9, 2023

	var defaultDistroImageConfig = &distro.ImageConfig{
	Timezone: common.ToPtr("UTC"),
	Locale: common.ToPtr("en_US"),
	}

depsolving: mark packages as user #28

depsolving: mark packages as user #28

Conversation

supakeen commented Jul 14, 2023 • edited Loading

supakeen commented Jul 14, 2023 • edited Loading

supakeen commented Jul 17, 2023 • edited Loading

supakeen commented Jul 20, 2023

supakeen commented Jul 20, 2023 • edited Loading

supakeen commented Jul 20, 2023

teg commented Aug 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achilleas-k Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

supakeen commented Aug 21, 2023

thozza commented Aug 21, 2023

achilleas-k commented Aug 21, 2023

supakeen commented Aug 22, 2023

thozza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 1, 2023

github-actions bot commented Nov 1, 2023

github-actions bot commented Nov 9, 2023

depsolving: mark packages as `user` #28

depsolving: mark packages as `user` #28

supakeen commented Jul 14, 2023 •

edited

Loading

supakeen commented Jul 14, 2023 •

edited

Loading

supakeen commented Jul 17, 2023 •

edited

Loading

supakeen commented Jul 20, 2023 •

edited

Loading

achilleas-k Aug 14, 2023 •

edited

Loading