Move caching to node executor for fast cache hits #485

hamersaw · 2022-09-23T16:34:20Z

TL;DR

Currently cache lookups require 3 node-level phase changes, which equates to 3 streak rounds of flytepropeller execution. The overhead of updating the k8s FlyteWorkflow CRD after each of these rounds may make it seem like cache lookups are slow. This PR updates cache lookups to happen in the node executor, which allows us to transition nodes directly from NotYetStarted to Success and immediately process downstream nodes in the same FlytePropeller round making cache lookup overhead minimal.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

In addition to the performance improvements this PR refactors the code to support future implementation of caching other Flyte node types (ie. subworkflow / launchplan and array nodes when they are implemented).

Tracking Issue

fixes flyteorg/flyte#2886

Follow-up issue

NA

Signed-off-by: Daniel Rammer <[email protected]>

…acheableNode interface Signed-off-by: Daniel Rammer <[email protected]>

Signed-off-by: Daniel Rammer <[email protected]>

…ents Signed-off-by: Daniel Rammer <[email protected]>

Signed-off-by: Daniel Rammer <[email protected]>

codecov · 2022-10-12T03:17:37Z

Codecov Report

Merging #485 (5125f31) into master (cbfcdf3) will increase coverage by 0.25%.
The diff coverage is 57.18%.

❗ Current head 5125f31 differs from pull request most recent head 5c7c3c7. Consider uploading reports for the commit 5c7c3c7 to get more accurate results

Signed-off-by: Daniel Rammer <[email protected]>

…xecution of cached Signed-off-by: Daniel Rammer <[email protected]>

Signed-off-by: Daniel Rammer <[email protected]>

kumare3 · 2023-08-08T23:07:34Z

pkg/controller/nodes/array/handler.go

@@ -295,6 +295,18 @@ func (a *arrayNodeHandler) Handle(ctx context.Context, nCtx interfaces.NodeExecu

 			retryAttempt := subNodeStatus.GetAttempts()

+			// fastcache will not emit task events for cache hits. we need to manually detect a
+			// transition to `SUCCEEDED` and add an `ExternalResourceInfo` for it.
+			if cacheStatus == idlcore.CatalogCacheStatus_CACHE_HIT && len(arrayEventRecorder.TaskEvents()) == 0 {


is there one potential problem here, consider this case

Round 1:
node 1 - cache-hit
node 2 - cache-hit
node 3 - system error
Round 2:
node 1 - cache hit
node 2 - cache miss
- event write failure?

Or maybe this will not happen, as when we actually get a system error I think we update the state?
is it possible to simulate this and test this?

One other option is allow admin to move the state from success to running!

For the system error on node 3 are is this a Flyte retryable error where we attempt the task again, or a propeller error? In the former case, correct - we persist state and handle this. In the later case, we may get failures in updating ultimately abort the workflow. I think this is how it would work with the current TaskHandler caching though too right?

pkg/controller/nodes/array/handler.go

pkg/controller/nodes/executor.go

kumare3

I have one major comment, otherwise the code looks so much better.
This makes it actually way more readable. Not sure why the caching stuff was at the task level at all 🗡️

Signed-off-by: Daniel Rammer <[email protected]>

kumare3

thank you for the explanation

Signed-off-by: Daniel Rammer <[email protected]>

* fast cache working-ish Signed-off-by: Daniel Rammer <[email protected]> * processing downstream immediately on cache hit Signed-off-by: Daniel Rammer <[email protected]> * moved cache write to node executor Signed-off-by: Daniel Rammer <[email protected]> * working cache and cache serialize Signed-off-by: Daniel Rammer <[email protected]> * starting to clean up Signed-off-by: Daniel Rammer <[email protected]> * removed commented out code Signed-off-by: Daniel Rammer <[email protected]> * removed separate IsCacheable and IsCacheSerializable functions from CacheableNode interface Signed-off-by: Daniel Rammer <[email protected]> * refactored reservation owner id to new function to remove duplication Signed-off-by: Daniel Rammer <[email protected]> * added cache metrics to the node executor Signed-off-by: Daniel Rammer <[email protected]> * cleaned up node cache.go Signed-off-by: Daniel Rammer <[email protected]> * more cleanup Signed-off-by: Daniel Rammer <[email protected]> * setting cache information in phase info so that it is available in events Signed-off-by: Daniel Rammer <[email protected]> * minor refactoring and bug fixes Signed-off-by: Daniel Rammer <[email protected]> * doing an outputs lookup on cache to ensure correctness during failures Signed-off-by: Daniel Rammer <[email protected]> * fix unit tests Signed-off-by: Daniel Rammer <[email protected]> * fixed lint issues Signed-off-by: Daniel Rammer <[email protected]> * moved catalog package to the node level Signed-off-by: Daniel Rammer <[email protected]> * refactored task handler Signed-off-by: Daniel Rammer <[email protected]> * fixed catalog imports on unit testes Signed-off-by: Daniel Rammer <[email protected]> * started cache unit tests Signed-off-by: Daniel Rammer <[email protected]> * added CheckCatalogCache unit tests Signed-off-by: Daniel Rammer <[email protected]> * unit tests for node cache file Signed-off-by: Daniel Rammer <[email protected]> * added node executor cache unit tests Signed-off-by: Daniel Rammer <[email protected]> * fixed cache unit tets Signed-off-by: Daniel Rammer <[email protected]> * fixed lint issues Signed-off-by: Daniel Rammer <[email protected]> * transitioning to 'Succeeded' immediately on cache hit Signed-off-by: Daniel Rammer <[email protected]> * supporting cache overwrite Signed-off-by: Daniel Rammer <[email protected]> * fixed lint issues Signed-off-by: Daniel Rammer <[email protected]> * removed automatic downstream on cache hit Signed-off-by: Daniel Rammer <[email protected]> * bumping boilerplate support tools to go 1.19 to fix generate Signed-off-by: Daniel Rammer <[email protected]> * fixed unit tests and linter Signed-off-by: Daniel Rammer <[email protected]> * removed unnecessary async catalog client from nodeExecutor Signed-off-by: Daniel Rammer <[email protected]> * general refactoring Signed-off-by: Daniel Rammer <[email protected]> * fastcache working with arraynode Signed-off-by: Daniel Rammer <[email protected]> * fixed unit tests - no longer checking for output existance on first execution of cached Signed-off-by: Daniel Rammer <[email protected]> * updating documentation TODOs Signed-off-by: Daniel Rammer <[email protected]> * updated arraynode fastcache to correctly report cache hits Signed-off-by: Daniel Rammer <[email protected]> * remove print statement Signed-off-by: Daniel Rammer <[email protected]> * fixed cache serialize Signed-off-by: Daniel Rammer <[email protected]> * fixed unit tests Signed-off-by: Daniel Rammer <[email protected]> --------- Signed-off-by: Daniel Rammer <[email protected]>

hamersaw added 26 commits September 22, 2022 20:14

fast cache working-ish

817ae4b

Signed-off-by: Daniel Rammer <[email protected]>

processing downstream immediately on cache hit

16d3fbc

Signed-off-by: Daniel Rammer <[email protected]>

Merge branch 'master' into feature/fast-cache-move-to-node

43f40e7

Signed-off-by: Daniel Rammer <[email protected]>

moved cache write to node executor

08eda31

Signed-off-by: Daniel Rammer <[email protected]>

working cache and cache serialize

ac773ae

Signed-off-by: Daniel Rammer <[email protected]>

starting to clean up

c3464da

Signed-off-by: Daniel Rammer <[email protected]>

removed commented out code

3f0b705

Signed-off-by: Daniel Rammer <[email protected]>

removed separate IsCacheable and IsCacheSerializable functions from C…

fcb9174

…acheableNode interface Signed-off-by: Daniel Rammer <[email protected]>

refactored reservation owner id to new function to remove duplication

46b525e

Signed-off-by: Daniel Rammer <[email protected]>

added cache metrics to the node executor

82a76c5

Signed-off-by: Daniel Rammer <[email protected]>

cleaned up node cache.go

1a33a4d

Signed-off-by: Daniel Rammer <[email protected]>

more cleanup

783858a

Signed-off-by: Daniel Rammer <[email protected]>

setting cache information in phase info so that it is available in ev…

43c6827

…ents Signed-off-by: Daniel Rammer <[email protected]>

minor refactoring and bug fixes

6310902

Signed-off-by: Daniel Rammer <[email protected]>

doing an outputs lookup on cache to ensure correctness during failures

502d749

Signed-off-by: Daniel Rammer <[email protected]>

fix unit tests

fe2d056

Signed-off-by: Daniel Rammer <[email protected]>

fixed lint issues

a3e1581

Signed-off-by: Daniel Rammer <[email protected]>

moved catalog package to the node level

8a49895

Signed-off-by: Daniel Rammer <[email protected]>

refactored task handler

6471ce8

Signed-off-by: Daniel Rammer <[email protected]>

fixed catalog imports on unit testes

a0e4619

Signed-off-by: Daniel Rammer <[email protected]>

started cache unit tests

439af82

Signed-off-by: Daniel Rammer <[email protected]>

added CheckCatalogCache unit tests

02e314c

Signed-off-by: Daniel Rammer <[email protected]>

unit tests for node cache file

4b8c241

Signed-off-by: Daniel Rammer <[email protected]>

added node executor cache unit tests

757d3b3

Signed-off-by: Daniel Rammer <[email protected]>

fixed cache unit tets

3dc1f14

Signed-off-by: Daniel Rammer <[email protected]>

fixed lint issues

4093692

Signed-off-by: Daniel Rammer <[email protected]>

hamersaw added 3 commits October 19, 2022 13:02

transitioning to 'Succeeded' immediately on cache hit

270396a

Signed-off-by: Daniel Rammer <[email protected]>

Merge branch 'master' into feature/fast-cache-move-to-node

aa67293

Signed-off-by: Daniel Rammer <[email protected]>

supporting cache overwrite

0b65120

Signed-off-by: Daniel Rammer <[email protected]>

hamersaw requested review from kumare3 and EngHabu as code owners April 3, 2023 19:01

hamersaw requested a review from eapolinario April 3, 2023 19:01

hamersaw added 10 commits August 1, 2023 10:30

Merge branch 'master' into feature/fast-cache-move-to-node

b940d00

Signed-off-by: Daniel Rammer <[email protected]>

bumping boilerplate support tools to go 1.19 to fix generate

5ea74d8

Signed-off-by: Daniel Rammer <[email protected]>

fixed unit tests and linter

cabfc58

Signed-off-by: Daniel Rammer <[email protected]>

removed unnecessary async catalog client from nodeExecutor

95bf8e1

Signed-off-by: Daniel Rammer <[email protected]>

general refactoring

5d019bf

Signed-off-by: Daniel Rammer <[email protected]>

fastcache working with arraynode

e0f6715

Signed-off-by: Daniel Rammer <[email protected]>

fixed unit tests - no longer checking for output existance on first e…

f3ca83c

…xecution of cached Signed-off-by: Daniel Rammer <[email protected]>

updating documentation TODOs

15c46af

Signed-off-by: Daniel Rammer <[email protected]>

Merge branch 'master' into feature/fast-cache-move-to-node

fa721e6

Signed-off-by: Daniel Rammer <[email protected]>

updated arraynode fastcache to correctly report cache hits

9f8b7d9

Signed-off-by: Daniel Rammer <[email protected]>

kumare3 reviewed Aug 8, 2023

View reviewed changes

pkg/controller/nodes/array/handler.go Outdated Show resolved Hide resolved

kumare3 reviewed Aug 8, 2023

View reviewed changes

pkg/controller/nodes/executor.go Show resolved Hide resolved

kumare3 reviewed Aug 8, 2023

View reviewed changes

remove print statement

4347b6d

Signed-off-by: Daniel Rammer <[email protected]>

kumare3 previously approved these changes Aug 9, 2023

View reviewed changes

fixed cache serialize

9245f3e

Signed-off-by: Daniel Rammer <[email protected]>

hamersaw dismissed kumare3’s stale review via 9245f3e August 14, 2023 19:25

hamersaw added 2 commits August 14, 2023 14:42

fixed unit tests

c672b07

Signed-off-by: Daniel Rammer <[email protected]>

Merge branch 'master' into feature/fast-cache-move-to-node

5c7c3c7

Signed-off-by: Daniel Rammer <[email protected]>

hamersaw mentioned this pull request Aug 14, 2023

[Housekeeping] Remove datacatalog client from TaskExecutionContext flyteorg/flyte#3957

Open

2 tasks

kumare3 approved these changes Aug 14, 2023

View reviewed changes

hamersaw merged commit 6e06386 into master Aug 14, 2023

hamersaw deleted the feature/fast-cache-move-to-node branch August 14, 2023 22:39

hamersaw mentioned this pull request Dec 19, 2023

Fixing cache overwrite metadata update flyteorg/flyte#4617

Merged

3 tasks

hamersaw mentioned this pull request Jan 2, 2024

[Core feature] Allow caching of launchplans/subworkflows flyteorg/flyte#2734

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move caching to node executor for fast cache hits #485

Move caching to node executor for fast cache hits #485

hamersaw commented Sep 23, 2022 •

edited

Loading

codecov bot commented Oct 12, 2022 •

edited

Loading

kumare3 Aug 8, 2023

kumare3 Aug 8, 2023

hamersaw Aug 9, 2023

kumare3 left a comment

kumare3 left a comment

Move caching to node executor for fast cache hits #485

Move caching to node executor for fast cache hits #485

Conversation

hamersaw commented Sep 23, 2022 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented Oct 12, 2022 • edited Loading

Codecov Report

kumare3 Aug 8, 2023

Choose a reason for hiding this comment

kumare3 Aug 8, 2023

Choose a reason for hiding this comment

hamersaw Aug 9, 2023

Choose a reason for hiding this comment

kumare3 left a comment

Choose a reason for hiding this comment

kumare3 left a comment

Choose a reason for hiding this comment

hamersaw commented Sep 23, 2022 •

edited

Loading

codecov bot commented Oct 12, 2022 •

edited

Loading