-
Notifications
You must be signed in to change notification settings - Fork 63
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
terraform: add AWS/EKS deployment for ChatQnA #480
Conversation
b94f5d2
to
dcd3cf9
Compare
@mkbhanda @daisy-ycguo PTAL. This has now been tested on AWS EKS. |
47b7faa
to
955a70b
Compare
@poussa Could you pls fix this DCO issue by sing-off your commit? i.e. git commit -s |
b9576de
to
251b461
Compare
8b282e9
to
ba5a0ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor commends and a few questions. Thank you!
3695da5
to
ede5438
Compare
Signed-off-by: Sakari Poussa <[email protected]>
1e45f2b
to
6554e0d
Compare
for more information, see https://pre-commit.ci
@mkbhanda thanks for the review. I fixed most of your suggestions and commented the rest. |
Thoughts on a folder structure that is more scalable? I also have Terraform examples and Ansible code to commit from opea-project/GenAIExamples#970 Suggestion: GenAIExamples/ChatQnA/terraform/intel/cpu/xeon/aws/eks/main.tf We'll also have Ansible, so it would scale to GenAIExamples/ChatQnA/ansible/intel/cpu/xeon/ubuntu/single-node/playbook.yaml |
My idea of the terraform structure is:
This way we don't need to expose app name (e.g. chatqna), vendor name (e.g. intel), product names (e.g., xeon) or accelerator type (e.g. gaudi) in the directory structure. They are hidden in NOTE: the top level directory can be in any repo but that is a separate topic. |
Ok, I think we are looking to solve two different problems. I think you are going for generic infrastructure for any use case. |
variable "cluster_name" { | ||
description = "EKS cluster name" | ||
type = string | ||
default = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A suggestion, remove the default so that an input is required and it prompts the user if missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the region also not have a default. Alternately a default region establishes that we can find the instance type mentioned in that region.
terraform we should add, since there will be other deployment methods like aws/cloudformation, maybe ansible. So that should have a place in the path structure. |
Of course, this is inspired by https://github.com/NVIDIA/nim-deploy/tree/main/cloud-service-providers |
Right, that was the idea. And GenAIInfra/scripts... supports Ansible, Bash, etc which should not be under /cloud-service-providers/... |
I am in favor of this organization for the same reasons that @poussa cites and also agree keeping terraform as part of the path to differentiate between cloud formation etc. Thank you @lucasmelogithub for reviewing and providing comments. I am OK with a default name of OPEA for the cluster, keep it easy. For a more savvy user with multiple clusters, they can change the vars file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -0,0 +1,4 @@ | |||
cluster_name = "opea-chatqna" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we just call it opea?
variable "cluster_name" { | ||
description = "EKS cluster name" | ||
type = string | ||
default = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the region also not have a default. Alternately a default region establishes that we can find the instance type mentioned in that region.
Description
Add AWS/EKS terraform deployment for OPEA applications. This PR add support for ChatQnA.
Followup PRs add more cloud providers and OPEA applications.
Issues
opea-project/GenAIExamples#427
opea-project/GenAIExamples#551
opea-project/GenAIExamples#550
opea-project/GenAIExamples#549
opea-project/GenAIExamples#510
Type of change
List the type of change like below. Please delete options that are not relevant.
Dependencies
None
Tests
None