-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: set quay image expiry to prevent overflow of images #851
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #851 +/- ##
==========================================
+ Coverage 80.20% 82.12% +1.91%
==========================================
Files 64 76 +12
Lines 4492 6092 +1600
==========================================
+ Hits 3603 5003 +1400
- Misses 600 745 +145
- Partials 289 344 +55
Flags with carried forward coverage won't be shown. Click here to find out more.
|
9e481f6
to
563b3bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL setting tag expiration from a dockerfile 👍
operator image and bundle image covered.
Missing the catalog image. In https://github.com/Kuadrant/kuadrant-operator/blob/main/make/catalog.mk#L10-L12 opm generate dockerfile
allows adding extra labels
bin/opm generate dockerfile kuadrant-operator-catalog
Error: stat kuadrant-operator-catalog: no such file or directory
Usage:
opm generate dockerfile <dcRootDir> [flags]
Flags:
-i, --binary-image string Image in which to build catalog. (default "quay.io/operator-framework/opm:latest")
-l, --extra-labels strings Extra labels to include in the generated Dockerfile. Labels should be of the form 'key=value'.
-h, --help help for dockerfile
Global Flags:
--skip-tls-verify skip TLS certificate verification for container image registries while pulling bundles
--use-http use plain HTTP for container image registries while pulling bundles
427a457
to
2667a58
Compare
cd $(PROJECT_PATH)/catalog && $(OPM) generate dockerfile kuadrant-operator-catalog -l quay.expires-after=$(QUAY_IMAGE_EXPIRY) | ||
catalog-dockerfile: $(CATALOG_DOCKERFILE) ## Generate catalog dockerfile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also allow setting the Quay image expiry by following the same pattern to how it was done for the bundle image. If we choose to implement this, the Docker label will be consistently set via build arguments. However, I'm not sure if it's worth the extra step just for the sake of consistency. If anyone has any thoughts on this, feel free to share, otherwise achieving this through opm
labeling is simpler.
cd $(PROJECT_PATH)/catalog && $(OPM) generate dockerfile kuadrant-operator-catalog -l quay.expires-after=$(QUAY_IMAGE_EXPIRY) | |
catalog-dockerfile: $(CATALOG_DOCKERFILE) ## Generate catalog dockerfile. | |
cd $(PROJECT_PATH)/catalog && $(OPM) generate dockerfile kuadrant-operator-catalog | |
echo "$$QUAY_DOCKERFILE_LABEL" >> $(CATALOG_DOCKERFILE) | |
catalog-dockerfile: $(CATALOG_DOCKERFILE) ## Generate catalog dockerfile. | |
# Build a catalog image by adding bundle images to an empty catalog using the operator package manager tool, 'opm'. | |
# Ref https://olm.operatorframework.io/docs/tasks/creating-a-catalog/#catalog-creation-with-raw-file-based-catalogs | |
.PHONY: catalog-build | |
catalog-build: ## Build a catalog image. | |
# Build the Catalog | |
docker build --build-arg QUAY_IMAGE_EXPIRY=$(QUAY_IMAGE_EXPIRY) $(PROJECT_PATH)/catalog -f $(PROJECT_PATH)/catalog/kuadrant-operator-catalog.Dockerfile -t $(CATALOG_IMG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no hard opinion on this matter. For me what it matters is that the label should be there.
I do have a "soft opinion", though. I would do the labeling using the opm
command. Mainly because we do not have a committed version of the dockerfile, it is autogenerated. So, the expiry time is not going to be committed in the repo even though it is hardcoded in the (ephemeral) dockerfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also do not like
echo "$$QUAY_DOCKERFILE_LABEL" >> $(CATALOG_DOCKERFILE)
if we need to do that, we do that. But modifying dynamically the dockerfile is somewhat unwanted. Can we ensure the value of $QUAY_DOCKERFILE_LABEL
is syntactically correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we need to do that, we do that. But modifying dynamically the dockerfile is somewhat unwanted.
@eguzki Yes, we don't need to do this for the catalog dockerfile 👍 But I think we need to do this for the bundle Dockerfile since this is also autogenerated, is overwritten every time and committed.
Line 393 in 107947e
echo "$$QUAY_DOCKERFILE_LABEL" >> bundle.Dockerfile |
kuadrant-operator/bundle.Dockerfile
Lines 23 to 26 in 107947e
# Quay image expiry | |
ARG QUAY_IMAGE_EXPIRY | |
ENV QUAY_IMAGE_EXPIRY=${QUAY_IMAGE_EXPIRY:-never} | |
LABEL quay.expires-after=${QUAY_IMAGE_EXPIRY} |
I'm not aware of a way to add extra labels to the generated bundle dockerfile via operator-sdk 🤔
Can we ensure the value of $QUAY_DOCKERFILE_LABEL is syntactically correct?
It should be correct, but not sure how we can ensure this? It's functioning at least since the label was set correctly for the bundle image built 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, I ran into the same issue when wanted to add openshift specific labels to the bundle image.
I am afraid, I do not know how to do this cleanly, either.
We decided to have git commited the bundle.Dockerfile (and bundle
dir) for a reason. Reason being having traceability. Updating the file on-the-fly sort of defeats that purpose.
I propose to do what you suggest. A sacrifice for the greater good. Do no git update bundle.Dockerfile
and commit it, change it on the fly instead and revert changes when the image build is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to do what you suggest. A sacrifice for the greater good. Do no git update bundle.Dockerfile and commit it, change it on the fly instead and revert changes when the image build is done.
@eguzki I'm thinking of reverting the last commit for this. Reason is the CI build for the bundle image needs to be updated also to account for this (why it's not working on the CI build currently) since it does not use the makefile commands to build the image, and likely be a duplicated effort to also do this in the CI.
kuadrant-operator/.github/workflows/build-images-base.yaml
Lines 138 to 168 in e4f3ac3
- name: Run make bundle | |
id: make-bundle | |
run: | | |
make bundle REGISTRY=${{ env.IMG_REGISTRY_HOST }} ORG=${{ env.IMG_REGISTRY_ORG }} \ | |
VERSION=${{ env.VERSION }} IMAGE_TAG=${{ env.IMG_TAGS }} \ | |
AUTHORINO_OPERATOR_VERSION=${{ inputs.authorinoOperatorVersion }} \ | |
LIMITADOR_OPERATOR_VERSION=${{ inputs.limitadorOperatorVersion }} \ | |
DNS_OPERATOR_VERSION=${{ inputs.dnsOperatorVersion }} \ | |
WASM_SHIM_VERSION=${{ inputs.wasmShimVersion }} \ | |
REPLACES_VERSION=${{ inputs.replacesVersion }} \ | |
CHANNELS=${{ inputs.channels }} | |
- name: Set up Docker Buildx | |
uses: docker/setup-buildx-action@v3 | |
- name: Login to container registry | |
uses: docker/login-action@v2 | |
with: | |
username: ${{ secrets.IMG_REGISTRY_USERNAME }} | |
password: ${{ secrets.IMG_REGISTRY_TOKEN }} | |
registry: ${{ env.IMG_REGISTRY_HOST }} | |
- name: Build and Push Bundle Image | |
id: build-bundle-image | |
uses: docker/build-push-action@v5 | |
with: | |
context: . | |
file: ./bundle.Dockerfile | |
platforms: linux/amd64,linux/arm64 | |
push: true | |
provenance: false | |
tags: | | |
${{ env.IMG_REGISTRY_HOST }}/${{ env.IMG_REGISTRY_ORG }}/${{ env.OPERATOR_NAME }}-bundle:${{ env.IMG_TAGS }} | |
build-args: QUAY_IMAGE_EXPIRY=${{ inputs.quayImageExpiry }} |
This, to me, is not as clean as the previous approach with just always appending the neccessay QUAY_DOCKERFILE_LABEL
in the make bundle
command after bundle image dockerfile has been generated, which will be the actual dockerfile used when building the bundle image rather than hiding this detail 🤔
Since we commit this Dockerfile, it's apparent from the Dockerfile that it can accept a build argument to allow expirying quay images if passed. Otherwise this detail is hidden and harder to debug in general I feel if an issue occurs.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, when you run bundle
makefile target, $(OPERATOR_SDK) generate bundle ...
will overwrite the bundle.Dockerfile and then we add the label again, right? something like
bundle:
....
$(OPERATOR_SDK) generate bundle -q --overwrite
.....
@echo "$$QUAY_DOCKERFILE_LABEL" >> bundle.Dockerfile
If this is what you suggest, I am ok with it. And I propose:
s/QUAY_DOCKERFILE_LABEL/QUAY_EXPIRY_TIME_LABEL/
Or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, when you run
bundle
makefile target,$(OPERATOR_SDK) generate bundle ...
will overwrite the bundle.Dockerfile and then we add the label again, right? If this is what you suggest, I am ok with it.
Yes, exactly 👍
. And I propose:
s/QUAY_DOCKERFILE_LABEL/QUAY_EXPIRY_TIME_LABEL/
. Or something similar.
Sounds good, updated to QUAY_EXPIRY_TIME_LABEL
👍
@@ -25,3 +25,4 @@ jobs: | |||
limitadorOperatorVersion: ${{ vars.LIMITADOR_OPERATOR_SHA }} | |||
dnsOperatorVersion: ${{ vars.DNS_OPERATOR_SHA }} | |||
wasmShimVersion: ${{ vars.WASM_SHIM_SHA }} | |||
quayImageExpiry: 2w |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abitary value of when to expire nighty images. If anyone has a better suited value, I can update it to this value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @jsmolar (QE) can recommend a value that would work well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 or 2 weeks for nightlies is reasonable. I don't see any reason why we would need to keep them longer.
2667a58
to
107947e
Compare
@@ -13,3 +13,5 @@ jobs: | |||
with: | |||
kuadrantOperatorVersion: ${{ github.ref_name }} | |||
kuadrantOperatorTag: ${{ github.ref_name }} | |||
# Conditionally set quayImageExpiry to expire quay images only for non-release branches | |||
quayImageExpiry: ${{ startsWith(github.ref_name, 'release') && 'never' || '1w' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abitary value of when to expire branch images. If anyone has a better suited value, I can update it to this value.
I'm assuming we only want to expire dev branch images and not release branches. Or do we care about release branch images? Seems the actual tagged v.X.Y.Z(-RC) images are run manually using the base images workflow where the image should be set to never expire 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!, regarding the default expiry time, would trust QA input.
🔑
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following https://github.com/Kuadrant/kuadrant-operator/pull/851/files#r1749898448
bundle.Dockerfile
cannot be changed, it is autogenerated.
Signed-off-by: KevFan <[email protected]>
107947e
to
db07932
Compare
78ab5ac
to
e4f3ac3
Compare
Signed-off-by: KevFan <[email protected]>
e4f3ac3
to
4321d37
Compare
Description
Part of: #769
Associated with: #843
Currently, all built Quay images never expire and are consuming unnecessary quota on Quay.io. Quay provides a way to set tag expiration from a Dockerfile to prevent this issue for images that don't need to be kept indefinitely, such as development branches and nightly builds. This can help prevent the problem from escalating over time.
Verification
expire_images
tag has an expiry at the following quay repos which was build by the CI for this branch:Remove local image
Build image
Inspect image labels
The
quay.expires-after
label should be set tonever
Remove local image again
Build image with a set expiry
Inspect image labels
The
quay.expires-after
label should be set to3d