Skip to content

[WIP] SPLAT-2206: Added AWS dedicated host support #2484

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] SPLAT-2206: Added AWS dedicated host support #2484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions features/features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,8 +772,8 @@ var (
mustRegister()

FeatureGateAWSDedicatedHosts = newFeatureGate("AWSDedicatedHosts").
reportProblemsToJiraComponent("Installer").
contactPerson("faermanj").
reportProblemsToJiraComponent("splat").
contactPerson("rvanderp3").
productScope(ocpSpecific).
enhancementPR("https://github.com/openshift/enhancements/pull/1781").
enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).
Expand Down
35 changes: 35 additions & 0 deletions machine/v1beta1/types_awsprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,28 @@ type AWSMachineProviderConfig struct {
// If this value is selected, capacityReservationID must be specified to identify the target reservation.
// +optional
MarketType MarketType `json:"marketType,omitempty"`

// hostID specifies the Dedicated Host on which the instance must be started.
// This field is mutually exclusive with DynamicHostAllocation.
// When set, the value must be a valid AWS Dedicated Host ID in the form
// "h-" followed by 17 lowercase hexadecimal characters.
// The maximum length is 19 characters, and the field may be omitted.
// +kubebuilder:validation:XValidation:rule="self == null || self.matches('^h-[0-9a-f]{17}$')",message="hostID must start with 'h-' and end in 17 alphanumeric characters"
// +kubebuilder:validation:MaxLength=19
// +openshift:enable:FeatureGate=AWSDedicatedHosts
// +optional
HostID *string `json:"hostID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between setting this to "" and omitting the field entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be no difference. I would assume this field is not set if user not intending to place instances into a dedicated host.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no difference, this should not be a pointer and should have a minimum length of 1. This is probably what the linter is complaining about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is validated by Go based webhooks, and not openapi, the linter is wrong on this one.

If we make this not a pointer, then the Go code has no way to know if this was deliberately set to "" or not. We don't want "" to be valid, so this needs to be a pointer so that we can check that.

In this case (and future cases like this in these providerspec APIs) we will want to make exceptions to the serialization rules on the linter.

We may want to even disable the serialization rules on these particular APIs somehow 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I went into standard API review mode here and forgot this API is webhook validation 🤦

Thanks for catching that!

We may want to even disable the serialization rules on these particular APIs somehow

Can we do this via codegen configurations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do this via codegen configurations?

No, but we should be able to disable using the .golangci-lint.yaml config, ideally we could have a different config for the APIs that act like this, these MAPI ones aren't the only ones (e.g. the aggregated APIs we support too)

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add a min length here as well so that an empty string ("") isn't a valid value. Linter will complain about this because it isn't aware of the machine API nuance, but we can override that.

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 can add that. Thanks!


// hostAffinity specifies the dedicated host affinity setting for the instance.
// Valid values are "AnyAvailable", "Host", and omitted.
// When HostAffinity is set to "Host", an instance started onto a specific host always restarts on the same host if stopped.
// When HostAffinity is set to "AnyAvailable", and you stop and restart the instance, it can be restarted on any available host.
// When HostAffinity is omitted and HostID is defined, the instance is started onto the specified host.
// When HostAffinity is defined, HostID is required.
Copy link
Contributor

Choose a reason for hiding this comment

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

Whether it is set to host or default, hostID will be required?

Also, when referring to a field use the serialized form of the field name as that is what end-users would be familiar with:

Suggested change
// When HostAffinity is defined, HostID is required.
// When HostAffinity is defined, hostID is required.

Copy link
Contributor Author

@vr4manta vr4manta Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, if hostID is set, this field will be could required. The idea was that if they do not set this field when hostID is set, it will default to host behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, hostID is required when hostAffinity is set to host and should be forbidden otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I'll make sure this godoc states this. I have the webhook logic doing this already.

// +kubebuilder:validation:MaxLength=64
// +openshift:enable:FeatureGate=AWSDedicatedHosts
// +optional
HostAffinity *HostAffinity `json:"hostAffinity,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if I don't supply this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will default to host. I have been playing with trying to make linter happy with the configs today (which I put this into WIP). I will update the godoc to reflect this default behavior shortly.

Copy link
Contributor

Choose a reason for hiding this comment

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

So are the options here actually omitted (i.e don't set hostAffinity and hostID to use default AWS affinity logic) and Host (i.e I want it to use be assigned to this host)?

Copy link
Contributor Author

@vr4manta vr4manta Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So I am discussing this with @rvanderp3 , "Default" we are gonna change to "AnyAvailable". I believe with the AWS API, the idea is this field dictates whether hostID is looked at or not. We are gonna confirm this and see what the behavior should be to clear up any confusions.

}

// BlockDeviceMappingSpec describes a block device mapping
Expand Down Expand Up @@ -355,3 +377,16 @@ const (
// When set to CapacityBlock the instance utilizes pre-purchased compute capacity (capacity blocks) with AWS Capacity Reservations.
MarketTypeCapacityBlock MarketType = "CapacityBlock"
)

// HostAffinity describes the host affinity of an EC2 Instance
type HostAffinity string

const (
// HostAffinityAnyAvailable is a HostAffinity enum value
// When set to AnyAvailable the instance is put on a host using the AWS default logic.
HostAffinityAnyAvailable HostAffinity = "AnyAvailable"

// HostAffinityHost is a HostAffinity enum value
// When set to host the instance runs on the specified host.
HostAffinityHost HostAffinity = "Host"
)
10 changes: 10 additions & 0 deletions machine/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

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

2 changes: 2 additions & 0 deletions machine/v1beta1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

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

14 changes: 14 additions & 0 deletions openapi/generated_openapi/zz_generated.openapi.go
View file
Open in desktop

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

8 changes: 8 additions & 0 deletions openapi/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21970,6 +21970,14 @@
"format": "int64",
"default": 0
},
"hostAffinity": {
"description": "hostAffinity specifies the dedicated host affinity setting for the instance. Valid values are \"AnyAvailable\", \"Host\", and omitted. When HostAffinity is set to \"Host\", an instance started onto a specific host always restarts on the same host if stopped. When HostAffinity is set to \"AnyAvailable\", and you stop and restart the instance, it can be restarted on any available host. When HostAffinity is omitted and HostID is defined, the instance is started onto the specified host. When HostAffinity is defined, HostID is required.",
"type": "string"
},
"hostID": {
"description": "hostID specifies the Dedicated Host on which the instance must be started. This field is mutually exclusive with DynamicHostAllocation. When set, the value must be a valid AWS Dedicated Host ID in the form \"h-\" followed by 17 lowercase hexadecimal characters. The maximum length is 19 characters, and the field may be omitted.",
"type": "string"
},
"iamInstanceProfile": {
"description": "iamInstanceProfile is a reference to an IAM role to assign to the instance",
"$ref": "#/definitions/com.github.openshift.api.machine.v1beta1.AWSResourceReference"
Expand Down