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

feat: Nodegroup as a resource #369

Merged
merged 4 commits into from
Dec 28, 2018

Conversation

mumoshu
Copy link
Contributor

@mumoshu mumoshu commented 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:

  • 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

Acknowledgements:

This is a successor of #281 and #332.

All the original credits goes to Richard Case [email protected] who has started #281. Thanks a lot, Richard!

Signed-off-by: Yusuke Kuoka [email protected]

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.

Thanks for wrapping this up @mumoshu! I'd love to get it in ASAP, and due to holidays and timezones I'm going to address my review comments myself.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pkg/cfn/builder/nodegroup.go Outdated Show resolved Hide resolved
pkg/ctl/create/cluster.go Outdated Show resolved Hide resolved
pkg/ctl/delete/delete.go Outdated Show resolved Hide resolved
pkg/ctl/get/get.go Outdated Show resolved Hide resolved
pkg/ctl/get/get.go.orig Outdated Show resolved Hide resolved
pkg/ctl/scale/nodegroup.go.orig Outdated Show resolved Hide resolved
pkg/eks/api/api.go Show resolved Hide resolved
pkg/ctl/cmdutils/nodegroup.go Outdated Show resolved Hide resolved
@errordeveloper
Copy link
Contributor

I've started making changes, but looks like I might now be able to finish this today and need to switch to doing something else. I will continue after the holidays, i.e. most likely on Friday.

@errordeveloper errordeveloper changed the title feat: Nodegroup as a resource WIP: feat: Nodegroup as a resource Dec 24, 2018
@errordeveloper

This comment has been minimized.

@mumoshu
Copy link
Contributor Author

mumoshu commented Dec 25, 2018

@errordeveloper Thanks for your efforts! I've added small commits hoping they help you focus on bigger things.

Also - I started getting 404 errors from the weaveworks/eksctl project in CircleCI due to unknown reason. Perhaps someone in your org. made the whole CircleCI org private, by any chance?

The build seems broken seeing the status check, but is passing on my machine.

@mumoshu
Copy link
Contributor Author

mumoshu commented Dec 26, 2018

Please cherry-pick #375 to fix Circle builds. See #374 for more context.

@mumoshu
Copy link
Contributor Author

mumoshu commented Dec 26, 2018

look delete cluster code path - there seems to be an infinite loop

@errordeveloper I've reproduced and fixed this in 19f9f1f

@mumoshu
Copy link
Contributor Author

mumoshu commented Dec 26, 2018

fix name of initial nodegroup

I believe this has been fixed in da00fc9, in case that's what you meant.

Sorry for leaving many mistakes but this should be good to go now(again).

@errordeveloper

This comment has been minimized.

@mumoshu

This comment has been minimized.

@errordeveloper

This comment has been minimized.

@errordeveloper errordeveloper force-pushed the nodegroup-as-resource branch 3 times, most recently from f65c323 to 8010128 Compare December 27, 2018 16:48
@errordeveloper

This comment has been minimized.

@errordeveloper
Copy link
Contributor

errordeveloper commented Dec 27, 2018

My final TODOs:

  • decide what to squash
  • re-read the diff
  • run integration tests
  • remove WIP tag
  • merge and cut a release

`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 <[email protected]> who has started eksctl-io#281. Thanks a lot, Richard!

Signed-off-by: Yusuke Kuoka <[email protected]>
@errordeveloper errordeveloper force-pushed the nodegroup-as-resource branch 3 times, most recently from 67a91a8 to 24d6f7f Compare December 27, 2018 20:27
@errordeveloper errordeveloper changed the title WIP: feat: Nodegroup as a resource feat: Nodegroup as a resource Dec 28, 2018
@errordeveloper errordeveloper force-pushed the nodegroup-as-resource branch 3 times, most recently from 626c72b to 8022c72 Compare December 28, 2018 11:51
}

// BlockingWaitDeleteStack kills a stack by name and waits for DELETED status
func (c *StackCollection) BlockingWaitDeleteStack(name 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.

We add Blocking to the name of the old function, because that's what it is actually. We keep using it where it was used previously, but use new non-blocking version above when we need to delete all nodegroups in parallel and run a task for each of the nodegroups.

- move name generator functions into one file make usage consistent
- review stack manager code
- make examples in docs more consistent
- update output message formatting, improve consistency
- update some usage messages, move more flags into cmdutils
- refactor `ctl.GetCredentials`
- refactor stack listers
- do not reset tags when scaling nodegroup
errordeveloper
errordeveloper previously approved these changes Dec 28, 2018
- ensure CGO is always off and netgo is always on
- add musl-dev (somehow cgo still gets dragged in)
- add .dockerignore files
@errordeveloper errordeveloper merged commit c0e754c into eksctl-io:master Dec 28, 2018
@errordeveloper
Copy link
Contributor

🎉

@kylemclaren
Copy link

Been waiting on this feature, thanks to all who made it happen 🎉

torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022
enable users to set ec2-endpoint for nonstandard regions
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.

3 participants