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

Inline RPM dependencies in container image build #1510

Closed
wants to merge 9 commits into from

Conversation

gsaslis
Copy link
Member

@gsaslis gsaslis commented Nov 28, 2024

This PR introduces a radically different - and much more wholistic - approach to producing the apicast container image.

The current build relies on RPM packages that must have been built and pushed in a repository, so they can be pulled and installed into this image. However, this poses several issues with regards to how those RPMs are built, how easy they are to update, etc. etc.

The approach here is not entirely new - it comes from the product build, which has already adopted this multi-stage approach in building RPMs within the container image and then installing them in the final produced image.
It should be noted that - eventually - we could remove the RPM packaging altogether and essentially convert the RPM spec files into Containerfile instructions. This would further simplify the build steps. In this PR, we are only taking one first step towards that.

Some notes:

  • all dependencies have either been added as git submodules, or vendored into the repo,
  • all RPM builds now happen in a multi-stage build,
  • this is just the first version of the Containerfile that I was able to get a passing build on, with podman build -f Containerfile .
  • there are still several improvements to be implemented in the multi-stage build, to reduce some duplication. The previous build system didn't allow reusing previous build stages in subsequent ones (i.e. it didn't allow FROM rpm-builder AS openresty, etc.), so that still needs some clearing up.
  • we should probably only keep one of Containerfile and Dockerfile. I just didn't overwrite the Dockerfile in one go to allow for a better chance of review.

Open issue: the jaegertracing-cpp-client RPM is currently commented out. That hadn't previously been ported to its own build stage and I don't know if it's worth doing that now. Luckily, support for opentracing is planned to be dropped in the next 3scale 2.16 release (considering we now have support for opentelemetry), so those lines could probably be removed altogether.

@tkan145 I am creating this as a draft PR, so we can slowly start reviewing it.

(Note: while I was able to build a container image, I haven’t tested it in any way yet. I’d also like your help in figuring out if there is anything important missing in the new Containerfile from the existing Dockerfile ).

@gsaslis gsaslis changed the base branch from master to 3scale-2.15-stable November 28, 2024 16:59
@tkan145
Copy link
Contributor

tkan145 commented Dec 2, 2024

@gsaslis Overall LGTM. If this target is 2.15 then I'm afraid we still need to support opentracing.

I noticed that the CI jobs are failing, I will look into it but it doesn't seem related to this commit

@gsaslis
Copy link
Member Author

gsaslis commented Dec 2, 2024

If this target is 2.15 then I'm afraid we still need to support opentracing.

That's a good point!! I'll create the 2.16 branch to point at HEAD of 2.15 and use that as the target branch. ;)

@gsaslis gsaslis changed the base branch from 3scale-2.15-stable to 3scale-2.16-stable December 2, 2024 07:19
@tkan145
Copy link
Contributor

tkan145 commented Dec 4, 2024

@gsaslis would you mind to rebase this PR on top of master branch? I think it's better to merge it into master, merging into 2.16 now and cherry-picking commits from master later can be hairy.

Also 3scale-2.16-stable is currently based on 2.15 branch instead of master

@gsaslis
Copy link
Member Author

gsaslis commented Dec 4, 2024

Also 3scale-2.16-stable is currently based on 2.15 branch instead of master

shouldn't it be ? I mean... doesn't the latest release carry over from the previous one ?

I'm not sure what your current branching strategy here is, but for the other components we usually avoid cherry-picking individual commits and we just fast-forward to the HEAD of main/master branch.

Having said that, please feel free to move 3scale-2.16-stable to point wherever you like. In the product build I'll just point to 3scale-2.16-stable.

Signed-off-by: Yorgos Saslis <[email protected]>
Signed-off-by: Yorgos Saslis <[email protected]>
the RPM builds fine without this package. Finding the `rpmdevtools` package is a problem anyway because it's part of the entitled content - and not available in UBI repos.

Signed-off-by: Yorgos Saslis <[email protected]>
- all RPM builds happen in a multi-stage build
- this is just the first version of the Containerfile that I was able to get a passing build on, with `podman build -f Containerfile .`
- there are still several improvements to be implemented in the multi-stage build, to reduce some duplication. The previous build system didn't allow reusing previous build stages in subsequent ones (i.e. it didn't allow `FROM rpm-builder AS openresty`, etc.), so that still needs some clearing up.
- we should probably only keep one of `Containerfile` and `Dockerfile`. I just didn't overwrite the `Dockerfile` in one go to allow for a better chance of review.

Open issue: the jaegertracing-cpp-client RPM is currently commented out. That hadn't previously been ported to its own build stage and I don't know if it's worth doing that now. Hopefully, support for opentracing can be dropped in the next release (considering we now have support for opentelemetry).

Signed-off-by: Yorgos Saslis <[email protected]>
Signed-off-by: Yorgos Saslis <[email protected]>
@gsaslis gsaslis changed the base branch from 3scale-2.16-stable to master December 4, 2024 09:22
@gsaslis gsaslis marked this pull request as ready for review December 4, 2024 09:22
@gsaslis gsaslis requested a review from a team as a code owner December 4, 2024 09:22
@tkan145
Copy link
Contributor

tkan145 commented Dec 5, 2024

shouldn't it be ? I mean... doesn't the latest release carry over from the previous one ?
Normally it would be, but the current 2.15 branch includes CVE backport commits so it's not as clean as expected and fast-forwarding would cause conflicts.

So I tend to base new branch on master and fast-forward later.

Having said that, please feel free to move 3scale-2.16-stable to point wherever you like. In the product build I'll just point to 3scale-2.16-stable.

Thanks mate

@gsaslis
Copy link
Member Author

gsaslis commented Dec 5, 2024

closing in favour of #1514

(branch name was causing jobs to fail)

@gsaslis gsaslis closed this Dec 5, 2024
@gsaslis gsaslis deleted the feat/better_build branch December 5, 2024 11:26
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.

2 participants