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

[Housekeeping] Use passthrough workflowstore to perform CRD terminations / manipulations #2857

Closed
2 tasks done
Tracked by #2917
hamersaw opened this issue Sep 7, 2022 · 2 comments · Fixed by flyteorg/flytepropeller#496
Closed
2 tasks done
Tracked by #2917
Labels
good first issue Good for newcomers hacktoberfest housekeeping Issues that help maintain flyte and keep it tech-debt free propeller Issues related to flyte propeller

Comments

@hamersaw
Copy link
Contributor

hamersaw commented Sep 7, 2022

Describe the issue

FlytePropeller uses the workflowstore to abstract FltyeWorkflow CRD updates and retrievals from the k8s API. To provide additional functionality, this API is designed to wrap other implementations with additional layers. In the default-case this manifests as:
(1) the passthrough workflowstore which is a basic overlay of the k8s API.
(2) the resource version caching workflowstore which uses the k8s resource version to mitigate processing or updating stale workflows.

PRs for clearing managed fields from the CRD and tracking terminated workflows were implemented in the resource version caching workflowstore. This functionality should really be performed at the passthrough workflowstore level, or in another wrapped workflowstore.

What if we do not do this?

If users change configuration to remove the resource version caching workflowstore and instead use only the passthrough workflowstore then they will not benefit from these improvements.

Related component(s)

No response

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@hamersaw hamersaw added good first issue Good for newcomers propeller Issues related to flyte propeller housekeeping Issues that help maintain flyte and keep it tech-debt free labels Sep 7, 2022
@hamersaw hamersaw added this to the 1.3.0-candidate milestone Sep 7, 2022
@daniel-shuy
Copy link
Contributor

daniel-shuy commented Oct 20, 2022

I think the logic to clear CRD managed fields should be moved to the passthrough workflowstore, while the logic to track terminated workflows seems like it should be its own new wrapped workflowstore.

Question is, should the track terminated workflowstore wrap the passthrough workflowstore or the resource version caching workflowstore (or should the resource version caching workflowstore wrap the track terminated workflowstore)? The logic to track terminated workflows is unrelated to resource version caching, but if it wraps the passthrough workflowstore, then there would be no way to have both resource version caching and tracking terminated workflows together.

@daniel-shuy
Copy link
Contributor

daniel-shuy commented Oct 20, 2022

Another approach is to rework the workflow config to take in a list of policies, then chain them together. For example, if a new PolicyTrackTerminated policy is added, specifying both PolicyResourceVersionCaching and PolicyTrackTerminated will initialize a track terminated workflowstore that wraps a resource version caching workflowstore, which in turn wraps the passthrough workflowstore.

This however is a breaking change (or actually it may not be since a YAML string will be parsed as a single-element list). What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers hacktoberfest housekeeping Issues that help maintain flyte and keep it tech-debt free propeller Issues related to flyte propeller
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants