Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[JENKINS-64569] Add support for EBS Throughput parameter #621

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

Panfilwk
Copy link
Contributor

@Panfilwk Panfilwk commented May 24, 2021

Closes https://issues.jenkins.io/browse/JENKINS-64569 by adding the throughput param to the end of the parser for the colon separated string. Based on other existing tests which have fewer separators than options, this should be entirely backwards compatible with existing block device config strings.

I've written a simple test, but I'm having issues getting the util tests to run both locally and in CI. It seems like maybe they aren't getting picked up by the Maven project?

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@Panfilwk Panfilwk changed the title WIP: [JENKINS-64569] Add support for EBS Throughput parameter [JENKINS-64569] Add support for EBS Throughput parameter May 25, 2021
@res0nance
Copy link
Contributor

Hey I noticed the tests were not getting run in CI and enabled them but accidentally broke this PR
I Enabled them in #625
Could you rebase it?

@res0nance res0nance added the enhancement Feature additions or enhancements label Jun 2, 2021
@Panfilwk Panfilwk force-pushed the feature/JENKINS-64569 branch from 99b3de8 to fcd0b4c Compare June 4, 2021 14:55
@Panfilwk
Copy link
Contributor Author

Panfilwk commented Jun 4, 2021

Thanks @res0nance! Just rebased and all the device mapping tests (including the new one) passed locally, so we'll see what happens in CI :)

One thing I was lightly concerned about is that the throughput param in EC2 is only supported for gp3 volume types. It seems like the prior art here with provisioned iops is to let the EC2 SDK handle the error when a volume type doesn't support a parameter, but I just wanted to make sure that's acceptable here also.

@Panfilwk
Copy link
Contributor Author

Panfilwk commented Jun 4, 2021

Is there any action left on my end for this to get merged? This feature is mostly a nice-to-have for us, so I'm not in an enormous rush, but also don't want to be blocking :)

@res0nance
Copy link
Contributor

Is there any action left on my end for this to get merged? This feature is mostly a nice-to-have for us, so I'm not in an enormous rush, but also don't want to be blocking :)

No, I was waiting for CI to finish

@res0nance res0nance merged commit 9770b6e into jenkinsci:master Jun 4, 2021
@Panfilwk
Copy link
Contributor Author

Panfilwk commented Jun 4, 2021

Awesome, thanks again for your help with this!

@julian-alarcon
Copy link

julian-alarcon commented Sep 28, 2021

There is a missing change in the documentation related with the throughput value (needed using gp3 type volumes) https://github.com/jenkinsci/ec2-plugin/blob/master/src/main/resources/hudson/plugins/ec2/SlaveTemplate/help-customDeviceMapping.html#L32

Also, an example with encrypted disabled could be useful. Currently I'm not sure if it should be nonencrypted , non-encrypted, noencrypted, etc.

@julian-alarcon
Copy link

julian-alarcon commented Sep 28, 2021

Mmm, it seems that I just need to set the encrypted section empty to disable encryption according to this code.
https://github.com/jenkinsci/ec2-plugin/blob/master/src/main/java/hudson/plugins/ec2/util/DeviceMappingParser.java#L85

So, /dev/xvda=:900::gp3:::250 should set a size of 900, gp3, and 250 of the throughput value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature additions or enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants