-
Notifications
You must be signed in to change notification settings - Fork 198
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
core: Don't allow noent when resolving pkgcache rev #2545
Conversation
If we get here, it's that we expect the pkgcache to be there. So don't allow ENOENT (we weren't even checking for the ENOENT case here, which shows that this was the intent). Related: https://bugzilla.redhat.com/show_bug.cgi?id=1925584
/lgtm |
This alone doesn't fix https://bugzilla.redhat.com/show_bug.cgi?id=1925584 though. It just makes the error more clear. We need to figure out why it's not finding the rev. |
I added another commit here "Make failure to find packages fatal, add more error prefixing" - let's make this the PR to fix this bug. |
Current theory: we're not setting the dbpath macro in all cases; currently playing with
|
Hmm, nope. |
The reason the rev went missing is the code in master isn't finding the package database, so we end up pruning the pkgcache refs. This PR makes "no packages in root" a fatal error. |
OK, I was able to reproduce https://bugzilla.redhat.com/show_bug.cgi?id=1925717 at least. I think what's happening there is that overrides finalization happens before we create the
Hmm, the problem there might be that the RC files get re-read at |
Currently testing diff --git a/src/libpriv/rpmostree-rpm-util.cxx b/src/libpriv/rpmostree-rpm-util.cxx
index cd62ee98..a866faaf 100644
--- a/src/libpriv/rpmostree-rpm-util.cxx
+++ b/src/libpriv/rpmostree-rpm-util.cxx
@@ -842,6 +855,8 @@ get_sack_for_root (int dfd,
if (!dnf_sack_setup (sack, 0, error))
return FALSE;
+ rpmostree_set_rpm_macro_define ("_dbpath", "/" RPMOSTREE_RPMDB_LOCATION);
+
if (!dnf_sack_load_system_repo (sack, NULL, 0, error))
return FALSE;
|
Ahh right, that looks likely to work. |
OK so CI is passing here again because we have coreos/fedora-coreos-config@714faaf So...we could merge this PR as is and make a new PR that also pulls in the new libsolv explicitly in CI? |
Yeah, let's do that. I'd like to see the actual error in CI (the previous failure was wiped out by a Jenkins restart). /lgtm |
@jlebon: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Yeah, let's do that. I'd like to see the actual error in CI (the previous failure was wiped out by a Jenkins restart). /lgtm |
@jlebon: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm |
/refresh |
[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:
Approvers can indicate their approval by writing |
This path is a bit dangerous because we don't want to affect any traditional systems or containers that just happen to have the In the short term, I think we need to live with hacking in our override dynamically in memory. Now...maybe this would be cleaner as a top-level libdnf API? Something like: |
Once hit, how does one get out of this error situation since upgrades fail? |
For now, probably best to |
I meant more injecting the macro at compose time. Still, I agree we do want to get this fixed by setting the rpmdb path too so there isn't a dependency on the rpm-ostree version on the compose server. |
This is what I get:
|
What does |
Ugh, I was trying to figure out why I couldn't reproduce this on git master when using So I can confirm that #2545 (comment) doesn't fix it either. This does work though:
And is way more foolproof. So my vote is to just ship that in our OSTree commits and then ratchet it into compose servers before dropping the libsolv pin. |
Won't that break e.g. coreos-assembler? |
FWIW https://pagure.io/workstation-ostree-config/pull-request/190 is inbound to revert libsolv for Silverblue. FCOS also is reverted. That leaves Fedora IOT and other desktop spins out though for now. |
OK, added a commit in #2553 which does this! We'll see if that fixes CI.
Hmm, can you expand? |
When you mentioned adding the macro, I thought you meant add it to our RPM package, which would affect both server and client side. #2553 only changes the client side, which will help a lot but I'm still concerned about the server side. But to elaborate here, if we added the macro to our package, we'd end up affecting e.g. OTOH it is kind of tempting to try switching the coreos-assembler container to use |
I am also still having a problem. During researching the issue I found that one can see available commits with:
I then rebased on latest commit, and performed
I then run journalctl logs
P.S. Latest commit as of the time of posting is:
Update: I was also able to get this concise error, in case it helps:
I don't know if that 's the same issue this PR addresses or a separate one? cc @jlebon |
Hmm interesting. Yes, this is a different bug. Can you open a new issue in this repo? |
If we get here, it's that we expect the pkgcache to be there. So don't
allow ENOENT (we weren't even checking for the ENOENT case here, which
shows that this was the intent).
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1925584