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

feat(core): make cloudwatch agent installation optional #338

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

horsmand
Copy link
Contributor

@horsmand horsmand commented Mar 9, 2021

Fixes #308

Testing was a bit tricky to go through all the different code paths. I extracted the powershell and bash scripts and modified them slightly to work on their own, then manually spun up Windows and Linux hosts through the EC2 console using the scripts as user data. Doing this I was able to make modifications like setting the inputs to different values to using bad S3 locations I knew it wouldn't be able to download from, to see how it worked through different code paths. After I could use the scripts successfully like that, I then deployed the Basic example app, modified to have both a Linux and Windows worker fleet, to make sure that the unmodified scripts worked.


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

Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

Just some small comments.

Can you elaborate on how this was tested?

THIRD-PARTY Outdated Show resolved Hide resolved
THIRD-PARTY Show resolved Hide resolved
THIRD-PARTY Outdated

------

** openssl 1.0.2k-fips; version 1.0.2k-fips -- https://www.openssl.org/
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here for version number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

THIRD-PARTY Show resolved Hide resolved
host: IScriptHost,
shouldInstallAgent: boolean,
skipValidation: boolean,
) {
// Grant access to the required CloudWatch Agent installer files
const cloudWatchAgentBucket = Bucket.fromBucketArn(this, 'CloudWatchAgentBucket', 'arn:aws:s3:::amazoncloudwatch-agent');
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that we missed updating this. The bucket ARN changes based on the region now, no?

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 good catch, this was definitely a miss in my testing. When running the script through the RFDK example app, I was using the Thinkbox Deadline Linux and Windows base AMI's, which both have the CloudWatch agent pre-installed.

Since I don't have an AMI with Deadline installed but the CloudWatch agent not installed, I commented out the check for an existing agent in both configuration scripts and then turned off the worker fleet's health check so I could deploy the fleets and confirm the failure by logging into the hosts through the Systems Manager and checking the logs. After that, I updated this code to grant permissions for the regional bucket and redeployed to confirm that it was working.

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 one small comment, otherwise this looks good to me!

ddneilson
ddneilson previously approved these changes Mar 10, 2021
@horsmand horsmand merged commit ac052ea into aws:mainline Mar 10, 2021
@horsmand horsmand deleted the cwa_update branch March 10, 2021 22:06
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.

Modify behavior of CloudWatchAgent agent installation
3 participants