generate files in dockerfiles #219

cpmeadors · 2022-09-22T16:18:14Z

These changes add multistage docker files for both the operator and the operator bundle images. The builder stage uses the make targets to generate the necessary files to build the two images. This should fix the Prow CI image build issues and it should allow the copy of the bundle files in the midstream repo to be removed. Ultimately it means the same process is being use to build the images no matter what is building them.

Outstanding issues are the need for "go mod vendor" and the hard coding of the VERSION for the bundle.

openshift-ci · 2022-09-22T16:18:18Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cpmeadors · 2022-09-22T16:18:31Z

/test all

cpmeadors · 2022-09-29T18:33:05Z

/test ci-index-sandboxed-containers-operator-bundle

bear-redhat · 2022-09-30T14:01:11Z

/test ci-index-sandboxed-containers-operator-bundle

bundle.Dockerfile

jensfr · 2022-10-05T20:34:02Z

bundle.Dockerfile

+LABEL operators.operatorframework.io.bundle.manifests.v1=manifests/
+LABEL operators.operatorframework.io.bundle.metadata.v1=metadata/
+LABEL operators.operatorframework.io.bundle.package.v1=sandboxed-containers-operator
+LABEL operators.operatorframework.io.bundle.channels.v1=alpha


should be stable-1.3?

jensfr

lgtm, except for two small nits

Dockerfile

pmores · 2022-10-06T10:11:39Z

Dockerfile

-COPY go.sum go.sum
-# cache deps before building and copying source so that we don't need to re-download as much
-# and so that source changes don't invalidate our downloaded layer
+COPY ./ ./


I'm not sure about this change. I'm afraid it will greatly and unnecessarily increase the size of the resulting layer which might not be an issue on build machines but can be an impediment for development in my experience. I know that even without this change, when I worked on this Dockerfile it accumulated over a hundred of gigs in layer cache during just a couple of hour of work so I had to purge periodically, with the obvious bad effects on the next build time.

I think this is more efficient than doing multiple COPY to most of the dirs. Fewer commands in a dockerfile is generally a good thing. I find I have to purge docker caches/images a lot when I do any dockerfile development

we could also add .dockerignore file to limit what is copied.

I'd really like to make sure here that the dev cycle is not made worse unnecessarily. As is, I believe this change is practically guaranteed to cause that as the COPY ./ ./ command will create a big layer that's going to be invalidated, rebuilt and re-cached every single time since it contains the sources that are changing because they're being worked on.

You're right that fewer commands is a good thing but that's precisely for the reason of avoiding creation of unnecessary layers (the cost of the COPY commands in terms of execution time tends to be negligible and dominated by other commands in the Dockerfile by several orders of magnitude). Also, notice how the COPY commands in the original version are ordered so that things that rarely change go first - this makes sure that after an initial build these COPY commands aren't actually performed at all on subsequent builds and a cached layer is used instead.

It's true that we can achieve the same with .dockerignore although I suspect that in this case that would be more tedious than just copying only what's needed. But I don't mind either way.

pmores · 2022-10-06T10:26:42Z

Thanks @cpmeadors and kudos for figuring this out! I left a bunch of comments but also overall, I'd like to make sure that the efficiency of the originals (esp. in case of Dockerfile) in terms of layering and disk space requirements is preserved.

cpmeadors · 2022-10-06T16:59:42Z

config/manager/kustomization.yaml

@@ -12,5 +12,4 @@ apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 images:
 - name: controller
-  newName: controller
-  newTag: latest
+  newName: openshift-sandboxed-containers-operator


This change came from running:
IMAGE_TAG_BASE=registry-proxy.engineering.redhat.com/rh-osbs/ IMG=openshift-sandboxed-containers-operator make bundle
which runs:
cd config/manager && $(KUSTOMIZE) edit set image controller=$(IMG)

I can't quite get my head around if this should be a permanent change or if it even matters because it get substituted anyway.

I don't think it matters much as it is being overwritten typically when you run make IMG=xxx deploy. Maybe setting it to quay.io/openshift_sandboxed_containers/openshift-sandboxed-containers-operator:latest would make sense as the default?

OK. this is again controlled by VARS in the makefile. According to the operator-sdk docs and makefile comments, we should set IMAGE_TAG_BASE to repo+imagename and then set IMG ?= $(IMAGE_TAG_BASE):$(VERSION).
This should represent our default operator repo, image name, and image version. The question is what should that be. I feel like it should be the production build. If you need a local build, you should set the IMG as mentioned in our docs. Does this logic make sense?

The idea is that if you do a make docker-build or bundle-build with no options you would get the production version.

…custome dockerfile, updated IMAGE_TAG_BASE and IMG defaults

jensfr · 2022-10-07T07:34:49Z

Makefile

@@ -231,3 +233,31 @@ catalog-build: opm ## Build a catalog image.
 .PHONY: catalog-push
 catalog-push: ## Push a catalog image.
 	$(MAKE) docker-push IMG=$(CATALOG_IMG)
+


this chunk should go away when you rebase against master

this chunk should go away when you rebase against master

Yes but there is more to it. See below.

gkurz · 2022-10-07T07:50:05Z

.dockerignore

+config/crd/bases/kataconfiguration.openshift.io_kataconfigs.yaml
+config/rbac/role.yaml
+config/webhook/manifests.yaml
+bundle


Maybe add a terminating newline for a smoother cat experience ?

gkurz · 2022-10-07T07:50:17Z

.gitignore

@@ -85,5 +85,4 @@ config/crd/bases/kataconfiguration.openshift.io_kataconfigs.yaml
 config/rbac/role.yaml
 config/webhook/manifests.yaml
 # make bundle files
-bundle.Dockerfile
 bundle/


Missing newline since commit 846b467. Maybe fix this as well while here ?

Makefile

bundle.Dockerfile

pmores · 2022-10-07T08:16:45Z

Thanks, but why do we now have 21 commits? If this is not a target branch mess up I think this needs some clean-up and squashing.

jensfr · 2022-10-07T08:18:44Z

I think bundle.Dockerfile can be removed as the Makefile is now using bundle-custom.Dockerfile. And then the bundle-clean target also needs to remove bundle-custom.Dockerfile

pmores · 2022-10-07T08:21:51Z

Dockerfile

+COPY config config/
+COPY controllers controllers/
+COPY go.mod go.mod
+COPY go.sum go.sum


These layers need to be sorted from less frequently changed to more frequently changed, please check out my earlier explanations or, even better, review Dockerfile best practices.

Are you comfortable handling the dockerfiles optimisations, or would you prefer that I do it subsequently if necessary?

Agreed. Will move go.mod and go.sum above api in all docker files

Makefile

gkurz · 2022-10-07T10:06:51Z

Thanks, but why do we now have 21 commits? If this is not a target branch mess up I think this needs some clean-up and squashing.

Agreed. This PR should also be rebased on current main since #221 got merged.

I think bundle.Dockerfile can be removed as the Makefile is now using bundle-custom.Dockerfile. And then the bundle-clean target also needs to remove bundle-custom.Dockerfile

AFAICS bundle.Dockerfile is still generated by make bundle:

cd config/manager && /home/greg/Work/openshift/sandboxed-containers-operator/bin/kustomize edit set image controller=quay.io/rhgkurz/openshift-sandboxed-containers-operator
/home/greg/Work/openshift/sandboxed-containers-operator/bin/kustomize build config/manifests | operator-sdk generate bundle -q --overwrite --version 1.3.1 --channels="stable-1.3" --default-channel="stable-1.3"
INFO[0000] Creating bundle.Dockerfile

Just for my understanding : we're deliberately ignoring the generated bundle.Dockerfile and we're only to use the custom one from git instead from now on, right ? Not sure to fully understand the rationale for this change. Some explainations in the commit changelog would be appreciated.

EDIT: also, since we're now carrying this Dockerfile, how are we supposed to make it evolve in the future ?

And the bundle-clean target should also continue to remove bundle.Dockerfile no matter what, not bundle-custom.Dockerfile since it belongs to the repo.

jensfr

see comments

gkurz · 2022-10-07T10:29:35Z

bundle-custom.Dockerfile

+# Copy files to locations specified by labels.
+COPY --from=builder /workspace/bundle/manifests /manifests/
+COPY --from=builder /workspace/bundle/metadata /metadata/
+COPY --from=builder /workspace/bundle/tests/scorecard /tests/scorecard/


Trailing newline here also please.

config/manager/kustomization.yaml

pmores · 2022-10-07T11:14:25Z

Just for my understanding : we're deliberately ignoring the generated bundle.Dockerfile and we're only to use the custom one from git instead from now on, right ? Not sure to fully understand the rationale for this change. Some explainations in the commit changelog would be appreciated.

That's true, and until that's added I think I can explain at least some of the rationale (I haven't realised that the original bundle.Dockerfile was generated until now which shows that my understanding isn't quite complete either).

The generated bundle.Dockerfile is basically just a couple of LABELs and a few COPY commands just to get stuff inside bundle/ into the bundle image. That stopped being adequate the moment we removed generated files (that includes bundle/*) from the repository. Since they don't come from the repo anymore, the contents of bundle/ have to be generated first by bundle.Dockerfile.

I believe/assume that's the reason for bundle-custom.Dockerfile - essentially a bundle.Dockerfile with the added ability to generate generated files first.

And the bundle-clean target should also continue to remove bundle.Dockerfile no matter what, not bundle-custom.Dockerfile since it belongs to the repo.

Yes, you're right, now I understand, bundle.Dockerfile should indeed be cleaned up (or, better yet, never generated in the first place if operator-sdk offers such an option) as the custom Dockerfile is always used to build the bundle image.

…or image_tag_base

gkurz

LGTM. Thanks @cpmeadors !

openshift-ci · 2022-10-07T16:47:47Z

@cpmeadors: 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/4.9-sandboxed-containers-operator-e2e	`0196e15`	link	false	`/test 4.9-sandboxed-containers-operator-e2e`
ci/prow/sandboxed-containers-operator-e2e	`0196e15`	link	false	`/test sandboxed-containers-operator-e2e`

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.

jensfr

lgtm, thanks!

These changes add multistage docker files for both the operator and the operator bundle images. The builder stage uses the make targets to generate the necessary files to build the two images. This should fix the Prow CI image build issues and it should allow the copy of the bundle files in the midstream repo to be removed. Ultimately it means the same process is being use to build the images no matter what is building them. Original patch by Cameron Meadors <cmeadors@redhat.com> Backported by Signed-off-by: Jens Freimann <jfreimann@redhat.com>

Backport #219 "generate files in dockerfiles"

generate files in dockerfiles

d4eb09c

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2022

cpmeadors and others added 2 commits October 4, 2022 13:42

Merge branch 'openshift:master' into generate-files-in-dockerfile

812fae6

fixed missing file and cleaned up dockerfile

27e7845

cpmeadors marked this pull request as ready for review October 5, 2022 17:32

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 5, 2022

openshift-ci bot requested review from jensfr and pmores October 5, 2022 17:33

Clean up of dockerfiles to make them consisten

a1f3444

jensfr reviewed Oct 5, 2022

View reviewed changes

bundle.Dockerfile Show resolved Hide resolved

jensfr reviewed Oct 5, 2022

View reviewed changes

jensfr approved these changes Oct 5, 2022

View reviewed changes

pmores reviewed Oct 6, 2022

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

pmores reviewed Oct 6, 2022

View reviewed changes

cpmeadors added 11 commits October 6, 2022 09:44

use make build

5878da7

update bundle dockerfile after update to operator-sdk 1.23

eba0f5d

set channel defaults

7f12296

revert back to sdk 1.20.1

d29a93a

generate files in dockerfiles

f3329b7

fixed missing file and cleaned up dockerfile

b42328d

Clean up of dockerfiles to make them consisten

5a3ea96

use make build

40eff87

update bundle dockerfile after update to operator-sdk 1.23

c671dce

set channel defaults

4a01d38

revert back to sdk 1.20.1

1a490af

cpmeadors added 2 commits October 6, 2022 12:03

add .dockerignore, use 1.20.1 operater-sdk in docker file

eedfe4d

go back to only copying what is needed in dockerfiles

fa0a824

cpmeadors commented Oct 6, 2022

View reviewed changes

removed hardcoded version from dockerfile, pointed bundle-build to a …

3821f2a

…custome dockerfile, updated IMAGE_TAG_BASE and IMG defaults

jensfr reviewed Oct 7, 2022

View reviewed changes

gkurz reviewed Oct 7, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

gkurz reviewed Oct 7, 2022

View reviewed changes

bundle.Dockerfile Show resolved Hide resolved

pmores reviewed Oct 7, 2022

View reviewed changes

jensfr reviewed Oct 7, 2022

View reviewed changes

Makefile Show resolved Hide resolved

jensfr requested changes Oct 7, 2022

View reviewed changes

gkurz reviewed Oct 7, 2022

View reviewed changes

config/manager/kustomization.yaml Show resolved Hide resolved

gkurz reviewed Oct 7, 2022

View reviewed changes

config/manager/kustomization.yaml Show resolved Hide resolved

cpmeadors and others added 2 commits October 7, 2022 10:55

add trailing newlines, reorder copy lines in dockerfiles, add fixme f…

8f3bbac

…or image_tag_base

Merge branch 'openshift:master' into generate-files-in-dockerfile

0196e15

gkurz approved these changes Oct 7, 2022

View reviewed changes

jensfr approved these changes Oct 7, 2022

View reviewed changes

jensfr merged commit dd91e7c into openshift:master Oct 7, 2022

jensfr added a commit that referenced this pull request Oct 14, 2022

Merge pull request #224 from jensfr/backport-219

c9f0fd3

Backport #219 "generate files in dockerfiles"

This was referenced Oct 24, 2022

add .dockerignore from operator-sdk #212

Closed

Add build artifacts to .gitignore #208

Closed

cpmeadors deleted the generate-files-in-dockerfile branch November 10, 2022 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generate files in dockerfiles #219

generate files in dockerfiles #219

cpmeadors commented Sep 22, 2022 •

edited

openshift-ci bot commented Sep 22, 2022

cpmeadors commented Sep 22, 2022

cpmeadors commented Sep 29, 2022

bear-redhat commented Sep 30, 2022

jensfr Oct 5, 2022

jensfr left a comment

pmores Oct 6, 2022

cpmeadors Oct 6, 2022

cpmeadors Oct 6, 2022

pmores Oct 6, 2022

pmores commented Oct 6, 2022

cpmeadors Oct 6, 2022

jensfr Oct 6, 2022

cpmeadors Oct 6, 2022

cpmeadors Oct 6, 2022

jensfr Oct 7, 2022

gkurz Oct 7, 2022

gkurz Oct 7, 2022

gkurz Oct 7, 2022

pmores commented Oct 7, 2022

jensfr commented Oct 7, 2022 •

edited

pmores Oct 7, 2022

cpmeadors Oct 7, 2022

gkurz commented Oct 7, 2022 •

edited

jensfr left a comment

gkurz Oct 7, 2022

pmores commented Oct 7, 2022

gkurz left a comment

openshift-ci bot commented Oct 7, 2022

jensfr left a comment

generate files in dockerfiles #219

generate files in dockerfiles #219

Conversation

cpmeadors commented Sep 22, 2022 • edited

openshift-ci bot commented Sep 22, 2022

cpmeadors commented Sep 22, 2022

cpmeadors commented Sep 29, 2022

bear-redhat commented Sep 30, 2022

Choose a reason for hiding this comment

jensfr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmores commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmores commented Oct 7, 2022

jensfr commented Oct 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkurz commented Oct 7, 2022 • edited

jensfr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmores commented Oct 7, 2022

gkurz left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 7, 2022

jensfr left a comment

Choose a reason for hiding this comment

cpmeadors commented Sep 22, 2022 •

edited

jensfr commented Oct 7, 2022 •

edited

gkurz commented Oct 7, 2022 •

edited