Skip to content
This repository has been archived by the owner on Jul 4, 2024. It is now read-only.

Cleanup image cache in update compass script #2744

Merged
merged 15 commits into from
Nov 29, 2022
Merged

Cleanup image cache in update compass script #2744

merged 15 commits into from
Nov 29, 2022

Conversation

ivantenevvasilev
Copy link
Contributor

@ivantenevvasilev ivantenevvasilev commented Nov 16, 2022

Description

Changes proposed in this pull request:

  • remove cached images in the k3d instance nodes as it doesn't detect changes after a second execution of PR jobs

Pull Request status

  • Implementation
  • [N/A] Unit tests
  • [N/A] Integration tests
  • [N/A] chart/compass/values.yaml is updated
  • [N/A] Mocks are regenerated, using the automated script

@ivantenevvasilev ivantenevvasilev added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 16, 2022
@ivantenevvasilev ivantenevvasilev requested review from a team as code owners November 16, 2022 15:22
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2022
@kyma-bot kyma-bot added 🦖 team-raptor Team Raptor Label size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 16, 2022
@ivantenevvasilev ivantenevvasilev added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2022
@kyma-bot kyma-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 23, 2022
@ivantenevvasilev
Copy link
Contributor Author

/retest

@ivantenevvasilev ivantenevvasilev changed the title [Test PR] Validate PR jobs Patch imagePullPolicy to Always on Deployments and CronJobs Nov 23, 2022
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2022
@ivantenevvasilev ivantenevvasilev changed the title Patch imagePullPolicy to Always on Deployments and CronJobs Cleanup image cache in update compass script Nov 24, 2022
@ivantenevvasilev ivantenevvasilev added the 👋 request review Review required label Nov 24, 2022
@@ -9,4 +9,7 @@ set -o errexit
CURRENT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
INSTALLATION_DIR=${CURRENT_DIR}/../../

docker exec k3d-kyma-agent-0 sh -c "ctr images rm \$(ctr image list -q name~=eu.gcr.io/kyma-project/incubator)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the man page - ctr is an unsupported debug and administrative client for interacting with the containerd daemon. Because it is unsupported, the commands, options, and operations are not guaranteed to be backward compatible or stable from release to release of the containerd project.

Could be because of that in the future releases to encounter the same problem that this PR fix? the drawback of the problem is it's tricky to be found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't we have to explicitly bump the k3d version? If it updates and the command fails, then it's better to have the job fail than to give false positives/negatives. Also from what I understand is that they do not gurantee the backwards compatibility, but it's not very likely to happen. Plus in the logs we can see when and which images it has removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also considering this is a tool for managing the containerd images, I assume there will always be a way to remove an image, so worst case scenario, it would require to change the script for cleaning up the images.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's valid, my point was if the "breaking change" is not a breaking but rather the command not working as expected or misbehaves and we encounter the same issue which is hard to notice in first glance. If the command directly fail that okay because we'll find out something is wrong and as you mentioned most probably there will be another option to fix it/delete the image.

I just saw that the ctr is unsupported and was wondering if will be an impact for us. But if you think there is no such probability or the impact is low and we can fast determine what's happening we can use it.

@kyma-bot kyma-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 25, 2022
@ivantenevvasilev
Copy link
Contributor Author

/test pre-main-compass-gke-benchmark

1 similar comment
@ivantenevvasilev
Copy link
Contributor Author

/test pre-main-compass-gke-benchmark

@kyma-bot kyma-bot added lgtm Looks good to me! approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 29, 2022
@kyma-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DimitarPetrov, PetarTodorovv

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@ivantenevvasilev
Copy link
Contributor Author

/hold cancel

@kyma-bot kyma-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2022
@kyma-bot kyma-bot merged commit 3fb8212 into main Nov 29, 2022
@PetarTodorovv PetarTodorovv deleted the test-pr-jobs branch November 29, 2022 09:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Looks good to me! size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. 🦖 team-raptor Team Raptor Label 👋 request review Review required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants