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

Launch AWS ElasticSearch Service per tenant #2

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

tieujason330
Copy link

Type: Number
Default: '2'
MinValue: '1'
MaxValue: '20'

Choose a reason for hiding this comment

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

@tieujason330 I assume this is auto-scaled based on demand?

Copy link
Author

Choose a reason for hiding this comment

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

no this is a fixed amount of nodes being used, there's no auto-scaling option for aws elasticsearch. or we'd need to handle it ourselves

@madanadit
Copy link

madanadit commented Dec 8, 2021

Please monitor the service cost in our AWS bill over the next couple of weeks to get a sense of operation costs. Thank you.

@madanadit madanadit requested a review from Xenorith December 8, 2021 06:04
@Xenorith
Copy link

Xenorith commented Dec 8, 2021

Please monitor the service cost in our AWS bill over the next couple of weeks to get a sense of operation costs. Thank you.

jason's account accumulated a nice steady cost in the last week: https://docs.google.com/spreadsheets/d/18tj-covBTYCc7R2v6fS0M9JIaWwtnqP622iEL_O64Xw/edit#gid=1188663573

@madanadit
Copy link

Okay, not too bad. $36 per tenant for OpenSearch per day. We'll optimize when we have too.

@@ -692,8 +815,24 @@ Resources:
Value: !GetAtt rds.Outputs.RdsEndpoint
- Name: DB_NAME
Value: !Ref RDSDatabase
- Name: ES_CLUSTER_ENDPOINT
Copy link

Choose a reason for hiding this comment

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

is it also not possible to add these new env vars directly under Environment as opposed to in both the if and else sections of the if statement on the ProvisionRDS condition?

also wouldn't it be more accurate to add these under their own "if ProvisionES" block and then join them with the others? or is that just really clunky to define in CFT

Copy link
Author

Choose a reason for hiding this comment

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

i wasn't able to add multiple if conditions here, keep getting syntax errors

@@ -215,6 +331,8 @@ Conditions:
HasDomainName: !Not [!Equals [!Ref DomainName, '']]
ProvisionEFS: !Equals [!Ref UseEFS, 'true']
ProvisionRDS: !Equals [!Ref UseRDS, 'true']
ProvisionES: !Equals [!Ref UseES, 'true']
ESEnforceHTTPS: !Equals [ !Ref "EnforceHTTPS", "true" ]
Copy link

Choose a reason for hiding this comment

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

we don't use ESEnforceHTTPS anywhere?

Copy link
Author

Choose a reason for hiding this comment

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

removed

Condition: ProvisionES
Properties:
TemplateURL: !Sub https://${SaaSBoostBucket}.s3.amazonaws.com/tenant-onboarding-es.yaml
TimeoutInMinutes: 45
Copy link

Choose a reason for hiding this comment

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

45 minutes for timeout?! is this maybe accounting for the fact that we could relaunch a tenant with existing data that would take a long time to load? i noticed in the referenced PR that this was 30, which is still a fairly large value

Copy link
Author

Choose a reason for hiding this comment

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

elasticsearch takes a while to launch, usually around 15+ minutes, i think mainly because the domain endpoint gets created and it waits til the endpoint is accessible. i can set it back to 30, i think i set to 45 when i initially tested

Copy link

@Xenorith Xenorith left a comment

Choose a reason for hiding this comment

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

hopefully wont be too yucky to merge the main fork into this when the feature is merged

@tieujason330
Copy link
Author

unfortunately based on that PR, it sounds like the PR creator is just going to leave it open for the community to use since there's too much work to be done to make it production-ready/merged into main. i'll happily redo my changes if it ever goes into main though since that'll probably mean it'll include much more customization options in the admin ui.

@tieujason330 tieujason330 merged commit a6a5dc3 into TachyonNexus:main Dec 9, 2021
@tieujason330 tieujason330 deleted the add-elasticsearch branch December 9, 2021 00:21
tieujason330 added a commit that referenced this pull request Jan 20, 2022
* elasticsearch cft

* set 30 timeout

remove
tieujason330 added a commit that referenced this pull request Feb 12, 2022
* elasticsearch cft

* set 30 timeout

remove
tieujason330 added a commit to tieujason330/aws-saas-boost that referenced this pull request Mar 3, 2022
* elasticsearch cft

* set 30 timeout

remove
tieujason330 added a commit that referenced this pull request Mar 3, 2022
* elasticsearch cft

* set 30 timeout

remove
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