Query for available instance types #2837
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
justinsb
force-pushed
the
query_for_available_instance_types
branch
from
e045500 to
2667787
Compare
|
Related to #2811 |
|
|
||
| switch ig.Spec.Role { | ||
| case kops.InstanceGroupRoleMaster: | ||
| // Some regions do not (currently) support the m3 family; the c4 large is the cheapest non-burstable instance |
There was a problem hiding this comment.
c4-large is not always the cheapest instance btw. Usually, but sometimes m4.large.
There was a problem hiding this comment.
Right, but we can't change now.
There was a problem hiding this comment.
Why can we change now... ? If we really can't change now- maybe we start by throwing an errata in the logs that lets people know that we are phasing %s instance out in x version. (and in the changelog for 1.7)
There was a problem hiding this comment.
Yeah, I guess we can change with adequate notification/warnings, learning from the root volume size change. But: we might want to see how many people that affects negatively, and I don't want to lump that in with this change.
| // Also some accounts are no longer supporting m3 in us-east-1 zones | ||
| candidates = []string{"m3.medium", "c4.large"} | ||
|
|
||
| // TODO: We used to have logic like the following... |
There was a problem hiding this comment.
Can we remove this and get it into an issue?
| if zones.IsSuperset(igZones) { | ||
| return instanceType, nil | ||
| } else { | ||
| glog.V(2).Infof("can't use instance type %q, available in zones %v but need %v", instanceType, zones, igZones) |
There was a problem hiding this comment.
can't use instance type %q, available in zones %v but need %v
so is this cannot use it or it is not in the az?
|
|
||
| response, err := c.ec2.DescribeReservedInstancesOfferings(request) | ||
| if err != nil { | ||
| return zones, fmt.Errorf("error checking if instance type %q is supported in region %q: %v", instanceType, c.region, err) |
There was a problem hiding this comment.
Should we tell them what we recommend?
There was a problem hiding this comment.
I'm not sure what we recommend in this case?
There was a problem hiding this comment.
return a list of what is available? Not a big deal, and not for this PR. Just a thought
There was a problem hiding this comment.
Ah - this is an actual error querying, but I take the point.
Let's see what happens in the real world here... whether we need special cases here also.
justinsb
force-pushed
the
query_for_available_instance_types
branch
from
2667787 to
ef99e61
Compare
|
Related #2683 (comment) |
|
Related #2771 |
|
Related #157 |
Allow the cloud to determine the instance type, and on AWS we query the available instance types via the reserved instance API.
This change is