Tweak ec2 instance parsing to include more instance types #11724
Conversation
| @@ -0,0 +1,328 @@ | |||
| // Copyright 2016 Canonical Ltd. | |||
There was a problem hiding this comment.
file was renamed from instance_information.go and some methods from the old internal/ec2instancetypes package copied across.
supportedInstanceTypes() and instancetypeCosts() are new
provider/ec2/instancetypes.go
Outdated
| } | ||
| for _, info := range instTypeResults.InstanceTypes { | ||
| instType := instances.InstanceType{ | ||
| Name: *info.InstanceType, |
There was a problem hiding this comment.
InstanceType is never nil
There was a problem hiding this comment.
Defensive programming would ignore the assumption and just continue the loop if the value is nil.
There was a problem hiding this comment.
added check here and in other places eg zone names
| var token *string | ||
| for { | ||
| offeringResults, err := ec2Session.DescribeInstanceTypeOfferings(&ec2.DescribeInstanceTypeOfferingsInput{ | ||
| LocationType: aws.String("availability-zone"), |
There was a problem hiding this comment.
not sure if this works for "local-zones" but it should
There was a problem hiding this comment.
Filters are OR'ed together so it's fine was my assumption.
| } | ||
| var zoneNames []string | ||
| zoneFilter := &ec2.Filter{Name: aws.String("location")} | ||
| for _, z := range zoneResults.AvailabilityZones { |
There was a problem hiding this comment.
This needs to support local zones like us-west-2-lax-1a or filter them out completely. Or if only local zones support an instance type, mark it as deprecated.
There was a problem hiding this comment.
I am not sure we care about the distinction. The only reason we get zone names is so that we can use them to know when to mark an instance as deprecated (terrible name but it precedes this work). If an instance type is not available across all zones (combination of az plus local), then deprecated = true.
provider/ec2/instancetypes.go
Outdated
| Name: *info.InstanceType, | ||
| VirtType: virtType(info), | ||
| Deprecated: info.CurrentGeneration == nil || !*info.CurrentGeneration, | ||
| Cost: costs[*info.InstanceType], |
There was a problem hiding this comment.
Is the default cost of 0 treated correctly? We should default unknown costs as max uint64 so they are chosen as a last resort.
There was a problem hiding this comment.
Other providers default to 0 but then again they don't have costs for anything.
It's hard to know because in my testing, only 2 instance types didn't get a cost, one being t1.nano, and that's a low cost instance so we'd want to select that if possible.
Maybe using max int is ok
wallyworld
force-pushed
the
aws-instancetypes-update
branch
from
e597616
to
aa05f33
Compare
wallyworld
force-pushed
the
aws-instancetypes-update
branch
from
aa05f33
to
f896b00
Compare
|
|
|
@wallyworld a discourse post/email would be great about this new (great) change |
|
Also note #11725 (review) |
#11725 Merges 2.8 into develop to bring forward: - #11724 from wallyworld/aws-instancetypes-update - #11719 from achilleasa/2.8-support-kvm-for-focal-hosts - #11706 from manadart/2.8-controller-no-set-lld - #11717 from manadart/2.8-no-clean-shutdown-svc - #11715 from SimonRichardson/idle-condition - #11720 from hpidcock/static-link-and-strip - #11701 from hmlanigan/apply-profile-race - #11683 from manadart/2.8-instance-poller-lld - #11700 from SimonRichardson/integration-tests-lxd-profile - #11711 from wallyworld/schemagen-flags - #11707 from howbazaar/2.8-core-model-updates - #11710 from wallyworld/tweak-cmr-tests - #11705 from wallyworld/merge-2.7-20200612 - #11696 from wallyworld/k8s-dashboard-bootstrap
Description of change
The way we got instance type metadata for ec2 was just wrong.
A symptom was that we failed to know about instance types in some regions, and falsely claimed instance types were available when they were not (for a given user). We also needed to hand craft and build into juju a large map each time a new instance type was available.
This PR uses the EC2 API to query available instance types for the credential in use, and also looks up the cost info as well. This is cached on the provider and reset when the cloud/cred info changes.
Also, bootstrap --build-agent now takes account of GOARCH so we can test custom agent binaries on different arches.
QA steps
The test case covers the issue in the bug (arm64 m6g instance in ap-northeast-1)
Bug reference
https://bugs.launchpad.net/juju/+bug/1872670