Skip to content

Commit

Permalink
PR Comments
Browse files Browse the repository at this point in the history
  • Loading branch information
ellistarn committed Oct 29, 2021
1 parent ad4ab5e commit ac5daa3
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 26 deletions.
2 changes: 1 addition & 1 deletion pkg/cloudprovider/aws/instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func (p *InstanceProvider) launchInstances(ctx context.Context, constraints *v1a
if capacityTypes := constraints.Requirements.Requirement(v1alpha1.CapacityTypeLabel); len(capacityTypes) == 0 {
return nil, fmt.Errorf("invariant violated, must contain at least one capacity type")
} else if len(capacityTypes) == 1 {
capacityType, _ = capacityTypes.PopAny()
capacityType = capacityTypes.UnsortedList()[0]
}
// Get Launch Template Configs, which may differ due to GPU or Architecture requirements
launchTemplateConfigs, err := p.getLaunchTemplateConfigs(ctx, constraints, instanceTypes, capacityType)
Expand Down
39 changes: 18 additions & 21 deletions pkg/cloudprovider/aws/instancetypes.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,16 @@ import (
"github.com/awslabs/karpenter/pkg/cloudprovider"
"github.com/awslabs/karpenter/pkg/cloudprovider/aws/apis/v1alpha1"
"github.com/awslabs/karpenter/pkg/utils/functional"
"github.com/mitchellh/hashstructure/v2"
"github.com/patrickmn/go-cache"
"k8s.io/apimachinery/pkg/util/sets"
"knative.dev/pkg/logging"
)

const (
instanceTypesCacheKey = "types"
instanceTypeZonesCacheKey = "zones"
)

type InstanceTypeProvider struct {
ec2api ec2iface.EC2API
subnetProvider *SubnetProvider
Expand All @@ -44,25 +48,8 @@ func NewInstanceTypeProvider(ec2api ec2iface.EC2API, subnetProvider *SubnetProvi
}
}

// Get all instance types that are available per availability zone
// Get instance type options given the constraints
func (p *InstanceTypeProvider) Get(ctx context.Context, constraints *v1alpha1.Constraints) ([]cloudprovider.InstanceType, error) {
hash, err := hashstructure.Hash(constraints, hashstructure.FormatV2, nil)
if err != nil {
return nil, fmt.Errorf("hashing constraints, %w", err)
}
if cached, ok := p.cache.Get(fmt.Sprint(hash)); ok {
return cached.([]cloudprovider.InstanceType), nil
}
instanceTypes, err := p.get(ctx, constraints)
if err != nil {
return nil, err
}
p.cache.SetDefault(fmt.Sprint(hash), instanceTypes)
logging.FromContext(ctx).Debugf("Discovered %d EC2 instance types", len(instanceTypes))
return instanceTypes, nil
}

func (p *InstanceTypeProvider) get(ctx context.Context, constraints *v1alpha1.Constraints) ([]cloudprovider.InstanceType, error) {
// Get InstanceTypes from EC2
instanceTypes, err := p.getInstanceTypes(ctx)
if err != nil {
Expand Down Expand Up @@ -91,7 +78,11 @@ func (p *InstanceTypeProvider) get(ctx context.Context, constraints *v1alpha1.Co
return result, nil
}

func (p *InstanceTypeProvider) getInstanceTypeZones(ctx context.Context) (map[string]sets.String, error) {
func (p *InstanceTypeProvider) getInstanceTypeZones(ctx context.Context) (result map[string]sets.String, err error) {
if cached, ok := p.cache.Get(instanceTypeZonesCacheKey); ok {
return cached.(map[string]sets.String), nil
}
defer func() { p.cache.SetDefault(instanceTypeZonesCacheKey, result) }()
zones := map[string]sets.String{}
if err := p.ec2api.DescribeInstanceTypeOfferingsPagesWithContext(ctx, &ec2.DescribeInstanceTypeOfferingsInput{LocationType: aws.String("availability-zone")},
func(output *ec2.DescribeInstanceTypeOfferingsOutput, lastPage bool) bool {
Expand All @@ -105,11 +96,16 @@ func (p *InstanceTypeProvider) getInstanceTypeZones(ctx context.Context) (map[st
}); err != nil {
return nil, fmt.Errorf("describing instance type zone offerings, %w", err)
}
logging.FromContext(ctx).Debugf("Discovered EC2 instance types zonal offerings")
return zones, nil
}

// getInstanceTypes retrieves all instance types from the ec2 DescribeInstanceTypes API using some opinionated filters
func (p *InstanceTypeProvider) getInstanceTypes(ctx context.Context) (map[string]*InstanceType, error) {
func (p *InstanceTypeProvider) getInstanceTypes(ctx context.Context) (result map[string]*InstanceType, err error) {
if cached, ok := p.cache.Get(instanceTypesCacheKey); ok {
return cached.(map[string]*InstanceType), nil
}
defer func() { p.cache.SetDefault(instanceTypesCacheKey, result) }()
instanceTypes := map[string]*InstanceType{}
if err := p.ec2api.DescribeInstanceTypesPagesWithContext(ctx, &ec2.DescribeInstanceTypesInput{
Filters: []*ec2.Filter{
Expand All @@ -128,6 +124,7 @@ func (p *InstanceTypeProvider) getInstanceTypes(ctx context.Context) (map[string
}); err != nil {
return nil, fmt.Errorf("fetching instance types using ec2.DescribeInstanceTypes, %w", err)
}
logging.FromContext(ctx).Debugf("Discovered %d EC2 instance types", len(instanceTypes))
return instanceTypes, nil
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/cloudprovider/fake/cloudprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ func (c *CloudProvider) Create(_ context.Context, constraints *v1alpha5.Constrai
for i := 0; i < quantity; i++ {
name := strings.ToLower(randomdata.SillyName())
instance := instanceTypes[0]
zone, _ := instance.Zones().Intersection(constraints.Requirements.Zones()).PopAny()
operatingSystem, _ := instance.OperatingSystems().PopAny()
zone := instance.Zones().Intersection(constraints.Requirements.Zones()).UnsortedList()[0]
operatingSystem := instance.OperatingSystems().UnsortedList()[0]

go func() {
err <- bind(&v1.Node{
Expand Down
2 changes: 1 addition & 1 deletion pkg/controllers/allocation/scheduling/taints.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (ts Taints) WithPod(pod *v1.Pod) Taints {
for _, toleration := range pod.Spec.Tolerations {
// Only OpEqual is supported. OpExists does not make sense for
// provisioning -- in theory we could create a taint on the node with a
// random string, but it's unclear use case this would accomplish.
// random string, but it's unclear what use case this would accomplish.
if toleration.Operator != v1.TolerationOpEqual {
continue
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/controllers/allocation/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ var _ = Describe("Allocation", func() {
Expect(pod.Spec.NodeName).To(Equal(nodes.Items[0].Name))
}
})
FIt("should provision nodes for pods with supported node selectors", func() {
It("should provision nodes for pods with supported node selectors", func() {
schedulable := []client.Object{
// Constrained by provisioner
test.UnschedulablePod(test.PodOptions{NodeSelector: map[string]string{v1alpha5.ProvisionerNameLabelKey: provisioner.Name}}),
Expand Down

0 comments on commit ac5daa3

Please sign in to comment.