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.

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

WIP: Nodegroup as a resource #281

Closed
wants to merge 1 commit into from
Closed

Conversation

richardcase
Copy link
Contributor

@richardcase richardcase commented Oct 24, 2018

Description

This is WIP for adding the following:

  • eksctl get nodegroup
  • eksctl create nodegroup
  • eksctl delete nodegroup

Tasks to complete

  • Create get command
  • Test get command
  • Create create command
  • Test create command
  • Create delete command
  • Test delete command
  • Update delete cluster command
  • Update scale nodegroup command
  • Add support for node labels
  • Add integration tests to cover nodegroup lifecycle
  • Update documentation

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

pkg/ctl/create/nodegroup.go Show resolved Hide resolved
pkg/ctl/get/nodegroup.go Show resolved Hide resolved
pkg/ctl/create/nodegroup.go Outdated Show resolved Hide resolved
pkg/ctl/create/nodegroup.go Outdated Show resolved Hide resolved
Signed-off-by: Richard Case <richard.case@outlook.com>
@errordeveloper
Copy link
Contributor

@richardcase #293 is probably relevant for creation, especially in the context of #232; but neither have to be required for initial implementation (we can fix such limitations separately, if solution is not obvious).

}
fs.DurationVar(&cfg.WaitTimeout, "timeout", api.DefaultWaitTimeout, "max wait time in any polling operations")
//TODO: is this needed for nodegroup creation????
fs.StringSliceVar(&availabilityZones, "zones", nil, "(auto-select if unspecified)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this would be needed (see #232). When unspecified it'd be inherited from the cluster stack.

@richardcase
Copy link
Contributor Author

@errordeveloper - i'm still working on this. Taking me a little longer than i ever anticipated due to starting a new job.

@mumoshu
Copy link
Contributor

mumoshu commented Nov 30, 2018

@richardcase Hey! Can I take this?

// Btw, thanks for your awesome work ☺️

@richardcase
Copy link
Contributor Author

Hi @mumoshu - that would be great if you could take it, thanks a lot 👍 I've started a new job and struggling with time.

Do you want to work against this branch/pr or would you like to start clean?

@mumoshu
Copy link
Contributor

mumoshu commented Nov 30, 2018

@richardcase Thanks for the response!

I'd rather keep this PR as-is for reference. But I do want to write a big credit/acknowledgement on your work in my PR.

Would it sounds ok to you?

mumoshu pushed a commit to mumoshu/eksctl that referenced this pull request Dec 3, 2018
This is a successor of eksctl-io#281 but rebased onto the eksctl-io#328

The original credit goes to Richard Case <richard.case@outlook.com>

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@mumoshu mumoshu mentioned this pull request Dec 3, 2018
7 tasks
return err
}
ng.ID = maxSeq + 1
logger.Info("will create a Cloudformation stack for nodegroup %d for cluster %s", ng.ID, cfg.ClusterName)
Copy link
Contributor

@mumoshu mumoshu Dec 3, 2018

Choose a reason for hiding this comment

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

@richardcase This seems to result in CreateNodeGroups trying to create all the stacks for all the node groups, not only the stack for the specified node gorup ng *api.NodeGroup.

So, shouldn't we change this to work against the single node group per each create nodegroup command?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the code would look something like err := stackManager.CreateNodeGroup(ng)

if err != nil {
return err
}
ng.ID = maxSeq + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks after you create a nodegroup and then delete it. We'd better use a semi-random name other than an auto-incremented index. UUID can be used for example, but I'd use the same fancy names used for cluster names.


Let's say you had firstly created a nodegroup with ID=0(=maxSeq + 1), the subsequent create nodegroup sees maxSeq as 1! Which breaks assumptions in the codebase and results in index out of bound errors.

This happens because CloudFormation DescribeStacks API returns stacks in DELETE_COMPLETE for a while(maybe several to dozen minutes). See the StackStatus: "DELETE_COMPLETE", part of the example API response below:

2018-12-04T17:05:59+09:00 [▶]  stacks = [{
  Capabilities: ["CAPABILITY_IAM"],
  CreationTime: 2018-12-04 07:21:34.572 +0000 UTC,
  DeletionTime: 2018-12-04 07:49:49.204 +0000 UTC,
  Description: "EKS nodes (AMI family: AmazonLinux2, SSH access: false, subnet topology: Public) [created and managed by eksctl]",
  DisableRollback: false,
  Outputs: [{
      ExportName: "eksctl-adorable-sheepdog-1543570021-nodegroup-0::InstanceRoleARN",
      OutputKey: "InstanceRoleARN",
      OutputValue: "arn:aws:iam::ACCOUNT:role/eksctl-adorable-sheepdog-15435700-NodeInstanceRole-9D8J0JPOZOA"
    }],
  RollbackConfiguration: {

  },
  StackId: "arn:aws:cloudformation:us-west-2:ACCOUNT:stack/eksctl-adorable-sheepdog-1543570021-nodegroup-0/3aed7b30-f795-11e8-99dc-500c337110fd",
  StackName: "eksctl-adorable-sheepdog-1543570021-nodegroup-0",
  StackStatus: "DELETE_COMPLETE",
  Tags: [{
      Key: "eksctl.cluster.k8s.io/v1alpha1/cluster-name",
      Value: "adorable-sheepdog-1543570021"
    },{
      Key: "eksctl.cluster.k8s.io/v1alpha1/nodegroup-id",
      Value: "0"
    }]
}]

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: So I was able to fix the index out of bound errors just by replacing a bunch of ng.ID that are used to drill down ClusterConfig to a single node group inside it, to use api.NodeGroup instead.

I'll keep it going with ng.ID = maxSeq + 1 for now.

mumoshu added a commit to mumoshu/eksctl that referenced this pull request Dec 22, 2018
`eksctl` now allows you to manage any number of nodegroups other than the initial nodegroup.

Changes:

- `eksctl create nodegroup --cluster CLUSTER_NAME [NODEGROUP_NAME]` is added

  Creates an additional nodegroup.
  The nodegroup name is randomly generated when omitted.

- `eksctl get nodegroup --cluster CLUSTER_NAME` is added

  Lists all the nodegroups including the initial one and the additional ones.

- `eksctl delete nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` is added

  Deletes a nodegroup by name.

- `eksctl create cluster` has been changed to accept an optional `--nodegroup NODEGROUP_NAME` that specifies the nodegroup name.

- `eksctl delete cluster CLUSTER_NAME` has been changed to recursively delete all the nodegroups including additional ones.

- `eksctl scale nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` has been changed to accept the target nodegroup name as the second argument

Checklist:

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

Acknowledgements:

This is a successor of eksctl-io#281 and eksctl-io#332.

All the original credits goes to Richard Case <richard.case@outlook.com> who has started eksctl-io#281. Thanks a lot, Richard!

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@mumoshu mumoshu mentioned this pull request Dec 22, 2018
3 tasks
mumoshu added a commit to mumoshu/eksctl that referenced this pull request Dec 22, 2018
`eksctl` now allows you to manage any number of nodegroups other than the initial nodegroup.

Changes:

- `eksctl create nodegroup --cluster CLUSTER_NAME [NODEGROUP_NAME]` is added

  Creates an additional nodegroup.
  The nodegroup name is randomly generated when omitted.

- `eksctl get nodegroup --cluster CLUSTER_NAME` is added

  Lists all the nodegroups including the initial one and the additional ones.

- `eksctl delete nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` is added

  Deletes a nodegroup by name.

- `eksctl create cluster` has been changed to accept an optional `--nodegroup NODEGROUP_NAME` that specifies the nodegroup name.

- `eksctl delete cluster CLUSTER_NAME` has been changed to recursively delete all the nodegroups including additional ones.

- `eksctl scale nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` has been changed to accept the target nodegroup name as the second argument

Checklist:

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

Acknowledgements:

This is a successor of eksctl-io#281 and eksctl-io#332.

All the original credits goes to Richard Case <richard.case@outlook.com> who has started eksctl-io#281. Thanks a lot, Richard!

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
errordeveloper pushed a commit to mumoshu/eksctl that referenced this pull request Dec 24, 2018
`eksctl` now allows you to manage any number of nodegroups other than the initial nodegroup.

Changes:

- `eksctl create nodegroup --cluster CLUSTER_NAME [NODEGROUP_NAME]` is added

  Creates an additional nodegroup.
  The nodegroup name is randomly generated when omitted.

- `eksctl get nodegroup --cluster CLUSTER_NAME` is added

  Lists all the nodegroups including the initial one and the additional ones.

- `eksctl delete nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` is added

  Deletes a nodegroup by name.

- `eksctl create cluster` has been changed to accept an optional `--nodegroup NODEGROUP_NAME` that specifies the nodegroup name.

- `eksctl delete cluster CLUSTER_NAME` has been changed to recursively delete all the nodegroups including additional ones.

- `eksctl scale nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` has been changed to accept the target nodegroup name as the second argument

Checklist:

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

Acknowledgements:

This is a successor of eksctl-io#281 and eksctl-io#332.

All the original credits goes to Richard Case <richard.case@outlook.com> who has started eksctl-io#281. Thanks a lot, Richard!

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
errordeveloper pushed a commit to mumoshu/eksctl that referenced this pull request Dec 27, 2018
`eksctl` now allows you to manage any number of nodegroups other than the initial nodegroup.

Changes:

- `eksctl create nodegroup --cluster CLUSTER_NAME [NODEGROUP_NAME]` is added

  Creates an additional nodegroup.
  The nodegroup name is randomly generated when omitted.

- `eksctl get nodegroup --cluster CLUSTER_NAME` is added

  Lists all the nodegroups including the initial one and the additional ones.

- `eksctl delete nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` is added

  Deletes a nodegroup by name.

- `eksctl create cluster` has been changed to accept an optional `--nodegroup NODEGROUP_NAME` that specifies the nodegroup name.

- `eksctl delete cluster CLUSTER_NAME` has been changed to recursively delete all the nodegroups including additional ones.

- `eksctl scale nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` has been changed to accept the target nodegroup name as the second argument

Checklist:

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

Acknowledgements:

This is a successor of eksctl-io#281 and eksctl-io#332.

All the original credits goes to Richard Case <richard.case@outlook.com> who has started eksctl-io#281. Thanks a lot, Richard!

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
errordeveloper pushed a commit to mumoshu/eksctl that referenced this pull request Dec 27, 2018
`eksctl` now allows you to manage any number of nodegroups other than the initial nodegroup.

Changes:

- `eksctl create nodegroup --cluster CLUSTER_NAME [NODEGROUP_NAME]` is added

  Creates an additional nodegroup.
  The nodegroup name is randomly generated when omitted.

- `eksctl get nodegroup --cluster CLUSTER_NAME` is added

  Lists all the nodegroups including the initial one and the additional ones.

- `eksctl delete nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` is added

  Deletes a nodegroup by name.

- `eksctl create cluster` has been changed to accept an optional `--nodegroup NODEGROUP_NAME` that specifies the nodegroup name.

- `eksctl delete cluster CLUSTER_NAME` has been changed to recursively delete all the nodegroups including additional ones.

- `eksctl scale nodegroup --cluster CLUSTER_NAME NODEGROUP_NAME` has been changed to accept the target nodegroup name as the second argument

Checklist:

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

Acknowledgements:

This is a successor of eksctl-io#281 and eksctl-io#332.

All the original credits goes to Richard Case <richard.case@outlook.com> who has started eksctl-io#281. Thanks a lot, Richard!

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@errordeveloper errordeveloper deleted the nodegroup-resource branch March 1, 2019 07:20
torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants