add a prune package to handle cleanup of pods and job resources #75
Conversation
Pull Request Test Coverage Report for Build 1391762787
💛 - Coveralls |
prune/maxcount.go
Outdated
| if !config.DryRun { | ||
| err := config.removeResource(resources[i]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| removeCount-- | ||
| if removeCount == 0 { | ||
| break | ||
| } |
There was a problem hiding this comment.
Can we move this code to a separate function that is called outside the strategy since it's universal across all strategies? Then we can make the strategy function signature func(config Config, resources []ResourceInfo) (toPrune []ResourceInfo, err error) and remove the deletion responsibility from the strategy implementation.
There was a problem hiding this comment.
I moved the config.DryRun check into the removeResource func to make it cleaner.
prune/prune.go
Outdated
| ) | ||
|
|
||
| // ResourceKind describes the Kubernetes Kind we are wanting to prune | ||
| type ResourceKind string |
There was a problem hiding this comment.
Should we reuse a type from the Kubernetes library here like GroupKind or GroupResource? Could make it easier to integrate with other components later if we use a universal type. We would still be able to be opinionated on which resources we support removing.
There was a problem hiding this comment.
latest commit uses the apimachinery schema.GroupVersionKind to hold that value which should be better.
prune/prune.go
Outdated
|
|
||
| // StrategyImplementation function allows a means to specify | ||
| // custom prune strategies | ||
| type StrategyImplementation func(cfg Config, resources []ResourceInfo) error |
There was a problem hiding this comment.
Nit: since its a function type, not a struct, would StrategyFunc be a better type name?
There was a problem hiding this comment.
better name, latest commit uses that name.
prune/prune.go
Outdated
| // Config defines a pruning configuration and ultimately | ||
| // determines what will get pruned | ||
| type Config struct { | ||
| Ctx context.Context //context used by pruning |
There was a problem hiding this comment.
Its considered bad practice by the go standard library to use a context as a struct member.
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx
There was a problem hiding this comment.
ok, I moved it outside the Config with this latest commit.
prune/prune.go
Outdated
| PreDeleteHook PreDelete //called before resource is deleteds | ||
| } | ||
|
|
||
| var log = logf.Log.WithName("prune") |
There was a problem hiding this comment.
Can we move this to the config struct?
There was a problem hiding this comment.
moved the log into the Config with latest commit.
prune/prune.go
Outdated
| switch config.Strategy.Mode { | ||
| case MaxAgeStrategy: | ||
| err = pruneByMaxAge(config, resourceList) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| case MaxCountStrategy: | ||
| err = pruneByMaxCount(config, resourceList) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| case CustomStrategy: | ||
| err = config.CustomStrategy(config, resourceList) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| default: | ||
| return fmt.Errorf("unknown strategy") | ||
| } |
There was a problem hiding this comment.
Nit: you can just check err once at the end of the switch statement instead of in each branch.
There was a problem hiding this comment.
doh! thanks, new version corrects that.
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we want to return some sort of error if we attempt to delete an unsupported resource?
There was a problem hiding this comment.
added in the error return. thanks for catching that.
|
@ryantking GREAT review! Thanks for that. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmrodri 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 |
openshift-ci
bot
added
the
approved
Description of the change:
this package can be used by operator authors to implement a basic resource pruning capability
in their operators to cleanup Pods and Jobs that are considered prunable.
The use of this package is optional by authors. Author focused documentation would follow this PR
to show authors how to use this package.
This package would be used for pruning resources not owned by a CustomResource, but more temporary in nature. If left on cluster, resources can accumulate causing for example heavy etcd disk usage.
Motivation for the change:
operator authors inside Red Hat and the community at large outside of Red Hat typically
write their own custom logic to perform this form of pruning. This package implementation
covers Pod and Job cleanup which from experience are the most typical resources that need
to be cleaned up. This package would offer up a basic capability for authors to make use of.