Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds support for Kubernetes 1.11 clusters to be created #349

Merged
merged 1 commit into from
Dec 17, 2018
Merged

Adds support for Kubernetes 1.11 clusters to be created #349

merged 1 commit into from
Dec 17, 2018

Conversation

christopherhein
Copy link
Contributor

@christopherhein christopherhein commented Dec 17, 2018 •
edited
Loading

Description

Adds a new flag to allow setting the Kubernetes version and defaults to 1.11

Checklist

  • Code compiles correctly (i.e make build)
  • Added tests that cover your change (if possible)
  • All tests passing (i.e. make test)
  • Added/modified documentation as required (such as the README)
  • Added yourself to the humans.txt file

closes #344

/cc @arun-gupta @errordeveloper @tiffanyfay @nckturner

@errordeveloper
Copy link
Contributor

@christopherhein thank you! I'll review shortly.

@errordeveloper errordeveloper self-assigned this Dec 17, 2018
errordeveloper
errordeveloper reviewed Dec 17, 2018
View reviewed changes
Copy link
Contributor

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christopherhein this is great! I'll rebase and add another commit to address comments above.

README.md Outdated
@@ -85,6 +85,12 @@ To create the same kind of basic cluster, but with a different name, run:
eksctl create cluster --name=cluster-1 --nodes=4
```

Amazon Elastic Container Service for Kubernetes supports two versions `1.10` and `1.11`, with `eksctl` you can deploy either version by passing `--version` this is automatically set to `1.11` if not included.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just say EKS here.

pkg/ami/auto_resolver.go Outdated
@@ -27,7 +27,7 @@ type AutoResolver struct {

// Resolve will return an AMI to use based on the default AMI for
// each region
func (r *AutoResolver) Resolve(region string, instanceType string, imageFamily string) (string, error) {
func (r *AutoResolver) Resolve(region string, version string, instanceType string, imageFamily string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (r *AutoResolver) Resolve(region string, version string, instanceType string, imageFamily string) (string, error) {
func (r *AutoResolver) Resolve(region, version, instanceType, imageFamily string) (string, error) {

pkg/ami/error.go Outdated
instanceType string
imageFamily string
}

// NewErrFailedResolution creates a new instance of ErrFailedResolution for a
// give region, instance type and image family
func NewErrFailedResolution(region string, instanceType string, imageFamily string) *ErrFailedResolution {
func NewErrFailedResolution(region string, version string, instanceType string, imageFamily string) *ErrFailedResolution {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func NewErrFailedResolution(region string, version string, instanceType string, imageFamily string) *ErrFailedResolution {
func NewErrFailedResolution(region, version, instanceType, imageFamily string) *ErrFailedResolution {

pkg/ami/resolver.go Outdated
@@ -8,9 +8,9 @@ var (
// Resolve will resolve an AMI from the supplied region
// and instance type. It will invoke a specific resolver
// to do the actual determining of AMI.
func Resolve(region string, instanceType string, imageFamily string) (string, error) {
func Resolve(region string, version string, instanceType string, imageFamily string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func Resolve(region string, version string, instanceType string, imageFamily string) (string, error) {
func Resolve(region, version, instanceType, imageFamily string) (string, error) {

pkg/ami/resolver.go Outdated
}

// Resolver provides an interface to enable implementing multiple
// ways to determine which AMI to use from the region/instance type/image family.
type Resolver interface {
Resolve(region string, instanceType string, imageFamily string) (string, error)
Resolve(region string, version string, instanceType string, imageFamily string) (string, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Resolve(region string, version string, instanceType string, imageFamily string) (string, error)
Resolve(region, version, instanceType, imageFamily string) (string, error)

pkg/ctl/create/cluster.go Outdated
@@ -116,7 +118,7 @@ func groupFlagsInUsage(cmd *cobra.Command) {
}
groupToFlagSet := make(map[string]*pflag.FlagSet)
for _, g := range groups {
groupToFlagSet[g] = pflag.NewFlagSet(g, /* Unused. Can be anythng. */ pflag.ContinueOnError)
groupToFlagSet[g] = pflag.NewFlagSet(g /* Unused. Can be anythng. */, pflag.ContinueOnError)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs rebase.

pkg/ctl/create/cluster.go
// add default storage class
if cfg.Addons.Storage {
// add default storage class only for version 1.10 clusters
if cfg.Addons.Storage && p.Version == "1.10" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted!

pkg/eks/api.go
@@ -63,6 +63,9 @@ func (p ProviderServices) STS() stsiface.STSAPI { return p.sts }
// Region returns provider-level region setting
func (p ProviderServices) Region() string { return p.spec.Region }

// Version returns provider-level version setting
func (p ProviderServices) Version() string { return p.spec.Version }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I would put this under cluster actually, provider is supposed to be very high-level.

pkg/nodebootstrap/maxpods.go Outdated
@@ -36,11 +36,13 @@ var maxPodsPerNodeType = map[string]int{
"d2.xlarge": 58,
"f1.16xlarge": 394,
"f1.2xlarge": 58,
"f1.4xlarge": 234,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've committed this to master already.

pkg/testutils/mock_provider.go Outdated
@@ -33,6 +33,7 @@ func NewMockProvider() *MockProvider {
var ProviderConfig = &api.ProviderConfig{
Region: api.DefaultEKSRegion,
Profile: "default",
Version: "1.10",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent...

@errordeveloper errordeveloper force-pushed the feature/344-support-1-11 branch from 333b602 to 93405b1 Compare December 17, 2018 13:16
errordeveloper
errordeveloper reviewed Dec 17, 2018
View reviewed changes
pkg/ami/static_resolver_ami.go
@@ -3,25 +3,43 @@ package ami
// This file was generated by static_resolver_ami_generate.go; DO NOT EDIT.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should update generator code instead.

@christopherhein @errordeveloper
Adds support for Kubernetes 1.11 clusters to be created
b339814 
Signed-off-by: Christopher Hein <me@christopherhein.com>
@errordeveloper errordeveloper force-pushed the feature/344-support-1-11 branch from 93405b1 to b339814 Compare December 17, 2018 14:00
errordeveloper
errordeveloper approved these changes Dec 17, 2018
View reviewed changes
Copy link
Contributor

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended-up squashing changes into one commit, anyway - good to go now!

@errordeveloper errordeveloper merged commit d891b06 into eksctl-io:master Dec 17, 2018
@christopherhein christopherhein deleted the feature/344-support-1-11 branch December 17, 2018 16:42
torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022
Merge pull request eksctl-io#349 from leakingtapan/fix
791b290 
Update copyright to 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@errordeveloper errordeveloper errordeveloper approved these changes

Assignees

@errordeveloper errordeveloper

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for Creating EKS Kubernetes v1.11 Clusters
2 participants
@christopherhein @errordeveloper