Refactor the main API struct #283

errordeveloper · 2018-10-24T17:59:18Z

Description

These changes will allow for:

multiple nodegroups (EKS Node Groups update proposal #100, nodegroup-scoped SSH keys #144, nodegroup as a resource #257, Add option to launch all nodes into the same AZ #232)
public and private subnets (Ability to create custom VPC much like KOPS does #279, Can use existing vpc and subnets #42)

This is a big change, but it shouldn't affect user-visible functionality at all.

Checklist

Code compiles correctly (i.e make build)
All tests passing (i.e. make test)

errordeveloper · 2018-10-24T18:08:05Z

I'm running integration tests now. Would be great to see some reviews, as it's a big change, and we can use the opportunity to rename/restructure things etc.

pkg/ctl/create/cluster.go

@@ -32,7 +32,8 @@ var (
 )

 func createClusterCmd() *cobra.Command {
-	cfg := &api.ClusterConfig{}
+	cfg := api.NewClusterConfig()
+	ng := &cfg.NodeGroups[0]


errordeveloper · 2018-10-24T20:12:05Z

Marked WIP as integration tests failed.

pkg/cfn/builder/nodegroup.go

errordeveloper · 2018-10-24T20:51:28Z

I think there is still a fair bit of refactoring needed to consistently represent multiple nodegroups, I'll try to have a go at it tomorrow, but it might be a thing for another PR.

errordeveloper · 2018-10-24T21:11:40Z

Actually, it turned to be easy enough to remove single "initial" nodegroup assumption, but comments and log messages still need to be updated.

pkg/cfn/manager/nodegroup.go

errordeveloper · 2018-10-25T09:48:03Z

Need to re-run integration tests, and if all is well will remove WIP tag.

errordeveloper · 2018-10-25T11:16:08Z

Turns out ListReadyStacks was the issue. It didn't return nodegroup stack because its status was UPDATE_COMPLETED, and ListReadyStacks doesn't handle those yet - not sure if it should.
Here, we need any kind of stacks (even the ones with dodgy status, i.e. failed updated or whatever), so we want to list all stacks and filter-out only the ones that had been deleted. I've applied a fix (use ListStacks basically), and re-running integration tests now.

errordeveloper · 2018-10-25T11:42:33Z

=== RUN   TestCreateIntegration
Running Suite: (Integration) Create, Get, Scale & Delete
========================================================
Random Seed: 1540465964
Will run 12 of 12 specs

• [SLOW TEST:891.016 seconds]
(Integration) Create, Get, Scale & Delete
/Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:51
  when creating a cluster with 1 node
  /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:70
    should not return an error
    /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:71
------------------------------
•••
------------------------------
• [SLOW TEST:12.745 seconds]
(Integration) Create, Get, Scale & Delete
/Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:51
  when creating a cluster with 1 node
  /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:70
    and we create a deployment using kubectl
    /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:126
      should deploy the service to the cluster
      /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:142
------------------------------
••
------------------------------
• [SLOW TEST:96.492 seconds]
(Integration) Create, Get, Scale & Delete
/Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:51
  when creating a cluster with 1 node
  /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:70
    and scale the cluster
    /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:187
      should not return an error
      /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:188
------------------------------
• [SLOW TEST:33.326 seconds]
(Integration) Create, Get, Scale & Delete
/Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:51
  when creating a cluster with 1 node
  /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:70
    and scale the cluster
    /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:187
      should make it 2 nodes total
      /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:206
------------------------------
• [SLOW TEST:718.169 seconds]
(Integration) Create, Get, Scale & Delete
/Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:51
  when creating a cluster with 1 node
  /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:70
    and deleting the cluster
    /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:219
      should not return an error
      /Users/ilya/Code/eksctl/src/github.com/weaveworks/eksctl/integration/creategetdelete_test.go:220
------------------------------
••
Ran 12 of 12 Specs in 1757.606 seconds
SUCCESS! -- 12 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestCreateIntegration (1757.61s)
PASS
ok      github.com/weaveworks/eksctl/integration        1757.643s

pkg/eks/api/api.go

+
+// HasSuffiecentPublicSubnets validates if there is a suffiecent
+// number of subnets available to create a cluster
+func (c ClusterVPC) HasSuffiecentPublicSubnets() bool {


This will accommodate for - multiple nodegroups - public and private subnets - use taskmanager tasks for nodegroup(s) deletion - cleanup nodegroup terminology

stefanprodan

LGTM Great work Ilya!

richardcase

This looks really good, a great change that will help a lot with future changes. The structual and naming changes are a real benefit. Great work @errordeveloper 🍾

pkg/cfn/manager/tasks.go

-type task func(chan error) error
+type task struct {
+	call func(chan error, interface{}) error
+	data interface{}


pkg/ctl/create/cluster.go

@@ -32,13 +32,14 @@ var (
 )

 func createClusterCmd() *cobra.Command {
-	cfg := &api.ClusterConfig{}
+	cfg := api.NewClusterConfig()
+	ng := cfg.NewNodeGroup()


pkg/ctl/delete/cluster.go

@@ -89,13 +99,13 @@ func doDeleteCluster(cfg *api.ClusterConfig, name string) error {

 	if clusterErr {
 		if handleIfError(ctl.DeprecatedDeleteControlPlane(), "control plane") {
-			handleIfError(stackManager.DeprecatedDeleteStackControlPlane(waitDelete), "stack control plane")
+			handleIfError(stackManager.DeprecatedDeleteStackControlPlane(waitDelete), "stack control plane (deprecated)")


pkg/ctl/scale/nodegroup.go

@@ -28,7 +29,7 @@ func scaleNodeGroupCmd() *cobra.Command {

 	fs.StringVarP(&cfg.ClusterName, "name", "n", "", "EKS cluster name")

-	fs.IntVarP(&cfg.Nodes, "nodes", "N", -1, "total number of nodes (scale to this number)")
+	fs.IntVarP(&ng.DesiredCapacity, "nodes", "N", -1, "total number of nodes (scale to this number)")


errordeveloper · 2018-10-26T08:32:47Z

@stefanprodan @richardcase thanks for the reviews!

Make storageclass paramter case in-sensitive

errordeveloper requested review from richardcase, Lazyshot and stefanprodan October 24, 2018 18:07

errordeveloper commented Oct 24, 2018

View reviewed changes

errordeveloper changed the title ~~Refactor the main API struct~~ WIP: Refactor the main API struct Oct 24, 2018

errordeveloper force-pushed the refactor-cluster-config branch from aff744a to 216a104 Compare October 24, 2018 20:14

golangcibot reviewed Oct 24, 2018

View reviewed changes

pkg/cfn/builder/nodegroup.go Show resolved Hide resolved

errordeveloper force-pushed the refactor-cluster-config branch from 216a104 to 5579070 Compare October 24, 2018 20:50

errordeveloper force-pushed the refactor-cluster-config branch from 5579070 to 91d685d Compare October 24, 2018 20:52

errordeveloper force-pushed the refactor-cluster-config branch 2 times, most recently from 92d32e4 to d0a6007 Compare October 25, 2018 07:56

golangcibot reviewed Oct 25, 2018

View reviewed changes

pkg/cfn/manager/nodegroup.go Outdated Show resolved Hide resolved

errordeveloper force-pushed the refactor-cluster-config branch 5 times, most recently from 48d8cec to 7f6bb22 Compare October 25, 2018 09:05

This comment has been minimized.

# to view

errordeveloper force-pushed the refactor-cluster-config branch from 7f6bb22 to efad73a Compare October 25, 2018 11:12

errordeveloper force-pushed the refactor-cluster-config branch from efad73a to dfccefb Compare October 25, 2018 11:32

errordeveloper changed the title ~~WIP: Refactor the main API struct~~ Refactor the main API struct Oct 25, 2018

errordeveloper mentioned this pull request Oct 25, 2018

WIP: More VPC features #285

Closed

2 tasks

Lazyshot reviewed Oct 26, 2018

View reviewed changes

pkg/eks/api/api.go Outdated

// HasSuffiecentPublicSubnets validates if there is a suffiecent

// number of subnets available to create a cluster

func (c ClusterVPC) HasSuffiecentPublicSubnets() bool {

This comment was marked as abuse.

# to view

This comment was marked as abuse.

# to view

Refactor the main API struct

9c0d8a6

This will accommodate for - multiple nodegroups - public and private subnets - use taskmanager tasks for nodegroup(s) deletion - cleanup nodegroup terminology

errordeveloper force-pushed the refactor-cluster-config branch from dfccefb to 9c0d8a6 Compare October 26, 2018 06:28

stefanprodan approved these changes Oct 26, 2018

View reviewed changes

richardcase approved these changes Oct 26, 2018

View reviewed changes

errordeveloper merged commit 7339396 into master Oct 26, 2018

errordeveloper deleted the refactor-cluster-config branch October 26, 2018 08:33

errordeveloper mentioned this pull request Oct 26, 2018

Add --vpc-cidr flag #286

Merged

4 tasks

torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022

Merge pull request eksctl-io#283 from leakingtapan/storageclassparam

e539e1b

Make storageclass paramter case in-sensitive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the main API struct #283

Refactor the main API struct #283

errordeveloper commented Oct 24, 2018 •

edited

Loading

errordeveloper commented Oct 24, 2018 •

edited

Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

errordeveloper commented Oct 24, 2018

errordeveloper commented Oct 24, 2018

errordeveloper commented Oct 24, 2018

errordeveloper commented Oct 25, 2018

This comment has been minimized.

errordeveloper commented Oct 25, 2018

errordeveloper commented Oct 25, 2018

This comment was marked as abuse.

This comment was marked as abuse.

stefanprodan left a comment

richardcase left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

errordeveloper commented Oct 26, 2018

Refactor the main API struct #283

Refactor the main API struct #283

Conversation

errordeveloper commented Oct 24, 2018 • edited Loading

Description

Checklist

errordeveloper commented Oct 24, 2018 • edited Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

errordeveloper commented Oct 24, 2018

errordeveloper commented Oct 24, 2018

errordeveloper commented Oct 24, 2018

errordeveloper commented Oct 25, 2018

This comment has been minimized.

errordeveloper commented Oct 25, 2018

errordeveloper commented Oct 25, 2018

This comment was marked as abuse.

This comment was marked as abuse.

stefanprodan left a comment

Choose a reason for hiding this comment

richardcase left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

errordeveloper commented Oct 26, 2018

errordeveloper commented Oct 24, 2018 •

edited

Loading

errordeveloper commented Oct 24, 2018 •

edited

Loading