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.

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

filemanager: annotate portal run id #529

Merged
merged 14 commits into from
Sep 4, 2024
Merged

Conversation

mmalenic
Copy link
Member

@mmalenic mmalenic commented Sep 3, 2024

Closes #528

Changes

  • Add a new microservice which annotates filemanager records with the portal run id.
    • This consumes WorkflowRunStateChange events, for SUCCEEDED, FAILED, and ABORTED states, and calls the filemanager API with a PATCH request for updating the attributes using a /api/v1/s3?key=*<portal_run_id>* query.
  • This is a single Lambda function which is deployed using GoFunction, with the filemanager endpoint, and reference to the secret containing the JWT token.
  • Integration tests with the filemanager microservice are implemented using testcontainers.
    • There's a slight fix of the filemanager compose.yml as it was not possible to launch the original compose file due to the build requiring access to a postgres database. This whole setup would be simplified if filemanager didn't need access to a development database to compile code.

@mmalenic mmalenic self-assigned this Sep 3, 2024
@mmalenic mmalenic added filemanager an issue relating to the filemanager feature New feature labels Sep 3, 2024
Copy link
Member

@reisingerf reisingerf left a comment

Choose a reason for hiding this comment

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

LGTM

}

req = req.WithMethod("PATCH").WithS3Endpoint().WithQuery(url.Values{
"key": {fmt.Sprintf("*%v*", event.Detail.PortalRunId)},
Copy link
Member

Choose a reason for hiding this comment

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

Could add the / before & after the portalRunId to make sure we are hitting the intended path part and not some random filename / string in some other part of the key.
But I guess the chances of that happening are negligible, especially since we are not using the portalRunId for anything else in the key....

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

As an additional comment, at some point it might be nice for the WorkflowRunStateChange event to also contain an outputPath where objects are expected to turn up. This would allow this service to query the filemanager API with /api/v1/s3?key=<output_path>/<portal_run_id>/*, which should be faster because it will hit the index that supports prefix matching, and it would probably be even more robust.

Copy link
Member

Choose a reason for hiding this comment

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

Hm,...

To some extend that's already in place... WorkflowRunStateChange events can contain a Payload JSON which may contain that information. However, this is not enforced nor standardised. That's on purpose (at least for now) to keep the use cases flexible. We may change that in the future...

Also, I am not sure that would always work... In the normal case the portalRunId update would happen immediately after the objects have been written so we would not expect a key change. However, especially the prefix of the key can change (e.g. hot storage vs archive) and we try to focus on the key part starting from the portalRunId.
So depending on when this procedure is run, this may or may not work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay makes sense. I think we leave it for now and see if it emerges later as a possibility.

@mmalenic
Copy link
Member Author

mmalenic commented Sep 3, 2024

Recent updates

Copy link
Member

@brainstorm brainstorm left a comment

Choose a reason for hiding this comment

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

LGTM other than a bit too repetitive error control ;)

@mmalenic mmalenic merged commit 24f56f9 into main Sep 4, 2024
5 checks passed
@mmalenic mmalenic deleted the attributes/portal-run-id branch September 4, 2024 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature filemanager an issue relating to the filemanager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add microservice which updates filemanager records
4 participants