Skip to content

Conversation

europaul
Copy link
Contributor

Description

This PR addresses some suggestions from the original PR that introduced vector #5008 (comment).

Remove the dependency on the minimal version of vector built externally. Now we have the full vector package built in the EVE build system, using lf-edge's eve-rust image as a base toolchain.

PR dependencies

None.

How to test and validate this PR

Only includes changes to the EVE build. No testing necessary.

Changelog notes

N/A

PR Backports

Nope.

Checklist

  • I've provided a proper description
  • I've added the proper documentation
  • I've tested my PR on amd64 device
  • I've tested my PR on arm64 device
  • I've written the test verification instructions
  • I've set the proper labels to this PR

@europaul europaul requested a review from Copilot August 13, 2025 12:54
@github-actions github-actions bot requested a review from eriknordmark August 13, 2025 12:54
@europaul europaul requested a review from rucoder August 13, 2025 12:58
@europaul
Copy link
Contributor Author

I think the comments from yetus can be disregarded.

Check Docker Hashes Consistency seems to be making a mistake: we base this Dockerfile on eve-rust:1.85.1-2, not on eve-rust:1.85.1-1

@christoph-zededa
Copy link
Contributor

Check Docker Hashes Consistency seems to be making a mistake: we base this Dockerfile on eve-rust:1.85.1-2, not on eve-rust:1.85.1-1

It is complaining that monitor is using a different image version than vector. Why would we need two different versions?

@europaul
Copy link
Contributor Author

Check Docker Hashes Consistency seems to be making a mistake: we base this Dockerfile on eve-rust:1.85.1-2, not on eve-rust:1.85.1-1

It is complaining that monitor is using a different image version than vector. Why would we need two different versions?

Those are two different tags. Both use rust 1.85.1, but the first one uses gcc for cross-compilation and the second one uses clang. If clang does a good job in the future, I think we'll use only clang version starting from the next rust version update.

@christoph-zededa
Copy link
Contributor

Check Docker Hashes Consistency seems to be making a mistake: we base this Dockerfile on eve-rust:1.85.1-2, not on eve-rust:1.85.1-1

It is complaining that monitor is using a different image version than vector. Why would we need two different versions?

Those are two different tags. Both use rust 1.85.1, but the first one uses gcc for cross-compilation and the second one uses clang. If clang does a good job in the future, I think we'll use only clang version starting from the next rust version update.

sounds like the image should be called eve-rust-gcc:... and eve-rust-clang:...

btw. the checker does not work on the diff, so if it gets merged, then every PR will be red

@OhmSpectator
Copy link
Member

Check Docker Hashes Consistency seems to be making a mistake: we base this Dockerfile on eve-rust:1.85.1-2, not on eve-rust:1.85.1-1

It is complaining that monitor is using a different image version than vector. Why would we need two different versions?

Those are two different tags. Both use rust 1.85.1, but the first one uses gcc for cross-compilation and the second one uses clang. If clang does a good job in the future, I think we'll use only clang version starting from the next rust version update.

sounds like the image should be called eve-rust-gcc:... and eve-rust-clang:...

I want to here @rucoder's opinion here then =)

@OhmSpectator
Copy link
Member

I see all the ARM builds were cancelled... Did anyone do it on purpose?

@rucoder
Copy link
Contributor

rucoder commented Aug 13, 2025

@europaul @OhmSpectator we talked about this before with @rene and eve-rust image must be excluded from that check! this is perfectly fine and expected to have different rust versions for different packages!

@christoph-zededa
Copy link
Contributor

@europaul @OhmSpectator we talked about this before with @rene and eve-rust image must be excluded from that check! this is perfectly fine and expected to have different rust versions for different packages!

Doesn't this duplicate work, f.e. checking CVEs?

Copilot

This comment was marked as outdated.

@OhmSpectator
Copy link
Member

@europaul @OhmSpectator we talked about this before with @rene and eve-rust image must be excluded from that check! this is perfectly fine and expected to have different rust versions for different packages!

Could you explain why different components require different versions of our own package? Do we have examples of other similar packages? @christoph-zededa, is there a mechanism to configure this in the checker?

@christoph-zededa
Copy link
Contributor

@europaul @OhmSpectator we talked about this before with @rene and eve-rust image must be excluded from that check! this is perfectly fine and expected to have different rust versions for different packages!

Could you explain why different components require different versions of our own package? Do we have examples of other similar packages? @christoph-zededa, is there a mechanism to configure this in the checker?

We do, see commit by @rene: 6b8a09b

RUN cp /app/target/$CARGO_BUILD_TARGET/release/vector /app/target/

# Assemble the final image
FROM alpine:3.21 AS runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

This adds a different version of busybox to the image and squashfs cannot deduplicate it then. Is it really needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest, to throw out the different version of busybox, etc. but therefore keep the debug symbols.

Copy link
Contributor

Choose a reason for hiding this comment

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

could we use eve-alpine here or 3.21 somehow is required?

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 @christoph-zededa! I'll change it to eve-alpine

@OhmSpectator OhmSpectator requested a review from Copilot August 13, 2025 14:50
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR dockerizes the build of the vector package by replacing the external dependency on a minimal vector build with a full vector package built within the EVE build system. The change uses the lf-edge eve-rust image as the base toolchain and implements a multi-stage Docker build to compile vector from source with specific features enabled.

Key changes:

  • Replaces external vector image dependency with in-house Docker build process
  • Implements multi-stage build with cross-platform support for amd64, arm64, and riscv64
  • Adds comprehensive vector feature configuration and Rust optimization flags
Comments suppressed due to low confidence (1)

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@europaul
Copy link
Contributor Author

I see all the ARM builds were cancelled... Did anyone do it on purpose?

I also found it strange...

@europaul
Copy link
Contributor Author

btw. the checker does not work on the diff, so if it gets merged, then every PR will be red

@christoph-zededa I think this problem will go away when we release the next version of eve-rust e.g. 1.86.0 (that will use clang) and use it to build both vector and monitor

btw, then we can also upgrade to vector v0.48

@christoph-zededa
Copy link
Contributor

btw. the checker does not work on the diff, so if it gets merged, then every PR will be red

@christoph-zededa I think this problem will go away when we release the next version of eve-rust e.g. 1.86.0 (that will use clang) and use it to build both vector and monitor

btw, then we can also upgrade to vector v0.48

fyi: #4664 (comment)

ARG RUST_VERSION=lfedge/eve-rust:1.85.1-2
FROM --platform=$BUILDPLATFORM ${RUST_VERSION} AS toolchain-base
ARG TARGETARCH
RUN apk update && apk add --no-cache git perl protoc
Copy link
Contributor

Choose a reason for hiding this comment

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

same, could we use eve-alpine + eve-alpine-deploy.sh script instead of RUN apk here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@europaul git perl protoc are part of eve-rust 1.85-2

@rucoder
Copy link
Contributor

rucoder commented Aug 13, 2025

@europaul @OhmSpectator we talked about this before with @rene and eve-rust image must be excluded from that check! this is perfectly fine and expected to have different rust versions for different packages!

Could you explain why different components require different versions of our own package? Do we have examples of other similar packages? @christoph-zededa, is there a mechanism to configure this in the checker?

We do, see commit by @rene: 6b8a09b

@europaul just add vector to the list mentioned by @christoph-zededa
@OhmSpectator this is normal for rust apps to pin a rust version.

@rene
Copy link
Contributor

rene commented Aug 14, 2025

BTW, since we are updating the eve-alpine package here, the go version will be updated alongside to 1.25.0.

Wow-wow-wow... Can we do it as a dedicated PR?.. It's a major change, bigger than the vector itself.

@europaul , let's not bump alpine packages here, please. You can just add the package that you need on the main file and make this change to Dockerfile:

diff --git a/pkg/alpine/Dockerfile b/pkg/alpine/Dockerfile
index 5be2a6ada..14a43d5bd 100644
--- a/pkg/alpine/Dockerfile
+++ b/pkg/alpine/Dockerfile
@@ -12,8 +12,7 @@ ARG ALPINE_VERSION=3.16
 # this is only needed once, when this package
 # is rebased on the new version of Alpine and
 # you have to have FROM alpine:x.y.z above:
-# Trigger update for go package
-RUN apk update && apk upgrade -a
+#RUN apk update && apk upgrade -a
 
 # Copy Dockerfile so we can include it in the hash
 COPY Dockerfile abuild.conf /etc/

without the apk update && apk upgrade -a the version will remain the same and it will install the inotify-tools package.

rene
rene previously requested changes Aug 14, 2025
Copy link
Contributor

@rene rene left a comment

Choose a reason for hiding this comment

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

No go version bump here. See my comment on how to proceed.

@rene
Copy link
Contributor

rene commented Aug 14, 2025

FYI I just cancelled the workflows, it would take ages to run and changes were requested to @europaul anyways.

@europaul
Copy link
Contributor Author

@rene could you explain why we shouldn't update eve-alpine normally? I don't really understand this...

@rene
Copy link
Contributor

rene commented Aug 14, 2025

@rene could you explain why we shouldn't update eve-alpine normally? I don't really understand this...

Our approach is to use eve-alpine as an image with all packages that we need cached inside (i.e., those listed on pkg/alpine/mirrors). Also, apk keeps the list of all packages available on all the repos and their versions.

When you need a package that is not installed in the eve-alpine image, you should add it to the corresponding file under pkg/alpine/mirrors/ (according to which repo the package is located, i.e., main, community, etc). Notice that add a new package doesn't update any other package.

When we want to bump all packages and their versions, for instance, when we want to bump Go version, then we need to run the following commands inside pkg/alpine/Dockerfile:

apk update && apk upgrade -a 

This was the case when I updated Alpine to 3.16.9 (458c9eb) and @christoph-zededa bumped Go version (eabd917). However, in case of this PR, we just want to add inotify-tools to be cached inside eve-alpine image and that's why you can safely comment the commands I cited above.

The point is that we don't need to bump all packages again, let's wait a bit for more stability and here you just need to add the inotify-tools package.

@europaul
Copy link
Contributor Author

@rene how about the following plan:

  1. I already create a PR to pin go toolchain version Get go from go.dev instead of alpine's edge/community mirror #5184
  2. Since we haven't found a way to just add a new package without updating the existing ones, I would next create another PR to include all dependencies like @rucoder does in Update Alpine 3.16 package lists #5175 and also include inotify-tools
  3. Then once those two PR's are merged I will rebase this one and we can proceed

@europaul europaul added the main-quest The fate of the project rests on this PR. Prioritise review to advance the storyline! label Aug 20, 2025
@europaul
Copy link
Contributor Author

UPD: I put all the requirements for this PR wrt packages and build toolchain in #5184, so as soon as it's merged, I'll rebase and we can proceed with this one.

@europaul
Copy link
Contributor Author

/rerun red

3 similar comments
@europaul
Copy link
Contributor Author

/rerun red

@europaul
Copy link
Contributor Author

/rerun red

@europaul
Copy link
Contributor Author

/rerun red

@europaul
Copy link
Contributor Author

@rene @OhmSpectator the builds ran successfully - the issue was with the ARM runners - they had 4GB of RAM which was not enough. @christoph-zededa increased it now to 8GBs.

@OhmSpectator OhmSpectator dismissed rene’s stale review August 26, 2025 10:39

The request is addressed

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.

I would like @rucoder to approve it first.
Also, what do you think about using cargo caches here to speed up the build? I have no idea how it works, know it exists. @rucoder should have a better idea.

# building the final image
FROM toolchain AS builder
ARG VECTOR_FEATURES
ADD https://github.com/vectordotdev/vector.git#v0.47.0 /app
Copy link
Member

Choose a reason for hiding this comment

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

Can we parametrise the version here?

ENV CARGO_BUILD_TARGET="aarch64-unknown-linux-musl"

FROM toolchain-base AS target-riscv64
ENV CARGO_BUILD_TARGET="riscv64gc-unknown-linux-gnu"
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to remove RISC-V "support" from the project, but for now, are you sure GNU works here? I don't care if it does not, though...

@europaul
Copy link
Contributor Author

Also, what do you think about using cargo caches here to speed up the build? I have no idea how it works, know it exists. @rucoder should have a better idea.

yeah, @rucoder what do you think about the caches? do they improve the performance just because it's a staged build or because we can reuse some parts across different building blocks?

@rucoder
Copy link
Contributor

rucoder commented Aug 26, 2025

yeah, @rucoder what do you think about the caches? do they improve the performance just because it's a staged build or because we can reuse some parts across different building blocks?

If you mean cargo chef then it is for development only. However we could export .cargo into GH cache. Idk whether it is worth it or not. Cargo directory may be huge.

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.

I would also ask @rucoder to approve or comment. Rust builds is his kingdom. Run the tests meanwhile.

Copy link
Contributor

@rucoder rucoder left a comment

Choose a reason for hiding this comment

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

LGTM

@europaul
Copy link
Contributor Author

europaul commented Sep 2, 2025

/rerun red

1 similar comment
@europaul
Copy link
Contributor Author

europaul commented Sep 3, 2025

/rerun red

To ensure consistency with the latest changes in the vector package,
we update the eve-rust version used in the monitor package.

Signed-off-by: Paul Gaiduk <[email protected]>
Remove the dependency on the minimal version of vector built externally.
Now we have the full vector package built in the EVE build system, using
lf-edge's eve-rust image as a base toolchain.

Signed-off-by: Paul Gaiduk <[email protected]>
@europaul
Copy link
Contributor Author

europaul commented Sep 4, 2025

@OhmSpectator do you think we can merge this? the CI ran already before I had to do the last rebase because of the git conflicts (actually there was no conflict, two different adjacent lines were changed)

@europaul
Copy link
Contributor Author

europaul commented Sep 5, 2025

/rerun red

1 similar comment
@europaul
Copy link
Contributor Author

europaul commented Sep 5, 2025

/rerun red

@OhmSpectator OhmSpectator merged commit a012085 into lf-edge:master Sep 8, 2025
50 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
main-quest The fate of the project rests on this PR. Prioritise review to advance the storyline!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants