Bug 1836339: GCP: Enable disk type and size customization #3595

rna-afk · 2020-05-13T16:31:05Z

Currently, the installer does not allow the users to customize the
type and size of the disks for the workers and control plane.
Added the option for the user to specify the type and the size of
the disks for both machines in GCP.

The user can specify two types of disks, pd-standard and pd-ssd disks
which are the options that GCP/Terraform provides. pd-standard is not
recommended for control planes and will not be allowed as a value in
the DefaultMachinePlatform and the master compute section.

rna-afk · 2020-05-13T17:30:21Z

/test e2e-gcp

abhinavdahiya · 2020-05-13T17:36:54Z

pkg/types/gcp/machinepools.go

+	// OSDisk defines the storage for instance.
+	//
+	// +optional
+	OSDisk `json:"OSDisk"`


Suggested change

OSDisk `json:"OSDisk"`

OSDisk `json:"osDisk"`

pkg/types/gcp/machinepools.go

pkg/types/validation/machinepools.go

abhinavdahiya · 2020-05-13T17:40:35Z

@rna-afk also make sure https://github.com/openshift/installer/blob/master/docs/dev/explain.md#generating-the-documentation is used to generate the new explain

rna-afk · 2020-05-14T13:31:02Z

also make sure https://github.com/openshift/installer/blob/master/docs/dev/explain.md#generating-the-documentation is used to generate the new explain

Nice feature! Added them.

pkg/asset/machines/gcp/machines.go

rna-afk · 2020-05-14T22:45:31Z

/test images

abhinavdahiya · 2020-05-15T16:23:07Z

/test e2e-gcp

abhinavdahiya · 2020-05-15T16:23:36Z

pkg/types/gcp/machinepools.go

+// OSDisk defines the disk for machines on GCP.
+type OSDisk struct {
+	// DiskType defines the type of disk.
+	// The valid values are <`pd-standard`, `pd-ssd`>


no need to keep the list in <>

abhinavdahiya · 2020-05-15T16:24:08Z

pkg/types/gcp/validation/machinepool.go

@@ -17,5 +19,42 @@ func ValidateMachinePool(platform *gcp.Platform, p *gcp.MachinePool, fldPath *fi
 		}
 	}

+	if p.OSDisk.DiskSizeGB < 0 {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("diskSizeGB"), p.OSDisk.DiskSizeGB, "Storage DiskSizeGB must be positive"))


Suggested change

allErrs = append(allErrs, field.Invalid(fldPath.Child("diskSizeGB"), p.OSDisk.DiskSizeGB, "Storage DiskSizeGB must be positive"))

allErrs = append(allErrs, field.Invalid(fldPath.Child("diskSizeGB"), p.OSDisk.DiskSizeGB, "must be a positive value"))

openshift-ci-robot · 2020-05-15T16:25:38Z

@rna-afk: This pull request references Bugzilla bug 1836339, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1836339: GCP: Enable disk type and size customization

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.

abhinavdahiya · 2020-05-15T17:40:09Z

/test e2e-gcp

rna-afk · 2020-05-18T17:10:01Z

/test e2e-gcp

abhinavdahiya · 2020-05-18T18:11:51Z

pkg/types/gcp/machinepools.go

+type OSDisk struct {
+	// DiskType defines the type of disk.
+	// The valid values are pd-standard and pd-ssd
+	// For master nodes, the valid value is pd-ssd.


Suggested change

// For master nodes, the valid value is pd-ssd.

// For control plane nodes, the valid value is pd-ssd.

abhinavdahiya · 2020-05-18T18:13:12Z

pkg/types/validation/machinepools.go

+	if platform != nil {
+		allErrs = append(allErrs, gcpvalidation.ValidateDefaultDiskType(platform.GCP.DefaultMachinePlatform, f)...)
+	}


i think https://github.com/openshift/installer/pull/3520/files#diff-8722408266807d908915d162b2a30ed9R23 is the better place to validate the platform's default machinepool.

abhinavdahiya · 2020-05-18T20:20:46Z

pkg/types/gcp/validation/platform_test.go

+					},
+				},
+			},
+			valid: false,


shouldn't this be valid true?

abhinavdahiya · 2020-05-18T20:24:38Z

/approve

/test e2e-gcp

openshift-ci-robot · 2020-05-18T20:24:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

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~~ [abhinavdahiya]

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

Currently, the installer does not allow the users to customize the type and size of the disks for the workers and control plane. Added the option for the user to specify the type and the size of the disks for both machines in GCP. The user can specify two types of disks, pd-standard and pd-ssd disks which are the options that GCP/Terraform provides. pd-standard is not recommended for control planes and will not be allowed as a value in the DefaultMachinePlatform and the master compute section.

abhinavdahiya · 2020-05-18T20:29:54Z

/test e2e-gcp

openshift-ci-robot · 2020-05-18T21:50:30Z

@rna-afk: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-ovirt	`d90c191`	link	`/test e2e-ovirt`
ci/prow/e2e-openstack	`d90c191`	link	`/test e2e-openstack`
ci/prow/e2e-aws-scaleup-rhel7	`d90c191`	link	`/test e2e-aws-scaleup-rhel7`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

abhinavdahiya · 2020-05-18T23:25:04Z

/lgtm

openshift-ci-robot · 2020-05-18T23:29:44Z

@rna-afk: All pull requests linked via external trackers have merged: openshift/installer#3595. Bugzilla bug 1836339 has been moved to the MODIFIED state.

In response to this:

Bug 1836339: GCP: Enable disk type and size customization

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.

rna-afk requested a review from abhinavdahiya May 13, 2020 16:31

rna-afk assigned patrickdillon May 13, 2020

openshift-ci-robot requested review from jcpowermac and jstuever May 13, 2020 16:31

abhinavdahiya reviewed May 13, 2020

View reviewed changes

rna-afk force-pushed the gcp-disk-types branch from 1249b01 to 02443d6 Compare May 13, 2020 17:37

abhinavdahiya reviewed May 13, 2020

View reviewed changes

pkg/types/gcp/machinepools.go Show resolved Hide resolved

pkg/types/validation/machinepools.go Show resolved Hide resolved

rna-afk force-pushed the gcp-disk-types branch from 02443d6 to 00ade38 Compare May 13, 2020 17:40

abhinavdahiya added the version/4.6 Tracking changes that should end up in 4.6 release label May 13, 2020

rna-afk force-pushed the gcp-disk-types branch from 00ade38 to 2aad21c Compare May 13, 2020 19:34

patrickdillon reviewed May 14, 2020

View reviewed changes

pkg/asset/machines/gcp/machines.go Outdated Show resolved Hide resolved

rna-afk force-pushed the gcp-disk-types branch from 2aad21c to 461d4d1 Compare May 14, 2020 15:34

abhinavdahiya changed the title ~~GCP: Enable disk type and size customization~~ Bug: 1836339: GCP: Enable disk type and size customization May 15, 2020

abhinavdahiya removed the version/4.6 Tracking changes that should end up in 4.6 release label May 15, 2020

abhinavdahiya reviewed May 15, 2020

View reviewed changes

rna-afk force-pushed the gcp-disk-types branch from 461d4d1 to 2a7e20d Compare May 15, 2020 16:25

abhinavdahiya changed the title ~~Bug: 1836339: GCP: Enable disk type and size customization~~ Bug 1836339: GCP: Enable disk type and size customization May 15, 2020

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 15, 2020

rna-afk force-pushed the gcp-disk-types branch 2 times, most recently from c414a6b to ab7d7e2 Compare May 15, 2020 17:29

abhinavdahiya reviewed May 18, 2020

View reviewed changes

rna-afk force-pushed the gcp-disk-types branch 2 times, most recently from b1e4e4d to bd00c26 Compare May 18, 2020 19:31

abhinavdahiya reviewed May 18, 2020

View reviewed changes

pkg/types/gcp/validation/platform_test.go Outdated

},

},

},

valid: false,

Copy link

Contributor

abhinavdahiya May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be valid true?

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2020

rna-afk force-pushed the gcp-disk-types branch from bd00c26 to d90c191 Compare May 18, 2020 20:29

openshift-ci-robot assigned abhinavdahiya May 18, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2020

openshift-merge-robot merged commit f6716ea into openshift:master May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1836339: GCP: Enable disk type and size customization #3595

Bug 1836339: GCP: Enable disk type and size customization #3595

rna-afk commented May 13, 2020

rna-afk commented May 13, 2020

abhinavdahiya May 13, 2020

abhinavdahiya commented May 13, 2020

rna-afk commented May 14, 2020

rna-afk commented May 14, 2020

abhinavdahiya commented May 15, 2020

abhinavdahiya May 15, 2020

abhinavdahiya May 15, 2020

openshift-ci-robot commented May 15, 2020

abhinavdahiya commented May 15, 2020

rna-afk commented May 18, 2020

abhinavdahiya May 18, 2020

abhinavdahiya May 18, 2020

abhinavdahiya May 18, 2020

abhinavdahiya commented May 18, 2020

openshift-ci-robot commented May 18, 2020

abhinavdahiya commented May 18, 2020

openshift-ci-robot commented May 18, 2020 •

edited

abhinavdahiya commented May 18, 2020

openshift-ci-robot commented May 18, 2020

	allErrs = append(allErrs, field.Invalid(fldPath.Child("diskSizeGB"), p.OSDisk.DiskSizeGB, "Storage DiskSizeGB must be positive"))
	allErrs = append(allErrs, field.Invalid(fldPath.Child("diskSizeGB"), p.OSDisk.DiskSizeGB, "must be a positive value"))

	// For master nodes, the valid value is pd-ssd.
	// For control plane nodes, the valid value is pd-ssd.

Bug 1836339: GCP: Enable disk type and size customization #3595

Bug 1836339: GCP: Enable disk type and size customization #3595

Conversation

rna-afk commented May 13, 2020

rna-afk commented May 13, 2020

abhinavdahiya May 13, 2020

Choose a reason for hiding this comment

abhinavdahiya commented May 13, 2020

rna-afk commented May 14, 2020

rna-afk commented May 14, 2020

abhinavdahiya commented May 15, 2020

abhinavdahiya May 15, 2020

Choose a reason for hiding this comment

abhinavdahiya May 15, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented May 15, 2020

abhinavdahiya commented May 15, 2020

rna-afk commented May 18, 2020

abhinavdahiya May 18, 2020

Choose a reason for hiding this comment

abhinavdahiya May 18, 2020

Choose a reason for hiding this comment

abhinavdahiya May 18, 2020

Choose a reason for hiding this comment

abhinavdahiya commented May 18, 2020

openshift-ci-robot commented May 18, 2020

abhinavdahiya commented May 18, 2020

openshift-ci-robot commented May 18, 2020 • edited

abhinavdahiya commented May 18, 2020

openshift-ci-robot commented May 18, 2020

openshift-ci-robot commented May 18, 2020 •

edited