-
Notifications
You must be signed in to change notification settings - Fork 629
✨ Initial dedicated hosts implementation #5548
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
✨ Initial dedicated hosts implementation #5548
Conversation
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.
Looks good, a few questions related to the API validation.
00db5ca
to
cdafa87
Compare
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.
/lgtm
cc @nrb @richardcase
LGTM label has been added. Git tree hash: eeb699a9acb60c59cfb1b6da396ab9b431428e5d
|
/test ? |
@mtulio: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
/test pull-cluster-api-provider-aws-e2e |
/test pull-cluster-api-provider-aws-e2e-eks |
I manually canceled the e2e I triggered as e2e is still under investigation! |
/hold |
cdafa87
to
c2cee83
Compare
/hold cancel |
4c1c1b5
to
1bcfdf7
Compare
1bcfdf7
to
2ea5ce8
Compare
changes implemented due to underlying function signature changes. placing hold while testing. |
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.
LGTM 🚀 (Once e2e are passing)
/test ? |
@richardcase: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
/test pull-cluster-api-provider-aws-e2e-eks |
Looks good from my side, however, until the aws-janitor change merges lets hold this: /hold Also lets check a larger set of e2e's |
@rvanderp3 - looks like we will need to initially have the e2e test for the dedicated hosts disabled. I'll message you to discuss. |
/hold cancel testing looks good on my side |
Co-authored-by: Richard Case <[email protected]>
ad156e5
to
d9d72f7
Compare
@rvanderp3: 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. |
/retest-required |
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.
/lgtm
LGTM label has been added. Git tree hash: 4a9d5bce713bd76208f054efb69d01026b413323
|
/assign @richardcase |
Thanks for this @rvanderp3 . /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richardcase The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
// When hostAffinity is set to default, and you stop and restart the instance, it can be restarted on any available host. | ||
// When HostAffinity is defined, HostID is required. | ||
// +optional | ||
// +kubebuilder:validation:Enum:=default;host |
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, these should be PascalCase to be consistent with K8s conventions
// When HostAffinity is defined, HostID is required. | ||
// +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.
No need for this to be a pointer as the zero value is not a valid choice
|
||
// HostID specifies the Dedicated Host on which the instance must be started. | ||
// +optional | ||
HostID *string `json:"hostID,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.
Ideally this would set both minimum and maximum length constraints, also, is there a pattern that is valid for this?
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.
good callouts @JoelSpeed , will follow up with another PR. there is a pattern we could check for validation
This commit implements dynamic dedicated host allocation capability to support bare metal workloads like OpenShift Virtualization with HyperShift on ROSA HCP, particularly for BYOL Microsoft Windows workloads. Key changes: - Add DynamicHostAllocationSpec API with instance family, type, quantity, AZ, and tagging support - Implement dedicated host service with allocation, release, and validation operations - Integrate dynamic allocation into machine lifecycle with automatic cleanup - Add comprehensive webhook validation for configuration consistency - Include unit tests and usage examples - Generate updated CRDs with new API fields The implementation follows existing CAPA patterns from PR kubernetes-sigs#5548 and provides automatic provisioning and management of AWS dedicated hosts based on instance requirements. Related: kubernetes-sigs#5548
/kind feature
What this PR does / why we need it:
Enable machines to be provisioned on to dedicated hosts.
Special notes for your reviewer:
Resumes work started in cluster-api-provider-aws#5504.
Checklist:
Release note: