Add the ability to configure disk size and type for GCP clusters (Kube Deploy 638) #77

spew · 2018-04-17T17:00:15Z

What this PR does / why we need it:
This PR enables administrators using gcp-deployer to customize the size and types of the disks in their clusters.

Special notes for your reviewer:
Please ignore the first commit, a5bcde4 (Refactor GCEClient: wrap compute.Service in an interface for mocking GCP compute), it is a prerequisite commit and has its own PR open here: #73.

Release note:

Add the ability to customize the size and type of disks of GCP compute nodes.

@kubernetes/kube-deploy-reviewers

k8s-ci-robot · 2018-04-17T17:00:29Z

Hi @spew. Thanks for your PR.

I'm waiting for a kubernetes or kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

krousey · 2018-04-17T18:30:20Z

/ok-to-test

krousey · 2018-04-17T20:00:29Z

/test all

spew · 2018-04-30T17:35:33Z

Rebased on top of the latest.

spew · 2018-04-30T17:38:19Z

/retest

spew · 2018-04-30T17:38:45Z

/test all

k8s-ci-robot · 2018-04-30T17:44:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: spew
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jessicaochen

Assign the PR to them by writing /assign @jessicaochen in a comment when ready.

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

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

jessicaochen · 2018-05-01T19:49:15Z

cloud/google/machineactuator.go

+	for idx, disk := range config.Disks {
+		diskSizeGb := disk.InitializeParams.DiskSizeGb
+		if diskSizeGb < minDiskSizeGb {
+			diskSizeGb = minDiskSizeGb


Might want to spit out a warning log that the configuration is bad and you are defaulting to the min.

Yes, will do.

jessicaochen · 2018-05-01T19:53:36Z

cloud/google/machineactuator_test.go

+	}
+	return c.mockZoneOperationsGet(project, zone, operation)
+}
+


I would suggest creating the cluster and machine in the code instead of parsing yaml files. Parsing is the CLI tool's job and not the machine actuator's job. Plus, it is easier to test more dynamic scenarios when the objects are not hardcoded in a file somewhere.

There's some complexity here in that "machineactuator.go" expects to decode() the raw bytes of machine.Spec.ProviderConfig. For that reason I was trying to use yaml to setup various scenarios as it requires less changes.

I'm experimenting with some other ideas where the decoding() is decoupled from machineactuator.go. I will likely update the PR with a different approach.

jessicaochen · 2018-05-01T19:54:19Z

cloud/google/machineactuator_test.go

+	return c.mockZoneOperationsGet(project, zone, operation)
+}
+
+func TestOneDisk(t *testing.T) {


I may have just not seen it but is there a test for your logic which will set the min disk size if the given disk size is too small?

Yeah, we should add this test. I will do so. Note that this is pre-existing behavior as of a few days ago where another commit effectively set the disk size to 30GB at all times.

jessicaochen · 2018-05-01T19:58:20Z

cloud/google/gceproviderconfig/types.go

@@ -30,4 +30,15 @@ type GCEProviderConfig struct {

 	// The name of the OS to be installed on the machine.
 	OS string `json:"os"`
+	Image       string `json:"image"`


The OS is what eventually maps to the OS Image. Why is this field being introduced in this PR?

This was added as an error with resolving a merge conflict. Removing.

jessicaochen · 2018-05-01T19:59:30Z

cloud/google/gceproviderconfig/types.go

+	Disks       []Disk `json:"disks"`
+}
+
+type Disk struct {


Since you are modifying the the gce provider config, remember to bump all relevant images after merge. eg. the gce-machine-actuator image.

Thanks for the reminder -- I was not aware of this.

jessicaochen · 2018-05-01T20:02:28Z

pkg/apis/cluster/v1alpha1/machine_types.go

@@ -183,3 +185,16 @@ func (MachineSchemeFns) DefaultingFunction(o interface{}) {
 	// set default field values here
 	log.Printf("Defaulting fields for Machine %s\n", obj.Name)
 }
+
+func ParseMachineYaml(file string) (*Machine, error) {


This seems awfully specific to the machine actuator tests.

It is assuming that the file contains only one machine object and no other objects. Perhaps it is better to co-locate such logic with the tests and then promote to a common location if other code actually uses it?

This will be removed in a future revision of this PR.

spew · 2018-05-03T16:04:05Z

After revisiting things to rebase on top of latest as well as attempt to use test fixtures / objects for machine/cluster instead of YAML I reworked things quite a bit. I have a prerequisite commit / PR open here:

#130

Until that is through, please ignore this PR:

spew · 2018-05-09T20:36:42Z

PR 130 is through and committed, so I rebased this commit on top of the latest and it is ready for review again.

@jessicaochen
@krousey

jessicaochen

lgtm (allowing krousey to provide backslash)

jessicaochen · 2018-05-10T17:33:45Z

cloud/google/machineactuator.go

+	for idx, disk := range config.Disks {
+		diskSizeGb := disk.InitializeParams.DiskSizeGb
+		if diskSizeGb < minDiskSizeGb {
+			glog.Info("increasing disk size to %$v gb, the supplied disk size of %v gb is below the minimum", minDiskSizeGb, diskSizeGb)


Is the %$v intentional?

Not a nit -- definitely a mistake. Will correct as well as rebase on top of the latest changes (they look significant).

Ah. In that case, I will withdraw my approval so the PR dashboard will tell me to take a look after the rebase.

jessicaochen

Ah. In that case, I will withdraw my approval so the PR dashboard will tell me to take a look after the rebase.

spew · 2018-05-10T19:54:08Z

PR is rebased onto the tip of master.

jessicaochen

lgtm (allowing krousey to provide backslash)

spew · 2018-05-15T16:40:22Z

@krousey Have you had a chance to look at this before I click merge?

krousey · 2018-05-15T17:33:04Z

@spew I've been out for a bit. Looking now. Do no click merge.

krousey · 2018-05-15T17:43:16Z

cloud/google/gceproviderconfig/v1alpha1/types.go

+}
+
+type Disk struct {
+	InitializeParams DiskInitializeParams `json:"initializeParams"`


Why add this level of indirection? Wouldn't

disks: - diskSizeGb: 30 diskType: "pd-standard" - diskSizeGb: 50 diskType: "pd-standard"

work?

It would be then it would not match the GCE API / structure which is I thought what we wanted to do. It has the InitializeParams structure.

https://cloud.google.com/compute/docs/reference/rest/beta/instances/insert

Do agree what you are proposing is fine too -- let me know if you want to move to that structure.

krousey · 2018-05-15T17:47:51Z

cloud/google/machineactuator.go

+			AutoDelete: true,
+			Boot:       idx == 0,
+			InitializeParams: &compute.AttachedDiskInitializeParams{
+				SourceImage: imagePath,


I think image path is only for the boot disk.

While all disks can have an image, this is a good catch in that the intention of this change was not to put the image on disks other than the boot disk! I'll change this and add a test for this scenario.

krousey · 2018-05-15T17:53:49Z

cloud/google/gceproviderconfig/v1alpha1/register.go

+	return &codec, nil
+}
+
+func (codec *GCEProviderConfigCodec) DecodeFromProviderConfig(providerConfig clusterv1.ProviderConfig) (*GCEProviderConfig, error) {


cc @mkjelland
I think we're going to move to where we have separate config for clusters and machines, and also we're going to have provider status for both of them too. Would it be possible for this API to take a runtime.Object to decode into instead of returning a specific type?

Also, could EncodeToProviderConfig take a runtime.Object as well?

This probably doesn't need to be done in this PR.

Happy to take that up in a follow up, more targetted PR.

I can take care of this in my PR to create the separate config types. I was thinking it might be nicer to have 2 methods, that way they can return the correct type. Otherwise the caller has to cast the return value to the type they want, right? Or did you have something else in ming @krousey?

I was thinking of just having an output parameter of type runtime.Object. That way you could do something like

func (codec *GCEProviderConfigCodec) DecodeProviderConfigInto(providerConfig runtime.RawExtension, out runtime.Object) error {...} var blah GCEProviderConfig err := c.DecodeProviderConfigInto(m.Spec.ProviderConfig.Value, &blah)

krousey · 2018-05-15T17:54:41Z

cloud/google/machineactuator.go

@@ -719,6 +705,28 @@ func (gce *GCEClient) getImagePath(img string) (imagePath string) {
 	return defaultImg
 }

+func newDisks(config *gceconfigv1.GCEProviderConfig, zone string, imagePath string, minDiskSizeGb int64) []*compute.AttachedDisk {


This min size should eventually be a validation error

Yeah, min size was brought in recent time when we bumped up the min to 30 for custom images, and now for any image (as of a PR a few weeks ago). I was just trying to keep it as it was. I wonder if it should just be removed all together?

krousey · 2018-05-15T17:59:01Z

cloud/google/machineactuator_test.go

+	checkDiskValues(t, receivedInstance.Disks[1], false, 45, "pd-standard")
+}
+
+func checkInstanceValues(t *testing.T, instance *compute.Instance, diskCount int) {


Perhaps make use of https://golang.org/pkg/testing/#T.Helper

Nice, that is really useful.

This change adds the ability for GCP clusters to have custom sized disks and types.

spew · 2018-05-15T20:47:53Z

Updated post @krousey 's feedback. Noteably:

Reworked the "is boot disk" logic. Now only set the image and minimum size if it is the boot disk.
Updated the tests to ensure that the image is only set for the boot disk.
Started using the Helper() method on Testing.

krousey · 2018-05-16T23:29:03Z

/lgtm

…ker-build Add verify-docker-build to pre-submit tests

Verify manifests script

Clusterctl create cluster was not able to properly create the target cluster. It would sit in an infinite loop, attempting to clone the VM template. The fix was to not use annotations for task ref and vm ref during create. Instead, use provider status. Once this fix was in, the Target cluster would then sit in an infinite loop, attempting to clone the VM template. The fix was to check for nodeRef in the actuator's Exists() function. In addition, moved utils from provisioner/common to provisioner/govmomi as it's only ever used by the govmomi actuator. Fixes kubernetes-sigs#70

spew requested a review from krousey April 17, 2018 17:00

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 17, 2018

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 17, 2018

spew mentioned this pull request Apr 17, 2018

Allow disk size and type to be configured for GCP kubernetes-retired/kube-deploy#638

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 17, 2018

rsdcastro mentioned this pull request Apr 17, 2018

Allow disk size and type to be configured for GCP #84

Closed

spew force-pushed the kube-deploy-638 branch from 44cb101 to 5f6bd2e Compare April 17, 2018 21:31

spew force-pushed the kube-deploy-638 branch from 5f6bd2e to 0b6aaf3 Compare April 30, 2018 17:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 30, 2018

spew assigned krousey, jessicaochen and kcoronado Apr 30, 2018

spew force-pushed the kube-deploy-638 branch from 0b6aaf3 to a383c56 Compare April 30, 2018 17:44

jessicaochen suggested changes May 1, 2018

View reviewed changes

spew force-pushed the kube-deploy-638 branch from a383c56 to 9afe08f Compare May 9, 2018 20:35

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2018

spew force-pushed the kube-deploy-638 branch 2 times, most recently from aae9762 to 0b4e29e Compare May 9, 2018 22:28

jessicaochen approved these changes May 10, 2018

View reviewed changes

jessicaochen suggested changes May 10, 2018

View reviewed changes

spew force-pushed the kube-deploy-638 branch from 0b4e29e to 6349bd2 Compare May 10, 2018 19:53

jessicaochen approved these changes May 11, 2018

View reviewed changes

krousey suggested changes May 15, 2018

View reviewed changes

Add the ability to configure disk size and type for GCP clusters

30eb6d1

This change adds the ability for GCP clusters to have custom sized disks and types.

spew force-pushed the kube-deploy-638 branch from 6349bd2 to 30eb6d1 Compare May 15, 2018 20:48

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2018

krousey approved these changes May 16, 2018

View reviewed changes

spew merged commit a81a73f into kubernetes-sigs:master May 17, 2018

spew deleted the kube-deploy-638 branch May 17, 2018 03:45

mkjelland mentioned this pull request May 17, 2018

Split GCEProviderConfig GCEClusterProviderConfig and GCEMachineProviderConfig #183

Merged

chuckha pushed a commit to chuckha/cluster-api that referenced this pull request Oct 2, 2019

Merge pull request kubernetes-sigs#77 from ashish-amarnath/verify-doc…

13632bd

…ker-build Add verify-docker-build to pre-submit tests

chuckha pushed a commit to chuckha/cluster-api that referenced this pull request Oct 2, 2019

Merge pull request kubernetes-sigs#77 from amy/verify

0c38ba3

Verify manifests script

Add the ability to configure disk size and type for GCP clusters (Kube Deploy 638) #77

Add the ability to configure disk size and type for GCP clusters (Kube Deploy 638) #77

Conversation

spew commented Apr 17, 2018 • edited Loading

k8s-ci-robot commented Apr 17, 2018

krousey commented Apr 17, 2018

krousey commented Apr 17, 2018

spew commented Apr 30, 2018

spew commented Apr 30, 2018

spew commented Apr 30, 2018

k8s-ci-robot commented Apr 30, 2018

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

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

spew commented May 3, 2018

spew commented May 9, 2018

jessicaochen left a 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

jessicaochen left a comment

Choose a reason for hiding this comment

spew commented May 10, 2018

jessicaochen left a comment

Choose a reason for hiding this comment

spew commented May 15, 2018

krousey commented May 15, 2018

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

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

spew commented May 15, 2018

krousey commented May 16, 2018

spew commented Apr 17, 2018 •

edited

Loading