fix(image): merges manifests builder stages to one #773

bartoszmajsak · 2023-12-01T09:39:32Z

Description

We can combine two build stages into one, as there is no need to always build both images to only then decide from which one we want to copy manifests to the target image. Instead manifests stage will either copy local manifests or fetches using the script based on FETCH_MANIFESTS argument (previously known as USE_LOCAL).

In addition, it opens up IMAGE_BUILD_FLAGS variable to be overwritten when executing make targets instead of changing its value in the Makefile directly, as it was previously needed.

How Has This Been Tested?

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

We can combine two build stages to one, as there is no need to always build both images to only then decide from which one we want to copy manifests to the target image. Instead `manifests` stage will either copy local manifests or fetches using the script based on `FETCH_MANIFESTS` argument (previously known as `USE_LOCAL`). In addition it opens up `IMAGE_BUILD_FLAGS` variable to be overwritten when executing make targets instead of changing its value in the `Makefile` directly, as it was previously needed.

ykaliuta · 2023-12-01T11:24:08Z

Makefile

-# set to "true" to use local instead
-# see target "image-build"
-IMAGE_BUILD_FLAGS = --build-arg USE_LOCAL=false
+IMAGE_BUILD_FLAGS ?= --build-arg FETCH_MANIFESTS=true


It does not make sense for the usage like in the README.md. Command line variables have priority in make anyway. Even over environment regardless of -e flag.

But may be having a separate variable to avoid passing --build-arg can be a good idea, what do you think?

It does not make sense for the usage like in the README.md. Command line variables have priority in make anyway. Even over environment regardless of -e flag.

That's right. Thanks for catching it. I think the source of confusion for me was not expecting to see both build stages (git clone and COPY) executed in the original form of the docker file when I was expecting to see only the one based on the flag. That's now cleaned up in 0d3a574

But may be having a separate variable to avoid passing --build-arg can be a good idea, what do you think?

I am on the verge here. I have

alias image='make image -e IMAGE_BUILD_FLAGS="--build-arg FETCH_MANIFESTS=false" -e IMG=quay.io/maistra-dev/opendatahub-operator:$$VERSION$ -e IMAGE_BUILDER=docker -e VERSION=dev-$(git branch --show-current)'

so I'm good.

Would it make it easier for you to have such a flag?

But may be having a separate variable to avoid passing --build-arg can be a good idea, what do you think?

I am on the verge here. I have

alias image='make image -e IMAGE_BUILD_FLAGS="--build-arg FETCH_MANIFESTS=false" -e IMG=quay.io/maistra-dev/opendatahub-operator:$$VERSION$ -e IMAGE_BUILDER=docker -e VERSION=dev-$(git branch --show-current)'

so I'm good.

Great! Just keep in mind that -e is redundant here, it works differently for make than to podman-run.

Would it make it easier for you to have such a flag?

No actually, I haven't ever used that in practice :). So I personally would vote to avoid the change at all. Especially because it's unrelated to the purpose of the patch.

@bartoszmajsak was thinking about avoiding go version setting in 4 places. One of the ideas was getting it from go.mod and instead of patching Dockerfile, pass it as the argument (similar to toolbox), but then your alias will be broken.

ykaliuta · 2023-12-01T11:28:41Z

Dockerfiles/Dockerfile

 ARG GOLANG_VERSION=1.19
-ARG USE_LOCAL=false
+ARG FETCH_MANIFESTS=true


Does it make sense to leave it here (except as a matter of documentation) since according to the Dockerfile documentation ARG is valid till the next FROM (USE_LOCAL was expanded in the FROM line itself before)?

That's right, I cleared it up in 51d8c5d

ykaliuta · 2023-12-01T12:09:52Z

It actually did not build both images (if I understand my build logs correctly) due to dependency tracking. But the change makes Dockerfile much cleaner. A question I have -- does it make sense to avoid that manifest stage and do it in the bulider? It used go-toolset image to make manifests before.

bartoszmajsak · 2023-12-01T14:10:28Z

It actually did not build both images (if I understand my build logs correctly) due to dependency tracking.

@ykaliuta Ok I'm triple puzzled now, have a look at the build log below

docker build

❯ make image -e IMG=quay.io/maistra-dev/opendatahub-operator:$\(VERSION\) -e IMAGE_BUILDER=docker -e VERSION=
$(git branch --show-current)                                                                                 
docker build --no-cache -f Dockerfiles/Dockerfile  --build-arg USE_LOCAL=true -t quay.io/maistra-dev/opendata
hub-operator:incubation .                                                                                    
Sending build context to Docker daemon  48.51MB                                                              
Step 1/33 : ARG GOLANG_VERSION=1.19                                                                          
Step 2/33 : ARG USE_LOCAL=false                                                                              
Step 3/33 : ARG OVERWRITE_MANIFESTS=""                                                                       
Step 4/33 : FROM registry.access.redhat.com/ubi8/go-toolset:$GOLANG_VERSION as builder_local_false           
 ---> 8d5f48f7fee4                                                                                           
Step 5/33 : ARG OVERWRITE_MANIFESTS                                                                          
 ---> Running in fd2771df5f75                                                                                
Removing intermediate container fd2771df5f75                                                                 
 ---> b149d829c88a                                                                                           
Step 6/33 : USER root                                                                                        
 ---> Running in 05dd34e299fa                                                                                
Removing intermediate container 05dd34e299fa
 ---> c065dbd4d8e2
Step 7/33 : WORKDIR /opt
 ---> Running in fbdfc66830ab
Removing intermediate container fbdfc66830ab
 ---> 5b00defbec40
Step 8/33 : COPY get_all_manifests.sh get_all_manifests.sh
 ---> 356097080b83
Step 9/33 : RUN ./get_all_manifests.sh ${OVERWRITE_MANIFESTS}
 ---> Running in 7f856e101399
Cloning repo kserve: opendatahub-io:kserve:release-v0.11.0:config:kserve
Cloning into './.kserve'... 
Cloning repo odh-dashboard: opendatahub-io:odh-dashboard:incubation:manifests:dashboard
Cloning into './.odh-dashboard'...
Cloning repo odh-notebook-controller: opendatahub-io:kubeflow:v1.7-branch:components/odh-notebook-controller/config:odh-notebook-controller/odh-notebook-controller 
Cloning into './.kubeflow'...
Cloning repo notebooks: opendatahub-io:notebooks:main:manifests:notebooks
Cloning into './.notebooks'...
Cloning repo odh-model-controller: opendatahub-io:odh-model-controller:release-0.11.0:config:odh-model-controller
Cloning into './.odh-model-controller'...
Cloning repo ray: opendatahub-io:kuberay:master:ray-operator/config:ray
Cloning into './.kuberay'...
Cloning repo kf-notebook-controller: opendatahub-io:kubeflow:v1.7-branch:components/notebook-controller/config:odh-notebook-controller/kf-notebook-controller
Cloning into './.kubeflow'...
Cloning repo data-science-pipelines-operator: opendatahub-io:data-science-pipelines-operator:main:config:data-science-pipelines-operator 
Cloning into './.data-science-pipelines-operator'...
Cloning repo trustyai: trustyai-explainability:trustyai-service-operator:release/1.10.2:config:trustyai-service-operator
Cloning into './.trustyai-service-operator'...
Cloning repo model-mesh: opendatahub-io:modelmesh-serving:release-0.11.0:config:model-mesh
Cloning into './.modelmesh-serving'...
Cloning repo codeflare: opendatahub-io:codeflare-operator:main:config:codeflare
Cloning into './.codeflare-operator'...
Removing intermediate container 7f856e101399
 ---> a1671278199f
Step 10/33 : FROM registry.access.redhat.com/ubi8/go-toolset:$GOLANG_VERSION as builder_local_true
 ---> 8d5f48f7fee4
Step 11/33 : USER root
 ---> Running in 4f8948077214
Removing intermediate container 4f8948077214
 ---> d20e290c3c70
Step 12/33 : WORKDIR /opt
 ---> Running in d1a749b1d44d
Removing intermediate container d1a749b1d44d
 ---> 9b304fee78da
Step 13/33 : COPY odh-manifests/ /opt/odh-manifests/
 ---> 9d406db3f325
Step 14/33 : FROM builder_local_${USE_LOCAL} as builder
```

docker version

Client: Version: 20.10.17 API version: 1.41 Go version: go1.16.15 Git commit: aa7e414 Built: Fri Jul 8 23:32:01 2022 OS/Arch: linux/amd64 Context: default Experimental: true

Server:
 Engine:
  Version:          20.10.17
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.16.15
  Git commit:       f756502
  Built:            Fri Jul  8 23:32:01 2022
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.6.14
  GitCommit:        9ba4b250366a5ddde94bb7c9d1def331423aa323
 runc:
  Version:          1.1.4
  GitCommit:        v1.1.4-0-g5fd4c4d
 docker-init:
  Version:          0.19.0
  GitCommit:

ykaliuta · 2023-12-01T14:51:14Z

It actually did not build both images (if I understand my build logs correctly) due to dependency tracking.

@ykaliuta Ok I'm triple puzzled now, have a look at the build log below

Hah, interesting. Looks like podman is more clever. But I remember I read about dependencies in docker as well in the docs :) Makes more sense then. But what about squashing it one phase then? AFAIU it was splitted into two phases to make that condition on the image builder level. With the patch when it's in the script there is no sense for additional phase. Or I'm missing something?

podman log

```podman build --no-cache -f Dockerfiles/Dockerfile --build-arg USE_LOCAL=false -t quay.io/ykaliuta/opendatahub-operator:latest . [1/4] STEP 1/6: FROM registry.access.redhat.com/ubi8/go-toolset:1.19 AS builder_local_false [1/4] STEP 2/6: ARG OVERWRITE_MANIFESTS --> 0658cce527fa [1/4] STEP 3/6: USER root --> 6d2a0b9363d0 [1/4] STEP 4/6: WORKDIR /opt --> 3bd13acfa0fb [1/4] STEP 5/6: COPY get_all_manifests.sh get_all_manifests.sh --> fc074ef2b318 [1/4] STEP 6/6: RUN ./get_all_manifests.sh ${OVERWRITE_MANIFESTS} Cloning repo kserve: opendatahub-io:kserve:release-v0.11.0:config:kserve Cloning into './.kserve'... Cloning repo odh-dashboard: opendatahub-io:odh-dashboard:incubation:manifests:dashboard Cloning into './.odh-dashboard'... Cloning repo odh-notebook-controller: opendatahub-io:kubeflow:v1.7-branch:components/odh-notebook-controller/config:odh-notebook-controller/odh-notebook-controller Cloning into './.kubeflow'... Cloning repo notebooks: opendatahub-io:notebooks:main:manifests:notebooks Cloning into './.notebooks'... Cloning repo odh-model-controller: opendatahub-io:odh-model-controller:release-0.11.0:config:odh-model-controller Cloning into './.odh-model-controller'... Cloning repo ray: opendatahub-io:kuberay:master:ray-operator/config:ray Cloning into './.kuberay'... Cloning repo kf-notebook-controller: opendatahub-io:kubeflow:v1.7-branch:components/notebook-controller/config:odh-notebook-controller/kf-notebook-controller Cloning into './.kubeflow'... Cloning repo data-science-pipelines-operator: opendatahub-io:data-science-pipelines-operator:main:config:data-science-pipelines-operator Cloning into './.data-science-pipelines-operator'... Cloning repo trustyai: trustyai-explainability:trustyai-service-operator:release/1.10.2:config:trustyai-service-operator Cloning into './.trustyai-service-operator'... Cloning repo model-mesh: opendatahub-io:modelmesh-serving:release-0.11.0:config:model-mesh Cloning into './.modelmesh-serving'... Cloning repo codeflare: opendatahub-io:codeflare-operator:main:config:codeflare Cloning into './.codeflare-operator'... --> a0196c56ddc5 [3/4] STEP 1/13: FROM a0196c56ddc53a2f62783f1a40a0ecc47ec0c05055fb7f7dbe1afa161d9ede1d AS builder [3/4] STEP 2/13: USER root --> 92530b5fda6a [3/4] STEP 3/13: WORKDIR /workspace --> 262e23812a42 [3/4] STEP 4/13: COPY go.mod go.mod --> e39a33962a8b [3/4] STEP 5/13: COPY go.sum go.sum --> d145cda8323c [3/4] STEP 6/13: RUN go mod download --> 4a2bee6f7ac5 [3/4] STEP 7/13: COPY apis/ apis/ --> 432be0644019 [3/4] STEP 8/13: COPY components/ components/ --> 8d409afb27fd [3/4] STEP 9/13: COPY controllers/ controllers/ --> b046471d1ddf [3/4] STEP 10/13: COPY main.go main.go --> 35582a002821 [3/4] STEP 11/13: COPY pkg/ pkg/ --> 2c2841d130be [3/4] STEP 12/13: COPY infrastructure/ infrastructure/ --> 405d2af8cba9 [3/4] STEP 13/13: RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o manager main.go --> b4b01e4fb965 [4/4] STEP 1/7: FROM registry.access.redhat.com/ubi8/ubi-minimal:latest [4/4] STEP 2/7: WORKDIR / --> 5a1ddd47a55a [4/4] STEP 3/7: COPY --from=builder /workspace/manager . --> 4873e93cdc67 [4/4] STEP 4/7: COPY --chown=1001:0 --from=builder /opt/odh-manifests /opt/manifests --> 4fc738ad01f0 [4/4] STEP 5/7: RUN chown -R 1001:0 /opt/manifests && chmod -R a+r /opt/manifests --> 984a23fbba03 [4/4] STEP 6/7: USER 1001 --> ff88d059ae68 [4/4] STEP 7/7: ENTRYPOINT ["/manager"] [4/4] COMMIT quay.io/ykaliuta/opendatahub-operator:latest --> e154b0975db4 Successfully tagged quay.io/ykaliuta/opendatahub-operator:latest e154b0975db48709bbcb7aa5b5f3f8da65fddbf404beb7bde816a6c6b08e2328 ```

bartoszmajsak · 2023-12-01T16:45:44Z

Hah, interesting. Looks like podman is more clever. But I remember I read about dependencies in docker as well in the docs :) Makes more sense then.

I assumed podman is clever, but I'm on a very outdated fedora on my main workstation and podman ain't happy about it :)

But what about squashing it one phase then? AFAIU it was splitted into two phases to make that condition on the image builder level. With the patch when it's in the script there is no sense for additional phase. Or I'm missing something?

Let's not start this squash discussion again! Just kidding :D Personally, I like such separation, but if we significantly trim on execution time and image sizes, then I'm happy to do it. WDYT @zdtsw?

bartoszmajsak · 2023-12-04T06:58:56Z

/test opendatahub-operator-e2e

bartoszmajsak · 2023-12-04T06:59:39Z

@ykaliuta @zdtsw Is there anything else to be improved in this PR?

zdtsw

/lgtm

openshift-ci · 2023-12-04T07:21:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zdtsw
Once this PR has been reviewed and has the lgtm label, please assign etirelli 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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ykaliuta · 2023-12-04T07:24:40Z

/lgtm

zdtsw · 2023-12-13T06:19:16Z

/test opendatahub-operator-e2e

openshift-ci · 2024-01-04T15:52:14Z

New changes are detected. LGTM label has been removed.

bartoszmajsak · 2024-01-08T09:11:45Z

/retest

bartoszmajsak · 2024-01-11T10:09:38Z

/test opendatahub-operator-e2e

openshift-ci · 2024-04-24T16:45:50Z

@bartoszmajsak: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/opendatahub-operator-pr-image-mirror	`da6dd48`	link	true	`/test opendatahub-operator-pr-image-mirror`
ci/prow/ci-index	`da6dd48`	link	true	`/test ci-index`
ci/prow/opendatahub-operator-e2e	`da6dd48`	link	true	`/test opendatahub-operator-e2e`
ci/prow/images	`da6dd48`	link	true	`/test images`

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

ykaliuta · 2024-11-19T09:04:27Z

The PR is still in my github list of assignment. It was approved, looks good to me as well, but was not merged due to failing tests.
@bartoszmajsak can we rebase and finish it?

bartoszmajsak · 2024-11-19T09:45:01Z

@ykaliuta Unfortunately, I don't have the bandwidth to dive into a year-old PR at the very moment. That said, I’d like to know if this approach is still worth it. If it does I can squeeze in some time in the coming days.

We can combine two build stages into one, as there is no need to always build both images (not done by podman) to only then decide from which one we want to copy manifests to the target image. Instead manifests stage will either copy local manifests or fetches using the script based on USE_LOCAL argument. Move USE_LOCAL and OVERWIRTE_MANIFESTS args under FROM since args have scope of the FROM they are declared in. It requires opt/manifests directory to exist, but since it's a part of git repo, it's fine. Original patch from: Bartosz Majsak <[email protected]> [1] [1] opendatahub-io#773 Signed-off-by: Yauheni Kaliuta <[email protected]>

ykaliuta · 2024-11-19T11:39:47Z

@ykaliuta Unfortunately, I don't have the bandwidth to dive into a year-old PR at the very moment. That said, I’d like to know if this approach is still worth it. If it does I can squeeze in some time in the coming days.

Well, you are the customer of that, with podman nobody noticed :)

Ok, I fetched the main change into #1381 avoiding interface changes.

bartoszmajsak · 2024-11-19T17:09:55Z

Thanks @ykaliuta! Closing in favor of #1381

We can combine two build stages into one, as there is no need to always build both images (not done by podman) to only then decide from which one we want to copy manifests to the target image. Instead manifests stage will either copy local manifests or fetches using the script based on USE_LOCAL argument. Move USE_LOCAL and OVERWIRTE_MANIFESTS args under FROM since args have scope of the FROM they are declared in. It requires opt/manifests directory to exist, but since it's a part of git repo, it's fine. Original patch from: Bartosz Majsak <[email protected]> [1] [1] opendatahub-io#773 Signed-off-by: Yauheni Kaliuta <[email protected]>

openshift-ci bot requested review from etirelli and LaVLaS December 1, 2023 09:39

bartoszmajsak requested review from zdtsw and ykaliuta and removed request for etirelli and LaVLaS December 1, 2023 09:39

bartoszmajsak force-pushed the builder_img_stages branch from db658ef to b7735c3 Compare December 1, 2023 09:41

ykaliuta reviewed Dec 1, 2023

View reviewed changes

bartoszmajsak added 2 commits December 1, 2023 13:48

fix: removes redundant ARGs

51d8c5d

chore: reverts var declaration

0d3a574

bartoszmajsak requested a review from ykaliuta December 1, 2023 12:52

Merge branch 'incubation' into builder_img_stages

194684d

zdtsw approved these changes Dec 4, 2023

View reviewed changes

openshift-ci bot assigned zdtsw Dec 4, 2023

openshift-ci bot added the lgtm label Dec 4, 2023

openshift-ci bot assigned ykaliuta Dec 4, 2023

zdtsw enabled auto-merge (squash) December 13, 2023 06:20

Merge branch 'incubation' into builder_img_stages

da6dd48

openshift-ci bot removed the lgtm label Jan 4, 2024

ykaliuta mentioned this pull request Nov 19, 2024

Dockerfile: merges manifests builder stages to one #1381

Open

5 tasks

bartoszmajsak closed this Nov 19, 2024

auto-merge was automatically disabled November 19, 2024 17:09
Pull request was closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(image): merges manifests builder stages to one #773

fix(image): merges manifests builder stages to one #773

bartoszmajsak commented Dec 1, 2023

ykaliuta Dec 1, 2023

bartoszmajsak Dec 1, 2023

ykaliuta Dec 1, 2023

ykaliuta Mar 22, 2024

ykaliuta Dec 1, 2023

bartoszmajsak Dec 1, 2023

ykaliuta commented Dec 1, 2023

bartoszmajsak commented Dec 1, 2023

ykaliuta commented Dec 1, 2023 •

edited

Loading

bartoszmajsak commented Dec 1, 2023

bartoszmajsak commented Dec 4, 2023

bartoszmajsak commented Dec 4, 2023

zdtsw left a comment

openshift-ci bot commented Dec 4, 2023

ykaliuta commented Dec 4, 2023

zdtsw commented Dec 13, 2023

openshift-ci bot commented Jan 4, 2024

bartoszmajsak commented Jan 8, 2024

bartoszmajsak commented Jan 11, 2024

openshift-ci bot commented Apr 24, 2024

ykaliuta commented Nov 19, 2024

bartoszmajsak commented Nov 19, 2024

ykaliuta commented Nov 19, 2024

bartoszmajsak commented Nov 19, 2024

fix(image): merges manifests builder stages to one #773

fix(image): merges manifests builder stages to one #773

Conversation

bartoszmajsak commented Dec 1, 2023

Description

How Has This Been Tested?

Merge criteria:

ykaliuta Dec 1, 2023

Choose a reason for hiding this comment

bartoszmajsak Dec 1, 2023

Choose a reason for hiding this comment

ykaliuta Dec 1, 2023

Choose a reason for hiding this comment

ykaliuta Mar 22, 2024

Choose a reason for hiding this comment

ykaliuta Dec 1, 2023

Choose a reason for hiding this comment

bartoszmajsak Dec 1, 2023

Choose a reason for hiding this comment

ykaliuta commented Dec 1, 2023

bartoszmajsak commented Dec 1, 2023

ykaliuta commented Dec 1, 2023 • edited Loading

bartoszmajsak commented Dec 1, 2023

bartoszmajsak commented Dec 4, 2023

bartoszmajsak commented Dec 4, 2023

zdtsw left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 4, 2023

ykaliuta commented Dec 4, 2023

zdtsw commented Dec 13, 2023

openshift-ci bot commented Jan 4, 2024

bartoszmajsak commented Jan 8, 2024

bartoszmajsak commented Jan 11, 2024

openshift-ci bot commented Apr 24, 2024

ykaliuta commented Nov 19, 2024

bartoszmajsak commented Nov 19, 2024

ykaliuta commented Nov 19, 2024

bartoszmajsak commented Nov 19, 2024

ykaliuta commented Dec 1, 2023 •

edited

Loading