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.

Sign up for GitHub

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

Jump to bottom

feat(deadline): create Deadline Groups and Pools on deploy for ConfigureSpotEventPlugin #470

Merged
merged 3 commits into from
Jul 8, 2021
Merged

feat(deadline): create Deadline Groups and Pools on deploy for ConfigureSpotEventPlugin #470

merged 3 commits into from
Jul 8, 2021

Conversation

kozlove-aws
Copy link
Contributor

Problem

After creating and configuring a spot fleet via RFDK constructs SpotEventPluginFleet & ConfigureSpotEventPlugin, there is a requirement for manually creating deadline groups and pools before the spot fleet request can be created.

Solution

  • Add requests to create Groups/Pools during SEP configuration

Testing

  • Deployed fleet with two new groups and two new pools
  • Validated that groups and pools were created
  • Deployed updated configuration with two fleets where groups are different but with same one pool
  • Validated that new groups and pool created.

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

@jusiskin jusiskin added the contribution/core This is a PR that came from AWS. label Jun 15, 2021
@jusiskin jusiskin linked an issue Jun 16, 2021 that may be closed by this pull request
Create deadline group/pool implicitly while configuring SEP #358
Closed
2 tasks
jericht
jericht requested changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@jericht jericht left a comment

Choose a reason for hiding this comment

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

Overall nice work! Just added some suggestions to reorganize the code a bit.

packages/aws-rfdk/lib/deadline/lib/configure-spot-event-plugin.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/deadline/lib/spot-event-plugin-fleet.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/lambdas/nodejs/configure-spot-event-plugin/handler.ts Outdated Show resolved Hide resolved
...ages/aws-rfdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/spot-event-plugin-client.ts Outdated Show resolved Hide resolved
...ages/aws-rfdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/spot-event-plugin-client.ts Outdated Show resolved Hide resolved
...fdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/test/spot-event-plugin-client.test.ts Outdated Show resolved Hide resolved
...fdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/test/spot-event-plugin-client.test.ts Outdated Show resolved Hide resolved
...fdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/test/spot-event-plugin-client.test.ts Outdated Show resolved Hide resolved
...fdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/test/spot-event-plugin-client.test.ts Outdated Show resolved Hide resolved
...fdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/test/spot-event-plugin-client.test.ts Outdated Show resolved Hide resolved
@kozlove-aws kozlove-aws force-pushed the create_groups_pools branch 2 times, most recently from a2a92f7 to 9983426 Compare June 24, 2021 16:20
jericht
jericht requested changes Jun 24, 2021
View reviewed changes
Copy link
Contributor

@jericht jericht left a comment

Choose a reason for hiding this comment

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

Just a couple of tiny nitpicks, otherwise looks good

packages/aws-rfdk/lib/lambdas/nodejs/configure-spot-event-plugin/test/handler.test.ts Outdated Show resolved Hide resolved
...ages/aws-rfdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/spot-event-plugin-client.ts Outdated Show resolved Hide resolved
...ages/aws-rfdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/spot-event-plugin-client.ts Outdated Show resolved Hide resolved
...fdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/test/spot-event-plugin-client.test.ts Outdated Show resolved Hide resolved
jericht
jericht previously approved these changes Jun 24, 2021
View reviewed changes
jusiskin
jusiskin requested changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Great work. I think it might help to make the new SpotEventPluginClient APIs a little more intuitive.

packages/aws-rfdk/lib/lambdas/nodejs/configure-spot-event-plugin/handler.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/lambdas/nodejs/configure-spot-event-plugin/test/handler.test.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/lambdas/nodejs/configure-spot-event-plugin/types.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/lambdas/nodejs/configure-spot-event-plugin/types.ts Outdated Show resolved Hide resolved
...ages/aws-rfdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/spot-event-plugin-client.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/lambdas/nodejs/configure-spot-event-plugin/test/handler.test.ts Outdated Show resolved Hide resolved
...fdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/test/spot-event-plugin-client.test.ts Show resolved Hide resolved
@jusiskin jusiskin changed the title fix(core): Add Groups and Pools for SpotEventPluginFleet feat(deadline): create Deadline Groups and Pools on deploy for ConfigureSpotEventPlugin Jun 30, 2021
jusiskin
jusiskin requested changes Jun 30, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

The API looks much better now. The only thing missing is more test coverage to ensure the new APIs behave as expected.

packages/aws-rfdk/lib/lambdas/nodejs/configure-spot-event-plugin/test/handler.test.ts Outdated Show resolved Hide resolved
...fdk/lib/lambdas/nodejs/lib/configure-spot-event-plugin/test/spot-event-plugin-client.test.ts Outdated Show resolved Hide resolved
jusiskin
jusiskin requested changes Jul 6, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Functionally and code quality-wise this looks ready to me. I just remembered that we will need to update some related places in our documentation:

  1. aws-rfdk/packages/aws-rfdk/lib/deadline/lib/configure-spot-event-plugin.ts

    Lines 320 to 321 in 11e30f6

    * - Create the Deadline Group associated with the Spot Fleet Request Configuration. See [Deadline Documentation](https://docs.thinkboxsoftware.com/products/deadline/10.1/1_User%20Manual/manual/pools-and-groups.html).
    * - Create the Deadline Pools to which the fleet Workers are added. See [Deadline Documentation](https://docs.thinkboxsoftware.com/products/deadline/10.1/1_User%20Manual/manual/pools-and-groups.html).
  2. https://github.com/aws/aws-rfdk/blob/mainline/packages/aws-rfdk/lib/deadline/README.md#configure-spot-event-plugin

jusiskin
jusiskin requested changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Found a few more places that mention creating the groups/pools manually:

jusiskin
jusiskin previously approved these changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Thanks for the great feature enhancement, @kozlove-aws. This looks ready to me!

jericht
jericht approved these changes Jul 8, 2021
View reviewed changes
Copy link
Contributor

@jericht jericht left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@jusiskin jusiskin merged commit b35ed6d into aws:mainline Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jericht jericht jericht approved these changes

@jusiskin jusiskin jusiskin approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create deadline group/pool implicitly while configuring SEP
3 participants
@kozlove-aws @jusiskin @jericht