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

Always use $Default LT version #595

Merged
merged 4 commits into from
Aug 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions designs/aws-launch-templates-options.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,17 @@ Furthermore, in keeping with Kubernetes
we should use `node.k8s.aws/` as the prefix for any node labels we
choose to use.

Lastly, in keeping with another Kubernetes convention (such as
In keeping with another Kubernetes convention (such as
`kubernetes.io/service-name`), we should use `lower-case-with-hypens`,
not `camelCase` for words following the `/`.

Lastly, using launch template names, rather than ids, will be more
familiar to Kubernetes users and should cut down on manual work and
errors. Since the EC2 APIs that use launch templates can take either
names or ids, it seems like names are an more useful choice. In the
future we can consider adding id support as well, if there is demand
for it.

## Architecture

One partial solution to the architecture issue brought up above would
Expand All @@ -146,9 +153,9 @@ spec:
labels:
# applied only to arm64
arm64:
node.k8s.aws/launch-template-id: id-of-arm64-lt
node.k8s.aws/launch-template-name: name-of-arm64-lt
x86_64:
node.k8s.aws/launch-template-id: id-of-x86_64-lt
node.k8s.aws/launch-template-name: name-of-x86_64-lt
# applied everywhere
other-label: other-value
```
Expand All @@ -164,7 +171,7 @@ spec:
other-label: other-value
# applied only to arm64
arm64-labels:
node.k8s.aws/launch-template-id: id-of-arm64-based-lt
node.k8s.aws/launch-template-name: name-of-arm64-based-lt
# applied only to x86_64
x86_64-labels:
```
Expand All @@ -177,9 +184,9 @@ apiVersion: provisioning.karpenter.sh/v1alpha2
kind: Provisioner
spec:
labels:
node.k8s.aws/launch-template-id/arm64: id-of-arm64-lt
node.k8s.aws/launch-template-name/arm64: name-of-arm64-lt
# or?
node.k8s.aws/arm64/launch-template-id: id-of-arm64-lt
node.k8s.aws/arm64/launch-template-name: name-of-arm64-lt
```

This might be very non-standard however and defy expectations. Also,
Expand Down Expand Up @@ -208,12 +215,12 @@ apiVersion: provisioning.karpenter.sh/v1alpha2
kind: Provisioner
spec:
labels:
node.k8s.aws/launch-template-id: id-of-lt-with-x86_64-based-ami
node.k8s.aws/launch-template-name: name-of-lt-with-x86_64-based-ami
```

Then it could ignore that pending pod since there is no way it could
possibly work. Another possiblity is that it could ignore the launch
template `node.k8s.aws/launch-template-id` for that pod and revert to
template `node.k8s.aws/launch-template-name` for that pod and revert to
the default dynamically-generated
([Bottlerocket](https://aws.amazon.com/bottlerocket/)) launch template
that would work for that architecture.
Expand All @@ -224,11 +231,11 @@ that would work for that architecture.
For now the recommendation is to support the following in provisioner
`spec.labels`:

- `node.k8s.aws/launch-template-id`: (optional) id of launch template
- `node.k8s.aws/launch-template-name`: (optional) id of launch template
and cannot be specified if `architecture`
- `node.k8s.aws/launch-template-version`: version number or `$LATEST`
(optional, default `$LATEST`) (cannot be specified unless
`node.k8s.aws/launch-template-id` is present
`node.k8s.aws/launch-template-name` is present
- `node.k8s.aws/capacity-type`: listed here for completeness

### The Fine Print
Expand All @@ -254,7 +261,7 @@ metadata:
name: podA
spec:
nodeSelector:
node.k8s.aws/launch-template-id=x
node.k8s.aws/launch-template-name=x
---
apiVersion: v1
kind: Pod
Expand All @@ -268,7 +275,7 @@ two instances would launch.

#### Multiple Provisioners

While specifying a `launch-template-id` on a provisioner will limit
While specifying a `launch-template-name` on a provisioner will limit
that provisioner to a single default launch template (pod specs can
still override by specifying a launch template), it seems like the
complexity of the other solutions that might allow multiple launch
Expand Down
38 changes: 14 additions & 24 deletions pkg/cloudprovider/aws/constraints.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,16 @@ const (
)

var (
AWSLabelPrefix = "node.k8s.aws/"
CapacityTypeLabel = AWSLabelPrefix + "capacity-type"
LaunchTemplateIdLabel = AWSLabelPrefix + "launch-template-id"
LaunchTemplateVersionLabel = AWSLabelPrefix + "launch-template-version"
SubnetNameLabel = AWSLabelPrefix + "subnet-name"
SubnetTagKeyLabel = AWSLabelPrefix + "subnet-tag-key"
SecurityGroupNameLabel = AWSLabelPrefix + "security-group-name"
SecurityGroupTagKeyLabel = AWSLabelPrefix + "security-group-tag-key"
AllowedLabels = []string{
AWSLabelPrefix = "node.k8s.aws/"
CapacityTypeLabel = AWSLabelPrefix + "capacity-type"
LaunchTemplateNameLabel = AWSLabelPrefix + "launch-template-name"
SubnetNameLabel = AWSLabelPrefix + "subnet-name"
SubnetTagKeyLabel = AWSLabelPrefix + "subnet-tag-key"
SecurityGroupNameLabel = AWSLabelPrefix + "security-group-name"
SecurityGroupTagKeyLabel = AWSLabelPrefix + "security-group-tag-key"
AllowedLabels = []string{
CapacityTypeLabel,
LaunchTemplateIdLabel,
LaunchTemplateVersionLabel,
LaunchTemplateNameLabel,
SubnetNameLabel,
SubnetTagKeyLabel,
SecurityGroupNameLabel,
Expand All @@ -70,22 +68,18 @@ func (c *Constraints) GetCapacityType() string {
}

type LaunchTemplate struct {
Id string
Name string
Version string
}

func (c *Constraints) GetLaunchTemplate() *LaunchTemplate {
id, ok := c.Labels[LaunchTemplateIdLabel]
name, ok := c.Labels[LaunchTemplateNameLabel]
if !ok {
return nil
}
version, ok := c.Labels[LaunchTemplateVersionLabel]
if !ok {
version = DefaultLaunchTemplateVersion
}
return &LaunchTemplate{
Id: id,
Version: version,
Name: name,
Version: DefaultLaunchTemplateVersion,
}
}

Expand Down Expand Up @@ -152,11 +146,7 @@ func (c *Constraints) validateCapacityType(ctx context.Context) (errs *apis.Fiel
}

func (c *Constraints) validateLaunchTemplate(ctx context.Context) (errs *apis.FieldError) {
if _, versionExists := c.Labels[LaunchTemplateVersionLabel]; versionExists {
if _, bothExist := c.Labels[LaunchTemplateIdLabel]; !bothExist {
return errs.Also(apis.ErrMissingField(fmt.Sprintf("spec.labels[%s]", LaunchTemplateIdLabel)))
}
}
// nothing to validate at the moment
return errs
}

Expand Down
7 changes: 3 additions & 4 deletions pkg/cloudprovider/aws/fake/ec2api.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,8 @@ func (e *EC2API) Reset() {

func (e *EC2API) CreateFleetWithContext(ctx context.Context, input *ec2.CreateFleetInput, options ...request.Option) (*ec2.CreateFleetOutput, error) {
e.CalledWithCreateFleetInput.Add(input)
if input.LaunchTemplateConfigs[0].LaunchTemplateSpecification.LaunchTemplateId == nil &&
input.LaunchTemplateConfigs[0].LaunchTemplateSpecification.LaunchTemplateName == nil {
return nil, fmt.Errorf("missing launch template id or name")
if input.LaunchTemplateConfigs[0].LaunchTemplateSpecification.LaunchTemplateName == nil {
return nil, fmt.Errorf("missing launch template name")
}
instance := &ec2.Instance{
InstanceId: aws.String(randomdata.SillyName()),
Expand All @@ -78,7 +77,7 @@ func (e *EC2API) CreateFleetWithContext(ctx context.Context, input *ec2.CreateFl

func (e *EC2API) CreateLaunchTemplateWithContext(ctx context.Context, input *ec2.CreateLaunchTemplateInput, options ...request.Option) (*ec2.CreateLaunchTemplateOutput, error) {
e.CalledWithCreateLaunchTemplateInput.Add(input)
launchTemplate := &ec2.LaunchTemplate{LaunchTemplateName: input.LaunchTemplateName, LaunchTemplateId: aws.String("test-launch-template-id")}
launchTemplate := &ec2.LaunchTemplate{LaunchTemplateName: input.LaunchTemplateName}
e.LaunchTemplates.Store(input.LaunchTemplateName, launchTemplate)
return &ec2.CreateLaunchTemplateOutput{LaunchTemplate: launchTemplate}, nil
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/cloudprovider/aws/instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@ func (p *InstanceProvider) launchInstance(ctx context.Context,
},
LaunchTemplateConfigs: []*ec2.FleetLaunchTemplateConfigRequest{{
LaunchTemplateSpecification: &ec2.FleetLaunchTemplateSpecificationRequest{
LaunchTemplateId: aws.String(launchTemplate.Id),
Version: aws.String(launchTemplate.Version),
LaunchTemplateName: aws.String(launchTemplate.Name),
Version: aws.String(launchTemplate.Version),
},
Overrides: overrides,
}},
Expand Down
2 changes: 1 addition & 1 deletion pkg/cloudprovider/aws/launchtemplate.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func (p *LaunchTemplateProvider) Get(ctx context.Context, provisioner *v1alpha3.
return nil, err
}
return &LaunchTemplate{
Id: aws.StringValue(launchTemplate.LaunchTemplateId),
Name: aws.StringValue(launchTemplate.LaunchTemplateName),
Version: fmt.Sprint(DefaultLaunchTemplateVersion),
}, nil
}
Expand Down
62 changes: 21 additions & 41 deletions pkg/cloudprovider/aws/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,113 +276,98 @@ var _ = Describe("Allocation", func() {
pods := ExpectProvisioningSucceeded(ctx, env.Client, controller, provisioner, test.PendingPod())
// Assertions
node := ExpectNodeExists(env.Client, pods[0].Spec.NodeName)
Expect(node.Labels).ToNot(HaveKey(LaunchTemplateIdLabel))
Expect(node.Labels).ToNot(HaveKey(LaunchTemplateVersionLabel))
Expect(node.Labels).ToNot(HaveKey(LaunchTemplateNameLabel))
Expect(fakeEC2API.CalledWithCreateFleetInput.Cardinality()).To(Equal(1))
input := fakeEC2API.CalledWithCreateFleetInput.Pop().(*ec2.CreateFleetInput)
Expect(input.LaunchTemplateConfigs).To(HaveLen(1))
launchTemplate := input.LaunchTemplateConfigs[0].LaunchTemplateSpecification
Expect(*launchTemplate.LaunchTemplateId).To(Equal("test-launch-template-id"))
Expect(*launchTemplate.Version).To(Equal(DefaultLaunchTemplateVersion))
})
It("should default to a provisioner's launch template id and version", func() {
// Setup
provisioner.Spec.Labels = map[string]string{
LaunchTemplateIdLabel: randomdata.SillyName(),
LaunchTemplateVersionLabel: randomdata.SillyName(),
LaunchTemplateNameLabel: randomdata.SillyName(),
}
ExpectCreated(env.Client, provisioner)
pods := ExpectProvisioningSucceeded(ctx, env.Client, controller, provisioner, test.PendingPod())
// Assertions
node := ExpectNodeExists(env.Client, pods[0].Spec.NodeName)
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateIdLabel, provisioner.Spec.Labels[LaunchTemplateIdLabel]))
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateVersionLabel, provisioner.Spec.Labels[LaunchTemplateVersionLabel]))
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateNameLabel, provisioner.Spec.Labels[LaunchTemplateNameLabel]))
Expect(fakeEC2API.CalledWithCreateFleetInput.Cardinality()).To(Equal(1))
input := fakeEC2API.CalledWithCreateFleetInput.Pop().(*ec2.CreateFleetInput)
Expect(input.LaunchTemplateConfigs).To(HaveLen(1))
launchTemplate := input.LaunchTemplateConfigs[0].LaunchTemplateSpecification
Expect(*launchTemplate.LaunchTemplateId).To(Equal(provisioner.Spec.Labels[LaunchTemplateIdLabel]))
Expect(*launchTemplate.Version).To(Equal(provisioner.Spec.Labels[LaunchTemplateVersionLabel]))

Expect(*launchTemplate.LaunchTemplateName).To(Equal(provisioner.Spec.Labels[LaunchTemplateNameLabel]))
})
It("should default to a provisioner's launch template and the default launch template version", func() {
// Setup
provisioner.Spec.Labels = map[string]string{LaunchTemplateIdLabel: randomdata.SillyName()}
provisioner.Spec.Labels = map[string]string{LaunchTemplateNameLabel: randomdata.SillyName()}
ExpectCreated(env.Client, provisioner)
pods := ExpectProvisioningSucceeded(ctx, env.Client, controller, provisioner, test.PendingPod())
// Assertions
node := ExpectNodeExists(env.Client, pods[0].Spec.NodeName)
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateIdLabel, provisioner.Spec.Labels[LaunchTemplateIdLabel]))
Expect(node.Labels).ToNot(HaveKey(LaunchTemplateVersionLabel))
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateNameLabel, provisioner.Spec.Labels[LaunchTemplateNameLabel]))
Expect(fakeEC2API.CalledWithCreateFleetInput.Cardinality()).To(Equal(1))
input := fakeEC2API.CalledWithCreateFleetInput.Pop().(*ec2.CreateFleetInput)
Expect(input.LaunchTemplateConfigs).To(HaveLen(1))
launchTemplate := input.LaunchTemplateConfigs[0].LaunchTemplateSpecification
Expect(*launchTemplate.LaunchTemplateId).To(Equal(provisioner.Spec.Labels[LaunchTemplateIdLabel]))
Expect(*launchTemplate.LaunchTemplateName).To(Equal(provisioner.Spec.Labels[LaunchTemplateNameLabel]))
Expect(*launchTemplate.Version).To(Equal(DefaultLaunchTemplateVersion))
})
It("should allow a pod to override the launch template id and version", func() {
// Setup
provisioner.Spec.Labels = map[string]string{
LaunchTemplateIdLabel: randomdata.SillyName(),
LaunchTemplateVersionLabel: randomdata.SillyName(),
LaunchTemplateNameLabel: randomdata.SillyName(),
}
ExpectCreated(env.Client, provisioner)
pods := ExpectProvisioningSucceeded(ctx, env.Client, controller, provisioner,
test.PendingPod(test.PodOptions{NodeSelector: map[string]string{
LaunchTemplateIdLabel: randomdata.SillyName(),
LaunchTemplateVersionLabel: randomdata.SillyName(),
LaunchTemplateNameLabel: randomdata.SillyName(),
}}),
)
// Assertions
node := ExpectNodeExists(env.Client, pods[0].Spec.NodeName)
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateIdLabel, pods[0].Spec.NodeSelector[LaunchTemplateIdLabel]))
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateVersionLabel, pods[0].Spec.NodeSelector[LaunchTemplateVersionLabel]))
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateNameLabel, pods[0].Spec.NodeSelector[LaunchTemplateNameLabel]))
Expect(fakeEC2API.CalledWithCreateFleetInput.Cardinality()).To(Equal(1))
input := fakeEC2API.CalledWithCreateFleetInput.Pop().(*ec2.CreateFleetInput)
Expect(input.LaunchTemplateConfigs).To(HaveLen(1))
launchTemplate := input.LaunchTemplateConfigs[0].LaunchTemplateSpecification
Expect(*launchTemplate.LaunchTemplateId).To(Equal(pods[0].Spec.NodeSelector[LaunchTemplateIdLabel]))
Expect(*launchTemplate.Version).To(Equal(pods[0].Spec.NodeSelector[LaunchTemplateVersionLabel]))
Expect(*launchTemplate.LaunchTemplateName).To(Equal(pods[0].Spec.NodeSelector[LaunchTemplateNameLabel]))
})
It("should allow a pod to override the launch template id and use the default launch template version", func() {
// Setup
provisioner.Spec.Labels = map[string]string{LaunchTemplateIdLabel: randomdata.SillyName()}
provisioner.Spec.Labels = map[string]string{LaunchTemplateNameLabel: randomdata.SillyName()}
ExpectCreated(env.Client, provisioner)
pods := ExpectProvisioningSucceeded(ctx, env.Client, controller, provisioner,
test.PendingPod(test.PodOptions{NodeSelector: map[string]string{LaunchTemplateIdLabel: randomdata.SillyName()}}),
test.PendingPod(test.PodOptions{NodeSelector: map[string]string{LaunchTemplateNameLabel: randomdata.SillyName()}}),
)
// Assertions
node := ExpectNodeExists(env.Client, pods[0].Spec.NodeName)
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateIdLabel, pods[0].Spec.NodeSelector[LaunchTemplateIdLabel]))
Expect(node.Labels).ToNot(HaveKey(LaunchTemplateVersionLabel))
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateNameLabel, pods[0].Spec.NodeSelector[LaunchTemplateNameLabel]))
Expect(fakeEC2API.CalledWithCreateFleetInput.Cardinality()).To(Equal(1))
input := fakeEC2API.CalledWithCreateFleetInput.Pop().(*ec2.CreateFleetInput)
Expect(input.LaunchTemplateConfigs).To(HaveLen(1))
launchTemplate := input.LaunchTemplateConfigs[0].LaunchTemplateSpecification
Expect(*launchTemplate.LaunchTemplateId).To(Equal(pods[0].Spec.NodeSelector[LaunchTemplateIdLabel]))
Expect(*launchTemplate.LaunchTemplateName).To(Equal(pods[0].Spec.NodeSelector[LaunchTemplateNameLabel]))
Expect(*launchTemplate.Version).To(Equal(DefaultLaunchTemplateVersion))
})
It("should allow a pod to override the launch template id and use the provisioner's launch template version", func() {
// Setup
provisioner.Spec.Labels = map[string]string{
LaunchTemplateIdLabel: randomdata.SillyName(),
LaunchTemplateVersionLabel: randomdata.SillyName(),
LaunchTemplateNameLabel: randomdata.SillyName(),
}
ExpectCreated(env.Client, provisioner)
pods := ExpectProvisioningSucceeded(ctx, env.Client, controller, provisioner,
test.PendingPod(test.PodOptions{NodeSelector: map[string]string{LaunchTemplateIdLabel: randomdata.SillyName()}}),
test.PendingPod(test.PodOptions{NodeSelector: map[string]string{LaunchTemplateNameLabel: randomdata.SillyName()}}),
)
// Assertions
node := ExpectNodeExists(env.Client, pods[0].Spec.NodeName)
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateIdLabel, pods[0].Spec.NodeSelector[LaunchTemplateIdLabel]))
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateVersionLabel, provisioner.Spec.Labels[LaunchTemplateVersionLabel]))
Expect(node.Labels).To(HaveKeyWithValue(LaunchTemplateNameLabel, pods[0].Spec.NodeSelector[LaunchTemplateNameLabel]))
Expect(fakeEC2API.CalledWithCreateFleetInput.Cardinality()).To(Equal(1))
input := fakeEC2API.CalledWithCreateFleetInput.Pop().(*ec2.CreateFleetInput)
Expect(input.LaunchTemplateConfigs).To(HaveLen(1))
launchTemplate := input.LaunchTemplateConfigs[0].LaunchTemplateSpecification
Expect(*launchTemplate.LaunchTemplateId).To(Equal(pods[0].Spec.NodeSelector[LaunchTemplateIdLabel]))
Expect(*launchTemplate.Version).To(Equal(provisioner.Spec.Labels[LaunchTemplateVersionLabel]))
Expect(*launchTemplate.LaunchTemplateName).To(Equal(pods[0].Spec.NodeSelector[LaunchTemplateNameLabel]))
})
})
Context("Subnets", func() {
Expand Down Expand Up @@ -594,19 +579,14 @@ var _ = Describe("Allocation", func() {
})
It("should support launch templates", func() {
provisioner.Spec.Labels = map[string]string{
"node.k8s.aws/launch-template-version": randomdata.SillyName(),
"node.k8s.aws/launch-template-id": "23",
"node.k8s.aws/launch-template-name": randomdata.SillyName(),
}
Expect(provisioner.Validate(ctx)).To(Succeed())
})
It("should allow launch template id to be specified alone", func() {
provisioner.Spec.Labels = map[string]string{"node.k8s.aws/launch-template-id": "23"}
provisioner.Spec.Labels = map[string]string{"node.k8s.aws/launch-template-name": randomdata.SillyName()}
Expect(provisioner.Validate(ctx)).To(Succeed())
})
It("should fail if only launch template version label present", func() {
provisioner.Spec.Labels = map[string]string{"node.k8s.aws/launch-template-version": randomdata.SillyName()}
Expect(provisioner.Validate(ctx)).ToNot(Succeed())
})
It("should support on demand capacity type", func() {
provisioner.Spec.Labels = map[string]string{"node.k8s.aws/capacity-type": CapacityTypeOnDemand}
Expect(provisioner.Validate(ctx)).To(Succeed())
Expand Down