compose: Add --ex-lockfile and --ex-write-lockfile-to #1745

r4f4 · 2019-01-29T22:08:03Z

This is a skeleton of the solution for issue #1670.

The writing the lockfile part works but reading hasn't been tested yet.
I'm sending this PR so that we can start a discussion on my approach and I'll change things accordingly.

What's known to be missing:

Using the repodata checksum when reading the lock file
Make write-lockfile behave like coreos-assembler fetch. Right now the file is generated during the compose process.
How to properly pass the pkg NEVRAs to the depsolver.
Relevant tests: might involve creating a cached repo with fake/empty pkgs to make it deterministic.

rh-atomic-bot · 2019-01-29T22:13:52Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

r4f4 · 2019-02-01T22:47:50Z

Ok, so after some fixups, here is the current state. I generated a version lockfile a week ago.
Today I run a compose tree using that lock file. This is the output (with a print I added to src/libpriv/rpmostree-core.c:1926):

# ./rpm-ostree compose tree --workdir /tmp/workdir-2/ --repo /tmp/buildrepo --write-lockfile /tmp/version.lock --lockfile versions_lock.yaml /tmp/composedata/fedora-base.json
RPM-OSTree Version: 2019.1
No previous commit for fedora/stable/x86_64/base
Enabled rpm-md repositories: updates fedora
Updating metadata for 'updates'... done
rpm-md repo 'updates'; generated: 2019-02-01T01:33:06Z
Updating metadata for 'fedora'... done
rpm-md repo 'fedora'; generated: 2018-04-25T04:27:32Z
Importing rpm-md... done
Adding chrony-3.4-1.fc28.x86_64 to the goal
Adding container-selinux-2:2.79-1.git6f01752.fc28.noarch to the goal
Adding docker-2:1.13.1-61.git9cb56fd.fc28.x86_64 to the goal
Adding efibootmgr-16-2.fc28.x86_64 to the goal
Adding fedora-release-atomichost-28-6.noarch to the goal
Adding grub2 to the goal
Adding grub2-efi to the goal
Adding iputils-20161105-9.fc28.x86_64 to the goal
Adding kernel-4.19.16-200.fc28.x86_64 to the goal
Adding nss-altfiles-2.18.1-10.fc27.x86_64 to the goal
Adding ostree-2018.9-1.fc28.x86_64 to the goal
Adding ostree-grub2-2018.9-1.fc28.x86_64 to the goal
Adding selinux-policy-targeted-3.14.1-50.fc28.noarch to the goal
Adding shim to the goal
Adding systemd-238-10.git438ac26.fc28.x86_64 to the goal
Adding tuned-2.10.0-4.fc28.noarch to the goal
error: Packages not found: docker-2:1.13.1-61.git9cb56fd.fc28.x86_64, kernel-4.19.16-200.fc28.x86_64

# dnf repoquery docker
Last metadata expiration check: 0:39:58 ago on Fri Feb  1 22:04:03 2019.
docker-2:1.13.1-62.git9cb56fd.fc29.x86_64
docker-2:1.13.1-63.git1185cfd.fc29.x86_64

So it's trying to install a version that's not available anymore in the repos. Which is a good sign here, showing that the lockfile is working :)

rh-atomic-bot · 2019-04-15T13:57:43Z

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

r4f4 · 2019-05-21T12:43:03Z

Rebased on top of master, fixed a memory leak, fixed a double unref and squashed commits.
What would you guys like to see here before an initial review is made?

rust/src/lockfile.rs

cgwalters · 2019-05-29T13:41:29Z

src/libpriv/rpmostree-core.c

@@ -1954,7 +1956,7 @@ rpmostree_context_prepare (RpmOstreeContext *self,
  g_autoptr(GPtrArray) missing_pkgs = NULL;
  for (char **it = pkgnames; it && *it; it++)
    {
-      const char *pkgname = *it;
+      const char *pkgname = g_hash_table_lookup (self->vlockmap, *it) ?: *it;


Heh, this part is surprisingly simple.

jlebon

Regarding the lockfile spec itself, I'm not sure if it's worth breaking it down per-repo. (In fact, we're not actually verifying this part when reading it in anyway, right?). In pipeline contexts and even pungi, it's not uncommon to have custom/temporary repos for the build. It also makes it harder when overriding lockfile items, which is something we'll need for FCOS.

rust/src/lockfile.rs

src/app/rpmostree-compose-builtin-tree.c

rust/src/lockfile.rs

src/app/rpmostree-composeutil.c

src/libpriv/rpmostree-core.c

r4f4 · 2019-05-30T21:06:37Z

Update PR with the suggestions. Dropping WIP.

src/app/rpmostree-compose-builtin-tree.c

src/app/rpmostree-composeutil.c

jlebon · 2019-05-31T13:45:41Z

src/app/rpmostree-composeutil.c

+      return NULL;
+    }
+
+  g_autoptr(GVariant) value = g_variant_lookup_value (jsonmetav, "packages", G_VARIANT_TYPE ("av"));


a(ss)? That should simplify the loop below.

I tried, it doesn't work. The parsed json is like this

Read: {'packages': <[<[<'acl-2.2.53-2.fc29.x86_64'>, <'sha256:0a7446de7de962fbbfdb204da0a7aef70cc8bd4ee4b2c03b0d730bf62b9a2223'>]>, <[<'atomic-registries-1.22.1-27.gitb507039.fc29.x86_64'>, <'sha256:19fdd5bcb79bc84c4c3d17d63d2ee39173d3d2504ea11ed9f0b7d0d0cf86550c'>]>, <[<'audit-libs-3.0-0.7.20190326git03e7489.fc29.x86_64'>, <'sha256:a907544ffac01209ce49026a0ebedef58d62639aa9d234537577c72d458cc850'>]>, <[<'basesystem-11-6.fc29.noarch'>, <'sha256:e4b86fe4f119873e8705dae47163f4afb6ccf8885426e04d960ff18eed7320bb'>]>, <[<'bash-4.4.23-6.fc29.x86_64'>, <'sha256:1899be2cb4a4793d6a6c62285a6b77f5c22eb5534e76d94d2a5d421cf2aa59ad'>]>,

as you can see, everything is wrapped inside a GVariant. Unless I'm missing something.

Ahh hmm, yeah I see what you mean. Seems like that's just how json_gvariant_deserialize works. Which makes sense, since it can't know that the JSON is following some particular scheme, so it just wraps all leaf nodes in the JSON into a GVariant. Yuck. So, I do think that parsing with serde would make this much nicer and give us scheme checking as well, but happy to stick with this for now!

tests/compose-tests/test-lockfile-write.sh

jlebon · 2019-05-31T14:41:42Z

src/libpriv/rpmostree-rpm-util.c

+{
+  /* The manifest might specify not only package names (foo-1.x) but also
+   * something-that-foo-provides */
+  g_autoptr(GPtrArray) alts = rpmostree_get_matching_packages (sack, name);


Hmm, I know I suggested this, but I'm not completely satisfied either with how "indirect" it all seems. One root issue here of course is #731; libsolv simply doesn't make it easy to provide resolution information. (It's not entirely its fault either; depsolving is a hard problem that isn't exactly disposed to provide clear cut answers to these sorts of questions).

Anyway, good to roll with this for now! Though it might need more thinking and tweaks in the future.

Just couldn't help thinking about this a bit more :)

ISTM like in the lockfile case, maybe we do want to require manifests to not use "provides" but instead purely pkgnames. So e.g. when using --lockfile or --write-lockfile-to, we would only search by pkgname rather than the default dnf_context_install() fuzzy logic and error out if the pkgname isn't found. That would really simplify the semantics of the lockfile.

But yeah, I'm happy getting this in as is for now, and then tweak it in follow-ups!

The tiny downside of course is that it requires slightly more churn on treefile writers to react to things like Obsoletes, package splits, etc... Doesn't seem like an unreasonable burden though.

Follow-up to this in #1849.

r4f4 · 2019-05-31T20:22:46Z

Updated with requested changes.

jlebon

Awesome, this looks good to me now as a first cut! Final tidbit, can you squash the two commits together and change the commit message title to something like: compose: Add --lockfile and --write-lockfile-to?

jlebon · 2019-06-03T18:12:18Z

src/app/rpmostree-composeutil.c

+      return NULL;
+    }
+
+  g_autoptr(GVariant) value = g_variant_lookup_value (jsonmetav, "packages", G_VARIANT_TYPE ("av"));


Ahh hmm, yeah I see what you mean. Seems like that's just how json_gvariant_deserialize works. Which makes sense, since it can't know that the JSON is following some particular scheme, so it just wraps all leaf nodes in the JSON into a GVariant. Yuck. So, I do think that parsing with serde would make this much nicer and give us scheme checking as well, but happy to stick with this for now!

cgwalters · 2019-06-03T18:50:56Z

So, I do think that parsing with serde would make this much nicer and give us scheme checking as well, but happy to stick with this for now!

Heh, we're giving him conflicting advice. I feel like what we really need to do is get over the hump and try to have Rust driving things and that would naturally lead to things like this. Possibly even to the point of having the CLI entrypoint be Rust.

jlebon · 2019-06-03T19:17:54Z

Heh, we're giving him conflicting advice.

Yeah, all I mean is that rather than reserializing and deserializing to pass to the C side as the first approach did, we could just pass e.g. two GStrvs (one for pkgnames and another for checksums). It'd be cleaner than mucking with JSON-GLib and give us type checking as well. That said, as I mentioned earlier, I'm totally cool with how it is right now too and doing this later on. 🕙

Possibly even to the point of having the CLI entrypoint be Rust.

Hehe, I was actually toying with this when I started hacking on ex history; trying to make the whole invocation essentially stay on the Rust side. Rewriting the CLI isn't the most fun thing to do though.

jlebon · 2019-06-03T20:48:07Z

bot, add author to whitelist

jlebon · 2019-06-03T20:51:01Z

Awesome, thanks so much for working on this and putting up with all the reviews! 🎉
@rh-atomic-bot r+ 2e237c3

(BTW, just realized this patch was never tested in CI. I've enabled it now, so if it bounces it should be easier to iterate until we go green.)

Fixes #1670 This patch introduces a new `compose tree --write-lockfile-to=manifest.lock` argument and a new `compose tree --lockfile=manifest.lock` to read it back for subsequent invocations. Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com> Closes: #1745 Approved by: jlebon

rh-atomic-bot · 2019-06-03T20:51:14Z

⌛ Testing commit 2e237c3 with merge 8ff1c33...

jlebon · 2019-06-03T20:54:23Z

Also just noticed I messed up during my recommendation for the commit message and gave you the switch names without the ex- prefix. Not a big deal, though if it bounces, we can fix it up!

rh-atomic-bot · 2019-06-03T21:31:03Z

💔 Test failed - status-atomicjenkins

jlebon · 2019-06-04T13:23:47Z

Hmm, looks like test-ucontainer.sh is failing with:

Executing(%clean): /bin/sh -e /var/tmp/rpm-tmp.Qe7wab
Enabled rpm-md repositories: test-repo
rpm-md repo 'test-repo' (cached); generated: 2019-06-03T22:20:59Z

(/var/tmp/checkout/.libs/lt-rpm-ostree ex container assemble:29578): GLib-CRITICAL **: 22:20:59.411: g_hash_table_lookup: assertion 'hash_table != NULL' failed
/var/tmp/checkout/tests/check/test-ucontainer.sh: line 48: 29578 Trace/breakpoint trap   (core dumped) rpm-ostree ex container assemble foo.conf

jlebon · 2019-06-04T13:25:07Z

src/libpriv/rpmostree-core.c

  for (char **it = pkgnames; it && *it; it++)
    {
      const char *pkgname = *it;
-      g_autoptr(GError) local_error = NULL;
+      g_autoptr(DnfPackage) pkg = rpmostree_get_locked_package (sack, self->vlockmap, pkgname);


Either should conditionalize this bit on self->vlockmap != NULL here or you can also check it in rpmostree_get_locked_package.

Fixes coreos#1670 This patch introduces a new `compose tree --ex-write-lockfile-to=manifest.lock` argument and a new `compose tree --ex-lockfile=manifest.lock` to read it back for subsequent invocations. Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>

jlebon · 2019-06-04T15:14:43Z

@rh-atomic-bot r+ cd3e6c6

rh-atomic-bot · 2019-06-04T15:14:50Z

⌛ Testing commit cd3e6c6 with merge 79dfcea...

rh-atomic-bot · 2019-06-04T16:08:27Z

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 79dfcea to master...

cgwalters · 2019-06-04T16:17:49Z

Awesome!

So an interesting next step here I think would be to change the FCOS pipeline to make use of this?

dustymabe · 2019-06-04T21:01:19Z

So an interesting next step here I think would be to change the FCOS pipeline to make use of this?

right - we're setting up mechanical refs to print out lockfiles and then we'll create new lockfiles (full control of content) to use as input for our dev/prod refs

dustymabe · 2019-06-04T21:01:34Z

nice work @r4f4

One problem with how we use lockfiles right now is that we don't enforce them for dependencies. That is, if `foo` requires `bar`, but only `foo` is in the manifest, then while `foo` will be locked`, `bar` will never be checked against the lockfile because it was never explicitly requested. Higher-level though, I don't like how indirect the locking here feels. See some comments about that in: coreos#1745 (comment) coreos#1745 (comment) Essentially, the manifest is an input file of patterns, and all we really know from the lockfile output is that the set of packages in there satisfies this input in some way. But: 1. there are multiple ways to satisfy the same input (hence why hints like `SOLVER_FAVOR` exist) 2. the solution is dependent on how the solver is implemented (i.e. different libsolv versions might yield different solutions) 3. the solution is dependent on flags fed to the solver (i.e. different libdnf versions might yield different solutions) So any attempt at cross-checking between the input file and the lockfile is going to be very hard. Using a stricter mode as I suggested in coreos#1745 of only allowing pure pkgnames or NEVRAs would help, but it wouldn't address the dependency issue. (Though I'm still thinking about possibly doing this anyway.) The solution I propose here is instead to take the nuclear approach: we completely exclude from the sack all packages of the same name as packages in our lockfiles, but which do not match the NEVRA. Therefore, any possible solution has to also satisfy our lockfile (or error out).

One problem with how we use lockfiles right now is that we don't enforce them for dependencies. That is, if `foo` requires `bar`, but only `foo` is in the manifest, then while `foo` will be locked, `bar` will never be checked against the lockfile because it was never explicitly requested. Higher-level though, I don't like how indirect the locking here feels. See some comments about that in: coreos#1745 (comment) coreos#1745 (comment) Essentially, the manifest is an input file of patterns, and all we really know from the lockfile output is that the set of packages in there satisfies this input in some way. But: 1. there are multiple ways to satisfy the same input (hence why hints like `SOLVER_FAVOR` exist) 2. the solution is dependent on how the solver is implemented (i.e. different libsolv versions might yield different solutions) 3. the solution is dependent on flags fed to the solver (i.e. different libdnf versions might yield different solutions) So any attempt at cross-checking between the input file and the lockfile is going to be very hard. Using a stricter mode as I suggested in coreos#1745 of only allowing pure pkgnames or NEVRAs would help, but it wouldn't address the dependency issue. (Though I'm still thinking about possibly doing this anyway.) The solution I propose here is instead to take the nuclear approach: we completely exclude from the sack all packages of the same name as packages in our lockfiles, but which do not match the NEVRA. Therefore, any possible solution has to also satisfy our lockfile (or error out).

One problem with how we use lockfiles right now is that we don't enforce them for dependencies. That is, if `foo` requires `bar`, but only `foo` is in the manifest, then while `foo` will be locked, `bar` will never be checked against the lockfile because it was never explicitly requested. Higher-level though, I don't like how indirect the locking here feels. See some comments about that in: #1745 (comment) #1745 (comment) Essentially, the manifest is an input file of patterns, and all we really know from the lockfile output is that the set of packages in there satisfies this input in some way. But: 1. there are multiple ways to satisfy the same input (hence why hints like `SOLVER_FAVOR` exist) 2. the solution is dependent on how the solver is implemented (i.e. different libsolv versions might yield different solutions) 3. the solution is dependent on flags fed to the solver (i.e. different libdnf versions might yield different solutions) So any attempt at cross-checking between the input file and the lockfile is going to be very hard. Using a stricter mode as I suggested in #1745 of only allowing pure pkgnames or NEVRAs would help, but it wouldn't address the dependency issue. (Though I'm still thinking about possibly doing this anyway.) The solution I propose here is instead to take the nuclear approach: we completely exclude from the sack all packages of the same name as packages in our lockfiles, but which do not match the NEVRA. Therefore, any possible solution has to also satisfy our lockfile (or error out). Closes: #1849 Approved by: cgwalters

jlebon added the WIP label Mar 1, 2019

r4f4 force-pushed the versionlock branch from 721969e to cada2e2 Compare May 10, 2019 13:00

r4f4 force-pushed the versionlock branch from cada2e2 to b43bc07 Compare May 21, 2019 12:41

cgwalters reviewed May 29, 2019

View reviewed changes

jlebon reviewed May 29, 2019

View reviewed changes

r4f4 force-pushed the versionlock branch from b43bc07 to 38145be Compare May 30, 2019 21:03

r4f4 changed the title ~~WIP: version lock for packages in the treefile~~ Version lock for packages in the treefile May 30, 2019

jlebon reviewed May 31, 2019

View reviewed changes

r4f4 force-pushed the versionlock branch from 38145be to 0d87344 Compare May 31, 2019 20:20

jlebon removed the WIP label Jun 3, 2019

jlebon reviewed Jun 3, 2019

View reviewed changes

r4f4 force-pushed the versionlock branch from 0d87344 to 2e237c3 Compare June 3, 2019 20:21

jlebon changed the title ~~Version lock for packages in the treefile~~ compose: Add --lockfile and --write-lockfile-to Jun 3, 2019

r4f4 force-pushed the versionlock branch from 2e237c3 to 213a84c Compare June 3, 2019 22:10

jlebon reviewed Jun 4, 2019

View reviewed changes

jlebon changed the title ~~compose: Add --lockfile and --write-lockfile-to~~ compose: Add --ex-lockfile and --ex-write-lockfile-to Jun 4, 2019

r4f4 force-pushed the versionlock branch from 213a84c to cd3e6c6 Compare June 4, 2019 14:13

jlebon added the reviewed/approved label Jun 4, 2019

rh-atomic-bot closed this in 79dfcea Jun 4, 2019

jlebon mentioned this pull request Jun 4, 2019

composetree: allow url in manifest package list #1847

Open

jlebon mentioned this pull request Jun 5, 2019

core: Strengthen how we enforce lockfiles #1849

Closed

r4f4 deleted the versionlock branch June 6, 2019 09:01

jlebon mentioned this pull request Jun 13, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose: Add --ex-lockfile and --ex-write-lockfile-to #1745

compose: Add --ex-lockfile and --ex-write-lockfile-to #1745

r4f4 commented Jan 29, 2019

rh-atomic-bot commented Jan 29, 2019

r4f4 commented Feb 1, 2019

rh-atomic-bot commented Apr 15, 2019

r4f4 commented May 21, 2019

cgwalters May 29, 2019

jlebon left a comment

r4f4 commented May 30, 2019

jlebon May 31, 2019

r4f4 May 31, 2019

jlebon Jun 3, 2019

jlebon May 31, 2019

jlebon May 31, 2019

jlebon May 31, 2019

jlebon Jun 5, 2019

r4f4 commented May 31, 2019

jlebon left a comment

jlebon Jun 3, 2019

cgwalters commented Jun 3, 2019

jlebon commented Jun 3, 2019

jlebon commented Jun 3, 2019

jlebon commented Jun 3, 2019

rh-atomic-bot commented Jun 3, 2019

jlebon commented Jun 3, 2019

rh-atomic-bot commented Jun 3, 2019

jlebon commented Jun 4, 2019

jlebon Jun 4, 2019

jlebon commented Jun 4, 2019

rh-atomic-bot commented Jun 4, 2019

rh-atomic-bot commented Jun 4, 2019

cgwalters commented Jun 4, 2019

dustymabe commented Jun 4, 2019

dustymabe commented Jun 4, 2019

compose: Add --ex-lockfile and --ex-write-lockfile-to #1745

compose: Add --ex-lockfile and --ex-write-lockfile-to #1745

Conversation

r4f4 commented Jan 29, 2019

rh-atomic-bot commented Jan 29, 2019

r4f4 commented Feb 1, 2019

rh-atomic-bot commented Apr 15, 2019

r4f4 commented May 21, 2019

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

r4f4 commented May 30, 2019

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

r4f4 commented May 31, 2019

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Jun 3, 2019

jlebon commented Jun 3, 2019

jlebon commented Jun 3, 2019

jlebon commented Jun 3, 2019

rh-atomic-bot commented Jun 3, 2019

jlebon commented Jun 3, 2019

rh-atomic-bot commented Jun 3, 2019

jlebon commented Jun 4, 2019

Choose a reason for hiding this comment

jlebon commented Jun 4, 2019

rh-atomic-bot commented Jun 4, 2019

rh-atomic-bot commented Jun 4, 2019

cgwalters commented Jun 4, 2019

dustymabe commented Jun 4, 2019

dustymabe commented Jun 4, 2019