From 3404c93e4ed0114a6cef2d5dfb71fdbe2d99bdaa Mon Sep 17 00:00:00 2001 From: Venkata Gunapati Date: Fri, 31 Mar 2023 16:50:44 -0700 Subject: [PATCH 1/2] Fix: Not honoring gp3 volume iops and throughput values Signed-off-by: Venkata Gunapati --- api/v1alpha1/instancegroup_types_test.go | 3 +- ...stancemgr.keikoproj.io_instancegroups.yaml | 31 +++++++++++++------ controllers/providers/aws/constructors.go | 18 +++++++---- controllers/provisioners/eks/eks.go | 8 ++--- controllers/provisioners/eks/helpers_test.go | 2 +- .../provisioners/eks/scaling/launchconfig.go | 2 +- .../eks/scaling/launchtemplate.go | 2 +- controllers/provisioners/eks/update_test.go | 13 ++++---- go.mod | 2 +- go.sum | 4 +-- 10 files changed, 52 insertions(+), 33 deletions(-) diff --git a/api/v1alpha1/instancegroup_types_test.go b/api/v1alpha1/instancegroup_types_test.go index 7581aec1..6b6e1be2 100644 --- a/api/v1alpha1/instancegroup_types_test.go +++ b/api/v1alpha1/instancegroup_types_test.go @@ -1,10 +1,9 @@ /* - Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at - http://www.apache.org/licenses/LICENSE-2.0 + http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, diff --git a/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml b/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml index 56a02cea..55249530 100644 --- a/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml +++ b/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml @@ -57,10 +57,14 @@ spec: description: InstanceGroup is the Schema for the instancegroups API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' type: string kind: - description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' type: string metadata: type: object @@ -203,20 +207,27 @@ spec: type: array taints: items: - description: The node this Taint is attached to has the "effect" on any pod that does not tolerate the Taint. + description: The node this Taint is attached to has the + "effect" on any pod that does not tolerate the Taint. properties: effect: - description: Required. The effect of the taint on pods that do not tolerate the taint. Valid effects are NoSchedule, PreferNoSchedule and NoExecute. + description: Required. The effect of the taint on pods + that do not tolerate the taint. Valid effects are + NoSchedule, PreferNoSchedule and NoExecute. type: string key: - description: Required. The taint key to be applied to a node. + description: Required. The taint key to be applied to + a node. type: string timeAdded: - description: TimeAdded represents the time at which the taint was added. It is only written for NoExecute taints. + description: TimeAdded represents the time at which + the taint was added. It is only written for NoExecute + taints. format: date-time type: string value: - description: The taint value corresponding to the taint key. + description: The taint value corresponding to the taint + key. type: string required: - effect @@ -382,7 +393,8 @@ spec: provisioner: type: string strategy: - description: AwsUpgradeStrategy defines the upgrade strategy of an AWS Instance Group + description: AwsUpgradeStrategy defines the upgrade strategy of an + AWS Instance Group properties: crd: properties: @@ -424,7 +436,8 @@ spec: type: string conditions: items: - description: InstanceGroupConditions describes the conditions of the InstanceGroup + description: InstanceGroupConditions describes the conditions of + the InstanceGroup properties: status: type: string diff --git a/controllers/providers/aws/constructors.go b/controllers/providers/aws/constructors.go index ccc56780..efa719bd 100644 --- a/controllers/providers/aws/constructors.go +++ b/controllers/providers/aws/constructors.go @@ -16,7 +16,7 @@ limitations under the License. package aws import ( - "strings" + "fmt" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/autoscaling" @@ -25,7 +25,7 @@ import ( "github.com/keikoproj/instance-manager/controllers/common" ) -func (w *AwsWorker) GetAutoScalingBasicBlockDevice(name, volType, snapshot string, volSize, iops int64, delete, encrypt *bool) *autoscaling.BlockDeviceMapping { +func (w *AwsWorker) GetAutoScalingBasicBlockDevice(name, volType, snapshot string, volSize, iops int64, throughput int64, delete, encrypt *bool) *autoscaling.BlockDeviceMapping { device := &autoscaling.BlockDeviceMapping{ DeviceName: aws.String(name), Ebs: &autoscaling.Ebs{ @@ -40,20 +40,23 @@ func (w *AwsWorker) GetAutoScalingBasicBlockDevice(name, volType, snapshot strin if encrypt != nil { device.Ebs.Encrypted = encrypt } - if iops != 0 && strings.EqualFold(volType, "io1") { + if iops != 0 && common.ContainsEqualFold(AllowedVolumeTypesWithProvisionedIOPS, volType) { device.Ebs.Iops = aws.Int64(iops) } + if throughput != 0 && common.ContainsEqualFold(AllowedVolumeTypesWithProvisionedThroughput, volType) { + device.Ebs.Throughput = aws.Int64(throughput) + } if volSize != 0 { device.Ebs.VolumeSize = aws.Int64(volSize) } if !common.StringEmpty(snapshot) { device.Ebs.SnapshotId = aws.String(snapshot) } - + fmt.Printf("%+v\n", device) return device } -func (w *AwsWorker) GetLaunchTemplateBlockDeviceRequest(name, volType, snapshot string, volSize, iops int64, delete, encrypt *bool) *ec2.LaunchTemplateBlockDeviceMappingRequest { +func (w *AwsWorker) GetLaunchTemplateBlockDeviceRequest(name, volType, snapshot string, volSize, iops int64, throughput int64, delete, encrypt *bool) *ec2.LaunchTemplateBlockDeviceMappingRequest { device := &ec2.LaunchTemplateBlockDeviceMappingRequest{ DeviceName: aws.String(name), Ebs: &ec2.LaunchTemplateEbsBlockDeviceRequest{ @@ -68,9 +71,12 @@ func (w *AwsWorker) GetLaunchTemplateBlockDeviceRequest(name, volType, snapshot if encrypt != nil { device.Ebs.Encrypted = encrypt } - if iops != 0 && strings.EqualFold(volType, "io1") { + if iops != 0 && common.ContainsEqualFold(AllowedVolumeTypesWithProvisionedIOPS, volType) { device.Ebs.Iops = aws.Int64(iops) } + if throughput != 0 && common.ContainsEqualFold(AllowedVolumeTypesWithProvisionedThroughput, volType) { + device.Ebs.Throughput = aws.Int64(throughput) + } if volSize != 0 { device.Ebs.VolumeSize = aws.Int64(volSize) } diff --git a/controllers/provisioners/eks/eks.go b/controllers/provisioners/eks/eks.go index 7e20cbf6..0bfe7521 100644 --- a/controllers/provisioners/eks/eks.go +++ b/controllers/provisioners/eks/eks.go @@ -54,10 +54,10 @@ var ( InstanceMgrLifecycleLabel = "instancemgr.keikoproj.io/lifecycle" InstanceMgrImageLabel = "instancemgr.keikoproj.io/image" - AllowedOsFamilies = []string{OsFamilyWindows, OsFamilyBottleRocket, OsFamilyAmazonLinux2} - DefaultManagedPolicies = []string{"AmazonEKSWorkerNodePolicy", "AmazonEC2ContainerRegistryReadOnly"} - CNIManagedPolicy = "AmazonEKS_CNI_Policy" - SupportedArchitectures = []string{"x86_64", "arm64"} + AllowedOsFamilies = []string{OsFamilyWindows, OsFamilyBottleRocket, OsFamilyAmazonLinux2} + DefaultManagedPolicies = []string{"AmazonEKSWorkerNodePolicy", "AmazonEC2ContainerRegistryReadOnly"} + CNIManagedPolicy = "AmazonEKS_CNI_Policy" + SupportedArchitectures = []string{"x86_64", "arm64"} ) // New constructs a new instance group provisioner of EKS type diff --git a/controllers/provisioners/eks/helpers_test.go b/controllers/provisioners/eks/helpers_test.go index 1e4b394a..90af0b25 100644 --- a/controllers/provisioners/eks/helpers_test.go +++ b/controllers/provisioners/eks/helpers_test.go @@ -189,7 +189,7 @@ func TestGetBasicUserDataWindows(t *testing.T) { ctx := MockContext(ig, k, w) configuration.BootstrapOptions = &v1alpha1.BootstrapOptions{ - MaxPods: 4, + MaxPods: 4, ContainerRuntime: "containerd", } configuration.Labels = map[string]string{ diff --git a/controllers/provisioners/eks/scaling/launchconfig.go b/controllers/provisioners/eks/scaling/launchconfig.go index 186348ff..7b4df504 100644 --- a/controllers/provisioners/eks/scaling/launchconfig.go +++ b/controllers/provisioners/eks/scaling/launchconfig.go @@ -288,7 +288,7 @@ func (lc *LaunchConfiguration) placementTenancy(placement *v1alpha1.PlacementSpe func (lc *LaunchConfiguration) blockDeviceList(volumes []v1alpha1.NodeVolume) []*autoscaling.BlockDeviceMapping { var devices []*autoscaling.BlockDeviceMapping for _, v := range volumes { - devices = append(devices, lc.GetAutoScalingBasicBlockDevice(v.Name, v.Type, v.SnapshotID, v.Size, v.Iops, v.DeleteOnTermination, v.Encrypted)) + devices = append(devices, lc.GetAutoScalingBasicBlockDevice(v.Name, v.Type, v.SnapshotID, v.Size, v.Iops, v.Throughput, v.DeleteOnTermination, v.Encrypted)) } return sortConfigDevices(devices) } diff --git a/controllers/provisioners/eks/scaling/launchtemplate.go b/controllers/provisioners/eks/scaling/launchtemplate.go index fd5c40de..ac52a66d 100644 --- a/controllers/provisioners/eks/scaling/launchtemplate.go +++ b/controllers/provisioners/eks/scaling/launchtemplate.go @@ -334,7 +334,7 @@ func (lt *LaunchTemplate) RotationNeeded(input *DiscoverConfigurationInput) bool func (lt *LaunchTemplate) blockDeviceListRequest(volumes []v1alpha1.NodeVolume) []*ec2.LaunchTemplateBlockDeviceMappingRequest { var devices []*ec2.LaunchTemplateBlockDeviceMappingRequest for _, v := range volumes { - devices = append(devices, lt.GetLaunchTemplateBlockDeviceRequest(v.Name, v.Type, v.SnapshotID, v.Size, v.Iops, v.DeleteOnTermination, v.Encrypted)) + devices = append(devices, lt.GetLaunchTemplateBlockDeviceRequest(v.Name, v.Type, v.SnapshotID, v.Size, v.Iops, v.Throughput, v.DeleteOnTermination, v.Encrypted)) } return devices diff --git a/controllers/provisioners/eks/update_test.go b/controllers/provisioners/eks/update_test.go index 650f44f3..c930cb75 100644 --- a/controllers/provisioners/eks/update_test.go +++ b/controllers/provisioners/eks/update_test.go @@ -408,7 +408,7 @@ func TestLaunchConfigurationDrifted(t *testing.T) { SecurityGroups: aws.StringSlice([]string{"sg-1", "sg-2"}), KeyName: aws.String("somekey"), UserData: aws.String("userdata"), - BlockDeviceMappings: []*autoscaling.BlockDeviceMapping{w.GetAutoScalingBasicBlockDevice("/dev/xvda", "gp2", "", 40, 100, nil, nil)}, + BlockDeviceMappings: []*autoscaling.BlockDeviceMapping{w.GetAutoScalingBasicBlockDevice("/dev/xvda", "gp2", "", 40, 100, 200, nil, nil)}, } existingConfig := &scaling.CreateConfigurationInput{ @@ -422,10 +422,11 @@ func TestLaunchConfigurationDrifted(t *testing.T) { UserData: "userdata", Volumes: []v1alpha1.NodeVolume{ { - Name: "/dev/xvda", - Type: "gp2", - Size: 40, - Iops: 100, + Name: "/dev/xvda", + Type: "gp2", + Size: 40, + Iops: 100, + Throughput: 200, }, }, } @@ -448,7 +449,7 @@ func TestLaunchConfigurationDrifted(t *testing.T) { keyDrift.KeyName = aws.String("some-key") usrDrift.UserData = aws.String("some-userdata") devDrift.BlockDeviceMappings = []*autoscaling.BlockDeviceMapping{ - w.GetAutoScalingBasicBlockDevice("some-device", "some-type", "", 32, 0, nil, nil), + w.GetAutoScalingBasicBlockDevice("some-device", "some-type", "", 32, 0, 0, nil, nil), } tests := []struct { diff --git a/go.mod b/go.mod index e9bad204..f323b79e 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.17 require ( github.com/Masterminds/semver v1.5.0 - github.com/aws/aws-sdk-go v1.38.24 + github.com/aws/aws-sdk-go v1.38.71 github.com/cucumber/godog v0.8.1 github.com/evanphx/json-patch v4.11.0+incompatible github.com/ghodss/yaml v1.0.0 diff --git a/go.sum b/go.sum index f1e8cb3c..728e0298 100644 --- a/go.sum +++ b/go.sum @@ -51,8 +51,8 @@ github.com/armon/go-metrics v0.0.0-20180917152333-f0300d1749da/go.mod h1:Q73ZrmV github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8= github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY= github.com/aws/aws-sdk-go v1.35.7/go.mod h1:tlPOdRjfxPBpNIwqDj61rmsnA85v9jc0Ps9+muhnW+k= -github.com/aws/aws-sdk-go v1.38.24 h1:zbKHDxFepE77ihVMZ+wZ62Ci646zkorN8rB5s4fj4kU= -github.com/aws/aws-sdk-go v1.38.24/go.mod h1:hcU610XS61/+aQV88ixoOzUoG7v3b31pl2zKMmprdro= +github.com/aws/aws-sdk-go v1.38.71 h1:aWhtgoOiDhBCfaAj9XbxzcyvjEAKovbtv7d5mCVBZXw= +github.com/aws/aws-sdk-go v1.38.71/go.mod h1:hcU610XS61/+aQV88ixoOzUoG7v3b31pl2zKMmprdro= github.com/benbjohnson/clock v1.1.0 h1:Q92kusRqC1XV2MjkWETPvjJVqKetz1OzxZB7mHJLju8= github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA= github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q= From 4dde850b115f51fc1702353f37ae5eb3fbaf357b Mon Sep 17 00:00:00 2001 From: Venkata Gunapati Date: Fri, 31 Mar 2023 16:57:22 -0700 Subject: [PATCH 2/2] clean up Signed-off-by: Venkata Gunapati --- ...stancemgr.keikoproj.io_instancegroups.yaml | 31 ++++++------------- controllers/providers/aws/constructors.go | 3 -- 2 files changed, 9 insertions(+), 25 deletions(-) diff --git a/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml b/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml index 55249530..56a02cea 100644 --- a/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml +++ b/config/crd/bases/instancemgr.keikoproj.io_instancegroups.yaml @@ -57,14 +57,10 @@ spec: description: InstanceGroup is the Schema for the instancegroups API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' type: string metadata: type: object @@ -207,27 +203,20 @@ spec: type: array taints: items: - description: The node this Taint is attached to has the - "effect" on any pod that does not tolerate the Taint. + description: The node this Taint is attached to has the "effect" on any pod that does not tolerate the Taint. properties: effect: - description: Required. The effect of the taint on pods - that do not tolerate the taint. Valid effects are - NoSchedule, PreferNoSchedule and NoExecute. + description: Required. The effect of the taint on pods that do not tolerate the taint. Valid effects are NoSchedule, PreferNoSchedule and NoExecute. type: string key: - description: Required. The taint key to be applied to - a node. + description: Required. The taint key to be applied to a node. type: string timeAdded: - description: TimeAdded represents the time at which - the taint was added. It is only written for NoExecute - taints. + description: TimeAdded represents the time at which the taint was added. It is only written for NoExecute taints. format: date-time type: string value: - description: The taint value corresponding to the taint - key. + description: The taint value corresponding to the taint key. type: string required: - effect @@ -393,8 +382,7 @@ spec: provisioner: type: string strategy: - description: AwsUpgradeStrategy defines the upgrade strategy of an - AWS Instance Group + description: AwsUpgradeStrategy defines the upgrade strategy of an AWS Instance Group properties: crd: properties: @@ -436,8 +424,7 @@ spec: type: string conditions: items: - description: InstanceGroupConditions describes the conditions of - the InstanceGroup + description: InstanceGroupConditions describes the conditions of the InstanceGroup properties: status: type: string diff --git a/controllers/providers/aws/constructors.go b/controllers/providers/aws/constructors.go index efa719bd..4a59d3d8 100644 --- a/controllers/providers/aws/constructors.go +++ b/controllers/providers/aws/constructors.go @@ -16,8 +16,6 @@ limitations under the License. package aws import ( - "fmt" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/autoscaling" "github.com/aws/aws-sdk-go/service/ec2" @@ -52,7 +50,6 @@ func (w *AwsWorker) GetAutoScalingBasicBlockDevice(name, volType, snapshot strin if !common.StringEmpty(snapshot) { device.Ebs.SnapshotId = aws.String(snapshot) } - fmt.Printf("%+v\n", device) return device }