Move workflowstore logic to correct abstraction

[daniel-shuy]

TL;DR

• Move logic to clear managed fields from the CRD from the resource version caching workflowstore to the passthrough workflowstore
• Move logic to track terminated workflows from the resource version caching workflowstore to a new terminated tracking workflowstore

Type

• Bug Fix
• Feature
• Plugin
• Housekeeping

Are all requirements met?

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

Complete description

• The logic to clear managed fields from the CRD was moved to the passthrough workflowstore (the base workflowstore) since it seems like a core behavior that all workflowstores should have.
• The logic to track terminated workflows was moved to a new terminated tracking workflowstore because it doesn't seem like a core behavior.
• I changed the resource version caching workflowstore to wrap the new terminated tracking workflowstore so that there's no breaking changes (resource version caching workflowstore will continue to track terminated workflows).

I wonder if the workflowstore API structure could be improved. At the moment it is impossible to enable the resource version caching without also enabling the terminated tracking. Maybe the configuration can be changed to take in a list of policies, and the workflowstores would chain one another, e.g.

var workflowStore FlyteWorkflow
if cfg.InMemory {
    workflowStore = NewInMemoryWorkflowStore()
} else {
    workflowStore = NewPassthroughWorkflowStore(ctx, scope, workflows, lister)
}

var err error
for _, policy = cfg.Policy {
    switch policy {
	case PolicyTrackTerminated:
		workflowStore, err = NewTerminatedTrackingStore(ctx, scope, workflowStore)
	case PolicyResourceVersionCache:
		workflowStore = NewResourceVersionCachingStore(ctx, scope, workflowStore)
    }
}

Tracking Issue

fixes flyteorg/flyte#2857

Follow-up issue

NA

[codecov]

Codecov Report

Merging #496 (c4c0ac3) into master (7ffb502) will increase coverage by 0.03%.
The diff coverage is 62.50%.

[hamersaw]

@daniel-shuy thanks so much for your patience on this. I think moving the managed fields clearing to the passthrough and creating a new "tracking terminated" is the correct approach. Thanks for the initiative!

Two things to get this merged:
(1) Can you add DCO signoff to the commit? Since we are Linux Foundation project it is required - typically a quick google search shows a few ways to do this. Let me know if you want me to help with this!
(2) Looks like there is a small lint issue in the tests. Should be able to define some constants to fix it. The utility in this case is questionable, but generally the linter has good suggestions to keep the code relatively uniform, so we try our hardest to keep it happy.

[daniel-shuy]

@hamersaw Thanks for confirming! Any comment on the proposal to improve the workflowstore configuration structure?

[daniel-shuy]

Its annoying that the lint is failing even for an existing test (resource_version_caching_test.go). What's even more annoying is that its suggesting to use const, but because the consts will be package scoped, the tests will pollute each others scope (sometimes it frustrates me that Golang doesn't have file level scopes 🤦‍♂️)

[hamersaw]

@hamersaw Thanks for confirming! Any comment on the proposal to improve the workflowstore configuration structure?

I agree that this idea of chaining policies would make this solution a little cleaner. My only concern is the backwards comparability of this solution. Currently everybody should be running the "ResourceVersionCaching" workflow store - it's the default and is really the only one that makes sense. Creating a new "TrackingTerminated" from a code standpoint is the correct abstraction. However, there really is no reason to only have the "TrackingTerminated" policy without the "ResrouceVersionCaching". And we want as many people as possible to benefit from these changes without having to update their existing configuration.

In a perfect world, we would:
(1) implement configuration for the policy chaining as you suggested
(2) create a default configuration in FlytePropeller with the terminated tracking and resource version caching so that anytime FlytePropeller is created is uses these unless the user specifically overrides the defaults.
(3) ensure that there is no specific setting of these policies in the default deployment helm charts. if the policy is defaulted to something, then these would always override and this would never be deployed. alternatively, we could use a new configuration value and deprecate the existing policy (maybe "policies" and make it a list)?

This is a bit of work and we would have to do a bit of testing to 100% be sure that the tracking terminated is available, because we can not have a regression here. I'll leave it up to you if you want to scope this work to cover this!

[hamersaw]

sometimes it frustrates me that Golang doesn't have file level scopes

a LOT of times it frustrates me that Golang doesn't use file level scopes!

[daniel-shuy]

there really is no reason to only have the "TrackingTerminated" policy without the "ResrouceVersionCaching"

@hamersaw I see,thanks for the explanation. So this abstraction is only meant for code maintainability, not configurability. In that case I guess there's no need to change it.

[hamersaw]

@daniel-shuy looks like the DCO is giving a bit of trouble - this may help. You can rebase with signoff with git rebase HEAD~2 --signoff to signoff the last two commits and then force push.

[daniel-shuy]

@hamersaw oops I forgot I had 2 commits and only rebased the last