Introduce default plugin for task type #199
Conversation
Co-authored-by: Haytham AbuelFutuh <habuelfutuh@lyft.com>
Co-authored-by: Haytham AbuelFutuh <habuelfutuh@lyft.com>
|
friendly ping @EngHabu |
|
ping @EngHabu |
There was a problem hiding this comment.
I think we are close!
I'll try to write down my understand and plz correct me where I got it wrong.
We have these different settings:
1- Plugin Author decides which task types a plugin can handle,
2- Plugin Author decides which task types a plugin can be the default for (We should consider punting on this)
3- Admin decides which plugins are enabled
4- Admin decides which plugins are defaults for which task types
Some validation before attempting the resolution:
- If a plugin exist in Bump IDL/Plugins versions #4 but the plugin isn't enabled (not in Add build-tool cli with a crd validation command #3 nor allpluginsEnabled) or doesn't exist, fail?
The resolution for which defaults are should be:
- If a task type exist in Bump IDL/Plugins versions #4, that plugin choice wins
- Otherwise, tasks defined in support GCP as auth provider #2 take precedence, if there is a conflict, fail
I think we should consider getting rid of #2 and just consider #1 as such... In what scenarios would these be different?
pkg/controller/nodes/task/handler.go
Outdated
| for _, defaultTaskType := range p.DefaultForTaskTypes { | ||
| if defaultTaskType == tt { | ||
| if existingHandler, alreadyDefaulted := t.defaultPlugins[tt]; alreadyDefaulted && existingHandler.GetID() != cp.GetID() { | ||
| logger.Panicf(ctx, "TaskType [%s] has multiple default handlers specified: [%s] and [%s]", |
There was a problem hiding this comment.
nit: I think you can just return an error... and let the top layer panic... I generally think panic should happen at the very top layer of any application (specifically the top most layer that cannot propagate errors any further) and everywhere else we should just propagate errors (Must*-style functions is a sort-of an exception)
pkg/controller/nodes/task/handler.go
Outdated
| break | ||
| } | ||
| if len(registeredPlugins) != 1 { | ||
| logger.Panicf(ctx, "Multiple plugins registered to handle task type: %s. ([%+v])", taskType, registeredPlugins) |
2 is not supported. Plugin authors cannot specify defaults, only registered tasks. Plugin resolution in propeller should ignore any default values plugin authors attempt to set. re: 3 this will require an idl and admin change. currently admin only provides overrides for specific plugins for a task type. if we want the net affect of this change to be 3, such that the defaults are specified in admin rather than in the propeller config I can close this PR and take that approach. |
Co-authored-by: Haytham AbuelFutuh <habuelfutuh@lyft.com>
|
re 2: see why it's not supported currently here: #199 (comment) (one of your comments was on an outdated file version, so not sure if that led to any confusion) i updated to error when a default for a task type is specified for a plugin that is not enabled. other than that the order of default preference should indeed be 4, then 1 |
Co-authored-by: Haytham AbuelFutuh <habuelfutuh@lyft.com>
…ugins are enabled
Co-authored-by: Haytham AbuelFutuh <habuelfutuh@lyft.com>
Co-authored-by: Haytham AbuelFutuh <habuelfutuh@lyft.com>
…ller into per-task-default-plugins
TL;DR
Introduce default plugin for task type configurable in the flytepropeller config.
Type
Are all requirements met?
Complete description
Tracking Issue
flyteorg/flyte#516
Follow-up issue
NA