feat: Implement EFS IOPs specification in Project YAML by samwaln · Pull Request #313 · aws/amazon-genomics-cli

samwaln · 2022-02-09T01:33:39Z

Description of Changes

We want to be able to specify the filesystem used and the throughput. This allows customers with large numbers of large files to accelerate workflows. Filesystem options are S3 and EFS, with a default of S3 for Cromwell and Nextflow and a default of EFS for MiniWDL. Valid throughput values are 1 to 1024.

Description of how you validated changes

I have added a test that checks that the default throughput is set. I have run ./scripts/run-dev.sh and agc project validate with a valid agc-project.yaml and an invalid version containing an invalid filesystem.

Checklist

If this change would make any existing documentation invalid, I have included those updates within this PR
I have added unit tests that prove my fix is effective or that my feature works
I have linted my code before raising the PR
Title of this Pull Request follows Conventional Commits standard: https://www.conventionalcommits.org/en/v1.0.0/

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

markjschreiber · 2022-02-10T01:05:25Z

+								Type:   "wdl",
+								Engine: "miniwdl",
+								Filesystem: Filesystem{
+									FSType:        "S3",


Should this be EFS?

markjschreiber · 2022-02-10T01:05:51Z

            - type: wdl
              engine: miniwdl
+              filesystem:
+                fsType: S3


markjschreiber · 2022-02-10T01:07:10Z

+            - type: wdl
+              engine: cromwell
+              filesystem:
+                fsType: EFS


Cromwell can't use EFS. This test should probably fail.

markjschreiber · 2022-02-10T01:08:11Z

+									Type:   "wdl",
+									Engine: "miniwdl",
+									Filesystem: Filesystem{
+										FSType:        "S3",


miniwdl uses EFS

markjschreiber · 2022-02-10T01:09:57Z

+									Engine: "nextflow",
+									Filesystem: Filesystem{
+										FSType:        "S3",
+										Configuration: FSConfig{FSProvisionedThroughput: 0},


S3 doesn't have a provisioned throughput. We should prevent this configuration from being an option with S3

markjschreiber · 2022-02-10T01:13:07Z

+									Type:   "wdl",
+									Engine: "miniwdl",
+									Filesystem: Filesystem{
+										FSType:        "S3",


markjschreiber · 2022-02-10T01:13:22Z

+						Type:   "wdl",
+						Engine: "miniwdl",
+						Filesystem: Filesystem{
+							FSType:        "S3",


markjschreiber · 2022-02-10T01:14:29Z

   */
  public readonly engineName: string;
+  /**
+   * Name of the filesystem to use.


Name of the filesystem type to use (e.g. EFS, S3)

markjschreiber · 2022-02-10T01:33:09Z

Can you check and see if it's possible to enforce values for engine and filesystem in the packages/cli/internal/pkg/cli/spec/project_schema.json. For example, currently cromwell and nextflow must use s3 and miniwdl must use EFS.

You will also need to add logic so that the fileystem information is passed through to the cdk that generates the filesystem so that the resulting EFS volume has the desired provisioned IOPs.

markjschreiber · 2022-02-15T20:33:55Z

+    });
+  }
+
+  protected createAccessPointcreateFileSystem(vpc: IVpc): FileSystem {


Should this just be createFileSystem It doesn't create an access point unless you meant to call that method internally?

markjschreiber · 2022-02-15T20:36:43Z

+   */
+  public readonly filesystemType?: string;
+  /**
+   * Name of the filesystem type to use (e.g. EFS, S3).


This code comment is wrong, should be something like "Amount of provisioned IOPs"

markjschreiber · 2022-02-15T20:48:19Z

+  /**
+   * Filesystem provisioned throughput to use for EFS.
+   */
+  readonly iops: Size;


I think this should be an optional parameter. When the filesystem is S3 you can't supply iops and when the filesystem is EFS then iops is an option.

markjschreiber · 2022-02-15T20:52:07Z

Looking good. General comments:

Does the code handle the case where EFS is specified as the filesystem but provisioned IOPs is not (meaning you get the default MaxIO with burst mode). Provisioned IOPs is optional.
There seems to be a lot of content from the snakemake implementation. Possibly a rebase on top of HEAD would generate a cleaner commit?

tneely · 2022-02-22T21:53:30Z


    switch (engineName) {
      case ENGINE_CROMWELL:
+        if (filesystemType != "S3") {


I'm a bit confused here. What's the point of exposing filesystemType if we error on any non-default values?

I think the plan here is to have other filesystem types in the future.

samwaln temporarily deployed to slack February 9, 2022 01:33 Inactive

markjschreiber reviewed Feb 10, 2022

View reviewed changes

Comment thread packages/cdk/lib/env/context-app-parameters.ts

tneely suggested changes Feb 10, 2022

View reviewed changes

Comment thread packages/cli/internal/pkg/cli/spec/context.go Outdated

Comment thread packages/cli/internal/pkg/cli/spec/context.go Outdated

Comment thread packages/cli/internal/pkg/cli/spec/project_test.go Outdated

Comment thread packages/cli/internal/pkg/cli/spec/project_test.go

markjschreiber reviewed Feb 15, 2022

View reviewed changes

samwaln force-pushed the efs-options branch from b023437 to 96d2ee5 Compare February 16, 2022 01:09

samwaln changed the base branch from main to release February 16, 2022 01:18

samwaln changed the base branch from release to main February 16, 2022 01:18

Samantha Waln added 3 commits February 15, 2022 17:23

feat: Implement EFS IOPs specification in Project YAML

fc05213

Address comments on filesystem specification

c398778

Change default throughput and minor changes to address comments

5531a80

samwaln force-pushed the efs-options branch from 96d2ee5 to 5531a80 Compare February 16, 2022 02:07

markjschreiber approved these changes Feb 18, 2022

View reviewed changes

tneely reviewed Feb 22, 2022

View reviewed changes

tneely approved these changes Feb 23, 2022

View reviewed changes

samwaln merged commit c1af22b into main Feb 23, 2022

samwaln deleted the efs-options branch February 23, 2022 20:16

Conversation

samwaln commented Feb 9, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markjschreiber commented Feb 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markjschreiber Feb 15, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

markjschreiber commented Feb 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

markjschreiber Feb 15, 2022 •

edited

Loading