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

Add instance details to AWSMachine, enables more than one instance type for instance creation #4112

Closed
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
2 changes: 2 additions & 0 deletions api/v1beta1/awsmachine_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func (src *AWSMachine) ConvertTo(dstRaw conversion.Hub) error {

dst.Spec.Ignition = restored.Spec.Ignition
dst.Spec.InstanceMetadataOptions = restored.Spec.InstanceMetadataOptions
dst.Spec.InstanceDetails = restored.Spec.InstanceDetails

return nil
}
Expand Down Expand Up @@ -83,6 +84,7 @@ func (r *AWSMachineTemplate) ConvertTo(dstRaw conversion.Hub) error {
dst.Spec.Template.ObjectMeta = restored.Spec.Template.ObjectMeta
dst.Spec.Template.Spec.Ignition = restored.Spec.Template.Spec.Ignition
dst.Spec.Template.Spec.InstanceMetadataOptions = restored.Spec.Template.Spec.InstanceMetadataOptions
dst.Spec.Template.Spec.InstanceDetails = restored.Spec.Template.Spec.InstanceDetails

return nil
}
Expand Down
1 change: 1 addition & 0 deletions api/v1beta1/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions api/v1beta2/awsmachine_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,14 @@ type AWSMachineSpec struct {
// +optional
// +kubebuilder:validation:Enum:=default;dedicated;host
Tenancy string `json:"tenancy,omitempty"`

// InstanceDetails is a list of instance details which are used to create AWSMachines.
// When InstanceDetails is supplied, AWSMachines are created using the instance type and spot options
// of the first item in the list instead of using values provided in the spec.
// If the instance request can not be satisfied due to a lack of capacity, the next instance detail is
// used to create the ec2 instance.
// +optional
InstanceDetails []AWSInstanceDetails `json:"instanceDetails,omitempty"`
}

// CloudInit defines options related to the bootstrapping systems where
Expand Down Expand Up @@ -185,6 +193,18 @@ type CloudInit struct {
SecureSecretsBackend SecretBackend `json:"secureSecretsBackend,omitempty"`
}

// AWSInstanceDetails describes an instance details for creating a AWSMachine.
type AWSInstanceDetails struct {
// InstanceType is the type of instance to create. Example: m4.xlarge
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinLength:=2
InstanceType string `json:"instanceType"`

// SpotMarketOptions allows users to configure instances to be run using AWS Spot instances.
// +optional
SpotMarketOptions *SpotMarketOptions `json:"spotMarketOptions,omitempty"`
}

// Ignition defines options related to the bootstrapping systems where Ignition is used.
type Ignition struct {
// Version defines which version of Ignition will be used to generate bootstrap data.
Expand Down
24 changes: 24 additions & 0 deletions api/v1beta2/awsmachine_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,20 @@ func (r *AWSMachine) ValidateUpdate(old runtime.Object) error {
delete(cloudInit, "secureSecretsBackend")
}

// allow limited changes to instanceDetails if we are defaulting and the field was missing
if newInstanceDetails, ok := newAWSMachineSpec["instanceDetails"].(map[string]interface{}); ok {
_, oldOk := oldAWSMachineSpec["instanceDetails"]
if !oldOk && len(newInstanceDetails) == 1 {
instanceDetails := map[string]interface{}{
"instanceType": oldAWSMachineSpec["instanceType"],
}
if spotMarketOptions, ok := oldAWSMachineSpec["spotMarketOptions"]; ok {
instanceDetails["spotMarketOptions"] = spotMarketOptions
}
oldAWSMachineSpec["instanceDetails"] = []interface{}{instanceDetails}
}
}

if !cmp.Equal(oldAWSMachineSpec, newAWSMachineSpec) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Errr, correct me if I'm wrong, but if the previous thing changes something in the machine spec because of the re-order, shouldn't this thing fail here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow your question, but I did catch an issue in reviewing this part of the webhook and refactored the logic to used here to try and match the style of the awsmachinetemplate webhook. I also added some explicit awsmachine_webhook tests.

allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "cannot be modified"))
}
Expand Down Expand Up @@ -244,6 +258,16 @@ func (r *AWSMachine) Default() {

r.Spec.Ignition.Version = DefaultIgnitionVersion
}

// always encode the instance type and spot market options into instance details if unset
if len(r.Spec.InstanceDetails) == 0 {
r.Spec.InstanceDetails = []AWSInstanceDetails{
{
InstanceType: r.Spec.InstanceType,
SpotMarketOptions: r.Spec.SpotMarketOptions,
},
}
}
}

func (r *AWSMachine) validateAdditionalSecurityGroups() field.ErrorList {
Expand Down
40 changes: 40 additions & 0 deletions api/v1beta2/awsmachine_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,44 @@ func TestAWSMachineUpdate(t *testing.T) {
},
wantErr: true,
},
{
name: "changing the instance details to match the instance type is allowed",
oldMachine: &AWSMachine{
Spec: AWSMachineSpec{
InstanceType: "test",
},
},
newMachine: &AWSMachine{
Spec: AWSMachineSpec{
InstanceDetails: []AWSInstanceDetails{
{
InstanceType: "test",
},
},
InstanceType: "test",
},
},
wantErr: false,
},
{
name: "changing the instance details to a new type is not allowed",
oldMachine: &AWSMachine{
Spec: AWSMachineSpec{
InstanceType: "test",
},
},
newMachine: &AWSMachine{
Spec: AWSMachineSpec{
InstanceDetails: []AWSInstanceDetails{
{
InstanceType: "test2",
},
},
InstanceType: "test",
},
},
wantErr: true,
},
}
for _, tt := range tests {
ctx := context.TODO()
Expand All @@ -365,9 +403,11 @@ func TestAWSMachineUpdate(t *testing.T) {
GenerateName: "machine-",
Namespace: "default",
}

if err := testEnv.Create(ctx, machine); err != nil {
t.Errorf("failed to create machine: %v", err)
}

machine.Spec = tt.newMachine.Spec
if err := testEnv.Update(ctx, machine); (err != nil) != tt.wantErr {
t.Errorf("ValidateUpdate() error = %v, wantErr %v", err, tt.wantErr)
Expand Down
25 changes: 24 additions & 1 deletion api/v1beta2/awsmachinetemplate_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,12 @@ func (r *AWSMachineTemplateWebhook) SetupWebhookWithManager(mgr ctrl.Manager) er
type AWSMachineTemplateWebhook struct{}

// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta2-awsmachinetemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=awsmachinetemplates,versions=v1beta2,name=validation.awsmachinetemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta2-awsmachinetemplate,mutating=true,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachinetemplate,versions=v1beta2,name=mawsmachinetemplate.kb.io,name=mutation.awsmachinetemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1

var _ webhook.CustomValidator = &AWSMachineTemplateWebhook{}
var (
_ webhook.CustomValidator = &AWSMachineTemplateWebhook{}
_ webhook.Defaulter = &AWSMachineTemplate{}
)

func (r *AWSMachineTemplate) validateRootVolume() field.ErrorList {
var allErrs field.ErrorList
Expand Down Expand Up @@ -226,6 +230,12 @@ func (r *AWSMachineTemplateWebhook) ValidateUpdate(ctx context.Context, oldRaw r

var allErrs field.ErrorList

// allow limited changes to instanceDetails if we are defaulting and the field was missing
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like I saw this sentence already... :D

// because we know what the field should contain
if len(oldAWSMachineTemplate.Spec.Template.Spec.InstanceDetails) == 0 && len(newAWSMachineTemplate.Spec.Template.Spec.InstanceDetails) == 1 {
oldAWSMachineTemplate.Default()
}

if !topology.ShouldSkipImmutabilityChecks(req, newAWSMachineTemplate) && !cmp.Equal(newAWSMachineTemplate.Spec, oldAWSMachineTemplate.Spec) {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec"), newAWSMachineTemplate, "AWSMachineTemplate.Spec is immutable"))
}
Expand All @@ -237,3 +247,16 @@ func (r *AWSMachineTemplateWebhook) ValidateUpdate(ctx context.Context, oldRaw r
func (r *AWSMachineTemplateWebhook) ValidateDelete(_ context.Context, _ runtime.Object) error {
return nil
}

// Default implements webhook.Defaulter so that a mutating webhook will be registered for the type.
func (r *AWSMachineTemplate) Default() {
// always encode the instance type and spot market options into instance details if unset
if len(r.Spec.Template.Spec.InstanceDetails) == 0 {
r.Spec.Template.Spec.InstanceDetails = []AWSInstanceDetails{
{
InstanceType: r.Spec.Template.Spec.InstanceType,
SpotMarketOptions: r.Spec.Template.Spec.SpotMarketOptions,
},
}
}
}
31 changes: 30 additions & 1 deletion api/v1beta2/awsmachinetemplate_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,36 @@ func TestAWSMachineTemplateValidateUpdate(t *testing.T) {
},
wantError: true,
},
{
name: "allow setting instance details to the same instance types",
modifiedTemplate: &AWSMachineTemplate{
ObjectMeta: metav1.ObjectMeta{},
Spec: AWSMachineTemplateSpec{
Template: AWSMachineTemplateResource{
Spec: AWSMachineSpec{
InstanceType: "test",
InstanceDetails: []AWSInstanceDetails{{InstanceType: "test"}},
},
},
},
},
wantError: false,
},
{
name: "disallow setting instance details to the new instance types",
modifiedTemplate: &AWSMachineTemplate{
ObjectMeta: metav1.ObjectMeta{},
Spec: AWSMachineTemplateSpec{
Template: AWSMachineTemplateResource{
Spec: AWSMachineSpec{
InstanceType: "test",
InstanceDetails: []AWSInstanceDetails{{InstanceType: "test2"}},
},
},
},
},
wantError: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -128,7 +158,6 @@ func TestAWSMachineTemplateValidateUpdate(t *testing.T) {
Spec: AWSMachineTemplateSpec{
Template: AWSMachineTemplateResource{
Spec: AWSMachineSpec{
CloudInit: CloudInit{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh..? This isn't needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discovered #4116 working on this issue, and in fixing it, we don't need the CloudInit struct anymore, we removed the previous legacy code path for cloudinit in the awsmachinewebhook validateUpdate handler.

InstanceType: "test",
},
},
Expand Down
27 changes: 27 additions & 0 deletions api/v1beta2/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 29 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,35 @@ spec:
description: ImageLookupOrg is the AWS Organization ID to use for
image lookup if AMI is not set.
type: string
instanceDetails:
description: InstanceDetails is a list of instance details which are
used to create AWSMachines. When InstanceDetails is supplied, AWSMachines
are created using the instance type and spot options of the first
item in the list instead of using values provided in the spec. If
the instance request can not be satisfied due to a lack of capacity,
the next instance detail is used to create the ec2 instance.
items:
description: AWSInstanceDetails describes an instance details for
creating a AWSMachine.
properties:
instanceType:
description: 'InstanceType is the type of instance to create.
Example: m4.xlarge'
minLength: 2
type: string
spotMarketOptions:
description: SpotMarketOptions allows users to configure instances
to be run using AWS Spot instances.
properties:
maxPrice:
description: MaxPrice defines the maximum price the user
is willing to pay for Spot VM instances
type: string
type: object
required:
- instanceType
type: object
type: array
instanceID:
description: InstanceID is the EC2 instance ID for this machine.
type: string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,36 @@ spec:
description: ImageLookupOrg is the AWS Organization ID to
use for image lookup if AMI is not set.
type: string
instanceDetails:
description: InstanceDetails is a list of instance details
which are used to create AWSMachines. When InstanceDetails
is supplied, AWSMachines are created using the instance
type and spot options of the first item in the list instead
of using values provided in the spec. If the instance request
can not be satisfied due to a lack of capacity, the next
instance detail is used to create the ec2 instance.
items:
description: AWSInstanceDetails describes an instance details
for creating a AWSMachine.
properties:
instanceType:
description: 'InstanceType is the type of instance to
create. Example: m4.xlarge'
minLength: 2
type: string
spotMarketOptions:
description: SpotMarketOptions allows users to configure
instances to be run using AWS Spot instances.
properties:
maxPrice:
description: MaxPrice defines the maximum price
the user is willing to pay for Spot VM instances
type: string
type: object
required:
- instanceType
type: object
type: array
instanceID:
description: InstanceID is the EC2 instance ID for this machine.
type: string
Expand Down
21 changes: 21 additions & 0 deletions config/webhook/manifests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,27 @@ webhooks:
resources:
- awsmachines
sideEffects: None
- admissionReviewVersions:
- v1
- v1beta1
clientConfig:
service:
name: webhook-service
namespace: system
path: /mutate-infrastructure-cluster-x-k8s-io-v1beta2-awsmachinetemplate
failurePolicy: Fail
name: mutation.awsmachinetemplate.infrastructure.cluster.x-k8s.io
rules:
- apiGroups:
- infrastructure.cluster.x-k8s.io
apiVersions:
- v1beta2
operations:
- CREATE
- UPDATE
resources:
- awsmachinetemplate
sideEffects: None
- admissionReviewVersions:
- v1
- v1beta1
Expand Down
5 changes: 5 additions & 0 deletions controllers/awsmachine_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,11 @@ func (r *AWSMachineReconciler) Reconcile(ctx context.Context, req ctrl.Request)
return ctrl.Result{}, nil
}

// TODO: Remove this after v1beta2, this is needed for instanceDetails to be set if it was missing
// The defaulting must happen before `NewMachineScope` is called since otherwise we keep detecting
// differences that result in patch operations.
awsMachine.Default()

// Create the machine scope
machineScope, err := scope.NewMachineScope(scope.MachineScopeParams{
Client: r.Client,
Expand Down
Loading