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

Create cluster via config file #381

Merged
merged 11 commits into from
Jan 7, 2019
Merged

Create cluster via config file #381

merged 11 commits into from
Jan 7, 2019

Conversation

errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper commented Jan 2, 2019

Description

This is initial step towards #19. Let's add this in to start with and aim to evolve it as an experimental feature.

This change adds some of the Kubernetes API tooling, and adds JSON tags to api.ClusterConfig.
From user's perspective, there is now a --config-file/-f flag in eksctl create cluster. The format of the object right now is based directly on api.ClusterConfig, we may eventually mould it into Cluster API ProvideSpec, but for now let's keep it simple.

Checklist

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

Closes #377.

@errordeveloper
Copy link
Contributor Author

errordeveloper commented Jan 2, 2019

TODOs:

  • squash
  • re-read the diff
  • fix AMI resolution problem
  • make all operation on ng use cfg.NodeGroups instead
  • test each of example configs that are usable directly:
    • examples/01-simple-cluster.yaml
    • examples/02-custom-vpc-cidr-no-nodes.yaml
    • examples/05-two-nodegroups.yaml
  • complete iniline TODOs or move to issues

@errordeveloper errordeveloper force-pushed the cluster-config branch 2 times, most recently from b92286b to 20f68cf Compare January 2, 2019 15:57
// +optional
AvailabilityZones []string `json:"availabilityZones,omitempty"`

// TODO: refactor and move IAM addons to nodegroup
Copy link
Contributor Author

@errordeveloper errordeveloper Jan 2, 2019

Choose a reason for hiding this comment

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

Will save for another PR (see #361 & #363).

@errordeveloper
Copy link
Contributor Author

errordeveloper commented Jan 3, 2019

This PR now has a fix for #385 (f98fcf8).

@errordeveloper errordeveloper force-pushed the cluster-config branch 2 times, most recently from f98fcf8 to 66fd306 Compare January 3, 2019 14:45
// +optional
InstanceRoleARN string `json:"instanceRoleARN,omitempty"` // TODO: read-only

// TODO move to separate struct
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@errordeveloper errordeveloper changed the title WIP: Create cluster via config file Create cluster via config file Jan 3, 2019
@errordeveloper errordeveloper force-pushed the cluster-config branch 3 times, most recently from 5123632 to 8a8dcc4 Compare January 3, 2019 19:15
@errordeveloper
Copy link
Contributor Author

errordeveloper commented Jan 3, 2019

I'm rather happy with this, but would like to re-test examples/05-two-nodegroups.yaml once again, and run the integration suite.

@errordeveloper
Copy link
Contributor Author

Integration tests passed, ./eksctl create cluster -f ./examples/05-two-nodegroups.yaml passed and nodes have right name labels set correctly.

@errordeveloper errordeveloper force-pushed the cluster-config branch 2 times, most recently from 0d66a41 to 41ea017 Compare January 4, 2019 10:07
ng.Labels[api.NodeGroupNameLabel] = ng.Name

for l := range ng.Labels {
if len(strings.Split(l, "/")) > 2 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should actually call proper validation function here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@errordeveloper errordeveloper force-pushed the cluster-config branch 2 times, most recently from 0b82a2c to 29e1992 Compare January 5, 2019 03:15
Copy link
Contributor

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

Just a nit. LGTM overall! Thanks for your efforts ☺️

- Make it incompatible with all flags that modify state of config
- Most default are implemented by constructors (where possible)
- fix bug where tags slice was shared accidentially
- move tag constants to api package
- automatically set node labels for cluster and nodegroup names
- ensure labels have max 1 '/' separator, otherwise kubelet fails to start
- fix missing return on nodegroup deletion error causing false-positive
- ensure describe stack is called with StackId whenever possible, to avoid
  error on stacks that happen to get deleted already
@errordeveloper
Copy link
Contributor Author

errordeveloper commented Jan 6, 2019

@mumoshu thanks for looking over this PR! I've removed stale TODO note, and made DesiredCapacity field optional. Please take another look, whenever you have a moment.

mumoshu
mumoshu previously approved these changes Jan 6, 2019
PrivateNetworking bool `json:"privateNetworking"`

// +optional
DesiredCapacity int `json:"desiredCapacity"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the "desired" capacity not be met?
Would it be clearer to have something named like, e.g., the below?

Count int `json:"count"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we had it as NodeCount at the top level some time ago, but now it's called DesiredCapacity to match the AWS API. However, the flag is still --nodes, so there is a little inconsistency there too. Some refactoring is pending here also (#385).

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough 😅
Matching the AWS API probably takes precedence over any other argument/preference 👍

marccarre
marccarre previously approved these changes Jan 7, 2019
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