Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add support for custom sized root block devices. #46

Merged
merged 4 commits into from
Jan 9, 2025

Conversation

ethanholz
Copy link
Contributor

This adds new support for new custom root devices when provisioning AWS instances. It adds the ability to read and modify the existing block devices for the AMI, however this requires adding the ec2:DescribeImages permission. This change will not work if you do not edit the permissions of the IAM policy to be aligned with this:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Action": [
        "ec2:RunInstances",
        "ec2:TerminateInstances",
        "ec2:DescribeInstances",
        "ec2:DescribeInstanceStatus",
        "ec2:DescribeImages"
      ],
      "Resource": "*"
    }
  ]
}

Unit testing is not written for this but a brief integration test can be found using this branch here: https://github.com/omsf-eco-infra/openmm-gpu-test/actions/runs/12698008379. I think proper testing can be added for this when we migrate to start-aws-gha-runner, since most of this testing is about argument validation.

@ethanholz ethanholz requested a review from dwhswenson January 9, 2025 22:10
@ethanholz ethanholz self-assigned this Jan 9, 2025
@ethanholz ethanholz linked an issue Jan 9, 2025 that may be closed by this pull request
Copy link
Contributor

@dwhswenson dwhswenson left a comment

Choose a reason for hiding this comment

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

In addition to the tiny documentation update, would it be possible to add a small test to cover this functionality?

action.yml Outdated
@@ -16,6 +16,9 @@ inputs:
aws_image_id:
description: "The machine AMI to use for your runner. This AMI can be a default but should have docker installed in the AMI. Will not start if not specified."
required: false
aws_root_device_size:
description: "The root device size in GB to use for your runner. Optional"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add: "If not given, defaults to ?? GB" (fill in correct value for ?? -- is that 30? can't remember)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is dependent on the AMI. If you don't provide us with one, we default to the AMI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can improve the docs to reflect that though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Probably good to point that out, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is 30 for most images, but the Deep Learning ones use 75 GB root devices.

@dwhswenson
Copy link
Contributor

Ah, missed the mention of testing in the PR message (jumped straight to the code). Just add the docs improvement and this should be good to go.

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 29.16667% with 17 lines in your changes missing coverage. Please review.

Project coverage is 90.78%. Comparing base (cc804d9) to head (cf84d64).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/gha_runner/clouddeployment.py 23.80% 16 Missing ⚠️
src/gha_runner/__main__.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
- Coverage   95.20%   90.78%   -4.43%     
==========================================
  Files           3        3              
  Lines         334      358      +24     
==========================================
+ Hits          318      325       +7     
- Misses         16       33      +17     
Flag Coverage Δ
unittests 90.78% <29.16%> (-4.43%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ethanholz ethanholz merged commit 964c18c into omsf-eco-infra:main Jan 9, 2025
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to expand root block device for AMI
2 participants