Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding initial support for bottlerocket without custom launch template #1110

Merged
merged 15 commits into from
Feb 8, 2022
1 change: 1 addition & 0 deletions pkg/apis/provisioning/v1alpha5/requirements.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ var (
ArchitectureAmd64 = "amd64"
ArchitectureArm64 = "arm64"
OperatingSystemLinux = "linux"
BottleRocket = "bottlerocket"
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to keep AWS specific concepts isolated to the cloudprovider/aws package, check out the APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should this be OperatingSystemBottleRocket to match the linux var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this to cloudprovider/aws/apis/v1alpha1/register.go, renamed to OperatingSystemBottleRocket.


// RestrictedLabels are injected by Cloud Providers
RestrictedLabels = sets.NewString(
Expand Down
16 changes: 13 additions & 3 deletions pkg/cloudprovider/aws/ami.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/aws/aws-sdk-go/service/ssm/ssmiface"
"github.com/aws/karpenter/pkg/apis/provisioning/v1alpha5"
"github.com/aws/karpenter/pkg/cloudprovider"
"github.com/aws/karpenter/pkg/cloudprovider/aws/apis/v1alpha1"
"github.com/patrickmn/go-cache"
"k8s.io/client-go/kubernetes"
"knative.dev/pkg/logging"
Expand All @@ -46,15 +47,15 @@ func NewAMIProvider(ssm ssmiface.SSMAPI, clientSet *kubernetes.Clientset) *AMIPr
}

// Get returns a set of AMIIDs and corresponding instance types. AMI may vary due to architecture, accelerator, etc
func (p *AMIProvider) Get(ctx context.Context, instanceTypes []cloudprovider.InstanceType) (map[string][]cloudprovider.InstanceType, error) {
func (p *AMIProvider) Get(ctx context.Context, instanceTypes []cloudprovider.InstanceType, constraints *v1alpha1.Constraints) (map[string][]cloudprovider.InstanceType, error) {
bwagner5 marked this conversation as resolved.
Show resolved Hide resolved
version, err := p.kubeServerVersion(ctx)
if err != nil {
return nil, fmt.Errorf("kube server version, %w", err)
}
// Separate instance types by unique queries
amiQueries := map[string][]cloudprovider.InstanceType{}
for _, instanceType := range instanceTypes {
query := p.getSSMQuery(instanceType, version)
query := p.getSSMQuery(ctx, constraints.AWS.AMIFamily, instanceType, version)
amiQueries[query] = append(amiQueries[query], instanceType)
}
// Separate instance types by unique AMIIDs
Expand Down Expand Up @@ -83,12 +84,21 @@ func (p *AMIProvider) getAMIID(ctx context.Context, query string) (string, error
return ami, nil
}

func (p *AMIProvider) getSSMQuery(instanceType cloudprovider.InstanceType, version string) string {
func (p *AMIProvider) getSSMQuery(ctx context.Context, amiFamily string, instanceType cloudprovider.InstanceType, version string) string {
var amiSuffix string
var arch string
if !instanceType.NvidiaGPUs().IsZero() || !instanceType.AWSNeurons().IsZero() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

EKS optimized AMIs for this combination doesn't exist right now

amiSuffix = "-gpu"
} else if instanceType.Architecture() == v1alpha5.ArchitectureArm64 {
amiSuffix = "-arm64"
arch = "arm64"
} else if instanceType.Architecture() == v1alpha5.ArchitectureAmd64 {
arch = "arm64"
rayterrill marked this conversation as resolved.
Show resolved Hide resolved
}

if amiFamily == v1alpha5.BottleRocket {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on doing a switch statement here for all ami families? We may need this for flatcar, ubuntu, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellistarn we actually talked about this today at work.

Is there a concept of "release channels" for these OS versions (bottlerocket and amazon linux)? I see AL2's is called "recommended" - does that imply there's a latest that's newer/less tested? Similarly for bottlerocket - does latest imply there's a recommended?

We were also wondering if it might be beneficial to allow users to specify their own SSM query, so they can control their own "release cadence" outside of AWS' as needed (we might want to do something like let our DEV stuff pull latest, but "pin" PROD to something like "stable" that's more battle tested.

logging.FromContext(ctx).Debugf("AMIFamily was: %s", amiFamily)
bwagner5 marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Sprintf("/aws/service/bottlerocket/aws-k8s-%s/%s/latest/image_id", version, arch)
Copy link
Contributor

Choose a reason for hiding this comment

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

@suket22 It would be really nice if bottlerocket and eksoptimizedami were aligned to allow for simple substitution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these SSM parameters are managed and owned by the BottleRocket folks - we could ask them if they'd like to restructure these but I don't think it's likely.

}
return fmt.Sprintf("/aws/service/eks/optimized-ami/%s/amazon-linux-2%s/recommended/image_id", version, amiSuffix)
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/cloudprovider/aws/apis/v1alpha1/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ type AWS struct {
// TypeMeta includes version and kind of the extensions, inferred if not provided.
// +optional
metav1.TypeMeta `json:",inline"`
// AMIFamily is the AMI family that instances use.
// +optional
AMIFamily string `json:"amiFamily"`
bwagner5 marked this conversation as resolved.
Show resolved Hide resolved
// InstanceProfile is the AWS identity that instances use.
// +required
InstanceProfile string `json:"instanceProfile"`
Expand Down
4 changes: 4 additions & 0 deletions pkg/cloudprovider/aws/instancetype.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ func (i *InstanceType) Name() string {
return aws.StringValue(i.InstanceType)
}

func (i *InstanceType) AMIFamily() string {
rayterrill marked this conversation as resolved.
Show resolved Hide resolved
return ""
}

func (i *InstanceType) Offerings() []cloudprovider.Offering {
return i.AvailableOfferings
}
Expand Down
42 changes: 40 additions & 2 deletions pkg/cloudprovider/aws/launchtemplate.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,20 @@ func (p *LaunchTemplateProvider) Get(ctx context.Context, constraints *v1alpha1.
return nil, err
}
// Get constrained AMI ID
amis, err := p.amiProvider.Get(ctx, instanceTypes)
amis, err := p.amiProvider.Get(ctx, instanceTypes, constraints)
if err != nil {
return nil, err
}
// Construct launch templates
launchTemplates := map[string][]cloudprovider.InstanceType{}
for amiID, instanceTypes := range amis {
// Get userData for Node
userData, err := p.getUserData(ctx, constraints, instanceTypes, additionalLabels)
var userData string
if constraints.AMIFamily == "bottlerocket" {
bwagner5 marked this conversation as resolved.
Show resolved Hide resolved
userData, err = p.getBottleRocketUserData(ctx, constraints, additionalLabels)
} else {
userData, err = p.getUserData(ctx, constraints, instanceTypes, additionalLabels)
bwagner5 marked this conversation as resolved.
Show resolved Hide resolved
}
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -220,6 +225,39 @@ func sortedKeys(m map[string]string) []string {
return keys
}

func (p *LaunchTemplateProvider) getBottleRocketUserData(ctx context.Context, constraints *v1alpha1.Constraints, additionalLabels map[string]string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also support maxPods as that was functionality added in this PR.

For BR, it should be settings.kubernetes.max-pods

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a follow-up change too :)

var userData bytes.Buffer
bwagner5 marked this conversation as resolved.
Show resolved Hide resolved
userData.WriteString(fmt.Sprintf(`[settings.kubernetes]
cluster-name = "%s"
api-server = "%s"
`,
injection.GetOptions(ctx).ClusterName,
injection.GetOptions(ctx).ClusterEndpoint))
bwagner5 marked this conversation as resolved.
Show resolved Hide resolved
caBundle, err := p.GetCABundle(ctx)
bwagner5 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return "", fmt.Errorf("getting ca bundle for user data, %w", err)
}
if caBundle != nil {
userData.WriteString(fmt.Sprintf(`cluster-certificate = "%s"

`,
*caBundle))
}

nodeLabelArgs := functional.UnionStringMaps(additionalLabels, constraints.Labels)
if len(nodeLabelArgs) > 0 {
userData.WriteString(`[settings.kubernetes.node-labels]
`)
for key, val := range nodeLabelArgs {
userData.WriteString(fmt.Sprintf(`"%s" = "%s"
`,
key, val))
}
}

bwagner5 marked this conversation as resolved.
Show resolved Hide resolved
return base64.StdEncoding.EncodeToString(userData.Bytes()), nil
}

// getUserData returns the exact same string for equivalent input,
// even if elements of those inputs are in differing orders,
// guaranteeing it won't cause spurious hash differences.
Expand Down
5 changes: 5 additions & 0 deletions pkg/cloudprovider/fake/instancetype.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ type InstanceTypeOptions struct {
Name string
Offerings []cloudprovider.Offering
Architecture string
AMIFamily string
rayterrill marked this conversation as resolved.
Show resolved Hide resolved
OperatingSystems sets.String
CPU resource.Quantity
Memory resource.Quantity
Expand All @@ -105,6 +106,10 @@ func (i *InstanceType) Name() string {
return i.options.Name
}

func (i *InstanceType) AMIFamily() string {
rayterrill marked this conversation as resolved.
Show resolved Hide resolved
return "bottlerocket"
}

func (i *InstanceType) Offerings() []cloudprovider.Offering {
return i.options.Offerings
}
Expand Down
1 change: 1 addition & 0 deletions pkg/cloudprovider/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type InstanceType interface {
// Note that though this is an array it is expected that all the Offerings are unique from one another
Offerings() []Offering
Architecture() string
AMIFamily() string
rayterrill marked this conversation as resolved.
Show resolved Hide resolved
OperatingSystems() sets.String
CPU() *resource.Quantity
Memory() *resource.Quantity
Expand Down