ROSAENG-59425 | feat: add worker disk size support#158
Conversation
|
Hi @luis-falcon. Thanks for your PR. I'm waiting for a terraform-redhat member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a nullable ChangesWorker disk size parameter configuration
sequenceDiagram
participant Root as Root Terraform config
participant Module as module.rosa_cluster_hcp
participant Resource as rhcs_cluster_rosa_hcp.rosa_hcp_cluster
Root->>Module: pass worker_disk_size = var.worker_disk_size
Module->>Resource: set worker_disk_size = var.worker_disk_size
🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/rosa-cluster-hcp/main.tf`:
- Line 76: Add module tests to modules/rosa-cluster-hcp/tests/*.tftest.hcl to
cover the worker_disk_size passthrough in both set and unset paths: create one
test case where worker_disk_size = null (to assert the module omits the value or
uses the platform default) and a second test case where worker_disk_size = 400
(to assert the module forwards the explicit value into the child
module/variable). Use the existing test harness/mocks used by other tests in the
directory to instantiate the module, assert the rendered configuration contains
the expected behavior for the worker_disk_size attribute, and name the tests
clearly (e.g., worker_disk_size_null and worker_disk_size_explicit).
In `@modules/rosa-cluster-hcp/variables.tf`:
- Around line 316-320: Add a validation block to the variable "worker_disk_size"
that only allows null or whole positive GiB values: require var.worker_disk_size
== null || (var.worker_disk_size >= 1 && floor(var.worker_disk_size) ==
var.worker_disk_size). Update the variable "worker_disk_size" declaration to
include a validation { condition = ... error_message = "worker_disk_size must be
null or a whole positive integer (GiB)." } so fractional or negative inputs are
rejected at module input validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c40bccdf-ffd3-489f-aa8e-139299ed00f2
📒 Files selected for processing (4)
main.tfmodules/rosa-cluster-hcp/main.tfmodules/rosa-cluster-hcp/variables.tfvariables.tf
luis-falcon
changed the title
luis-falcon
force-pushed
the
ROSAENG-59425-add-worder-disk-size-support
branch
3 times, most recently
from
06e5406 to
5a78a9b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/ok-to-test |
luis-falcon
force-pushed
the
ROSAENG-59425-add-worder-disk-size-support
branch
from
5a78a9b to
7832f6a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/rosa-cluster-hcp/variables.tf (1)
316-320: ⚡ Quick winConsider enriching the description to match the root variable's clarity.
The root
variables.tfprovides a more helpful description:"Default worker machine pool root disk size in GiB (e.g., 300, 400, 500). Leave null to use platform default."The module's description could benefit from similar detail about the null behavior and example values to improve the user experience at this public module boundary.📝 Suggested enhancement
variable "worker_disk_size" { type = number default = null - description = "Worker node root disk size in GiB." + description = "Worker node root disk size in GiB (e.g., 300, 400, 500). Leave null to use platform default." validation {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/rosa-cluster-hcp/variables.tf` around lines 316 - 320, Update the variable "worker_disk_size" description to match the clarity of the root module: state that it is the default worker machine pool root disk size in GiB, show example values (e.g., 300, 400, 500), and explicitly mention that setting it to null will use the platform default; modify the description string in the variable "worker_disk_size" block accordingly so callers at the module boundary get the same guidance as the root variables.tf.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@modules/rosa-cluster-hcp/variables.tf`:
- Around line 316-320: Update the variable "worker_disk_size" description to
match the clarity of the root module: state that it is the default worker
machine pool root disk size in GiB, show example values (e.g., 300, 400, 500),
and explicitly mention that setting it to null will use the platform default;
modify the description string in the variable "worker_disk_size" block
accordingly so callers at the module boundary get the same guidance as the root
variables.tf.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 078fe8d2-7f73-4268-8419-e6e2003e7dde
📒 Files selected for processing (5)
main.tfmodules/rosa-cluster-hcp/main.tfmodules/rosa-cluster-hcp/tests/rosa_cluster_hcp.tftest.hclmodules/rosa-cluster-hcp/variables.tfvariables.tf
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/rosa-cluster-hcp/tests/rosa_cluster_hcp.tftest.hcl
- modules/rosa-cluster-hcp/main.tf
luis-falcon
force-pushed
the
ROSAENG-59425-add-worder-disk-size-support
branch
from
7832f6a to
a871ea4
Compare
|
/approve |
luis-falcon
force-pushed
the
ROSAENG-59425-add-worder-disk-size-support
branch
from
a871ea4 to
34413a2
Compare
luis-falcon
changed the title
|
/ok-to-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amandahla, luis-falcon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: lfalconm <lfalconm@redhat.com>
luis-falcon
force-pushed
the
ROSAENG-59425-add-worder-disk-size-support
branch
from
34413a2 to
3039d40
Compare
|
/override ci/prow/rosa-hcp-public Failued due VPC error while destroying/clean up |
|
@amandahla: Overrode contexts on behalf of amandahla: ci/prow/rosa-hcp-private, ci/prow/rosa-hcp-public DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/lgtm |
|
/ok-to-test |
|
/override ci/prow/rosa-hcp-public |
|
@amandahla: Overrode contexts on behalf of amandahla: ci/prow/rosa-hcp-private, ci/prow/rosa-hcp-public DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/check-required-labels |
openshift-merge-bot
Bot
merged commit ad256db
into
terraform-redhat:main
3 participants
Customer has requirements from IBM to have at least 300Gb of storage for IBM Maximo Application Suite.
They're deploying Openshift on AWS Platform using ROSA HCP.
They would like to increase the worker node root disk size beyond the default 300 GiB. The ROSA HCP documentation mentions support for machine pool disk sizing (for example via --worker-disk-size), but we have not been able to find a way to configure this when using the official Terraform module.
The underlying provider supports it since version 1.6.5 https://github.com/terraform-redhat/terraform-provider-rhcs/blob/main/CHANGELOG.md
But the module does not have a variable that allows this option to be passed.
why this change is needed,
To be able to choose disk size for default worker nodepool at creation time. Rosa cli supports already this with the (--worker-disk-size) flag.
what changed,
Only added variables needed for this feature to be enables and be passed to the underlying provider (https://github.com/terraform-redhat/terraform-provider-rhcs)
how you validated it.
I have made the terraform init and terraform validate. I crated a terraform main.tf using the documentation found in https://docs.redhat.com/en/documentation/red_hat_openshift_service_on_aws/4/html/install_clusters/creating-a-red-hat-openshift-service-on-aws-cluster-with-terraform and adding the new parameter as a variable like so :
module "rosa-hcp" { source = "terraform-redhat/rosa-hcp/rhcs" version = "1.6.3" cluster_name = local.cluster_name openshift_version = var.openshift_version account_role_prefix = local.cluster_name operator_role_prefix = local.cluster_name replicas = local.worker_node_replicas aws_availability_zones = local.region_azs ... worker_disk_size. = 400I deployed a full cluster and verified the nodepools were the correct disk size passed during creation .
Commit format requirement:
[JIRA-TICKET] | [TYPE]:
TYPE must be one of:
feat, fix, docs, style, refactor, test, chore, build, ci, perf
For details, see: ./CONTRIBUTING.md
-->
Summary by CodeRabbit
New Features
Tests