Skip to content

Cirrus: Print images that should be pruned #3335

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Jul 17, 2019
Merged

Cirrus: Print images that should be pruned #3335

openshift-merge-robot merged 1 commit into from
Jul 17, 2019

Conversation

@cevich
Copy link
Member

@cevich cevich commented Jun 14, 2019

Over time unless they're removed, the project could grow quite a large
collection of VM images. While generally cheap (less than a penny each,
per month), these will become a significant cost item if not kept
in-check.

Add a specialized container for handling image-pruning, but limit
it to only finding and printing (not actually deleting) images.

Also update the image-building workflow so that base-images used to
compose cache-images are also labeled with metadata.

N/B: As an additional safeguard, the service account which
executes the new container in production DOES NOT
have access to delete images. This can be enabled
by adding the GCE IAM role: CustomComputeImagePrune

Signed-off-by: Chris Evich cevich@redhat.com

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2019
@cevich
Copy link
Member Author

cevich commented Jun 14, 2019

NOTE: This is completely untested as of this comment.

@cevich cevich force-pushed the imgprune branch 2 times, most recently from a75390a to 096c27f Compare June 14, 2019 20:50
@rh-atomic-bot
Copy link
Collaborator

rh-atomic-bot commented Jun 17, 2019

☔ The latest upstream changes (presumably #3308) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2019
@cevich cevich force-pushed the imgprune branch 3 times, most recently from e8186f2 to 0171d6a Compare June 28, 2019 17:51
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2019
@cevich cevich force-pushed the imgprune branch 2 times, most recently from ca32e62 to c9e2fac Compare July 11, 2019 11:20
@cevich cevich changed the title WIP: Cirrus: Print images that should be pruned Cirrus: Print images that should be pruned Jul 11, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2019
@cevich
Copy link
Member Author

cevich commented Jul 11, 2019

@edsantiago mind taking an initial look? I know you're not fond of the entrypoint.sh name, but I kept it for consistency with other images. This new task and container image is the counter-part of time-stamping the "last used" datestamp on VM images (meta task). However, the new one only runs post-merge, on the master branch, and (for now) only prints the victim's names. Example run of new task in Cirrus-CI using hand-built image

edsantiago
edsantiago suggested changes Jul 11, 2019
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

First-pass review; looks mostly good, with some suggestions and questions. I need to make another pass but I'm drained right now.

edsantiago
edsantiago reviewed Jul 11, 2019
View reviewed changes
@cevich cevich changed the title Cirrus: Print images that should be pruned WIP: Cirrus: Print images that should be pruned Jul 12, 2019
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 12, 2019
@cevich cevich force-pushed the imgprune branch 2 times, most recently from 757f13d to 93e97ab Compare July 12, 2019 17:56
@cevich
Copy link
Member Author

cevich commented Jul 12, 2019

Example run based on 93e97ab4b9f1dfe7e1658a1a5aa726b43fe155a3

cevich
cevich commented Jul 12, 2019
View reviewed changes
@cevich
Cirrus: Print images that should be pruned
e8564dc 
Over time unless they're removed, the project could grow quite a large
collection of VM images.  While generally cheap (less than a penny each,
per month), these will become a significant cost item if not kept
in-check.

Add a specialized container for handling image-pruning, but limit
it to only finding and printing (not actually deleting) images.

Also update the image-building workflow so that base-images used to
compose cache-images are also labeled with metadata.

N/B: As an additional safeguard, the service account which
     executes the new container in production *DOES NOT*
     have access to delete images.  This can be enabled
     by adding the GCE IAM role: CustomComputeImagePrune

Signed-off-by: Chris Evich <cevich@redhat.com>
@cevich
Copy link
Member Author

cevich commented Jul 15, 2019

rebased and force-pushed. Only change was a minor README fix.

Ignoring the big scary image delete problem for now.

@cevich
Copy link
Member Author

cevich commented Jul 15, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2019
@cevich cevich changed the title WIP: Cirrus: Print images that should be pruned Cirrus: Print images that should be pruned Jul 15, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2019
@edsantiago
Copy link
Member

edsantiago commented Jul 15, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Jul 16, 2019

Changes LGTM, tests aren't hip and it looks the same as your other PR's issues.

@cevich
Copy link
Member Author

cevich commented Jul 16, 2019

tests happy now

@rhatdan
Copy link
Member

rhatdan commented Jul 17, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Jul 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, edsantiago, rhatdan

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2019
@openshift-merge-robot openshift-merge-robot merged commit 04a9cb0 into containers:master Jul 17, 2019
@cevich cevich deleted the imgprune branch June 30, 2021 18:11
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mheon mheon Awaiting requested review from mheon

@mrunalp mrunalp Awaiting requested review from mrunalp

1 more reviewer

@edsantiago edsantiago edsantiago approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@edsantiago edsantiago

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@cevich @rh-atomic-bot @edsantiago @TomSweeneyRedHat @rhatdan @openshift-ci-robot @openshift-merge-robot