From be1ac97e0f3fc890e4cc28cc866614bfd7285421 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Fri, 3 May 2019 17:45:29 +0100 Subject: [PATCH 1/9] Added a lookup of the AMI root device in order to support AMIs with root devices that do not use /dev/xvda Issue #774 --- pkg/ami/api.go | 14 ++++++++++++++ pkg/apis/eksctl.io/v1alpha5/types.go | 2 ++ pkg/cfn/builder/api_test.go | 2 ++ pkg/cfn/builder/nodegroup.go | 2 +- pkg/ctl/create/cluster.go | 6 ++++++ pkg/ctl/create/nodegroup.go | 6 ++++++ pkg/eks/api.go | 14 ++++++++++++++ 7 files changed, 45 insertions(+), 1 deletion(-) diff --git a/pkg/ami/api.go b/pkg/ami/api.go index 7f6ab4e4f9..91008d8e77 100644 --- a/pkg/ami/api.go +++ b/pkg/ami/api.go @@ -57,6 +57,20 @@ func IsAvailable(api ec2iface.EC2API, id string) (bool, error) { return *output.Images[0].State == "available", nil } +// LookupRootDeviceName gets the root block device mapping +func LookupRootDeviceName(api ec2iface.EC2API, id string) (string, error) { + input := &ec2.DescribeImagesInput{ + ImageIds: []*string{&id}, + } + + output, err := api.DescribeImages(input) + if err != nil { + return "", errors.Wrapf(err, "unable to determine root device name for %q", id) + } + + return *output.Images[0].RootDeviceName, nil +} + // FindImage will get the AMI to use for the EKS nodes by querying AWS EC2 API. // It will only look for images with a status of available and it will pick the // image with the newest creation date. diff --git a/pkg/apis/eksctl.io/v1alpha5/types.go b/pkg/apis/eksctl.io/v1alpha5/types.go index d12bc9e30e..35b3243445 100644 --- a/pkg/apis/eksctl.io/v1alpha5/types.go +++ b/pkg/apis/eksctl.io/v1alpha5/types.go @@ -395,6 +395,8 @@ type NodeGroup struct { // +optional VolumeType string `json:"volumeType"` // +optional + RootDevice string `json:"rootDevice,omitempty"` + // +optional MaxPodsPerNode int `json:"maxPodsPerNode,omitempty"` // +optional diff --git a/pkg/cfn/builder/api_test.go b/pkg/cfn/builder/api_test.go index dbffda6a19..3708551a2e 100644 --- a/pkg/cfn/builder/api_test.go +++ b/pkg/cfn/builder/api_test.go @@ -248,6 +248,7 @@ var _ = Describe("CloudFormation template builder API", func() { ng.AMIFamily = "AmazonLinux2" ng.VolumeSize = 2 ng.VolumeType = api.NodeVolumeTypeIO1 + ng.RootDevice = "/dev/xvda" if withFullVPC { cfg.VPC = testVPC() @@ -354,6 +355,7 @@ var _ = Describe("CloudFormation template builder API", func() { DesiredCapacity: nil, VolumeSize: 2, VolumeType: api.NodeVolumeTypeIO1, + RootDevice: "/dev/xvda", IAM: &api.NodeGroupIAM{ WithAddonPolicies: api.NodeGroupIAMAddonPolicies{ ImageBuilder: api.Disabled(), diff --git a/pkg/cfn/builder/nodegroup.go b/pkg/cfn/builder/nodegroup.go index 0db0b9ed22..dc9b9a0cf2 100644 --- a/pkg/cfn/builder/nodegroup.go +++ b/pkg/cfn/builder/nodegroup.go @@ -128,7 +128,7 @@ func (n *NodeGroupResourceSet) addResourcesForNodeGroup() error { if n.spec.VolumeSize > 0 { launchTemplateData.BlockDeviceMappings = []gfn.AWSEC2LaunchTemplate_BlockDeviceMapping{{ - DeviceName: gfn.NewString("/dev/xvda"), + DeviceName: gfn.NewString(n.spec.RootDevice), Ebs: &gfn.AWSEC2LaunchTemplate_Ebs{ VolumeSize: gfn.NewInteger(n.spec.VolumeSize), VolumeType: gfn.NewString(n.spec.VolumeType), diff --git a/pkg/ctl/create/cluster.go b/pkg/ctl/create/cluster.go index a3ad919618..5b109e3aa5 100644 --- a/pkg/ctl/create/cluster.go +++ b/pkg/ctl/create/cluster.go @@ -253,6 +253,12 @@ func doCreateCluster(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg stri } logger.Info("nodegroup %q will use %q [%s/%s]", ng.Name, ng.AMI, ng.AMIFamily, cfg.Metadata.Version) + // Lookup AMI root device name + if err := ctl.GetRootDevice(ng); err != nil { + return err + } + logger.Info("%q has root device %q [%s/%s]", ng.AMI, ng.RootDevice, cfg.Metadata.Version) + if err := ctl.SetNodeLabels(ng, meta); err != nil { return err } diff --git a/pkg/ctl/create/nodegroup.go b/pkg/ctl/create/nodegroup.go index d1bb14f72d..f1cc2272c2 100644 --- a/pkg/ctl/create/nodegroup.go +++ b/pkg/ctl/create/nodegroup.go @@ -121,6 +121,12 @@ func doCreateNodeGroups(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg s } logger.Info("nodegroup %q will use %q [%s/%s]", ng.Name, ng.AMI, ng.AMIFamily, cfg.Metadata.Version) + // Lookup AMI root device name + if err := ctl.GetRootDevice(ng); err != nil { + return err + } + logger.Info("%q has root device %q [%s/%s]", ng.AMI, ng.RootDevice, cfg.Metadata.Version) + if err := ctl.SetNodeLabels(ng, meta); err != nil { return err } diff --git a/pkg/eks/api.go b/pkg/eks/api.go index 8d0f39764e..f24f9fcf04 100644 --- a/pkg/eks/api.go +++ b/pkg/eks/api.go @@ -260,6 +260,20 @@ func (c *ClusterProvider) EnsureAMI(version string, ng *api.NodeGroup) error { return nil } +// GetRootDevice looks up the root device for the ami +func (c *ClusterProvider) GetRootDevice(ng *api.NodeGroup) (error) { + + // Get Root Device Name + rootDevice, err := ami.LookupRootDeviceName(c.Provider.EC2(), ng.AMI) + if err != nil { + return errors.Wrapf(err, "%s root device name is not available", ng.AMI) + } + + ng.RootDevice = rootDevice + + return nil +} + // SetNodeLabels initialises and validate node labels based on cluster and nodegroup names func (c *ClusterProvider) SetNodeLabels(ng *api.NodeGroup, meta *api.ClusterMeta) error { if ng.Labels == nil { From 15391e73e9eab96ef4d022ba3318185d9e5602cb Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Tue, 7 May 2019 12:18:43 +0100 Subject: [PATCH 2/9] Renamed rootDevice to volumeName to be consistent Added length check to the results in LookupRootDeviceName Moved the root device lookup into ctl.EnsureAMI --- humans.txt | 1 + pkg/ami/api.go | 4 ++++ pkg/apis/eksctl.io/v1alpha5/types.go | 2 +- pkg/cfn/builder/api_test.go | 4 ++-- pkg/cfn/builder/nodegroup.go | 2 +- pkg/ctl/create/cluster.go | 6 ------ pkg/ctl/create/nodegroup.go | 6 ------ pkg/eks/api.go | 10 ++-------- 8 files changed, 11 insertions(+), 24 deletions(-) diff --git a/humans.txt b/humans.txt index dcaa20fc94..12b1296c8d 100644 --- a/humans.txt +++ b/humans.txt @@ -38,6 +38,7 @@ Dann Church @D3nn Roli Schilter @rndstr Mitchel Humpherys @mgalgs Fred Cox @mcfedr +Adam Johnson @adamjohnson01 /* Thanks */ diff --git a/pkg/ami/api.go b/pkg/ami/api.go index 91008d8e77..3f126ea20b 100644 --- a/pkg/ami/api.go +++ b/pkg/ami/api.go @@ -68,6 +68,10 @@ func LookupRootDeviceName(api ec2iface.EC2API, id string) (string, error) { return "", errors.Wrapf(err, "unable to determine root device name for %q", id) } + if len(output.Images) < 1 { + return "", errors.Wrapf(err, "unable to find %q", id) + } + return *output.Images[0].RootDeviceName, nil } diff --git a/pkg/apis/eksctl.io/v1alpha5/types.go b/pkg/apis/eksctl.io/v1alpha5/types.go index 35b3243445..09aef7b068 100644 --- a/pkg/apis/eksctl.io/v1alpha5/types.go +++ b/pkg/apis/eksctl.io/v1alpha5/types.go @@ -395,7 +395,7 @@ type NodeGroup struct { // +optional VolumeType string `json:"volumeType"` // +optional - RootDevice string `json:"rootDevice,omitempty"` + VolumeName string `json:"volumeName,omitempty"` // +optional MaxPodsPerNode int `json:"maxPodsPerNode,omitempty"` diff --git a/pkg/cfn/builder/api_test.go b/pkg/cfn/builder/api_test.go index 3708551a2e..3b7a62330a 100644 --- a/pkg/cfn/builder/api_test.go +++ b/pkg/cfn/builder/api_test.go @@ -248,7 +248,7 @@ var _ = Describe("CloudFormation template builder API", func() { ng.AMIFamily = "AmazonLinux2" ng.VolumeSize = 2 ng.VolumeType = api.NodeVolumeTypeIO1 - ng.RootDevice = "/dev/xvda" + ng.VolumeName = "/dev/xvda" if withFullVPC { cfg.VPC = testVPC() @@ -355,7 +355,7 @@ var _ = Describe("CloudFormation template builder API", func() { DesiredCapacity: nil, VolumeSize: 2, VolumeType: api.NodeVolumeTypeIO1, - RootDevice: "/dev/xvda", + VolumeName: "/dev/xvda", IAM: &api.NodeGroupIAM{ WithAddonPolicies: api.NodeGroupIAMAddonPolicies{ ImageBuilder: api.Disabled(), diff --git a/pkg/cfn/builder/nodegroup.go b/pkg/cfn/builder/nodegroup.go index dc9b9a0cf2..c2d83e5d68 100644 --- a/pkg/cfn/builder/nodegroup.go +++ b/pkg/cfn/builder/nodegroup.go @@ -128,7 +128,7 @@ func (n *NodeGroupResourceSet) addResourcesForNodeGroup() error { if n.spec.VolumeSize > 0 { launchTemplateData.BlockDeviceMappings = []gfn.AWSEC2LaunchTemplate_BlockDeviceMapping{{ - DeviceName: gfn.NewString(n.spec.RootDevice), + DeviceName: gfn.NewString(n.spec.VolumeName), Ebs: &gfn.AWSEC2LaunchTemplate_Ebs{ VolumeSize: gfn.NewInteger(n.spec.VolumeSize), VolumeType: gfn.NewString(n.spec.VolumeType), diff --git a/pkg/ctl/create/cluster.go b/pkg/ctl/create/cluster.go index 5b109e3aa5..a3ad919618 100644 --- a/pkg/ctl/create/cluster.go +++ b/pkg/ctl/create/cluster.go @@ -253,12 +253,6 @@ func doCreateCluster(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg stri } logger.Info("nodegroup %q will use %q [%s/%s]", ng.Name, ng.AMI, ng.AMIFamily, cfg.Metadata.Version) - // Lookup AMI root device name - if err := ctl.GetRootDevice(ng); err != nil { - return err - } - logger.Info("%q has root device %q [%s/%s]", ng.AMI, ng.RootDevice, cfg.Metadata.Version) - if err := ctl.SetNodeLabels(ng, meta); err != nil { return err } diff --git a/pkg/ctl/create/nodegroup.go b/pkg/ctl/create/nodegroup.go index f1cc2272c2..d1bb14f72d 100644 --- a/pkg/ctl/create/nodegroup.go +++ b/pkg/ctl/create/nodegroup.go @@ -121,12 +121,6 @@ func doCreateNodeGroups(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg s } logger.Info("nodegroup %q will use %q [%s/%s]", ng.Name, ng.AMI, ng.AMIFamily, cfg.Metadata.Version) - // Lookup AMI root device name - if err := ctl.GetRootDevice(ng); err != nil { - return err - } - logger.Info("%q has root device %q [%s/%s]", ng.AMI, ng.RootDevice, cfg.Metadata.Version) - if err := ctl.SetNodeLabels(ng, meta); err != nil { return err } diff --git a/pkg/eks/api.go b/pkg/eks/api.go index f24f9fcf04..308c6197dc 100644 --- a/pkg/eks/api.go +++ b/pkg/eks/api.go @@ -257,19 +257,13 @@ func (c *ClusterProvider) EnsureAMI(version string, ng *api.NodeGroup) error { return ami.NewErrNotFound(ng.AMI) } - return nil -} - -// GetRootDevice looks up the root device for the ami -func (c *ClusterProvider) GetRootDevice(ng *api.NodeGroup) (error) { - // Get Root Device Name rootDevice, err := ami.LookupRootDeviceName(c.Provider.EC2(), ng.AMI) if err != nil { - return errors.Wrapf(err, "%s root device name is not available", ng.AMI) + return errors.Wrapf(err, "%s failed root device name lookup", ng.AMI) } - ng.RootDevice = rootDevice + ng.VolumeName = rootDevice return nil } From e54ea5ae2488e67979225b02c75a303111ad4446 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Wed, 8 May 2019 14:21:41 +0100 Subject: [PATCH 3/9] Switched VolumeSize and VolumeType to be pointers Merged IsAvailable and RootDeviceLookup Added check for instance-store amis --- pkg/ami/api.go | 26 +++----------------- pkg/apis/eksctl.io/v1alpha5/defaults.go | 6 ++--- pkg/apis/eksctl.io/v1alpha5/defaults_test.go | 2 ++ pkg/apis/eksctl.io/v1alpha5/types.go | 17 ++++++++----- pkg/cfn/builder/api_test.go | 22 ++++++++++------- pkg/cfn/builder/nodegroup.go | 6 ++--- pkg/ctl/cmdutils/nodegroup_filter_test.go | 11 ++++++--- pkg/ctl/cmdutils/nodegroup_flags.go | 4 +-- pkg/eks/api.go | 12 ++++----- 9 files changed, 52 insertions(+), 54 deletions(-) diff --git a/pkg/ami/api.go b/pkg/ami/api.go index 3f126ea20b..bbf76fbdec 100644 --- a/pkg/ami/api.go +++ b/pkg/ami/api.go @@ -40,39 +40,21 @@ var ImageClasses = []string{ } // IsAvailable checks if a given AMI ID is available in AWS EC2 -func IsAvailable(api ec2iface.EC2API, id string) (bool, error) { +func IsAvailable(api ec2iface.EC2API, id string) (bool, string, string, error) { input := &ec2.DescribeImagesInput{ ImageIds: []*string{&id}, } output, err := api.DescribeImages(input) if err != nil { - return false, errors.Wrapf(err, "unable to find %q", id) + return false, "", "", errors.Wrapf(err, "unable to find %q", id) } if len(output.Images) < 1 { - return false, nil + return false, "", "", nil } - return *output.Images[0].State == "available", nil -} - -// LookupRootDeviceName gets the root block device mapping -func LookupRootDeviceName(api ec2iface.EC2API, id string) (string, error) { - input := &ec2.DescribeImagesInput{ - ImageIds: []*string{&id}, - } - - output, err := api.DescribeImages(input) - if err != nil { - return "", errors.Wrapf(err, "unable to determine root device name for %q", id) - } - - if len(output.Images) < 1 { - return "", errors.Wrapf(err, "unable to find %q", id) - } - - return *output.Images[0].RootDeviceName, nil + return *output.Images[0].State == "available", *output.Images[0].RootDeviceName, *output.Images[0].RootDeviceType, nil } // FindImage will get the AMI to use for the EKS nodes by querying AWS EC2 API. diff --git a/pkg/apis/eksctl.io/v1alpha5/defaults.go b/pkg/apis/eksctl.io/v1alpha5/defaults.go index e74b5f69d3..12e1887e40 100644 --- a/pkg/apis/eksctl.io/v1alpha5/defaults.go +++ b/pkg/apis/eksctl.io/v1alpha5/defaults.go @@ -45,9 +45,9 @@ func SetNodeGroupDefaults(_ int, ng *NodeGroup) error { } } - if ng.VolumeSize > 0 { - if ng.VolumeType == "" { - ng.VolumeType = DefaultNodeVolumeType + if *ng.VolumeSize > 0 { + if *ng.VolumeType == "" { + *ng.VolumeType = DefaultNodeVolumeType } } diff --git a/pkg/apis/eksctl.io/v1alpha5/defaults_test.go b/pkg/apis/eksctl.io/v1alpha5/defaults_test.go index d9beafcb9f..dd8b1437cc 100644 --- a/pkg/apis/eksctl.io/v1alpha5/defaults_test.go +++ b/pkg/apis/eksctl.io/v1alpha5/defaults_test.go @@ -14,6 +14,7 @@ var _ = Describe("Default settings", func() { It("Providing an SSH key enables SSH", func() { testNodeGroup := NodeGroup{ + VolumeSize: &DefaultNodeVolumeSize, SSH: &NodeGroupSSH{ Allow: Disabled(), PublicKeyPath: &testKeyPath, @@ -27,6 +28,7 @@ var _ = Describe("Default settings", func() { It("Enabling SSH without a key uses default key", func() { testNodeGroup := NodeGroup{ + VolumeSize: &DefaultNodeVolumeSize, SSH: &NodeGroupSSH{ Allow: Enabled(), }, diff --git a/pkg/apis/eksctl.io/v1alpha5/types.go b/pkg/apis/eksctl.io/v1alpha5/types.go index 09aef7b068..13803f535a 100644 --- a/pkg/apis/eksctl.io/v1alpha5/types.go +++ b/pkg/apis/eksctl.io/v1alpha5/types.go @@ -78,8 +78,6 @@ const ( // DefaultNodeCount defines the default number of nodes to be created DefaultNodeCount = 2 - // DefaultNodeVolumeType defines the default root volume type to use - DefaultNodeVolumeType = NodeVolumeTypeGP2 // NodeVolumeTypeGP2 is General Purpose SSD NodeVolumeTypeGP2 = "gp2" // NodeVolumeTypeIO1 is Provisioned IOPS SSD @@ -128,6 +126,13 @@ var ( // DefaultNodeSSHPublicKeyPath is the default path to SSH public key DefaultNodeSSHPublicKeyPath = "~/.ssh/id_rsa.pub" + + // DefaultNodeVolumeType defines the default root volume type to use + DefaultNodeVolumeType = NodeVolumeTypeGP2 + + // DefaultNodeVolumeSize defines the default root volume size + DefaultNodeVolumeSize = 0 + ) // Enabled return pointer to true value @@ -338,8 +343,8 @@ func (c *ClusterConfig) NewNodeGroup() *NodeGroup { }, DesiredCapacity: nil, InstanceType: DefaultNodeType, - VolumeSize: 0, - VolumeType: DefaultNodeVolumeType, + VolumeSize: &DefaultNodeVolumeSize, + VolumeType: &DefaultNodeVolumeType, IAM: &NodeGroupIAM{ WithAddonPolicies: NodeGroupIAMAddonPolicies{ ImageBuilder: Disabled(), @@ -391,9 +396,9 @@ type NodeGroup struct { MaxSize *int `json:"maxSize,omitempty"` // +optional - VolumeSize int `json:"volumeSize"` + VolumeSize *int `json:"volumeSize"` // +optional - VolumeType string `json:"volumeType"` + VolumeType *string `json:"volumeType"` // +optional VolumeName string `json:"volumeName,omitempty"` // +optional diff --git a/pkg/cfn/builder/api_test.go b/pkg/cfn/builder/api_test.go index 3b7a62330a..ef33ecd09e 100644 --- a/pkg/cfn/builder/api_test.go +++ b/pkg/cfn/builder/api_test.go @@ -242,12 +242,15 @@ var _ = Describe("CloudFormation template builder API", func() { Endpoint: endpoint, } + volType := api.NodeVolumeTypeIO1 + volSize := 2 + cfg.AvailabilityZones = testAZs ng.Name = "ng-abcd1234" ng.InstanceType = "t2.medium" ng.AMIFamily = "AmazonLinux2" - ng.VolumeSize = 2 - ng.VolumeType = api.NodeVolumeTypeIO1 + ng.VolumeSize = &volSize + ng.VolumeType = &volType ng.VolumeName = "/dev/xvda" if withFullVPC { @@ -322,7 +325,8 @@ var _ = Describe("CloudFormation template builder API", func() { } Describe("GetAllOutputsFromClusterStack", func() { - defaultSSHKeyPath := "~/.ssh/id_rsa.pub" + volSize := 2 + volType := api.NodeVolumeTypeIO1 expected := &api.ClusterConfig{ TypeMeta: api.ClusterConfigTypeMeta(), Metadata: &api.ClusterMeta{ @@ -353,8 +357,8 @@ var _ = Describe("CloudFormation template builder API", func() { AttachIDs: []string{}, }, DesiredCapacity: nil, - VolumeSize: 2, - VolumeType: api.NodeVolumeTypeIO1, + VolumeSize: &volSize, + VolumeType: &volType, VolumeName: "/dev/xvda", IAM: &api.NodeGroupIAM{ WithAddonPolicies: api.NodeGroupIAMAddonPolicies{ @@ -370,7 +374,7 @@ var _ = Describe("CloudFormation template builder API", func() { }, SSH: &api.NodeGroupSSH{ Allow: api.Disabled(), - PublicKeyPath: &defaultSSHKeyPath, + PublicKeyPath: &api.DefaultNodeSSHPublicKeyPath, }, }, }, @@ -951,7 +955,7 @@ var _ = Describe("CloudFormation template builder API", func() { Context("NodeGroupEBS", func() { cfg, ng := newClusterConfigAndNodegroup(true) - ng.VolumeSize = 0 + *ng.VolumeSize = 0 ng.IAM.WithAddonPolicies.EBS = api.Enabled() build(cfg, "eksctl-test-ebs-cluster", ng) @@ -996,7 +1000,7 @@ var _ = Describe("CloudFormation template builder API", func() { Context("NodeGroupFSX", func() { cfg, ng := newClusterConfigAndNodegroup(true) - ng.VolumeSize = 0 + *ng.VolumeSize = 0 ng.IAM.WithAddonPolicies.FSX = api.Enabled() build(cfg, "eksctl-test-fsx-cluster", ng) @@ -1030,7 +1034,7 @@ var _ = Describe("CloudFormation template builder API", func() { Context("NodeGroupEFS", func() { cfg, ng := newClusterConfigAndNodegroup(true) - ng.VolumeSize = 0 + *ng.VolumeSize = 0 ng.IAM.WithAddonPolicies.EFS = api.Enabled() build(cfg, "eksctl-test-efs-cluster", ng) diff --git a/pkg/cfn/builder/nodegroup.go b/pkg/cfn/builder/nodegroup.go index c2d83e5d68..35ac2facbb 100644 --- a/pkg/cfn/builder/nodegroup.go +++ b/pkg/cfn/builder/nodegroup.go @@ -126,12 +126,12 @@ func (n *NodeGroupResourceSet) addResourcesForNodeGroup() error { launchTemplateData.KeyName = gfn.NewString(*n.spec.SSH.PublicKeyName) } - if n.spec.VolumeSize > 0 { + if *n.spec.VolumeSize > 0 { launchTemplateData.BlockDeviceMappings = []gfn.AWSEC2LaunchTemplate_BlockDeviceMapping{{ DeviceName: gfn.NewString(n.spec.VolumeName), Ebs: &gfn.AWSEC2LaunchTemplate_Ebs{ - VolumeSize: gfn.NewInteger(n.spec.VolumeSize), - VolumeType: gfn.NewString(n.spec.VolumeType), + VolumeSize: gfn.NewInteger(*n.spec.VolumeSize), + VolumeType: gfn.NewString(*n.spec.VolumeType), }, }} } diff --git a/pkg/ctl/cmdutils/nodegroup_filter_test.go b/pkg/ctl/cmdutils/nodegroup_filter_test.go index 03ed632d0d..46c017faac 100644 --- a/pkg/ctl/cmdutils/nodegroup_filter_test.go +++ b/pkg/ctl/cmdutils/nodegroup_filter_test.go @@ -243,10 +243,13 @@ func newClusterConfig() *api.ClusterConfig { func addGroupA(cfg *api.ClusterConfig) { var ng *api.NodeGroup + ng1aVolSize := 768 + ng1aVolType := api.NodeVolumeTypeIO1 + ng = cfg.NewNodeGroup() ng.Name = "test-ng1a" - ng.VolumeSize = 768 - ng.VolumeType = "io1" + ng.VolumeSize = &ng1aVolSize + ng.VolumeType = &ng1aVolType ng.IAM.AttachPolicyARNs = []string{"foo"} ng.Labels = map[string]string{"group": "a", "seq": "1"} ng.SSH = nil @@ -284,9 +287,11 @@ func addGroupB(cfg *api.ClusterConfig) { ng.Labels = map[string]string{"group": "b", "seq": "1"} ng.SSH = nil + ng3bVolSize := 192 + ng = cfg.NewNodeGroup() ng.Name = "test-ng3b" - ng.VolumeSize = 192 + ng.VolumeSize = &ng3bVolSize ng.SecurityGroups.AttachIDs = []string{"sg-1", "sg-2"} ng.SecurityGroups.WithLocal = api.Disabled() ng.Labels = map[string]string{"group": "b", "seq": "1"} diff --git a/pkg/ctl/cmdutils/nodegroup_flags.go b/pkg/ctl/cmdutils/nodegroup_flags.go index 9c95259a37..382d40b735 100644 --- a/pkg/ctl/cmdutils/nodegroup_flags.go +++ b/pkg/ctl/cmdutils/nodegroup_flags.go @@ -31,8 +31,8 @@ func AddCommonCreateNodeGroupFlags(cmd *cobra.Command, fs *pflag.FlagSet, p *api } }) - fs.IntVar(&ng.VolumeSize, "node-volume-size", ng.VolumeSize, "node volume size in GB") - fs.StringVar(&ng.VolumeType, "node-volume-type", ng.VolumeType, fmt.Sprintf("node volume type (valid options: %s)", strings.Join(api.SupportedNodeVolumeTypes(), ", "))) + fs.IntVar(ng.VolumeSize, "node-volume-size", *ng.VolumeSize, "node volume size in GB") + fs.StringVar(ng.VolumeType, "node-volume-type", *ng.VolumeType, fmt.Sprintf("node volume type (valid options: %s)", strings.Join(api.SupportedNodeVolumeTypes(), ", "))) fs.IntVar(&ng.MaxPodsPerNode, "max-pods-per-node", 0, "maximum number of pods per node (set automatically if unspecified)") diff --git a/pkg/eks/api.go b/pkg/eks/api.go index 308c6197dc..d332ca4756 100644 --- a/pkg/eks/api.go +++ b/pkg/eks/api.go @@ -248,7 +248,7 @@ func (c *ClusterProvider) EnsureAMI(version string, ng *api.NodeGroup) error { } // Check the AMI is available - available, err := ami.IsAvailable(c.Provider.EC2(), ng.AMI) + available, devName, devType, err := ami.IsAvailable(c.Provider.EC2(), ng.AMI) if err != nil { return errors.Wrapf(err, "%s is not available", ng.AMI) } @@ -257,13 +257,13 @@ func (c *ClusterProvider) EnsureAMI(version string, ng *api.NodeGroup) error { return ami.NewErrNotFound(ng.AMI) } - // Get Root Device Name - rootDevice, err := ami.LookupRootDeviceName(c.Provider.EC2(), ng.AMI) - if err != nil { - return errors.Wrapf(err, "%s failed root device name lookup", ng.AMI) + if devType == "instance-store" { + return fmt.Errorf("%q is an instance-store ami and EBS block device mappings not supported for instance-store AMIs", ng.AMI) } - ng.VolumeName = rootDevice + if devType == "ebs" { + ng.VolumeName = devName + } return nil } From 9767a8e547da6b1948c186b3b4cf6a85b5b31c2f Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Wed, 8 May 2019 14:58:47 +0100 Subject: [PATCH 4/9] Added validation check that makes sure that VolumeSize is set if VolumeType is set --- pkg/apis/eksctl.io/v1alpha5/validation.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/apis/eksctl.io/v1alpha5/validation.go b/pkg/apis/eksctl.io/v1alpha5/validation.go index 06a0345860..14d2c0f910 100644 --- a/pkg/apis/eksctl.io/v1alpha5/validation.go +++ b/pkg/apis/eksctl.io/v1alpha5/validation.go @@ -124,6 +124,10 @@ func ValidateNodeGroup(i int, ng *NodeGroup) error { return nil } + if ng.VolumeSize == nil && IsSetAndNonEmptyString(ng.VolumeType) { + return fmt.Errorf("VolumeType can not be set without VolumeSize") + } + if err := validateNodeGroupIAM(i, ng, ng.IAM.InstanceProfileARN, "instanceProfileARN", path); err != nil { return err } From 900bdc87df00c40805752cb62b15d696f5c59d59 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Thu, 9 May 2019 08:12:40 +0100 Subject: [PATCH 5/9] Added comment about the number of images returned by api.IsAvailable --- pkg/ami/api.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/ami/api.go b/pkg/ami/api.go index bbf76fbdec..a0caca9c70 100644 --- a/pkg/ami/api.go +++ b/pkg/ami/api.go @@ -50,6 +50,7 @@ func IsAvailable(api ec2iface.EC2API, id string) (bool, string, string, error) { return false, "", "", errors.Wrapf(err, "unable to find %q", id) } + // This will never return more than one as we are looking up a single ami id if len(output.Images) < 1 { return false, "", "", nil } From 41f3b3be965b4b00b0cb4110951ff59daa65e00e Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Thu, 9 May 2019 16:08:48 +0100 Subject: [PATCH 6/9] Fix error format --- pkg/eks/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/eks/api.go b/pkg/eks/api.go index d332ca4756..197a80ef23 100644 --- a/pkg/eks/api.go +++ b/pkg/eks/api.go @@ -258,7 +258,7 @@ func (c *ClusterProvider) EnsureAMI(version string, ng *api.NodeGroup) error { } if devType == "instance-store" { - return fmt.Errorf("%q is an instance-store ami and EBS block device mappings not supported for instance-store AMIs", ng.AMI) + return fmt.Errorf("%q is an instance-store AMI and EBS block device mappings not supported for instance-store AMIs", ng.AMI) } if devType == "ebs" { From b991ad1a04d7fee25a93319eccbe48fcca5a9c56 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Fri, 10 May 2019 10:54:14 +0100 Subject: [PATCH 7/9] Renamed the IsAvailable function to Use and moved RootDevice checks and configuration into it --- pkg/ami/api.go | 23 +++++++++++++++++------ pkg/eks/api.go | 20 ++------------------ 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/pkg/ami/api.go b/pkg/ami/api.go index a0caca9c70..0ac4d704e9 100644 --- a/pkg/ami/api.go +++ b/pkg/ami/api.go @@ -11,6 +11,8 @@ import ( "github.com/aws/aws-sdk-go/service/ec2/ec2iface" api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5" + "github.com/wintoncode/eksctl/pkg/ami" + "fmt" ) const ( @@ -39,23 +41,32 @@ var ImageClasses = []string{ "ImageClassGPU", } -// IsAvailable checks if a given AMI ID is available in AWS EC2 -func IsAvailable(api ec2iface.EC2API, id string) (bool, string, string, error) { +// Use checks if a given AMI ID is available in AWS EC2 as well as checking and populating RootDevice information +func Use(api ec2iface.EC2API, ng *api.NodeGroup) error { input := &ec2.DescribeImagesInput{ - ImageIds: []*string{&id}, + ImageIds: []*string{&ng.AMI}, } output, err := api.DescribeImages(input) if err != nil { - return false, "", "", errors.Wrapf(err, "unable to find %q", id) + return errors.Wrapf(err, "unable to find image %q", ng.AMI) } // This will never return more than one as we are looking up a single ami id if len(output.Images) < 1 { - return false, "", "", nil + return ami.NewErrNotFound(ng.AMI) } - return *output.Images[0].State == "available", *output.Images[0].RootDeviceName, *output.Images[0].RootDeviceType, nil + // Instance-store AMIs cannot have their root volume size managed + if *output.Images[0].RootDeviceType == "instance-store" { + return fmt.Errorf("%q is an instance-store AMI and EBS block device mappings not supported for instance-store AMIs", ng.AMI) + } + + if *output.Images[0].RootDeviceType == "ebs" { + ng.VolumeName = *output.Images[0].RootDeviceName + } + + return nil } // FindImage will get the AMI to use for the EKS nodes by querying AWS EC2 API. diff --git a/pkg/eks/api.go b/pkg/eks/api.go index 197a80ef23..873f62f8f9 100644 --- a/pkg/eks/api.go +++ b/pkg/eks/api.go @@ -247,25 +247,9 @@ func (c *ClusterProvider) EnsureAMI(version string, ng *api.NodeGroup) error { ng.AMI = id } - // Check the AMI is available - available, devName, devType, err := ami.IsAvailable(c.Provider.EC2(), ng.AMI) - if err != nil { - return errors.Wrapf(err, "%s is not available", ng.AMI) - } + // Check the AMI is available and populate RootDevice information + return ami.Use(c.Provider.EC2(), ng) - if !available { - return ami.NewErrNotFound(ng.AMI) - } - - if devType == "instance-store" { - return fmt.Errorf("%q is an instance-store AMI and EBS block device mappings not supported for instance-store AMIs", ng.AMI) - } - - if devType == "ebs" { - ng.VolumeName = devName - } - - return nil } // SetNodeLabels initialises and validate node labels based on cluster and nodegroup names From 0ef1652ac4e685666c82ffc5ec2339ae16f9c028 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Fri, 10 May 2019 10:59:03 +0100 Subject: [PATCH 8/9] Fix ami error message --- pkg/ami/api.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/ami/api.go b/pkg/ami/api.go index 0ac4d704e9..5d609405c2 100644 --- a/pkg/ami/api.go +++ b/pkg/ami/api.go @@ -11,7 +11,6 @@ import ( "github.com/aws/aws-sdk-go/service/ec2/ec2iface" api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5" - "github.com/wintoncode/eksctl/pkg/ami" "fmt" ) @@ -54,7 +53,7 @@ func Use(api ec2iface.EC2API, ng *api.NodeGroup) error { // This will never return more than one as we are looking up a single ami id if len(output.Images) < 1 { - return ami.NewErrNotFound(ng.AMI) + return NewErrNotFound(ng.AMI) } // Instance-store AMIs cannot have their root volume size managed From 25e1f6f3286150c1136d4a86c1eb39a868c123d2 Mon Sep 17 00:00:00 2001 From: Ilya Dmitrichenko Date: Fri, 10 May 2019 14:00:10 +0100 Subject: [PATCH 9/9] Few tweaks to handing of pointers and validation --- pkg/ami/api.go | 15 +++++----- pkg/apis/eksctl.io/v1alpha5/defaults.go | 6 ++-- pkg/apis/eksctl.io/v1alpha5/types.go | 3 +- pkg/apis/eksctl.io/v1alpha5/validation.go | 36 +++++++++++++---------- pkg/cfn/builder/api_test.go | 26 ++++++++-------- pkg/cfn/builder/nodegroup.go | 6 ++-- 6 files changed, 46 insertions(+), 46 deletions(-) diff --git a/pkg/ami/api.go b/pkg/ami/api.go index 5d609405c2..620e52bcde 100644 --- a/pkg/ami/api.go +++ b/pkg/ami/api.go @@ -10,8 +10,9 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/ec2/ec2iface" - api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5" "fmt" + + api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5" ) const ( @@ -41,12 +42,12 @@ var ImageClasses = []string{ } // Use checks if a given AMI ID is available in AWS EC2 as well as checking and populating RootDevice information -func Use(api ec2iface.EC2API, ng *api.NodeGroup) error { +func Use(ec2api ec2iface.EC2API, ng *api.NodeGroup) error { input := &ec2.DescribeImagesInput{ ImageIds: []*string{&ng.AMI}, } - output, err := api.DescribeImages(input) + output, err := ec2api.DescribeImages(input) if err != nil { return errors.Wrapf(err, "unable to find image %q", ng.AMI) } @@ -61,8 +62,8 @@ func Use(api ec2iface.EC2API, ng *api.NodeGroup) error { return fmt.Errorf("%q is an instance-store AMI and EBS block device mappings not supported for instance-store AMIs", ng.AMI) } - if *output.Images[0].RootDeviceType == "ebs" { - ng.VolumeName = *output.Images[0].RootDeviceName + if *output.Images[0].RootDeviceType == "ebs" && !api.IsSetAndNonEmptyString(ng.VolumeName) { + ng.VolumeName = output.Images[0].RootDeviceName } return nil @@ -71,7 +72,7 @@ func Use(api ec2iface.EC2API, ng *api.NodeGroup) error { // FindImage will get the AMI to use for the EKS nodes by querying AWS EC2 API. // It will only look for images with a status of available and it will pick the // image with the newest creation date. -func FindImage(api ec2iface.EC2API, ownerAccount, namePattern string) (string, error) { +func FindImage(ec2api ec2iface.EC2API, ownerAccount, namePattern string) (string, error) { input := &ec2.DescribeImagesInput{ Owners: []*string{&ownerAccount}, Filters: []*ec2.Filter{ @@ -98,7 +99,7 @@ func FindImage(api ec2iface.EC2API, ownerAccount, namePattern string) (string, e }, } - output, err := api.DescribeImages(input) + output, err := ec2api.DescribeImages(input) if err != nil { return "", errors.Wrapf(err, "error querying AWS for images") } diff --git a/pkg/apis/eksctl.io/v1alpha5/defaults.go b/pkg/apis/eksctl.io/v1alpha5/defaults.go index 12e1887e40..76e8429efb 100644 --- a/pkg/apis/eksctl.io/v1alpha5/defaults.go +++ b/pkg/apis/eksctl.io/v1alpha5/defaults.go @@ -45,10 +45,8 @@ func SetNodeGroupDefaults(_ int, ng *NodeGroup) error { } } - if *ng.VolumeSize > 0 { - if *ng.VolumeType == "" { - *ng.VolumeType = DefaultNodeVolumeType - } + if !IsSetAndNonEmptyString(ng.VolumeType) { + ng.VolumeType = &DefaultNodeVolumeType } if ng.IAM == nil { diff --git a/pkg/apis/eksctl.io/v1alpha5/types.go b/pkg/apis/eksctl.io/v1alpha5/types.go index 13803f535a..69c48f0910 100644 --- a/pkg/apis/eksctl.io/v1alpha5/types.go +++ b/pkg/apis/eksctl.io/v1alpha5/types.go @@ -132,7 +132,6 @@ var ( // DefaultNodeVolumeSize defines the default root volume size DefaultNodeVolumeSize = 0 - ) // Enabled return pointer to true value @@ -400,7 +399,7 @@ type NodeGroup struct { // +optional VolumeType *string `json:"volumeType"` // +optional - VolumeName string `json:"volumeName,omitempty"` + VolumeName *string `json:"volumeName,omitempty"` // +optional MaxPodsPerNode int `json:"maxPodsPerNode,omitempty"` diff --git a/pkg/apis/eksctl.io/v1alpha5/validation.go b/pkg/apis/eksctl.io/v1alpha5/validation.go index 14d2c0f910..c036eb0aeb 100644 --- a/pkg/apis/eksctl.io/v1alpha5/validation.go +++ b/pkg/apis/eksctl.io/v1alpha5/validation.go @@ -120,28 +120,32 @@ func ValidateNodeGroup(i int, ng *NodeGroup) error { return fmt.Errorf("%s.name must be set", path) } - if ng.IAM == nil { - return nil + if ng.VolumeSize == nil { + if IsSetAndNonEmptyString(ng.VolumeType) { + return fmt.Errorf("%s.volumeType can not be set without %s.volumeSize", path, path) + } + if IsSetAndNonEmptyString(ng.VolumeName) { + return fmt.Errorf("%s.volumeName can not be set without %s.volumeSize", path, path) + } } - if ng.VolumeSize == nil && IsSetAndNonEmptyString(ng.VolumeType) { - return fmt.Errorf("VolumeType can not be set without VolumeSize") - } + if ng.IAM != nil { + if err := validateNodeGroupIAM(i, ng, ng.IAM.InstanceProfileARN, "instanceProfileARN", path); err != nil { + return err + } + if err := validateNodeGroupIAM(i, ng, ng.IAM.InstanceRoleARN, "instanceRoleARN", path); err != nil { + return err + } - if err := validateNodeGroupIAM(i, ng, ng.IAM.InstanceProfileARN, "instanceProfileARN", path); err != nil { - return err - } - if err := validateNodeGroupIAM(i, ng, ng.IAM.InstanceRoleARN, "instanceRoleARN", path); err != nil { - return err - } + if err := ValidateNodeGroupLabels(ng); err != nil { + return err + } - if err := ValidateNodeGroupLabels(ng); err != nil { - return err + if err := validateNodeGroupSSH(ng.SSH); err != nil { + return fmt.Errorf("only one ssh public key can be specified per node-group") + } } - if err := validateNodeGroupSSH(ng.SSH); err != nil { - return fmt.Errorf("only one ssh public key can be specified per node-group") - } return nil } diff --git a/pkg/cfn/builder/api_test.go b/pkg/cfn/builder/api_test.go index ef33ecd09e..c1d8ae5f48 100644 --- a/pkg/cfn/builder/api_test.go +++ b/pkg/cfn/builder/api_test.go @@ -242,16 +242,16 @@ var _ = Describe("CloudFormation template builder API", func() { Endpoint: endpoint, } - volType := api.NodeVolumeTypeIO1 - volSize := 2 - cfg.AvailabilityZones = testAZs ng.Name = "ng-abcd1234" ng.InstanceType = "t2.medium" ng.AMIFamily = "AmazonLinux2" - ng.VolumeSize = &volSize - ng.VolumeType = &volType - ng.VolumeName = "/dev/xvda" + ng.VolumeSize = new(int) + *ng.VolumeSize = 2 + ng.VolumeType = new(string) + *ng.VolumeType = api.NodeVolumeTypeIO1 + ng.VolumeName = new(string) + *ng.VolumeName = "/dev/xvda" if withFullVPC { cfg.VPC = testVPC() @@ -325,8 +325,6 @@ var _ = Describe("CloudFormation template builder API", func() { } Describe("GetAllOutputsFromClusterStack", func() { - volSize := 2 - volType := api.NodeVolumeTypeIO1 expected := &api.ClusterConfig{ TypeMeta: api.ClusterConfigTypeMeta(), Metadata: &api.ClusterMeta{ @@ -357,9 +355,9 @@ var _ = Describe("CloudFormation template builder API", func() { AttachIDs: []string{}, }, DesiredCapacity: nil, - VolumeSize: &volSize, - VolumeType: &volType, - VolumeName: "/dev/xvda", + VolumeSize: aws.Int(2), + VolumeType: aws.String(api.NodeVolumeTypeIO1), + VolumeName: aws.String("/dev/xvda"), IAM: &api.NodeGroupIAM{ WithAddonPolicies: api.NodeGroupIAMAddonPolicies{ ImageBuilder: api.Disabled(), @@ -955,7 +953,7 @@ var _ = Describe("CloudFormation template builder API", func() { Context("NodeGroupEBS", func() { cfg, ng := newClusterConfigAndNodegroup(true) - *ng.VolumeSize = 0 + ng.VolumeSize = nil ng.IAM.WithAddonPolicies.EBS = api.Enabled() build(cfg, "eksctl-test-ebs-cluster", ng) @@ -1000,7 +998,7 @@ var _ = Describe("CloudFormation template builder API", func() { Context("NodeGroupFSX", func() { cfg, ng := newClusterConfigAndNodegroup(true) - *ng.VolumeSize = 0 + ng.VolumeSize = nil ng.IAM.WithAddonPolicies.FSX = api.Enabled() build(cfg, "eksctl-test-fsx-cluster", ng) @@ -1034,7 +1032,7 @@ var _ = Describe("CloudFormation template builder API", func() { Context("NodeGroupEFS", func() { cfg, ng := newClusterConfigAndNodegroup(true) - *ng.VolumeSize = 0 + ng.VolumeSize = nil ng.IAM.WithAddonPolicies.EFS = api.Enabled() build(cfg, "eksctl-test-efs-cluster", ng) diff --git a/pkg/cfn/builder/nodegroup.go b/pkg/cfn/builder/nodegroup.go index 35ac2facbb..f522a3c12c 100644 --- a/pkg/cfn/builder/nodegroup.go +++ b/pkg/cfn/builder/nodegroup.go @@ -126,11 +126,11 @@ func (n *NodeGroupResourceSet) addResourcesForNodeGroup() error { launchTemplateData.KeyName = gfn.NewString(*n.spec.SSH.PublicKeyName) } - if *n.spec.VolumeSize > 0 { + if volumeSize := n.spec.VolumeSize; volumeSize != nil && *volumeSize > 0 { launchTemplateData.BlockDeviceMappings = []gfn.AWSEC2LaunchTemplate_BlockDeviceMapping{{ - DeviceName: gfn.NewString(n.spec.VolumeName), + DeviceName: gfn.NewString(*n.spec.VolumeName), Ebs: &gfn.AWSEC2LaunchTemplate_Ebs{ - VolumeSize: gfn.NewInteger(*n.spec.VolumeSize), + VolumeSize: gfn.NewInteger(*volumeSize), VolumeType: gfn.NewString(*n.spec.VolumeType), }, }}