Repository: keikoproj/instance-manager
Pull request #233 (2021-02-03 02:19:45)

feat: add support for new volume types
Signed-off-by: Jonah Back <jonah@jonahback.com>

codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/233?src=pr&el=h1) Report
> Merging [#233](https://codecov.io/gh/keikoproj/instance-manager/pull/233?src=pr&el=desc) (4f6977e) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/d62a07803de7dec069e42bf01ea659a5bb8e949a?el=desc) (d62a078) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/keikoproj/instance-manager/pull/233/graphs/tree.svg?width=650&height=150&src=pr&token=IJbjmSBliL)](https://codecov.io/gh/keikoproj/instance-manager/pull/233?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #233   +/-   ##
=======================================
  Coverage   87.90%   87.90%           
=======================================
  Files          17       17           
  Lines        2207     2207           
=======================================
  Hits         1940     1940           
  Misses        162      162           
  Partials      105      105           
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/233?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/233?src=pr&el=footer). Last update [d62a078...dd64886](https://codecov.io/gh/keikoproj/instance-manager/pull/233?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

eytan-avisror: LG, added some minor suggestion to refactor.
Assume you verified manually this works? 🥇 
backjo: Yep, verified this works on our EKS clusters. One caveat that might be applicable for some folks is that the LaunchConfiguration APIs don't support the new volume types - only the LaunchTemplate APIs
eytan-avisror: > Yep, verified this works on our EKS clusters. One caveat that might be applicable for some folks is that the LaunchConfiguration APIs don't support the new volume types - only the LaunchTemplate APIs

In that case, should we add another validation above the provisioned IOPS one to validate if user is trying to use invalid types with template/config?

We can add `TemplateAllowedVolumeTypes` & `ConfigurationAllowedVolumesTypes`
You can use predicates `spec.IsLaunchTemplate()` or `spec.IsLaunchConfiguration()` to check what the CR is declaring.

backjo: Added validation based on ScalingConfigurationType

Patch:
diff --git a/api/v1alpha1/instancegroup_types.go b/api/v1alpha1/instancegroup_types.go
index 5584e77..8478791 100644
--- a/api/v1alpha1/instancegroup_types.go
+++ b/api/v1alpha1/instancegroup_types.go
@@ -410,7 +410,7 @@ func (s *EKSSpec) IsLaunchConfiguration() bool {
 	return false
 }
 
-func (c *EKSConfiguration) Validate() error {
+func (c *EKSConfiguration) Validate(scalingConfigurationType ScalingConfigurationType) error {
 	if common.StringEmpty(c.EksClusterName) {
 		return errors.Errorf("validation failed, 'clusterName' is a required parameter")
 	}
@@ -487,12 +487,16 @@ func (c *EKSConfiguration) Validate() error {
 	}
 
 	for _, v := range c.Volumes {
-		if !common.ContainsEqualFold(awsprovider.AllowedVolumeTypes, v.Type) {
-			return errors.Errorf("validation failed, volume type '%v' is unsuppoeted", v.Type)
+		if scalingConfigurationType == LaunchConfiguration && !common.ContainsEqualFold(awsprovider.ConfigurationAllowedVolumeTypes, v.Type) {
+			return errors.Errorf("validation failed, volume type '%v' is unsupported", v.Type)
 		}
 
-		if v.Iops != 0 && !strings.EqualFold(v.Type, "io1") {
-			log.Info("cannot apply IOPS configuration for volumeType, only type 'io1' supported", "volumeType", v.Type)
+		if scalingConfigurationType == LaunchTemplate && !common.ContainsEqualFold(awsprovider.TemplateAllowedVolumeTypes, v.Type) {
+			return errors.Errorf("validation failed, volume type '%v' is unsupported", v.Type)
+		}
+
+		if v.Iops != 0 && !common.ContainsEqualFold(awsprovider.AllowedVolumeTypesWithProvisionedIOPS, v.Type) {
+			log.Info("cannot apply IOPS configuration for volumeType, only types ['io1','io2','gp3'] supported", "volumeType", v.Type)
 		}
 
 		if v.SnapshotID != "" {
@@ -626,7 +630,7 @@ func (ig *InstanceGroup) Validate() error {
 			return err
 		}
 
-		if err := config.Validate(); err != nil {
+		if err := config.Validate(spec.Type); err != nil {
 			return err
 		}
 	}
diff --git a/controllers/providers/aws/aws.go b/controllers/providers/aws/aws.go
index 2b1d1ad..703200c 100644
--- a/controllers/providers/aws/aws.go
+++ b/controllers/providers/aws/aws.go
@@ -105,9 +105,11 @@ var (
 		"GroupTotalCapacity",
 	}
 
-	AllowedVolumeTypes               = []string{"gp2", "io1", "sc1", "st1"}
-	LifecycleHookTransitionLaunch    = "autoscaling:EC2_INSTANCE_LAUNCHING"
-	LifecycleHookTransitionTerminate = "autoscaling:EC2_INSTANCE_TERMINATING"
+	ConfigurationAllowedVolumeTypes       = []string{"gp2", "io1", "sc1", "st1"}
+	TemplateAllowedVolumeTypes            = []string{"gp2", "gp3", "io1", "io2", "sc1", "st1"}
+	AllowedVolumeTypesWithProvisionedIOPS = []string{"io1", "io2", "gp3"}
+	LifecycleHookTransitionLaunch         = "autoscaling:EC2_INSTANCE_LAUNCHING"
+	LifecycleHookTransitionTerminate      = "autoscaling:EC2_INSTANCE_TERMINATING"
 )
 
 const (


Test patch:
diff --git a/api/v1alpha1/instancegroup_types_test.go b/api/v1alpha1/instancegroup_types_test.go
index e86bac5..c3188d1 100644
--- a/api/v1alpha1/instancegroup_types_test.go
+++ b/api/v1alpha1/instancegroup_types_test.go
@@ -201,6 +201,36 @@ func TestInstanceGroupSpecValidate(t *testing.T) {
 			},
 			want: "validation failed, Tenancy must be one of default, dedicated, host",
 		},
+		{
+			name: "eks with gp3 volume validates",
+			args: args{
+				instancegroup: MockInstanceGroup("eks", "rollingUpdate", &EKSSpec{
+					MaxSize: 1,
+					MinSize: 1,
+					Type:    "LaunchTemplate",
+					EKSConfiguration: &EKSConfiguration{
+						EksClusterName:     "my-eks-cluster",
+						NodeSecurityGroups: []string{"sg-123456789"},
+						Image:              "ami-12345",
+						InstanceType:       "m5.large",
+						KeyPairName:        "thisShouldBeOptional",
+						Subnets:            []string{"subnet-1111111", "subnet-222222"},
+						Placement: &PlacementSpec{
+							AvailabilityZone:     "us-west-2a",
+							HostResourceGroupArn: "arn:aws:resource-groups:us-west-2:1122334455:group/resourceName",
+							Tenancy:              "host",
+						},
+						Volumes: []NodeVolume{
+							{
+								Type: "gp3",
+								Iops: 230,
+							},
+						},
+					},
+				}, nil, nil),
+			},
+			want: "",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {