Add gcp local ssd disk support #414

tennix · 2020-02-17T10:27:18Z

What this PR does / why we need it:

Which issue(s) this PR fixes:
This PR enables user to add local ssd disks on GCP, the local ssd disk supports two interfaces: NVME and SCSI. See details in the below documents:

Special notes for your reviewer:

I've tested this on GCP and created 4 local NVME ssd disks. Note a GCE instance can only attach at most 8 local ssd disks. So there may need a validation for the total number of local ssd disks.

Release note:

Add GCP local ssd disk support

hardikdr · 2020-02-18T14:49:18Z

@tennix Thanks a lot for the PR, we'll take a look soon.

prashanth26

LGTM

prashanth26 · 2020-02-28T10:10:07Z

@hardikdr @ggaurav10 - Do you see any concerns here?

ggaurav10 · 2020-02-28T10:16:00Z

pkg/driver/driver_gcp.go

+		var attachedDisk compute.AttachedDisk
+		if disk.Type == "SCRATCH" {
+			attachedDisk = compute.AttachedDisk{
+				AutoDelete: disk.AutoDelete,


This default to false, right?
@hardikdr @prashanth26 - Wouldn't it lead to similar issues that we recently saw of orphan disks when a VM is deleted.

Good catch. What would be the best way to handle this? Default the values to true on MCM or Gardener maybe?

IMO, defaulting should be done in MCM, keeping it mind that it can be used independently of Gardener

@tennix - Can we default this flag to true in the MCM?

I don't think it's related to this. Unlike to persistent disks, the local ssd disks will be deleted once the instance is deleted. But I can change the default value to true if you like.

I'd actually prefer to delete these disks by default with the VM, unless specified in machinieclass. Considering the fact that we constantly create/delete the machines with autoscaler, we may end up with many such orphan disks, and we don't have active automation to keep track of these disks later.

According to the documentation the local-ssds are expected to be deleted with the VM termination. - https://cloud.google.com/compute/docs/disks#localssds .

@tennix please correct if that's not the case.

Though, to save us from the possible changes at gcp's behaviour, I would prefer to set it to true.

The issue actually is, the API type for AutoDelete is defined as bool and leaves no good way to check if it's set at MachineClass or not.

@tennix Please feel free to change the type to *bool from bool, that'll help you in checking AutoDelete==nil(implies not set at MachineClass), and default to true.

Okay, I've changed GCPDisk AutoDelete type to *bool and set AttachDisk.AutoDelete default value to true if users does not specify the field. @hardikdr @prashanth26 @ggaurav10 PTAL again.

ggaurav10 · 2020-02-28T10:16:13Z

pkg/driver/driver_gcp.go

+			}
+		} else {
+			attachedDisk = compute.AttachedDisk{
+				AutoDelete: disk.AutoDelete,


same as above

@tennix - Can we default this flag to true in the MCM?

For persistent disks, is it better to default to false so if the VM is deleted there is still a chance to recover the data in persistent disk especially for production environment?

@tennix - Can we default this flag to true in the MCM?

Actually, at the moment, Gardener sets the AutoDelete to true for all the GCPMachineClasses, hence the current codepath should not impact us immediately.

For persistent disks, is it better to default to false so if the VM is deleted there is still a chance to recover the data in persistent disk especially for production environment?

I would actually suggest to set the AutoDelete=true even for the persistent disks, and let the user change the behaviour if needed via MachineClass.

The change to the api type to *bool would be useful here as well.

prashanth26 · 2020-03-03T06:31:45Z

pkg/driver/driver_gcp.go

+		var attachedDisk compute.AttachedDisk
+		if disk.Type == "SCRATCH" {
+			attachedDisk = compute.AttachedDisk{
+				AutoDelete: disk.AutoDelete,


@tennix - Can we default this flag to true in the MCM?

prashanth26 · 2020-03-03T06:31:51Z

pkg/driver/driver_gcp.go

+			}
+		} else {
+			attachedDisk = compute.AttachedDisk{
+				AutoDelete: disk.AutoDelete,


@tennix - Can we default this flag to true in the MCM?

CLAassistant · 2020-04-07T09:13:04Z

All committers have signed the CLA.

hardikdr

Overall, thanks a lot @tennix for the PR, and sorry for the delay in reviewing. We got held up in a few other immediate tasks.

There are few minor comments related to defaulting the disk-deletion with VM.
As it seems, local-ssds are deleted with the VM-termination[needs your confirmation] and this PR doesn't alter the behavior for the persistent-disks[defaults to false in master],

I'll leave the decision to you if you want to change the overall defaulting of both local-ssds and persistent-disks to true - to be deleted with VM-deletion.

Also it would be very nice if you could add an example here, as a second disk with local-ssd type.

hardikdr · 2020-04-07T11:04:17Z

pkg/driver/driver_gcp.go

+		var attachedDisk compute.AttachedDisk
+		if disk.Type == "SCRATCH" {
+			attachedDisk = compute.AttachedDisk{
+				AutoDelete: disk.AutoDelete,


According to the documentation the local-ssds are expected to be deleted with the VM termination. - https://cloud.google.com/compute/docs/disks#localssds .

@tennix please correct if that's not the case.

Though, to save us from the possible changes at gcp's behaviour, I would prefer to set it to true.

The issue actually is, the API type for AutoDelete is defined as bool and leaves no good way to check if it's set at MachineClass or not.

@tennix Please feel free to change the type to *bool from bool, that'll help you in checking AutoDelete==nil(implies not set at MachineClass), and default to true.

hardikdr · 2020-04-07T11:30:17Z

pkg/driver/driver_gcp.go

+			}
+		} else {
+			attachedDisk = compute.AttachedDisk{
+				AutoDelete: disk.AutoDelete,


@tennix - Can we default this flag to true in the MCM?

Actually, at the moment, Gardener sets the AutoDelete to true for all the GCPMachineClasses, hence the current codepath should not impact us immediately.

For persistent disks, is it better to default to false so if the VM is deleted there is still a chance to recover the data in persistent disk especially for production environment?

I would actually suggest to set the AutoDelete=true even for the persistent disks, and let the user change the behaviour if needed via MachineClass.

The change to the api type to *bool would be useful here as well.

hardikdr · 2020-04-09T18:19:00Z

@prashanth @ggaurav10 can you please take a quick look and approve if fine with it?

prashanth26

One small change. Otherwise looks fine to me.

pkg/apis/machine/v1alpha1/types.go

prashanth26

LGTM.

pkg/apis/machine/v1alpha1/types.go

add gcp local ssd disk support

a0ac494

tennix requested review from ggaurav10 and a team as code owners February 17, 2020 10:27

prashanth26 approved these changes Feb 18, 2020

View reviewed changes

prashanth26 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 18, 2020

gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 18, 2020

prashanth26 added platform/gcp Google cloud platform/infrastructure reviewed/lgtm Has approval for merging needs/review Needs review and removed reviewed/lgtm Has approval for merging labels Feb 28, 2020

ggaurav10 reviewed Feb 28, 2020

View reviewed changes

prashanth26 suggested changes Mar 3, 2020

View reviewed changes

prashanth26 added needs/changes Needs (more) changes and removed needs/review Needs review labels Mar 3, 2020

hardikdr reviewed Apr 7, 2020

View reviewed changes

set gcp disk AutoDelete default value to true

1129925

hardikdr added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 9, 2020

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 9, 2020

tennix requested review from prashanth26, ggaurav10 and hardikdr April 9, 2020 09:46

hardikdr approved these changes Apr 9, 2020

View reviewed changes

prashanth26 reviewed Apr 10, 2020

View reviewed changes

pkg/apis/machine/v1alpha1/types.go Show resolved Hide resolved

address comment

5caf330

prashanth26 approved these changes Apr 14, 2020

View reviewed changes

pkg/apis/machine/v1alpha1/types.go Show resolved Hide resolved

hardikdr added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 14, 2020

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 14, 2020

ggaurav10 approved these changes Apr 14, 2020

View reviewed changes

hardikdr merged commit ec6f466 into gardener:master Apr 14, 2020

tennix deleted the gcp-local-ssd branch April 14, 2020 06:09

This was referenced Apr 16, 2020

Upgrade machine-controller-manager #446

Closed

Add local ssd disk interface support gardener/gardener#2206

Closed

Add local ssd disk interface support gardener/gardener-extension-provider-gcp#71

Merged

prashanth26 mentioned this pull request Aug 13, 2020

Additional (+ encrypted) ephemeral worker/node disks #338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gcp local ssd disk support #414

Add gcp local ssd disk support #414

tennix commented Feb 17, 2020 •

edited

Loading

hardikdr commented Feb 18, 2020

prashanth26 left a comment

prashanth26 commented Feb 28, 2020

ggaurav10 Feb 28, 2020

prashanth26 Feb 28, 2020

ggaurav10 Feb 28, 2020

prashanth26 Mar 3, 2020

tennix Mar 4, 2020

hardikdr Mar 13, 2020

hardikdr Apr 7, 2020

tennix Apr 9, 2020 •

edited

Loading

ggaurav10 Feb 28, 2020

prashanth26 Mar 3, 2020

tennix Mar 4, 2020

hardikdr Apr 7, 2020

prashanth26 Mar 3, 2020

prashanth26 Mar 3, 2020

CLAassistant commented Apr 7, 2020

hardikdr left a comment

hardikdr Apr 7, 2020

hardikdr Apr 7, 2020

hardikdr commented Apr 9, 2020

prashanth26 left a comment

prashanth26 left a comment

Add gcp local ssd disk support #414

Add gcp local ssd disk support #414

Conversation

tennix commented Feb 17, 2020 • edited Loading

hardikdr commented Feb 18, 2020

prashanth26 left a comment

Choose a reason for hiding this comment

prashanth26 commented Feb 28, 2020

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

tennix Apr 9, 2020 • edited Loading

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

CLAassistant commented Apr 7, 2020

hardikdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardikdr commented Apr 9, 2020

prashanth26 left a comment

Choose a reason for hiding this comment

prashanth26 left a comment

Choose a reason for hiding this comment

tennix commented Feb 17, 2020 •

edited

Loading

tennix Apr 9, 2020 •

edited

Loading