-
Notifications
You must be signed in to change notification settings - Fork 69
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
Use Hosted Control Planes for ROSA to speed up cluster creation #748
Conversation
375545c
to
31f246e
Compare
f8930fc
to
e2986e1
Compare
Multi-az clusters created and deployed as expected on inspection: https://github.com/ryanemerson/keycloak-benchmark/actions/runs/8452406289 |
provision/opentofu/modules/rosa/hcp/scripts/rosa_verify_network.sh
Outdated
Show resolved
Hide resolved
variable "openshift_version" { | ||
type = string | ||
default = "4.14.5" | ||
} | ||
|
||
variable "instance_type" { | ||
type = string | ||
default = "m5.4xlarge" | ||
} | ||
|
||
variable "replicas" { | ||
type = number | ||
default = 2 | ||
} |
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.
Nitpick: As we now have a good place here for the defaults, I would tend to not keep them in other places and just maintain them here.
So we would have fewer parameters on our workflows. It would also be ok to remove some and keep some from the list if you think we're about to use them now and then.
As this is a nitpick, feel free to keep for another future PR.
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've removed the default values from the GH rosa-cluster-create workflow, but left the inputs so that the default can still be overridden. The rosa_create_cluster.sh
script then adds the variables as -var
args to tofu
if a value is specified.
@ryanemerson - great job, happy to see the first working cluster created via OpenTofu! |
3b17e86
to
ff2be5a
Compare
I've addressed all comments and have executed a new GH run to verify: https://github.com/ryanemerson/keycloak-benchmark/actions/runs/8465601858 |
ff2be5a
to
2affb88
Compare
The previous run uncovered an issue with the EFS script. All working as expected now: https://github.com/ryanemerson/keycloak-benchmark/actions/runs/8466454176 |
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 the change, it is good to see it will shave off 2x20 min for building our ROSA clusters.
I'm approving it today, and will merge it on Monday or Tuesday, so we'll not run into trouble over the long weekend.
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.
Nice work @ryanemerson! This is a huge step forward towards using OpenTofu! I expected creating ROSA clusters would be simpler in tofu though.
Anyway, I think this is ready for merging. I added two comments but even if it will require some changes I am ok to do it later.
} | ||
cd ${SCRIPT_DIR}/../opentofu/modules/rosa/hcp | ||
tofu init | ||
tofu workspace new ${CLUSTER_NAME} || true |
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 am confused why this is here. Is each cluster in a separate workspace? My expectation was there will be a new workspace created in the beginning and then all resources will be created in that workspace. For example, the daily run will have only one workspace. Did I miss something?
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 don't think it's possible for us to use a single workspace for two clusters as we're effectively using the same module but with different configurations in order to create the two clusters. I think on executing apply for the second time with a different cluster_name
opentofu will attempt to update the first cluster's resources.
This is all new to me as well though, so maybe I'm misunderstanding 🙂
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.
It is new to me as well but using the same module more than once should be possible.
For example, I used two modules here: https://github.com/mhajas/keycloak-benchmark/blob/opentofu-poc/provision/opentofu/main.tf#L7-L14
You can also specify aliases for providers and use more provider configurations within one tf file like this:
https://github.com/mhajas/keycloak-benchmark/blob/opentofu-poc/provision/opentofu/infinispan/main.tf#L7
and
https://github.com/mhajas/keycloak-benchmark/blob/opentofu-poc/provision/opentofu/infinispan/main.tf#L92
But as I said, it is ok for me to merge this as is and play with enhancements later.
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.
For example, I used two modules here: https://github.com/mhajas/keycloak-benchmark/blob/opentofu-poc/provision/opentofu/main.tf#L7-L14
I hadn't considered referencing two modules from the root like that 🙂
I think the downside to ^ approach is that we would have to make more changes to our various *.sh scripts and GH actions as we would either need to replace the use of rosa_create_cluster.sh
or adapt it to support creating multiple clusters at once.
The flip side is that it would definitely make it easier to create multiple clusters in parallel, as the first stage of the module could be to determine CIDR ranges for both clusters before provisioning them.
- Remove multi-az cluster create options - Remove need for ccoctl when provisioning EFS as the cloud-credential-operator is not installed on HCP clusters Resolves keycloak#673 Signed-off-by: Ryan Emerson <[email protected]>
…es for aurora_delete.sh. Fixes keycloak#750 Signed-off-by: Ryan Emerson <[email protected]>
2affb88
to
2595ef4
Compare
I just realized one more thing. I probably know the answer as we are using the same script for choosing CIDR but I think it is worth double-checking. Can we run cluster creation in parallel with the new approach? https://github.com/keycloak/keycloak-benchmark/blob/main/.github/workflows/rosa-multi-az-cluster-create.yml#L61-L62 |
Not yet, AFAIK, as we're still using the same script. Still it is a worthy next step. At the moment the script checks only the available IP address, but doesn't allocate it. Maybe we can find a mechanism to reserve the CIDR by setting some expiring information somewhere (maybe a S3 bucket entry with a timeout?) No idea what the most lightweight approach would be. cc: @ryanemerson |
I think the simplest way, i.e. no additional dependencies, to allow parallel creation would be to utilise a terraform module that creates multiple clusters in a single workspace as suggested by @mhajas #748 (comment). |
I have removed the multi-az cluster options associated with creating rosa clusters as we haven't been using these and we don't have a need for this functionality, at least in the short term. If there are any objections I can attempt to reinstate these.
Resolves #673
Resolves #750
Remaining actions: