Flexible instance types #314

JoshVanL · 2018-06-18T20:00:43Z

What this PR does / why we need it:
Although users are already able to chose their own instance types, this validates the supported types and restricts the types available to master

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #310

NONE

JoshVanL · 2018-06-18T20:04:16Z

This adds some validation to the instance types and restricts the master instance types.
/cc @MattiasGees
What do you think?

MattiasGees

@JoshVanL general gist. I think this is what we want. I made some extra comments.

It would also be great if we can document this feature in the docs.

MattiasGees · 2018-06-19T08:26:20Z

pkg/tarmak/cluster/cluster.go

-// Verify cluster
-func (c *Cluster) Verify() (result error) {
-	return c.VerifyInstancePools()
+	if err := c.Environment().Provider().ValidateInstanceTypes(c.InstancePools()); err != nil {


I think this needs to become a verify check and not an validate check. This will contact AWS and is a more expensive call. This is to keep working in line with #280

MattiasGees · 2018-06-19T08:26:37Z

pkg/tarmak/cluster/cluster.go

-// Verify instance pools
-func (c *Cluster) VerifyInstancePools() (result error) {
+// Verify cluster
+func (c *Cluster) Verify() error {


Why remove the VerifyInstancePools?

MattiasGees · 2018-06-19T08:28:12Z

pkg/tarmak/provider/amazon/amazon.go

+	return []string{"io1", "gp2", "st1", "sc1"}
+}
+
+func (a *Amazon) awsInstanceTypes() []string {


I think you are missing some newer instance types, is there a way to make this dynamic and check this through an AWS call?

I've had a look and I can't find a straight forward way of achieving this. I've updated this list though

MattiasGees · 2018-06-19T08:29:46Z

pkg/tarmak/provider/amazon/amazon.go

 		}

 		if err := a.verifyInstanceType(instanceType, instance.Zones(), svc); err != nil {
 			result = multierror.Append(result, err)
 		}
+
+		if instance.Name() == "master" {


I think we also should do a check for vault and etcd. I don't think we should run these on burstable (t) types.

MattiasGees · 2018-06-21T10:29:50Z

I tested this and this works great.
I encountered one problem. When we use the default settings we assign to the instances atm we get verification errors due to small instance types :). So I think we should adjust that.

Currently we set etcd to small and vault to tiny which translates to an t2.medium and t2.nano.

For etcd I would switch the default to an m5.large but for vault I am not sure we should go that big as we only request SSL certs from time to time and don't do anything heavy on it.

JoshVanL · 2018-06-21T11:46:56Z

I have bumped up etcd instances to use medium (m4.large) as default at init. I think there should be some consensus though on these sizes and what vault should have / be burstable.

MattiasGees · 2018-06-21T13:29:22Z

@JoshVanL I discussed this a bit with @simonswine. We agreed that denying of certain instance types shouldn't happen.
As for development environments we can see it happen that you run way less and that a burst-able type could work. We agreed instead of giving an error that a warning like It is not advised to use t2.nano for etcd would be better. Is that something we can implement in an easy way?

JoshVanL · 2018-06-21T13:33:33Z

@MattiasGees yes that can be done easily

MattiasGees · 2018-06-21T16:30:14Z

/lgtm

MattiasGees · 2018-06-22T16:25:01Z

@JoshVanL I tried bringing up a cluster with smallest instances for etcd and master, but couldn't see any warnings displayed. So I think there is something wrong with the warning logger.

JoshVanL · 2018-06-25T13:01:54Z

@MattiasGees
Thanks Mattias for spotting, should be fixed now

MattiasGees · 2018-06-26T09:14:45Z

Works now
/lgtm

jetstack-bot · 2018-07-10T08:24:21Z

New changes are detected. LGTM label has been removed.

JoshVanL · 2018-07-10T08:26:41Z

/assign @simonswine

jetstack-bot · 2018-07-31T07:48:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: simonswine

If they are not already assigned, you can assign the PR to them by writing /assign @simonswine in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

jetstack-bot · 2018-09-15T15:44:46Z

@JoshVanL: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jetstack-bot · 2019-04-11T14:29:44Z

@JoshVanL: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
tarmak-puppet-module-tarmak-acceptance-1-14-centos	`650ca81`	link	`/test puppet-tarmak-acceptance-centos v1.14`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 18, 2018

jetstack-bot requested a review from simonswine June 18, 2018 20:00

MattiasGees suggested changes Jun 19, 2018

View reviewed changes

JoshVanL force-pushed the 310-flexible-instance-type branch from f9af401 to 33d4de2 Compare June 20, 2018 18:55

JoshVanL changed the title ~~WIP: Flexible instance type~~ Flexible instance type Jun 20, 2018

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2018

JoshVanL changed the title ~~Flexible instance type~~ Flexible instance types Jun 20, 2018

jetstack-bot assigned MattiasGees Jun 21, 2018

jetstack-bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jun 21, 2018

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2018

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2018

JoshVanL force-pushed the 310-flexible-instance-type branch from d495954 to cef7b12 Compare July 10, 2018 08:24

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2018

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2018

jetstack-bot assigned simonswine Jul 10, 2018

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2018

JoshVanL force-pushed the 310-flexible-instance-type branch from cef7b12 to 0e6576b Compare July 11, 2018 08:56

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2018

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2018

JoshVanL force-pushed the 310-flexible-instance-type branch from 0e6576b to 48c42e9 Compare July 31, 2018 07:48

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2018

jetstack-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Sep 7, 2018

JoshVanL force-pushed the 310-flexible-instance-type branch from 48c42e9 to f606621 Compare September 10, 2018 08:06

JoshVanL added 8 commits September 11, 2018 09:20

Include list of possible instance types for validation

52ed47c

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Adds validation for volumes and master instance types

3ced6e8

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Verify -> Validate and use result.ErrorOrNil where appropriate

533124a

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Adds review changes

4f686e9

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Update etcd instance to use medium size by default

bd07cb0

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Warn instead of error on unadvised instance types

97d3972

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Make use of type constants and compare type not name

8a16f84

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Fixed amazon verification string matching

650ca81

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL force-pushed the 310-flexible-instance-type branch from f606621 to 650ca81 Compare September 11, 2018 08:21

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2018

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flexible instance types #314

Flexible instance types #314

JoshVanL commented Jun 18, 2018

JoshVanL commented Jun 18, 2018 •

edited

MattiasGees left a comment

MattiasGees Jun 19, 2018

JoshVanL Jun 20, 2018

MattiasGees Jun 19, 2018

JoshVanL Jun 20, 2018

MattiasGees Jun 19, 2018

JoshVanL Jun 20, 2018

MattiasGees Jun 19, 2018

JoshVanL Jun 20, 2018

MattiasGees commented Jun 21, 2018

JoshVanL commented Jun 21, 2018

MattiasGees commented Jun 21, 2018

JoshVanL commented Jun 21, 2018

MattiasGees commented Jun 21, 2018

MattiasGees commented Jun 22, 2018

JoshVanL commented Jun 25, 2018

MattiasGees commented Jun 26, 2018

jetstack-bot commented Jul 10, 2018

JoshVanL commented Jul 10, 2018

jetstack-bot commented Jul 31, 2018

jetstack-bot commented Sep 15, 2018

jetstack-bot commented Apr 11, 2019

Flexible instance types #314

Are you sure you want to change the base?

Flexible instance types #314

Conversation

JoshVanL commented Jun 18, 2018

JoshVanL commented Jun 18, 2018 • edited

MattiasGees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattiasGees commented Jun 21, 2018

JoshVanL commented Jun 21, 2018

MattiasGees commented Jun 21, 2018

JoshVanL commented Jun 21, 2018

MattiasGees commented Jun 21, 2018

MattiasGees commented Jun 22, 2018

JoshVanL commented Jun 25, 2018

MattiasGees commented Jun 26, 2018

jetstack-bot commented Jul 10, 2018

JoshVanL commented Jul 10, 2018

jetstack-bot commented Jul 31, 2018

jetstack-bot commented Sep 15, 2018

jetstack-bot commented Apr 11, 2019

JoshVanL commented Jun 18, 2018 •

edited