compose: Add --ex-lockfile-strict #1858

jlebon · 2019-06-14T18:49:40Z

Today, lockfiles only restrict the NEVRA of specifc package names from
which libsolv can pick. But nothing stops libsolv from picking entirely
different packages which still satisfy the manifest requests.

This was mostly a theoretical issue in Fedora CoreOS, but became reality
with the addition of Fedora 32 packages in the pool. libsolv would
happily try to pick e.g. libcurl-minimal from f32 instead of sticking
with the f31 libcurl from the lockfiles:

coreos/fedora-coreos-streams#75 (comment)

(But more generally, see
coreos/fedora-coreos-tracker#454).

Let's add a --ex-lockfile-strict mode, which in CI and production
pipeline build contexts will require that (1) only locked packages are
considered by libsolv, and (2) all locked packages were marked for
install.

One important thing to note here is that we don't short-circuit libsolv
and manually hy_goal_install lockfile packages. We want to make sure
the treefile is still canonical. Strict mode simply ensures that the
result agrees with the lockfile.

That said, even in developer contexts, we don't want the
libcurl-minimal issue that happened to be triggered. But we still want
to allow flexibility in adding and removing packages to make hacking
easier. I have some follow-up patches which will enable this.

Note: part (2) above was no longer true when this PR was merged, but I forgot to fix the commit message. See #1858 (comment).

jlebon · 2019-06-14T18:54:00Z

There's possibly a cleaner way of doing this. One issue is that there's no "fail instead of installing dependencies" flag to give to libdnf. We could avoid depsolving entirely and let librpm depcheck the transaction, though we rely on the goal in a bunch of places too.

cgwalters · 2019-06-14T18:56:52Z

We talked through this when the lockfile semantics were being added, today we match e.g. cargo and I believe the rationale there is it's way more convenient for development.

Probably what we want here is really for the *COS pipelines to use cosa build --lockfile-strict?

jlebon · 2019-06-14T19:02:36Z

Right yeah, I'm open to switching the default vs flagged options around.

dustymabe · 2019-06-14T19:15:13Z

Right yeah, I'm open to switching the default vs flagged options around.

I'm 👍 for either way as the default.

rh-atomic-bot · 2019-06-17T17:21:48Z

☔ The latest upstream changes (presumably dbf28ac) made this pull request unmergeable. Please resolve the merge conflicts.

jlebon · 2020-04-14T20:52:18Z

OK, reworked this! Commit message updated.
Split out prep patches in #2057.

jlebon · 2020-04-15T13:59:24Z

Rebased! (And reworked the relaxed path to be more efficient.)

jlebon · 2020-04-15T21:45:33Z

Would appreciate some eyes on this and #2058 soon-ish!

cgwalters

Overall, this looks very good! That said, per our live discussion I am increasingly feeling that what we really want to do is reify this - we should extract something like:

rpm-ostree compose update-locked-rpmmd-repo --lockfile /path/to/lockfile.json --rpmmd-repo /path/to/repo

And we run that as a service that generates e.g. coreos-testing-devel.repo coreos-stable.repo etc.

This would make it trivial to use e.g. dnf download to get access to an RPM locked in this way, which would be generally useful.

And further then, this complex and subtle lockfile logic doesn't need to live in rpm-ostree, it could live in libdnf or some other tool/library that runs on the buildsystem side.

The huge difference between what we're doing and what Rust's cargo and other tools like that are doing is that we actually want to enforce a lot of uniformity in what we're locking, we don't expect many other consumers.

cgwalters · 2020-04-15T22:41:41Z

src/libpriv/rpmostree-core.c

+  g_autoptr(GHashTable) goal_nevras =
+    g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
+  for (guint i = 0; i < pkgs->len; i++)
+    g_hash_table_add (goal_nevras, g_strdup (dnf_package_get_nevra (pkgs->pdata[i])));


(If we were writing this in Rust we could be confident to avoid a strdup because we could guarantee the lifetime of the hashset with nevra is <= lifetime of the pkgs list)

cgwalters · 2020-04-16T13:07:18Z

src/libpriv/rpmostree-core.c

+                return NULL;
+
+              if (g_str_equal (chksum, repodata_chksum))
+                g_ptr_array_add (pkgs, g_object_ref (match));


How about something like:
n_checksum_mismatches++;
and then below:

cgwalters · 2020-04-16T13:08:44Z

src/libpriv/rpmostree-core.c

+                g_ptr_array_add (pkgs, g_object_ref (match));
+            }
+        }
+      if (old_pkgs_len == pkgs->len)


I think an if (matched) would be a bit clearer here.

cgwalters · 2020-04-16T13:09:08Z

src/libpriv/rpmostree-core.c

+            }
+        }
+      if (old_pkgs_len == pkgs->len)
+        return glnx_null_throw (error, "Couldn't find locked package '%s'%s%s",


(%d non-matching packages), n_checksum_mismatches or so?

cgwalters · 2020-04-16T13:12:44Z

src/libpriv/rpmostree-core.c

-            }
+          Map *map = dnf_packageset_get_map (pset);
+          map_setall (map);
+          map_subtract (map, dnf_packageset_get_map (locked_pset));
          dnf_sack_add_excludes (sack, pset);


This is fine as is, but it'd be clearer if there was an API like dnf_sack_include_only(locked_pset).

Today, lockfiles only restrict the NEVRA of specifc package names from which libsolv can pick. But nothing stops libsolv from picking entirely different packages which still satisfy the manifest requests. This was mostly a theoretical issue in Fedora CoreOS, but became reality with the addition of Fedora 32 packages in the pool. libsolv would happily try to pick e.g. `libcurl-minimal` from f32 instead of sticking with the f31 `libcurl` from the lockfiles: coreos/fedora-coreos-streams#75 (comment) (But more generally, see coreos/fedora-coreos-tracker#454). Let's add a `--ex-lockfile-strict` mode, which in CI and production pipeline build contexts will require that (1) *only* locked packages are considered by libsolv, and (2) *all* locked packages were marked for install. One important thing to note here is that we don't short-circuit libsolv and manually `hy_goal_install` lockfile packages. We want to make sure the treefile is still canonical. Strict mode simply ensures that the result agrees with the lockfile. That said, even in developer contexts, we don't want the `libcurl-minimal` issue that happened to be triggered. But we still want to allow flexibility in adding and removing packages to make hacking easier. I have some follow-up patches which will enable this.

jlebon · 2020-04-17T03:04:31Z

Updated for comments! I also ended up dropping check_locked_pkgs entirely after more thinking. Basically, having some packages in the lockfile but not in the transaction is now acceptable in strict mode too. This can happen if we add an override which pulls in less deps than the overridden package. That said of course, we still ensure that all packages we do pull in are locked, which is the most important part.

jlebon · 2020-04-17T12:10:43Z

/test sanity

jlebon · 2020-04-17T16:24:00Z

With nuking check_locked_pkgs we also drop the check that there are only DNF_PACKAGE_INFO_INSTALL goals, but note that this patch instead introduces an assertion check here: https://github.com/coreos/rpm-ostree/pull/1858/files#diff-07963b1a5922690400970963652cbe3bR1961-R1968.

cgwalters · 2020-04-17T19:11:59Z

/test sanity
Should be fixed now

cgwalters · 2020-04-17T19:13:23Z

/lgtm

openshift-ci-robot · 2020-04-17T19:13:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,jlebon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Minor follow-up to coreos#1858. Make the help string here more helpful and accurate.

Minor follow-up to #1858. Make the help string here more helpful and accurate.

jlebon mentioned this pull request Jun 14, 2019

Add support for reading and writing lockfiles coreos/coreos-assembler#553

Merged

jlebon added the WIP label Jun 14, 2019

jlebon mentioned this pull request Sep 5, 2019

manifest: add libvarlink-util coreos/fedora-coreos-config#154

Merged

jlebon mentioned this pull request Nov 26, 2019

[testing] Add coreos-keep-cgroup-v1.service coreos/fedora-coreos-config#238

Merged

jlebon mentioned this pull request Apr 8, 2020

Use strict lockfile semantics for FCOS coreos/fedora-coreos-tracker#454

Closed

jlebon force-pushed the pr/strict-lockfile branch from d1dccb0 to b2b82b1 Compare April 14, 2020 20:49

openshift-ci-robot added the approved label Apr 14, 2020

jlebon changed the title ~~WIP: compose: Add --ex-lockfile-relaxed and default to strict~~ compose: Add --ex-lockfile-strict Apr 14, 2020

jlebon removed the WIP label Apr 14, 2020

jlebon mentioned this pull request Apr 14, 2020

manifest: Add lockfile-repos field #2058

Merged

jlebon force-pushed the pr/strict-lockfile branch from b2b82b1 to 7dd5352 Compare April 15, 2020 13:58

jlebon mentioned this pull request Apr 15, 2020

[next-devel] lockfiles: remove for now coreos/fedora-coreos-config#351

Merged

cgwalters approved these changes Apr 16, 2020

View reviewed changes

jlebon force-pushed the pr/strict-lockfile branch from 7dd5352 to 05d19ab Compare April 17, 2020 03:04

jlebon mentioned this pull request Apr 17, 2020

testiso: Don't run iso-live-login scenario by default coreos/coreos-assembler#1365

Merged

openshift-ci-robot assigned cgwalters Apr 17, 2020

openshift-ci-robot added the lgtm label Apr 17, 2020

openshift-merge-robot merged commit 5345673 into coreos:master Apr 17, 2020

This was referenced Apr 17, 2020

cmd-build: add --strict for building in strict mode coreos/coreos-assembler#1370

Merged

[testing] tree: promote changes from testing-devel at a8c84a3e055500ce8119535240f17e9e78fb1f4e coreos/fedora-coreos-config#358

Merged

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Apr 20, 2020

app/compose: Fix help string for --ex-lockfile-strict

7688474

Minor follow-up to coreos#1858. Make the help string here more helpful and accurate.

jlebon mentioned this pull request Apr 20, 2020

app/compose: Fix help string for --ex-lockfile-strict #2064

Merged

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Apr 20, 2020

app/compose: Fix help string for --ex-lockfile-strict

e7d365b

Minor follow-up to coreos#1858. Make the help string here more helpful and accurate.

openshift-merge-robot pushed a commit that referenced this pull request Apr 20, 2020

app/compose: Fix help string for --ex-lockfile-strict

682aad2

Minor follow-up to #1858. Make the help string here more helpful and accurate.

cgwalters mentioned this pull request May 5, 2020

Add overrides for multi-arch coreos/fedora-coreos-config#374

Merged

jlebon mentioned this pull request Dec 7, 2020

Change default cosa --strict parameter coreos/coreos-ci-lib#58

Merged

jlebon deleted the pr/strict-lockfile branch April 23, 2023 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose: Add --ex-lockfile-strict #1858

compose: Add --ex-lockfile-strict #1858

jlebon commented Jun 14, 2019 •

edited

Loading

jlebon commented Jun 14, 2019

cgwalters commented Jun 14, 2019 •

edited

Loading

jlebon commented Jun 14, 2019

dustymabe commented Jun 14, 2019

rh-atomic-bot commented Jun 17, 2019

jlebon commented Apr 14, 2020

jlebon commented Apr 15, 2020

jlebon commented Apr 15, 2020

cgwalters left a comment

cgwalters Apr 15, 2020

cgwalters Apr 16, 2020

cgwalters Apr 16, 2020

cgwalters Apr 16, 2020

cgwalters Apr 16, 2020

jlebon commented Apr 17, 2020

jlebon commented Apr 17, 2020

jlebon commented Apr 17, 2020

cgwalters commented Apr 17, 2020

cgwalters commented Apr 17, 2020

openshift-ci-robot commented Apr 17, 2020

compose: Add --ex-lockfile-strict #1858

compose: Add --ex-lockfile-strict #1858

Conversation

jlebon commented Jun 14, 2019 • edited Loading

jlebon commented Jun 14, 2019

cgwalters commented Jun 14, 2019 • edited Loading

jlebon commented Jun 14, 2019

dustymabe commented Jun 14, 2019

rh-atomic-bot commented Jun 17, 2019

jlebon commented Apr 14, 2020

jlebon commented Apr 15, 2020

jlebon commented Apr 15, 2020

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Apr 15, 2020

Choose a reason for hiding this comment

cgwalters Apr 16, 2020

Choose a reason for hiding this comment

cgwalters Apr 16, 2020

Choose a reason for hiding this comment

cgwalters Apr 16, 2020

Choose a reason for hiding this comment

cgwalters Apr 16, 2020

Choose a reason for hiding this comment

jlebon commented Apr 17, 2020

jlebon commented Apr 17, 2020

jlebon commented Apr 17, 2020

cgwalters commented Apr 17, 2020

cgwalters commented Apr 17, 2020

openshift-ci-robot commented Apr 17, 2020

jlebon commented Jun 14, 2019 •

edited

Loading

cgwalters commented Jun 14, 2019 •

edited

Loading