EBS Root Volume Termination

[tioxy]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Enables to change EBS DeleteOnTermination by specifiying separated flags for Root Volume and Additional Volumes inside InstanceGroupSpec.

Which issue(s) this PR fixes:
Fixes #6373

Special notes for your reviewer:

Further discussion about UX changed field names. Previous source code is here

We found better to split deleteOnTermination into different fields, as it enabled a deeper customization of InstanceGroups (originally was suggested only a single field in the structure).

Now we can finally close the 1.15 Milestone 😄
Also, a huge thanks to @joshbranham's work

Does this PR introduce a user-facing change?:

Add capacity to retain EBS volumes after instance termination from InstanceGroups

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

---
kind: InstanceGroup
spec:
  rootVolumeDeleteOnTermination: false

---
kind: InstanceGroup
spec:
  volumes:
  - device: /dev/xvdd
    deleteOnTermination: false

[k8s-ci-robot]

Hi @tioxy. Thanks for your PR.

I'm waiting for a kubernetes 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[tioxy]

/assign @geojaz

[rifelpet]

/ok-to-test

Can you add a test for this? Perhaps just adding the field in this manifest and updating the terraform and cloudformation files in the same directory to match.

[joshbranham]

@tioxy have you tried building an instance group with and without this setting? I had some difficulty with an existing group and the kops update cluster diff showing the change to false, but maybe you handle that more elegantly than I had!

[tioxy]

Hi @joshbranham 😀

One of the changes that probably affected your attempt was around awstasks/launchconfiguration.go with the method Find(). It will probably output mislead information if you do not change the actual Launch Configuration struct field.

    actual.RootVolumeTermination = b.Ebs.DeleteOnTermination

I made a step-by-step below running kops updates against different instance group volume settings as you suggested 👍

I cleaned the ouput as much as I could to make it more clear

These are the manifests that I used to create a cluster with different volume settings:

 kind: InstanceGroup
 metadata:
   name: master-us-east-1a
 spec:
   role: Master
   rootVolumeRetainOnTermination: true     # retain root
   volumes:
   - device: /dev/xvdd
     encrypted: true
     size: 20
     type: gp2

 kind: InstanceGroup
 metadata:
   name: nodes
 spec:
   role: Node
   volumes:
   - device: /dev/xvdd
     encrypted: true
     retainOnTermination: true     # retain xvdd
     size: 20
     type: gp2

Then I ran kops update cluster (1st output):

  LaunchConfiguration/master-us-east-1a.masters.kops.tioxy.com
  	BlockDeviceMappings 	[{"DeviceName":"/dev/xvdd","EbsDeleteOnTermination":true,"EbsEncrypted":true,"EbsVolumeIops":null,"EbsVolumeSize":20,"EbsVolumeType":"gp2","VirtualName":null}]
  	RootVolumeSize      	64
        // retain root
  	RootVolumeTermination	false
  	RootVolumeType      	gp2	

  LaunchConfiguration/nodes.kops.tioxy.com
        // retain xvdd
  	BlockDeviceMappings 	[{"DeviceName":"/dev/xvdd","EbsDeleteOnTermination":false,"EbsEncrypted":true,"EbsVolumeIops":null,"EbsVolumeSize":20,"EbsVolumeType":"gp2","VirtualName":null}]
  	RootVolumeSize      	128
  	RootVolumeTermination	true
  	RootVolumeType      	gp2      	

I edited the nodes InstanceGroup to add rootVolumeRetainOnTermination: true:

 kind: InstanceGroup
 metadata:
   name: nodes
 spec:
   role: Node
   rootVolumeRetainOnTermination: true     # retaining root now
   volumes:
   - device: /dev/xvdd
     encrypted: true
     retainOnTermination: true
     size: 20
     type: gp2

And this the result after kops update cluster (2nd output):

Will modify resources:
  LaunchConfiguration/nodes.kops.tioxy.com
        // retaining root now
  	RootVolumeTermination	 true -> false

Must specify --yes to apply changes

[tioxy]

@rifelpet Even after kops delete cluster, the retained volumes will be available in the AWS account (which I think is the intended behavior).

Should we be worried about these dangling volumes during e2e tests (costs and unused resources)?

[tioxy]

Fixed tests 👍
Needed to add new fields to in-legacy-v1alpha2.yaml as well

[tioxy]

/test pull-kops-e2e-kubernetes-aws

[tioxy]

/assign @rifelpet

[geojaz]

@tioxy I think this looks good, and I'm trying to give it a test over here for you, but I noticed that I'm getting a failure on verify-goimports (which make ci runs). I'll take a look at the rest of it, but could you take a look at that one pls?

[joshbranham]

I would agree that using the following nomenclature makes the most sense: deleteOnTermination, with rootVolume in front etc as you have described, thanks for continuing to work on this as we deliberate :)

[joshbranham]

This all looks good to me, thanks @tioxy !

/lgtm

[rifelpet]

I agree it looks good! Can we mention the default value of true in the API field comments so that it shows up in the API documentation?

And I noticed this will only apply to LaunchConfigurations and not LaunchTemplates:

kops/upup/pkg/fi/cloudup/awstasks/launchtemplate.go

Lines 103 to 108 in e1f7d31

	bm[aws.StringValue(img.RootDeviceName)] = &BlockDeviceMapping{
	EbsDeleteOnTermination: aws.Bool(true),
	EbsVolumeSize: t.RootVolumeSize,
	EbsVolumeType: t.RootVolumeType,
	EbsVolumeIops: t.RootVolumeIops,
	}

I think we can address that in a followup PR but it might be worth mentioning in the API field comments that it currently only applies to LaunchConfigurations.

[tioxy]

@rifelpet Pushed the default/launch configuration api docs with new codegen.
Added launch configuration docs in a separate line to be easily removed when Launch Template is implemented.

Thank you all for such great reviews 👍

[rifelpet]

One last concern came to mind: if volumes are not deleted by the ASG, does a kops delete cluster delete them? We should determine whether it does or not and clearly document that as well. I could see either behavior being desired, so it's important to make sure users are aware of whichever one it is.

[geojaz]

@tioxy this looks great. thanks taking our feedback and running with it. also, you probably have some of the best github change comments I've seen! 🙌

I had some suggestions on the wording that i think make this more digestable for we humans. I didn't note every single change because it's basically all the same :)

[tioxy]

@geojaz Updated API field docs with your suggestion, way more human readable and straightforward 😅

@rifelpet Volumes are retained after cluster deletion. Just added this to API field docs as well.

I will start to work in the Launch Template implementation in a few days 😉

[geojaz]

/lgtm
/approve

nice work @tioxy !

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geojaz, tioxy

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 [geojaz]

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