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

Check for ExternalDBSettings while starting a load test #885

Merged
merged 3 commits into from
Feb 7, 2025
Merged

Conversation

agnivade
Copy link
Member

@agnivade agnivade commented Feb 4, 2025

In case of ExternalDB there will be no cluster identifier
from a Terraform deployment.

In case of ExternalDB there will be no cluster identifier
from a Terraform deployment.
@agnivade agnivade added the 2: Dev Review Requires review by a core committer label Feb 4, 2025
Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

What if the external DB is not an RDS cluster?

@agnivade
Copy link
Member Author

agnivade commented Feb 5, 2025

LMK how it looks now. I am distinguishing the presence of a ClusterIdentifier with an AWS instance, and the presence of a DataSource with whether External DB is being used or not.

Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

This looks better! I'm slightly concerned it's getting too complex, but that's a problem of the flexible configuration we have, of course, not of this PR. Thanks for this!

deployment/terraform/db_operations.go Outdated Show resolved Hide resolved
Comment on lines +99 to +102
// If an external non-AWS DB is used.
if t.config.ExternalDBSettings.DataSource != "" && t.config.ExternalDBSettings.ClusterIdentifier == "" {
return "available", nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but: is there a way we can test the connection to the datasource here? It would need to be from one of the app nodes, but maybe it's worth doing it to make sure all is working as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

At which phase do you want to test it? This DBStatus method is purely geared towards AWS instances so that we can turn them off to save money. I think we could test this in the deployment phase, but it should automatically fail if the servers won't start.

Co-authored-by: Alejandro García Montoro <alejandro.garciamontoro@gmail.com>
@agnivade agnivade added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Feb 7, 2025
@agnivade agnivade merged commit b74021f into master Feb 7, 2025
1 check passed
@agnivade agnivade deleted the ltstart branch February 7, 2025 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants