Skip to content
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

identity: remove requirement for coreos-assembler.basearch #876

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

cgwalters
Copy link
Member

This is aiming to increase compatibility of zincati with systems that aren't derived from Fedora CoreOS (e.g. IoT, desktop systems, etc.)

Additionally, even in the FCOS case, when one derives a container image from FCOS, the final build isn't generated by coreos-assembler and hence the ostree commit won't have this metadata.

In the end - I can't think of a use case for even trying to have zincati managing an image for a different architecture than the one it's compiled for.

So, copy the code I wrote for oci-spec-rs which maps from the Rust architecture to the GNU one. xref
containers/oci-spec-rs@52e450e

This is aiming to increase compatibility of zincati
with systems that aren't derived from Fedora CoreOS (e.g. IoT,
desktop systems, etc.)

Additionally, even in the FCOS case, when one derives a container
image from FCOS, the final build isn't generated by coreos-assembler
and hence the ostree commit won't have this metadata.

In the end - I can't think of a use case for even trying
to have zincati managing an image for a different architecture
than the one it's compiled for.

So, copy the code I wrote for oci-spec-rs which maps
from the Rust architecture to the GNU one.  xref
containers/oci-spec-rs@52e450e
@lucab lucab enabled auto-merge October 28, 2022 14:46
@lucab
Copy link
Contributor

lucab commented Oct 28, 2022

This is effectively a revert of #58, and I'm fine with merging this.
But if you read through the references there the idea wasn't to support cross-arch updates, but to make sure the underlying environment is sane (i.e. the underlying basearch and platform ID can be introspected through some stable API).
The fact that the commit metadata get lost and Zincati is the first thing to break is a canary in the mine.

/cc @jlebon

@cgwalters
Copy link
Member Author

OK, if you want to look at it we can - here's the neat thing, with this move we can use the standard container image metadata. Specifically I can change things (in the container case) to call https://docs.rs/ostree-ext/latest/ostree_ext/container/store/fn.query_image_ref.html and we can dig from there into https://docs.rs/oci-spec/0.5.8/oci_spec/image/struct.ImageConfiguration.html#method.architecture

(However, we would then need to re-map the Go/OCI architecture back to GNU, we have Go code for that in https://github.com/coreos/stream-metadata-go/blob/main/arch/arch.go that we'd need to put somewhere in Rust)

// Translate the architecture we were compiled for to the
// output from the `arch` command that is used for RPM and
// coreos-assembler builds.
pub(crate) fn current_architeture() -> &'static str {
Copy link
Member Author

Choose a reason for hiding this comment

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

@cgwalters
Copy link
Member Author

I've been thinking about this more, and I think

make sure the underlying environment is sane (i.e. the underlying basearch and platform ID can be introspected through some stable API).

is covered by coreos/stream-metadata-rust#34

We could also instead add rpm-ostree (really libdnf)'s idea of the architecture name into rpm-ostree status --json - not as part of the booted deployment.

But, I'm still not sure that the original change was really strongly motivated; we don't have any case for cross-architecture updates, so again the architecture that we built for Rust must be the same as the RPM/GNU/Linux one - after a suitable mapping. So it's more about maintaining that suitable mapping in a sustainable way, and I do think the stream-metadata-rust PR is that.

@lucab lucab merged commit 4848f9e into coreos:main Nov 2, 2022
@lucab lucab changed the title Remove requirement for coreos-assembler.basearch identity: remove requirement for coreos-assembler.basearch Nov 2, 2022
@lucab lucab added this to the vNext milestone Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants