Skip to content

Keepalived: Fix used alpine image #138

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 9, 2025
Merged

Keepalived: Fix used alpine image #138

merged 5 commits into from
Apr 9, 2025

Conversation

UnholyRope
Copy link
Contributor

Fixes the keepalived extension to be built using a valid alpine image.

t-lo
t-lo reviewed Apr 1, 2025
View reviewed changes
keepalived.sysext/create.sh Outdated
@@ -19,7 +19,7 @@ function populate_sysext_root() {
local img_arch="$(arch_transform 'x86-64' 'amd64' "$arch")"
img_arch="$(arch_transform 'arm64' 'arm64/v8' "$img_arch")"

local image="docker.io/${img_arch}/alpine:3.19"
local image="docker.io/${img_arch}/alpine:3.21"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
local image="docker.io/${img_arch}/alpine:3.21"
local image="docker.io/alpine:3.21"

as we explicitly specify --platform on the docker run command line. For some reason the original version even workd for x86-64 but breaks for arm64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. I updated it accordingly.

@t-lo
Copy link
Member

t-lo commented Apr 1, 2025

Thank you for your fix! This issue must have snuck in during the previous /usr/sbin fix. One minor nit to make arm64 builds work (see comment), otherwise the PR is good to go.

BTW you can test arm64 builds on non-native architectures, e.g. x86-64, if you have binfmt set up on the build host. Our bakery CI works that way.

If you like you can even re-add "keeplived latest" release_build_versions.txt as the most recent keepalived release, 2.3.3., fixes a build issue on Alpine that reproducibly broke the 2.3.2 build.

@t-lo t-lo self-requested a review April 1, 2025 18:18
@UnholyRope
Copy link
Contributor Author

Thank you for your fix! This issue must have snuck in during the previous /usr/sbin fix. One minor nit to make arm64 builds work (see comment), otherwise the PR is good to go.

BTW you can test arm64 builds on non-native architectures, e.g. x86-64, if you have binfmt set up on the build host. Our bakery CI works that way.

If you like you can even re-add "keeplived latest" release_build_versions.txt as the most recent keepalived release, 2.3.3., fixes a build issue on Alpine that reproducibly broke the 2.3.2 build.

I added keepalived latest back the release_buld_versions.txt and ran the pipeline. The build itself works but the release body is too long:

Error: Validation Failed: {"resource":"Release","code":"custom","field":"body","message":"body is too long (maximum is 125000 characters)"} - https://docs.github.com/rest/releases/releases#create-a-release

@t-lo
Copy link
Member

t-lo commented Apr 3, 2025 •
edited
Loading

Ah, okay. Looks like we should capture the build log in a file and make it part of the release artefacts instead of the release text.

That should be a separate PR though.

@UnholyRope
Copy link
Contributor Author

I removed the build output from the release as it can be viewed in the pipeline if needed.
The extension releases now look like this: https://github.com/UnholyRope/sysext-bakery/releases/tag/keepalived-v2.3.3.

t-lo
t-lo approved these changes Apr 9, 2025
View reviewed changes
Copy link
Member

@t-lo t-lo left a comment

Choose a reason for hiding this comment

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

LGTM, tested the builds locally. Thank you @UnholyRope !

@t-lo t-lo merged commit a6fb21e into flatcar:main Apr 9, 2025
@t-lo
Copy link
Member

t-lo commented Apr 9, 2025

Merging, thanks for the good work! Will follow up with a PR to CI release automation to capture build output into a separate log file.

t-lo added a commit that referenced this pull request Apr 9, 2025
@t-lo
GitHub release builds: build log in files, add dates
3e62601 
This change re-adds build logs to releases but uses separate files
instead of adding the log to Release.md.

This addresses a corner case where Release.md grew too large to be used
as release text (see
#138 (comment)).

Also, the PR adds build timestamps to release texts as well as removes
the contents of SHA256SUMS from the SHA256SUMS metadata Release.md to
avoid potential issues in the future as our release count grows. The
SHA256SUMS file is part of that release, no need to repeat it in
Release.md

Signed-off-by: Thilo Fromm <[email protected]>
@t-lo t-lo mentioned this pull request Apr 9, 2025
Merged
@github-actions github-actions bot mentioned this pull request Apr 22, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-lo t-lo t-lo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@UnholyRope @t-lo