✨ Add support to specify dedicated host ID and Affinity #5113
Conversation
k8s-ci-robot
added
release-note
|
[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 |
k8s-ci-robot
added
needs-priority
cncf-cla: yes
muraee
changed the title
muraee
force-pushed
the
dedicted-host-id
branch
from
f09c548
to
544d83b
Compare
pkg/cloud/services/ec2/instances.go
Outdated
| } | ||
|
|
||
| if i.Tenancy == infrav1.TenancyHost && i.DedicatedHostPlacement != nil { | ||
| input.Placement.Affinity = i.DedicatedHostPlacement.Affinity |
There was a problem hiding this comment.
These fields are labeled as optional; should we double check that they aren't nil as well? Or perhaps add a webhook to validate them before we get to this stage.
There was a problem hiding this comment.
the AWS sdk accepts nil as input, so I don't think we need to validate them.
| @@ -188,7 +200,11 @@ type AWSMachineSpec struct { | |||
| // Tenancy indicates if instance should run on shared or single-tenant hardware. | |||
| // +optional | |||
| // +kubebuilder:validation:Enum:=default;dedicated;host | |||
| Tenancy string `json:"tenancy,omitempty"` | |||
| Tenancy Tenancy `json:"tenancy,omitempty"` | |||
There was a problem hiding this comment.
By reading this API is not clear to me which input should I set for tenancy host with autoplacement, can we document that here?
There was a problem hiding this comment.
added more documentation, let me know if it makes sense.
There was a problem hiding this comment.
thanks! "If host tenancy is used and auto-placement is disabled for the Dedicated Host, It is required to set '.spec.hostPlacment.hostID'" though the requirement is actually not enforced. Can we document what happens if not set in that case, e.g. it's required or no host would be picked. Can we also document all matrix combinations this API enables: If both hostID and autoplacement are set, the hostID would take precedence? If the hostID doesn't exist would it fallback to autoplacement?
There was a problem hiding this comment.
we can't enforce it, because we don't know if auto-placement is enabled or not on the dedicated Host, since its created directly by the user out of band.
muraee
force-pushed
the
dedicted-host-id
branch
from
544d83b
to
bcf63a2
Compare
| // HostPlacement denotes the placement settings for the instance when tenancy=host. | ||
| type HostPlacement struct { | ||
| // HostID pins the instance to a sepcific Dedicated Host. | ||
| // +optional |
There was a problem hiding this comment.
isn't a hostID required if I set HostPlacement?
There was a problem hiding this comment.
why? you might just want to set the affinity.
muraee
force-pushed
the
dedicted-host-id
branch
from
bcf63a2
to
5e0e470
Compare
| Tenancy Tenancy `json:"tenancy,omitempty"` | ||
|
|
||
| // HostPlacement denotes the placement settings for the instance when tenancy=host. | ||
| // +optional |
There was a problem hiding this comment.
should this be PlacementHost consistently with placementGroup and placementGroupPartition?
Can we please open ticket to capture putting all placements choices in a common struct for future api bump?
There was a problem hiding this comment.
how about just Placement then, so we can group all options inside it in the next api bump?
|
/test pull-cluster-api-provider-aws-e2e |
|
/retest-required |
|
/test pull-cluster-api-provider-aws-e2e |
|
@muraee: 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. |
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
This PR adds support to set placement settings for the instance when tenancy=host
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist:
Release note: