fix(deadline): lock down DocDC engine to version 3.6.0 #230
Conversation
There was a problem hiding this comment.
Can we add some guidance to the relevant TSDoc entries? I'm thinking good places might be:
- the
Repositoryconstruct - the
DatabaseConnection.forDocDBmethod.
I was also hopeful we could perform a pre-flight run-time validation, but unfortunately it looks like the engine version is not available as a DatabaseCluster attribute.
| @@ -490,6 +490,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', | |||
There was a problem hiding this comment.
Two things:
- This is good for the case where
props.databaseis not supplied, but is it possible to add validation that the engine is correct for the code path where a caller specifies their own cluster and fail-fast? - Can we add a test for this new behavior?
There was a problem hiding this comment.
It's a good idea. I'll see what I can cobble together.
ddneilson
force-pushed
the
lock_docdb_version
branch
from
395e486
to
d1ae157
Compare
| if (cluster.engineVersion === '3.6.0') { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Looks good. I'm just thinking in the event that DocDB issues a patch version for 3.6. Just in case, should we relax the final version component here? So:
return cluster.engineVersion.startsWith('3.6.');It looks like RDS has patch engine versions for other DB types:
$ aws docdb describe-db-engine-versions --engine mariadb --engine-version 10.0.17
{
"DBEngineVersions": [
{
"Engine": "mariadb",
"EngineVersion": "10.0.17",
...There was a problem hiding this comment.
No harm in doing that, though the DocDB service team will never have any 3.6 value other than 3.6.0 and similarly, no 4.0 other than 4.0.0. They specifically don't do patch versions.
| @@ -288,6 +288,7 @@ export interface RepositoryProps { | |||
|
|
|||
| /** | |||
| * Specify the database where the deadline schema needs to be initialized. | |||
| * Note that Deadline supports only databases that are compatible with MongoDB 3.6. | |||
There was a problem hiding this comment.
This looks great. I had also suggested adding something to the TSDoc string of DatabaseConnection.forDocDB too. Perhaps even DocDBConnectionOptions.database. What do you think?
There was a problem hiding this comment.
Ah, right. Sure. I can add something there...
There was a problem hiding this comment.
The comment for DocDBConnectionOptions.database wasn't updated, do we need that too?
There was a problem hiding this comment.
Added those too in latest rev.
| @@ -163,6 +165,10 @@ class DocDBDatabaseConnection extends DatabaseConnection { | |||
| constructor(private readonly props: DocDBConnectionOptions) { | |||
| super(); | |||
|
|
|||
| if (!this.isCompatibleDocDBVersion()) { | |||
| Annotations.of(props.database).addError('engineVersion must be 3.6.0 to be compatible with Deadline'); | |||
There was a problem hiding this comment.
same comment as below. Should we relax this message to 3.6.x
There was a problem hiding this comment.
This still says 3.6.0 but our other messaging and the version check all use 3.6
There was a problem hiding this comment.
Ah, I discussed this with Josh offline. DocumentDB doesn't do patch-level versions. It will always be 3.6.0
jusiskin
changed the title
Fixes: #229
Testing
Did a quick deploy of the Deadline Repository & RCS. Verified it deployed successfully, and the DocDB engine version was correct. Also changed the engine version to 4.0.0 and did the same to verify that it changed there too.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license