Skip to content

correct role handling and S3FS bypass#130

Merged
ErinWeisbart merged 22 commits into
masterfrom
role_fix
Jan 21, 2023
Merged

correct role handling and S3FS bypass#130
ErinWeisbart merged 22 commits into
masterfrom
role_fix

Conversation

@ErinWeisbart

@ErinWeisbart ErinWeisbart commented Jun 16, 2022

Copy link
Copy Markdown
Member

Correct credential handling to enable AWS IAM role usage.

@ErinWeisbart

ErinWeisbart commented Aug 4, 2022

Copy link
Copy Markdown
Member Author

Closes #132 by changing load_data_csv access to download.
Closes #114.

@ErinWeisbart ErinWeisbart changed the title correct role handling correct role handling and S3FS bypass Aug 5, 2022
@ErinWeisbart

ErinWeisbart commented Aug 5, 2022

Copy link
Copy Markdown
Member Author

Noting that S3FS bypass appears to be working fine and I had several tests of all the files downloading correctly and then apparently stochastically one run threw:
OSError: [Errno 30] Read-only file system: '/home/ubuntu/local_input/path/to/image/cdp2bioactives_a22_s4_w33236d730-2143-4738-bfac-685f917a717d.tif.2FE98647'

@ErinWeisbart

ErinWeisbart commented Aug 5, 2022

Copy link
Copy Markdown
Member Author

Full S3FS bypass is working at this point.
Still haven't finished necessary changes to run-worker.sh to make it mount S3 bucket using new credential handling.

@ErinWeisbart ErinWeisbart mentioned this pull request Sep 20, 2022
Closed
@bethac07 bethac07 linked an issue Sep 20, 2022 that may be closed by this pull request
DOWNLOAD_FILES doesn't fully bypass S3FS #132
Closed
@ErinWeisbart ErinWeisbart marked this pull request as ready for review January 6, 2023 23:01
@ErinWeisbart ErinWeisbart requested a review from bethac07 January 6, 2023 23:01
@ErinWeisbart

ErinWeisbart commented Jan 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

I have this branch running on a dataset now and it works (!), successfully completely bypassing S3FS and using a role.
(I don't know if it mounts S3 using a role, but I think it will be helpful to consider this a complete unit and troubleshoot S3 mounting with a role if necessary in a separate PR).

bethac07
bethac07 reviewed Jan 20, 2023
View reviewed changes
Comment thread files/exampleJob.json Outdated
bethac07
bethac07 approved these changes Jan 20, 2023
View reviewed changes

@bethac07 bethac07 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall, this seems great, and super exciting that it's working! Thank you so much

For future, tiiiiiny nitpick - would be great to not mix in a ton of stylistic changes (like "->' in a PR like this already making big behavior changes, or at least make those an explicit commit, just to a) keep the size and readability of the PR reasonable and b) assuming commits aren't squashed, by having them be an explicit commit, you see that that's why a line was changed ("oh, that was done for stylistic reasons" as opposed to "wait, what does this have to do with role handling?"). For this one, there are a lot of commits, so up to you if you'd like us to squash or not, but just something small for the future.

Congrats on this, it was a huge achievement!

@bethac07 bethac07 mentioned this pull request Jan 20, 2023
Closed
@ErinWeisbart
Update exampleJob.json
2b95a36 
fix variable names
@ErinWeisbart

ErinWeisbart commented Jan 21, 2023

Copy link
Copy Markdown
Member Author

Thanks @bethac07 !
Sorry, this PR kinda turned into a chaos of commits in all the troubleshooting. I'm not honestly sure how a lot of stylistic changes ended up being mixed in, but I will certainly try harder to keep them to their own commit in the future.

@ErinWeisbart ErinWeisbart merged commit 63a409a into master Jan 21, 2023
@ErinWeisbart ErinWeisbart deleted the role_fix branch January 21, 2023 00:59
@ErinWeisbart ErinWeisbart linked an issue Jan 23, 2023 that may be closed by this pull request
Make DCP use roles better, not just passing around credentials #114
Closed
@ErinWeisbart ErinWeisbart mentioned this pull request Jan 23, 2023
Closed
@bethac07 bethac07 mentioned this pull request Dec 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bethac07 bethac07 bethac07 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DOWNLOAD_FILES doesn't fully bypass S3FS Make DCP use roles better, not just passing around credentials

2 participants

@ErinWeisbart @bethac07