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.

Sign up for GitHub

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

Improve logging when operator failed before generating artifact #1252

Merged
merged 5 commits into from
Apr 28, 2023
Merged

Improve logging when operator failed before generating artifact #1252

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
076aaf8
compiles
likawind Apr 25, 2023
9ec1142
Merge branch 'main' of https://github.com/aqueducthq/aqueduct into im…
likawind Apr 27, 2023
9ad500c
improve metadata handling
likawind Apr 28, 2023
3775dde
amend
likawind Apr 28, 2023
84ad3bf
improve storage management
likawind Apr 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/golang/lib/engine/aq_engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,6 +1005,10 @@ func (eng *aqEngine) execute(
// and check operators with warning severity.
if execState.HasBlockingFailure() {
log.Infof("Stopping execution of operator %v", op.ID())
if execState.Error != nil {
log.Infof("Reason: %s", execState.Error.Message())
}

for id, dagOp := range workflowDag.Operators() {
log.Infof("Checking status of operator %v", id)
// Skip if this operator has already been completed or is in progress.
Expand Down
27 changes: 18 additions & 9 deletions src/golang/lib/workflow/artifact/artifact.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package artifact
import (
"context"
"encoding/json"
"fmt"

"github.com/aqueducthq/aqueduct/lib/database"
"github.com/aqueducthq/aqueduct/lib/models"
Expand Down Expand Up @@ -207,9 +208,7 @@ func (a *ArtifactImpl) updateArtifactResultAfterComputation(
execState *shared.ExecutionState,
) {
changes := map[string]interface{}{
models.ArtifactResultStatus: execState.Status,
models.ArtifactResultExecState: execState,
models.ArtifactResultMetadata: nil,
models.ArtifactResultMetadata: nil,
}

if a.Computed(ctx) {
Expand All @@ -222,11 +221,18 @@ func (a *ArtifactImpl) updateArtifactResultAfterComputation(
)
if err != nil {
log.Errorf("Unable to read artifact result metadata from storage and unmarshal: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, is this error ever expected in any cases? If it's a really fatal error we should probably also add a Tip to the execution state?

Copy link
Contributor

Choose a reason for hiding this comment

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

And make it a system error or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue is often triggered by the upstream op failed without creating the metadata file. I don't really want to update the exec status here since in most cases the artifact should be canceled, and caller decides. The main goal here is to prevent this from returning an err object to caller which often gives us the false signal on real failure (the op failure). So I think logging it in the context for now is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add a comment here

return
execState.Error.Context = fmt.Sprintf(
"%s\nError reading metadata: %v",
execState.Error.Context,
Copy link
Contributor

Choose a reason for hiding this comment

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

is execState.Error != nil in this case? I worry this will panic if there wasn't an issue with the computation, but something bad happened when we ReadFromStorage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's a good catch, we should check the pointer state before assigning values

err,
)
}
changes[models.ArtifactResultMetadata] = &artifactResultMetadata
}

changes[models.ArtifactResultStatus] = execState.Status
changes[models.ArtifactResultExecState] = execState

_, err := a.resultRepo.Update(
ctx,
a.resultID,
Expand Down Expand Up @@ -322,12 +328,15 @@ func (a *ArtifactImpl) GetMetadata(ctx context.Context) (*shared.ArtifactResultM
return nil, nil
}

var metadata shared.ArtifactResultMetadata
err := utils.ReadFromStorage(ctx, a.storageConfig, a.execPaths.ArtifactMetadataPath, &metadata)
if err != nil {
return nil, err
// If the path is not provided, we assume the data is not available.
if a.execPaths.ArtifactMetadataPath != "" {
var metadata shared.ArtifactResultMetadata
err := utils.ReadFromStorage(ctx, a.storageConfig, a.execPaths.ArtifactMetadataPath, &metadata)
if err != nil {
return nil, err
}
a.resultMetadata = &metadata
}
a.resultMetadata = &metadata
}

return a.resultMetadata, nil
Expand Down