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

Add deckURI to NodeExecutionData #413

Merged
merged 18 commits into from
Jun 7, 2022
Merged

Add deckURI to NodeExecutionData #413

merged 18 commits into from
Jun 7, 2022

Conversation

pingsutw
Copy link
Member

Signed-off-by: Kevin Su [email protected]

TL;DR

Add deckURI to NodeExecutionData, so that Flyteconsole can retrieve the pre-signed deck URI and read the file.

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#2175

Follow-up issue

NA

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #413 (c4c95d2) into master (77b83f6) will increase coverage by 0.03%.
The diff coverage is 67.74%.

@@            Coverage Diff             @@
##           master     #413      +/-   ##
==========================================
+ Coverage   61.33%   61.37%   +0.03%     
==========================================
  Files         156      156              
  Lines       11095    11142      +47     
==========================================
+ Hits         6805     6838      +33     
- Misses       3586     3595       +9     
- Partials      704      709       +5     
Flag Coverage Δ
unittests 60.30% <66.66%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/config/config.go 20.00% <ø> (ø)
pkg/manager/impl/util/data.go 73.07% <ø> (ø)
dataproxy/service.go 47.61% <65.51%> (+9.43%) ⬆️
pkg/config/serverconfig_flags.go 58.69% <100.00%> (+0.91%) ⬆️
pkg/repositories/transformers/node_execution.go 72.39% <100.00%> (+0.14%) ⬆️
...cloudevent/implementations/cloudevent_publisher.go 81.81% <0.00%> (-4.63%) ⬇️
pkg/manager/impl/execution_manager.go 74.64% <0.00%> (ø)
pkg/runtime/application_config_provider.go 33.33% <0.00%> (ø)
scheduler/core/gocron_scheduler.go 83.07% <0.00%> (+0.08%) ⬆️
pkg/manager/impl/node_execution_manager.go 72.54% <0.00%> (+0.31%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77b83f6...c4c95d2. Read the comment docs.

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
go.mod Outdated Show resolved Hide resolved
@pingsutw pingsutw changed the title Add deckURI to NodeExecutionData [WIP] Add deckURI to NodeExecutionData May 23, 2022
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw changed the title [WIP] Add deckURI to NodeExecutionData Add deckURI to NodeExecutionData May 24, 2022
pingsutw added 3 commits May 24, 2022 23:33
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw requested review from katrogan and EngHabu June 1, 2022 19:46
dataproxy/service.go Outdated Show resolved Hide resolved
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Copy link
Member Author

pingsutw commented Jun 2, 2022

cc @EngHabu @katrogan one question, I'm able to get a signed URL when using flytekit, but it doesn't work when I use curl. Anything I missed in this PR?

(base) ➜  ~ curl http://34.201.131.50:30081//api/v1/dataproxy/artifact_urn              
Not Found
(base) ➜  ~ curl http://34.201.131.50:30081//api/v1/dataproxy/artifact_urn\?native_url\=deck.html
Not Found

@katrogan
Copy link
Contributor

katrogan commented Jun 2, 2022

@pingsutw why are there two slashes before api in the path?

@pingsutw
Copy link
Member Author

pingsutw commented Jun 2, 2022

same error if I use http://34.201.131.50:30081/api/v1/dataproxy/artifact_urn

@EngHabu
Copy link
Contributor

EngHabu commented Jun 2, 2022

Try this:

curl http://34.201.131.50:30081//api/v1/dataproxy/artifact_urn?native_url=s3://bucket/deck.html

Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Copy link
Member Author

pingsutw commented Jun 6, 2022

Fixed it, we should register the data proxy handler in the HTTP server. https://github.com/flyteorg/flyteadmin/pull/413/files#diff-4ce88803abf59984cddcf6ab336c97b3231b0cb87f90e07f2f42e622e330f325R197-R201

dataproxy/service.go Outdated Show resolved Hide resolved
dataproxy/service.go Show resolved Hide resolved

return &service.CreateDownloadLocationResponse{
SignedUrl: resp.URL.String(),
ExpiresAt: timestamppb.New(time.Now().Add(req.ExpiresIn.AsDuration())),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this valid that we can accurately say here . The library sets this time for signed url if its google
https://github.com/flyteorg/stow/blob/9c3f5f9ea24966206f0b2edc608da6ca803420e3/google/container.go#L55

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not accurate, but I think it's okay for now because we can't get the expiry time from SignedURLResponse
I can add it in a separate PR.

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw merged commit 26e5219 into master Jun 7, 2022
@pingsutw pingsutw deleted the deck branch June 7, 2022 16:11
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Add deckURI to NodeExecutionData

Signed-off-by: Kevin Su <[email protected]>

* Use new storage api in stdlib

Signed-off-by: Kevin Su <[email protected]>

* Fixed tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* one more test

Signed-off-by: Kevin Su <[email protected]>

* Add deck_uri in NodeExecutionClosure

Signed-off-by: Kevin Su <[email protected]>

* Fixed tests

Signed-off-by: Kevin Su <[email protected]>

* update tests

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* lint fix

Signed-off-by: Kevin Su <[email protected]>

* Updated idl

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* Add http endpoint

Signed-off-by: Kevin Su <[email protected]>

* few updates

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[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