-
Notifications
You must be signed in to change notification settings - Fork 980
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
Changes from 1 commit
ba99090
cd736bc
6de2379
0f8cafa
e6bc0e7
ee74210
64533d9
7640e6b
dbeddfa
6818aff
64b650a
e30936c
b504aee
586c42b
a89b7be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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.AMIFamily, instanceType, version) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider passing the full constraints object through. |
||
amiQueries[query] = append(amiQueries[query], instanceType) | ||
} | ||
// Separate instance types by unique AMIIDs | ||
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @suket22 , what about arm+gpus? https://aws.amazon.com/blogs/machine-learning/aws-and-nvidia-to-bring-arm-based-instances-with-gpus-to-the-cloud/. I don't see this here: https://docs.aws.amazon.com/eks/latest/userguide/retrieve-ami-id.html. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
} | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 varThere was a problem hiding this comment.
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.