OCM-2641, OCM-17: Specify worker disk size for machine pools #244

JohnStrunk · 2023-08-17T15:57:44Z

OCM-2641: Adds worker_disk_size to cluster resource. This is used only at cluster creation to determine the size of the default MP's disk size
OCM-17: Adds disk_size to the machinepool resource. -- Only used at pool creation time

Supersceedes #100

openshift-ci-robot · 2023-08-17T15:57:48Z

@JohnStrunk: This pull request references OCM-17 which is a valid jira issue.

In response to this:

Adds worker_disk_size to cluster resource. This is used only at cluster creation to determine the size of the default MP's disk size

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/test-infra repository.

openshift-ci · 2023-08-17T15:57:56Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2023-08-17T18:05:16Z

@JohnStrunk: This pull request references OCM-17 which is a valid jira issue.

In response to this:

OCM-2641: Adds worker_disk_size to cluster resource. This is used only at cluster creation to determine the size of the default MP's disk size

OCM-17: Adds disk_size to the machinepool resource. -- Only used at pool creation time

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/test-infra repository.

openshift-ci-robot · 2023-08-17T18:05:19Z

@JohnStrunk: This pull request references OCM-17 which is a valid jira issue.

In response to this:

OCM-2641: Adds worker_disk_size to cluster resource. This is used only at cluster creation to determine the size of the default MP's disk size

OCM-17: Adds disk_size to the machinepool resource. -- Only used at pool creation time

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/test-infra repository.

JohnStrunk · 2023-08-17T18:07:57Z

/hold
There seems to be a bug affecting modifying clusters w/ non-standard root disk sizes - OCM-3323

JohnStrunk · 2023-08-17T18:09:41Z

@bardielle This is ready for a first look, though I want to do some more testing once the above bug is resolved.

openshift-ci-robot · 2023-08-17T18:10:29Z

@JohnStrunk: This pull request references OCM-17 which is a valid jira issue.

In response to this:

OCM-2641: Adds worker_disk_size to cluster resource. This is used only at cluster creation to determine the size of the default MP's disk size

OCM-17: Adds disk_size to the machinepool resource. -- Only used at pool creation time

Supersceedes #100

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/test-infra repository.

bardielle · 2023-08-21T06:31:48Z

provider/cluster_rosa_classic_resource.go

+				Optional:    true,
+				Computed:    true,
+				PlanModifiers: []tfsdk.AttributePlanModifier{
+					tfsdk.UseStateForUnknown(),


I think we can add a validation here on the disk size value

JohnStrunk · 2023-08-24T18:20:25Z

/unhold
This is ready

amalykhi · 2023-08-27T07:40:07Z

/retest

amalykhi · 2023-08-27T08:52:54Z

/retest

amalykhi · 2023-08-27T09:15:38Z

/retest

amalykhi · 2023-08-27T14:58:28Z

/retest

JohnStrunk · 2023-08-29T13:02:32Z

/retest

nirarg · 2023-09-05T07:18:25Z

internal/ocm/resource/cluster_test.go

@@ -15,22 +15,22 @@ var _ = Describe("Cluster", func() {
 	})
 	Context("CreateNodes validation", func() {
 		It("Autoscaling disabled minReplicas set - failure", func() {
-			err := cluster.CreateNodes(false, nil, pointer(int64(2)), nil, nil, nil, nil, false)
+			err := cluster.CreateNodes(false, nil, pointer(int64(2)), nil, nil, nil, nil, false, nil)


Please add one test with non empty diskSize value

nirarg · 2023-09-05T07:37:09Z

internal/ocm/resource/cluster.go

+	if workerDiskSize != nil {
+		nodes.ComputeRootVolume(
+			cmv1.NewRootVolume().AWS(
+				cmv1.NewAWSVolume().Size(int(*workerDiskSize)),


Do we need to set default value here or just use the default value in the OCM backend?
What is the behavior in the CLI?

In the CLI, if the size is not set or identical to the flavor's default size, we do nothing and let the backend handle the defaults.

nirarg · 2023-09-05T07:41:06Z

provider/machine_pool_resource.go

@@ -184,6 +184,16 @@ func (t *MachinePoolResourceType) GetSchema(ctx context.Context) (result tfsdk.S
 					ValueCannotBeChangedModifier(),
 				},
 			},
+			"disk_size": {


Please generate the attributes in the documentation

regenerate it please

nirarg · 2023-09-06T09:16:31Z

/hold Will be merged only after v1.3.0 release

nirarg · 2023-09-06T09:25:34Z

/hold
This feature is on hold
Will be merged only after v1.3.0 release

JohnStrunk · 2023-09-21T20:57:47Z

CI failure during cluster destroy:

�[31m│�[0m �[0m�[1m�[31mError: �[0m�[0m�[1mdeleting IAM Role (rhcsci-soph7-ControlPlane-Role): unable to detach instance profiles: AccessDenied: User: arn:aws:iam::425464789085:user/terraform-provider-minimal is not authorized to perform: iam:RemoveRoleFromInstanceProfile on resource: instance profile rhcsci-soph7-v9jl4-master-profile because no identity-based policy allows the iam:RemoveRoleFromInstanceProfile action
�[31m│�[0m �[0m	status code: 403, request id: 33115eec-f165-4a5b-a4c7-3d908af11df0�[0m
�[31m│�[0m �[0m
�[31m│�[0m �[0m�[0m
�[31m╵�[0m�[0m
�[31m╷�[0m�[0m
�[31m│�[0m �[0m�[1m�[31mError: �[0m�[0m�[1mdeleting IAM Role (rhcsci-soph7-Worker-Role): unable to detach instance profiles: AccessDenied: User: arn:aws:iam::425464789085:user/terraform-provider-minimal is not authorized to perform: iam:RemoveRoleFromInstanceProfile on resource: instance profile rhcsci-soph7-v9jl4-worker-profile because no identity-based policy allows the iam:RemoveRoleFromInstanceProfile action
�[31m│�[0m �[0m	status code: 403, request id: cd877cb3-7361-4d5d-9628-5afdeeca52bd�[0m

Seems to be an AWS permission problem.

JohnStrunk · 2023-09-22T12:22:20Z

/retest

Adds `worker_disk_size` to cluster resource. This is used only at cluster creation to determine the size of the default MP's disk size Signed-off-by: John Strunk <[email protected]>

Signed-off-by: John Strunk <[email protected]>

bardielle · 2023-10-22T13:57:48Z

provider/clusterrosaclassic/cluster_rosa_classic_resource.go

@@ -252,6 +252,10 @@ func (r *ClusterRosaClassicResource) Schema(ctx context.Context, req resource.Sc
 					stringplanmodifier.RequiresReplace(),
 				},
 			},
+			"worker_disk_size": schema.Int64Attribute{


we should throw an error if the user tries to update it

I think we can add a plan modifier that prevents this value being changed.

This option needs to be treated identically to what's being done in #228 since this setting is configuring the default MP.

Since the framework upgrade, we have removed all of the ValueCannotBeChanged checks. Should I look into re-introducing that?
And, what about #228-- should that be changed as well?
If so, I'd prefer to move these checks to another PR.

I think it should also be changed in #228

bardielle · 2023-10-22T13:59:10Z

I had 2 comments but overall it looks good

Signed-off-by: John Strunk <[email protected]>

bardielle · 2023-10-24T11:18:14Z

/retest

bardielle · 2023-10-24T11:18:20Z

/lgtm

bardielle · 2023-10-24T11:18:51Z

/unhold

JohnStrunk · 2023-10-24T13:39:50Z

/retest

JohnStrunk · 2023-10-24T15:21:29Z

Looks like CI troubles again:

2023-10-24T13:48:04.183Z [ERROR] provider.terraform-provider-aws_v5.22.0_x5: Response contains error diagnostic: @caller=github.com/hashicorp/[email protected]/tfprotov5/internal/diag/diagnostics.go:58 tf_proto_version=5.4 tf_resource_type=aws_s3_bucket_policy tf_provider_addr=registry.terraform.io/hashicorp/aws @module=sdk.proto diagnostic_detail="" diagnostic_severity=ERROR tf_rpc=ApplyResourceChange diagnostic_summary="putting S3 Bucket (oidc-l1x6) Policy: operation error S3: PutBucketPolicy, https response error StatusCode: 403, RequestID: XVKX5BJF6YSHM3FF, HostID: m20I89LXf7YOjhYoGQoJpiVBtKn9wZMY0PyljlpKjMkTklb300pcS/J5j2mfCsPjrXnpTKwSwxdiTKp9vVz/ag==, api error AccessDenied: Access Denied" tf_req_id=b88605aa-841c-fb26-e157-5ed8fce16572 timestamp=2023-10-24T13:48:04.182Z
2023-10-24T13:48:04.185Z [ERROR] vertex "module.oidc_config_input_resources.module.rosa_oidc_config_resources[0].aws_s3_bucket_policy.allow_access_from_another_account" error: putting S3 Bucket (oidc-l1x6) Policy: operation error S3: PutBucketPolicy, https response error StatusCode: 403, RequestID: XVKX5BJF6YSHM3FF, HostID: m20I89LXf7YOjhYoGQoJpiVBtKn9wZMY0PyljlpKjMkTklb300pcS/J5j2mfCsPjrXnpTKwSwxdiTKp9vVz/ag==, api error AccessDenied: Access Denied

putting S3 Bucket (oidc-l1x6) Policy: operation error S3: PutBucketPolicy, https response error StatusCode: 403, RequestID: XVKX5BJF6YSHM3FF, HostID: m20I89LXf7YOjhYoGQoJpiVBtKn9wZMY0PyljlpKjMkTklb300pcS/J5j2mfCsPjrXnpTKwSwxdiTKp9vVz/ag==, api error AccessDenied: Access Denied

with module.oidc_config_input_resources.module.rosa_oidc_config_resources[0].aws_s3_bucket_policy.allow_access_from_another_account,
 on .terraform/modules/oidc_config_input_resources/oidc_config_resources/create_s3_bucket.tf line 21, in resource "aws_s3_bucket_policy" "allow_access_from_another_account":
  21: resource "aws_s3_bucket_policy" "allow_access_from_another_account"

amalykhi · 2023-10-25T07:02:26Z

/retest

sagidayan · 2023-10-29T13:36:11Z

/approve

openshift-ci · 2023-10-29T13:36:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sagidayan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sagidayan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the jira/valid-reference label Aug 17, 2023

openshift-ci bot added the do-not-merge/work-in-progress label Aug 17, 2023

JohnStrunk force-pushed the root-disk-size branch 2 times, most recently from cdff057 to 0907376 Compare August 17, 2023 18:00

JohnStrunk marked this pull request as ready for review August 17, 2023 18:06

openshift-ci bot removed the do-not-merge/work-in-progress label Aug 17, 2023

openshift-ci bot requested review from sagidayan and vkareh August 17, 2023 18:06

openshift-ci bot added the do-not-merge/hold label Aug 17, 2023

bardielle reviewed Aug 21, 2023

View reviewed changes

JohnStrunk force-pushed the root-disk-size branch from 0907376 to 5a8a32b Compare August 24, 2023 17:42

openshift-ci bot removed the do-not-merge/hold label Aug 24, 2023

leseb mentioned this pull request Aug 29, 2023

OCM-16: ability to set worker disk size #100

Closed

nirarg reviewed Sep 5, 2023

View reviewed changes

JohnStrunk force-pushed the root-disk-size branch from 0c54fc2 to 7636768 Compare September 5, 2023 16:04

openshift-ci bot added the do-not-merge/hold label Sep 6, 2023

openshift-merge-robot added the needs-rebase label Sep 16, 2023

JohnStrunk force-pushed the root-disk-size branch from 7636768 to e05aa9f Compare September 21, 2023 20:23

openshift-merge-robot removed the needs-rebase label Sep 21, 2023

JohnStrunk added 2 commits October 20, 2023 14:48

OCM-2641: Specify worker disk size for default MP

be32ab2

Adds `worker_disk_size` to cluster resource. This is used only at cluster creation to determine the size of the default MP's disk size Signed-off-by: John Strunk <[email protected]>

OCM-17: Specify worker disk size for machine pools

1f84947

Signed-off-by: John Strunk <[email protected]>

JohnStrunk force-pushed the root-disk-size branch from e05aa9f to 1f84947 Compare October 20, 2023 18:49

bardielle reviewed Oct 22, 2023

View reviewed changes

Regenerate docs

25107e7

Signed-off-by: John Strunk <[email protected]>

openshift-ci bot assigned bardielle Oct 24, 2023

openshift-ci bot added the lgtm label Oct 24, 2023

openshift-ci bot removed the do-not-merge/hold label Oct 24, 2023

openshift-ci bot added the approved label Oct 29, 2023

openshift-ci bot merged commit 5b6927c into terraform-redhat:main Oct 29, 2023
3 checks passed

OCM-2641, OCM-17: Specify worker disk size for machine pools #244

OCM-2641, OCM-17: Specify worker disk size for machine pools #244

Conversation

JohnStrunk commented Aug 17, 2023 • edited Loading

openshift-ci-robot commented Aug 17, 2023 • edited by openshift-ci bot Loading

openshift-ci bot commented Aug 17, 2023

openshift-ci-robot commented Aug 17, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented Aug 17, 2023 • edited by openshift-ci bot Loading

JohnStrunk commented Aug 17, 2023 • edited by openshift-ci bot Loading

JohnStrunk commented Aug 17, 2023

openshift-ci-robot commented Aug 17, 2023 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

JohnStrunk commented Aug 24, 2023

amalykhi commented Aug 27, 2023

amalykhi commented Aug 27, 2023

amalykhi commented Aug 27, 2023

amalykhi commented Aug 27, 2023

JohnStrunk commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirarg commented Sep 6, 2023

nirarg commented Sep 6, 2023

JohnStrunk commented Sep 21, 2023

JohnStrunk commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bardielle commented Oct 22, 2023

bardielle commented Oct 24, 2023

bardielle commented Oct 24, 2023

bardielle commented Oct 24, 2023

JohnStrunk commented Oct 24, 2023

JohnStrunk commented Oct 24, 2023

amalykhi commented Oct 25, 2023

sagidayan commented Oct 29, 2023

openshift-ci bot commented Oct 29, 2023

JohnStrunk commented Aug 17, 2023 •

edited

Loading

openshift-ci-robot commented Aug 17, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 17, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 17, 2023 •

edited by openshift-ci bot

Loading

JohnStrunk commented Aug 17, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 17, 2023 •

edited by openshift-ci bot

Loading