-
Notifications
You must be signed in to change notification settings - Fork 462
Use pivot.service to perform pivot #335
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 pivot.service to perform pivot #335
Conversation
|
Infra flakes /retest |
|
Infra flake /retest |
181abc3 to
0a163a0
Compare
|
OK, this now also proxies journal logs from the pivot unit: (Notice the Though one thing I'm unsure of is:
I'm not sure of the ramifications of this. IIUC, it's safe to enable this since we're not cross-compiling, right? |
Ahh right, this will require adding |
ashcrow
left a comment
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.
So far makes sense. One nit, but not a blocker.
hack/build-go.sh
Outdated
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.
CGO 😦 ... but understandable.
|
Building fails due to lack of C headers: |
Yup, mentioned this higher up in #335 (comment). :) Just waiting to see if someone more versed in golang than I can confirm |
|
@abhinavdahiya Any reason why this commit added |
To force static binaries. Without it @crawford was unable to test binaries locally. Also don't turn on cgo for all components, I think we should do it only for the daemon. |
I run NixOS, which is a really good way of double checking if you've made too many assumptions ;) I don't have libc, bash, or even a dynamic linker in the traditional location. I'm on a personal crusade to kill the misuse of the term "Linux Binary" (looking right at you, https://golang.org/dl/). |
57c884e to
51a6285
Compare
|
OK, comments addressed! This is mostly waiting for openshift/release#2783 now. |
|
Added more to the pre-req for review ... also ping'd owners in chat. 🤞 |
|
openshift/release#2783 merged /retest |
|
golang base image hasn't probably propagated yet (or built fwiw) |
|
Clayton said the base golang image should be rebuild now, trying... /retest |
|
Yeah, want to retest it one last time now that all the prereqs are baked into the latest RHCOS image. But need to set up a new cluster first. |
We will want to be able to proxy some of the host journal logs in the MCD logs.
Proxy the journal logs from `pivot.service` and `rpm-ostreed.service` after starting it so we can see the progress of the pivot operation directly from the MCD container logs. This is important for troubleshooting. We're also likely to proxy more journal logs in the future, so this just breaks the ice. One note here is that this requires `CGO_ENABLED=1` because the journal bindings use the dlopen package, which has `-ldl`. We're enabling it only for the machine-config-daemon component for now though.
3f77d41 to
43c676b
Compare
|
Nevermind, buildah and podman are doing heavy caching which I don't like |
|
trying an upgrade to the latest oscontainer for ootpa, I get this:
besides ootpa, code LGTM |
|
Hmm, can you check the journal for errors, e.g. |
|
ok, found this after scraping the journal at around the time pivot failed: |
|
alright, that seems to be the case why it still fails on ootpa, if I |
|
we can follow up on ootpa btw, maipo upgrade went well for me /approve |
Ahh yup, that's https://bugzilla.redhat.com/show_bug.cgi?id=1672404 (see also coreos/rpm-ostree#1732 (comment)). The dev rpm-ostree uses |
|
Hmm, looks like |
|
Yeah, that image (the rhel one in this repo for the daemon) still doesn't have systemd-devel and the base image is different, even if I thought it's just mirrored. |
Is there an open issue to fix that ... or should it flow into it and we just need to bug someone to speed up the change? |
I was going to bug someone already actually as my understanding was that rhel builder images are just mirrored |
|
/retest |
/cc @sosiouxme |
|
/test rhel-images I don't think the image is updated yet but let's try just in case. |
|
We could fall back to just running |
|
@cgwalters true. If this test fails again it's worth the change unless @sosiouxme believes the image will be |
|
looks like it's built now? /retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, jlebon, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
🎊 |
Add aarch64 support
Rather than directly executing
pivotfrom within the container, useits new service unit to run it. The main advantage of this is that
pivotcan correctly runostreeasinstall_twhich is required forwriting new SELinux labels. More generally though, we should be running
such host tools within the host context rather than from the container.
This works nicely though the downside is that we lose progress output
from pivot. I'd like to add journal proxying to fix that.
Closes: #314
Requires: openshift/pivot#25