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
Bug 1806686: Image pruner updates: Controller / Prune Registry #459
Bug 1806686: Image pruner updates: Controller / Prune Registry #459
Conversation
coreydaley
commented
Feb 17, 2020
- Move the image pruner to it's own controller
- Implement the --prune-registry flag for the pruner
/retest |
1 similar comment
/retest |
/assign @dmage @adambkaplan |
/retest |
1 similar comment
/retest |
@adambkaplan @dmage ptal |
/retest |
2 similar comments
/retest |
/retest |
/approve |
@dmage updated per your comments |
/retest |
4 similar comments
/retest |
/retest |
/retest |
/retest |
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.
Let's break out the --prune-registry fixes.
- Move the image pruner to it's own controller - Implement the --prune-registry flag for the pruner
Add tests for the --prune-registry flag
@adambkaplan ptal, all tests are passing. |
@coreydaley: This pull request references Bugzilla bug 1806686, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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
@dmage @ricardomaraschini Given the scope of this change I'd like a second opinion before we merge.
@coreydaley: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@coreydaley: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@coreydaley: This pull request references Bugzilla bug 1806686, which is valid. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Just a few nits, seems good.
} | ||
|
||
func (b ByCreationTimestamp) Less(i, j int) bool { | ||
return !b[j].CreationTimestamp.Time.After(b[i].CreationTimestamp.Time) |
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.
nit: could we convert this to
return b[j].CreationTimestamp.Time.Before(b[i].CreationTimestamp.Time)
var errs []error | ||
|
||
// Wait for the cronjob to have an updated --prune-registry flag | ||
err = wait.Poll(1*time.Second, framework.AsyncOperationTimeout, func() (stop bool, err error) { |
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.
nit: use time.Second
instead of 1*time.Second
@@ -8,6 +8,18 @@ import ( | |||
corev1 "k8s.io/api/core/v1" | |||
) | |||
|
|||
func FlagExistsWithValue(args []string, flag string, value string) error { | |||
for _, arg := range args { | |||
if strings.HasPrefix(arg, flag) { |
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 think that HasPrefix()
here might lead us to false positives in the future, i.e. we may have two flags that have the same prefix.
@@ -130,6 +140,15 @@ func (gcj *generatorPrunerCronJob) expected() (runtime.Object, error) { | |||
return cj, nil | |||
} | |||
|
|||
func (gcj *generatorPrunerCronJob) getPruneRegistry(cr *imageregistryapiv1.Config) bool { |
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.
nit: this function could be converted into return cr.Spec.ManagementState == operatorapi.Managed
. the function name seems misguiding as well as it does not return a "PruneRegistry" :-)
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.
The function handles three cases, Managed, Removed, and UnManaged, so the == would not handle all three.
I feel like the getPruneRegistry is inline with the rest of the get(functionNames) here. Feel free to suggest a new name.
Status: imageregistryv1.ImagePrunerStatus{}, | ||
} | ||
|
||
client, err := regopset.NewForConfig(c.kubeconfig) |
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.
You can use c.clients.RegOp
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.
Looks good to merge.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, coreydaley, 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 |
2 similar comments
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, coreydaley, 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 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, coreydaley, 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 |
@coreydaley: All pull requests linked via external trackers have merged. Bugzilla bug 1806686 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherrypick release-4.4 |
@coreydaley: new pull request created: #468 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |