Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add terraform for utility cluster. Add name override to gke #30847
add terraform for utility cluster. Add name override to gke #30847
Changes from 6 commits
fbc7fcd
05fad46
d308558
a5e0ed2
d3a57fe
74f14d6
173bfce
312552a
42f4d04
c647c5d
c7f26d7
a15e358
d986673
9ae533c
8d69572
48796d5
2ec1b81
dc51b45
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Thank you for adding outputs :-). Could you tell me what this output is needed for?
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.
This output is used in the upper module for helm to authenticate
https://github.com/apache/beam/pull/30847/files#diff-06d3b2b8f4c2307a7709a5e070cf31b505b71432cc46eca5871a402e74163eb8R33-R39
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.
I think the provisioning of the Kubernetes cluster and any workloads that depend on it should be in separate terraform modules. Then one would just follow typical gcloud command to connect.
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.
Could we remove this variable and just have the prefix to keep it simple?
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.
We need a predictable name so we dont have to change x number of workflows each time we redeploy for any reason. I would like to keep it this way
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.
Why not just keep the kafka cluster running continually and delete the topics after the workflows execute?
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.
We've had flakey tests in this repository due to waiting on spinning up new clusters.
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.
This way we ensure its fresh each time which is easier then to maintain a kafka instance and make sure it does not break between different tests. We could delete topics but still there could be issues.
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.
Could you please add more details about what the intent is to use this cluster instead of "datastores"?
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.
Done
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.
Maybe one can just create a new
tfvars
file storing these values and have the workflow involve provisioning the Kubernetes cluster first, separate from the strimzi workload.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.
This could be in its own module separate from the GKE cluster provisioning.
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.
Yes it is possible to put it in its own module but the idea behind the
utility-cluster
folder is to use the GKE module and install everything that is needed for that exact purpose via terraform and in one step. It does not make sense for me to separate out a module as there is no intetion to reuse this due to its specific purpose. Other clusters can crate different folders for different purposes.Let me know if this is fine and if not ill try to come up with different structure
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.
In my experience, I find co-mingling GKE provisioning with Kubernetes workload provisioning in the same terraform module to lead to problems in the future. I personally would like to see it in a separate module. I'm more than willing to defer to another Apache Beam committer's opinion, if they think the co-mingling design is ok and have a logical well articulated reason. Otherwise, I'm not comfortable approving this PR with the current design.
In summary, my design preference is:
.test-infra/terraform/google-cloud-platform/google-kubernetes-engine
folder could work