[2/n] Implement application logics to delete and handle deleted artifact contents #1399

likawind · 2023-06-01T19:09:45Z

Describe your changes and why you are making these changes

This PR adds application logics to handle artifact content deletion if the user choose to opt-out snapshotting. The outline of the work lifecycle follows these steps:

During wf registration, we assume client provides
the right should_persist tag to each artifact based on user inputs
During wf preview, no change is made there
During wf execution, no change is made on the execution logic.
During wf execution, we modified DB updates such that artifact with should_persist will be directly marked as 'deleted'
After wf execution, we added a step in eng.ExecuteWorkflow() that deletes all necessary artf contents. Note that this cannot be done during execution as downstream op may need still need to read the contents.
All APIs that read results returns content whenever possible regardless of exec state. Thus no change is necessary. The only caveat is that during execution, there will be time where the artf is marked as deleted while the content is still available, and we may want to have the client hide the content to provide consistent user view.

Related issue number (if any)

ENG-3000

Tests

Integration test should pass on this PR
We will test through this feature in follow up PRs after client side changes are done

Checklist before requesting a review

I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
I have performed a self-review of my code.
I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
If this is a new feature, I have added unit tests and integration tests.
I have run the integration tests locally and they are passing.
I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
All features on the UI continue to work correctly.
Added one of the following CI labels:
- run_integration_test: Runs integration tests
- skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

…to eng-3000-allow-users-to-opt-out-of-data-snapshots-2

kenxu95

All APIs that read results returns content whenever possible regardless of exec state. Thus no change is necessary. The only caveat is that during execution, there will be time where the artf is marked as deleted while the content is still available, and we may want to have the client hide the content to provide consistent user view.

Is this consistent view addressed in this PR, or is that a follow-up? I assume there will be additional PRs that deal with the client?

src/golang/lib/engine/aq_engine.go

kenxu95 · 2023-06-01T20:22:07Z

src/golang/lib/models/shared/execution_status.go

@@ -23,7 +23,11 @@ const (
 	CanceledExecutionStatus   ExecutionStatus = "canceled"
 	FailedExecutionStatus     ExecutionStatus = "failed"
 	SucceededExecutionStatus  ExecutionStatus = "succeeded"
-	UnknownExecutionStatus    ExecutionStatus = "unknown"
+	// 'erased' refers to 'erased after success'.


What is "erased" here mean?

I think here is a good place to lay out what the lifecycle is, with an ascii diagram:
Registered -> Pending -> Running -> Failed
.......................................................-> Cancelled
.......................................................-> Succeeded -> Deleted [Optional]

kenxu95 · 2023-06-01T20:25:51Z

src/golang/lib/workflow/artifact/artifact.go

@@ -25,6 +25,12 @@ type Artifact interface {
 	Signature() uuid.UUID
 	Type() shared.ArtifactType
 	Name() string
+	ShouldPersistContent() bool
+
+	// DeleteContent removes the artifact content from storage.


Can you note that this only deletes the latest artifact result? Also, what happens if the artifact content was never written due to workflow failure.

It only try to delete the content when it's computed, we can clarify that. I don't think it's the latest result, in fact in the context of this artifact object (which has a result attached), we delete content of that result

kenxu95 · 2023-06-01T20:28:18Z

src/golang/lib/engine/aq_engine.go

@@ -147,6 +147,15 @@ func (eng *aqEngine) ScheduleWorkflow(
 	return nil
 }

+func (eng *aqEngine) deleteTemporaryArtifactContents(ctx context.Context, dag dag_utils.WorkflowDag) {


Can you note here that this is best effort. Also, that it only deletes the latest artifact result right?

I think it's a bit weird to explicitly note about the latest result. Similar to the other commend, current dag / op / artf objects have a result included and is used to handle the lifecycle of 'one workflow execution'. And all operations over these objects are tied to that execution. In this way I feel which result to delete is clear from the context? Also technically, if we reconstruct a workflow from an older run, we can use the same method to delete contents from that run as well

current dag / op / artf objects have a result included and is used to handle the lifecycle of 'one workflow execution'.

I think I'm confused cus you can execute the same dag / op/ artf multiple times. An artifact is different than an artifact result so as soon as I see this method signature I'm thinking "Which artifact result am I deleting". But ok I think I see what you mean, an artf in orchestration means something different than an artifact in our data model? This seemed like implicit context I wanted to call out.

I don't think our current abstraction allows us to re-execute a dag by calling ExecuteWorkflow(dag) multiple times? If you look at the constructor, we explicitly require a result ID to pass in to construct the WorkflowDag object: NewWorkflowDag(..., dagResultID,...) . In this way I think our abstraction is explicitly designed to have one object track one result.

I think it's a good idea to clarify this context somewhere, and I will add some comments to WorkflowDag object's definition. I just don't feel it's specific to this feature so we don't have to call out that on the specific API we added

Yeah I'm not saying I want us to change the way we execute, I just wanted that context clarified somewhere cus I do think it's an important callout. Agreed we can put it at a higher layer (maybe in orchestration or as you said, in workflowDag)

Sg! I updated comments for wfDag / artf / op objects, lmk if you have suggestions on where else we can clarify this

likawind · 2023-06-01T20:59:04Z

@kenxu95 yes client change will be in future PRs

likawind · 2023-06-01T21:13:41Z

@kenxu95 updated comments!

kenxu95 · 2023-06-01T21:22:57Z

src/golang/lib/workflow/operator/base.go

@@ -372,6 +372,10 @@ func (bo *baseOperator) PersistResult(ctx context.Context) error {
 			artifactExecState.Status = shared.CanceledExecutionStatus
 		}

+		if artifactExecState.Status == shared.SucceededExecutionStatus && !outputArtifact.ShouldPersistContent() {
+			artifactExecState.Status = shared.DeletedExecutionStatus


This setting of status to Deletion before the artifact is even deleted seems confusing actually, can we not update the status in DeleteContent() instead? That way all the deletion logic is handled in one place.

In this way the artifact will be marked as succeeded during execution and deleted later. I'm not sure if it's going to confuse users since they will see the intermediate succeed state

Yes - I actually find that less confusing since it very clearly follows the lifecycle, where deleted is just a future state of succeeded. Right now, we technically would have operators using artifacts already marked as deleted, which seems weirder?

sounds good, let's do what you suggested

kenxu95

Unblocking with a couple final thoughts. Thanks!

src/golang/lib/engine/aq_engine.go

kenxu95 · 2023-06-05T19:12:10Z

src/golang/lib/workflow/artifact/artifact.go

+		} else {
+			a.execState.Status = shared.DeletedExecutionStatus
+			a.execState.Timestamps.DeletedAt = &now
+		}


This seems like logic we could potentially push down into SetExecState()? If that helper can make updates more additively, it'll simplify all the callers a lot.

likawind added 3 commits May 31, 2023 17:06

tmp

50968ef

Merge branch 'eng-3000-allow-users-to-opt-out-of-data-snapshots-1' in…

01a9c05

…to eng-3000-allow-users-to-opt-out-of-data-snapshots-2

should work

341f234

likawind requested review from hsubbaraj-spiral and kenxu95 June 1, 2023 19:09

kenxu95 requested changes Jun 1, 2023

View reviewed changes

Address comments

fea7a62

nit

eed4bde

kenxu95 reviewed Jun 1, 2023

View reviewed changes

comments and timestamps

383d03a

likawind requested a review from kenxu95 June 1, 2023 23:25

kenxu95 approved these changes Jun 5, 2023

View reviewed changes

likawind added 2 commits June 5, 2023 21:15

[3/n] Allows user to disable snapshots from SDK (#1400)

f557240

comments

a5ec567

likawind merged commit 836e1d2 into eng-3000-allow-users-to-opt-out-of-data-snapshots-1 Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2/n] Implement application logics to delete and handle deleted artifact contents #1399

[2/n] Implement application logics to delete and handle deleted artifact contents #1399

likawind commented Jun 1, 2023

kenxu95 left a comment

kenxu95 Jun 1, 2023

kenxu95 Jun 1, 2023

kenxu95 Jun 1, 2023

likawind Jun 1, 2023

kenxu95 Jun 1, 2023

likawind Jun 1, 2023

kenxu95 Jun 1, 2023

likawind Jun 1, 2023

kenxu95 Jun 1, 2023

likawind Jun 1, 2023

likawind commented Jun 1, 2023

likawind commented Jun 1, 2023

kenxu95 Jun 1, 2023

likawind Jun 1, 2023

kenxu95 Jun 1, 2023

likawind Jun 1, 2023

kenxu95 left a comment

kenxu95 Jun 5, 2023

[2/n] Implement application logics to delete and handle deleted artifact contents #1399

[2/n] Implement application logics to delete and handle deleted artifact contents #1399

Conversation

likawind commented Jun 1, 2023

Describe your changes and why you are making these changes

Related issue number (if any)

Tests

Checklist before requesting a review

kenxu95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likawind commented Jun 1, 2023

likawind commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenxu95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment