Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Flexible instance types #314

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

JoshVanL
Copy link
Contributor

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

@jetstack-bot 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
@JoshVanL
Copy link
Contributor Author

JoshVanL commented Jun 18, 2018

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

Copy link
Member

@MattiasGees MattiasGees left a comment

Choose a reason for hiding this comment

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

@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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// Verify instance pools
func (c *Cluster) VerifyInstancePools() (result error) {
// Verify cluster
func (c *Cluster) Verify() error {
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the VerifyInstancePools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return []string{"io1", "gp2", "st1", "sc1"}
}

func (a *Amazon) awsInstanceTypes() []string {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

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

if instance.Name() == "master" {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@JoshVanL JoshVanL changed the title WIP: Flexible instance type Flexible instance type Jun 20, 2018
@jetstack-bot 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 JoshVanL changed the title Flexible instance type Flexible instance types Jun 20, 2018
@MattiasGees
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

@MattiasGees yes that can be done easily

@MattiasGees
Copy link
Member

/lgtm

@jetstack-bot 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
@MattiasGees
Copy link
Member

@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
Copy link
Contributor Author

@MattiasGees
Thanks Mattias for spotting, should be fixed now

@MattiasGees
Copy link
Member

Works now
/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2018
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2018
@jetstack-bot jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2018
@jetstack-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

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

/assign @simonswine

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2018
@jetstack-bot 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 jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2018
@jetstack-bot
Copy link
Collaborator

[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:

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

@jetstack-bot 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 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
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 10, 2018
@jetstack-bot 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
Copy link
Collaborator

@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 jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2018
@jetstack-bot
Copy link
Collaborator

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Be more flexible for instance type
4 participants