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

(WIP) ✨ speed up container image builds #4289

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mateusoliveira43
Copy link
Contributor

Why the changes were made

When building a container image for different arch than you are on, if --platform=${BUILDPLATFORM} is not in FROM statement in Dockerfile, build will take longer.

On a project I work, we build container images for testing in clusters that can either be amd64 or arm64. So, sometimes the developer is on the same arch they need to build, sometimes not.

docker-buildx could fix the problem, but would build an unnecessary extra arch (for delivering the product, it does make sense to create all archs possible, but for developing, just building teh arch you need make developemt cycle faster).

How to test the changes made

Run docker-build docker-push and docker-buildx commands and check if they still work (and are not slower).

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mateusoliveira43
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 4, 2024
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 4, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @mateusoliveira43. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

- $(CONTAINER_TOOL) buildx create --name project-builder
$(CONTAINER_TOOL) buildx use project-builder
- $(CONTAINER_TOOL) buildx build --push --platform=$(PLATFORMS) --tag ${IMG} -f Dockerfile.cross .
- $(CONTAINER_TOOL) buildx build --push --platform=$(PLATFORMS) --tag ${IMG} .
- $(CONTAINER_TOOL) buildx rm project-builder
Copy link
Member

Choose a reason for hiding this comment

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

This code was added to ensure it functions as expected, specifically enabling Docker to generate images with support for all the platforms listed above. Could you confirm that everything will continue to work as intended after your changes?

To move forward, could we please add CI tests? We could create a new GitHub workflow (like build) that builds a sample, triggers this target, and verifies that the generated image supports all required platforms. What do you think? Does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, CI approach is good, will see how to create a test for it

Copy link
Member

@camilamacedo86 camilamacedo86 Nov 5, 2024

Choose a reason for hiding this comment

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

I just want to share with you that it is implemented as it is because, as far as I remember, the suggestion you are making here did not work well. So, for any changes here, we really need to properly ensure and with a CI so that we can ensure that we have no regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created separate PR for CI #4381

my idea is merge that one first (ensure buildx commands works in master branch), then check that this change does not break the job

what you think?

Copy link
Member

@camilamacedo86 camilamacedo86 Nov 22, 2024

Choose a reason for hiding this comment

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

Did you run it manually?
Did you check that the final image has all platforms?
If so, can you please add in the description?

Then I was thinking we might not need the CI.. we could skip that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can test (my primary machine does not have docker to test, but I have another one that has it) and post results

Copy link
Member

Choose a reason for hiding this comment

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

I was the one who originally implemented this code, and I recall attempting the approach you proposed here. It didn’t work across all scenarios, which is why the solution ended up being the Dockerfile.cross.

I tried to locate the comments from the original PR (link) that explain why we use Dockerfile.cross to provide more context, but I couldn’t find them quickly. However, I’m confident that the proposed changes won’t address all requirements and could potentially break existing functionality.

For these reasons, I don’t think we should make this change.

Copy link
Contributor Author

@mateusoliveira43 mateusoliveira43 Nov 22, 2024

Choose a reason for hiding this comment

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

I found this https://github.com/hadolint/hadolint/wiki/DL3029 that would also go against these changes 😬

investigated that and the rule is only for fixed platforms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I think we do not need to worry about make docker-buildx command with this change. Basically this changes Dockerfile to be equal to the Dockerfile.cross that would be created by the command

Copy link
Member

@camilamacedo86 camilamacedo86 Nov 23, 2024

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 I fully understand what you mean, but I believe we should avoid changing this code unless we are 100% certain the change will bring value and address all requirements. When this code was originally written, it was reviewed by three experts with extensive experience in this area, and they identified and addressed many issues and scenarios.

It’s hard to recall all the details now since it was implemented over two years ago. I’m open to any changes or improvements that add value, but I’m concerned that, in this case, we might inadvertently introduce an issue. Ditto, if you really want to proceed with this change, I strongly suggest testing all use cases locally first. We’ll also need to track everything in the description to ensure that all use cases are still functioning correctly, which is even more critical than relying on CI.

@camilamacedo86 camilamacedo86 changed the title ✨ speed up container image builds (WIP) ✨ speed up container image builds Nov 9, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2024
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

First we will need a GitHub Action to validate those changes.
So, it blocks this one.
I hope that you do not mind, I add WIP to title of the PR to clarify that it still in WIP

@mateusoliveira43
Copy link
Contributor Author

no problem @camilamacedo86

I am busy this week, but next week I will be able to finish this PR :)

@@ -1,5 +1,6 @@
# Build the manager binary
FROM golang:1.22 AS builder
FROM --platform=${BUILDPLATFORM} golang:1.22 AS builder
Copy link
Member

@camilamacedo86 camilamacedo86 Nov 22, 2024

Choose a reason for hiding this comment

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

Hi @mateusoliveira43,

I believe we cannot proceed with these changes. If I run the build on a Mac, it won't work properly when deploying to the cluster, correct? The Dockerfile is also used for the docker-build target, so this impacts more than just docker-buildx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my intention with this change is exactly this scenario with docker-build command

In the project I work, we build Linux images for developing, but some developers use MAC

We added this change to our Dockerfile to speed up the builds there for non Linux users

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants