-
Notifications
You must be signed in to change notification settings - Fork 610
✨ WIP: Initial dedicated hosts implementation #5504
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
base: main
Are you sure you want to change the base?
✨ WIP: Initial dedicated hosts implementation #5504
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Hey @faermanj - thanks for pinging. I have some questions about the API and test steps.
pkg/cloud/services/ec2/instances.go
Outdated
placementStr := input.Placement.GoString() | ||
s.scope.Warn("Placement already set for instance, overwriting with dedicated host placement", | ||
"hostId", i.HostID, | ||
"affinity", i.HostAffinity, | ||
"placement", placementStr) |
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.
What about this approach if you are using only to logging?
placementStr := input.Placement.GoString() | |
s.scope.Warn("Placement already set for instance, overwriting with dedicated host placement", | |
"hostId", i.HostID, | |
"affinity", i.HostAffinity, | |
"placement", placementStr) | |
s.scope.Warn("Placement already set for instance, overwriting with dedicated host placement", | |
"hostId", i.HostID, | |
"affinity", i.HostAffinity, | |
"placement", input.Placement.GoString()) |
api/v1beta2/awsmachine_types.go
Outdated
// Affinity specifies the dedicated host affinity setting for the instance. | ||
// When affinity is set to Host, an instance started onto a specific host always restarts on the same host if stopped. | ||
// +optional | ||
// +kubebuilder:validation:Enum:=Default;Host | ||
HostAffinity *string `json:"hostAffinity,omitempty"` |
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.
// Affinity specifies the dedicated host affinity setting for the instance. | |
// When affinity is set to Host, an instance started onto a specific host always restarts on the same host if stopped. | |
// +optional | |
// +kubebuilder:validation:Enum:=Default;Host | |
HostAffinity *string `json:"hostAffinity,omitempty"` | |
// HostAffinity specifies the dedicated host affinity setting for the instance. | |
// When hostAffinity is set to Host, an instance started onto a specific host always restarts on the same host if stopped. | |
// +optional | |
// +kubebuilder:validation:Enum:=Default;Host | |
HostAffinity *string `json:"hostAffinity,omitempty"` |
|
||
ginkgo.By("Creating cluster") | ||
clusterName := fmt.Sprintf("%s-%s", specName, util.RandomString(6)) | ||
vars := map[string]string{ |
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.
what about clusterVars
, or maybe add the map directly to the ClusterctlVariables
as it is used only there ?
api/v1beta2/types.go
Outdated
// HostID specifies the dedicated host on which the instance should be started | ||
// +optional | ||
HostID *string `json:"hostID,omitempty"` | ||
|
||
// Affinity specifies the dedicated host affinity setting for the instance. | ||
// +optional | ||
HostAffinity *string `json:"hostAffinity,omitempty"` |
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.
Do you need to set the defaults here too? IMHO it also would be nice to mention HostAffinity
depends on HostID
.
shared.ReleaseHost(e2eCtx, hostID) | ||
}() | ||
|
||
ginkgo.By("Creating cluster") |
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.
considering the feature is adding a field to the machineSpec, is it recommended to create a new cluster every time or you can use an existing generic cluster to create a new machine to validate if it has been provisioned to a Dedicated Host? (this is a general question, not sure if this is a recommended approach)
cc @richardcase @nrb
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.
In the EKS tests we reuse an existing cluster to help speed thing sup. In the non-EKS side (i.e. unmanaged suite) historically, new clusters were created every time. However, if we can speed up the e2e by just reusing an existing cluster, that would be great.
bc6c343
to
83f84c3
Compare
superseded by #5548 |
83f84c3
to
72ecf00
Compare
@faermanj: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/kind feature
What this PR does / why we need it:
Let users allocate machines to dedicated hosts.
Special notes for your reviewer:
Dedicated hosts must be pre-allocated.
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/dedicated-hosts-overview.html
Checklist:
Release note: