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

IR-119: Add IgnoreInvalidRefs for image pruner #708

Merged

Conversation

dmage
Copy link
Member

@dmage dmage commented Jul 27, 2020

Invalid image references in pods/builds/etc cause the image pruner to abort its job. This is the safest approach because even if the pruner has bugs, it won't delete anything that is in use. But as there is no validation for image references, some clusters have invalid references because of their users mistakes and the pruner should ignore them.

The default value for ignoreInvalidRefs is false to maintain backward compatibility. But the operator will bootstrap the configuration object with ignoreInvalidRefs: true. So new clusters will have ignoreInvalidRefs: true, upgraded from 4.5 clusters will have ignoreInvalidRefs: false.

@dmage
Copy link
Member Author

dmage commented Jul 27, 2020

/assign @bparees

@dmage dmage force-pushed the pruner-ignore-invalid-refs branch 2 times, most recently from 858cc2e to 4742bbf Compare July 27, 2020 11:11
@@ -617,6 +617,10 @@ spec:
pruner jobs to retain. Defaults to 3 if not set.
type: integer
format: int32
ignoreInvalidRefs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ignoreInvalidRefs:
ignoreInvalidImageReferences:

makes it more explicit in case you need to ignore other types of references in the future for some reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed. I couldn't choose between two options: name it the same as the flag (--ignore-invalid-refs), or give it а more natural name.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can always deprecate/rename the cli arg to get things consistent in the future, probably better to move the api towards what we consider the better name to be (assuming we agree it's better).

@bparees
Copy link
Contributor

bparees commented Jul 27, 2020

general concept seems fine, suggested a rename of the field though

@dmage dmage force-pushed the pruner-ignore-invalid-refs branch from 4742bbf to 951d79f Compare July 27, 2020 17:45
@dmage
Copy link
Member Author

dmage commented Jul 27, 2020

/retest

@bparees
Copy link
Contributor

bparees commented Jul 27, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, dmage

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

@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 27, 2020
@openshift-merge-robot openshift-merge-robot merged commit 0bc64a9 into openshift:master Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants