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

core: Add OSTREE_COMMIT_META_KEY_ARCH #2121

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

cgwalters
Copy link
Member

Add a standard key for this. We actually had a case in OpenShift
builds recently where a ppc64le image was pushed over an x86_64
one and this started failing at runtime with a not immediately
obvious error.

I'll probably end up changing rpm-ostree at least to use
the RPM architecture for this key and fail if it doesn't match
the booted value.

Possibly that should live in ostree but it would involve adding
architecture schema here, which gets into a big mess. Let's
just standardize the key.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Hmm yeah, I think that makes sense. We can then use that in Zincati instead of the more "implementation-specific" coreos-assembler.basearch. Might be worth linking to coreos/coreos-assembler@e02ef26 in the commit message. LGTM otherwise!

Re.

I'll probably end up changing rpm-ostree at least to use
the RPM architecture for this key and fail if it doesn't match
the booted value.

doesn't that theoretically break going from a 32-bit to 64-bit kernel? I'm not sure if 32-bit kernels would still show x86_64 on a 64-bit capable CPU even if it was compiled for 32-bit. (Or I guess this applies more generally going between compatible "sub-architectures").

*
* Since: 2020.4
*/
#define OSTREE_COMMIT_META_KEY_ARCH "ostree.arch"
Copy link
Member

Choose a reason for hiding this comment

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

Small bikeshed to just spell this out as OSTREE_COMMIT_META_KEY_ARCHITECTURE "ostree.architecture", though good as is too.

Copy link
Member Author

Choose a reason for hiding this comment

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

doesn't that theoretically break going from a 32-bit to 64-bit kernel?

Right, so far we haven't hit that problem for any system we care about that I know. e.g. no one sane runs i686 kernels anymore, and for ARM in Fedora it's just aarch64. But I guess if we did do this in rpm-ostree we'd probably want to use the libdnf architecture mapping for compatible types to do it "properly".

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if 32-bit kernels would still show x86_64 on a 64-bit capable CPU even if it was compiled for 32-bit.

No, 32 bit kernels report as i686.

Add a standard key for this.  We actually had a case in OpenShift
builds recently where a `ppc64le` image was pushed over an `x86_64`
one and this started failing at runtime with a not immediately
obvious error.

I'll probably end up changing rpm-ostree at least to use
the RPM architecture for this key and fail if it doesn't match
the booted value.

Possibly that should live in ostree but it would involve adding
architecture schema here, which gets into a big mess.  Let's
just standardize the key.

xref coreos/coreos-assembler@e02ef26
@jlebon
Copy link
Member

jlebon commented Jun 5, 2020

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cgwalters
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants