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

Handle WorkflowClosure from storage #459

Merged
merged 28 commits into from
Aug 17, 2022

Conversation

ckiosidis
Copy link
Contributor

@ckiosidis ckiosidis commented Jul 20, 2022

Extends the FlyteWorkflow crd to contain a location reference to the workflowClosure is stored as protobuf
Introduces a in memory cache of loading the static json fields and reusing them in the FlyteWorkflow CRD if the DataReference is present.

TL;DR

Please replace this text with a description of what this PR accomplishes.

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

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#2705

Follow-up issue

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

@ckiosidis ckiosidis force-pushed the add-data-reference-fields-to-crd branch 2 times, most recently from 3167be8 to 36b39cd Compare July 20, 2022 12:52
@ckiosidis
Copy link
Contributor Author

@hamersaw

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #459 (ce3a422) into master (4938905) will decrease coverage by 0.05%.
The diff coverage is 42.00%.

❗ Current head ce3a422 differs from pull request most recent head 477c445. Consider uploading reports for the commit 477c445 to get more accurate results

@ckiosidis ckiosidis force-pushed the add-data-reference-fields-to-crd branch 8 times, most recently from 7c81a43 to 840c098 Compare July 25, 2022 12:37
@ckiosidis ckiosidis changed the title add data reference fields Handle CRDs that contain offloaded json static fields Jul 26, 2022
assert.Equal(t, 1, len(r.Finalizers))
assert.False(t, HasCompletedLabel(r))
assert.Equal(t, uint32(0), r.Status.FailedAttempts)
assert.Nil(t, r.WorkflowSpec)

Choose a reason for hiding this comment

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

Shouldn't it return the one with ID: "static-id"? instead of nil

pkg/controller/handler_test.go Outdated Show resolved Hide resolved
@ckiosidis ckiosidis force-pushed the add-data-reference-fields-to-crd branch 5 times, most recently from 9525a8a to f877e36 Compare July 28, 2022 14:14
@ckiosidis ckiosidis changed the title Handle CRDs that contain offloaded json static fields Handle WorkflowClosure from storage Aug 8, 2022
Copy link
Contributor

@hamersaw hamersaw left a comment

Choose a reason for hiding this comment

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

Few nits. Let's get this merged in the next few days!

pkg/controller/handler.go Outdated Show resolved Hide resolved
pkg/controller/handler_test.go Outdated Show resolved Hide resolved
pkg/controller/workflowclosurestore/active.go Outdated Show resolved Hide resolved
pkg/controller/workflowclosurestore/iface.go Outdated Show resolved Hide resolved
pkg/controller/workflowclosurestore/passthrough.go Outdated Show resolved Hide resolved
pkg/controller/handler.go Outdated Show resolved Hide resolved
@ckiosidis ckiosidis force-pushed the add-data-reference-fields-to-crd branch 2 times, most recently from f7a0c13 to 1b4c6cf Compare August 9, 2022 09:32
Babis Kiosidis and others added 26 commits August 17, 2022 16:11
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
and clear fields before updating the status

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Co-authored-by: Ketan Umare <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
@ckiosidis ckiosidis force-pushed the add-data-reference-fields-to-crd branch from 33350b5 to 477c445 Compare August 17, 2022 13:11
@hamersaw hamersaw merged commit fd7b180 into flyteorg:master Aug 17, 2022
@ckiosidis ckiosidis deleted the add-data-reference-fields-to-crd branch August 17, 2022 13:36
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
* add data reference fields

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* combine crd parts into one object/location

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* create and pass static obj storage to propeller handler

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* read static blob at the beginning
and clear fields before updating the status

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* formatting

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* pass tests

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* remove terminated wfs' blob obj from cache

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* no return on remove blob method

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* test happy offloaded spec scenario

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* set static fields on every streak

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* formatting

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* added crdOffloadStore interface

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

* made crdoffloadstore configurable

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

* add metrics/formatting

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* load static workflow data outside streak loop

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* cleaned up metric reporting

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

* renamed inmemory to active

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

* added lruCRDOffloadStore unit tests

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

* added activeCRDOffloadStore unit tests

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

* added unit test for offloading crd error on handle

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

* handle offloaded WorkflowClosure instead of parts of the crd

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* cache wf crd fields instead of wf closure

Signed-off-by: Babis Kiosidis <[email protected]>

* Update pkg/controller/controller.go

Co-authored-by: Ketan Umare <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>

* reading workflow closure directly from data store rather than cache

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

* added prometheus metric

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

* updated comments

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

* fixed unit tests

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

* fixed lint issue

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

Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Babis Kiosidis <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Co-authored-by: Babis Kiosidis <[email protected]>
Co-authored-by: Daniel Rammer <[email protected]>
Co-authored-by: Ketan Umare <[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