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

Improve prune feature implementation #101

Closed
fgiloux opened this issue Jan 25, 2022 · 1 comment · Fixed by #105
Closed

Improve prune feature implementation #101

fgiloux opened this issue Jan 25, 2022 · 1 comment · Fixed by #105
Assignees

Comments

@fgiloux
Copy link
Contributor

fgiloux commented Jan 25, 2022

Feature Request

Is your feature request related to a problem? Please describe.
It seems that the implementation of the prune feature could be improved. As part of the fix for PR #100 I noticed the following:

  • Naming issue: Config is the main struct for the prune logic, which leads to methods like:
    Config.Execute. It does not sound right. Config may be passed to a factory creating a Pruner or a Processor structure but the Execuste method should be on the later structure: Pruner.Execute() or Processor.Execute() makes more sense.
  • The Config structure seems not optimal in the sense that for instance the same Strategy has to be applied to all the resources. If the logic is to be configured once for multiple resources it may make more sense to have a slice or a map of substructures containing the specificity for each resource: gvk, namespace, strategy, pre-deletion hook. A different strategy can then be selected by the user for each resource.
  • The ResourceInfo structure seems to be limiting. Using Unstructured would allow working with any type. The additional logic would then be able to work with fields specific to the type being processed.
  • The pod deletion strategy only considers succeeded pods. It should also consider failed pods. Basically all pods with the terminated state, possibly letting the user selects the phases that are candidates for pruning.
  • It may make more sense to use a closure for strategy rather than a structure strategyConfig and a func StrategyFunc. By doing so it would be possible to unify the interface for what is implemented by the library: pruneByMaxAge, pruneByMaxCount and what is provided by the user: CustomStrategy. They would all be a func, possibly passed as a Config field with this signature:
    func pruneStrategyt(resources []unstructured.Unstructured) (resourcesToRemove [][]unstructured.Unstructured, err error)
    
    Specific parameters part of the Config would be made available through closure variables. pruneByMaxCount would then not need a Config.strategy, which provides information meant for all possible strategies. It would make things easier to extend and the implementation would be more performant: No need for switch config.Strategy.Mode in prune.go. All the prune strategies would have the same signature.
  • The context is passed to the functions in the library but my feeling is that we have two use cases:
    • The prune logic is called from a reconciliation loop and the context may provide more information
    • The prune logic is called from a cron job (as per the documentation) and the context will provide little information
      I haven't seen it really used. Is it thought more as a safety net (if anything is needed that was not foreseen get it through the context)? Can this be addressed through the closure approach?
  • We should also aim and I believe the points listed above would help with that not to limit the logic to pods and jobs.
@fgiloux
Copy link
Contributor Author

fgiloux commented Jan 25, 2022

@ryantking ryantking self-assigned this Mar 28, 2022
everettraven added a commit to everettraven/operator-lib that referenced this issue Apr 6, 2022
enable pruning of more than just Pod & Job resources

related to operator-framework#101
everettraven added a commit to everettraven/operator-lib that referenced this issue Apr 6, 2022
explain why k8s objects created with unstructured.Unstructured

related to operator-framework#101
everettraven added a commit to everettraven/operator-lib that referenced this issue Apr 6, 2022
refactored to make more readable and added some default IsPruneableFunc functions

references operator-framework#101
everettraven added a commit to everettraven/operator-lib that referenced this issue Apr 7, 2022
more testing coverage of the auto pruning functionality

references operator-framework#101
everettraven added a commit to everettraven/operator-lib that referenced this issue Apr 7, 2022
commit 82011ff9f7ea80eaab282388642bf3c1a23c2fdc
Author: Bryce Palmer <bpalmer@redhat.com>
Date:   Thu Apr 7 11:19:36 2022 -0400

    add more test cases

    more testing coverage of the auto pruning functionality

    references operator-framework#101

    Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

commit 7a9d915
Author: Bryce Palmer <bpalmer@redhat.com>
Date:   Wed Apr 6 17:11:56 2022 -0400

    refactor and add defaults

    refactored to make more readable and added some default IsPruneableFunc functions

    references operator-framework#101

commit 24409f4
Author: Bryce Palmer <bpalmer@redhat.com>
Date:   Wed Apr 6 13:29:31 2022 -0400

    update a comment in tests

    explain why k8s objects created with unstructured.Unstructured

    related to operator-framework#101

commit 93334f6
Author: Bryce Palmer <bpalmer@redhat.com>
Date:   Wed Apr 6 13:24:52 2022 -0400

    first draft of updated pruning API

    enable pruning of more than just Pod & Job resources

    related to operator-framework#101

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven added a commit to everettraven/operator-lib that referenced this issue Apr 14, 2022
improve the existing prune package

fixes operator-framework#101

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven added a commit to everettraven/operator-lib that referenced this issue Apr 14, 2022
improve the existing prune package

fixes operator-framework#101

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven added a commit that referenced this issue May 10, 2022
update the existing pruning library to implement the new library outlined in the following EP: https://github.com/operator-framework/enhancements/blob/master/enhancements/automatic-resource-pruning.md

resolves #101
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants