Skip to content

Conversation

@ashcrow
Copy link
Member

@ashcrow ashcrow commented Jul 24, 2019

See: https://bugzilla.redhat.com/show_bug.cgi?id=1731317

For testing I:

  • ran a cosa build locally
  • created an oscontainer with this pr
  • exported the oscontainer to a tar
  • imported the tar into my host container storage
  • created a container and mounted it
  • checked that the pkglist.txt now existed
$ sudo podman load -i ./test.tar
Getting image source signatures
Copying blob 9b2483854a3e done
Copying config adb6280425 done
Writing manifest to image destination
Storing signatures
Loaded image(s): localhost/test:latest
$ sudo podman create --net=none --name os-container-test localhost/test:latest
f6a6deb6041b21544958ae2b999e9c214dfdc6a7a3ee13d7fd448e35560ff735
$ sudo podman mount os-container-test
/var/lib/containers/storage/overlay/093ddb214d0fcbd36781b80700403a95f384fcef07319b077ef905c61c005e2c/merged
$ sudo head -n 3 /var/lib/containers/storage/overlay/093ddb214d0fcbd36781b80700403a95f384fcef07319b077ef905c61c005e2c/merged/pkglist.txt
NetworkManager-1:1.14.0-14.el8.x86_64
NetworkManager-libnm-1:1.14.0-14.el8.x86_64
acl-2.2.53-1.el8.x86_64

/cc @jasinner

@ashcrow ashcrow added the bug Something isn't working label Jul 24, 2019
@ashcrow ashcrow requested review from cgwalters and miabbott July 24, 2019 17:52
@ashcrow
Copy link
Member Author

ashcrow commented Jul 24, 2019

My only question is if there is a valid use case where there is no version for ostree.

  • If so, I'll add a new input so the caller can specify the version
  • If not, I'd like to remove the setting of ostree_version to None

@ashcrow ashcrow changed the title src/oscontainer.py: Add commitmeta.json to container root WIP: src/oscontainer.py: Add commitmeta.json to container root Jul 24, 2019
@cgwalters
Copy link
Member

My only question is if there is a valid use case where there is no version for ostree.

Probably not, but as I noted above I think I wrote the command originally here wrong...we should really imagine this to be cosa buildextend-oscontainer or something, and have it accept a build ID and not just a raw ostree by default.

@ashcrow ashcrow force-pushed the commitmeta-in-os-container branch from dea2820 to c562c44 Compare July 24, 2019 18:41
@ashcrow ashcrow requested a review from darkmuggle July 24, 2019 18:43
@ashcrow ashcrow force-pushed the commitmeta-in-os-container branch from c562c44 to f20baff Compare July 24, 2019 18:52
@ashcrow ashcrow changed the title WIP: src/oscontainer.py: Add commitmeta.json to container root src/oscontainer.py: Add commitmeta.json to container root Jul 24, 2019
@jlebon
Copy link
Member

jlebon commented Jul 24, 2019

Thoughts on https://bugzilla.redhat.com/show_bug.cgi?id=1731317#c2?

Just to expand on this a little bit. Commit metadata is a mishmash of both stable and unstable things. E.g. the version key is pretty stable. But rpm-ostree OTOH uses the commit metadata pretty liberally. E.g. the "layered" version of commit metadata has a lot of "state" which e.g. status needs to know about. Those aren't necessarily stable things (we've changed format a few times, and although we do version it now, can we expect third-party clients to start versioning the code that'll inspect this data?).

If all we really need here is the pkglist, then it seems much more low-risk to me to just export a pkglist.(txt|json), no?

@jlebon
Copy link
Member

jlebon commented Jul 24, 2019

E.g. the "layered" version of commit metadata

To be clear, while I'm talking about the layered version here, and this is a "base layer" commit (which has much fewer keys), but one can imagine a strategy where the oscontainer in fact contains a layered commit with e.g. newer kubelet or whatever. There's also the higher-level concern that from this point on, any new key we add is essentially de facto made stable.

@cgwalters
Copy link
Member

There's also the higher-level concern that from this point on, any new key we add is essentially de facto made stable.

I see what you're saying but OTOH this content is in our build buckets too.

But it's also fine by me to filter it to just the pkglist.

@jlebon
Copy link
Member

jlebon commented Jul 24, 2019

I see what you're saying but OTOH this content is in our build buckets too.

Right, I guess this is the first ask I come across where we're directly saying to someone else "please use the commitmeta.json" (but I might've missed others). Esp. since we have a perfectly fine stable interface for the information they need (rpm-ostree db list), which even takes care of e.g. falling back to checking out the rpmdb for older commits (even though that's not a concern here; and I understand wanting a simple JSON/text file instead of having to install rpm-ostree).

IIRC, the motivation for extracting the commitmeta.json originally was for displaying it in the browser (which is developer-centric) or potentially using it to perform diffs (in which case ideally one would use the public rpm-ostree bindings for this instead of re-implementing it).

@ashcrow
Copy link
Member Author

ashcrow commented Jul 24, 2019

@jlebon to clarify, this would be:

rpm-ostree db list --repo=tmp/repo $OSTREE_COMMIT_HASH > pkglist.txt

Correct?

@jlebon
Copy link
Member

jlebon commented Jul 24, 2019

That'd work, though rpm-ostree db list is normally for humans. Since we're already using GI bindings here, something like this should work (not tested):

diff --git a/src/oscontainer.py b/src/oscontainer.py
index 96ab2aa..a338c02 100755
--- a/src/oscontainer.py
+++ b/src/oscontainer.py
@@ -10,8 +10,9 @@
 import gi

 gi.require_version('OSTree', '1.0')
+gi.require_version('RpmOstree', '1.0')

-from gi.repository import GLib, Gio, OSTree
+from gi.repository import GLib, Gio, OSTree, RpmOstree

 import argparse
 import json
@@ -117,6 +118,12 @@ def oscontainer_build(containers_storage, src, ref, image_name_and_tag,
         print("Copying ostree commit into container: {} ...".format(rev))
         run_verbose(["ostree", "--repo=" + dest_repo, "pull-local", src, rev])

+        pkgs = RpmOstree.db_query_all(r, rev, None)
+        with open(os.path.join(mnt, 'pkglist.txt'), 'w') as f:
+            for pkg in pkgs:
+                f.write(pkg.get_nevra())
+                f.write('\n')
+
         # We use /noentry to trick `podman create` into not erroring out
         # on a container with no cmd/entrypoint.  It won't actually be run.
         config = ['--entrypoint', '["/noentry"]',

@ashcrow ashcrow changed the title src/oscontainer.py: Add commitmeta.json to container root src/oscontainer.py: Add pkglist.txt to container root Jul 24, 2019
@ashcrow ashcrow changed the title src/oscontainer.py: Add pkglist.txt to container root WIP: src/oscontainer.py: Add pkglist.txt to container root Jul 24, 2019
@ashcrow ashcrow force-pushed the commitmeta-in-os-container branch from f20baff to 0c82eef Compare July 24, 2019 20:50
@ashcrow
Copy link
Member Author

ashcrow commented Jul 24, 2019

@jlebon PTAL. Tested locally.

@ashcrow ashcrow changed the title WIP: src/oscontainer.py: Add pkglist.txt to container root src/oscontainer.py: Add pkglist.txt to container root Jul 24, 2019
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM!

@ashcrow ashcrow force-pushed the commitmeta-in-os-container branch from 0c82eef to 776b50c Compare July 24, 2019 20:55
@ashcrow
Copy link
Member Author

ashcrow commented Jul 24, 2019

Updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants