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

Makefile: Add platform variant to eve container's TAG and rootfs version #4641

Merged
merged 1 commit into from
Mar 3, 2025

Conversation

rene
Copy link
Contributor

@rene rene commented Feb 25, 2025

In order to push the eve container for different variants, we must set different TAGs. Following the same approach for pkg/fw and pkg/nvidia, this commit adds the platform variant to the eve container's TAG when building for non-generic platforms. Nothing changes for generic (and specially x86) platforms.

Additionally, EVE-OS images must be differentiated across platform variants, so a remote controller can provide the same version for different platforms, e.g., generic, nvidia-jp5, nvidia-jp6. The platform variant is also added to the rootfs version (except for the default 'generic' platform).

The new TAG scheme changes from:

<HASH/EVE_RELEASE>-<HV>-<ARCH>

To:

<HASH/EVE_RELEASE>-<PLATFORM_VARIANT>-<HV>-<ARCH>

The PLATFORM variant is added right after the hash/release in order to keep the tuple <HV>-<ARCH>, keeping compatibilty with any script/tool that expects this kind of tag.

@OhmSpectator
Copy link
Member

OhmSpectator commented Feb 25, 2025

I don't think it's a problem of the PR, but should we check, what's it?

fuzz: elapsed: 25s, execs: 59882 (1/sec), new interesting: 83 (total: 83)
--- FAIL: FuzzResolveWithSrcIP (25.11s)
    fuzzing process hung or terminated unexpectedly: exit status 2
    Failing input written to testdata/fuzz/FuzzResolveWithSrcIP/ccb17a404f33b246
    To re-run:
    go test -run=FuzzResolveWithSrcIP/ccb17a404f33b246
FAIL
exit status 1
FAIL	github.com/lf-edge/eve/pkg/pillar/devicenetwork	25.145s

https://github.com/lf-edge/eve/actions/runs/13519855895/attempts/1?pr=4641

@rene
Copy link
Contributor Author

rene commented Feb 25, 2025

I don't think it's a problem of the PR, but should we check, what's it?

fuzz: elapsed: 25s, execs: 59882 (1/sec), new interesting: 83 (total: 83)
--- FAIL: FuzzResolveWithSrcIP (25.11s)
    fuzzing process hung or terminated unexpectedly: exit status 2
    Failing input written to testdata/fuzz/FuzzResolveWithSrcIP/ccb17a404f33b246
    To re-run:
    go test -run=FuzzResolveWithSrcIP/ccb17a404f33b246
FAIL
exit status 1
FAIL	github.com/lf-edge/eve/pkg/pillar/devicenetwork	25.145s

https://github.com/lf-edge/eve/actions/runs/13519855895/attempts/1?pr=4641

This one definitely not related, but the other two failing are related, I will work on the fix....

@rene rene marked this pull request as draft February 25, 2025 12:00
In order to push the eve container for different variants, we must set
different TAGs. Following the same approach for pkg/fw and pkg/nvidia, this
commit adds the platform variant to the eve container's TAG when building
for non-generic platforms. Nothing changes for generic (and specially x86)
platforms.

Additionally, EVE-OS images must be differentiated across platform
variants, so a remote controller can provide the same version for different
platforms, e.g., generic, nvidia-jp5, nvidia-jp6. The platform variant is
also added to the rootfs version (except for the default 'generic'
platform).

The new TAG scheme changes from:

<HASH/EVE_RELEASE>-<HV>-<ARCH>

To:

<HASH/EVE_RELEASE>-<PLATFORM_VARIANT>-<HV>-<ARCH>

The PLATFORM variant is added right after the hash/release in order to
keep the tuple <HV>-<ARCH>, keeping compatibilty with any script/tool
that expects this kind of tag.

Signed-off-by: Renê de Souza Pinto <[email protected]>
@deitch
Copy link
Contributor

deitch commented Feb 25, 2025

Personally, I don't like the <HASH/EVE_RELEASE>-<HV>-<ARCH> structure. All container registries support OCI indexes (aka docker multi-arch manifest, although they are slightly different), and even allow for different types of linked manifests (like the way docker does SBoMs and signatures, etc.). That kind of thing should just go underneath there. But it is not quite flexible enough yet to support HV and PLATFORM variants, so we cannot completely use it. And it doesn't help you for today.

<HASH/EVE_RELEASE>-<PLATFORM_VARIANT>--

Is it possible to have two tags for variant? One like today that is blank, and one that is generic? So we would have:

  • 14.5.6-kvm-amd64
  • 14.5.6-generic-kvm-amd64

They would be identical, and tags have no real cost. The reason for it is to have consistency with <HASH/EVE_RELEASE>-<PLATFORM_VARIANT>-<HV>-<ARCH> structure, so something always is in the PLATFORM_VARIANT part, but existing defaults still work.

If it is too much of a headache, don't bother, but I do think it will make things easier for us going forward.

@rene rene marked this pull request as ready for review February 25, 2025 14:35
@rene
Copy link
Contributor Author

rene commented Feb 25, 2025

Personally, I don't like the <HASH/EVE_RELEASE>-<HV>-<ARCH> structure. All container registries support OCI indexes (aka docker multi-arch manifest, although they are slightly different), and even allow for different types of linked manifests (like the way docker does SBoMs and signatures, etc.). That kind of thing should just go underneath there. But it is not quite flexible enough yet to support HV and PLATFORM variants, so we cannot completely use it. And it doesn't help you for today.

<HASH/EVE_RELEASE>-<PLATFORM_VARIANT>--

Is it possible to have two tags for variant? One like today that is blank, and one that is generic? So we would have:

* `14.5.6-kvm-amd64`

* `14.5.6-generic-kvm-amd64`

They would be identical, and tags have no real cost. The reason for it is to have consistency with <HASH/EVE_RELEASE>-<PLATFORM_VARIANT>-<HV>-<ARCH> structure, so something always is in the PLATFORM_VARIANT part, but existing defaults still work.

If it is too much of a headache, don't bother, but I do think it will make things easier for us going forward.

I understand and agree with your point @deitch . I could let the generic variant in the tag, but provide both doesn't seems to be that easy, AFAIK we can't create multiple tags easily with linuxkit as it can be done with docker. However, I can allow to force TAGPLAT, so one could use, for instance, make ZARCH=arm64 TAGPLAT=generic eve to generate the container with the generic tag... what do you think?

@deitch
Copy link
Contributor

deitch commented Feb 25, 2025

what do you think?

Not sure, how would that work?

Like I said, if it is too much of a headache, don't bother. Or maybe see if one of the easy tools can tag it after pushing.

@rene
Copy link
Contributor Author

rene commented Feb 26, 2025

what do you think?

Not sure, how would that work?

Like I said, if it is too much of a headache, don't bother. Or maybe see if one of the easy tools can tag it after pushing.

I was thinking on allow to override TAGPLAT from command line, but thinking twice I prefer to keep as it is, worst case we can manually create the tag from the publishing workflow... in anycase, this PR prepares the rootfs version and tag for platform variants, so the idea is that publishing workflow will only need to add the variants to the matrix so eve:xxx-<plat_variant>-<HV>-<ARCH> can be published as well...

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

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

Looks fine. I think it should not break anything for the generic platforms =)

@rene
Copy link
Contributor Author

rene commented Mar 3, 2025

@deitch , do you have any more comments? Otherwise it's ready for merge...

@deitch
Copy link
Contributor

deitch commented Mar 3, 2025

Nope @rene nothing to add. That Makefile is ugly and hard to read and way too complex, but that has nothing to do with this PR 😁

@rene rene merged commit 77e0e79 into lf-edge:master Mar 3, 2025
44 checks passed
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.

3 participants