-
Notifications
You must be signed in to change notification settings - Fork 168
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
build: Add /.coreos-aleph-version.json to target #768
Conversation
Though hmm, probably |
d726202
to
bd16482
Compare
OK reworked this to go in |
Strong plus on this, overall. Some comments on the approach though:
I'm not sure about all the pros and cons, but I did like the runtime approach. I think it would also play better if/when we support blowing the FS. |
bd16482
to
4d28827
Compare
OK, done.
Hmm. This to me is about knowing the true initial state; the original pristine disk image. If the user uses Ignition to repartition on boot, this would be carried over but not changed - we'd know the original filesystem from the version, and we'd know the new filesystem from their Ignition config. |
src/cmd-buildextend-metal
Outdated
@@ -173,7 +173,7 @@ if [ -z "${use_anaconda}" ]; then | |||
ref_arg=${commit} | |||
fi | |||
|
|||
runvm -drive "if=virtio,id=target,format=${image_format},file=${path}.tmp" -- /usr/lib/coreos-assembler/create_disk.sh /dev/vda "$ostree_repo" "${ref_arg}" "${ostree_remote}" /usr/lib/coreos-assembler/grub.cfg "$name" "${save_var_subdirs}" "\"$kargs\"" | |||
runvm -drive "if=virtio,id=target,format=${image_format},file=${path}.tmp" -- /usr/lib/coreos-assembler/create_disk.sh /dev/vda "${img}" "$ostree_repo" "${ref_arg}" "${ostree_remote}" /usr/lib/coreos-assembler/grub.cfg "$name" "${save_var_subdirs}" "\"$kargs\"" |
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.
Shouldn't we be passing ${build}
here?
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.
I think this is a "why not both?" situation - the imageid carries the platform and OS name too; it's completely unambiguous. But I can see the pure buildid being useful too so people don't have to parse the imageid. Added both.
src/create_disk.sh
Outdated
cat > rootfs/.coreos-aleph-version.json << EOF | ||
{ | ||
"build": "${buildid}", | ||
"ostree-commit": "${ostree_commit}" |
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.
Let's also add ostree-ref
here? Since that also encodes the stream they started on (esp. since build IDs are not unique per stream).
Can also probably add a |
4d28827
to
4a11d96
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! Will leave open for a bit for folks coming in from coreos/fedora-coreos-tracker#170.
I think that all these details are part of the commit metadata, so we could add logic somewhere (rpm-ostree / pivot / zincati) to dump them to disk on first-boot (i.e. if they are not on disk already). |
Mmmm, no, the buildid isn't because coreos-assembler currently supports generating new images without rebuilding the ostree commit. Second, the underlying full disk name (e.g.
This is a uniform public interface, right?
As I noted above, if this lands we can certainly change the in-progress rootfs redeploy PR to carry forward this information too. |
(We could also stick this file in |
Example build:
|
We had a realtime chat about this, I think the summary is in:
It's worth elaborating on all of this because it sheds light on the current build process and design. A core issue here is that we can't add this metadata to the ostree commit itself, because that would cause the commit hash to differ, and further, by default ostree will end up garbage collecting the original commit object. Now, ostree does have detached metadata which we could use for this, but it'd be a bit weird to do so I think because that metadata wouldn't exist in the upstream repo. (Using detached metadata would also require not GC'ing the original commit) We also discussed the fact that the Or to summarize: This PR is only adding information which does not exist in the current image. (Or exists but is subject to change/GC) |
4a11d96
to
06c7a71
Compare
Updated to add a comment describing the keys. |
I guess though a question is whether it's worth trying to plumb through support for an "original commit" into OSTree. It's possible; but crosses several layers in the design. OSTree for example doesn't have support for "tags" or "immutable refs" which we'd clearly want for this. And even if we did that I think it'd be an extension/enhancement to this PR; we can't store the metadata in commit objects, and detached metadata is weird for this. |
@cgwalters thanks for summarizing, these are all interesting points. From my side, as an OS consumer, I'd need some
The reason I need those information is to use them as foreign keys in order to walk the *COS metadata, e.g. starting from https://builds.coreos.fedoraproject.org/prod/streams/testing/releases.json. |
Soo...I think such a thing would require having the fedora-coreos-pipeline pass extended metadata down into cosa. Something like What do we expect to happen with our services like pinger when doing a We can try to work through those things; I'm a bit uncertain whether we should try to solve all of that before landing this PR. Not opposed, but given we're just adding JSON and it's easily extensible, I'd lean towards doing it as a second round after we've thought about some of the above issues. |
(Very quickly here on the above topic; I've been thinking about having |
The commit messages and linkbacks in this PR should mostly explain the current strategy on how FCOS deals with this today. The TL;DR is:
Note also we do inject the stream name through the commit metadata (see coreos/fedora-coreos-config#110). We can do this because we don't promote OSTree commits, we promote fedora-coreos-config content + lockfiles (which reminded me that I forgot to implement one piece of feedback you had around that: coreos/fedora-coreos-releng-automation#42). |
It will be very useful in the future to be able to more rigorously know the state that a given machine *started* from. For example, small tweaks like `chattr +i /sysroot` are things that won't happen for in-place updates. The term "aleph" here means "start". If we decide to introduce a mechanism (e.g. systemd unit) that performs those changes even for old in-place installs, it could be useful to know exactly what the starting state was. Note this ends up in the *physical* storage root `/` which appears as `/sysroot` when booted. Closes: coreos/fedora-coreos-tracker#170
06c7a71
to
2701e91
Compare
OK right, thanks. Having the Anyways, rebased 🏄♂️ and comments addressed! |
# like "exactly what mkfs.xfs version was used" we can do | ||
# that via looking at the upstream build and finding the | ||
# build logs for it, getting the coreos-assembler version, | ||
# and getting the `rpm -qa` from that. |
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.
finding the build logs for it, getting the coreos-assembler version, and getting the
rpm -qa
from that.
Is that true though? Most likely except for recent builds, the cosa image would have long been GC'ed already. We should probably include a buildroot.txt
file or something in the build dir listing all the packages in the container.
Another approach is building cosa using lockfiles (and storing them in this repo), which has nice properties though I don't think it's worth the overhead right now.
It will be very useful in the future to be able to more rigorously
know the build that a given machine started from. For example,
small tweaks like
chattr +i /sysroot
are things that won'thappen for in-place updates.
If we decide to introduce a mechanism (e.g. systemd
unit) that performs those changes even for old in-place installs,
it could be useful to know exactly what the starting state was.
I chose
/boot
since it's a relatively fixed location whichpeople generally won't wipe and replace without also erasing
everything else.