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

chore(deadline): clean up UsageBasedLicensing construct and tests #20

Merged
merged 1 commit into from
Aug 11, 2020
Merged

chore(deadline): clean up UsageBasedLicensing construct and tests #20

merged 1 commit into from
Aug 11, 2020

Conversation

jusiskin
Copy link
Contributor

@jusiskin jusiskin commented Jul 30, 2020

Problem

There were a few minor issues with the UsageBasedLicensing construct that needed improvement:

  1. The construct IDs it used internally were inaccurate/generic
  2. Code that instantiated a UsageBasedLicensing construct needed to supply at least one of the memoryReservationMib or memoryLimitMib props. The intention was that the containers would grow to use available memory on the host container instance.
  3. The log stream prefix defaulted to docker which is not intuitive when looking up the logs in CloudWatch
  4. The tests were not very granular in the behaviors they were asserting and their names and organization needed clean-up

Solution

  1. Rename the construct IDs to be meaningful and descript
  2. Remove the need to specify memory props. Internally a memoryReservationMib of 1024 MiB is used and the containers will grow to use available memory from the hoist
  3. The default log stream prefix was renamed to LicenseForwarder
  4. The tests were reorganized and broken apart to be more granular

Testing

  1. Performed a full build of the changes and confirmed that all of the tests passed
  2. Deployed and tested the UsageBasedLicensing construct end-to-end successfully
    • confirmed that the license forwarder connects and is shown in the Deadline Monitor
    • confirmed that the license forwarder listens on the correct port when a job is submitted with a corresponding UBL limit attached

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

@ddneilson ddneilson requested review from ddneilson and horsmand July 30, 2020 15:44
@jusiskin jusiskin changed the title fix(deadline): fix UsageBasedLicensing stack updates chore(deadline): clean up UsageBasedLicensing construct and tests Jul 30, 2020
@jusiskin jusiskin marked this pull request as ready for review August 11, 2020 02:32
ddneilson
ddneilson previously approved these changes Aug 11, 2020
vpc,
});

// synthStack(stack, 'default.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this left in by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was - good catch. Fixed

BREAKING CHANGE: construct IDs renamed in UsageBasedLicensing.
- Previously deployed resources will be terminated when updating
- Default log stream prefix changed from 'docker' to 'LicenseForwarder'
- Memory properties are no longer specified when constructing
  UsageBasedLicensing instances
@ddneilson ddneilson merged commit a5596d2 into aws:mainline Aug 11, 2020
evanspearman-a pushed a commit that referenced this pull request Aug 17, 2020
BREAKING CHANGE: construct IDs renamed in UsageBasedLicensing.
- Previously deployed resources will be terminated when updating
- Default log stream prefix changed from 'docker' to 'LicenseForwarder'
- Memory properties are no longer specified when constructing
  UsageBasedLicensing instances
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants