-
Notifications
You must be signed in to change notification settings - Fork 195
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
Use ostree-rs-ext 0.5.0 #3139
Use ostree-rs-ext 0.5.0 #3139
Conversation
e1199e3
to
c48f81a
Compare
rust/src/sysroot_upgrade.rs
Outdated
match &imgref.sigverify { | ||
ostree_container::SignatureSource::OstreeRemote(_) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here is that right now if we're pulling an ostree-remote-image
then since we don't support layered, ostree-signed imports, we default to un-encapsulation. If we're pulling a not-ostree-signed image, then we use layering.
But basically this is ostreedev/ostree-rs-ext#99 (comment)
c48f81a
to
64f65d2
Compare
64f65d2
to
a76117f
Compare
main
for now
OK, updated. But, this blocks on resolution to #3160 |
a76117f
to
06b1140
Compare
OK, I updated this. Turned out the tests required some changes because we now default to the "container store" path, which right now creates a new commit even when there's only one layer. That'd be nice to fix, but I need to think about how to do so most elegantly. For now, I tweaked the test to compare content checksums. |
Blah, https://bodhi.fedoraproject.org/updates/FEDORA-2021-1854bd6a04 has to hit bodhi stable first. |
Oh fun, I hit:
Which happens I think because the derived commit written by ostree doesn't have the packagelist metadata. Hmmmm...I guess in theory, what we need is for the ostree APIs here to expose a hook/callback so that rpm-ostree can inject that data. But, we should also be robust to the absence of it. |
This uses https://copr.fedorainfracloud.org/coprs/g/CoreOS/experimental/ "Contains builds of coreos/rpm-ostree#3139 and containers/skopeo#1476 for CentOS8, plus dependencies."
7de4033
to
772c272
Compare
/test all OK, this is updated now. |
Argh, buildroot container out of date again. |
This uses https://copr.fedorainfracloud.org/coprs/g/CoreOS/experimental/ "Contains builds of coreos/rpm-ostree#3139 and containers/skopeo#1476 for CentOS8, plus dependencies."
This uses https://copr.fedorainfracloud.org/coprs/g/CoreOS/experimental/ "Contains builds of coreos/rpm-ostree#3139 and containers/skopeo#1476 for CentOS8, plus dependencies."
772c272
to
bc77e38
Compare
Lifting draft on this one. |
main
for now5d09d75
to
597ac8d
Compare
OK 🆕 - the In the process of doing that I hit a few issues e.g. ostreedev/ostree-rs-ext#153 would make it much more ergonomic to do local builds, but I've worked around that by copying the image again. |
/retest |
597ac8d
to
1946795
Compare
if (cur_digest) | ||
auto refspec_s = std::string(refspec); | ||
auto import = CXX_TRY_VAL(pull_container(*self->repo, *cancellable, refspec_s), error); | ||
if (!import->changed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change detection logic correctly handle the case where a newer image was already pulled but not yet deployed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, good catch. I think the bug here dates to 7ac7ef4
AFAICS, the return
there was always unnecessary. But testing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, actually being able to remove that in turn means we can remove the now-unnecessary changed
boolean which in turn means we can drop that wrapper struct, etc. 🎉
Cargo.toml
Outdated
@@ -53,7 +53,7 @@ openat = "0.1.21" | |||
openat-ext = "^0.2.2" | |||
openssl = "0.10.38" | |||
os-release = "0.1.0" | |||
ostree-ext = "0.3.0" | |||
ostree-ext = { git = "https://github.com/ostreedev/ostree-rs-ext", branch = "main" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message says 0.5.0 but this is pointing to the main branch. Are you planning to cut another release and update this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll do that soon, I wanted to have green CI here, which I think we mostly have. But then I started doing more API changes, so will land those first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that's done!
1946795
to
efc9eb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
efc9eb2
to
600f128
Compare
Oops, forgot to push some local bits. Done now |
/retest |
Hmm. This may be reproducing a hang I've seen sometimes locally, but not managed to track down. I thought it would at least be surfaced by containers/containers-image-proxy-rs#3 but perhaps not. |
OK reproduced the hang locally in a kola run, but ssh'ing into the VM and rerunning
|
I think the problem here may go to #3082 Basically we have a mess due using both the glib mainloop and C threads, as well as tokio. Hmm. I think what we should be doing is doing |
OK I am disappointed I didn't debug this today. But, I made some progress in understanding things. One specific discovery is that tokio takes over the global We may need to add some sort of hook in tokio to allow deferring Now that said, I'm not totally sure that's the problem yet. What's really maddening is that for some reason this reliably reproduces when run in Using this tokio patch:
I get:
And the skopeo process has clearly exited: So...I think this is something going wrong in the guts of the skopeo child process machinery, possibly due to interaction with the glib mainloop. I started hacking on forking off the whole thing as a subprocess (from the daemon) which would be "sustainably hacky" but what is somewhat annoying about this is that ostree doesn't expose the whole "remount /sysroot as writable" bits. Hmm, I guess we could fork via |
containers/containers-image-proxy-rs#13 |
What seems obvious in retrospect is that the flow here is:
And then rerunning that loop will fail...but if we auto-exit (or if one restarts the process) then it will work fine. And some of my interactive debugging either got the auto-exit timeout or I was e.g. adding some new debug code via Whereas the kola run is fast enough that it's hitting the same daemon instance multiple times reliably. |
600f128
to
b878582
Compare
b878582
to
486c404
Compare
Just for reference I also tried the below. Originally, I thought we could simply do
but it currently causes glib to error out later. We'd have to have a persistent |
Part of coreos/enhancements#7