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

feat: set quay image expiry to prevent overflow of images #851

Merged
merged 2 commits into from
Sep 16, 2024
Merged

Conversation

KevFan
Copy link
Contributor

@KevFan KevFan commented Sep 5, 2024

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

  • You can check that the expire_images tag has an expiry at the following quay repos which was build by the CI for this branch:
  • If you want to verify manually locally that the image label is set:
    • Remove local image

      docker rmi quay.io/kuadrant/kuadrant-operator:latest
      docker rmi quay.io/kuadrant/kuadrant-operator-bundle:latest
      docker rmi quay.io/kuadrant/kuadrant-operator-catalog:latest
      
    • Build image

      make docker-build bundle-build catalog-dockerfile catalog-build
      
    • Inspect image labels

      docker inspect quay.io/kuadrant/kuadrant-operator:latest --format='{{json .Config.Labels}}'
      docker inspect quay.io/kuadrant/kuadrant-operator-bundle:latest --format='{{json .Config.Labels}}'
      docker inspect quay.io/kuadrant/kuadrant-operator-catalog:latest --format='{{json .Config.Labels}}'
      
    • The quay.expires-after label should be set to never

      image
    • Remove local image again

      docker rmi quay.io/kuadrant/kuadrant-operator:latest
      docker rmi quay.io/kuadrant/kuadrant-operator-bundle:latest
      docker rmi quay.io/kuadrant/kuadrant-operator-catalog:latest
      
    • Build image with a set expiry

       # Delete the generated catalog Dockerfile as it won't regenerate again with the correct label if it's already present
       rm catalog/kuadrant-operator-catalog.Dockerfile 
       QUAY_IMAGE_EXPIRY=3d make docker-build bundle-build catalog-dockerfile catalog-build
      
    • Inspect image labels

      docker inspect quay.io/kuadrant/kuadrant-operator:latest --format='{{json .Config.Labels}}'
      docker inspect quay.io/kuadrant/kuadrant-operator-bundle:latest --format='{{json .Config.Labels}}'
      docker inspect quay.io/kuadrant/kuadrant-operator-catalog:latest --format='{{json .Config.Labels}}'
      
    • The quay.expires-after label should be set to 3d

      image

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.12%. Comparing base (ece13e8) to head (4321d37).
Report is 184 commits behind head on main.

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     
Flag Coverage Δ
bare-k8s-integration 4.50% <ø> (?)
controllers-integration 71.96% <ø> (?)
gatewayapi-integration 10.96% <ø> (?)
integration ?
istio-integration 56.54% <ø> (?)
unit 31.57% <ø> (+1.54%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1beta1 (u) 71.42% <ø> (ø)
api/v1beta2 (u) 85.35% <80.00%> (-6.08%) ⬇️
pkg/common (u) 88.13% <ø> (-0.70%) ⬇️
pkg/istio (u) 72.50% <ø> (-1.41%) ⬇️
pkg/log (u) 94.73% <ø> (ø)
pkg/reconcilers (u) ∅ <ø> (∅)
pkg/rlptools (u) 83.64% <ø> (+4.19%) ⬆️
controllers (i) 82.56% <81.70%> (+5.76%) ⬆️

see 42 files with indirect coverage changes

@eguzki eguzki added the kind/enhancement New feature or request label Sep 5, 2024
Copy link
Contributor

@eguzki eguzki left a 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

@KevFan KevFan force-pushed the expire_images branch 9 times, most recently from 427a457 to 2667a58 Compare September 5, 2024 16:02
Comment on lines +25 to 26
cd $(PROJECT_PATH)/catalog && $(OPM) generate dockerfile kuadrant-operator-catalog -l quay.expires-after=$(QUAY_IMAGE_EXPIRY)
catalog-dockerfile: $(CATALOG_DOCKERFILE) ## Generate catalog dockerfile.
Copy link
Contributor Author

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.

Suggested change
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)

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

echo "$$QUAY_DOCKERFILE_LABEL" >> bundle.Dockerfile

# 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 🤔

Copy link
Contributor

@eguzki eguzki Sep 9, 2024

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.

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 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.

- 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?

Copy link
Contributor

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.

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, 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
Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

@KevFan KevFan self-assigned this Sep 5, 2024
@@ -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' }}
Copy link
Contributor Author

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 🤔

@KevFan KevFan marked this pull request as ready for review September 6, 2024 08:58
@KevFan KevFan requested a review from eguzki September 6, 2024 08:58
@KevFan KevFan requested review from a team and didierofrivia September 6, 2024 09:00
Copy link
Member

@didierofrivia didierofrivia left a 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.

🔑

Copy link
Contributor

@eguzki eguzki left a 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.

@KevFan KevFan force-pushed the expire_images branch 2 times, most recently from 78ab5ac to e4f3ac3 Compare September 10, 2024 07:48
@KevFan KevFan merged commit 2cd87da into main Sep 16, 2024
29 checks passed
@KevFan KevFan deleted the expire_images branch September 17, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request size/small
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants