Add Amazon S3Files support for Gen3Workflow by nss10 · Pull Request #170 · uc-cdis/gen3-workflow

nss10 · 2026-07-23T13:09:38Z

Link to JIRA ticket if there is one: MIDRC-1314

New Features

Gen3Workflow now supports creation of AWS resources related to Amazon S3Files and shares a file system id as a part of /storage/setup response.

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

…w aws module

…e aws sdk calls

github-actions · 2026-07-23T13:10:33Z

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

coveralls · 2026-07-23T13:12:32Z

Coverage Report for CI Build 30515455027

Coverage increased (+1.6%) to 84.375%

Details

Coverage increased (+1.6%) from the base build.
Patch coverage: No coverable lines changed in this PR.
54 coverage regressions across 4 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

54 previously-covered lines in 4 files lost coverage.

File	Lines Losing Coverage	Coverage
routes/s3.py	29	77.34%
routes/storage.py	12	80.0%
routes/ga4gh_tes.py	9	87.5%
config.py	4	80.0%

Coverage Stats


Relevant Lines:	1152
Covered Lines:	972
Line Coverage:	84.38%
Coverage Strength:	0.84 hits per line

💛 - Coveralls

github-actions · 2026-07-23T13:34:51Z

Integration Tests on Kind

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	25	4	29
TOTAL	25	4	29

github-actions · 2026-07-23T13:53:42Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	27	1	1	29
TOTAL	27	1	1	29

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_gen3_workflow.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

github-actions · 2026-07-23T15:11:41Z

Integration Tests on Kind

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	25	4	29
TOTAL	25	4	29

github-actions · 2026-07-23T15:31:23Z

Integration Tests

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	28	1	29
TOTAL	28	1	29

Please find the detailed integration test report here

Please find the Github Action logs here

github-actions · 2026-07-23T17:19:12Z

Integration Tests on Kind

filepath	error	skipped	SUBTOTAL
tests/test_gen3_workflow.py	25	4	29
TOTAL	25	4	29

github-actions · 2026-07-23T17:46:20Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	25	3	1	29
TOTAL	25	3	1	29

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_gen3_workflow.py	3	3
TOTAL	3	3

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

…ests need fixing)

github-actions · 2026-07-27T18:10:09Z

Integration Tests

Failed to Prepare CI environment

Please find the Github Action logs here

paulineribeyre · 2026-07-27T18:29:42Z

-    s3_client.put_bucket_lifecycle_configuration(
+    clients.s3_client.put_bucket_lifecycle_configuration(
        Bucket=user_bucket_name,
        LifecycleConfiguration={


might be worth looking into NoncurrentVersionTransition/NoncurrentVersionExpiration to remove versions earlier than the default LifecycleConfiguration expiration

Thank you, it was nice to know this param exists. I've arbitrarily set it to 3 days. I don't think we really need to preserve older versions of objects any longer. It can be a simple hardcoded constant value of 3. Let me know if you think it would be better if it is a configured value? Or maybe a fraction of configured expiration days -- like (1/3 * expiration_days)

Hardcoded is fine, it's not very important. I might even have set it to 0 or 1 haha

paulineribeyre · 2026-07-27T19:07:06Z

+    # TODO: Determine if these need to be configurable.
+    eks_sg_names = [
+        f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg",
+        f"{config["EKS_CLUSTER_NAME"]}_EKS_nodepool_jupyter_sg",
+    ]


this does seem too hardcoded, specifically the reference to jupyter - where do these come from?

These SG names are derived from Terraform — see here cloud.tf#L195 and here cloud.tf#L469.
I'm still torn on whether this security group logic should live in gen3-workflow or in gen3-terraform — would appreciate your thoughts.

These security groups only need to be created once per EKS cluster, and today they're created the first time a user in that cluster triggers a create-filesystem call. That one-time, idempotent nature makes moving this to an IaC tool (gen3-terraform) feel like the right home.

That said, this logic is only relevant to gen3-workflow, and it doesn't feel right to bake application-specific logic into a generic tool.

Coming to the SG names, they depend on which SGs get assigned to nodes, that are used for the tasks based on the ENABLE_OPTIMIZED_NODE_SCHEDULING config flag.

Sorry for the maybe naive questions:

Does terraform support conditional setup? Only set this up if gen3-workflow is enabled, or if gen3-workflow.s3files is enabled?

Do we need both config["EKS_CLUSTER_NAME"]}_EKS_workers_sg and {config["EKS_CLUSTER_NAME"]}_EKS_nodepool_jupyter_sg? Why would we need to use the jupyter security group?

If we need both I think leaving it here is ok too but it'd be nice to get these values programmatically if that's possible 🤔

Not naive at all.

Does terraform support conditional setup? Only set this up if gen3-workflow is enabled, or if gen3-workflow.s3files is enabled?

I'm not very sure. I don't think terraform looks into helm values in any way while building, so I don't think it is feasible to use gen3-workflow.enabled

Do we need both config["EKS_CLUSTER_NAME"]}_EKS_workers_sg and {config["EKS_CLUSTER_NAME"]}_EKS_nodepool_jupyter_sg?

Yes, we needed both of these (for devplanetv2 -- at least).
These values were pulled using the following steps,
1. Manually get the nodes where the pods could be deployed -- we use role=workflow & role=gpu
2. Identify what security groups are assigned to these nodes karpenter-config-resources-workflow.yaml#L38-L44 & karpenter-config-resources-gpu.yaml#L29C2-L36C81
3.Manually narrow down the main security group that has all the inbound and outbound rules and add the names of those groups here.

Why would we need to use the jupyter security group?

Jupyter security group is assigned to the nodes which have role=workflow in devplanetv2 devplanetv2/cluster-values/cluster-values.yaml#L107-L114

it'd be nice to get these values programmatically if that's possible

Getting these values programmatically might be difficult. Contrarily, we could make a convention, saying nodes allocated with role=workflow to be assigned with certain security group naming format.

I think the best idea (for now!) would be to make these configurable, and add instructions in deployment documentation on how to fetch them.

I was a bit skeptical while building these, because I wasn't sure what values do these security groups get when we develop user specific nodes.

TBH, I was not ready to make these hardcoded, just wanted to get another dev's opinion on whether to keep these in gen3-workflow at all, before implementing them.

Gotcha, using those nodes is temporary anyway, since the long-term plan is to create a nodepool per user for cost tracking. At that point the nodepools will likely be created by gen3-workflow and the security groups too, so we may be able to untangle this then. In the meantime configuring it manually sounds OK... Let's document it

github-actions · 2026-07-27T20:11:32Z

Integration Tests on Kind

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	13	12	4	29
TOTAL	13	12	4	29

Test summary after rerunning failed integration tests

filepath	failed	SUBTOTAL
tests/test_gen3_workflow.py	12	12
TOTAL	12	12

github-actions · 2026-07-29T19:36:40Z

Integration Tests

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	28	1	29
TOTAL	28	1	29

Please find the detailed integration test report here

Please find the Github Action logs here

paulineribeyre · 2026-07-29T19:57:27Z

+    # TODO: Determine if these need to be configurable.
+    eks_sg_names = [
+        f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg",
+        f"{config["EKS_CLUSTER_NAME"]}_EKS_nodepool_jupyter_sg",
+    ]


Sorry for the maybe naive questions:

Does terraform support conditional setup? Only set this up if gen3-workflow is enabled, or if gen3-workflow.s3files is enabled?

Do we need both config["EKS_CLUSTER_NAME"]}_EKS_workers_sg and {config["EKS_CLUSTER_NAME"]}_EKS_nodepool_jupyter_sg? Why would we need to use the jupyter security group?

If we need both I think leaving it here is ok too but it'd be nice to get these values programmatically if that's possible 🤔

github-actions · 2026-07-29T20:37:56Z

Integration Tests

Failed to Prepare CI environment

Please find the Github Action logs here

…o feat/s3Files

github-actions · 2026-07-29T23:01:26Z

Integration Tests on Kind

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	25	4	29
TOTAL	25	4	29

github-actions · 2026-07-29T23:19:54Z

Integration Tests

Failed to Prepare CI environment

Please find the Github Action logs here

github-actions · 2026-07-30T03:23:23Z

Integration Tests

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	28	1	29
TOTAL	28	1	29

Please find the detailed integration test report here

Please find the Github Action logs here

github-actions · 2026-07-30T05:25:23Z

Integration Tests on Kind

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	25	4	29
TOTAL	25	4	29

github-actions · 2026-07-30T05:31:50Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	27	1	1	29
TOTAL	27	1	1	29

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_gen3_workflow.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

github-actions · 2026-07-30T05:56:44Z

Integration Tests on Kind

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	25	4	29
TOTAL	25	4	29

github-actions · 2026-07-30T05:58:46Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	27	1	1	29
TOTAL	27	1	1	29

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_gen3_workflow.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

paulineribeyre · 2026-07-30T16:25:22Z

+    eks_sg_names = config.get("EKS_SECURITY_GROUP_NAMES")
+    if not eks_sg_names:
+        eks_sg_names = [f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg"]


instead of setting a default here, we could add a validation step in the config validation: "if s3files is enabled, len(EKS_SECURITY_GROUP_NAMES) must be >= 1".

not a big deal. i'm just not sure the default will always make sense

I like this idea. But I would like to get it done in a different PR.

I added that to #174

github-actions · 2026-07-30T16:34:59Z

Integration Tests on Kind

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	25	4	29
TOTAL	25	4	29

github-actions · 2026-07-30T16:36:58Z

Integration Tests

filepath	passed	skipped	SUBTOTAL
tests/test_gen3_workflow.py	28	1	29
TOTAL	28	1	29

Please find the detailed integration test report here

Please find the Github Action logs here

nss10 added 15 commits July 7, 2026 13:38

WIP: Enable bucket versioning

e343196

[UNTESTED]Add initial s3files setup + separate out s3 files into a ne…

efdeabb

…w aws module

Update EKS security group filter to use EKS group name

41926ab

Update docstring

6e9abd5

Add a TODO comment regarding security group creation, maybe can reduc…

6e368cd

…e aws sdk calls

Fix minor bugs and add more TODOs.

f4f2b75

Add file system status implementation and waiter

0248c3d

Update import path

e172079

Move boto3 clients to a different file to avoid circular imports

73ad87c

Update exception type

1ac0aa4

Fix get_s3_files_system and add/update TODO comments

f363791

Create dynamic IAM role for S3Files

15182e5

Add STORAGE_TYPE config variable, add documentation for s3files

fbc7545

Make S3Files a configurable setup

80cd425

Merge branch 'master' into feat/s3Files

0eaf83e

Make mount target provisioning a bgTask + Update docs

2cf8c10

nss10 added 2 commits July 23, 2026 12:05

Update comments

fc32d15

Add missing function call.

bb84de3

nss10 added 4 commits July 24, 2026 09:56

Add more comments

df89174

Re-organize aws_utils into aws.bucket, aws.aws_utils, aws.clients. (T…

00f569f

…ests need fixing)

Fix patching in unit tests. All current tests pass

3f56313

Add more TODOs

28614f8

paulineribeyre reviewed Jul 27, 2026

View reviewed changes

paulineribeyre reviewed Jul 28, 2026

View reviewed changes

Comment thread tests/test_s3_files.py Outdated

Add fixes related to review comments

3dd0630

paulineribeyre reviewed Jul 29, 2026

View reviewed changes

Fix lint errors in unit tests

db7fd5e

nss10 added 3 commits July 29, 2026 17:37

Fix more PR suggestions and add more unit tests

44e8406

Merge branch 'master' of https://github.com/uc-cdis/gen3-workflow int…

039af05

…o feat/s3Files

Fix lint issues

fed3b31

nss10 marked this pull request as ready for review July 30, 2026 03:02

nss10 added 2 commits July 29, 2026 23:59

Make security group field configurable and add docs on how to fetch them

c6d7202

Update the link in config.py

1e45803

remove TODO comment

6f9c69e

Remove unused imports

bc4e9b8

paulineribeyre approved these changes Jul 30, 2026

View reviewed changes

nss10 changed the title ~~Feat/s3Files~~ Add Amazon S3Files support for Gen3Workflow Jul 30, 2026

nss10 merged commit d0eb9db into master Jul 30, 2026
12 of 13 checks passed

paulineribeyre deleted the feat/s3Files branch July 30, 2026 18:05

Uh oh!

Conversation

nss10 commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

coveralls commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 30515455027

Coverage increased (+1.6%) to 84.375%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

github-actions Bot commented Jul 23, 2026

Integration Tests on Kind

Uh oh!

github-actions Bot commented Jul 23, 2026

Integration Tests

Uh oh!

github-actions Bot commented Jul 23, 2026

Integration Tests on Kind

Uh oh!

github-actions Bot commented Jul 23, 2026

Integration Tests

Uh oh!

github-actions Bot commented Jul 23, 2026

Integration Tests on Kind

Uh oh!

github-actions Bot commented Jul 23, 2026

Integration Tests

Uh oh!

github-actions Bot commented Jul 27, 2026

Integration Tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulineribeyre Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 27, 2026

Integration Tests on Kind

Uh oh!

Uh oh!

github-actions Bot commented Jul 29, 2026

Integration Tests

Uh oh!

Uh oh!

nss10 commented Jul 23, 2026 •

edited

Loading

coveralls commented Jul 23, 2026 •

edited

Loading

paulineribeyre Jul 29, 2026 •

edited

Loading