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

Upgrade Deadline Repo and DocDB; Fix Integration Tests #1221

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Upgrade Deadline Repo and DocDB; Fix Integration Tests #1221

merged 1 commit into from
Aug 8, 2024

Conversation

loachri
Copy link
Contributor

@loachri loachri commented Aug 2, 2024 •
edited
Loading

We have broken Integration tests for RFDK. The primary issue was due expired rds-ca-2019 CA certs. The fix was to upgrade the tests to use newer versions of MongoDB and DocDB, which use the newer rds-ca-rsa2048-g1 CA certs, as well as fix any downsteam issue that came as a result of upgrading MongoDB and DocDB

Deadline Repository was upgraded to 10.3.2.1, and DocDB has been upgraded to 5.0.0.

I've also increased timeouts in the repo installer as well as in the jest settings, as a considerable amount of test flakeyness was a result of these timeouts.

Finally, I've added more verbose debugging to the DeadlineRepository installer, so that it's more clear what's going on if it fails to install

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

@loachri loachri changed the title Fix Integration Tests [WIP] Fix Integration Tests Aug 2, 2024
@loachri loachri force-pushed the loachri/fix-polished branch from 39e1680 to e9390b1 Compare August 2, 2024 19:41
@loachri loachri changed the title [WIP] Fix Integration Tests Fix Integration Tests Aug 2, 2024
rondeau-aws
rondeau-aws reviewed Aug 7, 2024
View reviewed changes
...components/deadline/deadline_01_repository/scripts/bastion/testing/DL-read-docdb-response.sh
@@ -39,6 +39,5 @@ else
fi

# Mongo command to query for "deadline10db" database
mongo --quiet --ssl --host="$DB_ADDRESS" --sslCAFile="$CERT_CA" --username="$DB_USERNAME" --password="$DB_PASS" --eval='printjson( db.adminCommand( { listDatabases: 1, nameOnly: true, filter: { "name": "deadline10db" } } ) )'

mongo --quiet --ssl --host="$DB_ADDRESS" --sslCAFile="$CERT_CA" --username="$DB_USERNAME" --password="$DB_PASS" --eval='(function(){var output=db.adminCommand({ listDatabases: 1, nameOnly: true, filter: { "name": "deadline10db" } } );delete output.onTime;delete output.operationTime;printjson(output)})()'
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is getting rather long and difficult to read. Would it be possible to split it up? I think our style guide for this project recommends a line length of 120 characters.

Also, should we document why we added these delete commands?

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'm okay with just leaving this given that it's a shell script, and I don't want to jeopardize some sort of weirdness by splitting up the bash command into different lines.

I can add a comment here

packages/aws-rfdk/lib/deadline/scripts/bash/installDeadlineRepository.sh
@@ -183,7 +183,7 @@ fi
# sets the "u" shell option above. This is a use of the ${parameter+word} shell expansion. If the value of "parameter" is unset, nothing will be
# substituted in its place. If "parameter" is set, then the value of "word" is used, which is the expansion of the populated array.
# Since bash treats the expansion of an empty array as an unset variable, we can use this pattern expand the array only if it is populated.
$INSTALL_AS_NON_ROOT_CMD $REPO_INSTALLER --mode unattended --setpermissions false --prefix "$PREFIX" --installmongodb false --backuprepo false ${REPO_ARGS[@]+"${REPO_ARGS[@]}"}
$INSTALL_AS_NON_ROOT_CMD $REPO_INSTALLER --mode unattended --setpermissions false --prefix "$PREFIX" --installmongodb false --backuprepo false --debuglevel 4 ${REPO_ARGS[@]+"${REPO_ARGS[@]}"}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably fine, but I just want to make sure: did you intend to include the --debuglevel 4 here?

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, the installbuilder will not give any indication of failure otherwise

packages/aws-rfdk/lib/deadline/lib/repository.ts
@@ -677,7 +678,7 @@ export class Repository extends Construct implements IRepository {
*/
const parameterGroup = databaseAuditLogging ? new ClusterParameterGroup(this, 'ParameterGroup', {
description: 'DocDB cluster parameter group with enabled audit logs',
family: 'docdb3.6',
family: 'docdb5.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably call out the update to DocDB 5.0 in the release notes?

As far as I know, the release notes are automatically generated based on commit messages. Unfortunately I don't know if that can handle multiple notes in a single commit, or if we'd need to split up the commits.

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 see if I can split this up into multiple commits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split up the commit into multiple commits

integ/components/deadline/deadline_01_repository/lib/repository-testing-tier.ts
@@ -115,7 +115,7 @@ export class RepositoryTestingTier extends TestingTier {
'cd ~ec2-user',
'mkdir -p testScripts',
'cd testScripts',
'wget https://s3.amazonaws.com/rds-downloads/rds-combined-ca-bundle.pem',
'wget https://truststore.pki.rds.amazonaws.com/global/global-bundle.pem',
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably call this out in the release notes / CR description too?

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'll add this in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added mention of this to the commit notes. There's some mention of this in the CR description, since this is the code that uses the new CA cert rather than the expired 2019 one

packages/aws-rfdk/lib/deadline/lib/repository.ts
@@ -686,7 +687,7 @@ export class Repository extends Construct implements IRepository {
const instances = props.documentDbInstanceCount ?? Repository.DEFAULT_NUM_DOCDB_INSTANCES;
const dbCluster = new DatabaseCluster(this, 'DocumentDatabase', {
masterUser: {username: 'DocDBUser'},
engineVersion: '3.6.0',
engineVersion: '5.0.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we're still using DocumentDB 3.6 in a few places such as examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/storage-tier.ts. Do we need to update those too?

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'll update this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'll just update the example, since I don't want to update the integ structs without doing a retest, and I don't think we have bandwidth for a retest

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 this

@loachri loachri force-pushed the loachri/fix-polished branch from e9390b1 to 2a0b982 Compare August 8, 2024 16:57
@loachri loachri changed the title Fix Integration Tests Upgrade Deadline Repo and DocDB; Fix Integration Tests Aug 8, 2024
@loachri loachri force-pushed the loachri/fix-polished branch from 2a0b982 to 5db6a52 Compare August 8, 2024 17:01
marofke
marofke previously approved these changes Aug 8, 2024
View reviewed changes
@loachri loachri force-pushed the loachri/fix-polished branch from aaa1331 to abe3798 Compare August 8, 2024 19:19
@loachri
feat(deadline)!: upgrade deadline repo to 10.3.2.1 and DocDB to 5.0.0
7e844d5 
addtionally, use the newer rds-ca-rsa2048-g1 CA certs
as the original rds-ca-2019 CA certs have expired.
@loachri loachri force-pushed the loachri/fix-polished branch from abe3798 to 7e844d5 Compare August 8, 2024 19:40
@rondeau-aws rondeau-aws merged commit f9b989a into mainline Aug 8, 2024
3 checks passed
@rondeau-aws rondeau-aws deleted the loachri/fix-polished branch August 8, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marofke marofke marofke approved these changes

@rondeau-aws rondeau-aws rondeau-aws approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@loachri @marofke @rondeau-aws