Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

add composable PluginCleanupPolicy (flyteorg/flyte#1345) #203

Merged
merged 8 commits into from
Sep 9, 2021

Conversation

clairemcginty
Copy link
Contributor

Read then delete this section

- Make sure to use a concise title for the pull-request.

- Use #patch, #minor or #major in the pull-request title to bump the corresponding version. Otherwise, the patch version
will be bumped. More details

TL;DR

Adds an optional interface that k8s plugins can implement, PluginCleanupPolicy, as an alternative to flytepropeller simply deleting the resource.

cc @regadas , @EngHabu

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

flyteorg/flyte#1345 Adds an optional interface that k8s plugins can implement, PluginCleanupPolicy, as an alternative to flytepropeller simply deleting the resource. The idea is that it would be used in flytepropeller as in this branch, where it checks if the plugin implements PluginCleanupPolicy and if so substitutes that action. For our use case we just need the Abort action override; I know flytepropeller does some cleanup actions in its finalizers also, but we don't need to override that.

Note: I'm not sure about what unit tests to add here. My plan was to add specific unit tests to flytepropeller to check that the cleanup policy is being selected when applicable. I've also tested end to end (the proposed changes to flyteplugins, flytepropeller, and my custom plugin) with a local setup on Minikube.

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

@welcome
Copy link

welcome bot commented Aug 23, 2021

Thank you for opening this pull request! 🙌
These tips will help get your PR across the finish line: - Most of the repos have a PR template; if not, fill it out to the best of your knowledge. - Sign off your commits (Reference: DCO Guide).

@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #203 (a5f77ec) into master (892f35e) will increase coverage by 0.32%.
The diff coverage is 70.08%.

❗ Current head a5f77ec differs from pull request most recent head a3af23e. Consider uploading reports for the commit a3af23e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
+ Coverage   60.68%   61.00%   +0.32%     
==========================================
  Files         135      138       +3     
  Lines        8313     8538     +225     
==========================================
+ Hits         5045     5209     +164     
- Misses       2806     2851      +45     
- Partials      462      478      +16     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
go/tasks/pluginmachinery/flytek8s/config/config.go 50.00% <ø> (ø)
go/tasks/plugins/array/array_tests_base.go 0.00% <0.00%> (ø)
go/tasks/plugins/array/catalog.go 45.60% <0.00%> (ø)
go/tasks/plugins/k8s/sagemaker/outputs.go 20.51% <0.00%> (ø)
go/tasks/plugins/k8s/spark/config.go 100.00% <ø> (ø)
go/tasks/plugins/presto/execution_state.go 50.48% <0.00%> (-0.50%) ⬇️
go/tasks/pluginmachinery/utils/task.go 53.84% <53.84%> (ø)
go/tasks/plugins/hive/execution_state.go 72.01% <60.00%> (+0.08%) ⬆️
...tasks/pluginmachinery/flytek8s/container_helper.go 83.52% <65.51%> (-0.69%) ⬇️
go/tasks/plugins/webapi/snowflake/plugin.go 67.08% <67.08%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 892f35e...a3af23e. Read the comment docs.

regadas
regadas previously approved these changes Aug 24, 2021
Copy link
Contributor

@regadas regadas left a comment

Choose a reason for hiding this comment

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

LGTM!

go/tasks/pluginmachinery/k8s/plugin.go Outdated Show resolved Hide resolved
go/tasks/pluginmachinery/k8s/plugin.go Outdated Show resolved Hide resolved
go/tasks/pluginmachinery/k8s/plugin.go Outdated Show resolved Hide resolved
go/tasks/pluginmachinery/k8s/plugin.go Outdated Show resolved Hide resolved
go/tasks/pluginmachinery/k8s/plugin.go Outdated Show resolved Hide resolved
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Looks great! thank you for adapting the design. just a couple of minor comments and we can merge it today!

go/tasks/pluginmachinery/k8s/plugin.go Outdated Show resolved Hide resolved
go/tasks/pluginmachinery/k8s/plugin.go Outdated Show resolved Hide resolved
go/tasks/pluginmachinery/k8s/plugin.go Outdated Show resolved Hide resolved
@EngHabu
Copy link
Contributor

EngHabu commented Sep 8, 2021

Just to speed up the next review, there seems to be some lint errors and DCO isn't happy... mind fixing those too?

clairemcginty and others added 7 commits September 9, 2021 01:57
Signed-off-by: Claire McGinty <[email protected]>
Co-authored-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
Co-authored-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>
@EngHabu EngHabu merged commit bd6c1b6 into flyteorg:master Sep 9, 2021
@welcome
Copy link

welcome bot commented Sep 9, 2021

Congrats on merging your first pull request! 🎉

@clairemcginty clairemcginty deleted the add_cleanup_policy branch September 10, 2021 14:35
EngHabu pushed a commit to flyteorg/flytepropeller that referenced this pull request Sep 14, 2021
* use PluginCleanupPolicy if it exists

Signed-off-by: Claire McGinty <[email protected]>

* add unit test

Signed-off-by: Claire McGinty <[email protected]>

* cleanup test code

Signed-off-by: Claire McGinty <[email protected]>

* assert OnAbort is not attempted by default

Signed-off-by: Claire McGinty <[email protected]>

* update plugin override interface

Signed-off-by: Claire McGinty <[email protected]>

* lint

Signed-off-by: Claire McGinty <[email protected]>

* Apply PR suggestions

Signed-off-by: Claire McGinty <[email protected]>

* update flyteplugins lib

Signed-off-by: Claire McGinty <[email protected]>
pingsutw pushed a commit to pingsutw/flyte-monorepo that referenced this pull request Apr 4, 2023
* use PluginCleanupPolicy if it exists

Signed-off-by: Claire McGinty <[email protected]>

* add unit test

Signed-off-by: Claire McGinty <[email protected]>

* cleanup test code

Signed-off-by: Claire McGinty <[email protected]>

* assert OnAbort is not attempted by default

Signed-off-by: Claire McGinty <[email protected]>

* update plugin override interface

Signed-off-by: Claire McGinty <[email protected]>

* lint

Signed-off-by: Claire McGinty <[email protected]>

* Apply PR suggestions

Signed-off-by: Claire McGinty <[email protected]>

* update flyteplugins lib

Signed-off-by: Claire McGinty <[email protected]>
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* add composable PluginCleanupPolicy

Signed-off-by: Claire McGinty <[email protected]>

* fix generated mocks

Signed-off-by: Claire McGinty <[email protected]>

* use PluginAbortOverride in PluginProperties

Signed-off-by: Claire McGinty <[email protected]>

* Update go/tasks/pluginmachinery/k8s/plugin.go

Co-authored-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>

* add custom constructors for AbortBehavior

Signed-off-by: Claire McGinty <[email protected]>

* Apply Ketan's suggestions

Signed-off-by: Claire McGinty <[email protected]>

* Apply suggestions from code review

Co-authored-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Claire McGinty <[email protected]>

* lint/goimports

Signed-off-by: Claire McGinty <[email protected]>

Co-authored-by: Haytham Abuelfutuh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants